From f5bfa7c9a6d01c33130ff2f27d7523b380b33b97 Mon Sep 17 00:00:00 2001 From: Amperstrand Date: Wed, 30 Sep 2026 16:37:55 +0200 Subject: [PATCH 1/3] channeld: report fundee splice balance to hsmd in msat, not sat relative_splice_balance_fundee() wrapped the satoshi-denominated opener_relative/accepter_relative splicing fields in amount_msat() unchanged, but the hsmd_setup_channel push_value field it feeds is an amount_msat: the fundee's post-splice balance reached hsmd under-reported by 1000x. A stock hsmd never notices (it signs whatever it is asked to), but any hsmd/signer implementation that validates the balances reported to it reads the honest post-splice commitment as an overpayment and refuses to sign it, wedging fundee-side splices. Convert with amount_sat_to_msat() instead, matching every other consumer of these fields (amount_msat_add_sat_s64), and fail the peer on a negative contribution, which is never valid here. Add channeld/test/run-splice_fundee_msat.c, which pins byte-exact through the real static function that an X-sat contribution is reported as X*1000 msat (never X msat), and that negative and overflowing contributions fail the peer. Fixes: 4649bccbeaedaf462bc1f0cd1276c5a0ace32467 Changelog-Fixed: channeld: splicing was reporting the fundee's post-splice balance to hsmd in satoshis instead of millisatoshis. Signed-off-by: Amperstrand --- channeld/channeld.c | 20 +++- channeld/test/Makefile | 12 ++- channeld/test/run-splice_fundee_msat.c | 136 +++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 5 deletions(-) create mode 100644 channeld/test/run-splice_fundee_msat.c diff --git a/channeld/channeld.c b/channeld/channeld.c index 74781a32cf43..1adfabb5044a 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -3402,7 +3402,8 @@ relative_splice_balance_fundee(struct peer *peer, int chan_input_index) { /* Relative fundee channel balance */ - u64 push_value; + s64 push_value_sat; + struct amount_msat push_value_msat; /* We calculcate the `push_value` to send to the * hsmd, that is the remote amount in the channel @@ -3411,19 +3412,30 @@ relative_splice_balance_fundee(struct peer *peer, case TX_INITIATOR: /* push_value is the fundee relative value so if we open the channel * fundee is the remote node. */ - push_value = peer->splicing->accepter_relative; + push_value_sat = peer->splicing->accepter_relative; break; case TX_ACCEPTER: /* push_value is the fundee relative value so if the remote node open the channel * fundee in this case is the opener. */ - push_value = peer->splicing->opener_relative; + push_value_sat = peer->splicing->opener_relative; break; default: /* This should never happen. Help us to early catch the tx_role change */ abort(); } - return amount_msat(push_value); + /* opener_relative and accepter_relative are satoshi contributions + * (everywhere else they feed amount_msat_add_sat_s64), while the + * hsmd_setup_channel field is amount_msat: convert, don't + * reinterpret. A negative contribution is never valid. */ + if (push_value_sat < 0) + peer_failed_warn(peer->pps, &peer->channel_id, + "splice funding contribution negative"); + if (!amount_sat_to_msat(&push_value_msat, amount_sat(push_value_sat))) + peer_failed_warn(peer->pps, &peer->channel_id, + "splice funding contribution overflow"); + + return push_value_msat; } static struct amount_sat calc_balance(struct peer *peer) diff --git a/channeld/test/Makefile b/channeld/test/Makefile index f683ec31b26f..c2284df13c8d 100644 --- a/channeld/test/Makefile +++ b/channeld/test/Makefile @@ -21,7 +21,7 @@ channeld/test/run-full_channel: \ common/features.o \ common/htlc_state.o \ common/htlc_trim.o \ - common/htlc_tx.o \ + common/htlc_tx.o \ common/initial_commit_tx.o \ common/key_derive.o \ common/msg_queue.o \ @@ -31,6 +31,16 @@ channeld/test/run-full_channel: \ common/setup.o \ common/utils.o +# run-splice_fundee_msat includes channeld.c itself (so we can see statics), +# like the hsmd tests: do not also link channeld.o. The sibling channeld +# objects, hsmd client objects and libcommon.a satisfy the rest. +channeld/test/run-splice_fundee_msat: \ + $(filter-out channeld/channeld.o,$(CHANNELD_OBJS)) \ + $(HSMD_CLIENT_OBJS) \ + $(BITCOIN_OBJS) \ + wire/towire.o \ + wire/fromwire.o + $(CHANNELD_TEST_OBJS): $(CHANNELD_HEADERS) $(CHANNELD_SRC) channeld/test/Makefile check-units: $(CHANNELD_TEST_PROGRAMS:%=unittest/%) diff --git a/channeld/test/run-splice_fundee_msat.c b/channeld/test/run-splice_fundee_msat.c new file mode 100644 index 000000000000..b77ac9fd0ab0 --- /dev/null +++ b/channeld/test/run-splice_fundee_msat.c @@ -0,0 +1,136 @@ +/* Unit test for relative_splice_balance_fundee(). + * + * The opener_relative/accepter_relative splicing fields are satoshi + * amounts, while the hsmd_setup_channel push_value field the function + * feeds is amount_msat. The function must convert (sat -> msat), not + * reinterpret the raw integer: this pins that contract, byte-exact, + * through the real code in channeld.c. + * + * Following hsmd/test/run-bad-request-close.c, we pull in channeld.c + * itself (renaming its main) so the static function is testable. + */ +#include "config.h" +#include +#include +#include +#include +#include +#include + +int unused_main(int argc, char *argv[]); +#define main unused_main +#include "../channeld.c" +#undef main + +/* AUTOGENERATED MOCKS START */ +/* AUTOGENERATED MOCKS END */ + +/* Bits on the wire to hsmd are the raw millisatoshis integer: compare + * the raw field, not a pretty-printed string. */ +static void test_must_accept(const tal_t *ctx, + enum tx_role role, + s64 opener_sat, s64 accepter_sat, + u64 expect_msat) +{ + struct peer *peer = talz(ctx, struct peer); + struct amount_msat got; + + peer->splicing = tal(peer, struct splicing); + peer->splicing->opener_relative = opener_sat; + peer->splicing->accepter_relative = accepter_sat; + peer->pps = talz(peer, struct per_peer_state); + + got = relative_splice_balance_fundee(peer, role, NULL, 0, 0); + + if (got.millisatoshis != expect_msat) + errx(1, "role %u: expected %llu msat, got %llu msat", + role, (unsigned long long)expect_msat, + (unsigned long long)got.millisatoshis); +} + +/* A negative (or overflowing) contribution must never return: the + * real peer_failed_warn() writes what it can and exits nonzero, so we + * fork and insist the child dies. alarm() guards against a hang. */ +static void test_must_refuse(enum tx_role role, + s64 opener_sat, s64 accepter_sat, + const char *desc) +{ + pid_t child = fork(); + struct peer *peer; + + if (child == 0) { + alarm(30); + peer = talz(NULL, struct peer); + peer->splicing = tal(peer, struct splicing); + peer->splicing->opener_relative = opener_sat; + peer->splicing->accepter_relative = accepter_sat; + peer->pps = talz(peer, struct per_peer_state); + /* Must not return. */ + relative_splice_balance_fundee(peer, role, NULL, 0, 0); + _exit(0); + } else { + int status; + + if (waitpid(child, &status, 0) != child) + errx(1, "%s: waitpid failed", desc); + if (WIFEXITED(status) && WEXITSTATUS(status) == 0) + errx(1, "%s: returned instead of failing peer", desc); + if (!(WIFEXITED(status) || WIFSIGNALED(status))) + errx(1, "%s: child neither exited nor signalled", desc); + } +} + +int main(int argc, const char *argv[]) +{ + static const s64 amounts[] = { + 0, 1, 2, 999, 1000, 123456, 1000000, 2100000000000000 + }; + common_setup(argv[0]); + + /* The splice initiator reports the splice accepter's contribution + * and vice versa; distinct values pin which field each role + * reads. */ + for (size_t i = 0; i < ARRAY_SIZE(amounts); i++) { + s64 sat = amounts[i]; + u64 expect_msat = (u64)sat * 1000; + + test_must_accept(tmpctx, TX_INITIATOR, 7, sat, expect_msat); + test_must_accept(tmpctx, TX_ACCEPTER, sat, 7, expect_msat); + + /* The sat/msat wrap fingerprint: for a nonzero satoshi + * input the reported msat must never equal the satoshi + * number -- that is exactly the 1000x-under-report bug + * this test guards against. */ + if (sat > 0) { + struct peer *peer = talz(tmpctx, struct peer); + struct amount_msat got; + + peer->splicing = tal(peer, struct splicing); + peer->splicing->opener_relative = 7; + peer->splicing->accepter_relative = sat; + peer->pps = talz(peer, struct per_peer_state); + got = relative_splice_balance_fundee(peer, + TX_INITIATOR, + NULL, 0, 0); + if (amount_msat_eq(got, amount_msat((u64)sat))) + errx(1, "%llu sat reported as %llu msat: " + "raw sat wrapped into msat", + (unsigned long long)sat, + (unsigned long long)got.millisatoshis); + } + } + + /* Negative contributions: never valid, must fail the peer. */ + test_must_refuse(TX_INITIATOR, 7, -1, "accepter -1"); + test_must_refuse(TX_ACCEPTER, -1, 7, "opener -1"); + test_must_refuse(TX_INITIATOR, 7, -100000, "accepter -100000"); + test_must_refuse(TX_INITIATOR, 7, INT64_MIN, + "accepter INT64_MIN"); + /* A contribution whose msat conversion overflows must also fail + * the peer rather than wrap. */ + test_must_refuse(TX_ACCEPTER, INT64_MAX, 7, + "opener INT64_MAX"); + + common_shutdown(); + return 0; +} From 10361ee48dd83af37b82515be2ab3a9d46ca8b7f Mon Sep 17 00:00:00 2001 From: Amperstrand Date: Wed, 30 Sep 2026 16:38:14 +0200 Subject: [PATCH 2/3] channeld: splice push_value is the fundee's full post-splice balance hsmd_setup_channel's push_value is the fundee's balance at the start of a channel era: at channel open that is the pushed amount, and for a splice it is the fundee's pre-splice balance plus its funding contribution. Reporting a splice-role-selected contribution alone is wrong twice over: - The contribution was selected by SPLICE role, but the fundee is defined by CHANNEL role: the side that did not open the channel. The splice initiator is not necessarily the channel opener, so whenever the channel fundee is reporting -- whether it initiated the splice or accepted an opener-initiated one -- the splice roles are inverted with respect to the channel roles and the channel opener's contribution was reported instead of the fundee's. - A fundee that already held a balance at splice time was under-reported by exactly that balance. Neither defect is caught by the existing tests: stock hsmd signs unconditionally, so no test observes what hsmd was told. Select the contribution by channel opener role and add the fundee's pre-splice balance from the channel view (view[].owed[], an upper bound of the fundee's first post-splice commitment output, since pending HTLCs only reduce it). A negative contribution is a splice-out and subtracts; only an out-of-range result fails the peer. Extend the unit test with the channel/splice role matrix and the pre-splice balance cases, and add test_splice_fundee_with_balance, which routes 500k sat to the fundee before splicing in on top, so the reported balance is exercised on top of a pre-splice balance. Fixes: 4649bccbeaedaf462bc1f0cd1276c5a0ace32467 Changelog-Fixed: splicing: hsmd is now given the fundee's full post-splice balance, not just its funding contribution. Signed-off-by: Amperstrand --- channeld/channeld.c | 56 ++++---- channeld/test/run-splice_fundee_msat.c | 183 +++++++++++++++---------- tests/test_splicing.py | 48 +++++++ 3 files changed, 182 insertions(+), 105 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 1adfabb5044a..a68cbec8cb01 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -3401,39 +3401,31 @@ relative_splice_balance_fundee(struct peer *peer, int chan_output_index, int chan_input_index) { - /* Relative fundee channel balance */ - s64 push_value_sat; - struct amount_msat push_value_msat; - - /* We calculcate the `push_value` to send to the - * hsmd, that is the remote amount in the channel - * after the splice. */ - switch (our_role) { - case TX_INITIATOR: - /* push_value is the fundee relative value so if we open the channel - * fundee is the remote node. */ - push_value_sat = peer->splicing->accepter_relative; - break; - case TX_ACCEPTER: - /* push_value is the fundee relative value so if the remote node open the channel - * fundee in this case is the opener. */ - push_value_sat = peer->splicing->opener_relative; - break; - default: - /* This should never happen. Help us to early catch the tx_role change */ - abort(); - } - - /* opener_relative and accepter_relative are satoshi contributions - * (everywhere else they feed amount_msat_add_sat_s64), while the - * hsmd_setup_channel field is amount_msat: convert, don't - * reinterpret. A negative contribution is never valid. */ - if (push_value_sat < 0) - peer_failed_warn(peer->pps, &peer->channel_id, - "splice funding contribution negative"); - if (!amount_sat_to_msat(&push_value_msat, amount_sat(push_value_sat))) + /* The fundee is the side that did not open the channel. Pick its + * contribution by channel role, not splice role: the splice + * initiator is not necessarily the channel opener. */ + enum side fundee_side = peer->channel->opener == LOCAL ? REMOTE : LOCAL; + bool fundee_is_splice_initiator = + (fundee_side == LOCAL) == (our_role == TX_INITIATOR); + s64 fundee_contribution = fundee_is_splice_initiator + ? peer->splicing->opener_relative + : peer->splicing->accepter_relative; + + /* hsmd_setup_channel's push_value is the fundee's balance at the + * start of the new channel era: its pre-splice balance plus its + * funding contribution. opener_relative/accepter_relative are + * satoshi amounts (everywhere else they feed + * amount_msat_add_sat_s64); a negative contribution is a + * splice-out and subtracts. */ + struct amount_msat push_value_msat + = peer->channel->view[LOCAL].owed[fundee_side]; + + if (fundee_contribution == INT64_MIN || + !amount_msat_add_sat_s64(&push_value_msat, push_value_msat, + fundee_contribution)) peer_failed_warn(peer->pps, &peer->channel_id, - "splice funding contribution overflow"); + "splice funding contribution out of range" + " for fundee balance"); return push_value_msat; } diff --git a/channeld/test/run-splice_fundee_msat.c b/channeld/test/run-splice_fundee_msat.c index b77ac9fd0ab0..5dc21f47a717 100644 --- a/channeld/test/run-splice_fundee_msat.c +++ b/channeld/test/run-splice_fundee_msat.c @@ -1,10 +1,13 @@ /* Unit test for relative_splice_balance_fundee(). * - * The opener_relative/accepter_relative splicing fields are satoshi - * amounts, while the hsmd_setup_channel push_value field the function - * feeds is amount_msat. The function must convert (sat -> msat), not - * reinterpret the raw integer: this pins that contract, byte-exact, - * through the real code in channeld.c. + * hsmd_setup_channel's push_value is the fundee's balance at the start + * of the new channel era: its pre-splice balance plus its funding + * contribution. The splicing opener_relative/accepter_relative fields + * are satoshi amounts selected by SPLICE role, so the function must + * (a) pick the contribution of the side that did not open the CHANNEL, + * (b) convert sat -> msat, and (c) add the pre-splice balance. This + * pins that contract, byte-exact, through the real code in + * channeld.c. * * Following hsmd/test/run-bad-request-close.c, we pull in channeld.c * itself (renaming its main) so the static function is testable. @@ -25,48 +28,72 @@ int unused_main(int argc, char *argv[]); /* AUTOGENERATED MOCKS START */ /* AUTOGENERATED MOCKS END */ -/* Bits on the wire to hsmd are the raw millisatoshis integer: compare - * the raw field, not a pretty-printed string. */ -static void test_must_accept(const tal_t *ctx, - enum tx_role role, - s64 opener_sat, s64 accepter_sat, - u64 expect_msat) +static struct peer *make_peer(const tal_t *ctx, + enum side opener, + u64 owed_local_msat, u64 owed_remote_msat, + s64 opener_relative, s64 accepter_relative) { struct peer *peer = talz(ctx, struct peer); - struct amount_msat got; + + peer->channel = talz(peer, struct channel); + peer->channel->opener = opener; + peer->channel->view[LOCAL].owed[LOCAL].millisatoshis = owed_local_msat; + peer->channel->view[LOCAL].owed[REMOTE].millisatoshis = owed_remote_msat; peer->splicing = tal(peer, struct splicing); - peer->splicing->opener_relative = opener_sat; - peer->splicing->accepter_relative = accepter_sat; + peer->splicing->opener_relative = opener_relative; + peer->splicing->accepter_relative = accepter_relative; peer->pps = talz(peer, struct per_peer_state); + return peer; +} + +/* Bits on the wire to hsmd are the raw millisatoshis integer: compare + * the raw field, not a pretty-printed string. */ +static u64 push_value_msat(enum side opener, enum tx_role our_role, + u64 owed_local_msat, u64 owed_remote_msat, + s64 opener_relative, s64 accepter_relative) +{ + struct peer *peer = make_peer(tmpctx, opener, + owed_local_msat, owed_remote_msat, + opener_relative, accepter_relative); + struct amount_msat got; - got = relative_splice_balance_fundee(peer, role, NULL, 0, 0); + got = relative_splice_balance_fundee(peer, our_role, NULL, 0, 0); + return got.millisatoshis; +} - if (got.millisatoshis != expect_msat) - errx(1, "role %u: expected %llu msat, got %llu msat", - role, (unsigned long long)expect_msat, - (unsigned long long)got.millisatoshis); +static void test_must_be(enum side opener, enum tx_role our_role, + u64 owed_local_msat, u64 owed_remote_msat, + s64 opener_relative, s64 accepter_relative, + u64 expect_msat, const char *desc) +{ + u64 got = push_value_msat(opener, our_role, + owed_local_msat, owed_remote_msat, + opener_relative, accepter_relative); + if (got != expect_msat) + errx(1, "%s: expected %llu msat, got %llu msat", + desc, (unsigned long long)expect_msat, + (unsigned long long)got); } -/* A negative (or overflowing) contribution must never return: the - * real peer_failed_warn() writes what it can and exits nonzero, so we +/* An out-of-range contribution must never return: the real + * peer_failed_warn() writes what it can and exits nonzero, so we * fork and insist the child dies. alarm() guards against a hang. */ -static void test_must_refuse(enum tx_role role, - s64 opener_sat, s64 accepter_sat, +static void test_must_refuse(enum side opener, enum tx_role our_role, + u64 owed_local_msat, u64 owed_remote_msat, + s64 opener_relative, s64 accepter_relative, const char *desc) { pid_t child = fork(); - struct peer *peer; if (child == 0) { + struct peer *peer; alarm(30); - peer = talz(NULL, struct peer); - peer->splicing = tal(peer, struct splicing); - peer->splicing->opener_relative = opener_sat; - peer->splicing->accepter_relative = accepter_sat; - peer->pps = talz(peer, struct per_peer_state); + peer = make_peer(NULL, opener, + owed_local_msat, owed_remote_msat, + opener_relative, accepter_relative); /* Must not return. */ - relative_splice_balance_fundee(peer, role, NULL, 0, 0); + relative_splice_balance_fundee(peer, our_role, NULL, 0, 0); _exit(0); } else { int status; @@ -82,54 +109,64 @@ static void test_must_refuse(enum tx_role role, int main(int argc, const char *argv[]) { - static const s64 amounts[] = { - 0, 1, 2, 999, 1000, 123456, 1000000, 2100000000000000 - }; common_setup(argv[0]); - /* The splice initiator reports the splice accepter's contribution - * and vice versa; distinct values pin which field each role - * reads. */ - for (size_t i = 0; i < ARRAY_SIZE(amounts); i++) { - s64 sat = amounts[i]; - u64 expect_msat = (u64)sat * 1000; - - test_must_accept(tmpctx, TX_INITIATOR, 7, sat, expect_msat); - test_must_accept(tmpctx, TX_ACCEPTER, sat, 7, expect_msat); - - /* The sat/msat wrap fingerprint: for a nonzero satoshi - * input the reported msat must never equal the satoshi - * number -- that is exactly the 1000x-under-report bug - * this test guards against. */ - if (sat > 0) { - struct peer *peer = talz(tmpctx, struct peer); - struct amount_msat got; - - peer->splicing = tal(peer, struct splicing); - peer->splicing->opener_relative = 7; - peer->splicing->accepter_relative = sat; - peer->pps = talz(peer, struct per_peer_state); - got = relative_splice_balance_fundee(peer, - TX_INITIATOR, - NULL, 0, 0); - if (amount_msat_eq(got, amount_msat((u64)sat))) - errx(1, "%llu sat reported as %llu msat: " - "raw sat wrapped into msat", - (unsigned long long)sat, - (unsigned long long)got.millisatoshis); + /* The contribution comes from the side that did not open the + * channel, whatever the splice roles: distinct contributions and + * distinct pre-splice balances pin both the party and the base. + * + * opener_relative (the splice initiator's contribution) is + * 300 sat = 300000 msat; accepter_relative is 70000 sat = + * 70000000 msat; owed[LOCAL] is 123 msat, owed[REMOTE] 456 msat. + */ + test_must_be(LOCAL, TX_INITIATOR, 123, 456, 300, 70000, + 456 + 70000000, "we open; we initiate; fundee=remote accepter"); + test_must_be(LOCAL, TX_ACCEPTER, 123, 456, 300, 70000, + 456 + 300000, "we open; they initiate; fundee=remote initiator"); + test_must_be(REMOTE, TX_INITIATOR, 123, 456, 300, 70000, + 123 + 300000, "they open; we initiate; fundee=local initiator"); + test_must_be(REMOTE, TX_ACCEPTER, 123, 456, 300, 70000, + 123 + 70000000, "they open; they initiate; fundee=local accepter"); + + /* A fundee holding a pre-splice balance must have it reported, + * not just its contribution: 500000 sat routed earlier = + * 500000000 msat, plus a 1000 sat splice-in contribution. */ + test_must_be(LOCAL, TX_INITIATOR, 0, 500000000, 0, 1000, + 500000000 + 1000000, "fundee with balance, splice-in"); + + /* A negative contribution is a splice-out: it subtracts from the + * pre-splice balance. */ + test_must_be(LOCAL, TX_INITIATOR, 0, 500000000, 0, -4000, + 500000000 - 4000000, "fundee with balance, splice-out"); + + /* The sat/msat wrap fingerprint: with zero pre-splice balance a + * nonzero satoshi contribution must be reported as sat*1000 msat, + * never as the raw satoshi number -- that equality is exactly the + * 1000x-under-report bug this test guards against. */ + { + static const s64 amounts[] = { + 1, 2, 999, 1000, 123456, 1000000, 2100000000000000 + }; + for (size_t i = 0; i < ARRAY_SIZE(amounts); i++) { + s64 sat = amounts[i]; + test_must_be(LOCAL, TX_INITIATOR, 0, 0, 7, sat, + (u64)sat * 1000, "fresh fundee, remote accepter"); + test_must_be(REMOTE, TX_INITIATOR, 0, 0, sat, 7, + (u64)sat * 1000, "fresh fundee, local initiator"); } } - /* Negative contributions: never valid, must fail the peer. */ - test_must_refuse(TX_INITIATOR, 7, -1, "accepter -1"); - test_must_refuse(TX_ACCEPTER, -1, 7, "opener -1"); - test_must_refuse(TX_INITIATOR, 7, -100000, "accepter -100000"); - test_must_refuse(TX_INITIATOR, 7, INT64_MIN, - "accepter INT64_MIN"); - /* A contribution whose msat conversion overflows must also fail - * the peer rather than wrap. */ - test_must_refuse(TX_ACCEPTER, INT64_MAX, 7, - "opener INT64_MAX"); + /* Out-of-range contributions must fail the peer, not wrap: + * underflow (splice-out beyond the fundee's balance), INT64_MIN + * (negation would overflow), and overflow past u64 msat. */ + test_must_refuse(LOCAL, TX_INITIATOR, 0, 1000, 0, -100000, + "splice-out beyond balance"); + test_must_refuse(LOCAL, TX_INITIATOR, 0, 0, 0, INT64_MIN, + "contribution INT64_MIN"); + test_must_refuse(REMOTE, TX_INITIATOR, 0, 0, INT64_MAX, 7, + "contribution INT64_MAX"); + test_must_refuse(REMOTE, TX_INITIATOR, UINT64_MAX, 0, 1000, 7, + "balance + contribution overflow"); common_shutdown(); return 0; diff --git a/tests/test_splicing.py b/tests/test_splicing.py index a2799247c5cc..d5bfbdfe2a0c 100644 --- a/tests/test_splicing.py +++ b/tests/test_splicing.py @@ -991,3 +991,51 @@ def test_splice_candidate_spent_before_lock(node_factory, bitcoind): chan = only_one(l1.rpc.listpeerchannels()['channels']) assert chan['state'] == 'ONCHAIN' assert chan['funding_txid'] == splice_txid + + +@pytest.mark.openchannel('v1') +@pytest.mark.openchannel('v2') +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +def test_splice_fundee_with_balance(node_factory, bitcoind): + """Splice a channel whose fundee already holds a balance. + + The hsmd setup_channel push_value must report the fundee's full + post-splice balance (prior balance + splice contribution), not just + its funding contribution. Other splice tests do exercise funded + fundees, but none can observe this value: stock hsmd signs + unconditionally and no test asserts what hsmd was told. + """ + l1, l2 = node_factory.line_graph(2, fundamount=1000000, wait_for_announce=True) + + # Give the fundee a pre-splice balance: 500k sat routed to l2. + inv = l2.rpc.invoice(500000000, 'fundee-balance', 'fundee balance') + l1.rpc.xpay(inv['bolt11']) + wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['spendable_msat'] > 450000000) + pre = only_one(l2.rpc.listpeerchannels()['channels'])['spendable_msat'] + + chan_id = l1.get_channel_id(l2) + + # The funder splices in on top of the fundee's balance. + funds_result = l1.rpc.fundpsbt("111722sat", 0, 0, excess_as_change=True) + result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt']) + result = l1.rpc.splice_update(chan_id, result['psbt']) + assert result['commitments_secured'] is False + result = l1.rpc.splice_update(chan_id, result['psbt']) + assert result['commitments_secured'] is True + result = l1.rpc.signpsbt(result['psbt']) + result = l1.rpc.splice_signed(chan_id, result['signed_psbt']) + + l1.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE') + l2.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE') + bitcoind.generate_block(6, wait_for_mempool=1) + l1.daemon.wait_for_log(r'CHANNELD_AWAITING_SPLICE to CHANNELD_NORMAL') + l2.daemon.wait_for_log(r'CHANNELD_AWAITING_SPLICE to CHANNELD_NORMAL') + + # The fundee keeps its balance across the splice (allow for the + # larger channel reserve on the spliced capacity). + wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['spendable_msat'] >= pre - 5000000) + + # And the channel still routes: pay the fundee another invoice. + inv = l2.rpc.invoice(100000000, 'fundee-balance-2', 'fundee balance 2') + l1.rpc.xpay(inv['bolt11']) + wait_for(lambda: only_one(l2.rpc.listinvoices('fundee-balance-2')['invoices'])['status'] == 'paid') From b397c987e4ff6e1428d5167e38e79487a5310e68 Mon Sep 17 00:00:00 2001 From: Amperstrand Date: Thu, 1 Oct 2026 07:58:12 +0200 Subject: [PATCH 3/3] channeld: include fundee-owned pending HTLCs in the splice push_value hsmd_setup_channel's push_value is the fundee's balance at the start of the new channel era, and a fundee may hold HTLCs in flight when the splice is set up. The settled owed[] alone under-reports the fundee in one resolution direction: a fundee-owned HTLC that fails back returns its escrow to the fundee, raising its output above the settled at-setup balance. A signer that validates the reported balance then reads the honest post-splice commitment as an overpayment and refuses it, wedging the splice. Add the HTLCs pending at splice setup and attributable to the fundee (bucketed by owner side, the same bucketing check_balances uses for its pending_htlcs; callers run after check_balances so the set is final) to the total. The other side's pending HTLCs never count. A negative total -- settled + pending + contribution below zero -- remains a genuine over-draw and fails the peer; nothing is wrapped or clamped. Rails: owed + fundee-owned pending + contribution lands exactly (both role shapes, other side's HTLCs excluded; RED against the settled-only total: 400M vs 500M msat), and a cushioned true-negative total refuses. Changelog-Fixed: splicing: the fundee balance reported to hsmd now includes the fundee's pending HTLCs, not just its settled balance. Signed-off-by: Amperstrand --- channeld/channeld.c | 41 ++++++++++--- channeld/test/Makefile | 2 +- channeld/test/run-splice_fundee_msat.c | 84 ++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 10 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index a68cbec8cb01..bd5b3adb7d28 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -3401,25 +3401,48 @@ relative_splice_balance_fundee(struct peer *peer, int chan_output_index, int chan_input_index) { - /* The fundee is the side that did not open the channel. Pick its - * contribution by channel role, not splice role: the splice - * initiator is not necessarily the channel opener. */ + /* hsmd_setup_channel's push_value is the fundee's TOTAL balance at + * the start of the new channel era: pre-splice settled balance, + * plus HTLCs pending at splice setup attributable to the fundee, + * plus its funding contribution. A signer that validates the + * balances reported to it reads this as the fundee's entitlement + * for the new era, so it must cover the fundee's balance in every + * pending-HTLC resolution direction (a fundee-owned HTLC failing + * back raises the fundee output above its settled at-setup + * balance). A negative total is a genuine over-draw and fails the + * peer; nothing is ever wrapped or clamped. */ enum side fundee_side = peer->channel->opener == LOCAL ? REMOTE : LOCAL; bool fundee_is_splice_initiator = (fundee_side == LOCAL) == (our_role == TX_INITIATOR); s64 fundee_contribution = fundee_is_splice_initiator ? peer->splicing->opener_relative : peer->splicing->accepter_relative; + struct htlc_map_iter it; + const struct htlc *htlc; - /* hsmd_setup_channel's push_value is the fundee's balance at the - * start of the new channel era: its pre-splice balance plus its - * funding contribution. opener_relative/accepter_relative are - * satoshi amounts (everywhere else they feed - * amount_msat_add_sat_s64); a negative contribution is a - * splice-out and subtracts. */ + /* The fundee's pre-splice settled balance; views agree on owed[]. */ struct amount_msat push_value_msat = peer->channel->view[LOCAL].owed[fundee_side]; + /* HTLCs pending at splice setup attributable to the fundee, + * selected by owner side (the same bucketing check_balances uses + * for its pending_htlcs). Callers run after check_balances, so + * the view and htlc set are final for this round. */ + for (htlc = htlc_map_first(peer->channel->htlcs, &it); + htlc; + htlc = htlc_map_next(peer->channel->htlcs, &it)) { + if (htlc_owner(htlc) != fundee_side) + continue; + if (!amount_msat_accumulate(&push_value_msat, htlc->amount)) + peer_failed_warn(peer->pps, &peer->channel_id, + "Unable to add HTLC balance"); + } + + /* opener_relative/accepter_relative are satoshi contributions + * (everywhere else they feed amount_msat_add_sat_s64); a negative + * contribution is a splice-out and subtracts. A negative total, + * or an add that overflows, is an over-draw the peer is failed + * for rather than reported wrapped. */ if (fundee_contribution == INT64_MIN || !amount_msat_add_sat_s64(&push_value_msat, push_value_msat, fundee_contribution)) diff --git a/channeld/test/Makefile b/channeld/test/Makefile index c2284df13c8d..7cbcb4d38c88 100644 --- a/channeld/test/Makefile +++ b/channeld/test/Makefile @@ -21,7 +21,7 @@ channeld/test/run-full_channel: \ common/features.o \ common/htlc_state.o \ common/htlc_trim.o \ - common/htlc_tx.o \ + common/htlc_tx.o \ common/initial_commit_tx.o \ common/key_derive.o \ common/msg_queue.o \ diff --git a/channeld/test/run-splice_fundee_msat.c b/channeld/test/run-splice_fundee_msat.c index 5dc21f47a717..d0d7ebc88d85 100644 --- a/channeld/test/run-splice_fundee_msat.c +++ b/channeld/test/run-splice_fundee_msat.c @@ -39,6 +39,9 @@ static struct peer *make_peer(const tal_t *ctx, peer->channel->opener = opener; peer->channel->view[LOCAL].owed[LOCAL].millisatoshis = owed_local_msat; peer->channel->view[LOCAL].owed[REMOTE].millisatoshis = owed_remote_msat; + /* The real splice path always has a live htlc map; the total + * iterates it (the check_balances pending_htlcs bucketing). */ + peer->channel->htlcs = new_htable(peer->channel, htlc_map); peer->splicing = tal(peer, struct splicing); peer->splicing->opener_relative = opener_relative; @@ -47,6 +50,22 @@ static struct peer *make_peer(const tal_t *ctx, return peer; } +/* Install a pending HTLC owned by `owner`. Fully-acked states + * (SENT_ADD_ACK_REVOCATION -> LOCAL, RCVD_ADD_ACK_REVOCATION -> + * REMOTE) are what htlc_owner() buckets by; the enum's SENT/RCVD + * prefixes alone do NOT pick the owner. */ +static void add_pending_htlc(struct peer *peer, u64 id, + struct amount_msat amount, enum side owner) +{ + struct htlc *htlc = talz(peer->channel, struct htlc); + + htlc->id = id; + htlc->amount = amount; + htlc->state = owner == LOCAL ? SENT_ADD_ACK_REVOCATION + : RCVD_ADD_ACK_REVOCATION; + htlc_map_add(peer->channel->htlcs, htlc); +} + /* Bits on the wire to hsmd are the raw millisatoshis integer: compare * the raw field, not a pretty-printed string. */ static u64 push_value_msat(enum side opener, enum tx_role our_role, @@ -139,6 +158,45 @@ int main(int argc, const char *argv[]) test_must_be(LOCAL, TX_INITIATOR, 0, 500000000, 0, -4000, 500000000 - 4000000, "fundee with balance, splice-out"); + /* HTLCs pending at setup and owned by the fundee are part of the + * fundee's total (a failing-back fundee-owned HTLC raises the + * fundee output above its settled balance); the other side's + * pending HTLCs never count. Fundee LOCAL: owed 300k sat + + * fundee-owned pending 75k+25k sat + 100k sat contribution = 500k + * sat (other side's 400k sat pending excluded). */ + { + struct peer *peer = make_peer(tmpctx, REMOTE, + 300000000, 0, 100000, 0); + struct amount_msat got; + + add_pending_htlc(peer, 0, amount_msat(75000000), LOCAL); + add_pending_htlc(peer, 1, amount_msat(25000000), LOCAL); + add_pending_htlc(peer, 2, amount_msat(400000000), REMOTE); + got = relative_splice_balance_fundee(peer, TX_INITIATOR, + NULL, 0, 0); + if (got.millisatoshis != 500000000) + errx(1, "pending-term LOCAL: expected 500000000 msat," + " got %llu msat", + (unsigned long long)got.millisatoshis); + } + + /* Fundee REMOTE: owed 300k sat + fundee-owned pending 100k sat - + * 50k sat withdrawal = 350k sat (other side's pending excluded). */ + { + struct peer *peer = make_peer(tmpctx, LOCAL, + 0, 300000000, 0, -50000); + struct amount_msat got; + + add_pending_htlc(peer, 0, amount_msat(100000000), REMOTE); + add_pending_htlc(peer, 1, amount_msat(400000000), LOCAL); + got = relative_splice_balance_fundee(peer, TX_INITIATOR, + NULL, 0, 0); + if (got.millisatoshis != 350000000) + errx(1, "pending-term REMOTE: expected 350000000 msat," + " got %llu msat", + (unsigned long long)got.millisatoshis); + } + /* The sat/msat wrap fingerprint: with zero pre-splice balance a * nonzero satoshi contribution must be reported as sat*1000 msat, * never as the raw satoshi number -- that equality is exactly the @@ -168,6 +226,32 @@ int main(int argc, const char *argv[]) test_must_refuse(REMOTE, TX_INITIATOR, UINT64_MAX, 0, 1000, 7, "balance + contribution overflow"); + /* A true negative TOTAL fails the peer even when pending HTLCs + * cushion part of the withdrawal: owed 100k sat + fundee-owned + * pending 200k sat - 400k sat withdrawal = -100k sat. */ + { + pid_t child = fork(); + + if (child == 0) { + struct peer *peer; + alarm(30); + peer = make_peer(NULL, REMOTE, 100000000, 0, -400000, 0); + add_pending_htlc(peer, 0, amount_msat(200000000), LOCAL); + /* Must not return. */ + relative_splice_balance_fundee(peer, TX_INITIATOR, + NULL, 0, 0); + _exit(0); + } else { + int status; + + if (waitpid(child, &status, 0) != child) + errx(1, "true-negative total: waitpid failed"); + if (WIFEXITED(status) && WEXITSTATUS(status) == 0) + errx(1, "true-negative total: returned instead" + " of failing peer"); + } + } + common_shutdown(); return 0; }