Add tests and improve documentation for zero reserve channels#873
Add tests and improve documentation for zero reserve channels#873tankyleo wants to merge 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
Let me know how this sounds, is that the testing you had in mind ? |
|
vss tests don't compile but yeah lgtm |
tankyleo
force-pushed
the
2026-04-zero-reserve
branch
from
d3e3e57 to
f832421
Compare
|
Fixed the VSS tests, and reworked the documentation wording |
|
🔔 1st Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
src/lib.rs
Outdated
| /// This channel allows the target node to try to steal your funds with no financial | ||
| /// penalty, so this channel should only be opened to nodes you trust. | ||
| /// The target node may have nothing at stake if it tries to steal your funds in this | ||
| /// channel, so only open this channel to nodes you trust. |
There was a problem hiding this comment.
That 'only open this channel to nodes you trust' part is odd. There will only be one channel, there isn't the potential to open this one channel to multiple nodes/other nodes, just the one we're opening to...
FWIW, I preferred the previous version honestly.
tankyleo
force-pushed
the
2026-04-zero-reserve
branch
3 times, most recently
from
46ce6b3 to
da6525a
Compare
|
Reverted comment to the previous version, and added some coverage on full channel balance sends. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks!
Some more comments. Not quite sure about the doc changes yet. Tbh current version on main would be fine by me, but if we want to change them further, it should be a clear improvement.
src/lib.rs
Outdated
| /// | ||
| /// This channel allows the target node to try to steal your funds with no financial | ||
| /// penalty, so this channel should only be opened to nodes you trust. | ||
| /// This channel allows the target node to try to steal your funds in that channel with no |
There was a problem hiding this comment.
nit: Please avoid the 'this channel (..) that channel' duplication (here and below).
There was a problem hiding this comment.
right i think we wanted to clarify the target node cannot steal all your funds, just those in the channel you opened to that node.
src/liquidity.rs
Outdated
| /// balance. This allows clients to try to steal your funds with no financial penalty, so | ||
| /// this should only be set if you trust your clients. | ||
| /// When set, we will allow clients to spend their entire channel balance in the channels | ||
| /// we open to them. This allows clients to try to steal your funds in those channels with |
There was a problem hiding this comment.
Maybe just say 'your channel balance' rather than funds, which should allow to drop the 'in those channels' part? (also note it should be 'which', not 'with' in the current version, but probably better to drop that altogether).
There was a problem hiding this comment.
you mean "which no financial penalty" ? not sure about that :)
There was a problem hiding this comment.
Right hopefully with "channel balance", people understand this only applies to the funds they have in that particular channel, not the sum of all your channel balances across all your channels.
tests/common/mod.rs
Outdated
| let node_a_outbound_capacity_msat = node_a.list_channels()[0].outbound_capacity_msat; | ||
| let node_a_reserve_msat = | ||
| node_a.list_channels()[0].unspendable_punishment_reserve.unwrap() * 1000; | ||
| // TODO: Update this for zero-fee commitment channels |
There was a problem hiding this comment.
Mind opening an issue for this TODO, so we don't forget to actually address it. Could also be a bit more verbose, i.e., what are we waiting for exactly/what do we intend to update?
There was a problem hiding this comment.
Done here thank you
tankyleo
force-pushed
the
2026-04-zero-reserve
branch
from
da6525a to
e3dd7a7
Compare
|
Took your feedback tnull. If you'd rather drop the documentation commit let me know, I agree we should aim for clear improvements here. |
|
🔔 2nd Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
I renamed the |
4 participants
Follow-up to #853