Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: bump lookahead to 200 #506

Merged
merged 3 commits into from
Nov 22, 2020
Merged

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Aug 31, 2020

Closes #407
...but in a lazy way that more kicks the can down the road than anything.
See this comment specifically for details.

TODO:

  • run benchmarks.
  • fix tests (generate 200 addresses from bitcoin core and maybe verify with one other implementation for test vectors)

Update:

This was lowered from 1000 to 200 because in the serialized account metadata, the lookahead value is only a single byte:

bw.writeU8(this.lookahead);

benchmarks:

master branch

account: ops=1000, time=3.276540621, rate=305.19994

PR branch

account: ops=1000, time=46.609575762, rate=21.45482

No big surprise there - creating an account makes generating 20x more keys (380 total because of receive & change).
So we've increased account creation time from ~3ms to ~46ms

account.lookahead is saved to the database as a uint8 so values
larger than 255 will overflow anyway. BIP44 only recommends a gap
limit of 20 anyway so this should be plenty.
@pinheadmz pinheadmz changed the title wallet: bump lookahead to 1000 wallet: bump lookahead to 200 Sep 7, 2020
@pinheadmz pinheadmz marked this pull request as ready for review September 7, 2020 18:24
@coveralls
Copy link

coveralls commented Sep 7, 2020

Pull Request Test Coverage Report for Build 243442873

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 59.218%

Files with Coverage Reduction New Missed Lines %
lib/net/pool.js 1 32.63%
lib/net/peer.js 2 35.28%
Totals Coverage Status
Change from base Build 238010293: -0.01%
Covered Lines: 19385
Relevant Lines: 30471

💛 - Coveralls

@pinheadmz pinheadmz requested review from chjj and tynes September 7, 2020 18:28
@tynes
Copy link
Contributor

tynes commented Sep 9, 2020

It's nice to see the benchmark for account creation. I'm curious about how it would impact the performance of a rescan. If it's using a bloom filter then it should just be building the bloom filter that takes longer.

As performance becomes an issue, maybe https://github.com/chjj/bthreads/ can be used.

@pinheadmz
Copy link
Member Author

Interesting idea to use threads to generate keys in parallel. I know walletDB has workers but I'm not sure if they're used in rescanning. Off the top of my head, that seems like the kind of thing that must be done serially because of the UTXO system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wallet restore misses transactions due to dense transaction activity: (lookahead / BIP44 gap limit)
4 participants