Fix importwallet edge case rescan bug #10410

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/scanimp changing 2 files +65 −6

ryanofsky commented at 3:48 PM on May 16, 2017: member

Start importwallet rescans at the first block with timestamp greater or equal to the wallet birthday instead of the last block with timestamp less or equal. This fixes an edge case bug where importwallet could fail to start the rescan early enough if there are blocks with decreasing timestamps or multiple blocks with the same timestamp.

Fix importwallet edge case rescan bug

Start importwallet rescans at the first block with timestamp greater or equal
to the wallet birthday instead of the last block with timestamp less or equal.
This fixes an edge case bug where importwallet could fail to start the rescan
early enough if there are blocks with decreasing timestamps or multiple blocks
with the same timestamp.

2a8e35a11d

fanquake added the label RPC/REST/ZMQ on May 17, 2017
fanquake added the label Wallet on May 17, 2017
laanwj added the label Bug on May 17, 2017
sipa commented at 12:42 AM on May 18, 2017: member

utACK 2a8e35a11d4bd4828631654fc7b8b8fe8f0a2460
laanwj commented at 8:24 AM on May 18, 2017: member

Good catch. Makes the code more readable too, nice!

utACK https://github.com/bitcoin/bitcoin/commit/2a8e35a11d4bd4828631654fc7b8b8fe8f0a2460
laanwj added the label Needs backport on May 18, 2017
laanwj commented at 8:27 AM on May 18, 2017: member

Adding "needs backport" tag as this is a clear bugfix.
ryanofsky commented at 8:54 AM on May 18, 2017: member

@sipa, @laanwj, thanks for reviewing. I also have similar bugfix for importmulti in #10403 if you want to take a look.
gmaxwell commented at 10:04 AM on May 18, 2017: contributor

oops Guess I should have gone and looked for other cases that needed the earliest at least. Concept ACK.

in src/wallet/test/wallet_tests.cpp:456 in 2a8e35a11d

 451 | +    // Create two blocks with same timestamp to verify that importwallet rescan
 452 | +    // will pick up both blocks, not just the first.
 453 | +    const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 5;
 454 | +    SetMockTime(BLOCK_TIME);
 455 | +    coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
 456 | +    coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);

jnewbery commented at 2:32 PM on May 23, 2017:

This is a good test, but why not set the mocktime to BLOCK_TIME - 1 for the second block, or add a third block with block time going backwards? That would make it clearer that the importwallet() code can deal with non-monotonic block times.

jnewbery commented at 2:33 PM on May 23, 2017: member

Looks good. Tested ACK 2a8e35a11d4bd4828631654fc7b8b8fe8f0a2460. New test fails without the importwallet() change.
laanwj merged this on May 23, 2017
laanwj closed this on May 23, 2017
laanwj referenced this in commit e76a3927c3 on May 23, 2017
laanwj referenced this in commit 321419bc06 on May 23, 2017
laanwj removed the label Needs backport on May 23, 2017
nomnombtc referenced this in commit e48a850ed8 on Jul 17, 2017
codablock referenced this in commit a601206d3c on Jan 26, 2018
lateminer referenced this in commit 3926ebeec7 on Jan 5, 2019
andvgal referenced this in commit 5ddde7280f on Jan 6, 2019
CryptoCentric referenced this in commit 4765fe318a on Feb 27, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Bug Wallet RPC/REST/ZMQ