← Watch feed
Use a builder for sign_interactive_funding_tx arguments
Public commit record
78/100 · Adequate
What the developer wrote
Authored by Jeffrey Czyz
Use a builder for sign_interactive_funding_tx arguments
The signing helper had accumulated several boolean/option parameters beyond
the two nodes, so call sites passed opaque positional `false`s and bare
`None`s whose meaning was unclear without consulting the signature.
Replace the two overloaded functions with a single `sign_interactive_funding_tx`
taking a `SignInteractiveFundingTxArgs` builder, mirroring `PassAlongPathArgs`:
`new(initiator, acceptor)` defaults to a first-attempt splice on a confirmed
channel with no acceptor contribution, and each non-default behavior is opted
into by a named method (`zero_conf`, `with_acceptor_contribution`, `replacing`).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit is a code cleanup inside the project's test suite. It replaces a helper function with several hard-to-read positional arguments (like bare `false` and `None`) with a 'builder' pattern that names each option. This makes the tests easier to read and maintain, but it does not change any production behavior or fix a security bug.
Recommended action
No security action needed. This is a readability refactor of test helper code. Normal code-review approval is sufficient.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The change refactors sign_interactive_funding_tx and removes sign_interactive_funding_tx_with_acceptor_contribution in lightning/src/ln/splicing_tests.rs. A new SignInteractiveFundingTxArgs builder struct is introduced with named methods (zero_conf, with_acceptor_contribution, replacing) to replace positional bool/Option<Txid> parameters. All call sites in the same test file are updated to use the builder. The actual logic inside the function body is unchanged; only argument packaging is modified. The file remains test-only code.
Changed components
lightning/src/ln/splicing_tests.rsInspect captured patch +116 / −86
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index 16dec1d..2bf7703 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -551,26 +551,59 @@ pub fn complete_interactive_funding_negotiation_for_both<'a, 'b, 'c, 'd>(
assert!(expected_acceptor_scripts.is_empty(), "Not all acceptor outputs were sent");
}
-pub fn sign_interactive_funding_tx<'a, 'b, 'c, 'd>(
- initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, is_0conf: bool,
+/// Arguments for [`sign_interactive_funding_tx`]. [`SignInteractiveFundingTxArgs::new`] defaults to a
+/// first-attempt splice on a confirmed channel where only the initiator contributes; augment with the
+/// builder methods as the scenario requires.
+pub struct SignInteractiveFundingTxArgs<'a, 'b, 'c, 'd> {
+ initiator: &'a Node<'b, 'c, 'd>,
+ acceptor: &'a Node<'b, 'c, 'd>,
+ is_0conf: bool,
+ acceptor_has_contribution: bool,
expected_replaced_txid: Option<Txid>,
+}
+
+impl<'a, 'b, 'c, 'd> SignInteractiveFundingTxArgs<'a, 'b, 'c, 'd> {
+ pub fn new(initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>) -> Self {
+ Self {
+ initiator,
+ acceptor,
+ is_0conf: false,
+ acceptor_has_contribution: false,
+ expected_replaced_txid: None,
+ }
+ }
+
+ /// The channel is zero-conf, so `splice_locked` is exchanged at signing and the initiator's
+ /// `splice_locked` is returned.
+ pub fn zero_conf(mut self) -> Self {
+ self.is_0conf = true;
+ self
+ }
+
+ /// The acceptor contributed inputs and so must also sign the funding transaction.
+ pub fn with_acceptor_contribution(mut self) -> Self {
+ self.acceptor_has_contribution = true;
+ self
+ }
+
+ /// This is an RBF replacing the negotiated candidate `prior_txid`, expected as the prior candidate
+ /// in the `TransactionType::InteractiveFunding` broadcast.
+ pub fn replacing(mut self, prior_txid: Txid) -> Self {
+ self.expected_replaced_txid = Some(prior_txid);
+ self
+ }
+}
+
+pub fn sign_interactive_funding_tx<'a, 'b, 'c, 'd>(
+ args: SignInteractiveFundingTxArgs<'a, 'b, 'c, 'd>,
) -> (Transaction, Option<(msgs::SpliceLocked, PublicKey)>) {
- sign_interactive_funding_tx_with_acceptor_contribution(
+ let SignInteractiveFundingTxArgs {
initiator,
acceptor,
is_0conf,
- false,
+ acceptor_has_contribution,
expected_replaced_txid,
- )
-}
-
-/// `expected_replaced_txid` is the expected txid of the prior negotiated candidate in the
-/// `TransactionType::InteractiveFunding` broadcast: `None` for a first splice attempt; `Some(txid)`
-/// for an RBF replacing that prior negotiated candidate.
-pub fn sign_interactive_funding_tx_with_acceptor_contribution<'a, 'b, 'c, 'd>(
- initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, is_0conf: bool,
- acceptor_has_contribution: bool, expected_replaced_txid: Option<Txid>,
-) -> (Transaction, Option<(msgs::SpliceLocked, PublicKey)>) {
+ } = args;
let node_id_initiator = initiator.node.get_our_node_id();
let node_id_acceptor = acceptor.node.get_our_node_id();
@@ -712,7 +745,8 @@ pub fn splice_channel<'a, 'b, 'c, 'd>(
funding_contribution,
new_funding_script.clone(),
);
- let (splice_tx, splice_locked) = sign_interactive_funding_tx(initiator, acceptor, false, None);
+ let (splice_tx, splice_locked) =
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(initiator, acceptor));
assert!(splice_locked.is_none());
expect_splice_pending_event(initiator, &node_id_acceptor);
@@ -1743,7 +1777,8 @@ fn fails_initiating_concurrent_splices(reconnect: bool) {
}),
);
- let (splice_tx, splice_locked) = sign_interactive_funding_tx(&nodes[0], &nodes[1], false, None);
+ let (splice_tx, splice_locked) =
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]));
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_1_id);
@@ -1947,8 +1982,8 @@ fn do_test_splice_tiebreak(
);
// Sign (acceptor has contribution) and broadcast.
- let (tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], false, true, None,
+ let (tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).with_acceptor_contribution(),
);
assert!(splice_locked.is_none());
@@ -2015,9 +2050,8 @@ fn do_test_splice_tiebreak(
);
// Sign (no acceptor contribution) and broadcast.
- let (tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], false, false, None,
- );
+ let (tx, splice_locked) =
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]));
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
@@ -2064,7 +2098,7 @@ fn do_test_splice_tiebreak(
);
let (new_splice_tx, splice_locked) =
- sign_interactive_funding_tx(&nodes[1], &nodes[0], false, None);
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(&nodes[1], &nodes[0]));
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[1], &node_id_0);
@@ -3580,9 +3614,12 @@ fn do_test_propose_splice_while_disconnected(use_0conf: bool) {
splice_ack.funding_contribution_satoshis,
new_funding_script,
);
- let (splice_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], use_0conf, true, None,
- );
+ let mut args =
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).with_acceptor_contribution();
+ if use_0conf {
+ args = args.zero_conf();
+ }
+ let (splice_tx, splice_locked) = sign_interactive_funding_tx(args);
expect_splice_pending_event(&nodes[0], &node_id_1);
expect_splice_pending_event(&nodes[1], &node_id_0);
@@ -4018,7 +4055,8 @@ fn acceptor_can_cancel_queued_funding_contributed_during_counterparty_splice() {
new_funding_script,
);
- let (splice_tx, splice_locked) = sign_interactive_funding_tx(initiator, acceptor, false, None);
+ let (splice_tx, splice_locked) =
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(initiator, acceptor));
assert!(splice_locked.is_none());
expect_splice_pending_event(initiator, &node_id_acceptor);
assert!(acceptor.node.get_and_clear_pending_events().is_empty());
@@ -6179,10 +6217,8 @@ fn test_splice_rbf_acceptor_basic() {
// Step 10: Sign and broadcast. The prior candidate in the broadcast's
// `TransactionType::InteractiveFunding` must point at the first splice tx it is replacing.
let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
- &nodes[0],
- &nodes[1],
- false,
- Some(first_splice_tx.compute_txid()),
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(first_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
@@ -6280,10 +6316,8 @@ fn test_splice_rbf_discard_unique_contribution() {
);
let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
- &nodes[0],
- &nodes[1],
- false,
- Some(first_splice_tx.compute_txid()),
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(first_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
@@ -6348,10 +6382,8 @@ fn test_splice_rbf_at_high_feerate() {
new_funding_script.clone(),
);
let (rbf_tx_1, splice_locked) = sign_interactive_funding_tx(
- &nodes[0],
- &nodes[1],
- false,
- Some(first_splice_tx.compute_txid()),
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(first_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
@@ -6372,8 +6404,9 @@ fn test_splice_rbf_at_high_feerate() {
contribution,
new_funding_script,
);
- let (_, splice_locked) =
- sign_interactive_funding_tx(&nodes[0], &nodes[1], false, Some(rbf_tx_1.compute_txid()));
+ let (_, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).replacing(rbf_tx_1.compute_txid()),
+ );
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
@@ -6573,8 +6606,9 @@ fn test_splice_rbf_insufficient_feerate_high() {
contribution,
new_funding_script,
);
- let (_, splice_locked) =
- sign_interactive_funding_tx(&nodes[0], &nodes[1], false, Some(splice_tx.compute_txid()));
+ let (_, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).replacing(splice_tx.compute_txid()),
+ );
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
@@ -7159,12 +7193,10 @@ pub fn do_test_splice_rbf_tiebreak(
);
// Sign (acceptor has contribution) and broadcast.
- let (rbf_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0],
- &nodes[1],
- false,
- true,
- Some(first_splice_tx.compute_txid()),
+ let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .with_acceptor_contribution()
+ .replacing(first_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
@@ -7240,12 +7272,9 @@ pub fn do_test_splice_rbf_tiebreak(
);
// Sign (acceptor has no contribution) and broadcast.
- let (rbf_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0],
- &nodes[1],
- false,
- false,
- Some(first_splice_tx.compute_txid()),
+ let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(first_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
@@ -7309,7 +7338,7 @@ pub fn do_test_splice_rbf_tiebreak(
// Sign (no acceptor contribution) and broadcast.
let (new_splice_tx, splice_locked) =
- sign_interactive_funding_tx(&nodes[1], &nodes[0], false, None);
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(&nodes[1], &nodes[0]));
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[1], &node_id_0);
@@ -7491,8 +7520,8 @@ fn test_splice_rbf_acceptor_recontributes() {
new_funding_script.clone(),
);
- let (first_splice_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], false, true, None,
+ let (first_splice_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).with_acceptor_contribution(),
);
assert!(splice_locked.is_none());
@@ -7529,12 +7558,10 @@ fn test_splice_rbf_acceptor_recontributes() {
);
// Step 11: Sign (acceptor has contribution) and broadcast.
- let (rbf_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0],
- &nodes[1],
- false,
- true,
- Some(first_splice_tx.compute_txid()),
+ let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .with_acceptor_contribution()
+ .replacing(first_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
@@ -7626,8 +7653,8 @@ fn test_splice_rbf_after_counterparty_rbf_aborted() {
new_funding_script,
);
- let (_first_splice_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], false, true, None,
+ let (_first_splice_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).with_acceptor_contribution(),
);
assert!(splice_locked.is_none());
@@ -7759,8 +7786,8 @@ fn test_splice_rbf_recontributes_feerate_too_high() {
new_funding_script.clone(),
);
- let (_first_splice_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], false, true, None,
+ let (_first_splice_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).with_acceptor_contribution(),
);
assert!(splice_locked.is_none());
@@ -7846,8 +7873,10 @@ fn test_splice_rbf_sequential() {
funding_contribution_1,
new_funding_script.clone(),
);
- let (splice_tx_1, splice_locked) =
- sign_interactive_funding_tx(&nodes[0], &nodes[1], false, Some(splice_tx_0.compute_txid()));
+ let (splice_tx_1, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(splice_tx_0.compute_txid()),
+ );
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
@@ -7867,8 +7896,10 @@ fn test_splice_rbf_sequential() {
funding_contribution_2,
new_funding_script.clone(),
);
- let (rbf_tx_final, splice_locked) =
- sign_interactive_funding_tx(&nodes[0], &nodes[1], false, Some(splice_tx_1.compute_txid()));
+ let (rbf_tx_final, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(splice_tx_1.compute_txid()),
+ );
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
@@ -7936,8 +7967,9 @@ fn test_splice_rbf_amends_prior_net_positive_contribution_request() {
contribution,
new_funding_script.clone(),
);
- let (tx, splice_locked) =
- sign_interactive_funding_tx(&nodes[0], &nodes[1], false, Some(replaced_txid));
+ let (tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).replacing(replaced_txid),
+ );
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
@@ -8070,8 +8102,9 @@ fn test_splice_rbf_amends_prior_net_negative_contribution_request() {
contribution,
new_funding_script.clone(),
);
- let (tx, splice_locked) =
- sign_interactive_funding_tx(&nodes[0], &nodes[1], false, Some(replaced_txid));
+ let (tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).replacing(replaced_txid),
+ );
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
@@ -8232,8 +8265,8 @@ fn test_splice_rbf_acceptor_contributes_then_disconnects() {
new_funding_script.clone(),
);
- let (_first_splice_tx, splice_locked) = sign_interactive_funding_tx_with_acceptor_contribution(
- &nodes[0], &nodes[1], false, true, None,
+ let (_first_splice_tx, splice_locked) = sign_interactive_funding_tx(
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]).with_acceptor_contribution(),
);
assert!(splice_locked.is_none());
@@ -9103,10 +9136,8 @@ fn test_splice_rbf_rejects_low_feerate_after_several_attempts() {
new_funding_script.clone(),
);
let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
- &nodes[0],
- &nodes[1],
- false,
- Some(prev_splice_tx.compute_txid()),
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(prev_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
@@ -9178,10 +9209,8 @@ fn test_splice_rbf_rejects_own_low_feerate_after_several_attempts() {
new_funding_script.clone(),
);
let (rbf_tx, splice_locked) = sign_interactive_funding_tx(
- &nodes[0],
- &nodes[1],
- false,
- Some(prev_splice_tx.compute_txid()),
+ SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1])
+ .replacing(prev_splice_tx.compute_txid()),
);
assert!(splice_locked.is_none());
expect_splice_pending_event(&nodes[0], &node_id_1);
@@ -9252,7 +9281,8 @@ fn test_no_disconnect_after_splice_completes() {
funding_contribution,
new_funding_script,
);
- let (_, splice_locked) = sign_interactive_funding_tx(&nodes[0], &nodes[1], false, None);
+ let (_, splice_locked) =
+ sign_interactive_funding_tx(SignInteractiveFundingTxArgs::new(&nodes[0], &nodes[1]));
assert!(splice_locked.is_none());
let _node_id_0 = nodes[0].node.get_our_node_id();
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.