Skip to content

Fixed splice_locked messgae to include splice_txid field and other interop fixes #6

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

Closed
wants to merge 3 commits into from
Closed

Fixed splice_locked messgae to include splice_txid field and other interop fixes #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79bf18b
Splice: Rotating funding pubkey fix
ddustin Jan 21, 2025
1d98b89
Fixed splice_locked messgae to include splice_txid field
remyers Jan 22, 2025
dc3ae19
Add debug outputs and fix prevtx issue
remyers Jan 27, 2025
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
25 changes: 16 additions & 9 deletions channeld/channeld.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,13 +511,14 @@ static void check_mutual_splice_locked(struct peer *peer)
}

/* Our peer told us they saw our splice confirm on chain with `splice_locked`.
* If we see it to we jump into tansitioning to post-splice, otherwise we mark
* If we see it to we jump into transitioning to post-splice, otherwise we mark
* a flag and wait until we see it on chain too. */
static void handle_peer_splice_locked(struct peer *peer, const u8 *msg)
{
struct channel_id chanid;
struct bitcoin_txid splice_txid;

if (!fromwire_splice_locked(msg, &chanid))
if (!fromwire_splice_locked(msg, &chanid, &splice_txid))
peer_failed_warn(peer->pps, &peer->channel_id,
"Bad splice_locked %s", tal_hex(msg, msg));

Expand Down Expand Up @@ -3306,7 +3307,7 @@ static void resume_splice_negotiation(struct peer *peer,
struct bitcoin_tx *bitcoin_tx;
u32 splice_funding_index;
const u8 *msg, *sigmsg;
u32 chan_output_index;
u32 new_output_index;
struct pubkey *their_pubkey;
struct bitcoin_tx *final_tx;
struct bitcoin_txid final_txid;
Expand Down Expand Up @@ -3337,8 +3338,8 @@ static void resume_splice_negotiation(struct peer *peer,
&peer->channel->funding_pubkey[LOCAL],
&peer->channel->funding_pubkey[REMOTE]);

find_channel_output(peer, current_psbt, &chan_output_index,
&peer->channel->funding_pubkey[REMOTE]);
find_channel_output(peer, current_psbt, &new_output_index,
&inflight->remote_funding);

splice_funding_index = find_channel_funding_input(current_psbt,
&peer->channel->funding);
Expand Down Expand Up @@ -3622,7 +3623,7 @@ static void resume_splice_negotiation(struct peer *peer,

final_tx = bitcoin_tx_with_psbt(tmpctx, current_psbt);
msg = towire_channeld_splice_confirmed_signed(tmpctx, final_tx,
chan_output_index);
new_output_index);
wire_sync_write(MASTER_FD, take(msg));
}
}
Expand Down Expand Up @@ -3944,6 +3945,7 @@ static void splice_initiator(struct peer *peer, const u8 *inmsg)
ictx->shared_outpoint = tal(ictx, struct bitcoin_outpoint);
*ictx->shared_outpoint = peer->channel->funding;
ictx->funding_tx = prev_tx;
status_info("splice_initiator ictx->shared_outpoint = %s",(ictx->shared_outpoint?"defined":"null"));

peer->splicing->tx_add_input_count = 0;
peer->splicing->tx_add_output_count = 0;
Expand Down Expand Up @@ -3995,6 +3997,7 @@ static void splice_initiator_user_finalized(struct peer *peer)

ictx->shared_outpoint = tal(ictx, struct bitcoin_outpoint);
*ictx->shared_outpoint = peer->channel->funding;
status_info("splice_initiator_user_finalized: ictx->shared_outpoint = %s",(ictx->shared_outpoint?"defined":"null"));
ictx->funding_tx = prev_tx;

