script: BIP341 txdata cannot be precomputed without spent outputs #27122

1. 
   sipa commented at 9:34 pm on February 17, 2023: member

   In PrecomputedTransactionData::Init, if force is set to true, m_bip341_taproot_ready is always set to true, suggesting that all its BIP341-relevant members (including m_spent_amounts_single_hash) are correct. If however no spent array of spent previous CTxOuts is provided, some of these members will be incorrect. This option was introduced in #21365.

   That doesn’t actually hurt, as without prevout data, it’s fundamentally impossible to generate correct BIP341 signatures anyway, and https://github.com/bitcoin/bitcoin/blob/f722a9bd132222d9d5cd503b5af25c905b205cdb/src/script/sign.cpp#L71 should prevent the logic from being used anyway.

   Still, don’t set m_bip341_taproot_ready variable when we clearly don’t have enough data to compute it.

   Discovered by Russell O’Connor.

2. BIP341 txdata cannot be precomputed without spent outputs 95f12de925
3. 
   DrahtBot commented at 9:34 pm on February 17, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	instagibbs, ajtowns, achow101

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

4. 
   sipa commented at 9:35 pm on February 17, 2023: member

   Note that this shouldn’t affect any consensus logic, as spent is always provided in that setting.
5. 
   sipa commented at 1:03 am on February 18, 2023: member

   cc @roconnor-blockstream
6. roconnor-blockstream referenced this in commit 777cc1a2c0 on Feb 18, 2023
7. maflcko renamed this:
   BIP341 txdata cannot be precomputed without spent outputs
   script: BIP341 txdata cannot be precomputed without spent outputs
   on Feb 20, 2023
8. DrahtBot added the label Consensus on Feb 20, 2023
9. fanquake requested review from instagibbs on Feb 20, 2023
10. fanquake requested review from ajtowns on Feb 20, 2023
11. instagibbs approved
12. 
    instagibbs commented at 3:13 pm on February 21, 2023: member

    ACK 95f12de92505522a32ba58acd5251c69e602d160

    I do wonder if it makes sense to rename force to something like signing_context or even more drastically split Init up into two obvious types ConsensusInit/SigningInit to make things clearer to future readers.

13. 
    ajtowns commented at 3:42 pm on February 21, 2023: contributor

    ACK 95f12de92505522a32ba58acd5251c69e602d160

    This was directly addressed in the comments earlier; should that be updated too?

    This only works if spent_outputs was provided as well, but if it wasn't, actual validation will fail anyway.

    The immediately following comment seems wrong too, as the branch is gated by a !scriptWitness.IsNull() test?

    Note that this branch may trigger for scriptPubKeys that aren't actually segwit but in that case validation will fail as SCRIPT_ERR_WITNESS_UNEXPECTED anyway.

    Also seems weird to have the uses_bip341 && uses_bip143 test both as part of the loop condition and as the last thing in the loop.

14. 
    roconnor-blockstream commented at 6:17 pm on February 21, 2023: contributor

    The immediately following comment seems wrong too, as the branch is gated by a !scriptWitness.IsNull() test?

    Note that this branch may trigger for scriptPubKeys that aren’t actually segwit but in that case validation will fail as SCRIPT_ERR_WITNESS_UNEXPECTED anyway.

    I believe this comment is saying it will trigger in the case the the scriptPubKey is OP_1 followed by a series of opcodes of an appropriate length, but is not a single push opcode, and a witness is (illegally) provided.

15. 
    roconnor-blockstream commented at 6:23 pm on February 21, 2023: contributor

    This was directly addressed in the comments earlier; should that be updated too?

    This only works if spent_outputs was provided as well, but if it wasn’t, actual validation will fail anyway.

    I believe this comment is trying to say that (under the implicit circumstances that force is false) if the spend outputs are not provided, it will fail to detect if an input is using bip341. But if spent outputs are not provided, then bip341 cannot be validated anyways.

16. 
    achow101 commented at 6:46 pm on February 21, 2023: member

    ACK 95f12de92505522a32ba58acd5251c69e602d160
17. achow101 merged this on Feb 21, 2023
18. achow101 closed this on Feb 21, 2023

    ---

19. roconnor-blockstream referenced this in commit aa8310c156 on Feb 23, 2023
20. sidhujag referenced this in commit 7d2e962742 on Feb 25, 2023
21. bitcoin locked this on Feb 21, 2024

Contributors
sipa DrahtBot instagibbs ajtowns roconnor-blockstream achow101

Review Requested
ajtowns

Labels
Consensus