From c263ac3b0ffe51a4afed9e0ce7f907b1d7e5c899 Mon Sep 17 00:00:00 2001 From: Damilola Edwards Date: Sat, 11 Jul 2026 10:10:54 +0100 Subject: [PATCH] Fix false success and racy counters in generate_eoa_transactions Two problems in the result accounting. The success, revert and unknown counters were plain ints incremented from per transaction receipt callbacks that run in their own goroutines, so concurrent increments raced and lost updates. And the task only waited for those callbacks when awaitReceipt was set, but failOnReject and failOnSuccess derive the result from the same counts; with awaitReceipt off (the default) the verdict read zeroed counters before the receipts arrived and reported success even when every transaction reverted. Make the counters atomic, and wait for the receipt callbacks whenever the result depends on their counts (awaitReceipt, failOnReject or failOnSuccess). Extract that condition into a helper so it can be tested. --- pkg/tasks/generate_eoa_transactions/task.go | 42 +++++++++---- .../generate_eoa_transactions/task_test.go | 60 +++++++++++++++++++ 2 files changed, 89 insertions(+), 13 deletions(-) create mode 100644 pkg/tasks/generate_eoa_transactions/task_test.go diff --git a/pkg/tasks/generate_eoa_transactions/task.go b/pkg/tasks/generate_eoa_transactions/task.go index 6201cbee..554ac156 100644 --- a/pkg/tasks/generate_eoa_transactions/task.go +++ b/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 diff --git a/pkg/tasks/generate_eoa_transactions/task_test.go b/pkg/tasks/generate_eoa_transactions/task_test.go new file mode 100644 index 00000000..a24b09e8 --- /dev/null +++ b/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) + } +}