Uh oh!
There was an error while loading. Please reload this page.
Splice with mempool fuzz fixes - #4708
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
I have re-verified the key changes against the actual code. Everything is consistent with my prior review:
No issues found. |
bc23128 to
a120ac1CompareI updated #4699 to hopefully no longer trigger false positives. Removed the splice state assert after event handling, because it wasn't reliable. Still there seem a lingering problem (with #4699 applied to this PR) using byte string |
joostjager
commented
Jun 19, 2026
The one above also seems to be a false positive. The queued contribution is not stale state left behind by the aborted tx_init_rbf. It is a newer local splice attempt on node 0 that starts progressing before node 0 processes node 1’s echoed tx_abort. |
ldk-reviews-bot
commented
Jun 20, 2026
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Jun 22, 2026
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
ldk-reviews-bot
commented
Jun 24, 2026
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
| /// Whether the holder contributed local inputs or outputs to the negotiated splice. | ||
| pub has_local_contribution: bool, | ||
There was a problem hiding this comment.
Can't we simply return None instead of Some(SpliceFundingNegotiated{ .. })?
There was a problem hiding this comment.
No because there's a case where we rely on SpliceFundingNegotiated being set to free the holding cell after quiescence terminates.
| if self.holder_commitment_point.current_point().is_none() { | ||
| return Err(ChannelError::WarnAndDisconnect(format!( | ||
| "Channel {} commitment point needs to be advanced once before spliced", | ||
| self.context.channel_id(), | ||
| ))); |
There was a problem hiding this comment.
Wasn't this performed early because it indicates the channel was created before we persisted the current commitment point? (a7ba4dd) Any specific reason to move it later?
There was a problem hiding this comment.
I don't see why the ordering should matter as long as we're still failing because of it. I just wanted to keep the WarnAndDisconnect paths together, and since we can use tx_abort to fail this specific case, we must be quiescent first to do so.
Uh oh!
There was an error while loading. Please reload this page.
ldk-reviews-bot
commented
Jun 25, 2026
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
jkczyz
left a comment
There was a problem hiding this comment.
LGTM
@joostjager Did you have any other concerns left from your earlier comments?
There's no need to inform users of negotiated splices when they're not contributing as it just produces noise. Once they do start contributing, they cannot stop, so we always emit the event going forward. Note that we still emit `Event::ChannelReady` with the new locked funding outpoint for each locked splice, so users can still learn that a splice occurred that way.
Previously, this could result in an acceptor not receiving a `Event::SpliceNegotiationFailed` for a splice in which they reused the same contribution (except for the feerate change). Our API should guarantee that users should always see `SpliceNegotiated` and `SpliceNegotiationFailed` events for splices that they contribute to.
While a contribution may be valid at the time the splice is requested, quiescence still needs to happen, which can affect the balances of the channel as it fully settles all pending state. After doing so, it's possible that the contribution is no longer valid. Since quiescence itself doesn't have a terminal message, we see a `WarnAndDisconnect` event happen.
We keep some `WarnAndDisconnect` cases as mandated by the spec, but otherwise prefer sending `tx_abort` to terminate quiescence and avoid reconnection loops.
Send `tx_abort` to terminate quiescence and avoid reconnection loops.
This mirrors what we do for counterparty `splice_init` messages, making sure we don't accept RBFs once a channel has requested shutdown.
a120ac1 to
19c358eComparejoostjager
commented
Jun 29, 2026
No, nothing left |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4708 +/- ##
==========================================
- Coverage 86.95% 86.94% -0.01%
==========================================
Files 161 161 Lines 111659 111680 +21 Branches 111659 111680 +21 ==========================================
+ Hits 97090 97104 +14 - Misses 12060 12068 +8 + Partials 2509 2508 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ldk-reviews-bot
commented
Jul 3, 2026
Hi @wpaulino, Thanks for your contributions to After too many struggles with bugs, outages, contributor bans, and, finally, a multi-week CI ban, the rust-lightning project is moving off of GitHub for day-to-day development. You can still file issues and access the git tree here, but PRs will now take place exclusively at https://git.rust-bitcoin.org/. As such, this PR has been migrated to https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4708 If you log in using GitHub (or otherwise link your GitHub account from https://git.rust-bitcoin.org/user/settings/security), ownership of your PRs, issues, and comments will automatically transfer. To push updates to this PR, you'll need to use |
from wpaulino/splice-with-mempool-fuzz-fixes into main Reviewed-on: https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4708 Reviewed-by: jkczyz <jkczyz@noreply.gitea.bitcoin.ninja> Reviewed-by: Matt Corallo <matt@noreply.gitea.bitcoin.ninja>
This PR includes a few more bug fixes that were discovered by the
chanmon_consistency_targetafter #4657.Fixes#4696.