validation: fix ActivateSnapshot to use hardcoded nChainTx #21681

1. 
   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)

2. 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
3. DrahtBot added the label Validation on Apr 14, 2021
4. ryanofsky approved
5. 
   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?

6. 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)
7. 
   Sjors commented at 7:22 am on April 22, 2021: member

   utACK 931684b modulo AppVeyor test issue:

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

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

   Other than breaking half the CI’s: sounds good :-)
10. 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
11. jamesob force-pushed on Apr 26, 2021
12. 
    jamesob commented at 6:05 pm on April 26, 2021: member

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

    Oops! Fixed.

13. 
    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.

14. ryanofsky approved
15. 
    ryanofsky commented at 0: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.
16. 
    Sjors commented at 12:16 pm on May 5, 2021: member

    utACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672
17. 
    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.
18. MarcoFalke merged this on May 5, 2021
19. MarcoFalke closed this on May 5, 2021

    ---

20. sidhujag referenced this in commit 093aba0704 on May 5, 2021
21. Fabcien referenced this in commit 83ee112814 on Apr 13, 2022
22. gwillen referenced this in commit 7b25418b9c on Jun 1, 2022
23. DrahtBot locked this on Aug 16, 2022

Contributors
jamesob ryanofsky MarcoFalke Sjors DrahtBot

Labels
Validation