Page MenuHomePhabricator
Differential D14213

[electrum] increase the max number of get_merkle requests in the network queue
ClosedPublic
Actions

Authored by PiRK on Jul 5 2023, 14:58.

Details

Reviewers
Fabien
bytesofman
Group Reviewers
Restricted Project
Commits
rABC405daef21b36: [electrum] increase the max number of get_merkle requests in the network queue
Summary

When first loading a wallet, the application first gets all the transactions for all monitored addresses from servers, then asks for the merkle branch for that transaction to verify it.

Currently this fetching of merkle branches is the only network call that is limited by the number of requests in the network queue. For a wallet with a large transaction history, this is the main bottleneck when doing the initial synchronization of the wallet. For the IFP address, fetching all the 60000+ transactions took about 5 minutes while the merkle verification took about one hour.

I tested locally with a queue limit of 10000 merkle requests without crashing any server and without getting banned, and managed to make the merkle verification take under 5 minutes. Therefore, I think bumping this limit to 100 is relatively safe for the network.

Note that there is only one callsite for get_merkle_for_transaction, and it does not specify the max_qlen parameter, so remove the parameter from that function.

Test Plan

Create a new wallet using "Import eCash address", paste the IFP address, wait for all transactions to be downloaded, then watch how fast the number of unverified transactions in the status bar decreases (10 times faster with this diff)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
get_merkle_queue_length
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24378
Build 48365: Build Diffelectrum-tests
Build 48364: arc lint + arc unit

Event Timeline

PiRK created this revision.Jul 5 2023, 14:58
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 5 2023, 14:58
PiRK requested review of this revision.Jul 5 2023, 14:58
Harbormaster completed remote builds in B24359: Diff 41263.Jul 5 2023, 14:58
PiRK retitled this revision from [electrum] increase the max number of blockchain.transaction.get_merkle requests in the network queue to [electrum] increase the max number of get_merkle requests in the network queue.Jul 5 2023, 15:00
bytesofman added a subscriber: bytesofman.Jul 5 2023, 22:59

100 does seem like an improvement from the current setting, which also seems to have been just picked at random.

If 1000 worked, why not 1000, or 10,000?

I guess we can't really know if or when 100 users all start to sync a big wallet at the same time. What is the the failure mode if the param is set too high?

PiRK added a comment.EditedJul 6 2023, 05:55
In D14213#317755, @bytesofman wrote:

100 does seem like an improvement from the current setting, which also seems to have been just picked at random.

If 1000 worked, why not 1000, or 10,000?

I guess we can't really know if or when 100 users all start to sync a big wallet at the same time. What is the the failure mode if the param is set too high?

I'm not really sure about the failure modes, that's why I was going for a relatively modest improvement that we can potentially raise further in the future if it turns out there is no issue.

The verification of merkle roots can be CPU intensive, and I'm running these tests on a performant computer (AMD Ryzen 9 5900X 12-Core), but I doubt that you can max out a CPU with network input even on a more modest computer.

I have just tested a max_qlen of 10000 , and it still seems to work fine (no additional GUI sluggishness, the number of unverified tx seems to drop at the expected rate)

I will investigate more, but 100 seems a first safe step to me, from my personal tests. I don't expect many users doing frequent initial syncs of 60000 tx wallets.

PiRK added a comment.Jul 6 2023, 05:58

Not sure the number can be compared, but just for reference the server-side default max number of address subscriptions for a single IP is 75000.

PiRK updated this revision to Diff 41285.Jul 6 2023, 08:04

rebase to trigger the CI tests

Harbormaster completed remote builds in B24378: Diff 41285.Jul 6 2023, 08:05
teamcity added a comment.Jul 6 2023, 08:05
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
PiRK added a comment.Jul 6 2023, 10:40

I got some info from Calin about this on the Electron Cash telegram channel

Screenshot from 2023-07-06 12-39-12.png (1×488 px, 308 KB)

PiRK planned changes to this revision.Jul 6 2023, 10:54

I accidentaly included the limit bumping to 10000, this was for testing only.

PiRK updated this revision to Diff 41286.Jul 6 2023, 11:03

restore to a limit of 100 requests and properly rebase on latest master (with setup script fix)

Harbormaster completed remote builds in B24379: Diff 41286.Jul 6 2023, 11:04
teamcity added a comment.Jul 6 2023, 11:04
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
PiRK updated this revision to Diff 41287.Jul 6 2023, 12:49

another attempts at rebasing. Sorry for the noise.

Harbormaster completed remote builds in B24380: Diff 41287.Jul 6 2023, 12:51
Fabien requested changes to this revision.Jul 6 2023, 14:15
Fabien added a subscriber: Fabien.
Fabien added inline comments.
electrum/electrumabc/network.py
2331 ↗(On Diff #41287)

Make it a constant with a proper comment rather than using a magic number that nobody will remember where it cames from in 6 months

This revision now requires changes to proceed.Jul 6 2023, 14:15
PiRK updated this revision to Diff 41290.Jul 6 2023, 15:02

use a constant and comment it

Harbormaster completed remote builds in B24383: Diff 41290.Jul 6 2023, 15:04
bytesofman accepted this revision.Jul 6 2023, 15:37
Fabien requested changes to this revision.Jul 6 2023, 16:42
Fabien added inline comments.
electrum/electrumabc/network.py
61 ↗(On Diff #41290)

This whole comment is good except for 1 thing: it doesn't tell what this does! It rate limits some requests made to the fulcrum server so it doesn't get overloaded

This revision now requires changes to proceed.Jul 6 2023, 16:42
PiRK updated this revision to Diff 41292.Jul 6 2023, 16:55

improve comment

Harbormaster completed remote builds in B24385: Diff 41292.Jul 6 2023, 16:57
PiRK edited the summary of this revision. (Show Details)Jul 6 2023, 16:57
Fabien accepted this revision.Jul 6 2023, 17:30
This revision is now accepted and ready to land.Jul 6 2023, 17:30
Closed by commit rABC405daef21b36: [electrum] increase the max number of get_merkle requests in the network queue (authored by PiRK). · Explain WhyJul 7 2023, 05:56
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC405daef21b36: [electrum] increase the max number of get_merkle requests in the network queue.

Revision Contents
Changeset List

PathSize
electrum/
electrumabc/
4 lines

Diff 41285

View Options

electrum/electrumabc/network.py

Loading...