Wumbo! #1425
Wumbo! #1425
Conversation
valentinewallace
force-pushed
the
2021-04-wumbo
branch
2 times, most recently
from
e4c156c to
3445214
Compare
Codecov Report
@@ Coverage Diff @@
## main #1425 +/- ##
==========================================
+ Coverage 90.84% 91.96% +1.12%
==========================================
Files 74 75 +1
Lines 41288 49132 +7844
Branches 41288 49132 +7844
==========================================
+ Hits 37507 45184 +7677
- Misses 3781 3948 +167
Continue to review full report at Codecov.
|
| pub const MAX_FUNDING_SATOSHIS_NO_WUMBO: u64 = 1 << 24; | ||
|
|
||
| /// Total bitcoin supply in satoshis. | ||
| pub const TOTAL_BITCOIN_SUPPLY_SATOSHIS: u64 = 21_000_000 * 1_0000_0000; |
There was a problem hiding this comment.
Just a tiny nitpick on naming (not blocking): I feel like "total" can be misinterpreted in various ways such as representing the total supply so far and not necessarily year 2140. If the intention is to communicate the maximum number of sats possible in any one channel, then maybe a name like MAX_SATOSHI_LIMIT would be more accurate?
There was a problem hiding this comment.
"Total bitcoin supply" is referred to quite a few times in the codebase as 21 million bitcoin, so I like that it's consistent with that
valentinewallace
force-pushed
the
2021-04-wumbo
branch
from
3445214 to
451e096
Compare
| @@ -58,9 +58,12 @@ use ln::chan_utils::CommitmentTransaction; | |||
| #[test] | |||
| fn test_insane_channel_opens() { | |||
| // Stand up a network of 2 nodes | |||
| use ln::channel::TOTAL_BITCOIN_SUPPLY_SATOSHIS; | |||
| let mut cfg = UserConfig::default(); | |||
| cfg.peer_channel_config_limits.max_funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS + 1; | |||
There was a problem hiding this comment.
Is there any need to use TOTAL_BITCOIN_SUPPLY_SATOSHIS here? Seems strange to use an invalid value when any valid value would suffice.
There was a problem hiding this comment.
It allows us to cover both the cases of config.max_funding_satoshis and the TOTAL_BITCOIN_SUPPLY check without recreating the channels with a new config. If it were lower, we'd never get to the TOTAL_BITCOIN_SUPPLY check I think
| /// Default value: 2^24. | ||
| // Note that this default was chosen because it was the maximum funding amount prior to wumbo | ||
| // channels being introduced. | ||
| pub max_funding_satoshis: u64, |
There was a problem hiding this comment.
Why have a config parameter? If we advertise support for wumbo, wouldn't it be surprising to a peer if they couldn't open a channel >= 2^24?
There was a problem hiding this comment.
Users are likely to implement it anyway via the accept_channel hooks, plus its a really important security parameter - what level of risk are you willing to tolerate if the channel funds somehow get lost because you went offline too long?
valentinewallace
dismissed stale reviews from dunxen, vincenzopalazzo, and TheBlueMatt
via
98506bf
valentinewallace
force-pushed
the
2021-04-wumbo
branch
from
451e096 to
98506bf
Compare
MAX_FUNDING_SATOSHIS will no longer be accurately named once wumbo is merged. Also, we'll want to check that wumbo channels don't exceed the total bitcoin supply
valentinewallace
force-pushed
the
2021-04-wumbo
branch
from
98506bf to
fd302a3
Compare
valentinewallace
dismissed stale reviews from dunxen, TheBlueMatt, and vincenzopalazzo
via
99b06ea
valentinewallace
force-pushed
the
2021-04-wumbo
branch
2 times, most recently
from
99b06ea to
3af4165
Compare
and a bonus grammar fix
Also redefine MAX_FUNDING_SATOSHIS_NO_WUMBO to no longer be off-by-one.
valentinewallace
force-pushed
the
2021-04-wumbo
branch
from
3af4165 to
5cfe19e
Compare
|
Squashed! |
Wumbo channels! 🥳
I'm not sure if we care about the
TOTAL_BITCOIN_SUPPLYcheck, so I haven't tested it yet. Let me know if we want it and I'll add testing.