Fix reorg race condition that can cause rare crashes #409
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daira
reviewed
Aug 18, 2022
daira
reviewed
Aug 18, 2022
LarryRuane
force-pushed
the
2022-07-reorg-race
branch
from
91fae0d
to
7afa136
Compare
|
Force-pushed to improve comment as a result of review comment (thanks, @daira), and also thanks for your review, @defuse |
LarryRuane
force-pushed
the
2022-07-reorg-race
branch
from
7afa136
to
94a135e
Compare
|
Force-pushed to fix a comment typo |
daira
reviewed
Aug 23, 2022
common/common_test.go
Outdated
| } | ||
| // Simulate that we're synced (caught up); | ||
| // Simulate that we're synced (caught up, latest block 380641); | ||
| // this should cause one 10s sleep (then retry). |
There was a problem hiding this comment.
Suggested change
| // this should cause one 10s sleep (then retry). | |
| // this should cause one sleep (then retry). |
The sleep interval isn't 10s.
There was a problem hiding this comment.
Good catch, and actually there isn't any sleep-retry in this case (I single-stepped in the debugger to make sure), so I removed that comment line. I think that comment was copy-pasted from elsewhere.
daira
approved these changes
Aug 23, 2022
Fixes issue 408. This bug was introduced by PR 393, which changed how txids are determined. That PR changed each call to the zcash getblock call into a pair of calls, the first to get the raw block data, the second to retrieve the txids in the block. (Unfortunately, you can't get both in a single getblock RPC.) But this ordering introduced a timing window in which the block at the given height can change, if a reorg occurred between the two calls. This PR reorders the getblock calls, so that the first call gets the transaction IDs, which also happens to return the block hash, so then the second getblock call can specify the block hash, rather than the height. This ensures that the two RPC calls return consistent data, definitely the same block.
LarryRuane
force-pushed
the
2022-07-reorg-race
branch
from
94a135e
to
1a556c3
Compare
|
Force-pushed to address @daira's review comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue #408.
This bug was introduced by PR #393, which changed how txids are
determined. That PR changed each call to the zcash getblock call into a
pair of calls, the first to get the raw block data, the second to
retrieve the txids in the block. (Unfortunately, you can't get both in a
single getblock RPC.) But this ordering introduced a timing window in
which the block at the given height can change, if a reorg occurred
between the two calls.
This PR reorders the getblock calls, so that the first call gets the
transaction IDs, which also happens to return the block hash, so then
the second getblock call can specify the block hash, rather than the
height. This ensures that the two RPC calls return consistent data,
definitely the same block.