AssumeUTXO follow-ups #28562

fjahr · 2023-10-02T22:42:30Z

Addressing what I consider to be non- or not-too-controversial comments from #27596.

Let me know if I missed anything among the many comments that can be easily included here.

DrahtBot · 2023-10-02T22:42:33Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Stale ACK	jamesob, Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28608 (assumeutxo state and locking cleanup by ryanofsky)

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.

jamesob

Github ACK 26c815b

Thanks for addressing these!

src/node/blockstorage.cpp

maflcko

Could also rename HaveTxsDownloaded to HaveNumChainTxs to fix the TODO in the function?

test/functional/feature_assumeutxo.py

fjahr · 2023-10-03T16:37:19Z

Addressed feedback from @MarcoFalke

ryanofsky

Code review ACK 69fbb77

src/rpc/blockchain.cpp

test/functional/feature_assumeutxo.py

src/init.cpp

maflcko · 2023-10-03T20:56:26Z

Left some more review comments on the parent pull.

fjahr · 2023-10-04T14:43:45Z

The handling of timeouts in feature_assumeutxo was moved out into #28586

fanquake · 2023-10-04T15:09:10Z

Can you point back to / comment on #27596 in regards to what is fixed / followed up on here, and what still needs fixing? Given there are now a number of post-merge review comments.

ryanofsky

Code review ACK 2c9a841

src/rpc/blockchain.cpp

ryanofsky · 2023-10-04T16:21:38Z

re: #28562 (comment)

Can you point back to / comment on #27596 in regards to what is fixed / followed up on here, and what still needs fixing? Given there are now a number of post-merge review comments.

This would be great, but there are a lot of open review comments on #27596 right now and this PR only addresses a small number of them, so it would be hard to compile a list of what is left and decide what is worth spending time to fix.

It would be good to comment on #27596 about what things were addressed here, though, so if someone wants to make another PR that addresses other issues that seem important, they can do that after this one.

DrahtBot · 2023-10-04T18:50:55Z

🐙 This pull request conflicts with the target branch and needs rebase.

… return a list of chainstates a9ef702 assumeutxo: change getchainstates RPC to return a list of chainstates (Ryan Ofsky) Pull request description: Current `getchainstates` RPC returns "normal" and "snapshot" fields which are not ideal because it requires new "normal" and "snapshot" terms to be defined, and the definitions are not really consistent with internal code. (In the RPC interface, the "snapshot" chainstate becomes the "normal" chainstate after it is validated, while in internal code there is no "normal chainstate" and the "snapshot chainstate" is still called that temporarily after it is validated). The current `getchainstates` RPC is also awkward to use if you to want information about the most-work chainstate, because you have to look at the "snapshot" field if it exists, and otherwise fall back to the "normal" field. Fix these issues by having `getchainstates` just return a flat list of chainstates ordered by work, and adding a new chainstate "validated" field alongside the existing "snapshot_blockhash" field so it is explicit if a chainstate was originally loaded from a snapshot, and whether the snapshot has been validated. This change was motivated by comment thread in bitcoin/bitcoin#28562 (comment) ACKs for top commit: Sjors: re-ACK a9ef702 jamesob: re-ACK a9ef702 achow101: ACK a9ef702 Tree-SHA512: b364e2e96675fb7beaaee60c4dff4b69e6bc2d8a30dea1ba094265633d1cddf9dbf1c5ce20c07d6e23222cf1e92a195acf6227e4901f3962e81a1e53a43490aa

Sjors · 2023-10-06T12:28:35Z

Can you give this a rebase? I think 952f037 can be dropped after #28590?

Sjors

utACK 0b2c93b modulo dropping commit 952f037 and its related comment.

src/rpc/blockchain.cpp

src/init.cpp

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

ryanofsky

Code review ACK 62db927. Just new commit removing assumevalid mentions and rebase since last review

src/init.cpp

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

…sing cursor Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

fjahr · 2023-10-06T17:47:37Z

Rebased, addressed a few more comments, dropped the changes that have been addressed elsewhere in the meantime. I also tried to comment on everything that is addressed here or elsewhere in #27596.

… flat list of chainstates Also add m_validated and m_target_block members to Chainstate class it is possible to determine chainstate properties directly from Chainstate objects and it is not neccessary for validation code make calls to ChainstateManager and decide what to do by looking at assumeutxo download state. Goal is to remove hardcoded logic handling assumeutxo snapshots from most validation code. Also to make it easy to fix locking issues like the fact that cs_main is currently held unnecessarily validating snapshots, and the fact that background chainstate is not immediately deleted when it is no longer used, wasting disk space and adding a long startup delay next time the node is restarted. This follows up on some previous discussions: bitcoin#24232 (comment) bitcoin#27746 (comment) bitcoin#28562 (comment)

ryanofsky

Code review ACK 5d227a6. Just suggested doc change and new EnsureChainman RPC cleanup commit since last review.

maflcko · 2023-10-09T15:40:25Z

src/validation.cpp

+        // Make sure nChainTx sum is correctly computed.
+        unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
+        assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
+               // For testing, allow transaction counts to be completely unset.


