psbt: Check Taproot tree depth and leaf versions #25513

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:taproot-psbt-check-leaf-ver changing 1 files +6 −0

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)

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

w0xlt approved
w0xlt commented at 5:15 PM on June 30, 2022: contributor

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

utACK 76fb300b63c5c0d01d768510ec69d894820432fa

Maybe reject 0x50 too as that's reserved for the annex.
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.
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.
DrahtBot added the label PSBT on Jul 1, 2022
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
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.
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.
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.
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).
MarcoFalke added this to the milestone 24.0 on Jul 19, 2022
sipa commented at 6:36 PM on July 19, 2022: member

utACK 76fb300b63c5c0d01d768510ec69d894820432fa
fanquake merged this on Jul 19, 2022
fanquake closed this on Jul 19, 2022
sidhujag referenced this in commit 0420a41b55 on Jul 20, 2022
bitcoin locked this on Jul 19, 2023

Contributors

Labels

Milestone
24.0

Linked (view graph)

#22558 psbt: Taproot fields for PSBT