Skip to content

test: add P2SH-P2WSH and P2SH 2of3 multisig integration tests - #49

Merged
bubb1es71 merged 1 commit into
bip451:mainfrom
SIDHARTH20K4:add-multisig-tests
Jul 21, 2026
Merged

bubb1es71 merged 1 commit into
bip451:mainfrom
SIDHARTH20K4:add-multisig-tests

Conversation

@SIDHARTH20K4

Copy link
Copy Markdown
Contributor

closes #22
adds integration tests for P2SH-P2WSH and P2SH 2of3 multisig dust inputs:

-test_spend_p2sh_2of3_multisig
-test_spend_p2sh_p2wsh_2of2_multisig
-test_spend_p2sh_p2wsh_2of3_multisig

Copilot AI review requested due to automatic review settings June 3, 2026 10:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds regression tests to ensure cmd_spend can create PSBTs that spend multisig “dust” UTXOs across multiple script/address types, and that the PSBTs can be finalized via multiple wallet signatures.

Changes:

  • Add a 2-of-2 P2SH-P2wSH multisig dust spend test.
  • Add a 2-of-3 P2SH-P2wSH multisig dust spend test.
  • Add a 2-of-3 legacy P2SH multisig dust spend test.

Comment thread src/main.rs Outdated
Comment on lines +1382 to +1383
assert!(result.is_some(), "expected a psbt to be created");
let psbt = result.unwrap();
Comment thread src/main.rs Outdated
Comment on lines +1362 to +1381
let ctx = TestContext::new();

let (addr, desc) = ctx.env.create_multisig(
&[&ctx.wallet1_name, &ctx.wallet2_name],
2,
&AddressType::P2shSegwit,
);

cmd_add(&ctx.secp, &ctx.db, ctx.network, &ctx.rpc_client, desc, 0);
ctx.env.send_to_address(&addr, Amount::from_sat(555));
ctx.env.mine_blocks(1);

let result = cmd_spend(
&ctx.db,
ctx.network,
&ctx.rpc_client,
Amount::from_sat(600),
addr,
false,
);
Comment thread src/main.rs Outdated
Comment on lines +1419 to +1424
// 2-of-3: any 2 of the 3 wallets must sign
let partially_signed = ctx.env.wallet_process_psbt(&ctx.wallet1_name, &psbt);
let fully_signed = ctx
.env
.wallet_process_psbt(&ctx.wallet2_name, &partially_signed);
broadcast_and_assert(&ctx, fully_signed, 1);
@SIDHARTH20K4

Copy link
Copy Markdown
Contributor Author

for the 2of3 tests, should I test multiple signer combinations (e.g. wallet1+wallet2 and wallet1+wallet3) or is a single combination enough for now?

@bubb1es71

Copy link
Copy Markdown
Collaborator

A single combination is enough, I don't see any new code paths being triggered by using different signers.

@bubb1es71

Copy link
Copy Markdown
Collaborator

The other copilot suggestions are valid and worth fixing.

@SIDHARTH20K4

SIDHARTH20K4 commented Jun 10, 2026 •

Copy link
Copy Markdown
Contributor Author

@bubb1es71
I have extracted a run_spend_test_multisig helper function to cut down on the repetition across multisig tests. and also fixed the .expect() suggestions from the earlier review.

@SIDHARTH20K4
SIDHARTH20K4 requested a review from Copilot June 17, 2026 10:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

Comment thread src/main.rs Outdated
Comment on lines +976 to +983
let dust = cmd_list(
&ctx.db,
ctx.network,
&ctx.rpc_client,
Amount::from_sat(dust_sats + 50),
false,
);
debug!("found dust: {:?}", dust.len());
Comment thread src/main.rs Outdated
Comment on lines +1398 to +1423
let (addr, desc) = ctx.env.create_multisig(
&[&ctx.wallet1_name, &ctx.wallet2_name, &ctx.wallet3_name],
2,
&AddressType::Bech32,
&AddressType::P2shSegwit,
);

cmd_add(&ctx.secp, &ctx.db, ctx.network, &ctx.rpc_client, desc, 0);
ctx.env.send_to_address(&addr, Amount::from_sat(555));
ctx.env.mine_blocks(1);

let result = cmd_spend(
&ctx.db,
ctx.network,
&ctx.rpc_client,
Amount::from_sat(600),
addr,
false,
);
let psbt = result.expect("expected a psbt to be created");

// 2-of-3: any 2 of the 3 wallets must sign
let partially_signed = ctx.env.wallet_process_psbt(&ctx.wallet1_name, &psbt);
let fully_signed = ctx
.env
.wallet_process_psbt(&ctx.wallet2_name, &partially_signed);
broadcast_and_assert(&ctx, fully_signed, 1);
Comment thread src/main.rs Outdated
&ctx.db,
ctx.network,
&ctx.rpc_client,
Amount::from_sat(dust_sats + 50),
Comment thread src/main.rs Outdated
&ctx.db,
ctx.network,
&ctx.rpc_client,
Amount::from_sat(dust_sats + 50),
@bubb1es71

Copy link
Copy Markdown
Collaborator

Hi let me know when this is ready for a human re-review.

@SIDHARTH20K4

Copy link
Copy Markdown
Contributor Author

Hi let me know when this is ready for a human re-review.

@bubb1es71
yes this is ready for human re-review!

@bubb1es71 bubb1es71 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ACK 693440d

Thanks for adding these tests and sorry for the delay in reviewing it.

@bubb1es71
bubb1es71 merged commit 693440d into bip451:main Jul 21, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add integration tests with multisig input scripts

3 participants