From 9333ba468449ead3b16d9a2d8528fde147920171 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 7 Nov 2024 20:28:50 +0800 Subject: [PATCH] sweep: make sure nil tx is handled After previous commit, it should be clear that the tx may be failed to created in a `TxFailed` event. We now make sure to catch it to avoid panic. --- sweep/fee_bumper.go | 22 ++++++++++++++++------ sweep/fee_bumper_test.go | 14 +++++++------- sweep/sweeper.go | 13 ++++++++++++- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index a6e669554..2348992f1 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -75,7 +75,17 @@ const ( // TxPublished is sent when the broadcast attempt is finished. TxPublished BumpEvent = iota - // TxFailed is sent when the broadcast attempt fails. + // TxFailed is sent when the tx has encountered a fee-related error + // during its creation or broadcast, or an internal error from the fee + // bumper. In either case the inputs in this tx should be retried with + // either a different grouping strategy or an increased budget. + // + // NOTE: We also send this event when there's a third party spend + // event, and the sweeper will handle cleaning this up once it's + // confirmed. + // + // TODO(yy): Remove the above usage once we remove sweeping non-CPFP + // anchors. TxFailed // TxReplaced is sent when the original tx is replaced by a new one. @@ -269,8 +279,10 @@ func (b *BumpResult) String() string { // Validate validates the BumpResult so it's safe to use. func (b *BumpResult) Validate() error { + isFailureEvent := b.Event == TxFailed || b.Event == TxFatal + // Every result must have a tx except the fatal or failed case. - if b.Tx == nil && b.Event != TxFatal { + if b.Tx == nil && !isFailureEvent { return fmt.Errorf("%w: nil tx", ErrInvalidBumpResult) } @@ -285,10 +297,8 @@ func (b *BumpResult) Validate() error { } // If it's a failed or fatal event, it must have an error. - if b.Event == TxFatal || b.Event == TxFailed { - if b.Err == nil { - return fmt.Errorf("%w: nil error", ErrInvalidBumpResult) - } + if isFailureEvent && b.Err == nil { + return fmt.Errorf("%w: nil error", ErrInvalidBumpResult) } // If it's a confirmed event, it must have a fee rate and fee. diff --git a/sweep/fee_bumper_test.go b/sweep/fee_bumper_test.go index 76a2a17d1..72b2da34e 100644 --- a/sweep/fee_bumper_test.go +++ b/sweep/fee_bumper_test.go @@ -91,13 +91,6 @@ func TestBumpResultValidate(t *testing.T) { } require.ErrorIs(t, b.Validate(), ErrInvalidBumpResult) - // A failed event without a tx will give an error. - b = BumpResult{ - Event: TxFailed, - Err: errDummy, - } - require.ErrorIs(t, b.Validate(), ErrInvalidBumpResult) - // A fatal event without a failure reason will give an error. b = BumpResult{ Event: TxFailed, @@ -118,6 +111,13 @@ func TestBumpResultValidate(t *testing.T) { } require.NoError(t, b.Validate()) + // Tx is allowed to be nil in a TxFailed event. + b = BumpResult{ + Event: TxFailed, + Err: errDummy, + } + require.NoError(t, b.Validate()) + // Tx is allowed to be nil in a TxFatal event. b = BumpResult{ Event: TxFatal, diff --git a/sweep/sweeper.go b/sweep/sweeper.go index de611eba0..16fb81ded 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1669,6 +1669,14 @@ func (s *UtxoSweeper) monitorFeeBumpResult(set InputSet, // in sweeper and rely solely on this event to mark // inputs as Swept? if r.Event == TxConfirmed || r.Event == TxFailed { + // Exit if the tx is failed to be created. + if r.Tx == nil { + log.Debugf("Received %v for nil tx, "+ + "exit monitor", r.Event) + + return + } + log.Debugf("Received %v for sweep tx %v, exit "+ "fee bump monitor", r.Event, r.Tx.TxHash()) @@ -1694,7 +1702,10 @@ func (s *UtxoSweeper) handleBumpEventTxFailed(resp *bumpResp) { r := resp.result tx, err := r.Tx, r.Err - log.Warnf("Fee bump attempt failed for tx=%v: %v", tx.TxHash(), err) + if tx != nil { + log.Warnf("Fee bump attempt failed for tx=%v: %v", tx.TxHash(), + err) + } // NOTE: When marking the inputs as failed, we are using the input set // instead of the inputs found in the tx. This is fine for current