Which testing do you mean? -chain=main -checkblockindex on mainnet?

The tests that are changed in the same commit.

I removed the "testing" check and ran the main binary (not the tests) and it crashed here.

Generally I don't think a good place to put test-only code is the main production code. Especially when it comes to consensus critical code. This makes it impossible to properly test and review the code in a production environment outside of unit tests.

I removed the "testing" check and ran the main binary (not the tests) and it crashed here.

Hm, can you say what exactly you removed and what you can to see the crash? I couldn't reproduce that so far.

Generally I don't think a good place to put test-only code is the main production code. Especially when it comes to consensus critical code. This makes it impossible to properly test and review the code in a production environment outside of unit tests.

I guess we should improve our test setup to include realistic nTx and nChainTx values so we can remove those checks again.

I used this diff IIRC, which also fails the functional tests:

diff --git a/src/validation.cpp b/src/validation.cpp index 9108f911f0..3bfa5091ad 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4847,10 +4847,7 @@ void ChainstateManager::CheckBlockIndex() // Make sure nChainTx sum is correctly computed. unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0; assert((pindex->nChainTx == pindex->nTx + prev_chain_tx) - // For testing, allow transaction counts to be completely unset. - || (pindex->nChainTx == 0 && pindex->nTx == 0) - // For testing, allow this nChainTx to be unset if previous is also unset. - || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)); + ); if (pindexFirstAssumeValid == nullptr && pindex->nStatus & BLOCK_ASSUMED_VALID) pindexFirstAssumeValid = pindex; if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;

Another report, which could replicate the crash, see ##28791

Turned into an issue: #29261

… flat list of chainstates Also add m_validity and m_target_block members to Chainstate class it is possible to determine chainstate properties directly from Chainstate objects and it is not neccessary for validation code make calls to ChainstateManager and decide what to do by looking at assumeutxo download state. Goal is to remove hardcoded logic handling assumeutxo snapshots from most validation code. Also to make it easy to fix locking issues like the fact that cs_main is currently held unnecessarily validating snapshots, and the fact that background chainstate is not immediately deleted when it is no longer used, wasting disk space and adding a long startup delay next time the node is restarted. This follows up on some previous discussions: bitcoin#24232 (comment) bitcoin#27746 (comment) bitcoin#28562 (comment)

…a list of chainstates a9ef702 assumeutxo: change getchainstates RPC to return a list of chainstates (Ryan Ofsky) Pull request description: Current `getchainstates` RPC returns "normal" and "snapshot" fields which are not ideal because it requires new "normal" and "snapshot" terms to be defined, and the definitions are not really consistent with internal code. (In the RPC interface, the "snapshot" chainstate becomes the "normal" chainstate after it is validated, while in internal code there is no "normal chainstate" and the "snapshot chainstate" is still called that temporarily after it is validated). The current `getchainstates` RPC is also awkward to use if you to want information about the most-work chainstate, because you have to look at the "snapshot" field if it exists, and otherwise fall back to the "normal" field. Fix these issues by having `getchainstates` just return a flat list of chainstates ordered by work, and adding a new chainstate "validated" field alongside the existing "snapshot_blockhash" field so it is explicit if a chainstate was originally loaded from a snapshot, and whether the snapshot has been validated. This change was motivated by comment thread in bitcoin#28562 (comment) ACKs for top commit: Sjors: re-ACK a9ef702 jamesob: re-ACK a9ef702 achow101: ACK a9ef702 Tree-SHA512: b364e2e96675fb7beaaee60c4dff4b69e6bc2d8a30dea1ba094265633d1cddf9dbf1c5ce20c07d6e23222cf1e92a195acf6227e4901f3962e81a1e53a43490aa

… flat list of chainstates Also add m_validity and m_target_block members to Chainstate class it is possible to determine chainstate properties directly from Chainstate objects and it is not neccessary for validation code make calls to ChainstateManager and decide what to do by looking at assumeutxo download state. Goal is to remove hardcoded logic handling assumeutxo snapshots from most validation code. Also to make it easy to fix locking issues like the fact that cs_main is currently held unnecessarily validating snapshots, and the fact that background chainstate is not immediately deleted when it is no longer used, wasting disk space and adding a long startup delay next time the node is restarted. This follows up on some previous discussions: bitcoin#24232 (comment) bitcoin#27746 (comment) bitcoin#28562 (comment)

fjahr force-pushed the 2023-10-au-followups branch 2 times, most recently from 45aa654 to 2bd1d39 Compare October 2, 2023 23:20

fjahr marked this pull request as ready for review October 2, 2023 23:40

fanquake requested review from jamesob and ryanofsky October 3, 2023 08:37

jamesob approved these changes Oct 3, 2023

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

maflcko reviewed Oct 3, 2023

View reviewed changes

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

fjahr force-pushed the 2023-10-au-followups branch from 0081d12 to 69fbb77 Compare October 3, 2023 16:36

ryanofsky approved these changes Oct 3, 2023

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from jamesob October 3, 2023 19:38

fjahr force-pushed the 2023-10-au-followups branch from 69fbb77 to 7a407a8 Compare October 3, 2023 21:35

