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.

Sign up for GitHub

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

Splice: Better balance checking #6748

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Splice: Better balance checking #6748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions channeld/channeld.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2961,13 +2961,35 @@ static struct amount_sat check_balances(struct peer *peer,
funding_amount_res, min_multiplied;
struct amount_msat funding_amount,
initiator_fee, accepter_fee;
struct amount_msat in[NUM_TX_ROLES], out[NUM_TX_ROLES];
struct amount_msat in[NUM_TX_ROLES], out[NUM_TX_ROLES],
pending_htlcs[NUM_TX_ROLES];
struct htlc_map_iter it;
const struct htlc *htlc;
bool opener = our_role == TX_INITIATOR;
u8 *msg;

/* The channel funds less any pending htlcs */
in[TX_INITIATOR] = peer->channel->view->owed[opener ? LOCAL : REMOTE];
in[TX_ACCEPTER] = peer->channel->view->owed[opener ? REMOTE : LOCAL];

/* pending_htlcs holds the value of all pending htlcs for each side */
pending_htlcs[TX_INITIATOR] = AMOUNT_MSAT(0);
pending_htlcs[TX_ACCEPTER] = AMOUNT_MSAT(0);
for (htlc = htlc_map_first(peer->channel->htlcs, &it);
htlc;
htlc = htlc_map_next(peer->channel->htlcs, &it)) {
struct amount_msat *itr;

if (htlc_owner(htlc) == opener ? LOCAL : REMOTE)
itr = &pending_htlcs[TX_INITIATOR];
else
itr = &pending_htlcs[TX_ACCEPTER];

if (!amount_msat_add(itr, *itr, htlc->amount))
peer_failed_warn(peer->pps, &peer->channel_id,
"Unable to add HTLC balance");
}

