-
Notifications
You must be signed in to change notification settings - Fork 492
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
WIP: Dual Funding (v2 Channel Establishment protocol) #524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generally good, and I like the rbf flow. @Roasbeef suggested we keep it more minimal (ie. no change of inputs), but that doesn't gain very much compared to this approach, AFAICT.
I've bikeshedded a few things, because that's what we do :)
02-peer-protocol.md
Outdated
- MUST fail the channel if: | ||
- `change_satoshis` would be negative. | ||
- if has not yet sent a `funding_compose`: | ||
- MUST send their `funding_compose` message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/their/its/
02-peer-protocol.md
Outdated
- MUST send their `funding_compose` message. | ||
- otherwise: | ||
- MUST use the sent and received `input_info` and `output_info` | ||
to create the funding transaction, using `max_extra_witness_len` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now called 'max_witness_len'.
02-peer-protocol.md
Outdated
the `put_limit`. | ||
- otherwise: | ||
- MUST fail the channel if: | ||
- `change_satoshis` would be negative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not be able to figure this out yet (fees). So, perhaps this should be 'sum of input amounts < sum of output amounts'? And add the change_satoshis test only once it has calculated the tx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually, I think this being negative should be allowed: in that case, funding_satoshis should be reduced. In effect, this allows you to say "I don't want change, take the fee out of my initial balance". Or perhaps we should make the change output optional for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a valid use case. Will remove the 'change_satoshis
must be positive' requirement.
Allowing negative change amounts means the resulting channel balance will be less than the indicated funding_satoshis
, which ostensibly came from the user. Implementations would probably want to add a way for users to indicate where to take the fees out -- either the provided channel fund amount or in addition to that. (You could do this with the existing flow too, since it's just UX.)
My hesitation with making the change_address
optional is that 1) it's an edge case (depends on available UTXO set) 2) it has a small chance of introducing bugs (i.e. broken implementation forgets to include a change address so you end up with a huge channel balance by accident). Leaving out the change address doesn't save you much in calculation overhead when picking out the input UTXO set either. The only thing it does save on is wire bytes, in the case where your funding amt works out exactly right.
02-peer-protocol.md
Outdated
to create the funding transaction, using `max_extra_witness_len` | ||
for each `input_info` and `feerate_per_kw_funding` as specified in | ||
`open_channel2`. | ||
- MUST send `commitment_signed2`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use commitment_signed
. It's identical, and performs the identical function, so I really think it deserves the same name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The rationale was that it might be nice to decouple the two flows as that would allow future modification without needing to worry much about backwards compatibility.
02-peer-protocol.md
Outdated
|
||
`satoshis` is the value of the input or output. The opener must provide | ||
one change address for the transaction, which is flagged with a | ||
`satoshis` value of zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said this below as well; delete one? We try to avoid spec-sounding language (i.e. "must") in Rationales to avoid confusion; I would say "The opener provides one output with satoshis
equal zero, as a change address once the amount of change can be calculated".
02-peer-protocol.md
Outdated
#### Requirements | ||
The sending node: | ||
- MUST set `witness` to the marshalled witness data for each of its | ||
inputs, in funding transaction order. FIXME: add reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is quoting me, but the term I should have used is 'serialized' which IIRC is what the BIPs use.
02-peer-protocol.md
Outdated
Exchanging witness data allows both sides to broadcast the funding | ||
transaction and maintains information symmetry. | ||
|
||
### The `funding_locked2` Message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, prefer direct 'funding_locked' reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inclusion of the funding_tx_id
means we need a v2 for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, OK.
02-peer-protocol.md
Outdated
* [`4`:`feerate_per_kw`] | ||
* [`4`:`feerate_per_kw_funding`] | ||
* [`2`:`num_inputs`] | ||
* [`num_inputs`\*`input_info`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
num_additional_inputs might be clearer?
02-peer-protocol.md
Outdated
e.g. an `output_info` entry with `satoshis` set to zero. | ||
- the `feerate_per_kw_funding` is not greater than the previously | ||
negotiated rate. | ||
- `change_satoshis` is negative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should allow "extract from channel" in this case, as well.
00c6e6f
to
8bacfe4
Compare
02-peer-protocol.md
Outdated
#### Requirements | ||
|
||
The sending node: | ||
- MUST ensure each `input_info` refers to an existing UTXO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, it must ensure that it's non-malleable.
02-peer-protocol.md
Outdated
- MAY send zero inputs and/or outputs. | ||
|
||
The receiving node: | ||
- if the total `input_info`.`satoshis` is less than the total `output_info`.`satoshis` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Summary of discussion with @niftynei elsewhere)
Unfortunately, the receiving node needs to know this input is not malleable. That requires that it know the output being spent. We could either have the sender include the entire TX, or insist the recipient know the UTXO. Both are unappealing: the 64k packet size means some txs can't be used, and the UTXO requirement means no CPFP.
I think we have to go for "send the input tx"; creating a "fetch tx" for this would lead to another untested code path. Which makes this different from splice, which doesn't have this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, I think we can simplify this requirement by checking the scriptSig
data in addition to the txid/vout for inputs, and merely verifying that it is either 00
or one of the two acceptable v0 redeemScript
formats for P2SH-P2W*. (i.e. 0014xxx
or 0020xxx
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, updated to include the scriptPubKey
of the desired output as well.
02-peer-protocol.md
Outdated
Exchanging witness data allows both sides to broadcast the funding | ||
transaction and maintains information symmetry. | ||
|
||
### The `funding_locked2` Message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, OK.
3cb65ff
to
4c0a516
Compare
bc409b8
to
cb8990b
Compare
Working on the implementation of this for c-lightning, and I've hit a bit of a conundrum around what's the best way forward wrt the In v1 of the channel establishment protocol, the entire funding balance is known as of the first Here's a few options that I've come up with for how to deal with this. Leaning towards the 3rd option.
|
1a9a018
to
867a6d8
Compare
02-peer-protocol.md
Outdated
|
||
If nodes have negotiated `option_dual_fund`: | ||
- the opening node: | ||
- MUST not send `open_channel` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
- MUST not send `open_channel` | |
- MUST NOT send `open_channel` |
02-peer-protocol.md
Outdated
- MUST add all received inputs to the funding transaction | ||
- MUST fail the channel if: | ||
- it receives a duplicate input to one it sent previously | ||
- if it receives this message after `funding_add_complete` is received. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: trailing if
02-peer-protocol.md
Outdated
Native SegWit inputs (P2WPKH and P2WSH) inputs, will have an empty `script` field | ||
See [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#examples). | ||
|
||
The `accepter` node may omit this message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, removed.
02-peer-protocol.md
Outdated
- MUST NOT send a total count of more than 8 outputs, across all `funding_add_output` messages. | ||
|
||
The receiving node: | ||
- MUST add all received outputs to the funding transaction | ||
- MUST fail the channel if | ||
- it receives this message after receiving `funding_add_complete` | ||
- if is the `opener`: | ||
- MUST fail the channel if: | ||
- receives a total count of more than 3 outputs, across all `funding_add_output`s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max of 8
then 3
?
02-peer-protocol.md
Outdated
- MUST NOT send a total count of more than 8 outputs, across all `funding_add_output` messages. | ||
|
||
The receiving node: | ||
- MUST add all received outputs to the funding transaction | ||
- MUST fail the channel if | ||
- it receives this message after receiving `funding_add_complete` | ||
- if is the `opener`: | ||
- MUST fail the channel if: | ||
- receives a total count of more than 3 outputs, across all `funding_add_output`s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max of 8
then 3
?
02-peer-protocol.md
Outdated
- the `num_outputs` does not correspond to the total sum of all `num_outputs` | ||
received in all `funding_add_output` messages | ||
- the total satoshis of the senders inputs is less than their outputs plus | ||
the funding_sats, specified earlier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the funding_sats, specified earlier | |
the `funding_satoshis`, specified earlier |
02-peer-protocol.md
Outdated
|
||
### The `funding_signed2` Message | ||
|
||
This message contains witness data for for the inputs that were |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message contains witness data for for the inputs that were | |
This message contains witness data for the inputs that were |
02-peer-protocol.md
Outdated
- MUST return an error if: | ||
- the `feerate_per_kw_funding` is not greater than the previously | ||
negotiated rate. | ||
- MUST return an error, but not fail the channel if: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the remote node know that it should not fail the channel upon receiving the error ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is, at best, a case for the 'soft error' idea that @rustyrussell has been talking about. Clearly the node that sent you the init_rbf
is confused, the polite thing to do in the case is to inform them that they've made a mistake. The other option, without a 'soft error' is probably to ignore the init_rbf
request entirely.
You make a good point though, what we really want here is that a node MUST NOT send an init_rbf
if it has already sent a funding_locked
message.
* [`u16`:`num_outputs`] | ||
* [`num_outputs*output_info`:`output_info`] | ||
|
||
#### Requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it has been stated for other messages, maybe :
Either node:
- MAY omit this message
?
- MAY include additional inputs. | ||
- MAY set the `num_inputs` to zero. | ||
- MUST transmit all outputs, excluduing the channel funding output. | ||
- MAY include at least one output with the `sats` set to zero, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also specify which output to reduce ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm reading further it seems that the opener should pay for the fees. Maybe we then should explicit the receiver of init_rbf
to check that the reduced (or removed) output is receiver's change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, retransmitting all of the outputs would give you the opportunity to update the outputs as necessary.
The change output is calculated dynamically by each of the peers, and is identifiable in the output set as "the output of value zero", so you wouldn't need to update it. In fact, the change calculation should 1) drop the change output if it's below dust and 2) add or subtract any resultant amount to the opener's satoshis in the funding output. So, you wouldn't need to update the outputs on an rbf, unless you wanted to preserve your funding output's value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, you wouldn't need to update the outputs on an rbf, unless you wanted to preserve your funding output's value.
That's what I assumed: doesn't it make it more simple for both participants if the funding amount stayed the same but the fee bump was taken on the change output of the funder instead ?
Since both add inputs to the tx, they will most likely also include change outputs, at least that what I assumed when writing that.
My question was then whether to introduce an explicit MUST on the receiver side to check that the tx the funder sends does not greeds on the fundee's change output (which implems would of course do, but ..)
But maybe I'm missing something here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I assumed: doesn't it make it more simple for both participants if the funding amount stayed the same but the fee bump was taken on the change output of the funder instead ?
It's up to the funder to know what it's estimated change burden will be and add sufficient funds such that the total amount of funding is untouched by a fee-bump, if that's what they desire.
does not greeds on the fundee's change output (which implems would of course do, but ..)
Fees never come out of the accepter's change; though it might (hypothetically) start eating out of their funding balance if the opener's balance is consumed by change. Reconsidering this and the point you made earlier, a nack_rbf
message (or a 'soft-error') might be appropriate here. This would allow the second node to disallow a fee update that would eat into the entirety of the fundchannel amount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool work! Thanks to pushing this forward
What's your thoughts about splitting the transaction composition part to enable batching from the dual-funding channel part ? There is at least 3 cases where it may be reused :
- channel closing
- channel novation (or splicing)
- channel opening
I think you need one party among all being the coordinator to act as tie-breaker and avoid endless input/output add, fees changes dependencies (though there is other design issue like fees management, belongs more to the ml)
09-features.md
Outdated
@@ -27,7 +27,8 @@ These flags may only be used in the `init` message: | |||
| 4/5 | `option_upfront_shutdown_script` | Commits to a shutdown scriptpubkey when opening channel | [BOLT #2][bolt02-open] | | |||
| 6/7 | `gossip_queries` | More sophisticated gossip control | [BOLT #7][bolt07-query] | | |||
| 10/11 | `gossip_queries_ex` | Gossip queries can include additional information | [BOLT #7][bolt07-query] | | |||
| 12/13| `option_static_remotekey` | Static key for remote output | [BOLT #3](03-transactions.md) | | |||
| 12/13 | `option_static_remotekey` | Static key for remote output | [BOLT #3](03-transactions.md)| | |||
| 28/29 | `option_dual_fund` | Use v2 of channel open, enables dual funding | [BOLT #2](02-peer-protocol) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be rebased for flat features ofc but what context should we assign, IN- (you should let old nodes routes through you even if you enforce dual funding opening with your local peers) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we waiting on a spec change for 'N+/-', it's not currently listed in the features doc (but C+/- is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to discuss this during the next meeting, but we still need some clarifying on feature bits (potentially the N+/N- modifiers).
|
||
Accepter sends their `funding_satoshi` value here instead of allowing the opener to derive | ||
it from their `funding_compose` response so that the opener can notionally decide whether | ||
to complete the opening without exposing their output set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we require ownership proof of input to avoid opener freely probing utxo set of accepter ? Though it may be tricky if it's a non-templated P2WSH, accepter need to solve the script...
Also, maybe introduce concept of a quarantine utxo set ? If input as been announced as part of a cancelled dual-funding, it should be used in priority for next ones, and you should avoid to mix it with lambda onchain transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vectors for revelation of your utxo set heavily depends on how much/how many inputs an accepter chooses to contribute to a channel, and will vary depending on the policy individual nodes set for their threshold and amount they've decided to contribute to a channel.
Currently, the c-lightning implementation mitigates this (slightly) by promoting any exposed utxos to be first eligible for spending. A repeated inquiry should expose the exact same utxos, until they're spent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this for a first version. We may TLV ownership proof in a later one.
Maybe we should add privacy note somewhere on the risks, including some implementation advice like the one you're describing without this being mandatory.
- MUST send at least one `funding_add_input` message | ||
- MUST NOT send a total count of more than 64 inputs, across all `funding_add_input` messages. | ||
- if is the `accepter`: | ||
- MAY omit this message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MAY omit this message | ||
|
||
The sending node: | ||
- MUST NOT send `funding_add_input` if it has already transmitted `funding_add_complete` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't write this common sense requirements in the current specs w.r.t funding_created which must come after open/accept sequence though obviously everyone encode this in their state machine ? IMO makes the spec unnecessarily verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure this is common sense -- it needs to be specified somewhere
- MUST send this after `accept_channel2` has been exchanged, but before `funding_add_complete` | ||
- MUST add all sent inputs to the funding transaction | ||
- MUST NOT re-transmit inputs it has already received from the peer | ||
- MUST ensure each `input_info` refers to a non-malleable (segwit) UTXO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may check also that's a mature coinbase (but generally we may have issues with light-clients, unable to verify mempool rejection reasons and starting to RBF as crazy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point. This isn't currently checked on funding transactions.
of zero. This will be used for change, or discarded if its value is | ||
be below `dust_limit_satoshis`. | ||
|
||
Change is calculated as the sum of the opener's `input_info`.`sats` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the opener paying only the fees, it means if Alice funds with 1 input as opener but Bob puts max input 16, Bob utxo aggregation cost is externalized to Alice ? Sure you can hardcode or use a smart-fee-algo(tm) to reject but incentive-level it means accepter should seek this limit to free-ride ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yess, I've got a draft for how to re-distribute fees (and a tweak to this protocol) I'm about to send to the mailing list. :)
the `funding_txid`, instead of the `temporary_node_id`. | ||
|
||
|
||
### The `funding_signed2` Message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message name is a bit confusing, funding_signedv1 refers to accepter giving commitment tx signatures to fundee, here it refers to signatures exchange for the funding tx (even now wondering if a funding_signed distinct from commitment_signed wasn't a spec oversight..)
- MUST send a `feerate_per_kw_funding` greater than the most recently | ||
negotiated rate. | ||
- MAY update the `feerate_per_kw`, the commitment transaction feerate. | ||
- MAY include additional inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC I think that hurts BIP 125 rule 2, you can't add new inputs to rbf'ed transactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a misinterpretation of rule 2; an "unconfirmed input" is a utxo that has not yet been confirmed in a block (mined).
- The replacement transaction may only include an unconfirmed input if that input was included in one of the original transactions. (An unconfirmed input spends an output from a currently-unconfirmed transaction.)
I think this is an oversight of the spec though; it should strictly exclude unconfirmed inputs (and thus disallow chaining), as they're difficult for the peer to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, thanks the rule is more liberal than I believed.
Yes sure the LN spec should disallow unconfirmed input to avoid chaining. That would be a real attack vector for zero-conf channels if people keep doing them. Maybe even ask for few confirmations to avoid messy 1-depth reorg edges cases.
RBF workflow, the RBF attempt MUST be abandoned. | ||
|
||
|
||
### Acknowledging Replace-By-Fee `ack_rbf` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What problem the ack_rbf is handling ? Can't receiver of init_rbf directly replies with a commitment_signed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, i think it is infact superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update: in order to avoid reversing the order of commitment_signed
exchange, you need to get at least one message back from the peer before you send them the commitment_signed
message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't the init_rbf
sender joins a commitment_signed
and interpret the lack of getting back a commitment_signed
as a refusal (though harder for the state machine I guess) ?
transaction has been broadcast but before a `funding_locked` message | ||
is exchanged. | ||
|
||
Once an `init_rbf` message has been successfully ack'd by the accepter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if rbf is refused by party ? Maybe we should recommend to always have an output available to rbf and avoid input being stuck in the mempool for 2 weeks if funding tx feerate isn't enough to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you mean 'output available to cpfp'. :)
Including an output for CPFP might be one strategy here, but there's no reason to make it standard in the spec, as its not required and if not a 'useful' output (i.e. a withdrawal or another closing output etc) contributes to utxo bloat.
Given that RBF is an option for opening transactions, it seems reasonable to not make CPFP mandatory here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning the UTXO bloat it's likely they add a change output so they may use it for CPFP.
I was thinking about the use-case of a zero-conf chans, for the party bearing the risk, adding a CPFP output and disabling RBF would be a lightweight mempool-enforced protection (though don't prevent malicious peer to directly send double-spend to miner..).
Sure no need to make it mandatory, it's up to the party if they want to be safe against stuck-in-the-mempool cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nicely spec-ed, thanks @niftynei !
It's honestly a bit hard for me to find meaningful design comments without trying to implement this, at first glance it looks good and there are already many interesting discussions/comments.
I was wondering how the batching would work in terms of fees. If we have A <-> B <-> C, it seems to me that the naive version would have A pay all the fees, which is quite unfair (A shouldn't pay for a B <-> C channel). How do you think distributing the fee between all openers would work?
|
||
The receiving node: | ||
- if the `witness_stack` length exceeds `max_witness_len`: | ||
- MUST error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that mean exactly? Sending an error message to the other party and forgetting everything about this channel creation attempt?
I haven't thought about it deeply enough yet, but I feel that aborting during the funding flow may have some subtle edge cases. Are there some gotchas you ran into while implementing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. You've highlighted what I believe is the most subjective error case.
How the opener chooses to handle this is hard to enforce. At this point, they have all the information necessary to successfully broadcast the transaction (the accepter is the only peer that transmits their signatures). If they choose to accept the lower effective feerate anyway and broadcast the tx, there's nothing stopping them. What we're looking to accomplish is incentivize the peer (accepter) to provide an accurate witness length, so that the agreed upon feerate is accurate. The way I've chosen to incentivize this is by threatening that the opener will cancel the open on them.
So yes, the opener would send the peer and error message and, ideally, spend an input to the tx such that the opening transaction was definitively invalidated.
f150a15
to
45fbb33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the most recent feedback @t-bast and @ariard. I just sent a message to the mailing list with a draft proposal for how to break out a more "universal collaborative transaction" protocol. It's a bit different than the protocol proposed here (namely in that it includes two removal messages), but should be largely compatible with the work already specified herein.
45fbb33
to
eb9bec1
Compare
ebfdc9e
to
36561fc
Compare
First draft of flow for v2 of channel establishment.
since we now require the opener to 'pre-send' the total number of inputs and outputs that they're going to be including in the funding tx, if the number of actual inputs and outputs that they send is less than this, we can fail the channel. we do this because we base our input selection on the number of inputs and outputs that they send. having a lower number means that we might be in violation of the rule so we fail this requirement also
fixes incorrect calculation of funding output's weight.
for v2 channel establishment we switch from the temporary channel_id to the channel_id derived from the funding_txid after both funding_compose messages have been exchanged, as at this point both opener/accepter have the ability to derive the funding_txid which is required for the channel_id.
On further consideration, the channel_id encapsulates all of the necessary info to determine which funding transaction attempt that the funding_locked message pertains to
We fix the total permitted contribution from the acceptor node to 4, and remove it from the protocol.
Fix the reserve to always be 1% of the channel balance, or the dust_limit, whichever is greater. This reduces the complexity around calculating the reserve balance.
Shorten the round trips required by only having the accepter send their sigs to the opener. This means only the opener is able to compose the funding transaction completely, and that only they are in control of publishing it, which is the same as current implementations.
Break funding_compose into 3 messages, to allow for asynchronous input and output construction. This is done to enable multi-party funding transaction construction
Further limit the total inputs/outputs allowed from a peer
Makes a non-malleable input more concise, and adds missing restriction that input must be confirmed, i.e. mined.
36561fc
to
2e40838
Compare
@rustyrussell suggested to add Lloyd Fournier's deterministic generation of funding output to this PR, see https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-December/002910.html |
Superceded by #851 |
This is a first draft proposal for adding a second open channel version. This will empower either side of the channel to contribute to the funding transaction.
Broadly speaking, the largest change between this and version one of
open_channel
is that both sides now construct and broadcast the funding transaction. This adds a few more communication rounds, as the commitment transaction signatures can't be exchanged until both sides have the info necessary to construct the funding transaction.Errata
Employs strawman idea of using a sha of the revocation points for the channel_id as put forth here https://lists.linuxfoundation.org/pipermail/lightning-dev/2018-December/001709.html