Skip to content

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

Merged
achow101 merged 2 commits intobitcoin:masterfrom
theStack:202207-test-add_combinepsbts_fail_test
Jul 28, 2022
Merged

test: check that combining PSBTs with different txs fails#25670
achow101 merged 2 commits intobitcoin:masterfrom
theStack:202207-test-add_combinepsbts_fail_test

Conversation

@theStack
Copy link
Contributor

This PR adds missing test coverage for the combinepsbt RPC, in the case of combining two PSBTs with different transactions:

bitcoin/src/psbt.cpp

Lines 24 to 27 in b8067cd

// Prohibited to merge two PSBTs over different transactions
if (tx->GetHash() != psbt.tx->GetHash()) {
return false;
}

The calling function CombinePSBTs checks for the false return value and then returns the transaction error string PSBT_MISMATCH:

bitcoin/src/psbt.cpp

Lines 433 to 435 in b8067cd

if (!out.Merge(*it)) {
return TransactionError::PSBT_MISMATCH;
}

case TransactionError::PSBT_MISMATCH:
return Untranslated("PSBTs not compatible (different transactions)");

@fanquake fanquake added the Tests label Jul 21, 2022
@theStack theStack force-pushed the 202207-test-add_combinepsbts_fail_test branch from d79d846 to b4f3322 Compare July 21, 2022 20:29
@theStack
Copy link
Contributor Author

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 fanquake requested review from achow101 and instagibbs July 22, 2022 07:48
Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b4f332275c593d5c8f454f3e2b7f5d491ef4f520

This will allow to create simple PSBTs as short one-liners, without the
need to have three individual assignments (globals, inputs, outputs).
@theStack theStack force-pushed the 202207-test-add_combinepsbts_fail_test branch from b4f3322 to 16a0b28 Compare July 23, 2022 07:02
@theStack
Copy link
Contributor Author

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

@theStack theStack force-pushed the 202207-test-add_combinepsbts_fail_test branch from 16a0b28 to 4e616d2 Compare July 23, 2022 07:09
@fanquake fanquake requested a review from instagibbs July 25, 2022 15:13
@instagibbs
Copy link
Member

reACK 4e616d2

@achow101
Copy link
Member

ACK 4e616d2

@achow101 achow101 merged commit 317ef03 into bitcoin:master Jul 28, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 29, 2022
…txs fails

4e616d2 test: check that combining PSBTs with different txs fails (Sebastian Falbesoner)
2a428c7 test: support passing PSBTMaps directly to PSBT ctor (Sebastian Falbesoner)

Pull request description:

  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

ACKs for top commit:
  instagibbs:
    reACK bitcoin@4e616d2
  achow101:
    ACK 4e616d2

Tree-SHA512: 45b2b224b13b44ad69ae62e4bc20f74cab32770cf8127b026ec47a7520f7253148fdbf1fad612afece59e45a6738bef9a351ae87ea98dc83d095cc78f6db0318
@bitcoin bitcoin locked and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants