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.

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

RBF: Final part #4399

Merged
merged 23 commits into from
Mar 6, 2021
Merged

Conversation

niftynei
Copy link
Collaborator

@niftynei niftynei commented Feb 24, 2021

This patchset finishes the RBF flows and adds tests for all the things.

@niftynei niftynei added this to the v0.9.4 milestone Feb 24, 2021
@niftynei niftynei changed the title Nifty/rbf last RBF: Final part Feb 24, 2021
@niftynei niftynei force-pushed the nifty/rbf-last branch 4 times, most recently from c8ee535 to c04d9ef Compare March 3, 2021 22:21
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack c04d9ef

Mainly textual feedback, looking good!

/* FIXME: call dualopend with psbt + amount!! */
/* Add serials to any input that's missing them */
psbt_add_serials(psbt, TX_INITIATOR);
if (!psbt_has_required_fields(psbt))
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: psbt_has_required_fields should probably describe the forest more, as well as the trees. e.g. "We require extra annotations on our PSBTs, which get passed in and out various RPC calls. In particular..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment exists on psbt_has_required_fields

/* psbt_has_required_fields - Validates psbt field completion
 *
 * Required fields are:
 * - a serial_id; input+output
 * - a prev_tx; input,non_witness_utxo
 * - redeemscript; input,iff is P2SH-P2W*
 * @psbt - psbt to validate
 *
 * Returns true if all required fields are present
 */

state->tx_state = tx_state;

if (state->our_role == TX_ACCEPTER) {
status_debug("tx sigs sent to peer %d", __LINE__);
Copy link
Contributor

Choose a reason for hiding this comment

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

debug statement...

lightningd/peer_control.c Outdated Show resolved Hide resolved
lightningd/dual_open_control.c Outdated Show resolved Hide resolved
openingd/dualopend.c Outdated Show resolved Hide resolved
handle_peer_shutdown(state, msg);
/* If we're done, exit */
if (shutdown_complete(state))
shutdown(state);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, do we want to return msg to the caller here (if not shutdown_complete) ? I expected a continue or a return NULL...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Yes, we should return NULL.

json_add_string(response, "next_feerate",
tal_fmt(tmpctx, "%d%s", next_feerate,
feerate_style_name(FEERATE_PER_KSIPA)));
json_add_num(response, "fee_step", feestep);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this calculation confused me. Perhaps rename "inflight" to "initial" and then it's clearer that you're counting steps to get to the current one (using < next_feerate rather than == last_feerate is also confusing to me).

bump = l1.rpc.openchannel_bump(chan_id, chan_amount, initpsbt['psbt'])

update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
assert update['commitments_secured']
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check next_feerate and fee_step in this test too?

Copy link
Collaborator Author

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

Updated, plus added a bit of a noleak hack to fixup the memleaks... but maybe this isn't the right approach? See last commit.

/* FIXME: call dualopend with psbt + amount!! */
/* Add serials to any input that's missing them */
psbt_add_serials(psbt, TX_INITIATOR);
if (!psbt_has_required_fields(psbt))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment exists on psbt_has_required_fields

/* psbt_has_required_fields - Validates psbt field completion
 *
 * Required fields are:
 * - a serial_id; input+output
 * - a prev_tx; input,non_witness_utxo
 * - redeemscript; input,iff is P2SH-P2W*
 * @psbt - psbt to validate
 *
 * Returns true if all required fields are present
 */

handle_peer_shutdown(state, msg);
/* If we're done, exit */
if (shutdown_complete(state))
shutdown(state);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Yes, we should return NULL.

@niftynei niftynei force-pushed the nifty/rbf-last branch 3 times, most recently from 8b7bdf8 to a97a2ba Compare March 5, 2021 00:55
@rustyrussell
Copy link
Contributor

Looks like you need 'dev-no-reconnect': (it's normally set if we are doing may_reconnect: False), since if we're very slow it could reconnect and we'll not get the sequence we expect. And YA genfile rebase...

Reorg a bit of the RBF code so we use the same codepaths for we-init vs
they-init starts.
We can use the funding amount to derive the reserve requirement.
We're *mostly* set up for both sides doing RBF, except that it reverses
the callback flow (using the plugin vs RPC calls) and we're not
currently smart enough to flip between them gracefully
We can't "first save" a channel twice; instead we split in two and just
update the underlying channel on subsequent passes (RBFs)
We move over to the new "warning" paradigm, instead of using
an "rbf_fail" message.

Every failure is either a warning or an error; on warnings we
hang up and reconnect later, effectively resetting the state.
Make sure we clean up unsaved channels appropriately on failure.

We forget the peer/channel if it's unsaved!
If we're doing an RBF, it's possible that the peer will send us a
funding_locked, shutdown, or tx_signatures message. (We get tx_sigs out
of order on a reconnect)

This lets us gracefully handle a shutdown or funding_locked
sent at any time (after first funding tx) as well.
Channels that we're in negotiation for, but don't have a commitment
transaction saved for yet.
Need these more accessible for next patch, which moves the next_feerate
info into listpeers
Changelog-Added: JSON-RPC: `listpeers` now includes 'last_feerate', 'next_feerate', 'initial_feerate' and 'next_fee_step' for channels in state DUALOPEND_AWAITING_LOCKIN

fixup! listpeers: include feerate info for RBF-candidate channels
since we might be in the middle of an RBF, update our checks to be more
robust
Make existing methods understand how unsaved channels work, re-work
errors so that we handle everything appropriately
We were getting a memleak error that the open_attempt isnt' being
cleaned up in test_rbf_reconnect_tx_construct. I had some trouble
reproducing it, so I removed the reliance on using `tmpctx` to clean it
up and was more surgical about cleaning it up inline.
If they've disconnected/reconnected we need to terminate all the
inflight stuff, plus go ahead and call 'disconnect' plugin trigger etc.
@rustyrussell
Copy link
Contributor

Ack 4b7af79

@rustyrussell rustyrussell merged commit 8cc2919 into ElementsProject:master Mar 6, 2021
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.

2 participants