Uh oh!
There was an error while loading. Please reload this page.
Add fuzz coverage for splicing + minor bug fixes - #4219
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4219 +/- ##
==========================================
- Coverage 89.33% 89.32% -0.01%
==========================================
Files 180 180 Lines 138055 138166 +111 Branches 138055 138166 +111 ==========================================
+ Hits 123326 123420 +94 - Misses 12122 12140 +18 + Partials 2607 2606 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| match self.send_stfu(logger) { | ||
| Ok(stfu) => Ok(Some(stfu)), | ||
| Err(e) => { | ||
| log_debug!(logger, "{e}"); | ||
| Ok(None) | ||
| }, | ||
| } |
There was a problem hiding this comment.
Could we have send_stfu return an Option instead?
There was a problem hiding this comment.
It's nice to have the error context come from the method itself IMO
Uh oh!
There was an error while loading. Please reload this page.
| if !is_splice { | ||
| self.assert_no_commitment_advancement(holder_commitment_transaction_number, "initial commitment_signed"); | ||
| self.channel_state = ChannelState::FundingNegotiated(FundingNegotiatedFlags::new()); | ||
| } |
There was a problem hiding this comment.
This method might still be useful for DRYing up async signing. Alternatively, could keep it but move these lines to Channel::funding_tx_constructed.
There was a problem hiding this comment.
It should just be a matter of setting a signer_pending_* var to true and maybe a log statement
TheBlueMatt
left a comment
There was a problem hiding this comment.
All LGTM, thanks! I know its annoying, but one thing worth doing would be to have a splice in the full_stack_target seeds. That will let us catch things that require only small modifications to existing messages (eg overfows in splicing fees/amount logic).
| match self.send_stfu(logger) { | ||
| Ok(stfu) => Ok(Some(stfu)), | ||
| Err(e) => { | ||
| log_debug!(logger, "{e}"); |
There was a problem hiding this comment.
Would context be helpful?
| log_debug!(logger,"{e}"); | |
| log_debug!(logger,"Quiescence pending, waiting to send stfu: {e}"); |
There was a problem hiding this comment.
There's a Attempting to initiate quiescence log at the top of the method that should provide enough context
There was a problem hiding this comment.
There may well be other things going on in the node that cause the logs to appear separately, I don't think two logs in a row is a good excuse to avoid context that won't be distracting.
When we propose quiescence due to a user initiated action, such as a splice, it's possible that the `stfu` message is not ready to be sent out yet due to the state machine pending a change. This would result in an error communicated back to the caller of `ChannelManager::splice_channel`, but this is unnecessary and confusing, the splice is still pending and will be carried out once quiescence is eventually negotiated.
We incorrectly assumed that both commitments must be at the same height, but this is certainly possible whenever updates are proposed in both directions at the same time.
This method was originally intended to DRY the logic between `FundedChannel` and `PendingChannelV2` when handling `funding_tx_constructed`, though it's not very useful anymore.
Splicing should also have fuzz coverage, and it depends on the quiescence protocol.
wpaulino
commented
Nov 11, 2025
I'll have a follow-up for this, will need to familiarize myself with the setup here. I just pushed coverage for splice-out as well. |
| .map(|chan| chan.outbound_capacity_msat) | ||
| .unwrap(); | ||
| if outbound_capacity_msat >= 20_000_000 { | ||
| let contribution = SpliceContribution::SpliceOut { |
There was a problem hiding this comment.
Would be nice to DRY these up somewhat.
| match self.send_stfu(logger) { | ||
| Ok(stfu) => Ok(Some(stfu)), | ||
| Err(e) => { | ||
| log_debug!(logger, "{e}"); |
There was a problem hiding this comment.
There may well be other things going on in the node that cause the logs to appear separately, I don't think two logs in a row is a good excuse to avoid context that won't be distracting.
TheBlueMatt
commented
Nov 12, 2025
Backported in #4221 |
No description provided.