test: Add test case for spending bare multisig #29120

BrandonOdiwuor commented at 2:03 PM on December 20, 2023: contributor

Fixes https://github.com/bitcoin/bitcoin/issues/29113

DrahtBot commented at 2:03 PM on December 20, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ajtowns, willcl-ark, maflcko, achow101
Stale ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29309 (Add a -permitbarepubkey option by vostrnad)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Dec 20, 2023

in test/functional/mempool_accept.py:378 in b13b95503d outdated

 373 | @@ -372,5 +374,25 @@ def run_test(self):
 374 |              maxfeerate=0,
 375 |          )
 376 |  
 377 | +        self.log.info('Spending a confirmed bare multisig is okay')
 378 | +        node = self.nodes[0]

maflcko commented at 2:07 PM on December 20, 2023:

why re-assign?

BrandonOdiwuor force-pushed on Dec 20, 2023

in test/functional/mempool_accept.py:395 in f21da15b78 outdated

 373 | @@ -372,5 +374,24 @@ def run_test(self):
 374 |              maxfeerate=0,
 375 |          )
 376 |  
 377 | +        self.log.info('Spending a confirmed bare multisig is okay')
 378 | +        address = self.wallet.get_address()
 379 | +        tx = tx_from_hex(raw_tx_reference)
 380 | +        privkey, pubkey = generate_keypair()
 381 | +        tx.vout[0].scriptPubKey = keys_to_multisig_script([pubkey] * 3, k=1)  # Some bare multisig script (1-of-3)

instagibbs commented at 4:50 PM on December 20, 2023:

test looks fine, would be best if we cover all the allowed and border cases:

n-of-m, where 1 <= n <= 3 as well as 1 <= m <= 3 (do two loops)
too-large, i.e. n/m == 4
too-small(?) n/m==0

2/3 may require mining them into blocks directly if the outputs are non-standard to make

ajtowns commented at 1:26 AM on January 8, 2024:

2/3 may require mining them into blocks directly if the outputs are non-standard to make

self.generateblock(node, address, [tx.serialize().hex()]) is already mining the tx-to-be-spent directly into a block.

2-of-3 shouldn't make any difference vs 1-of-3 or even 1-of-1 afaics. ~If the tx-to-be-spent is already confirmed, there shouldn't be any problem having a spend of a 20-of-20 checkmultisig in the mempool, so I don't think n/m=4 is actually an edge case. I think a 0-of-0 checkmultisig is also fine to spend, though is non-standard to create.~ The scriptPubKey being spent is checked for standardness (TX_INPUTS_NOT_STANDARD/bad-txns-nonstandard-inputs) so 0-of-0 and k-of-4 etc don't work.

instagibbs commented at 3:18 PM on January 8, 2024:

If the tx-to-be-spent is already confirmed, there shouldn't be any problem having a spend of a 20-of-20 checkmultisig in the mempool

we have tests for this? If not, this is probably the right thing to cover I guewss :)

BrandonOdiwuor commented at 1:26 PM on April 25, 2024:

response at #29120 (review)

luke-jr commented at 11:00 PM on December 26, 2023: member

Title has a typo. Maybe should test that it works with both values for -permitbaremultisig?

BrandonOdiwuor renamed this:
~~test: Add test case for speding bare multisig~~
test: Add test case for spending bare multisig
on Dec 28, 2023

BrandonOdiwuor force-pushed on Jan 8, 2024

DrahtBot added the label CI failed on Jan 15, 2024

maflcko commented at 11:35 AM on January 17, 2024: member

Could also update the documentation of the setting to clarify that it refers to the relay of transactions which create bare outputs, and not to the relay of transactions spending them?

nsvrn commented at 3:52 AM on January 20, 2024: contributor

I compiled with #28217, which sets default value of permitbaremultisig to false in policy.h and tried to run this test case with 'allowed': False for expected result but the test fails. I would have expected that one to pass the test.

ajtowns commented at 1:41 AM on January 22, 2024: contributor

I compiled with #28217, which sets default value of permitbaremultisig to false in policy.h and tried to run this test case with 'allowed': False for expected result but the test fails. I would have expected that one to pass the test.

The test case already explicitly sets permitbaremultisig=0, so changing the default should have no effect. These spends should always be allowed, so if you change the test to assert that it's not allowed, the test should fail.

BrandonOdiwuor commented at 2:48 PM on February 21, 2024: contributor

@maflcko could you elaborate more on this

#29120 (comment)

Could also update the documentation of the setting to clarify that it refers to the relay of transactions which create bare outputs, and not to the relay of transactions spending them?

