psbt: Check Taproot tree depth and leaf versions #25513

1. 
   achow101 commented at 3:15 pm on June 30, 2022: member

   Since TaprootBuilder has assertions for the depth and leaf versions, the PSBT decoder should check these values before calling TaprootBuilder::Add so that the assertions are not triggered on malformed taproot trees.

   Fixes #22558 (comment)

2. psbt: Check Taproot tree depth and leaf versions

   Since TaprootBuilder has assertions for the depth and leaf versions, the
   PSBT decoder should check these values before calling
   TaprootBuilder::Add so that the assertions are not triggered on
   malformed taproot trees.

   76fb300b63
3. w0xlt approved
4. 
   w0xlt commented at 5:15 pm on June 30, 2022: contributor

   ACK https://github.com/bitcoin/bitcoin/pull/25513/commits/76fb300b63c5c0d01d768510ec69d894820432fa
5. 
   Sjors commented at 5:28 pm on June 30, 2022: member

   utACK 76fb300b63c5c0d01d768510ec69d894820432fa

   Maybe reject 0x50 too as that’s reserved for the annex.

6. 
   achow101 commented at 6:05 pm on June 30, 2022: member

   Maybe reject 0x50 too as that’s reserved for the annex.

   I don’t think the annex byte has any effect on the leaf version. AFAICT, a future leaf version could b 0x50 too and it wouldn’t conflict with the annex.

7. 
   sipa commented at 6:08 pm on June 30, 2022: member

   From BIP341:

   What constraints are there on the leaf version? First, the leaf version cannot be odd as c[0] & 0xfe will always be even, and cannot be 0x50 as that would result in ambiguity with the annex. In addition, in order to support some forms of static analysis that rely on being able to identify script spends without access to the output being spent, it is recommended to avoid using any leaf versions that would conflict with a valid first byte of either a valid P2WPKH pubkey or a valid P2WSH script (that is, both v and v | 1 should be an undefined, invalid or disabled opcode or an opcode that is not valid as the first opcode). The values that comply to this rule are the 32 even values between 0xc0 and 0xfe and also 0x66, 0x7e, 0x80, 0x84, 0x96, 0x98, 0xba, 0xbc, 0xbe. Note also that this constraint implies that leaf versions should be shared amongst different witness versions, as knowing the witness version requires access to the output being spent.

   I guess it’s in theory possible to use 0x50 as leaf version, but it would be only be usable in spends that also have an annex.

8. DrahtBot added the label PSBT on Jul 1, 2022
9. 
   Sjors commented at 9:37 am on July 2, 2022: member

   If we want to add direct PSBT support for annexes later, presumably in the form of additional keytype’s, it may save us some headache if we disallow 0x50 as a leaf version here. The downside is that if in some far future we end up having to introduce leaf version 0x50, we (or the AI programmer) have to remember to remove the throw.

   https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki

10. 
    achow101 commented at 2:37 pm on July 2, 2022: member

    I don’t see how annexes are at all related to a field which only contains leaf scripts. We wouldn’t be adding annexes to this field - they would be in a separate field.
11. 
    sipa commented at 2:47 pm on July 2, 2022: member

    @achow101 The leaf version is the first byte of the last witness stack element in a P2TR script path spend, after skipping over the annex field if present, which would follow the control block.

    So while there is no actual consensus rule against leaf version 0x50, constructing a spend for it would imply the spend must have an annex as well, because otherwise the control block would be interpreted as an annex.

    For this reason, the BIP does state that leaf version cannot be 0x50.

12. 
    achow101 commented at 3:00 pm on July 2, 2022: member

    I get that, but I don’t see why we should be checking that in a leaf script deserializer. Sure, it may not be a good idea and we shouldn’t consider signing scripts with such a leaf version unless there is an annex, but that doesn’t mean that the deserializer should be enforcing it. Frankly, I don’t think we should even be adding these depth and leaf version checks in the first place, and the only reason I’m adding them is to avoid hitting asserts that exist elsewhere. If the user wants to do something stupid like using bad leaf versions or having things be too deep, then the signer should be dealing with it, not the deserializer. And even if they do, if those paths are never revealed or used, then it doesn’t matter.
13. 
    sipa commented at 3:05 pm on July 2, 2022: member

    @achow101 Fair point, I see where you’re coming from. There is perhaps another reason why depth checking makes sense too, as a DoS prevention mechanism (e.g. put script paths with 1000000 levels in a PSBT, which would hurt the users passing around such PSBTs, even if those script paths are unused).
14. MarcoFalke added this to the milestone 24.0 on Jul 19, 2022
15. 
    sipa commented at 6:36 pm on July 19, 2022: member

    utACK 76fb300b63c5c0d01d768510ec69d894820432fa
16. fanquake merged this on Jul 19, 2022
17. fanquake closed this on Jul 19, 2022

    ---

18. sidhujag referenced this in commit 0420a41b55 on Jul 20, 2022
19. bitcoin locked this on Jul 19, 2023

Contributors
achow101 w0xlt Sjors sipa

Labels
PSBT

Milestone
24.0