-
Notifications
You must be signed in to change notification settings - Fork 407
Add config support for 'their_channel_reserve_proportional_millionths' [#1498] #1619
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1619 +/- ##
==========================================
- Coverage 90.84% 90.78% -0.06%
==========================================
Files 80 80
Lines 44675 44690 +15
Branches 44675 44690 +15
==========================================
- Hits 40583 40571 -12
- Misses 4092 4119 +27
Continue to review full report at Codecov.
|
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.
Nice, generally the code looks good :)! I've yet to review the tests though.
I've left a few suggestions of improvements below.
@@ -795,6 +796,8 @@ pub const MAX_CHAN_DUST_LIMIT_SATOSHIS: u64 = MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS | |||
/// See https://github.com/lightning/bolts/issues/905 for more details. | |||
pub const MIN_CHAN_DUST_LIMIT_SATOSHIS: u64 = 354; | |||
|
|||
pub const MIN_THEIR_CHAN_RESERVE_SATOSHIS: u64 = 1000; |
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.
This should have some docs, including explaining why 1000 was chosen.
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 don't have an exact answer for this, current minimum was 1000, i just extracted it to constant and made it bit more explicit.
I think its just a reasonable value greater than dust limit.
lightning/src/ln/channel.rs
Outdated
/// This is used both for outgoing and incoming channels and has lower bound of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` | ||
pub(crate) fn get_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64, config: &UserConfig) -> u64 { | ||
let their_channel_reserve_proportion = (config.channel_handshake_config.their_channel_reserve_proportion_millionth as f64) / 1000_000.0; | ||
let calculated_reserve = their_channel_reserve_proportion.mul(channel_value_satoshis as f64) as u64; |
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.
As pointed out by CI, I recommend using *
instead of the mul
function, and simply doing something like:
let calculated_reserve = their_channel_reserve_proportion.mul(channel_value_satoshis as f64) as u64; | |
let calculated_reserve = their_channel_reserve_proportion * channel_value_satoshis as f64; | |
cmp::min(channel_value_satoshis, cmp::max(calculated_reserve as u64, MIN_THEIR_CHAN_RESERVE_SATOSHIS))` |
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.
will be using
let calculated_reserve = channel_value_satoshis.saturating_mul(config.channel_handshake_config.their_channel_reserve_proportion_millionth as u64) / 1000_000;
to avoid floating point arithmetic.
lightning/src/util/config.rs
Outdated
/// | ||
/// Default Value is 1% of channel_value, configured as 10,000 millionth | ||
/// Lower Bound on `their_channel_reserve_satoshis` is 1000 sats provided as safe implementation limit. | ||
/// Max `their_channel_reserve_satoshis` can be equal to channel_value i.e 1000,000 millionth |
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 find it very strange that the range is upper bound to value that is guaranteed to fail. If that's the case, then why is it even upper bound at all?
If we want to have an upper bound, wouldn't we at least want make the bound a value that's more likely to succeed?
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.
yes the idea was to handle unreasonable reserve configs by docs, because technically even a 99.99% reserve might work. and 60-30 channel_reserve b/w parties is unreasonable but valid.
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.
Ok I still find it a bit strange to bound the value to an upper bound that's guaranteed to fail, instead of just not having any bound at all and just let the channel negotiations fail if the user sets a too high value. But if others are ok with it, I am as well :).
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.
Thanks for tackling this!
074d9e2
to
a6674c7
Compare
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.
LGTM. some grammar and style nits, but code looks right.
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.
Tests looks good!
LGTM except a few nits.
0187274
to
17b4d31
Compare
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.
LGTM, mod one existing bug that we might as well fix here. Please also keep your commit messages to 70-80 chars long, including the title (I know, gotta get good at the twitter-style summaries that lose context for titles...)
lightning/src/util/config.rs
Outdated
/// channel_reserve values greater than 30% could be considered highly unreasonable, since that | ||
/// amount can never be used for payments. | ||
/// Also, if our selected channel_reserve for counterparty and counterparty's selected | ||
/// channel_reserve for us, sum upto or greater than channel_value, channel negotiations will fail. |
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.
Oops, looks like this isn't actually true for outbound channels lol. Should probably fix that - the full_channel_value
test in new_from_req should just be copied into accept_channel
.
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.
the check is already present in accept_channel which behaves similarly, channel.rs:2022
if msg.channel_reserve_satoshis > self.channel_value_satoshis - self.holder_selected_channel_reserve_satoshis {
return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than channel value minus our reserve ({})",
msg.channel_reserve_satoshis, self.channel_value_satoshis - self.holder_selected_channel_reserve_satoshis)));
}
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 doc nits and suggestions.
lightning/src/util/config.rs
Outdated
/// Default value: is 1% of channel_value, configured as 10,000 millionths | ||
/// Minimum value: If calculated proportional value is < 1000 sats, will be treated as 1000 sats | ||
/// instead, 1000 sats is just a safe implementation-specific lower bound. | ||
/// Maximum value: 1,000,000, any values larger than 1Million will be treated as 1Million (or 100%) |
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.
/// Maximum value: 1,000,000, any values larger than 1Million will be treated as 1Million (or 100%) | |
/// Maximum value: 1,000,000, i.e., 100% of the channel value, |
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 think there is information loss in this, will be keeping current version.
As for other nits, i have addressed most of them which seemed legible.
And I don't think using grammar check on PR is legal :P
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 guarantee no grammar checker was harmed in making of this review. :P
6c5d0ba
to
457b135
Compare
Can you rewrite the commit message to stay under 80 chars wide? Looks like its currently wrapped at 90? |
Done |
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 final docs nits before I'm ACK, thanks for bearing with us :)!
lightning/src/ln/channel.rs
Outdated
/// for channels serialized before we included our selected reserve value in the serialized | ||
/// data explicitly. | ||
pub(crate) fn get_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 { | ||
/// This is used both for outbound and inbound channels and has lower bound of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` |
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.
/// This is used both for outbound and inbound channels and has lower bound of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` | |
/// This is used both for outbound and inbound channels and has a lower bound of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` |
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.
- Missing article before countable noun is treated as optional and acceptable grammar in most IDE's for code documentation. (default:disabled in intellij)
- Same with period before paragraph end (default:disabled in intellij)
- Same for ',' after e.g or i.e (default:disabled in intellij)
I am currently using default Intellij grammar for "English(USA)" and it relaxes for these conditions by default.
I guess it could vary a little based on keyboard selection of user.
We are really discussing these IDE settings in PR's, which i don't think is worth our time in PR.
We can standardize them, if we want.
Either we stick to default acceptable grammar or we can decide to just enable those optional validation checks.
At the same time, we don't want to discard all grammar and write illegible docs. We should work towards having acceptable grammar and legible docs.
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.
We are really discussing these IDE settings in PR's, which i don't think is worth our time in PR.
We can standardize them, if we want.
Either we stick to default acceptable grammar or we can decide to just enable those optional validation checks.
I tend to disagree. There are best practices to write documentation in full, grammatically correct sentences, and we should do our best to follow these. I think whether a particular IDE picks up on some of the details doesn't matter all too much.
I also think discussing subtleties of wording, grammar etc. is time well spent in a PR, since it can make or break a good documentation and most importantly the process itself can reveal diverging understandings of the subject at hand.
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.
Given that the rest of the docs in the codebase is mixed with how strict the we are with period before paragraphs ends and similar, I can agree on letting it slip :).
Though I also tend to agree with @tnull here. I don't see why we shouldn't make our best efforts with the grammar in new PRs if it's quick to fix and doesn't take much time, given that suggested grammar is more correct. IMO when it tends to just be a waste of time and be too nit picky is when time is spent on discussing specific words or sentences that are equally correct, and the suggested wording brings no extra clarity.
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.
Its not about whether IDE picks it up or not, it has the functionality but the chosen default:disabled means these rules are considered flexible and acceptable.
I am up for discussing any subtleties, wordings or grammar. But for grammar, saying that only "British English" official rules are "correct" is far from reality.
And I doubt adding a comma after "e.g." makes or breaks a doc. We are not writing a book here. We need to be practical and subjectively decide how strict we are. Specifically when it adds no additional value in understanding.
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.
Mh, I think we're discussing two different things here. Should there be a considerable amount of time spent on splitting hairs regarding individual commas and/or missing articles? No, probably not.
But, I still think reviewers should point anything out that they think would improve the quality of the PR, even if it wouldn't warrant having a long discussion about it. Speaking for myself, most of the nits are really meant as suggestions and/or questions, which, at the end of the day, the author may or may not choose to consider.
lightning/src/ln/channel.rs
Outdated
} | ||
|
||
/// This is for legacy reasons, present for forward-compatibility. LDK versions older than " < 0.0.104" | ||
/// don't know how read/handle values other than default from storage. Hence we use this function |
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 this is a bit confusing:
LDK versions prior to 0.0.104 never persisted any value for holder_selected_channel_reserve_satoshis
at all to storage, and just used the default 1% (with the upper channel value bound, and the lower 1000 sats bound). Such versions never read any holder_selected_channel_reserve_satoshis
value from storage either, not even the default.
LDK versions 0.0.104 and later persists a value for holder_selected_channel_reserve_satoshis
if it differs from the legacy implementation, and uses that value instead.
This function is used to track what the default implementation of the holder_selected_channel_reserve_satoshis
was, before it was configurable, not for the purpose of "not persisting default values".
I actually lean towards not mentioning how the value was actually persisted previously in the docs for this function at all, as it feels a bit misplaced, and should be mentioned in the read/write logic instead if needed as that's where it's of interest. But I'll let you decide if it fits here :)!
I'd therefore change the docs for this function to something like:
Returns the value holder_selected_channel_reserve_satoshis
used to be set to, prior to when it was made configurable.
If you want to include something about serialization, you can also add something like:
The value was made configurable in LDK 0.0.110, although LDK 0.0.104 and later enabled serialization of channels with a value for holder_selected_channel_reserve_satoshis
that differed from the default.
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 mean but we wouldn't have this at all if it weren't for the serialization logic/change, so I think this function needs to specify that its really all about serialization in some way or another. I don't think phrasing it in terms of "when we started writing values other than the default" (as you suggest) is really correct, either, as versions between 104 and 110 dont ever write values at all, they just know how to read values other than the default. I'm open to changes here but I think the comment as it lies is clearer than your suggestion.
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 fair enough :)! I agree on that my suggestion isn't correct either.
What I specifically find confusing is the sentence:
"LDK versions older than " < 0.0.104" don't know how read/handle values other than default from storage.",
as those versions under no circumstances read the value from the storage at all, not even the default value. But, given the discussion about being too nit picky above, maybe that applies to me here as well 😁.
If @G8XSU thinks changing the wording here would clarify the docs, feel free to change this. Either way, I'm ok with the current wording here as well :)!
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.
Main purpose: Its present for forward-compatibility, so that we don't persist default values for channels created by older versions. This is for the case when we have to roll-back to previous version, we have at-least those old channels operational.
If in future, we decide to remove forward compat with <104, we can get rid of this function/logic.
I think this information is conveyed.
Didn't want to make it anymore complex than that.
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.
Generally looks good, just one more nit and a few minor suggestions.
lightning/src/util/config.rs
Outdated
/// as 1000 sats instead, which is a safe implementation-specific lower bound. | ||
/// Maximum value: 1,000,000, any values larger than 1 Million will be treated as 1 Million (or 100%) | ||
/// instead, although channel negotiations will fail in that case. | ||
pub their_channel_reserve_proportion_millionth: u32 |
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: For the sake of consistency with other similarly named variables, this should probably be proportional_millionths
.
pub their_channel_reserve_proportion_millionth: u32 | |
pub their_channel_reserve_proportional_millionths: u32 |
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.
Oh, i think matt mentioned it earlier as well. I might have misunderstood, i thought its for comments only. Will pluralize field name as well.
lightning/src/ln/channelmanager.rs
Outdated
@@ -1635,7 +1635,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
} | |||
} | |||
|
|||
/// Gets the current configuration applied to all new channels, as | |||
/// Gets the current configuration applied to all new channels |
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.
While this is trivial so kinda nbd, in general more PRs good - if something is trivial we can land the PR in 5 minutes, so best not to bundle things too much (I can be guilty of it too), just a general note for the future.
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.
💯
0fd574c
to
eb62099
Compare
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.
ACK eb62099
Edit: Scratch my comment about the new name of the config field.
However, please update the commit message for the config field as that's currently still using their_channel_reserve_proportion_millionth
and not the updated name.
lightning/src/ln/channel.rs
Outdated
// Test that `new_outbound` and `new_from_req` creates a channel with the correct value set | ||
// for `their_channel_reserve_proportion`, when configured with a valid percentage value. |
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 was trying to convey in my previous suggestion #1619 (comment) that I think this comment is a bit confusing, but I realize that I probably wasn't wasn't very clear :) :
The test asserts that the channel_reserve
s are set to the correct value when the config value is configured with a valid value, and I think it would make more sense to word the comment to covey that instead of referring to that the config value itself has a correct value set. The test asserts the channel_reserve
values, and doesn't explicitly assert that the config value itself is correct, although that is of course implicitly correct as well.
This isn't a big of a deal, but feel free to change this if you'd like. Either way I'm ok :).
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 am not sure if i understand this correctly,
We shouldn't be testing the setters anyways(its trivial), we should test the functionality resulting from config.
I will clarify the 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.
Thanks, your clarification addressed exactly what I found confusing :)!
Okay, now I'm confused by the discussion: as of 7c90ee3, it's still Other than that, LGTM. |
Fixed it |
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.
ACK 93ccd7f
Absolute final nits:
Use ` chars around their_channel_reserve_proportional_millionths in the commit message header instead of ' chars, so that it's displayed as their_channel_reserve_proportional_millionths
instead of 'their_channel_reserve_proportional_millionths'.
The first sentence in the commit message body is also a bit hard to understand, and has been updated in the config docs since it was written. If you want to, update it to something like the first sentence in the config flag docs i.e.:
Configures the counterparty's channel reserve proportional to the channel value , i.e., their_channel_reserve_satoshis
for both outbound and inbound channels.
Though these are absolutely not requirements, so feel free to merge this without changing this.
Git cli wasnt allowing for that char, was omitting the field altogether. |
Ok fair enough :) |
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.
LGTM.
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.
Oops, we shouldn't check in lightning/ldk-net_graph-v0.0.15-2021-05-31.bin
. Can you remove that and then we can land this (maybe add it to gitignore, I guess?)
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.
ACK f509398
I've left a comment regarding incorrect text alignment, which I thought was fixed in previous commit. Though it's just a minor issue, so feel free to merge this without the fix if you want to land this.
lightning/src/util/config.rs
Outdated
/// | ||
/// Default value: 1% of channel value, i.e., configured as 10,000 millionths. | ||
/// Minimum value: If the calculated proportional value is less than 1000 sats, it will be treated | ||
/// as 1000 sats instead, which is a safe implementation-specific lower bound. |
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 a bit confused, but I'm not sure when the mix between tabs and spaces after the /// got added again? I thought that they were addressed previously. Text alignment should be done with spaces only (and not tabs which is confusing as the rest of the code base uses tabs for indentation).
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.
had fixed it, idk how it got added again, maybe due to my habit.
lightningdevkit#1498] It is proportion of the channel value to configure as the `their_channel_reserve_satoshis` for both outbound and inbound channels. It decides the minimum balance that the other node has to maintain on their side, at all times.
092d1c1
Add configuration support for 'their_channel_reserve_satoshis' [#1498] while channel open/accept
Closes #1498
Closes #1619