Skip to content

Allow to override config defaults for inbound channels on a per-channel basis #3596

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 19, 2025
Merged

Allow to override config defaults for inbound channels on a per-channel basis #3596

merged 2 commits into from
Feb 19, 2025

Conversation

@joostjager
Copy link
Contributor

@joostjager joostjager commented Feb 12, 2025

Fixes #2914

@joostjager joostjager force-pushed the inbound-channel-config-override branch 3 times, most recently from 7caaca3 to 645f146 Compare February 12, 2025 12:13
@codecov
Copy link

codecov bot commented Feb 12, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.64%. Comparing base (1b281f1) to head (fb67798).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3596      +/-   ##
==========================================
- Coverage   88.66%   88.64%   -0.02%     
==========================================
  Files         149      149              
  Lines      116893   116953      +60     
  Branches   116893   116953      +60     
==========================================
+ Hits       103640   103671      +31     
- Misses      10753    10774      +21     
- Partials     2500     2508       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joostjager joostjager force-pushed the inbound-channel-config-override branch 2 times, most recently from 400369e to 5634e6c Compare February 12, 2025 20:06
TheBlueMatt
TheBlueMatt reviewed Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks!

@joostjager joostjager force-pushed the inbound-channel-config-override branch 2 times, most recently from 5014400 to 7d18b65 Compare February 13, 2025 10:20
joostjager
joostjager commented Feb 13, 2025
View reviewed changes
@joostjager joostjager force-pushed the inbound-channel-config-override branch from 7d18b65 to f8eb6b9 Compare February 13, 2025 10:22
@joostjager joostjager added the weekly goal Someone wants to land this this week label Feb 13, 2025
@joostjager joostjager requested a review from tnull February 13, 2025 10:24
@joostjager joostjager marked this pull request as ready for review February 13, 2025 10:25
@joostjager joostjager force-pushed the inbound-channel-config-override branch 3 times, most recently from d43f192 to fb67798 Compare February 13, 2025 10:50
TheBlueMatt
TheBlueMatt reviewed Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

tnull
tnull reviewed Feb 13, 2025
View reviewed changes
@joostjager joostjager force-pushed the inbound-channel-config-override branch 2 times, most recently from 19171a0 to 3a2b885 Compare February 14, 2025 15:37
joostjager
joostjager commented Feb 14, 2025
View reviewed changes
lightning/src/ln/functional_tests.rs
let channel_update = get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, nodes[0].node.get_our_node_id());
assert_eq!(channel_update.contents.fee_base_msat, 555);

get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id());
Copy link
Contributor Author

@joostjager joostjager Feb 14, 2025

Choose a reason for hiding this comment

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

I removed the force close steps that were present previously. Seems this is enough testing for manual acceptance?

@joostjager joostjager force-pushed the inbound-channel-config-override branch from 3a2b885 to c088dcd Compare February 14, 2025 15:45
@joostjager
Copy link
Contributor Author

joostjager commented Feb 14, 2025

Made the argument on the public api Option<InboundChannelConfigOverrides> to allow callers to pass None.

@joostjager joostjager requested a review from tnull February 14, 2025 15:49
joostjager
joostjager commented Feb 14, 2025
View reviewed changes
lightning/src/ln/functional_tests.rs

let as_channel_ready = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReady, nodes[0].node.get_our_node_id());
nodes[1].node.handle_channel_ready(nodes[0].node.get_our_node_id(), &as_channel_ready);
let as_channel_ready = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReady, nodes[1].node.get_our_node_id());
Copy link
Contributor Author

@joostjager joostjager Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Needed to extend the test to include channel ready in order to verify the channel update message.

joostjager
joostjager commented Feb 14, 2025
View reviewed changes
@joostjager joostjager force-pushed the inbound-channel-config-override branch from c088dcd to 1be72f2 Compare February 14, 2025 17:43
@joostjager
Allow channel update for accept_underpaying_htlcs
2a3e002 
Extends partial channel updates to optionally include the
accept_underpaying_htlcs flag.
@joostjager joostjager force-pushed the inbound-channel-config-override branch 3 times, most recently from 6edfd23 to b34967a Compare February 19, 2025 13:59
@joostjager
Allow config defaults override for inbound channels
02861dd 
This commit introduces a config override struct parameter to the
accept_inbound_channel methods. With manual channel acceptance enabled,
users can modify the default configuration as needed.
@joostjager joostjager force-pushed the inbound-channel-config-override branch from b34967a to 02861dd Compare February 19, 2025 16:17
TheBlueMatt
TheBlueMatt approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks! This is straightforward and reasonably well tested, so just gonna land this.

@TheBlueMatt TheBlueMatt merged commit cdc8e21 into lightningdevkit:main Feb 19, 2025
25 of 26 checks passed
tnull
tnull reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, post-merge ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnull tnull tnull left review comments

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

Assignees

No one assigned

Labels

weekly goal Someone wants to land this this week

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow to override config defaults for inbound channels on a per-channel basis

3 participants

@joostjager @tnull @TheBlueMatt