wallet: save and recover undo data from critical namestate transitions #564
Conversation
Pull Request Test Coverage Report for Build 614562008
💛 - Coveralls |
|
Requesting reviews from: @brandondees @chikeichan @turbomaze @HDardenne |
|
i am hoping to get that 100 hns, |
80ecfdf to
5013fff
Compare
|
This fixes the assertion error, but I'm now wondering what other actions are potentially affected by this bug. There could be some edge cases where updates/renewals/etc. are not rolled back properly. Granted, it's for a name you probably don't particularly care about (since you don't own it), but there may be other implications down the line. To be safe we could consider updating the block record/map for damn near everything. Though, that might get funky too. We should merge/release this but also continue thinking about potential edge cases here. |
Modifies `txdb` during TX confirm and unconfirm: If a TX updates namestate in a critical way, save it. Even if it does not affect wallet balance or TX history. During rollback, revert, reorg, or rescan -- look for the name undo data even if the TX is not in wallet history, and apply.
5013fff to
0f4f2f3
Compare
|
@rozaydin thank you for reviewing! I sent you 300 HNS |
|
tests passing here, looking at the rest now |
|
@chjj any idea of what exactly might get funky with tracking everything? seems like the sensible and safe thing to do from my limited view |
| assert.strictEqual(registerBlocks[0].txs.length, 2); | ||
| }); | ||
|
|
||
| it('should get namestate', async () => { |
There was a problem hiding this comment.
it looks like these test blocks are sequentially order dependent, right? this might should be spelled out somewhere up top or at the repo/contributing.md level since it's a general best practice to write test cases that are strictly independent and randomize the order.
There was a problem hiding this comment.
thats true, we should mention that in contirbuting. In this case I am using separate describe blocks for overall test cases, and it blocks to verify incrementally. Its a standard we frequently break in this repo.
There was a problem hiding this comment.
cool, well then yeah just something newcomers should be able to avoid tripping over by viewing the key docs
e7cfd30 to
0f4f2f3
Compare
| // Hash of the FINALIZE transaction | ||
| let aliceFinalizeHash; | ||
|
|
||
| async function mineBlocks(n, addr) { |
There was a problem hiding this comment.
i feel like all this setup procedure stuff could be a lot more DRY just by moving it up a scope. this could become cumbersome if some of the internal apis change and a lot of test code has to be changed.
There was a problem hiding this comment.
or if it's special per context it should probably be highlighted what exactly is special in each place
There was a problem hiding this comment.
true, lots of test modules have a mineBlocks function. They are usually slightly different in each case but we could probably add this to a util file in a future PR. It would need to accept a chain and miner as parameters though, which are usually local to each test. In this case I need the function to return the blocks, but thats not always the case in other test that mine blocks.
There was a problem hiding this comment.
gotcha, yeah i could see building up a small library of these sorts of things to raise the level of abstraction for various bob/alice scenarios but you'd need to find the common patterns. obvs all tangential to this branch, just something that sticks out to me as worth improving.
|
Read through all the changes, all looks good to me overall, overlooking style preference stuff. I think the test cases are probably adequate but could maybe be expanded to prove that balances aren't impacted too, if that's a potential side effect. |
|
tested import and rescan in this branch in Bob. My wallet used to run into rescan loop on second rescan after first import. Second and subsequent rescans now can perform without issues on using this PR. |
4bbf02f to
fcf716d
Compare
Closes #510
This PR fixes a wallet issue that throws an assertion error during a rescan in a shockingly simple scenario:
assertion error at TXDB.connectNamesBackground
There are other related scenarios where Alice doesn't send the OPEN but does bid, and still loses. The crux of the issue has to do with UNDO DATA. Blockchain is generally designed to be append-only and therefore only move forwards. However there are scenarios in which the chain is rewound: a reorganization or a wallet rescan. In these scenarios, the software depends on UNDO data, which are essentially snapshots of state just before a transition happens. To move backwards, the undo data is retrieved and applied. Essentially for every new block and every wallet transaction, a "negative" is stored in the database and used to reverse whatever state update the action caused initially.
The Handshake wallet is a mysterious beast because a lot of its state depends on the actions of OTHER USERS, like tracking bids and determining who won an auction. Bids and Reveals are stored in special buckets in the database so that they can be reverted even if they don't actually directly affect the wallet's balance.
TRANSFER, FINALIZE, and REVOKE transactions up until now are not stored in special buckets in the database. In fact, they are not stored at all unless they directly affect the wallet's balance. That's what this PR changes. We still don't need to store the entire TX but we do need to store the UNDO data, and we need to remember to retrieve it if one of the above "rewind" scenarios is triggered.
With all this background I can now explain the bug at the lowest level: When the wallet rescans, it starts by reverting / undoing / unconfirming all the transactions in every block from chain tip backwards to the rescan target (provided as an argument by the user, default
0aka genesis block). Before this PR, the wallet DID NOT revert transactions like TRANSFERs unless they involved the wallet directly. Because of this, when the wallet rescans the chain it encounters the TRANSFER a second time, but throws an error because it thinks the name is still in a transfer state (ns.transfer !== 0)To test / fix your own wallet
If you've encountered this bug, you can test this PR by attempting to fix your own wallet:
Bonus
Because this PR is urgent and likely to get merged and released quickly, I attached a rider ;-)
I cherry picked 4b75f8a from #499 which bypasses a too-strict length requirement in
Addresslength. (see also bcoin-org/bcoin#989) Without this fix, assertion errors are still thrown during rescan for wallets (like mine) that have ever sent to a nulldata address. I had to add this bug fix to test this PR against my own wallet.TODO
Just by eyeballing the code, I assume that this bug will have the same effect if Bob (see above) sends a FINALIZE or REVOKE -- because these covenant types update namestate properties that are also
assert()'ed in txdb. So this PR was written to also catch those types but I still need to add explicit testsAdd tests for FINALIZE and REVOKE5013fff💰 PR REVIEW BOUNTY: 💰
10 HNS: add a comment to the PR with an original meme expressing how tired we are all of all the damn wallet bugs
100 HNS: add a comment with a screen shot proving you pulled the branch and ran the test locally:
npm run test-file test/wallet-rescan-test.js1000 HNS: add a comment that actually influnces me to change the pull request in some way. if its just a typo/ nit ill send you..... oh i dunno 200 HNS