RBF: Final part #4399

niftynei · 2021-02-24T20:40:15Z

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

rustyrussell

Ack c04d9ef

Mainly textual feedback, looking good!

rustyrussell · 2021-03-03T23:29:29Z

lightningd/dual_open_control.c

-	/* 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))


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..."

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 */

rustyrussell · 2021-03-03T23:35:23Z

openingd/dualopend.c

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


debug statement...

lightningd/peer_control.c

lightningd/dual_open_control.c

openingd/dualopend.c

rustyrussell · 2021-03-04T00:14:35Z

openingd/dualopend.c

+			handle_peer_shutdown(state, msg);
+			/* If we're done, exit */
+			if (shutdown_complete(state))
+				shutdown(state);


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

Oops. Yes, we should return NULL.

rustyrussell · 2021-03-04T00:19:07Z

lightningd/peer_control.c

+		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);


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).

rustyrussell · 2021-03-04T00:21:59Z

tests/test_opening.py

+    bump = l1.rpc.openchannel_bump(chan_id, chan_amount, initpsbt['psbt'])
+
+    update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
+    assert update['commitments_secured']


Can we check next_feerate and fee_step in this test too?

niftynei

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

niftynei · 2021-03-04T19:57:23Z

lightningd/dual_open_control.c

-	/* 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))


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 */

niftynei · 2021-03-04T21:28:56Z

openingd/dualopend.c

+			handle_peer_shutdown(state, msg);
+			/* If we're done, exit */
+			if (shutdown_complete(state))
+				shutdown(state);


Oops. Yes, we should return NULL.

rustyrussell · 2021-03-05T02:55:00Z

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!

Will reuse these elsewhere soon

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

I can has RBF?

This reverts commit 2e40a13.

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.

@rustyrussell

Requested-In-Part-By: Rusty Russell @rustyrussell

If they've disconnected/reconnected we need to terminate all the inflight stuff, plus go ahead and call 'disconnect' plugin trigger etc.

rustyrussell · 2021-03-06T04:33:49Z

Ack 4b7af79

niftynei added this to the v0.9.4 milestone Feb 24, 2021

niftynei requested a review from cdecker as a code owner February 24, 2021 20:40

niftynei changed the title ~~Nifty/rbf last~~ RBF: Final part Feb 24, 2021

niftynei force-pushed the nifty/rbf-last branch 4 times, most recently from c8ee535 to c04d9ef Compare March 3, 2021 22:21

niftynei mentioned this pull request Mar 3, 2021

Dual-funding spec edits, cleanup #4410

Merged

rustyrussell approved these changes Mar 4, 2021

View reviewed changes

niftynei force-pushed the nifty/rbf-last branch from c04d9ef to e5ffc23 Compare March 4, 2021 21:56

niftynei commented Mar 4, 2021

View reviewed changes

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

rustyrussell force-pushed the nifty/rbf-last branch 2 times, most recently from 54cc802 to 7a70838 Compare March 5, 2021 03:09

niftynei force-pushed the nifty/rbf-last branch from 7a70838 to a697508 Compare March 5, 2021 18:39

niftynei added 12 commits March 5, 2021 13:40

df-rbf: init an rbf for real, using openchannel_bumpfee

84d7b29

Reorg a bit of the RBF code so we use the same codepaths for we-init vs they-init starts.

df-rbf: handle rbf failures in lightningd

e49a241

df: update reserve calculation

fc051d1

We can use the funding amount to derive the reserve requirement.

df-rbf: only permit the channel initiator to init RBF

fee2c15

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

df-rbf: separate first-time channel save from update (RBF)

24ea527

We can't "first save" a channel twice; instead we split in two and just update the underlying channel on subsequent passes (RBFs)

rbf: consolidate failure paths, use "warnings"

17c3382

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.

rbf: clean up channel on error

2f40c6b

Make sure we clean up unsaved channels appropriately on failure. We forget the peer/channel if it's unsaved!

df: move methods around so we can call them elsewhere

d04de94

Will reuse these elsewhere soon

listpeers: include info on channels that are in-process

e2f0987

Channels that we're in negotiation for, but don't have a commitment transaction saved for yet.

channel: move inflight + feerate methods to channel.c

4c5024c

Need these more accessible for next patch, which moves the next_feerate info into listpeers

niftynei added 10 commits March 5, 2021 13:42

df-rbf: update rejection criteria for tx_sigs

ea06a4c

since we might be in the middle of an RBF, update our checks to be more robust

df: cleanup error handling on lightningd side

97f0fe8

Make existing methods understand how unsaved channels work, re-work errors so that we handle everything appropriately

df-tests: happy path rbf test

2773688

I can has RBF?

df-rbf: reconnection tests (init_rbf + ack_rbf)

10fb911

df-rbf: test for interrupts during tx construction

1401761

df-tests: check that multiple RBFs and tx-sigs errors during RBF work

796b4ea

df-tests: check that rbf includes overlapping inputs

3ac665b

Revert "df: turn off integration tests that depend on v2 funding"

3dd2166

This reverts commit 2e40a13.

df: add (over zealous?) note about the usage of psbt_has_req_fields

36460c9

Requested-In-Part-By: Rusty Russell @rustyrussell

niftynei force-pushed the nifty/rbf-last branch from a697508 to 913252d Compare March 5, 2021 19:42

connectd: clean up the channel stuffs when we get a reconnect

4b7af79

If they've disconnected/reconnected we need to terminate all the inflight stuff, plus go ahead and call 'disconnect' plugin trigger etc.

niftynei force-pushed the nifty/rbf-last branch from 913252d to 4b7af79 Compare March 5, 2021 19:53

rustyrussell merged commit 8cc2919 into ElementsProject:master Mar 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBF: Final part #4399

RBF: Final part #4399

niftynei commented Feb 24, 2021 •

edited

Loading

rustyrussell left a comment

rustyrussell Mar 3, 2021

niftynei Mar 4, 2021

rustyrussell Mar 3, 2021

rustyrussell Mar 4, 2021

niftynei Mar 4, 2021

rustyrussell Mar 4, 2021

rustyrussell Mar 4, 2021

niftynei left a comment

niftynei Mar 4, 2021

niftynei Mar 4, 2021

rustyrussell commented Mar 5, 2021

rustyrussell commented Mar 6, 2021

RBF: Final part #4399

RBF: Final part #4399

Conversation

niftynei commented Feb 24, 2021 • edited Loading

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Mar 5, 2021

rustyrussell commented Mar 6, 2021

niftynei commented Feb 24, 2021 •

edited

Loading