DrahtBot mentioned this pull request Oct 3, 2023

test: Use pathlib over os path #28392

Merged

fjahr force-pushed the 2023-10-au-followups branch from 7a407a8 to 2c9a841 Compare October 4, 2023 14:44

ryanofsky mentioned this pull request Oct 4, 2023

assumeutxo: change getchainstates RPC to return a list of chainstates #28590

Merged

ryanofsky approved these changes Oct 4, 2023

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Oct 4, 2023

fanquake added this to the 26.0 milestone Oct 6, 2023

Sjors reviewed Oct 6, 2023

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from Sjors October 6, 2023 14:33

fjahr and others added 2 commits October 6, 2023 18:11

validation: remove unused mempool param in DetectSnapshotChainstate

0a39b8c

doc: Add and edit some comments around assumeutxo

a47fbe7

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

ryanofsky approved these changes Oct 6, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

fjahr and others added 7 commits October 6, 2023 19:43

doc: Add snapshot chainstate removal warning to reindexing documentation

2c9354f

validation, test: Improve and document nChainTx check for testability

73700fb

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

blockstorage: Let FlushChainstateBlockFile return true in case of mis…

82e48d2

…sing cursor Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

chain: Rename HaveTxsDownloaded to HaveNumChainTxs

a482f86

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

test: Rename wait_until_helper to wait_until_helper_internal

1ff1c34

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

doc: Drop references to assumevalid in assumeutxo docs

710e5db

rpc: Use Ensure(Any)Chainman in assumeutxo related RPCs

5d227a6

fjahr force-pushed the 2023-10-au-followups branch from 62db927 to 5d227a6 Compare October 6, 2023 17:43

DrahtBot added the CI failed label Oct 6, 2023

DrahtBot removed the CI failed label Oct 6, 2023

ryanofsky mentioned this pull request Oct 6, 2023

assumeutxo state and locking cleanup #28608

Draft

ryanofsky approved these changes Oct 6, 2023

View reviewed changes

fanquake merged commit 38f4b0d into bitcoin:master Oct 7, 2023
16 checks passed

maflcko reviewed Oct 9, 2023

View reviewed changes

maflcko removed the Needs rebase label Oct 10, 2023

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023

Merge bitcoin#28562: AssumeUTXO follow-ups

4858ad1

maflcko mentioned this pull request Nov 8, 2023

snapshots: don't core dump when running -checkblockindex after loadtxoutset #28791

Merged

maflcko mentioned this pull request Jan 17, 2024

assumeutxo: nTx and nChainTx violations in CheckBlockIndex #29261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AssumeUTXO follow-ups #28562

AssumeUTXO follow-ups #28562

fjahr commented Oct 2, 2023 •

edited

DrahtBot commented Oct 2, 2023 •

edited

jamesob left a comment

maflcko left a comment

fjahr commented Oct 3, 2023

ryanofsky left a comment •

edited

maflcko commented Oct 3, 2023

fjahr commented Oct 4, 2023

fanquake commented Oct 4, 2023

ryanofsky left a comment

ryanofsky commented Oct 4, 2023 •

edited

DrahtBot commented Oct 4, 2023

Sjors commented Oct 6, 2023

Sjors left a comment

ryanofsky left a comment

fjahr commented Oct 6, 2023 •

edited

ryanofsky left a comment

maflcko Oct 9, 2023

fjahr Oct 9, 2023

maflcko Oct 10, 2023 •

edited

fjahr Oct 10, 2023

maflcko Oct 10, 2023

maflcko Nov 8, 2023

maflcko Jan 17, 2024

AssumeUTXO follow-ups #28562

AssumeUTXO follow-ups #28562

Conversation

fjahr commented Oct 2, 2023 • edited

DrahtBot commented Oct 2, 2023 • edited

Code Coverage

Reviews

Conflicts

jamesob left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

fjahr commented Oct 3, 2023

ryanofsky left a comment • edited

Choose a reason for hiding this comment

maflcko commented Oct 3, 2023

fjahr commented Oct 4, 2023

fanquake commented Oct 4, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky commented Oct 4, 2023 • edited

DrahtBot commented Oct 4, 2023

Sjors commented Oct 6, 2023

Sjors left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

fjahr commented Oct 6, 2023 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

maflcko Oct 9, 2023

Choose a reason for hiding this comment

fjahr Oct 9, 2023

Choose a reason for hiding this comment

maflcko Oct 10, 2023 • edited

Choose a reason for hiding this comment

fjahr Oct 10, 2023

Choose a reason for hiding this comment

maflcko Oct 10, 2023

Choose a reason for hiding this comment

maflcko Nov 8, 2023

Choose a reason for hiding this comment

maflcko Jan 17, 2024

Choose a reason for hiding this comment

fjahr commented Oct 2, 2023 •

edited

DrahtBot commented Oct 2, 2023 •

edited

ryanofsky left a comment •

edited

ryanofsky commented Oct 4, 2023 •

edited

fjahr commented Oct 6, 2023 •

edited

maflcko Oct 10, 2023 •

edited