Fix false success and racy counters in generate_eoa_transactions

pkg/tasks/generate_eoa_transactions/task.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"math/big"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -153,9 +154,9 @@ func (t *Task) Execute(ctx context.Context) error {
 	perBlockCount := 0
 	totalCount := 0
 
-	sucessCount := 0
-	revertCount := 0
-	unknownCount := 0
+	// these counters are incremented from per-transaction receipt callbacks that
+	// run in their own goroutines, so they must be updated atomically.
+	var sucessCount, revertCount, unknownCount atomic.Int64
 
 	t.ctx.ReportProgress(0, "Generating EOA transactions...")
 
@@ -183,18 +184,18 @@ func (t *Task) Execute(ctx context.Context) error {
 				t.logger.Infof("transaction %v confirmed in block %v (nonce: %v, status: %v)", tx.Hash().Hex(), receipt.BlockNumber, tx.Nonce(), receipt.Status)
 
 				if receipt.Status == 0 {
-					revertCount++
+					revertCount.Add(1)
 				} else {
-					sucessCount++
+					sucessCount.Add(1)
 				}
 			case err != nil:
 				t.logger.Errorf("error awaiting transaction receipt: %v", err.Error())
 
-				unknownCount++
+				unknownCount.Add(1)
 			default:
 				t.logger.Warnf("no receipt for transaction: %v (maybe replaced?)", tx.Hash().Hex())
 
-				unknownCount++
+				unknownCount.Add(1)
 			}
 
 			pendingWaitGroup.Done()
@@ -243,18 +244,25 @@ func (t *Task) Execute(ctx context.Context) error {
 		}
 	}
 
-	if t.config.AwaitReceipt {
+	// wait for the receipt callbacks whenever the result depends on their counts,
+	// otherwise the verdict below would read zeroed counters before the receipts
+	// arrive and report success even if every transaction reverted.
+	if awaitReceiptsRequired(t.config) {
 		pendingWaitGroup.Wait()
 	}
 
-	t.logger.Infof("seding complete, total sent: %v, success: %v, reverted: %v, unknown: %v, pending: %v", totalCount, sucessCount, revertCount, unknownCount, totalCount-(sucessCount+revertCount+unknownCount))
+	successTotal := sucessCount.Load()
+	revertTotal := revertCount.Load()
+	unknownTotal := unknownCount.Load()
+
+	t.logger.Infof("seding complete, total sent: %v, success: %v, reverted: %v, unknown: %v, pending: %v", totalCount, successTotal, revertTotal, unknownTotal, int64(totalCount)-(successTotal+revertTotal+unknownTotal))
 
 	switch {
-	case t.config.FailOnSuccess && sucessCount > 0:
-		t.logger.Infof("set task result to failed, %v transactions succeeded unexpectedly (FailOnSuccess)", sucessCount)
+	case t.config.FailOnSuccess && successTotal > 0:
+		t.logger.Infof("set task result to failed, %v transactions succeeded unexpectedly (FailOnSuccess)", successTotal)
 		t.ctx.SetResult(types.TaskResultFailure)
-	case t.config.FailOnReject && revertCount > 0:
-		t.logger.Infof("set task result to failed, %v transactions reverted unexpectedly (FailOnReject)", revertCount)
+	case t.config.FailOnReject && revertTotal > 0:
+		t.logger.Infof("set task result to failed, %v transactions reverted unexpectedly (FailOnReject)", revertTotal)
 		t.ctx.SetResult(types.TaskResultFailure)
 	case totalCount == 0:
 		t.logger.Infof("set task result to failed, no transactions sent")
@@ -264,6 +272,14 @@ func (t *Task) Execute(ctx context.Context) error {
 	return nil
 }
 
+// awaitReceiptsRequired reports whether the task must wait for the per-transaction
+// receipt callbacks before evaluating its result. failOnReject and failOnSuccess
+// derive the result from the receipt counts, so they require the wait even when
+// awaitReceipt is not set on its own.
+func awaitReceiptsRequired(cfg Config) bool {
+	return cfg.AwaitReceipt || cfg.FailOnReject || cfg.FailOnSuccess
+}
+
 // generateTransaction builds and submits a single transaction.
 // The returned bool reports whether the tx was handed off to spamoor: when true,
 // spamoor owns the OnComplete callback (even if an error is also returned); when

pkg/tasks/generate_eoa_transactions/task_test.go

@@ -0,0 +1,60 @@
+package generateeoatransactions
+
+import (
+	"sync"
+	"sync/atomic"
+	"testing"
+)
+
+// TestAwaitReceiptsRequired covers the condition that decides whether the task waits
+// for receipt callbacks before evaluating its result. failOnReject and failOnSuccess
+// must force the wait even when awaitReceipt is off; otherwise the verdict reads the
+// counters before the receipts arrive and reports success even if every transaction
+// reverted.
+func TestAwaitReceiptsRequired(t *testing.T) {
+	tests := []struct {
+		name string
+		cfg  Config
+		want bool
+	}{
+		{"nothing set", Config{}, false},
+		{"awaitReceipt only", Config{AwaitReceipt: true}, true},
+		{"failOnReject forces wait", Config{FailOnReject: true}, true},
+		{"failOnSuccess forces wait", Config{FailOnSuccess: true}, true},
+		{"failOnReject without awaitReceipt still waits", Config{FailOnReject: true, AwaitReceipt: false}, true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := awaitReceiptsRequired(tc.cfg); got != tc.want {
+				t.Fatalf("awaitReceiptsRequired(%+v) = %v, want %v", tc.cfg, got, tc.want)
+			}
+		})
+	}
+}
+
+// TestReceiptCountersAreConcurrencySafe reproduces the receipt callbacks, which run in
+// their own goroutines, incrementing the shared counters. The counters must be atomic
+// so no increments are lost (and so this does not race under -race).
+func TestReceiptCountersAreConcurrencySafe(t *testing.T) {
+	const n = 500
+
+	var revertCount atomic.Int64
+
+	var wg sync.WaitGroup
+
+	for i := 0; i < n; i++ {
+		wg.Add(1)
+
+		go func() {
+			defer wg.Done()
+			revertCount.Add(1)
+		}()
+	}
+
+	wg.Wait()
+
+	if got := revertCount.Load(); got != n {
+		t.Fatalf("lost updates: revertCount = %d, want %d", got, n)
+	}
+}