Remove BIP 174's claim that Combine is commutative

[shesek]

The BIP asserts that fA(fB(psbt)) == fB(fA(psbt)), however the explanatory text before this doesn't actually say this and even hints that the ordering does matter: "processing [..] A and then B in a sequence". It seems that the BIP text only supports the Combine(fA(psbt), fB(psbt)) == fB(fA(psbt)) part, and that fA(fB(psbt)) slipped in by accident?

In practice, Bitcoin Core's combinepsbt isn't commutative and gives precedence to latter PSBTs in the array. Here's a quick example demonstrating this:

PSBT_A="cHNidP8BADMCAAAAAa83fnb+Vw0gR7jZBeABQNh2dFx8F+MbmvHAM8N5+O07AAAAAAD/////AAAAAAAAAQUBUQA="
PSBT_B="cHNidP8BADMCAAAAAa83fnb+Vw0gR7jZBeABQNh2dFx8F+MbmvHAM8N5+O07AAAAAAD/////AAAAAAAAAQUBUgA="
$ bitcoin-cli decodepsbt $(bitcoin-cli combinepsbt [\"$PSBT_A\",\"$PSBT_B\"]) | jq -r .inputs[0].witness_script.asm
1
$ bitcoin-cli decodepsbt $(bitcoin-cli combinepsbt [\"$PSBT_B\",\"$PSBT_A\"]) | jq -r .inputs[0].witness_script.asm
2

And here's a related discussion about rust-bitcoin's Psbt::combine(), which isn't commutative either but documented as "In accordance with BIP 174 this function is commutative": rust-bitcoin/rust-bitcoin#5486

[murchandamus]

cc: @achow101

[achow101]

It's supposed to be commutative, although I suppose that is contradictory with "The Combiner must remove any duplicate key-value pairs, in accordance with the specification. It can pick arbitrarily when conflicts occur."

Will need to think on this a bit more.

[shesek]

It's supposed to be commutative

Thanks for clarifying this and apologies for the confusion. I would consider updating the BIP text to say that explicitly, it is kind of confusing that its in the formula but not mentioned anywhere in the text.

One option for making Combine commutative, brought up by @apoelstra in rust-bitcoin/rust-bitcoin#5486 (comment), is to fail it entirely in case of conflicts.

This is already mentioned in the BIP, as a may: "For every type that a Combiner understands, it may refuse to combine PSBTs if it detects that there will be inconsistencies or conflicts for that type in the combined PSBT."

"It can pick arbitrarily when conflicts occur."

Other than implying non-commutativity, this is also contradictory with "The resulting PSBT must contain all of the key-value pairs from each of the PSBTs" (which basically seems impossible to comply with if there are conflicts and should probably be removed?).

[murchandamus]

@achow101, @shesek: Any update on this one?

[satsfy]

Core's combinepsbt has no logging, warning, or error for field-level conflicts. It is first wins: psbtxs[0] seeds the merge, and it silently drops subsequent conflicting values (redeem_script, witness_script, non_witness_utxo, taproot keys) and silently keeps the first entry for conflicting map keys (hd_keypaths, taproot script sigs, MuSig2 partials).

The text says Combine is commutative across input order and also says it can pick arbitrarily on conflict. These cannot both hold, since arbitrary choice on conflict makes the output depend on input order, which breaks commutativity. The path forward here is to rewrite the existing "may refuse to combine" as "must fail on conflicting duplicates".