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: --check-lookahead to update old wallets #569

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Mar 3, 2021

Closes #563

This probably should have been included in #506

This PR adds an extra wallet option to upgrade wallets created before #506 was merged, that still have a lookahead of 10. When the option is set, all accounts of all wallets are called with Account.setLookahead(Account.MAX_LOOKAHEAD ) which is now 200. This will set the metadata lookahead value, generate whatever keys/addresses are necessary and write to the database. This only needs to be run once per walletDB, since the update is permanent.

Usage:

hsd --wallet-check-lookahead

Log messages:

[warning] (wallet) Setting lookahead for wallet primary account 0
[warning] (wallet) Setting lookahead for wallet ten account 0

Can be verified by this bash test script: https://gist.github.com/pinheadmz/029bb4ff773b548cf632a5c300e4c17b

output:

HEAD is now at 7ca1b71ff v2.1.0
79552

hsd v2.1.0 - initialize
wallet lookahead:
10
wallet getaddressesbyaccount length:
22

Stopping.
Previous HEAD position was 7ca1b71ff v2.1.0
Switched to branch 'synclookahead1'
79564

hsd v2.3.0 #synclookahead1 - open old wallet
wallet lookahead:
10
wallet getaddressesbyaccount length:
22

Stopping.
79575

hsd v2.3.0 #synclookahead1 - check lookahead option set
wallet lookahead:
200
wallet getaddressesbyaccount length:
402

Stopping.
79585

hsd v2.3.0 #synclookahead1 - no extra option set
wallet lookahead:
200
wallet getaddressesbyaccount length:
402

@coveralls
Copy link

coveralls commented Mar 3, 2021

Pull Request Test Coverage Report for Build 618271193

  • 7 of 17 (41.18%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.003%) to 59.408%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/wallet/walletdb.js 7 17 41.18%
Files with Coverage Reduction New Missed Lines %
lib/protocol/consensus.js 1 82.79%
Totals Coverage Status
Change from base Build 615190829: -0.003%
Covered Lines: 19558
Relevant Lines: 30655

💛 - Coveralls

@pinheadmz pinheadmz added this to the v2.4.0 milestone Mar 13, 2021
@pinheadmz
Copy link
Member Author

p.s there was a lot of debate about user-triggered wallet migration here: p.s. there was a lot of debate on user-triggered wallet migration here: #415

That's why I chose this route, setting an optional command line option

@nodech
Copy link
Contributor

nodech commented Mar 15, 2021

Thanks for linking #415 to the PR, it helped a lot.

I have questions related to previous discussion mostly:

  • Is there reason why this was not written as migration ?

To me this change is similar to migrateChange, fixes the
existing database state to something we desire. One big difference
being: migrate-change was critical bug, where this is just recommended setting.

This works fine as well, it's just is it becuase this is not critical, does not
warrant it to be migration?
We could reuse migration=1 in that case.

Also migration is kinda misleading for the previous change to me. It is mostly
fix database state, not change database layout that would make it incompatible.


I guess this is note about previous PR: #506
Why not have Account.MAX_LOOKAHEAD = 255 and leave default to 200.
(This would make the value more obvious, maybe with comment?)
I thought 200 was just choses arbitrarily until looked at #506 and saw
that we store it in U8.


This works fine though.
LGTM.

@pinheadmz pinheadmz merged commit a69780e into handshake-org:master Mar 15, 2021
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 should migrate to new lookahead value
3 participants