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
validation: fix ActivateSnapshot to use hardcoded nChainTx #21681
Conversation
This fixes an oversight from the move of nChainTx from the user-supplied snapshot metadata into the hardcoded assumeutxo chainparams. Since the nChainTx is now unused in the metadata, it should be removed in a future commit.
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.
Code review ACK 931684b
It seems reasonable to just ignore the SnapshotMetadata::m_nchaintx value instead of checking it as long as it's going away. Will there be a followup PR to remove it?
utACK 931684b modulo AppVeyor test issue:
|
Thanks for the reviews @ryanofsky @Sjors. I've pushed a commit removing the nchaintx value from the snapshot metadata. This will of course render previously-generated snapshots unusable, but given that there's currently no way of actually making use of the snapshots I don't think that's an issue. |
Other than breaking half the CI's: sounds good :-) |
This value is no longer used and is instead specified statically in chainparams. This change means that previously generated snapshots will no longer be usable.
4317d41
to
91d93aa
Compare
Oops! Fixed. |
🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file. |
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.
Code review ACK 91d93aa. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
utACK 91d93aa |
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.
I haven't reviewed this because I am still reviewing #19806, but the changes are likely ok to merge in the meantime.
src/node/utxo_snapshot.h
Outdated
@@ -24,6 +24,8 @@ class SnapshotMetadata | |||
|
|||
//! Necessary to "fake" the base nChainTx so that we can estimate progress during |
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.
Reference for myself #19806 (comment)
Summary: bitcoin/bitcoin@769a1ef > test: Add tests with maleated snapshot data ---- https://github.com/bitcoin/bitcoin/pull/21681/files > validation: fix ActivateSnapshot to use hardcoded nChainTx > validation: remove nchaintx from assumeutxo metadata Only test related changes for these commits are in this revision ---- bitcoin/bitcoin@fa8fffe > refactor: Prefer clean assert over UB in coinstats ---- bitcoin/bitcoin@fa9b74f > Fix assumeutxo crash due to missing base_blockhash Only test related changes from this commit are in this revision. ---- bitcoin/bitcoin@fae33f9 > Fix assumeutxo crash due to invalid base_blockhash Only test related changes from this commit are in this revision. ---- This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [7/8] and test related changes from [[bitcoin/bitcoin#21681 | core#21681]], [[bitcoin/bitcoin#21582 | core#21582]] and [[bitcoin/bitcoin#21584 | core#21584]] Depends on D11240 Test Plan: `ninja check` With UBSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11241
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
See: #19806 (comment)