Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

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})

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 b4f3322

test/functional/rpc_psbt.py Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
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.

None yet

4 participants