openingd: bound funding_satoshis by total bitcoin supply - #9368
Conversation
dc74660 to
0fb3f26
Compare
| * - otherwise: | ||
| * - MUST set `funding_satoshis` to less than 2^24 satoshi. | ||
| */ | ||
| if (!feature_negotiated(state->our_features, |
There was a problem hiding this comment.
Is leaving setup_channel_funder() and the lightningd-side checks unpatched a deliberate scope decision (only fixing remotely-triggerable crashes)? The PR description explains the dualopend v2-flow uncertainty but doesn't mention these two sites
There was a problem hiding this comment.
Yes, I haven't tested what happens if the node operator chooses an absurdly high funding amount, so I'm not sure whether that gets rejected gracefully or by crashing openingd. When fuzzing with smite we generally only detect bugs that can be triggered by an outside peer/attacker.
I've updated the PR description to mention this gap in the PR.
| bool anchors_negotiated(struct feature_set *our_features, | ||
| const u8 *their_features); | ||
|
|
||
| struct amount_sat max_channel_funding(const struct feature_set *our_features, |
There was a problem hiding this comment.
Would it be worth moving max_channel_funding() into bitcoin/chainparams.c (or a shared header) so lightningd could reuse it too? Or is the module boundary (openingd vs lightningd) considered a hard line here?
There was a problem hiding this comment.
I personally wouldn't insist on keeping it in openingd. The main reason I put it there is because I was focused on fixing the bug that is triggerable by an outside peer.
IMO it makes sense to merge this bug fix first. If folks care enough about the UX when an operator chooses a funding amount more than 21M, this helper function could be moved to a central location and applied to the funder side in a follow-up PR.
|
|
||
|
|
||
| @pytest.mark.openchannel('v1') | ||
| def test_open_channel_funding_above_max_supply(node_factory, bitcoind): |
There was a problem hiding this comment.
Should the new test also send a case with push_msat alone pushed above max_supply (with funding_satoshis at/near the cap) to directly cover the second crash variant from the issue thread, even though it's mathematically implied by the push_msat <= funding_satoshis invariant? (Not required)
There was a problem hiding this comment.
Done, and also added a case that exercises the third variant where both outputs stay under max_supply.
funding_satoshis values above the total bitcoin supply were not rejected during open_channel/accept_channel negotiation and would later cause libwally to fail and openingd to crash during commitment transaction construction. Reject such funding_satoshis values immediately so that the negotiation terminates gracefully. Fixes: ElementsProject#9225 Changelog-Fixed: `openingd` no longer crashes when a peer opens a channel with a `funding_satoshis` value greater than the total bitcoin supply.
0fb3f26 to
f0ad794
Compare
2 participants
funding_satoshisvalues above the total bitcoin supply were not rejected duringopen_channel/accept_channelnegotiation and would later cause libwally to fail and openingd to crash during commitment transaction construction.Reject such
funding_satoshisvalues immediately so that the negotiation terminates gracefully.The new test covers the v1 funding flow by manually constructing and sending
open_channelmessages with excessivefunding_satoshisvalues and then completing the funding flow throughfunding_createdandfunding_signed.The dualopend sites are also fixed for consistency, though it's not clear whether the crash can even be reached via the v2 flow, and writing a test for the v2 flow would be significantly more complex than the v1 test.
Note that this PR only sanitizes
funding_satoshisvalues received from a peer -- it may still be possible the node operator to choose afunding_satoshisvalue above the total bitcoin supply and cause openingd to crash.Fixes: #9225