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?
10. 
    DrahtBot commented at 0:35 am on October 6, 2024: contributor

    There hasn’t been much activity lately. What is the status here?

    Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

11. 
    maflcko commented at 4:26 pm on October 9, 2024: member

    Closing for now due to inactivity (lack of reply to a question about the motivation for this change, as well as lack of reply to review feedback). Please leave a comment, if you want this re-opened.
12. maflcko closed this on Oct 9, 2024

    ---

Contributors
fjahr DrahtBot davidgumberg maflcko

Labels
Validation