for (size_t i = 0; i < psbt->num_inputs; i++)
if (i != chan_input_index)
add_amount_to_side(peer, in,
Expand All @@ -2985,12 +3007,22 @@ static struct amount_sat check_balances(struct peer *peer,
psbt_output_get_amount(psbt, i),
&psbt->outputs[i].unknowns);

/* Calculate total channel output amount */
/* Calculate original channel output amount */
if (!amount_msat_add(&funding_amount,
peer->channel->view->owed[LOCAL],
peer->channel->view->owed[REMOTE]))
peer_failed_warn(peer->pps, &peer->channel_id,
"Unable to calculate starting channel amount");
if (!amount_msat_add(&funding_amount,
funding_amount,
pending_htlcs[TX_INITIATOR]))
peer_failed_warn(peer->pps, &peer->channel_id,
"Unable to calculate starting channel amount");
if (!amount_msat_add(&funding_amount,
funding_amount,
pending_htlcs[TX_ACCEPTER]))
peer_failed_warn(peer->pps, &peer->channel_id,
"Unable to calculate starting channel amount");
Comment on lines +3016 to +3025
Copy link
Contributor

Choose a reason for hiding this comment

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

I cried in the last couple of days to understand this logic here, and I am still not sure 100% for what this code does

Can you clarify this operation? and why do we need to look at the pending htlc? we can not just look at the peer->channel->view? to get the balance of the channel. it should contains the on-flight htlc too, right?

Please add a couple of comments in the code to make the review easy to look at, thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems pending HTLC amounts are not included in the channel->view amount as my initial splicing code assumed. I've asked rusty for some clarification here: #6748 (comment)

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am still confused by this change here.

From the following comment https://github.com/ElementsProject/lightning/blob/master/common/initial_channel.h#L19 the peer->channel->view contains all the pending change, this mean that it contains also the pending_htlcs?

Is the comment outdated?

Can you clarify these operations for readers, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

as far as I understand, peer->channel->view only contains the main outputs.

I think there's a semantic confusion here, because the doc on the view field talks about "pending" in the sense of whether protocol changes (such as add_htlc) are committed with a signature, but the new code here is using "pending" in the sense of "live" or "in-flight" HTLCs. should probably drop the use of the world pending.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, pending means "uncommitted" in that comment. Should be fixed!


/* Tasks:
* Add up total funding_amount
Expand Down Expand Up @@ -3033,9 +3065,13 @@ static struct amount_sat check_balances(struct peer *peer,
peer_failed_warn(peer->pps, &peer->channel_id,
"Initiator funding is less than commited"
" amount. Initiator contributing %s but they"
" committed to %s.",
" committed to %s. Pending offered HTLC"
" balance of %s is not available for this"
" operation.",
fmt_amount_msat(tmpctx, in[TX_INITIATOR]),
fmt_amount_msat(tmpctx, out[TX_INITIATOR]));
fmt_amount_msat(tmpctx, out[TX_INITIATOR]),
fmt_amount_msat(tmpctx,
pending_htlcs[TX_INITIATOR]));
}

if (!amount_msat_sub(&initiator_fee, in[TX_INITIATOR], out[TX_INITIATOR]))
Expand All @@ -3051,9 +3087,13 @@ static struct amount_sat check_balances(struct peer *peer,
peer_failed_warn(peer->pps, &peer->channel_id,
"Accepter funding is less than commited"
" amount. Accepter contributing %s but they"
" committed to %s.",
" committed to %s. Pending offered HTLC"
" balance of %s is not available for this"
" operation.",
fmt_amount_msat(tmpctx, in[TX_INITIATOR]),
fmt_amount_msat(tmpctx, out[TX_INITIATOR]));
fmt_amount_msat(tmpctx, out[TX_INITIATOR]),
fmt_amount_msat(tmpctx,
pending_htlcs[TX_INITIATOR]));
}

if (!amount_msat_sub(&accepter_fee, in[TX_ACCEPTER], out[TX_ACCEPTER]))
Expand Down
41 changes: 41 additions & 0 deletions tests/test_splicing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,3 +276,44 @@ def test_commit_crash_splice(node_factory, bitcoind):
# Check that the splice doesn't generate a unilateral close transaction
time.sleep(5)
assert l1.db_query("SELECT count(*) as c FROM channeltxs;")[0]['c'] == 0


@pytest.mark.openchannel('v1')
@pytest.mark.openchannel('v2')
@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need')
def test_splice_stuck_htlc(node_factory, bitcoind, executor):
l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True, opts={'experimental-splicing': None})

l3.rpc.dev_ignore_htlcs(id=l2.info['id'], ignore=True)

inv = l3.rpc.invoice(10000000, '1', 'no_1')
executor.submit(l1.rpc.pay, inv['bolt11'])
l3.daemon.wait_for_log('their htlc 0 dev_ignore_htlcs')

# Now we should have a stuck invoice between l1 -> l2

chan_id = l1.get_channel_id(l2)

# add extra sats to pay fee
funds_result = l1.rpc.fundpsbt("109000sat", "slow", 166, excess_as_change=True)

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
result = l1.rpc.splice_update(chan_id, result['psbt'])
result = l1.rpc.signpsbt(result['psbt'])
result = l1.rpc.splice_signed(chan_id, result['signed_psbt'])

l2.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE')
l1.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE')

mempool = bitcoind.rpc.getrawmempool(True)
assert len(list(mempool.keys())) == 1
assert result['txid'] in list(mempool.keys())

bitcoind.generate_block(6, wait_for_mempool=1)

l2.daemon.wait_for_log(r'CHANNELD_AWAITING_SPLICE to CHANNELD_NORMAL')
l1.daemon.wait_for_log(r'CHANNELD_AWAITING_SPLICE to CHANNELD_NORMAL')

# Check that the splice doesn't generate a unilateral close transaction
time.sleep(5)
rustyrussell marked this conversation as resolved.
Show resolved Hide resolved
assert l1.db_query("SELECT count(*) as c FROM channeltxs;")[0]['c'] == 0
Loading