Include htlc_maximum/minimum in ChannelDetails #1297
Conversation
lightning/src/ln/channel.rs
Outdated
| /// Allowed in any state (including after shutdown) | ||
| pub fn get_counterparty_htlc_max_msat(&self) -> u64 { | ||
| return cmp::min( | ||
| self.channel_value_satoshis * 1000 * 9 / 10, |
There was a problem hiding this comment.
Hmm, I'm not so sure about taking the 90% discount on the ChannelDetails, it feels a bit strange to tell users "the htlc max is X" when its really Y. That said, I do think we need to subtract that channel reserve values (both our own and our counterparty's).
Note: same applies for the holder HTLC max above, we probably don't want to change how we announce channels, but we do want to expose it differently in the ChannelDetails.
There was a problem hiding this comment.
So it would look like self.channel_value_satoshis - self.counterparty_selected_channel_reserve_satoshis - self.holder_selected_chan_reserve_msat?
There was a problem hiding this comment.
Yes, with unit conversion. May need an unwrap_or(0)
lightning/src/ln/channel.rs
Outdated
| @@ -4197,6 +4196,15 @@ impl<Signer: Sign> Channel<Signer> { | |||
| ); | |||
| } | |||
|
|
|||
| /// Allowed in any state (including after shutdown) | |||
| pub fn get_counterparty_htlc_max_msat(&self) -> u64 { | |||
There was a problem hiding this comment.
I think this should return an Option and return None if we haven't received counterparty info yet (ie their accept_channel in an outbound channel.
There was a problem hiding this comment.
So check if the channel is_live, is_usable, or is_outbound and if not return None?
There was a problem hiding this comment.
Hmm, dunno if we need to wait that long, we can just check that self.channel_state >= ChannelState::TheirInitSent as u32
lightning/src/ln/channelmanager.rs
Outdated
| /// The maximum amount of msats that can be forwarded by a channel holder in an HTLC. | ||
| pub holder_htlc_maximum_msat: u64, | ||
| /// The minimum amount of msats that can be forwarded by a channel counterparty in an HTLC. | ||
| pub counterparty_htlc_minimum_msat: u64, |
There was a problem hiding this comment.
I think this and the next one probably belong in CounterpartyForwardingInfo instead.
…n to CounterpartyForwardingInfo
|
Made the changes. 56 tests ended up failing. Logs show unable to read unwrap. Not exactly sure how to fix. Probably poor implementation on my junior rust code. |
| @@ -1150,6 +1150,10 @@ pub struct CounterpartyForwardingInfo { | |||
| /// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s | |||
| /// `cltv_expiry_delta` for more details. | |||
| pub cltv_expiry_delta: u16, | |||
| /// The maximum amount of msats that can be forwarded by a channel holder in an HTLC. | |||
| pub holder_htlc_maximum_msat: u64, | |||
There was a problem hiding this comment.
Hmm, this struct is about settings our counterparty sets on HTLCs we forward outbound, I don't think holder_htlc_maximum_msat belongs here, but counterparty_htlc_maximum_msat probably does.
| @@ -5896,6 +5909,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer> | |||
| fee_base_msat: Readable::read(reader)?, | |||
| fee_proportional_millionths: Readable::read(reader)?, | |||
| cltv_expiry_delta: Readable::read(reader)?, | |||
| counterparty_htlc_minimum_msat: Readable::read(reader)?, | |||
| holder_htlc_maximum_msat: Readable::read(reader)?, | |||
There was a problem hiding this comment.
Right, this is tricky - so we can't add new fields in the serialization here as its not backwards/forwards compatible. We have to instead initialize dummy values here, then read them like we previously did and copy them here. We also should probably not store redundant data in our Channel storage and any fields we add to CounterpartyForwardingInfo we should drop from the main Channel struct.
There was a problem hiding this comment.
So it would be done like
let mut minimum_depth = None;
if ver == 1 {
// Read the old serialization from version 0.0.98.
minimum_depth = Some(Readable::read(reader)?);
} else {
// Read the 4 bytes of backwards-compatibility data.
let _dummy: u32 = Readable::read(reader)?;
}
There was a problem hiding this comment.
Yes, we'd want to keep the existing deserialization logic 1-for-1 and then just copy the field into its new location.
There was a problem hiding this comment.
So we would be removing any reference to counterparty_max_htlc_value_in_flight_msat & counterparty_htlc_minimum_msat in the Channel struct and impl's?
There was a problem hiding this comment.
No, just in read and write in the existing CounterpartyForwardingInfo read. We're basically moving the fields from Channel to CounterpartyForwardingInfo, not adding or removing them, so we should really remove them from Channel but then leave the write and read code as-is, only updating it to refer to the new location.
|
Are you still interested in working on this, @BennyBlockchain? |
|
Closing for now as the author appears to have moved on. Happy to reopen it if you want @BennyBlockchain! |
This PR addresses #1287 & #1199 (comment).