Zero-fee commitments support#660
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| /// `option_anchor_zero_fee_commitments`. All the caveats and warnings in | ||
| /// [`AnchorChannelsConfig`] still apply. | ||
| /// [`AnchorChannelsConfig`]: Config::anchor_channels_config | ||
| pub enable_zero_fee_commitments: bool, |
There was a problem hiding this comment.
I don't think we'll wan to add a new flag here that's probably hard to understand for the user? Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?
Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc
There was a problem hiding this comment.
FWIW, thinking about it again it seems that we should never set negotiate_anchor_zero_fee_commitments until we're positive our chain sources support submitpackage/TRUC, no? And once we are positive, we would always set it?
There was a problem hiding this comment.
Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?
Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.
Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc
Yes will expand
There was a problem hiding this comment.
Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.
Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)? Keep in mind that communicating these three modes to the user is already very hard, they always have a very hard time understanding what this means. Now, how would we communicate any changed assumptions for 0FC here? If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?
There was a problem hiding this comment.
Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)?
Let me see I don't think they change anything ? Whether to enable or disable 0FC channels is orthogonal to these modes ie trusted_peers_no_reserve and per_channel_reserve_sats should have no influence on whether we enable 0FC channels (only that per_channel_reserve_sats should be set to some value). I suspect you don't agree :)
If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?
It seems to me trusting our counterparty -> keeping 0 reserve is orthogonal to whether the user wants to enable 0FC channels ? for example a user trusts their counterparty, but wants to wait for greater adoption of Core v29+ before using 0FC channels.
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
cb1cdf9 to
c874049
Compare
|
Marked as draft: I think we should wait for electrum and esplora submit package support before merging this PR. |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
c874049 to
ef3ba7a
Compare
|
Successfully opened some 0FC channels, made payments, and force closed them with the esplora diff in this branch. https://mutinynet.com/tx/508a954d85f5b7daf224a2fdc54ea6de9a26c0f62f7d58284bf61c3cdfd346e6 |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
ef3ba7a to
3ebd017
Compare
tankyleo
changed the title
AnchorChannelsConfig::enable_zero_fee_commitments
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
3ebd017 to
eda13d4
Compare
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
4 times, most recently
from
e136f33 to
d4a2a04
Compare
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
9 times, most recently
from
771f45b to
a7c7911
Compare
|
@benthecarman lol codex had the same complaint when I ran it, ask codex to use POST not GET :) |
Maybe worth a comment somewhere that the default supports submit package so codex doesn't get mad every time lol |
|
🔔 1st Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
baff48e to
95a84fa
Compare
|
🔔 2nd Reminder Hey @benthecarman! This PR has been waiting for your review. |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
95a84fa to
c1eb972
Compare
|
Addressed ben's remaining comments, and squashed. No further changes. |
`BroadcasterInterface::broadcast_transactions` requires that any passed vector containing multiple transactions must be a single child together with its parents. We will lean on this contract in upcoming commits, so here we fix a case where we broke this contract.
Implementations of `BroadcasterInterface` cannot assume any topological ordering on the transactions received, so here we order the received transactions before adding them to the broadcast queue. Any consumers of the queue can now assume all transactions received to be topologically sorted. Codex wrote the tests.
The patch adds support for the `broadcast_package` method added in electrum protocol v1.6. Upcoming commits will require this patch to pass CI.
The mempool/electrs docker image used in those tests only supports submitpackage via the esplora interface, not the electrum interface. We also bump the Bitcoin Core version used in kotlin and python tests to support ephemeral dust.
Do this as early as possible during startup, only if `anchor_channels_config` is set.
We rely on the `BroadcasterInterface` contract whereby any multi-transaction vector must be a single child and its parents, and must be broadcasted together as a package using `submitpackage`. In a prior commit, we added the guarantee that any packages received from the broadcast queue are already topologically sorted, and hence can be passed directly to the `submit_package` Bitcoin Core RPC.
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
c1eb972 to
4e72af0
Compare
|
Rebased, update the postgres integration workflow, no other changes. |
|
🔔 3rd Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
4 participants
No description provided.