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

Fix importwallet edge case rescan bug #10410

Merged
merged 1 commit into from May 23, 2017
Merged

Conversation

ryanofsky
Copy link
Contributor

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.

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.
@sipa
Copy link
Member

sipa commented May 18, 2017

utACK 2a8e35a

@laanwj
Copy link
Member

laanwj commented May 18, 2017

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

utACK 2a8e35a

@laanwj
Copy link
Member

laanwj commented May 18, 2017

Adding "needs backport" tag as this is a clear bugfix.

@ryanofsky
Copy link
Contributor Author

@sipa, @laanwj, thanks for reviewing. I also have similar bugfix for importmulti in #10403 if you want to take a look.

@gmaxwell
Copy link
Contributor

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

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Tested ACK 2a8e35a. New test fails without the importwallet() change.

const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 5;
SetMockTime(BLOCK_TIME);
coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@laanwj laanwj merged commit 2a8e35a into bitcoin:master May 23, 2017
laanwj added a commit that referenced this pull request May 23, 2017
2a8e35a Fix importwallet edge case rescan bug (Russell Yanofsky)

Tree-SHA512: 59522c962290f9ef64436349d11183dd1fd829e515d1f5ec802b63dd813d04303e28d4f3ba38df77a6c151ee4c14f3ca5d3d82204c57456ac94054de62ae4bc7
laanwj pushed a commit that referenced this pull request May 23, 2017
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.

Github-Pull: #10410
Rebased-From: 2a8e35a
nomnombtc pushed a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017
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.

Github-Pull: bitcoin#10410
Rebased-From: 2a8e35a
codablock pushed a commit to codablock/dash that referenced this pull request Jan 26, 2018
2a8e35a Fix importwallet edge case rescan bug (Russell Yanofsky)

Tree-SHA512: 59522c962290f9ef64436349d11183dd1fd829e515d1f5ec802b63dd813d04303e28d4f3ba38df77a6c151ee4c14f3ca5d3d82204c57456ac94054de62ae4bc7
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 5, 2019
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.

Github-Pull: bitcoin#10410
Rebased-From: 2a8e35a
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
2a8e35a Fix importwallet edge case rescan bug (Russell Yanofsky)

Tree-SHA512: 59522c962290f9ef64436349d11183dd1fd829e515d1f5ec802b63dd813d04303e28d4f3ba38df77a6c151ee4c14f3ca5d3d82204c57456ac94054de62ae4bc7
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
2a8e35a Fix importwallet edge case rescan bug (Russell Yanofsky)

Tree-SHA512: 59522c962290f9ef64436349d11183dd1fd829e515d1f5ec802b63dd813d04303e28d4f3ba38df77a6c151ee4c14f3ca5d3d82204c57456ac94054de62ae4bc7
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants