validation: fix ActivateSnapshot to use hardcoded nChainTx #21681

pull jamesob wants to merge 2 commits into bitcoin:master from jamesob:2021-04-au-nchaintx-fix changing 4 files +10 −9

jamesob commented at 5:32 PM on April 14, 2021: member

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 (review)

validation: fix ActivateSnapshot to use hardcoded nChainTx

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.

931684b24a

DrahtBot added the label Validation on Apr 14, 2021
ryanofsky approved
ryanofsky commented at 9:06 PM on April 19, 2021: member

Code review ACK 931684b24a89aba884cb18c13fa67ccca339ee8c

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?

in src/node/utxo_snapshot.h:25 in 931684b24a outdated

  23 | @@ -24,6 +24,8 @@ class SnapshotMetadata
  24 |  
  25 |      //! Necessary to "fake" the base nChainTx so that we can estimate progress during

MarcoFalke commented at 5:51 AM on April 20, 2021:

Reference for myself #19806 (review)

Sjors commented at 7:22 AM on April 22, 2021: member

utACK 931684b modulo AppVeyor test issue:

src\test\validation_chainstatemanager_tests.cpp(278,1): error C3493: 'bad_nchaintx' cannot be implicitly captured because no default capture mode has been specified

jamesob commented at 5:37 PM on April 23, 2021: member

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.
Sjors commented at 11:48 AM on April 26, 2021: member

Other than breaking half the CI's: sounds good :-)

validation: remove nchaintx from assumeutxo metadata

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.

91d93aac4e

jamesob force-pushed on Apr 26, 2021
jamesob commented at 6:05 PM on April 26, 2021: member

Other than breaking half the CI's: sounds good :-)

Oops! Fixed.
DrahtBot commented at 9:32 AM on May 3, 2021: member



🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.
ryanofsky approved
ryanofsky commented at 12:35 AM on May 4, 2021: member

Code review ACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
Sjors commented at 12:16 PM on May 5, 2021: member

utACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672
MarcoFalke commented at 4:30 PM on May 5, 2021: member

I haven't reviewed this because I am still reviewing #19806, but the changes are likely ok to merge in the meantime.
MarcoFalke merged this on May 5, 2021
MarcoFalke closed this on May 5, 2021
sidhujag referenced this in commit 093aba0704 on May 5, 2021
Fabcien referenced this in commit 83ee112814 on Apr 13, 2022
gwillen referenced this in commit 7b25418b9c on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#19806 validation: UTXO snapshot activation #24912 refactor: update CBlockIndex::nChainTx to be uint64_t