-
Notifications
You must be signed in to change notification settings - Fork 436
Default to anchors and remove automatic channel acceptance #4354
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
Default to anchors and remove automatic channel acceptance #4354
Conversation
|
👋 I see @jkczyz was un-assigned. |
Hmm, can you ask claude to change those to use the |
ef441ac to
b1ee9e3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4354 +/- ##
=======================================
Coverage 86.01% 86.02%
=======================================
Files 156 156
Lines 102857 102827 -30
Branches 102857 102827 -30
=======================================
- Hits 88474 88453 -21
+ Misses 11876 11865 -11
- Partials 2507 2509 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 similar comment
pending_changelog/4337-manual-channel-accept-default-anchors.txt
Outdated
Show resolved
Hide resolved
b1ee9e3 to
96719d6
Compare
|
This also needs a rebase |
96719d6 to
daf61b3
Compare
|
rebased and renamed channel config to default to anchors |
daf61b3 to
927b6c1
Compare
|
Feel free to squash |
When accepting channels manually, it would fail if the # of peers without funded channels was == `MAX_UNFUNDED_CHANNEL_PEERS`, however, it should fail if the # of peers > `MAX_UNFUNDED_CHANNEL_PEERS`.
Removes the `manually_accept_inbound_channels` config option. In upcoming commit we will default to anchor channels which requires users checking if they have enough onchain funds to cover fees in case of a force close. Hence, we move to always require users to manually accept inbound channels.
Set `negotiate_anchors_zero_fee_htlc_tx` default to true.
Now that anchor channels are the default, rename `test_default_anchors_channel_config` to `test_default_channel_config` and the previous default to legacy.
927b6c1 to
306eea7
Compare
|
✅ Added second reviewer: @valentinewallace |
valentinewallace
left a comment
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.
Some nits but nothing blocking, feel free to take or leave the feedback for follow-up. Thank you for this update! 🫡
| // Assert that `nodes[1]` has no `MessageSendEvent::SendAcceptChannel` in `msg_events` before | ||
| // rejecting the inbound channel request. |
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: isn't this comment still relevant? and below
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.
yeah it applies but it felt redundant. Can re-add if you prefer.
| num_unfunded_channels + peer.inbound_channel_request_by_id.len() | ||
| } | ||
|
|
||
| #[rustfmt::skip] |
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.
In the future, we generally prefer to have rustfmt::skip removals in a separate commit, no need to bother for this PR though.
| revoked_htlc_txn[0].input[0].previous_output | ||
| ); | ||
| assert_eq!( | ||
| node_txn[1].input[1].previous_output, | ||
| revoked_htlc_txn[0].input[0].previous_output | ||
| revoked_htlc_txn[1].input[0].previous_output | ||
| ); |
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's the reason for this diff?
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 honestly not quite sure. At some point this was failing and changing them fixed it. This ordering seems correct though?
| /// [`Event::OpenChannelRequest`]: crate::events::Event::OpenChannelRequest | ||
| /// [`msgs::OpenChannel`]: crate::ln::msgs::OpenChannel | ||
| /// [`msgs::AcceptChannel`]: crate::ln::msgs::AcceptChannel | ||
| pub manually_accept_inbound_channels: bool, |
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's still several occurrences of manually_accept in the codebase atm. Not crucial, but I think at least the pending_changelog one should be updated
| } else { | ||
| let msgs = nodes[1].node.get_and_clear_pending_msg_events(); | ||
| assert!(msgs.is_empty(), "Expected no message events; got {:?}", msgs); | ||
| let events = nodes[1].node.get_and_clear_pending_events(); |
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: I think we can use the new handle_and_accept_open_channel helper here?
| /// [`ChannelManager::list_recent_payments`] for more information. | ||
| /// | ||
| /// Routes are automatically found using the [`Router] provided on startup. To fix a route for a | ||
| /// Routes are automatically found using the [`Router`] provided on startup. To fix a route for a |
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.
Nbd, but usually it's not recommended to put unrelated fixes in a commit, even if trivial
| let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, None]); | ||
| let legacy_cfg = test_legacy_channel_config(); | ||
| let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(legacy_cfg), None, None]); |
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: pre-existing but only need 2 cfgs here and below
| let chanmon_cfgs = create_chanmon_cfgs(2); | ||
| let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| let config = test_default_anchors_channel_config(); | ||
| let config = test_default_channel_config(); |
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: here and throughout the file's diff, we can just pass in None for the config rather than explicitly setting test_default_channel_config
|
|
||
| pub fn test_default_anchors_channel_config() -> UserConfig { | ||
| let mut config = test_default_channel_config(); | ||
| pub fn test_default_channel_config() -> UserConfig { |
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 do wonder if there's an opportunity with these updates to have the default test config match LDK's actual default config. Did you look into how hard that would be? Probably would require too many test updates...
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.
hmmm yeah when I tried using the now test_default_channel_config instead of the legacy one, 100+ tests were failing. I didn't go one-by-one in detail but it didn't seem trivial and I think most of them would require specific fixes. Perhaps something for an LLM
|
@valentinewallace sorry for the intermingling of unrelated fixes in some of the commits. Addressed some of the follow-ups here #4407 |
Closes #4337
Changes
negotiate_anchors_zero_fee_htlc_txdefault to true and removes themanually_accept_inbound_channelsconfig option.Edit: Left the tests with default to non-anchor