Repository: lightningnetwork/lnd (language: go)

Issue:
[bug]: sweep: data race on TxPublisher.currentHeight
`TxPublisher.currentHeight` is not guarded by a mutex but is accessed from multiple goroutines:

- in the `monitor` goroutine https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L718
- from outside calls to `Broadcast` https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L376  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L544-L545  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L1124-L1126  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L1181
- in the `handleFeeBumpTx` goroutine  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L856-L857  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L864-L865  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L1124-L1126  https://github.com/lightningnetwork/lnd/blob/e8a1d4876eb6477dbbaf0287e711ce9de56084ee/sweep/fee_bumper.go#L1181


Discussion / hints:
Can prob throw in an https://pkg.go.dev/sync/atomic#Int32
Thanks for reporting! Also found this issue while fixing the itest flakes and have a local branch already - still needs more work, meanwhile will use `atomic` to provide a temp fix.

Fix patch:
diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go
index 61e63cc71d..a0e558cbb6 100644
--- a/sweep/fee_bumper.go
+++ b/sweep/fee_bumper.go
@@ -267,7 +267,7 @@ type TxPublisher struct {
 	cfg *TxPublisherConfig
 
 	// currentHeight is the current block height.
-	currentHeight int32
+	currentHeight atomic.Int32
 
 	// records is a map keyed by the requestCounter and the value is the tx
 	// being monitored.
@@ -373,7 +373,9 @@ func (t *TxPublisher) initializeFeeFunction(
 	}
 
 	// Get the initial conf target.
-	confTarget := calcCurrentConfTarget(t.currentHeight, req.DeadlineHeight)
+	confTarget := calcCurrentConfTarget(
+		t.currentHeight.Load(), req.DeadlineHeight,
+	)
 
 	log.Debugf("Initializing fee function with conf target=%v, budget=%v, "+
 		"maxFeeRateAllowed=%v", confTarget, req.Budget,
@@ -542,7 +544,7 @@ func (t *TxPublisher) broadcast(requestID uint64) (*BumpResult, error) {
 
 	tx := record.tx
 	log.Debugf("Publishing sweep tx %v, num_inputs=%v, height=%v",
-		txid, len(tx.TxIn), t.currentHeight)
+		txid, len(tx.TxIn), t.currentHeight.Load())
 
 	// Set the event, and change it to TxFailed if the wallet fails to
 	// publish it.
@@ -715,7 +717,7 @@ func (t *TxPublisher) monitor(blockEvent *chainntnfs.BlockEpochEvent) {
 				epoch.Height)
 
 			// Update the best known height for the publisher.
-			t.currentHeight = epoch.Height
+			t.currentHeight.Store(epoch.Height)
 
 			// Check all monitored txns to see if any of them needs
 			// to be bumped.
@@ -788,7 +790,7 @@ func (t *TxPublisher) processRecords() {
 	}
 
 	// Get the current height to be used in the following goroutines.
-	currentHeight := t.currentHeight
+	currentHeight := t.currentHeight.Load()
 
 	// For records that are not confirmed, we perform a fee bump if needed.
 	for requestID, r := range feeBumpRecords {
@@ -854,7 +856,7 @@ func (t *TxPublisher) handleFeeBumpTx(requestID uint64, r *monitorRecord,
 		// TODO(yy): send this error back to the sweeper so it can
 		// re-group the inputs?
 		log.Errorf("Failed to increase fee rate for tx %v at "+
-			"height=%v: %v", oldTxid, t.currentHeight, err)
+			"height=%v: %v", oldTxid, t.currentHeight.Load(), err)
 
 		return
 	}
@@ -862,7 +864,7 @@ func (t *TxPublisher) handleFeeBumpTx(requestID uint64, r *monitorRecord,
 	// If the fee rate was not increased, there's no need to bump the fee.
 	if !increased {
 		log.Tracef("Skip bumping tx %v at height=%v", oldTxid,
-			t.currentHeight)
+			t.currentHeight.Load())
 
 		return
 	}
@@ -1122,7 +1124,7 @@ func (t *TxPublisher) createSweepTx(inputs []input.Input, changePkScript []byte,
 
 	// Validate and calculate the fee and change amount.
 	txFee, changeAmtOpt, locktimeOpt, err := prepareSweepTx(
-		inputs, changePkScript, feeRate, t.currentHeight,
+		inputs, changePkScript, feeRate, t.currentHeight.Load(),
 	)
 	if err != nil {
 		return nil, 0, err
@@ -1178,7 +1180,7 @@ func (t *TxPublisher) createSweepTx(inputs []input.Input, changePkScript []byte,
 
 	// We'll default to using the current block height as locktime, if none
 	// of the inputs commits to a different locktime.
-	sweepTx.LockTime = uint32(locktimeOpt.UnwrapOr(t.currentHeight))
+	sweepTx.LockTime = uint32(locktimeOpt.UnwrapOr(t.currentHeight.Load()))
 
 	prevInputFetcher, err := input.MultiPrevOutFetcher(inputs)
 	if err != nil {


Test patch: