test: check that combining PSBTs with different txs fails #25670

theStack commented at 8:17 PM on July 21, 2022: contributor

This PR adds missing test coverage for the combinepsbt RPC, in the case of combining two PSBTs with different transactions: https://github.com/bitcoin/bitcoin/blob/b8067cd435059eedb580975afc62c4e7a6f27321/src/psbt.cpp#L24-L27 The calling function CombinePSBTs checks for the false return value and then returns the transaction error string PSBT_MISMATCH: https://github.com/bitcoin/bitcoin/blob/b8067cd435059eedb580975afc62c4e7a6f27321/src/psbt.cpp#L433-L435 https://github.com/bitcoin/bitcoin/blob/b8067cd435059eedb580975afc62c4e7a6f27321/src/util/error.cpp#L30-L31

fanquake added the label Tests on Jul 21, 2022

theStack force-pushed on Jul 21, 2022

theStack commented at 8:30 PM on July 21, 2022: contributor

Force-pushed a different variant of the first commit (extending the PSBT ctor), avoiding mutable objects as default parameters due to the following linter complaint:

A mutable list or dict seems to be used as default parameter value:

test/functional/test_framework/psbt.py:    def __init__(self, g=PSBTMap(), i=[], o=[]):

This is how mutable list and dict default parameter values behave:

>>> def f(i, j=[], k={}):
...     j.append(i)
...     k[i] = True
...     return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})

The intended behaviour was likely:

>>> def f(i, j=None, k=None):
...     if j is None:
...         j = []
...     if k is None:
...         k = {}
...     j.append(i)
...     k[i] = True
...     return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})

fanquake requested review from achow101 on Jul 22, 2022

fanquake requested review from instagibbs on Jul 22, 2022

in test/functional/rpc_psbt.py:830 in b4f332275c outdated

 825 | +        tx.vin = [CTxIn(outpoint=COutPoint(hash=int('aa' * 32, 16), n=0), scriptSig=b"")]
 826 | +        tx.vout = [CTxOut(nValue=0, scriptPubKey=b"")]
 827 | +        psbt1 = PSBT(PSBTMap({PSBT_GLOBAL_UNSIGNED_TX: tx.serialize()}), [PSBTMap()], [PSBTMap()]).to_base64()
 828 | +        tx.vout[0].nValue += 1  # slightly modify tx
 829 | +        psbt2 = PSBT(PSBTMap({PSBT_GLOBAL_UNSIGNED_TX: tx.serialize()}), [PSBTMap()], [PSBTMap()]).to_base64()
 830 | +        assert_raises_rpc_error(-8, "PSBTs not compatible (different transactions)", self.nodes[0].combinepsbt, [psbt1, psbt2])

instagibbs commented at 6:45 PM on July 22, 2022:

Might as well check the naive [psbt1, psbt1] case to make sure that works

theStack commented at 7:02 AM on July 23, 2022:

Good idea, done.

in test/functional/rpc_psbt.py:827 in b4f332275c outdated

 819 | @@ -820,6 +820,15 @@ def test_psbt_input_keys(psbt_input, keys):
 820 |              assert hash.hex() in res_input[preimage_key]
 821 |              assert_equal(res_input[preimage_key][hash.hex()], preimage.hex())
 822 |  
 823 | +        self.log.info("Test that combining PSBTs with different transactions fails")
 824 | +        tx = CTransaction()
 825 | +        tx.vin = [CTxIn(outpoint=COutPoint(hash=int('aa' * 32, 16), n=0), scriptSig=b"")]
 826 | +        tx.vout = [CTxOut(nValue=0, scriptPubKey=b"")]
 827 | +        psbt1 = PSBT(PSBTMap({PSBT_GLOBAL_UNSIGNED_TX: tx.serialize()}), [PSBTMap()], [PSBTMap()]).to_base64()

instagibbs commented at 6:46 PM on July 22, 2022:

could use named args here

theStack commented at 7:03 AM on July 23, 2022:

Thanks, done, also changed the PSBT ctor to enforce named arguments.

in test/functional/rpc_psbt.py:829 in b4f332275c outdated

 824 | +        tx = CTransaction()
 825 | +        tx.vin = [CTxIn(outpoint=COutPoint(hash=int('aa' * 32, 16), n=0), scriptSig=b"")]
 826 | +        tx.vout = [CTxOut(nValue=0, scriptPubKey=b"")]
 827 | +        psbt1 = PSBT(PSBTMap({PSBT_GLOBAL_UNSIGNED_TX: tx.serialize()}), [PSBTMap()], [PSBTMap()]).to_base64()
 828 | +        tx.vout[0].nValue += 1  # slightly modify tx
 829 | +        psbt2 = PSBT(PSBTMap({PSBT_GLOBAL_UNSIGNED_TX: tx.serialize()}), [PSBTMap()], [PSBTMap()]).to_base64()

instagibbs commented at 6:46 PM on July 22, 2022:

could use named args here

instagibbs approved

instagibbs commented at 6:46 PM on July 22, 2022: member

ACK b4f332275c593d5c8f454f3e2b7f5d491ef4f520

test: support passing PSBTMaps directly to PSBT ctor

This will allow to create simple PSBTs as short one-liners, without the
need to have three individual assignments (globals, inputs, outputs).

2a428c7989

theStack force-pushed on Jul 23, 2022

theStack commented at 7:05 AM on July 23, 2022: contributor

Force-pushed with changes as suggested by @instagibbs (named arguments for PSBT ctor, adding call for success case of combining two identical PSBTs).

test: check that combining PSBTs with different txs fails 4e616d20c9

theStack force-pushed on Jul 23, 2022

fanquake requested review from instagibbs on Jul 25, 2022

instagibbs commented at 3:19 PM on July 25, 2022: member

reACK https://github.com/bitcoin/bitcoin/pull/25670/commits/4e616d20c9e92b5118a07d4a4b8562fffc66e767

MarcoFalke assigned achow101 on Jul 25, 2022

achow101 commented at 9:22 PM on July 28, 2022: member

ACK 4e616d20c9e92b5118a07d4a4b8562fffc66e767

achow101 merged this on Jul 28, 2022

achow101 closed this on Jul 28, 2022

sidhujag referenced this in commit f4498e8109 on Jul 29, 2022

bitcoin locked this on Jul 28, 2023