-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
assumeutxo: Get rid of faked nTx and nChainTx values #29370
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir. |
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
It makes sense that having backwards compatibility may not be worth it, but I don't see why test chains would set Updated 4c70d99 -> a3228f0 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Also, it would be good to include the test to ensure the crash no longer happens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the close review! Will work on your suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated a3228f0 -> 3bdbc3f (pr/nofake.2
-> pr/nofake.3
, compare) implementing all suggestions, adding test coverage for the RPCs, and adding a new commit that drops the no longer needed BLOCK_ASSUMED_VALID flag.
Updated 3bdbc3f -> 603a92c (pr/nofake.3
-> pr/nofake.4
, compare) removing more BLOCK_ASSUMED_VALID references
Needs rebase |
Yes, thanks for recognizing the CI failures. Rebased 603a92c -> 3f0b860 ( |
Could include the test #29370 (comment) , or did you want to leave that for later? |
Add ubsan suppressions for integer overflows in the getchaintxstats RPC. getchainstatstx line "int nTxDiff = pindex->nChainTx - past_block.nChainTx" can trigger ubsan integer overflows when assumeutxo snapshots are loaded, from subtracting unsigned values and assigning the result to a signed int. The overflow behavior probably exists in current code but is hard to trigger because it would require calling getchainstatstx at the right time with specific parameters as background blocks are being downloaded. But the overflow behavior becomes easier to trigger in the upcoming commit removing fake nChainTx values, so a suppression needs to be added before then for CI to pass. getchainstatstx should probably be improved separately in another PR to not need this suppression, and handle edge cases and missing nChainTx values more carefully.
…nected block Use Assume macro as suggested bitcoin#29370 (comment)
The checks are changing slightly in the next commit, so try to explains the ones that exist to avoid confusion (bitcoin#29370 (comment))
The `PopulateAndValidateSnapshot` function introduced in f6e2da5 from bitcoin#19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (see bitcoin#29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake. Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. This commit fixes at least two assert failures in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would happen previously. Tests for these failures are added separately in the next two commits. Compatibility note: This change could result in -checkblockindex failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake nTx values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.
Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The crash was an assert failure in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would previously happen if a snapshot was loaded, and a block was submitted which forked from the chain before the snapshot block and after the last downloaded background chain block. This block would not be marked assumed-valid because it would not be an ancestor of the snapshot, and it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake value, so the assert would fail. After the fix, prev->nChainTx is unset instead of being set to a fake value, so the assert succeeds. This test was originally posted by maflcko in bitcoin#29261 (comment) Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The crash was an assert failure in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would previously happen if the snapshot block was submitted after loading the snapshot and downloading a few blocks after the snapshot. In that case ReceivedBlockTransactions() previously would overwrite the nChainTx value of the submitted snapshot block with a fake value based on the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check would later fail on the first block after the snapshot. This test was originally posted by Martin Zumsande <mzumsande@gmail.com> in bitcoin#29370 (comment) Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Flag adds complexity and is not currently used for anything.
…nected block Use Assume macro as suggested bitcoin#29370 (comment)
The checks are changing slightly in the next commit, so try to explains the ones that exist to avoid confusion (bitcoin#29370 (comment))
Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The crash was an assert failure in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would previously happen if the snapshot block was submitted after loading the snapshot and downloading a few blocks after the snapshot. In that case ReceivedBlockTransactions() previously would overwrite the nChainTx value of the submitted snapshot block with a fake value based on the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check would later fail on the first block after the snapshot. This test was originally posted by Martin Zumsande <mzumsande@gmail.com> in bitcoin#29370 (comment) Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased 345ac98 -> 9d9a745 (pr/nofake.20
-> pr/nofake.21
, compare) due to conflict with #29478
// and with nTx > 0 (since we aren't setting the pruned flag); | ||
// see CheckBlockIndex(). | ||
pindex->nStatus |= BLOCK_ASSUMED_VALID; | ||
// Remove all data and validity flags by just setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #29370 (comment)
The comment above (
Reset the HAVE_DATA flags...
) could be removed or at least changed to match the changed code here.
The comment above is still accurate and still describes the main thing this is trying to do. Now just other flags are reset as well. Happy to change any of the comments if you have a specific suggestion though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 9d9a745
I reviewed the code and tested this running with checkblockindex
on signet.
// and the transactions were not pruned (pindexFirstMissing | ||
// is null), it is a potential candidate. The check | ||
// excludes pruned blocks, because if any blocks were | ||
// pruned between pindex the current chain tip, pindex will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 9b97d5b: pindex *and* the current
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9d9a745 🎯
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 9d9a7458a2570f7db56ab626b22010591089c312 🎯
m+XWEuj+TMxTAm2vqwQ2KlIlqiGQf/ADm9CzC8dphRCYKKQo4VacrJ9sy4MIZjiOABr94CzwXsoVSmSPlrElBw==
# setting and testing nChainTx values, and it exposed previous bugs. | ||
snapshot_hash = n0.getblockhash(SNAPSHOT_BASE_HEIGHT) | ||
snapshot_block = n0.getblock(snapshot_hash, 0) | ||
n1.submitblock(snapshot_block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit in ef174e9:
I wonder if it would be better to randomly either submit or not submit the block, because the test should be passing in both cases.
If a crash were to happen, it would be intermittent. However, it would still be deterministic, because the randomness seed is printed/logged.
ACK 9d9a745 |
Post merge utACK 9d9a745 |
…nected block Use Assume macro as suggested bitcoin#29370 (comment)
The checks are changing slightly in the next commit, so try to explains the ones that exist to avoid confusion (bitcoin#29370 (comment))
Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The crash was an assert failure in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would previously happen if the snapshot block was submitted after loading the snapshot and downloading a few blocks after the snapshot. In that case ReceivedBlockTransactions() previously would overwrite the nChainTx value of the submitted snapshot block with a fake value based on the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check would later fail on the first block after the snapshot. This test was originally posted by Martin Zumsande <mzumsande@gmail.com> in bitcoin#29370 (comment) Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
…nected block Use Assume macro as suggested bitcoin/bitcoin#29370 (comment)
The checks are changing slightly in the next commit, so try to explains the ones that exist to avoid confusion (bitcoin/bitcoin#29370 (comment))
Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The crash was an assert failure in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would previously happen if the snapshot block was submitted after loading the snapshot and downloading a few blocks after the snapshot. In that case ReceivedBlockTransactions() previously would overwrite the nChainTx value of the submitted snapshot block with a fake value based on the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check would later fail on the first block after the snapshot. This test was originally posted by Martin Zumsande <mzumsande@gmail.com> in bitcoin/bitcoin#29370 (comment) Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
The
PopulateAndValidateSnapshot
function introduced in f6e2da5 from #19806 has been setting fakenTx
andnChainTx
values that can show up in RPC results (#29328) and makeCBlockIndex
state hard to reason about, because it is difficult to know whether the values are real or fake.Revert to previous behavior of setting
nTx
andnChainTx
to 0 when the values are unknown, instead of faking them. Also drop no-longer neededBLOCK_ASSUMED_VALID
flag.Dropping the faked values also fixes assert failures in the
CheckBlockIndex
(pindex->nChainTx == pindex->nTx + prev_chain_tx)
check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.Compatibility note: This change could cause new
-checkblockindex
failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fakenTx
values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code toLoadBlockIndex
and changingnTx
values from 1 to 0 when they are fake (when(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS
), but a little simpler not to worry about being compatible in this case.