error = process_interactivetx_updates(tmpctx, ictx,
Expand Down Expand Up @@ -4143,6 +4146,10 @@ static void splice_initiator_user_update(struct peer *peer, const u8 *inmsg)
ictx->tx_add_input_count = peer->splicing->tx_add_input_count;
ictx->tx_add_output_count = peer->splicing->tx_add_output_count;

ictx->shared_outpoint = tal(ictx, struct bitcoin_outpoint);
*ictx->shared_outpoint = peer->channel->funding;
ictx->funding_tx = bitcoin_tx_from_txid(peer, peer->channel->funding.txid);

/* If there no are no changes, we consider the splice user finalized */
if (!interactivetx_has_changes(ictx, ictx->desired_psbt)) {
splice_initiator_user_finalized(peer);
Expand Down Expand Up @@ -5214,7 +5221,7 @@ static void peer_reconnect(struct peer *peer,
status_info("We have no pending splice but peer"
" expects one; resending splice_lock");
peer_write(peer->pps,
take(towire_splice_locked(NULL, &peer->channel_id)));
take(towire_splice_locked(NULL, &peer->channel_id, &peer->channel->funding.txid)));
Copy link
Owner

@ddustin ddustin Jan 22, 2025

Choose a reason for hiding this comment

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

This should be &peer->splice_state->locked_txid -- however this value is not preserved after channel restarts, so that needs to addressed as well.

Copy link
Owner

@ddustin ddustin Jan 22, 2025

Choose a reason for hiding this comment

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

This is a bit of a heavy lift. I think this is the best way to make splice_state->locked_txid work over restarts.

  1. Adding {SQL("ALTER TABLE channel_funding_inflights ADD locked_onchain BOOL DEFAULT 0"), NULL}, to db.c at the end of the db table be able to mark an inflight locked onchain
  2. inflight.c needs to add serializiation of is_locked with inflight->is_locked = fromwire_bool(cursor, max); in fromwire_inflight and towire_bool(pptr, inflight->is_locked); in towire_inflight.
  3. Modifying channeld_updating_inflight to include an is_locked parameter
  4. Update handle_update_inflight to take is_locked parameter and update that value in its inflight
  5. Modify wallet_inflight_save SQL query to save is_locked to the database
  6. Modify wallet_channel_load_inflights to load `is_locked from the database
  7. Update the inflight copy code in the loop above towire_channeld_init to add is_locked to the infcopy
  8. Below fromwire_channeld_init, loop through each inflight in peer->splice_state->inflights and get the txid from the psbt of the one that is_locked, placing it in peer->splice_state->locked_txid. Additionally it would be useful to assert only 1 is locked as two would be an error.

I thiiiiink that would do it.

Copy link
Author

@remyers remyers Jan 23, 2025

Choose a reason for hiding this comment

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

This is a bit of a heavy lift. I think this is the best way to make splice_state->locked_txid work over restarts.

I can't really comment on if that's the right approach. Do you need to use splice_state->locked_txid during reconnect? It should already be your latest funding tx once you have sent and received splice_locked. If it has reached acceptable depth then your node can resend splice_locked with the txid of the latest funding tx. You must have already have saved that information after you exchanged splice_locked with your peer.

However, when you do a reconnect before exchanging splice_locked I can see that you'll need to save the txid of the funding tx negotiated for the splice. We save this information with the splice commitment along with the signatures exchanged.

Copy link
Owner

@ddustin ddustin Feb 1, 2025

Choose a reason for hiding this comment

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

If it has reached acceptable depth then your node can resend splice_locked with the txid of the latest funding tx.

Yes you're right, peer->channel->funding.txid is correct in this case.

There needs to be a new separate case that uses peer->splice_state->locked_txid that I'm working on at the moment.

}
else {
splice_abort(peer, "next_funding_txid not recognized."
Expand Down Expand Up @@ -5593,10 +5600,10 @@ static void handle_funding_depth(struct peer *peer, const u8 *msg)
} else if(splicing && !peer->splice_state->locked_ready[LOCAL]) {
assert(scid);

msg = towire_splice_locked(NULL, &peer->channel_id);

peer->splice_state->locked_txid = txid;

msg = towire_splice_locked(NULL, &peer->channel_id, &peer->splice_state->locked_txid);

peer_write(peer->pps, take(msg));

peer->splice_state->locked_ready[LOCAL] = true;
Expand Down
13 changes: 12 additions & 1 deletion common/interactivetx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,19 +227,30 @@ static char *send_next(const tal_t *ctx,

/* If this the shared channel input, we send funding txid in
* in tlvs and do not send prevtx */
status_info("ictx->shared_outpoint = %s",(ictx->shared_outpoint?"defined":"null"));
char txid_hex[65];
if (ictx->shared_outpoint && bitcoin_txid_to_hex(&(ictx->shared_outpoint->txid), txid_hex, sizeof(txid_hex))) {
status_info("ictx->shared_outpoint->txid=%s, ictx->shared_outpoint->n=%d", txid_hex, ictx->shared_outpoint->n);
Copy link
Owner

@ddustin ddustin Feb 1, 2025

Choose a reason for hiding this comment

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

This should use fmt_bitcoin_txid instead of bitcoin_txid_to_hex

For example:

	if (!bitcoin_txid_eq(&signed_psbt_txid, &current_psbt_txid))
		status_failed(STATUS_FAIL_INTERNAL_ERROR,
			      "Signed PSBT txid %s does not match"
			      " current_psbt_txid %s",
			      fmt_bitcoin_txid(tmpctx, &signed_psbt_txid),
			      fmt_bitcoin_txid(tmpctx, &current_psbt_txid));

}
if (bitcoin_txid_to_hex(&(point.txid), txid_hex, sizeof(txid_hex))) {
status_info("point.txid=%s, point.n=%d", txid_hex, point.n);
Copy link
Owner

@ddustin ddustin Feb 1, 2025

Choose a reason for hiding this comment

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

fmt_bitcoin_txid here as well

}
status_info("here2");
Copy link
Owner

@ddustin ddustin Feb 1, 2025

Choose a reason for hiding this comment

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

I would cut this log statement

if (ictx->shared_outpoint
&& bitcoin_outpoint_eq(&point, ictx->shared_outpoint)) {
struct tlv_tx_add_input_tlvs *tlvs = tal(tmpctx, struct tlv_tx_add_input_tlvs);
struct tlv_tx_add_input_tlvs *tlvs = tlv_tx_add_input_tlvs_new(tmpctx);
tlvs->shared_input_txid = tal_dup(tlvs,
struct bitcoin_txid,
&point.txid);
status_info("Adding shared input %s", tal_hexstr(ctx, &serial_id, sizeof(serial_id)));
msg = towire_tx_add_input(NULL, cid, serial_id,
NULL, in->input.index,
in->input.sequence, tlvs);
} else {
msg = towire_tx_add_input(NULL, cid, serial_id,
prevtx, in->input.index,
in->input.sequence, NULL);
status_info("Adding splice input %s", tal_hexstr(ctx, &serial_id, sizeof(serial_id)));
}

tal_arr_remove(&set->added_ins, 0);
Expand Down
1 change: 1 addition & 0 deletions wire/peer_wire.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ msgdata,splice_ack,relative_satoshis,s64,
msgdata,splice_ack,funding_pubkey,point,
msgtype,splice_locked,77,
msgdata,splice_locked,channel_id,channel_id,
msgdata,splice_locked,splice_txid,sha256,
msgtype,shutdown,38
msgdata,shutdown,channel_id,channel_id,
msgdata,shutdown,len,u16,
Expand Down