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. Whichever comes first wins, psbtxs[0] initializes 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".

[murchandamus]

Thanks for chiming in, @satsfy. @achow101, have you had a chance to think more about this?

[achow101]

How about

diff --git a/bip-0174.mediawiki b/bip-0174.mediawiki
index 9cbbe254..c36a3493 100644
--- a/bip-0174.mediawiki
+++ b/bip-0174.mediawiki
@@ -505,8 +505,13 @@ For every type that a Combiner understands, it may refuse to combine PSBTs if it
 
 The Combiner does not need to know how to interpret scripts in order to combine PSBTs. It can do so without understanding scripts or the network serialization format.
 
-In general, the result of a Combiner combining two PSBTs from independent participants A and B should be functionally equivalent to a result obtained from processing the original PSBT by A and then B in a sequence.
-Or, for participants performing fA(psbt) and fB(psbt): Combine(fA(psbt), fB(psbt)) == fA(fB(psbt)) == fB(fA(psbt))
+In general, the result of a Combiner combining two PSBTs with no conflicting fields from independent participants A and B should be functionally equivalent to a result obtained from processing the original PSBT by A and then B in a sequence.
+Or, for participants performing fA(psbt) and fB(psbt) where fA() and fB() only add unique fields to the PSBT: Combine(fA(psbt), fB(psbt)) == fA(fB(psbt)) == fB(fA(psbt))
+
+Combiners may be asked to combine PSBTs which have conflicting fields, i.e. each PSBT has a field with identical keys but differing values.
+As discussed in [[Handling Duplicated Keys]], the combiner may arbitrariliy choose the value from one PSBT to use in the combined PSBT.
+Alternatively, the combiner may also refuse to combine the PSBTs.
+In this case, the previously discussed commutative property no longer holds.
 
 ===Input Finalizer===
 

[murchandamus]

I saw that Ava’s proposed phrasing had thumb-up emojis from @shesek and @satsfy, but there hadn’t been an update of the PR yet. So, I took the liberty of adding a commit that applied the patch proposed by Ava to @shesek’s branch.
I additionally included a slight rephrasing of the addition, as I would propose that "conflicting fields" is defined when it is first used, and the word "commutative" did not appear before the sentence stating that the commutative property wouldn’t hold in the special case.

Please feel free to drop my commit, if you prefer Ava’s original phrasing, or let me know if you like my proposed touch-up and that should be squashed in.

[achow101]

ACK

[jonatack]

I think this link should be

[[#handling-duplicated-keys|Handling Duplicated Keys]]

Added a fixup in #2203.

[murchandamus]

Thanks!