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) + } +}