[gzliudan]

Summary

testSyncTransaction (behind TestTransactionAnnouncement and TestTransactionPropagation) gave the fetcher one second for the entire batch: all 1024 transactions had to be announced or broadcast, fetched and pooled before time.After(time.Second) fired.

 testSyncTransaction
-  timeout := time.After(time.Second)             # one budget for all 1024 txs
+  idle := time.NewTimer(txSyncIdleTimeout)       # re-armed on every batch
+  total := time.NewTimer(txSyncTotalTimeout)     # but bounded end to end
   for {
     select {
     case ev := <-newTxs:
       got += len(ev.Txs)
-      if got == 1024 { break }
+      if got == len(alltxs) { return }
+      if got > len(alltxs) { t.Fatalf(...duplicate...) }
+      idle.Reset(txSyncIdleTimeout)
-    case <-timeout:
-      t.Fatal("Failed to retrieve all transaction")
+    case <-idle.C:
+      t.Fatalf("Failed to retrieve all transactions: got %d, want %d", got, len(alltxs))
+    case <-total.C:
+      t.Fatalf("Failed to retrieve all transactions: got %d, want %d after %v", got, len(alltxs), txSyncTotalTimeout)

Upstream ethereum#20234 built the timer inside the select, so it re-armed whenever a batch arrived; hoisting it out during the eth/65 fetch port (#2490) turned a liveness check into a latency assertion. The wait now bounds "no progress", not "not done yet", a second timer caps the whole thing at two minutes, and a count that overshoots the batch size fails immediately with a duplicate diagnosis instead of timing out. No production code changes.

Evidence

• Before: race (bulk 4/4) on feat(params,core,cmd/puppeth): enforce 0 < gap < epoch in CheckConfigForkOrder #2901 failed with --- FAIL: TestTransactionAnnouncement (20.70s) / protocol_test.go:371: Failed to retrieve all transaction, while eth/ was byte-identical to the previous commit, which passed the same job: ok .../eth 24.504s there against FAIL .../eth 90.405s here (eth/protocols/eth: 10.8s → 85.9s). A 4x slower worker was enough.
  After: the wait only expires once the peer stops delivering anything.
• 16 local runs of the pair under -race -short all pass: serial, GOMAXPROCS=2, taskset -c 0,1, full-core busy loops, and three concurrent test processes.
• Reverse checks — forcing txSyncIdleTimeout to time.Nanosecond fails fast with the count; forcing txSyncTotalTimeout to 100ms hits the total branch; a duplicate-delivery overlay (an extra Send(txs[:1]) in the fake pool) now fails with got 1027, want 1024 (duplicate or unexpected transactions) instead of passing.
• Post-change: go test -run '^TestTransaction(Announcement|Propagation)$' -count=3 ./eth/ ok 22.8s, go vet ./... clean, golangci-lint 0 issues, make test 7m19s all green.
• Since test: cut the -short carve-out at 4s #2947 the announcement variant is gated behind -short, so the race job no longer runs it; the non-short tests job still does.

Merge Danger

Door: two-way — test-only, no production path, cheap to revert.

Blast Radius: one package's test timing; a genuinely stuck fetcher still fails, now after 10s of silence instead of 1s in total; a fetcher that keeps trickling just under that limit is capped at two minutes, so the failure stays a clean count instead of a package timeout.

Ref: ethereum#20234

---

Proposed changes

Fix the eth tx-sync test wait so it bounds stalled progress rather than total latency, and make the failure diagnosable.

Types of changes

• build: Changes that affect the build system or external dependencies
• ci: Changes to CI configuration files and scripts
• chore: Changes that don't change source code or tests
• docs: Documentation only changes
• feat: A new feature
• fix: A bug fix
• perf: A code change that improves performance
• refactor: A code change that neither fixes a bug nor adds a feature
• revert: Revert something
• style: Changes that do not affect the meaning of the code
• test: Adding missing tests or correcting existing tests

Impacted Components

• Consensus
• Account
• Network
• Geth
• Smart Contract
• External components
• Not sure (Please specify below) — eth protocol tests only; no production package changed.

Checklist

• This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
• Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
• Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
• Tested the backwards compatibility.
• Tested with XDC nodes running this version co-exist with those running the previous version.
• Relevant documentation has been updated as part of this PR
• N/A

The unchecked items are not applicable: this PR only changes a unit test's wait, with no production path, node behaviour, wire format, or documentation change.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: fde2540d-25e6-4b33-8884-7b8f376912a1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn