Changing maximum htlc value based on channel being public

[Sharmalm]

This Pr solves issue #2851

[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: https://github.com/lightningdevkit/rust-lightning/issues/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 ? https://github.com/lightningdevkit/rust-lightning/blob/2c0fcf21f227ad1a01e4a884dda72b9537c16fae/lightning/src/ln/channel.rs#L3464

[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 :white_check_mark:

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

:exclamation: 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.