validation: Use witness maleation flag for non-segwit blocks #29540

1. 
   fjahr commented at 10:02 pm on March 2, 2024: contributor

   Depends on #29524

   This is an alternative to #29539 since the race condition documented there should be fixed by #29524. The race condition was discussed here. If the race is not possible we can cache this check for non-segwit blocks (= no expected witness) as well.

2. 
   DrahtBot commented at 10:02 pm on March 2, 2024: contributor

   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
   Concept ACK	davidgumberg

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

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label Validation on Mar 2, 2024
4. fjahr force-pushed on Mar 10, 2024
5. 
   fjahr commented at 11:11 pm on March 10, 2024: contributor

   Rebased since #29524 was merged
6. validation: Use witness maleation flag for non-segwit blocks 23abe0cd40
7. fjahr force-pushed on Mar 17, 2024
8. 
   davidgumberg commented at 4:42 am on April 8, 2024: contributor

   Concept ACK

   Nits that seem to be close to the scope of this PR:

   I think this comment is incorrect:

   0/*
   1 * Note: If the witness commitment is expected (i.e. `expect_witness_commitment
   2 * = true`), then the block is required to have at least one transaction and the
   3 * first transaction needs to have at least one input. */
   

   Isn’t it only if the witness commitment is expected and present (i.e. commitpos != NO_WITNESS_COMMITMENT ) that:

   0assert(!block.vtx.empty() && !block.vtx[0]->vin.empty());
   

   ---

   The comment in src/primitives/block.h is incorrect:

   0-     mutable bool m_checked_witness_commitment{false}; // CheckWitnessCommitment()
   1+     mutable bool m_checked_witness_commitment{false}; // CheckWitnessMalleation()
   

   ---

   Also, maybe the flag should be renamed to m_checked_witness_malleation so that it’s scope is less ambiguous

9. 
   maflcko commented at 8:10 am on April 9, 2024: member

   -0. Not sure if this is worth it? While the race should have been fixed, are there any benefits that warrant the review?

Contributors
fjahr DrahtBot davidgumberg maflcko

Labels
Validation