Collaborative transaction building by ddustin · Pull Request #5287 · ElementsProject/lightning

ddustin · 2022-05-24T20:42:36Z

Takes @niftynei's dualopen collaborative transaction building and makes it available for other daemons to use.

This is to make it available for channeld to do splicing and also other code that may want to us it in the future!

niftynei

Bunch of small nits and things, but overall lgtm! Biggest thing is that there's a decent amount of setup needed for the ictx -- do you have a new_ictx method somewhere that encapsulates this?

cdecker · 2022-09-12T14:45:30Z

This appears to duplicate a lot of code that, when changed, will cause things to slowly drift apart. Any chance of not just extracting this but also to rewire the existing uses to use interactivetx.c?

ddustin · 2022-09-12T14:56:08Z

This appears to duplicate a lot of code that, when changed, will cause things to slowly drift apart. Any chance of not just extracting this but also to rewire the existing uses to use interactivetx.c?

Yeah definitely. The plan is to make dual open use this code, cutting out its internal implementation, probably next release ish? Also a good moment for some interim tests to look for behavior changes in the refactor.

Dual open is currently the only user of interactivetx but splicing and dual close will want to use it.

cdecker · 2022-09-19T11:54:06Z

Excellent, thanks @ddustin. Want me to wait for dualopend to be migrated on top of interactivetx.c or would you like to hand this PR over as is (and create an issue so we don't forget about the migration) and I can shepherd it through CI as is.

niftynei

ACK with a few changes! lgtm, let's goo!

niftynei · 2022-10-27T18:08:28Z

+	ictx->channel_id = channel_id;
+	ictx->tx_msg_count[TX_ADD_INPUT] = 0;
+	ictx->tx_msg_count[TX_ADD_OUTPUT] = 0;
+	ictx->tx_msg_count[TX_RM_INPUT] = 0;


i think we can remove the "RM" counts, as the spec only counts adds (for the reason that you'll hit the add limit first!)

niftynei · 2022-10-27T18:10:46Z

+							struct channel_id channel_id);
+
+/* Blocks the thread until we run out of changes (and we send tx_complete),
+ * or an error occurs. If 'pause_when_complete' is set, this behavior changes


Suggested change

* or an error occurs. If 'pause_when_complete' is set, this behavior changes

* or an error occurs. If 'pause_when_complete' on the `interactivetx_context` is set, this behavior changes

niftynei · 2022-10-27T18:13:56Z

+struct interactivetx_context {
+
+	/* Users can set this to their own context */
+	void *ctx;


ctx means "allocation context", should we call it data_ptr?

niftynei · 2022-10-27T18:19:59Z

+	char *error = NULL;
+	struct wally_psbt *next_psbt;
+
+	if (ictx->current_psbt == NULL)


move this to the new_itctx method?

niftynei · 2022-10-27T18:25:53Z

+	 * Otherwise we advance `current_psbt` to `next_psbt` and begin
+	 * processing the change set in `ictx->change_set` */
+	if (ictx->current_psbt != next_psbt) {
+		tal_free(ictx->current_psbt);


cannot free this until the change_set is free'd!

niftynei · 2022-10-27T18:33:59Z

+	struct channel_id *cid = &ictx->channel_id;
+	struct psbt_changeset *set = ictx->change_set;
+	u64 serial_id;
+	u8 *msg = NULL;


do not set msg here, leave for later code to fill in.

cdecker · 2022-10-31T18:16:10Z

ACK with a few changes! lgtm, let's goo!

Gotcha, I'll shepherd this through CI and merge ASAP ^^

ACK 6f1f6e7

Takes the dualopen collaborative transaction building and makes it available for other daemons to use Changelog-Added: Added interactive transaction building routine

And internal changes don't get CHANGELOG entries, so removed "- Added interactive transaction building routine ([ElementsProject#5287])". We need to come up with some way of marking rust crate changes, as it's not immediately obvious what "- cln-plugin" means? Maybe we switch from JSON-RPC to Control as a more general prefix? Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

And internal changes don't get CHANGELOG entries, so removed "- Added interactive transaction building routine ([#5287])". We need to come up with some way of marking rust crate changes, as it's not immediately obvious what "- cln-plugin" means? Maybe we switch from JSON-RPC to Control as a more general prefix? Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

And internal changes don't get CHANGELOG entries, so removed "- Added interactive transaction building routine ([ElementsProject#5287])". We need to come up with some way of marking rust crate changes, as it's not immediately obvious what "- cln-plugin" means? Maybe we switch from JSON-RPC to Control as a more general prefix? Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

ddustin force-pushed the ddustin/interactivetx branch 10 times, most recently from c1e118a to 465a318 Compare May 24, 2022 22:02

ddustin marked this pull request as ready for review May 24, 2022 22:57

ddustin force-pushed the ddustin/interactivetx branch 2 times, most recently from 5ca53bf to 7a8813f Compare May 26, 2022 00:40

niftynei reviewed Jun 22, 2022

View reviewed changes

niftynei added this to the v22.10 milestone Jul 11, 2022

ddustin mentioned this pull request Sep 20, 2022

dualopen: Refactor dualopen to use interactivetx library #5616

Closed

niftynei reviewed Sep 22, 2022

View reviewed changes

Comment thread common/interactivetx.h Outdated

niftynei reviewed Sep 22, 2022

View reviewed changes

Comment thread common/interactivetx.c

cdecker requested a review from niftynei September 29, 2022 16:28

ddustin mentioned this pull request Oct 25, 2022

splicing: Adds the features needed to enable collaborative splicing & resizing of active channels. #5675

Closed

19 tasks

ddustin force-pushed the ddustin/interactivetx branch 7 times, most recently from c435cbc to 53603a6 Compare October 27, 2022 18:02

ddustin force-pushed the ddustin/interactivetx branch 3 times, most recently from 4366fca to 6f1f6e7 Compare October 27, 2022 18:26

niftynei approved these changes Oct 27, 2022

View reviewed changes

cdecker force-pushed the ddustin/interactivetx branch 2 times, most recently from 141f96c to 2fae6ba Compare November 1, 2022 12:01

Collaborative transaction building

1bb20c3

Takes the dualopen collaborative transaction building and makes it available for other daemons to use Changelog-Added: Added interactive transaction building routine

cdecker force-pushed the ddustin/interactivetx branch from 2fae6ba to 1bb20c3 Compare November 1, 2022 16:07

cdecker merged commit 76623b2 into ElementsProject:master Nov 2, 2022

rustyrussell mentioned this pull request Nov 19, 2022

CHANGELOG.md: order into a more user-first ordering. #5723

Merged

ddustin mentioned this pull request May 12, 2023

splicing: Adds the features needed to enable collaborative splicing & resizing of active channels. #6253

Merged

19 tasks

	* or an error occurs. If 'pause_when_complete' is set, this behavior changes
	* or an error occurs. If 'pause_when_complete' on the `interactivetx_context` is set, this behavior changes

Conversation

ddustin commented May 24, 2022

Uh oh!

niftynei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdecker commented Sep 12, 2022

Uh oh!

ddustin commented Sep 12, 2022

Uh oh!

cdecker commented Sep 19, 2022

Uh oh!

Uh oh!

Uh oh!

niftynei left a comment

Choose a reason for hiding this comment

Uh oh!

niftynei Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

niftynei Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

niftynei Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

niftynei Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

niftynei Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

niftynei Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

cdecker commented Oct 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants