wallet: add migration to regenerate missing change addresses (WIP) #415

chjj · 2020-03-31T14:37:01Z

lib/wallet/walletdb.js

chjj · 2020-05-26T12:33:38Z

Rebased and addressed nits.

codecov-commenter · 2020-05-26T12:37:15Z

Codecov Report

Merging #415 into master will increase coverage by 0.08%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   62.53%   62.62%   +0.08%     
==========================================
  Files         129      129              
  Lines       34866    34913      +47     
  Branches     5926     5933       +7     
==========================================
+ Hits        21805    21864      +59     
+ Misses      13061    13049      -12

Impacted Files	Coverage Δ
lib/wallet/layout.js	`100.00% <ø> (ø)`
lib/wallet/plugin.js	`100.00% <ø> (ø)`
lib/wallet/walletdb.js	`79.34% <89.65%> (+0.43%)`	⬆️
lib/wallet/wallet.js	`74.19% <100.00%> (+0.24%)`	⬆️
lib/protocol/consensus.js	`88.77% <0.00%> (-1.03%)`	⬇️
lib/net/hostlist.js	`50.29% <0.00%> (-0.36%)`	⬇️
lib/covenants/rules.js	`73.04% <0.00%> (-0.15%)`	⬇️
lib/script/script.js	`59.32% <0.00%> (-0.09%)`	⬇️
lib/wallet/txdb.js	`85.11% <0.00%> (+0.06%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3049d5...c25799d. Read the comment docs.

pinheadmz

Sweet thanks for the rebase. Tested with #417 can be merged together or separately...

ACK c4a2e18

Show Signature

pinheadmz's public key is on keybase

lib/wallet/walletdb.js

pinheadmz · 2020-07-22T19:05:48Z

rebase to 1351d80:

rebase on current master
include test from Change migration test #417
replace automatic wallet rescan with a big ugly red log message. The motivation for this is pruned nodes, and potentially SPV nodes as well (Missing transactions during block scan/rescan from a BIP44 seed for a SPV Wallet and Full Wallet Node bcoin-org/bcoin#835). In the developer meeting it was agreed that hsd shouldn't ever force a rescan, it is unexpected behavior and for a node used in production for an active business it's not great.

log message can be tested by adding this diff to the test:

diff --git a/test/wallet-change-test.js b/test/wallet-change-test.js
index 39376570..39d142d0 100644
--- a/test/wallet-change-test.js
+++ b/test/wallet-change-test.js
@@ -19,7 +19,9 @@ const node = new FullNode({
   prefix: path,
   memory: false,
   network: 'regtest',
-  plugins: [require('../lib/wallet/plugin')]
+  plugins: [require('../lib/wallet/plugin')],
+  logConsole: true,
+  logLevel: 'debug'
 });

 const {wdb} = node.require('walletdb');

pinheadmz · 2020-07-22T19:11:02Z

@turbomaze

pinheadmz · 2020-08-03T16:20:29Z

New thoughts on this, after testing #480 and having a hard time getting it to work.

Here's what I think the flow could be for this PR:

Upgraded hsd checks for walletDB version on launch, if it is not updated, quit the process with an error and instructions to back up wallet db and then...
To upgrade hsd, launch with extra option --migrate-wallet-to-0 or something in addition to all other params you normally launch with

This way, all the migration logic can stay in hsd, no external scripts. The user adds the option to all their normal options whatever they are, including prefix and network and all that, so the migration process already has all the data it needs (this is an issue passing all that to the migration script). The user can back up their walletDB any way they want to (we will have to remove the backup logic form this PR as well) and the if the user decides NOT to migrate they still have the option to revert hsd to the last version and relaunch before anything has happened to their data.

Thoughts @tynes @turbomaze ?

pinheadmz · 2020-08-07T19:29:09Z

Rebase to c25799d :

New flow works like this:

User upgrades to this version of hsd
Every time new version of hsd is run, check DB migration id
- if migration ID is present, continue with normal operation
- if ID is NOT present, scan for missing change addresses:
  - This may take some time (a warning is needed in changelog)
  - The database is not altered in anyway
  - If all addresses are present, DB migration ID is saved, continue with normal operation
  - if addresses are missing, quit and throw an error:

Affected user starts again, this time adding --wallet-migrate=0 to their launch.
- above sequence is run again
  - this time, if addresses are missing they are regenerated and saved
  - wallet automatically initiates a rescan

Because a rescan is required, a standalone wallet user will have to ensure that the wallet node is connected to a hsd full node

Test script: https://gist.github.com/pinheadmz/25fcd11ec8ba3916830ebc01d888bd7b
This script checks out the old version of hsd, manually creates change addresses and dumps a wallet backup. Then it checks out this branch, runs the migration flag and dumps the wallet again.

pinheadmz · 2020-08-07T19:37:42Z

@tynes @turbomaze if you guys are cool with this new scheme I'll spell it all out in the changelog in the next commit,

lib/wallet/walletdb.js

turbomaze · 2020-08-11T04:46:53Z

@tynes @turbomaze if you guys are cool with this new scheme I'll spell it all out in the changelog in the next commit,

Looks great!

#413, #411.

Users should be informed to backup manually before running migration.

pinheadmz · 2020-08-11T15:39:42Z

rebase to 1789576:
rebase on master

coveralls · 2020-08-11T15:44:26Z

Pull Request Test Coverage Report for Build 204277284

42 of 45 (93.33%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 59.124%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/wallet/walletdb.js	26	29	89.66%

Totals
Change from base Build 204174244:	0.1%
Covered Lines:	19348
Relevant Lines:	30451

💛 - Coveralls

pinheadmz · 2020-08-11T16:36:50Z

Ok added a message to the changelog - I think this is done. @chjj @tynes @turbomaze looking for final ACKs and then I say we merge and release hsd.

pinheadmz reviewed Mar 31, 2020

View reviewed changes

lib/wallet/walletdb.js Outdated Show resolved Hide resolved

pinheadmz added this to the 2.1.6 milestone May 6, 2020

chjj force-pushed the change-migration branch from 76b4eac to c4a2e18 Compare May 26, 2020 12:33

pinheadmz mentioned this pull request May 26, 2020

Change migration test #417

Closed

pinheadmz approved these changes May 26, 2020

View reviewed changes

pinheadmz reviewed May 28, 2020

View reviewed changes

lib/wallet/walletdb.js Show resolved Hide resolved

pinheadmz force-pushed the change-migration branch from c4a2e18 to 1351d80 Compare July 22, 2020 19:01

pinheadmz requested a review from tynes July 22, 2020 19:10

pinheadmz force-pushed the change-migration branch from aa024a5 to 223c481 Compare August 7, 2020 19:12

tynes requested changes Aug 7, 2020

View reviewed changes

lib/wallet/walletdb.js Show resolved Hide resolved

lib/wallet/walletdb.js Show resolved Hide resolved

pinheadmz mentioned this pull request Aug 10, 2020

walletdb: change migration script #480

Closed

chjj and others added 7 commits August 11, 2020 11:39

wallet: add migration to regenerate missing change addresses. see #414,

2a99082

#413, #411.

test: cover create change fix and wallet migration

e771f02

wallet: do not automatically backup wallet during migrate change

492507a

Users should be informed to backup manually before running migration.

wallet: migrate change and rescan using config option

a1dfea5

wallet: save migration state if there is no corruption

ae7e483

wallet: test migration flag

9de09e1

wallet: clean up migrateChange()

1789576

pinheadmz force-pushed the change-migration branch from 81308f8 to 1789576 Compare August 11, 2020 15:39

pkg: explain wallet repair flag in changelog

3a77b68

pinheadmz mentioned this pull request Aug 13, 2020

Start with migrate change flag kyokan/bob-wallet#200

Closed

pinheadmz merged commit a3ae4c1 into master Aug 17, 2020

This was referenced Jan 27, 2021

Migration path for wallets affected by the change address bug #414

Closed

Wallet does not recognize its own change address #411

Closed

pinheadmz mentioned this pull request Mar 13, 2021

wallet: --check-lookahead to update old wallets #569

Merged

pinheadmz deleted the change-migration branch June 11, 2022 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: add migration to regenerate missing change addresses (WIP) #415

wallet: add migration to regenerate missing change addresses (WIP) #415

chjj commented Mar 31, 2020

chjj commented May 26, 2020

codecov-commenter commented May 26, 2020 •

edited

Loading

pinheadmz left a comment

pinheadmz commented Jul 22, 2020

pinheadmz commented Jul 22, 2020

pinheadmz commented Aug 3, 2020

pinheadmz commented Aug 7, 2020

pinheadmz commented Aug 7, 2020

turbomaze commented Aug 11, 2020

pinheadmz commented Aug 11, 2020

coveralls commented Aug 11, 2020 •

edited

Loading

pinheadmz commented Aug 11, 2020

wallet: add migration to regenerate missing change addresses (WIP) #415

wallet: add migration to regenerate missing change addresses (WIP) #415

Conversation

chjj commented Mar 31, 2020

chjj commented May 26, 2020

codecov-commenter commented May 26, 2020 • edited Loading

Codecov Report

pinheadmz left a comment

Choose a reason for hiding this comment

pinheadmz commented Jul 22, 2020

pinheadmz commented Jul 22, 2020

pinheadmz commented Aug 3, 2020

pinheadmz commented Aug 7, 2020

pinheadmz commented Aug 7, 2020

turbomaze commented Aug 11, 2020

pinheadmz commented Aug 11, 2020

coveralls commented Aug 11, 2020 • edited Loading

Pull Request Test Coverage Report for Build 204277284

💛 - Coveralls

pinheadmz commented Aug 11, 2020

codecov-commenter commented May 26, 2020 •

edited

Loading

coveralls commented Aug 11, 2020 •

edited

Loading