maflcko commented at 3:11 PM on February 21, 2024: member

@maflcko could you elaborate more on this

Sure, what is the question?

BrandonOdiwuor commented at 3:13 PM on February 21, 2024: contributor

was asking what you meant by this?

Could also update the documentation of the setting to clarify that it refers to the relay of transactions which create bare outputs, and not to the relay of transactions spending them?

#29120 (comment)

maflcko commented at 3:15 PM on February 21, 2024: member

I mean that the documentation in init.cpp can be clarified that it refers to outputs, not inputs.

BrandonOdiwuor force-pushed on Mar 28, 2024

BrandonOdiwuor commented at 11:37 AM on March 28, 2024: contributor

@maflcko I have updated the PR to include the suggested documentation change

BrandonOdiwuor requested review from maflcko on Mar 28, 2024

BrandonOdiwuor requested review from instagibbs on Mar 28, 2024

DrahtBot removed the label CI failed on Mar 28, 2024

DrahtBot added the label CI failed on Apr 3, 2024

DrahtBot removed the label CI failed on Apr 4, 2024

in test/functional/mempool_accept.py:403 in 9c7b2d0d37 outdated

 384 | +        tx_spend = CTransaction()
 385 | +        tx_spend.vin.append(CTxIn(COutPoint(tx.sha256, 0), b""))
 386 | +        tx_spend.vout.append(CTxOut(tx.vout[0].nValue - int(fee*COIN), script_to_p2wsh_script(CScript([OP_TRUE]))))
 387 | +        tx_spend.rehash()
 388 | +        sign_input_legacy(tx_spend, 0, tx.vout[0].scriptPubKey, privkey, sighash_type=SIGHASH_ALL)
 389 | +        tx_spend.vin[0].scriptSig = bytes(CScript([OP_0])) + tx_spend.vin[0].scriptSig

rkrux commented at 6:03 AM on April 13, 2024:

@BrandonOdiwuor For my understanding, why is prepending bytes(CScript([OP_0])) to the script sig required after having gone through sign_input_legacy?

willcl-ark commented at 11:15 AM on April 25, 2024:

This is due to the OP_CHECKMULTISG off-by-one bug. See for example: https://bitcoin.stackexchange.com/questions/116960/why-checkmultisig-bug-cant-be-solved @instagibbs I think your suggestion here is incorrect? We do need to append the scriptsig otherwise the tx is simply invalid.

BrandonOdiwuor commented at 1:24 PM on April 25, 2024:

Thanks for this clarification @willcl-ark

instagibbs commented at 1:27 PM on April 25, 2024:

@willcl-ark yeah my comment was nonsense, deleted

rkrux commented at 6:05 AM on April 13, 2024: contributor

tACK 9c7b2d0

Make successful, all functional tests successful. Asked a question for clarity.

test: Add test case for spending bare multisig

Co-authored-by: Anthony Towns <aj@erisian.com.au>

e504b1fa1f

BrandonOdiwuor force-pushed on Apr 25, 2024

willcl-ark approved

willcl-ark commented at 2:30 PM on April 25, 2024: member

ACK 9c7b2d0d37b31f01403f7a0f2ea25b60b126c841

Confirmed this tests that when the -permitbaremultisig=0 policy is set, the mempool still permits spending (confirmed) bare multisig outputs. Mempool rejection of this output type is already tested earlier in the same test file.

DrahtBot commented at 12:41 AM on April 26, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/24252720432</sub>

DrahtBot added the label CI failed on Apr 26, 2024

DrahtBot removed the label CI failed on Apr 29, 2024

ajtowns commented at 1:17 PM on April 30, 2024: contributor

ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f ; LGTM and just checking the 1-of-3 case seems fine

DrahtBot requested review from rkrux on Apr 30, 2024

DrahtBot requested review from willcl-ark on Apr 30, 2024

willcl-ark approved

willcl-ark commented at 8:56 AM on May 1, 2024: member

reACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f

Appears to only a rebase on master since my previous ack

git range-diff 9c7b2d0d37b31f01403f7a0f2ea25b60b126c841...e504b1fa1fa4d014b329dea81bfdf1aa55db238f

maflcko commented at 9:39 AM on May 1, 2024: member

utACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f

achow101 commented at 6:36 PM on May 1, 2024: member

ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f

achow101 merged this on May 1, 2024

achow101 closed this on May 1, 2024

BrandonOdiwuor deleted the branch on May 2, 2024

bitcoin locked this on May 2, 2025