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

Use signed amounts in RBF messages #15

Merged

Conversation

t-bast
Copy link

@t-bast t-bast commented Mar 30, 2023

While dual funding only needs unsigned funding amounts, other protocols that leverage interactive-tx may use signed funding amounts, for example to take funds out of an existing channel (splice-out). It is thus more future-proof to use signed amounts in tx_init_rbf and tx_ack_rbf.

An alternative would be to keep the funding_output_contribution tlv unchanged and define a separate tlv for negative contributions (e.g. deduce_from_funding_output). When splicing funds into an existing channel, funding_output_contribution would be used (which could be renamed add_to_funding_output for symmetry), and when splicing funds out of an existing channel, deduce_from_funding_output would be used. This would remove the need for introducing signed amounts now, but it means we would have two mutually exclusive tlvs in tx_init_rbf and tx_ack_rbf messages.

I'm not sure which option is best and I'm hoping for feedback! @niftynei @dunxen @morehouse

@dunxen
Copy link

dunxen commented Mar 30, 2023

I'm not sure which option is best and I'm hoping for feedback!

Correct me if I'm off here, but as I understand it, the current splice draft in splice (or I guess splice_init for the eclair proposal):

funding_satoshis is the amount the sender is putting into the
post-splice channel. It includes their old funded channel amount
plus any satoshis they intend to add, or, less any satoshis they
intend to remove.

I'm not sure I understand why in tx_init_rbf and tx_ack_rbf this would be different as:

funding_output_contribution is the amount of satoshis that this peer
will contribute to the funding output of the transaction, when there is
such an output. Note that it may be different from the contribution
made in the previously completed transaction. If omitted, the sender is
not contributing to the funding output.

It seems the same way of handling removing from funding would be handled here. In other words I thought the contributions in RBF were absolute values and not deltas.

@t-bast
Copy link
Author

t-bast commented Mar 30, 2023

It seems the same way of handling removing from funding would be handled here. In other words I thought the contributions in RBF were absolute values and not deltas.

You're totally right that the current version of the splice spec PR uses absolute values. But I wouldn't look at the current state of that PR as it isn't up-to-date with what has been learned by implementing it in eclair and cln, and from discussions during spec meetings.

While it's not 100% sure yet, it looks like current implementers are now leaning more towards using deltas instead of absolute values. Based on my implementation work, where I initially started with absolute values and recently switched to using deltas, deltas have the following advantages:

  • it fixes the issue of millisatoshi balance truncation, which is very awkward to handle with absolute values
  • it better matches the UX: users mostly want to add/remove a given amount to/from a channel, not resize it to a chosen value
  • the implementation is simpler

Also, you can conceptually view dual funding as using a delta as well, where the previous balance was 0: once you see it that way, you don't have to make any special case in your code to distinguish dual funding attempts from splice attempts, which is nice.

@dunxen
Copy link

dunxen commented Mar 30, 2023

You're totally right that the current version of the splice spec PR uses absolute values. But I wouldn't look at the current state of that PR as it isn't up-to-date with what has been learned by implementing it in eclair and cln, and from discussions during spec meetings.

Ah got it! In that case it just seems more natural and less "patchy" to use signed values as you've proposed.

@morehouse
Copy link

If splicing ends up using deltas, it's a +1 from me for signed values.

01-messaging.md Outdated Show resolved Hide resolved
@t-bast t-bast force-pushed the signed-funding-contributions branch from 69d3954 to 30534ec Compare March 30, 2023 14:57
@t-bast
Copy link
Author

t-bast commented Mar 30, 2023

@niftynei @ddustin what's your current position on using deltas for splicing instead of absolute amounts? Which are you using in the latest iteration of cln?

@ddustin
Copy link

ddustin commented Apr 6, 2023

@niftynei @ddustin what's your current position on using deltas for splicing instead of absolute amounts? Which are you using in the latest iteration of cln?

I think we're all onboard. The current version uses total amount but changing to relative cleans up some other things anyway and feels like a cleaner protocol (TM) 👍.

01-messaging.md Outdated Show resolved Hide resolved
While dual funding only needs unsigned funding amounts, other protocols
that leverage interactive-tx may use signed funding amounts, for example
to take funds out of an existing channel (splice-out).

It is thus more future-proof to use signed amounts in `tx_init_rbf` and
`tx_ack_rbf`.
@t-bast t-bast force-pushed the signed-funding-contributions branch from 30534ec to 0e589c8 Compare April 11, 2023 08:42
Copy link

@morehouse morehouse left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

LGTM. Did some basic sanity checks on the test vectors.

Copy link
Owner

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

concept ACK, i is a no-go for me tho.

01-messaging.md Outdated
@@ -220,9 +221,16 @@ receiver to parse individual elements from `value`.
Various fundamental types are referred to in the message specifications:

* `byte`: an 8-bit byte
* `i8`: an 8-bit signed integer
Copy link
Owner

Choose a reason for hiding this comment

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

nit: use s not i, as it's better to do opposites in specs (unsigned vs signed -> u vs s)

Copy link
Author

Choose a reason for hiding this comment

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

Done in cf62b5a

I find it a bit weird, as i32/i64 seems to be the standard way in programming languages to refer to signed integers and I've never seen s32/s64 anywhere, but I don't really mind!

Copy link

Choose a reason for hiding this comment

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

Yeah s seems a bit strange but no big deal honestly.

@niftynei niftynei merged commit 34e7787 into niftynei:nifty/dual-fund Jun 22, 2023
@t-bast t-bast deleted the signed-funding-contributions branch June 30, 2023 14:05
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.

5 participants