Changing maximum htlc value based on channel being public

[Sharmalm]

This Pr solves issue #2851

[tnull]

Thanks for looking into this!

Hmm, I don't think we should adjust get_holder_htlc_maximum_msat. Rather, we'd likely want to adjust get_announced_htlc_max to return 10% of capacity or so when the channel is announced, and the currently used 95% otherwise.

Also, if you do this, can you add a test asserting that your change results in the expected behavior?

[tnull]

After some offline discussion with @TheBlueMatt we arrived at the conclusion that we should limit both the htlc_maximum_msat and max_htlc_value_in_flight_msat and switch them based on the announcement status.

That is, the changes likely need to happen in ChannelContext::get_holder_max_htlc_value_in_flight_msat and ChannelContext::get_announced_htlc_max_msat. However, the latter is a slightly orthogonal issue that probably needs to happen in a separate PR, so tracking here now: #2984

[Sharmalm]

Hii, In the issue comment you said we have to limit actual htlc_maximum_msat we set, so to limit it, we have to update pub inbound_htlc_maximum_msat: Option<u64> in channelmanager/channeldetails impl. So I changed pub fn get_holder_htlc_maximum_msat(&self) -> Option<u64> { in channel.rs as we know that inbound_htlc_maximum_msat: context.get_holder_htlc_maximum_msat(),.
Am i missing something here?

[tnull]

Hii, In the issue comment you said we have to limit actual htlc_maximum_msat we set

Yes, but as mentioned above, after some offline discussion we concluded that we want to limit both. However, #2851 pertains the inflight value, so we should likely tackle this issue first, and potentially touch the announced htlc_maximum_msat in a follow-up PR.

so to limit it, we have to update pub inbound_htlc_maximum_msat: Option<u64> in channelmanager/channeldetails impl. So I changed pub fn get_holder_htlc_maximum_msat(&self) -> Option<u64> { in channel.rs as we know that inbound_htlc_maximum_msat: context.get_holder_htlc_maximum_msat(),. Am i missing something here?

No, I don't think this is correct. Limiting in ChannelDetails will only change the value we return via the API, not the actual limit. The change likely needs to happen in ChannelContext::get_holder_max_htlc_value_in_flight_msat.

[Sharmalm]

Ok ,

That is, the changes likely need to happen in ChannelContext::get_holder_max_htlc_value_in_flight_msat and ChannelContext::get_announced_htlc_max_msat. However, the latter is a slightly orthogonal issue that probably needs to happen in a separate PR, so tracking here now:

I have a doubt here, that get_announced_htlc_max_msat is scope of ChannelContext but get_holder_max_htlc_value_in_flight_msat this fucntion is not in scope of ChannelContext. So should i update the get_holder_max_htlc_value_in_flight_msat to include in scope in ChanneContext.

[tnull]

Ok ,

That is, the changes likely need to happen in ChannelContext::get_holder_max_htlc_value_in_flight_msat and ChannelContext::get_announced_htlc_max_msat. However, the latter is a slightly orthogonal issue that probably needs to happen in a separate PR, so tracking here now:

I have a doubt here, that get_announced_htlc_max_msat is scope of ChannelContext but get_holder_max_htlc_value_in_flight_msat this fucntion is not in scope of ChannelContext. So should i update the get_holder_max_htlc_value_in_flight_msat to include in scope in ChanneContext.

I'm not quite following your question? As mentioend above, these are two separate issues, for now we should only make sure the inflight variant isn't limited if the channel is unannounced.

[Sharmalm]

yes i got that, My question was fn get_holder_max_htlc_value_in_flight_msat is not in the scope of ChannelContext, so for solving this particular issue should i update the fn get_holder_max_htlc_value_in_flight_msat so that i comes in the scope of ChannelContext ?

rust-lightning/lightning/src/ln/channel.rs

Line 3464 in 2c0fcf2

fn get_holder_max_htlc_value_in_flight_msat(channel_value_satoshis: u64, config: &ChannelHandshakeConfig) -> u64 {

[tnull]

yes i got that, My question was fn get_holder_max_htlc_value_in_flight_msat is not in the scope of ChannelContext, so for solving this particular issue should i update the fn get_holder_max_htlc_value_in_flight_msat so that i comes in the scope of ChannelContext ?

Ah, no, I think it likely should stay where it's at as we're also using it in Channel::read. However, you probably will need to hand an is_announced: bool flag in or similar.

[Sharmalm]

I have updated the changes in get_holder_max_htlc_value_in_flight_msat, as well as tests related to it, Please review it.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.82%. Comparing base (9325070) to head (7627b20).
Report is 105 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2980      +/-   ##
==========================================
+ Coverage   89.39%   90.82%   +1.42%     
==========================================
  Files         117      118       +1     
  Lines       95514   113356   +17842     
  Branches    95514   113356   +17842     
==========================================
+ Hits        85389   102951   +17562     
- Misses       7903     8260     +357     
+ Partials     2222     2145      -77     

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

[TheBlueMatt]

Hmm, we can't totally ignore the config value for private channels - outside of being useful for privacy the max-in-flight number is useful to limit exposure to loss when closing with a watchtower, etc. I'm not quite sure what the right API is, maybe we make it an Option to let the default differ based on the channel being public?