Skip to content

ethclient/simulated: Fix flaky rollback test #32280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 28, 2025
Merged

ethclient/simulated: Fix flaky rollback test #32280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bdbf7f
ethclient/simulated: Fix flaky rollback test
kashitaka Jul 25, 2025
74caa26
ethclient/simulated: change test to explicitly specify nonces
kashitaka Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 7 additions & 14 deletions ethclient/simulated/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func simTestBackend(testAddr common.Address) *Backend {
)
}

func newBlobTx(sim *Backend, key *ecdsa.PrivateKey) (*types.Transaction, error) {
func newBlobTx(sim *Backend, key *ecdsa.PrivateKey, nonce uint64) (*types.Transaction, error) {
client := sim.Client()

testBlob := &kzg4844.Blob{0x00}
Expand All @@ -67,12 +67,8 @@ func newBlobTx(sim *Backend, key *ecdsa.PrivateKey) (*types.Transaction, error)

addr := crypto.PubkeyToAddress(key.PublicKey)
chainid, _ := client.ChainID(context.Background())
nonce, err := client.PendingNonceAt(context.Background(), addr)
if err != nil {
return nil, err
}

chainidU256, _ := uint256.FromBig(chainid)

tx := types.NewTx(&types.BlobTx{
ChainID: chainidU256,
GasTipCap: gasTipCapU256,
Expand All @@ -88,18 +84,15 @@ func newBlobTx(sim *Backend, key *ecdsa.PrivateKey) (*types.Transaction, error)
return types.SignTx(tx, types.LatestSignerForChainID(chainid), key)
}

func newTx(sim *Backend, key *ecdsa.PrivateKey) (*types.Transaction, error) {
func newTx(sim *Backend, key *ecdsa.PrivateKey, nonce uint64) (*types.Transaction, error) {
client := sim.Client()

// create a signed transaction to send
head, _ := client.HeaderByNumber(context.Background(), nil) // Should be child's, good enough
gasPrice := new(big.Int).Add(head.BaseFee, big.NewInt(params.GWei))
addr := crypto.PubkeyToAddress(key.PublicKey)
chainid, _ := client.ChainID(context.Background())
nonce, err := client.PendingNonceAt(context.Background(), addr)
if err != nil {
return nil, err
}

tx := types.NewTx(&types.DynamicFeeTx{
ChainID: chainid,
Nonce: nonce,
Expand Down Expand Up @@ -161,7 +154,7 @@ func TestSendTransaction(t *testing.T) {
client := sim.Client()
ctx := context.Background()

signedTx, err := newTx(sim, testKey)
signedTx, err := newTx(sim, testKey, 0)
if err != nil {
t.Errorf("could not create transaction: %v", err)
}
Expand Down Expand Up @@ -252,7 +245,7 @@ func TestForkResendTx(t *testing.T) {
parent, _ := client.HeaderByNumber(ctx, nil)

// 2.
tx, err := newTx(sim, testKey)
tx, err := newTx(sim, testKey, 0)
if err != nil {
t.Fatalf("could not create transaction: %v", err)
}
Expand Down Expand Up @@ -297,7 +290,7 @@ func TestCommitReturnValue(t *testing.T) {
}

// Create a block in the original chain (containing a transaction to force different block hashes)
tx, _ := newTx(sim, testKey)
tx, _ := newTx(sim, testKey, 0)
if err := client.SendTransaction(ctx, tx); err != nil {
t.Errorf("sending transaction: %v", err)
}
Expand Down
22 changes: 11 additions & 11 deletions ethclient/simulated/rollback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,19 @@ func TestTransactionRollbackBehavior(t *testing.T) {
defer sim.Close()
client := sim.Client()

btx0 := testSendSignedTx(t, testKey, sim, true)
tx0 := testSendSignedTx(t, testKey2, sim, false)
tx1 := testSendSignedTx(t, testKey2, sim, false)
btx0 := testSendSignedTx(t, testKey, sim, true, 0)
tx0 := testSendSignedTx(t, testKey2, sim, false, 0)
tx1 := testSendSignedTx(t, testKey2, sim, false, 1)

sim.Rollback()

if pendingStateHasTx(client, btx0) || pendingStateHasTx(client, tx0) || pendingStateHasTx(client, tx1) {
t.Fatalf("all transactions were not rolled back")
}

btx2 := testSendSignedTx(t, testKey, sim, true)
tx2 := testSendSignedTx(t, testKey2, sim, false)
tx3 := testSendSignedTx(t, testKey2, sim, false)
btx2 := testSendSignedTx(t, testKey, sim, true, 0)
tx2 := testSendSignedTx(t, testKey2, sim, false, 0)
tx3 := testSendSignedTx(t, testKey2, sim, false, 1)

sim.Commit()

Expand All @@ -61,7 +61,7 @@ func TestTransactionRollbackBehavior(t *testing.T) {

// testSendSignedTx sends a signed transaction to the simulated backend.
// It does not commit the block.
func testSendSignedTx(t *testing.T, key *ecdsa.PrivateKey, sim *Backend, isBlobTx bool) *types.Transaction {
func testSendSignedTx(t *testing.T, key *ecdsa.PrivateKey, sim *Backend, isBlobTx bool, nonce uint64) *types.Transaction {
t.Helper()
client := sim.Client()
ctx := context.Background()
Expand All @@ -71,9 +71,9 @@ func testSendSignedTx(t *testing.T, key *ecdsa.PrivateKey, sim *Backend, isBlobT
signedTx *types.Transaction
)
if isBlobTx {
signedTx, err = newBlobTx(sim, key)
signedTx, err = newBlobTx(sim, key, nonce)
} else {
signedTx, err = newTx(sim, key)
signedTx, err = newTx(sim, key, nonce)
}
if err != nil {
t.Fatalf("failed to create transaction: %v", err)
Expand All @@ -96,13 +96,13 @@ func pendingStateHasTx(client Client, tx *types.Transaction) bool {
)

// Poll for receipt with timeout
deadline := time.Now().Add(2 * time.Second)
Copy link
Member

@rjl493456442 rjl493456442 Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for changing the wait time duration?

Copy link
Member

@rjl493456442 rjl493456442 Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our CI system is occasionally overloaded, have a loose duration is usually preferred

Copy link
Contributor Author

@kashitaka kashitaka Jul 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjl493456442 Thanks for the comment! As mentioned in the PR description, It reduces test time for TestTransactionRollbackBehavior from about 7 seconds to 2 seconds.

I shortened the wait time because I ran the test thousands times to reproduce the flakiness, and here was the bottle neck. The test remained stable with the shorter duration, so I kept the change for some improvement.

Copy link
Member

@rjl493456442 rjl493456442 Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. This loop is for checking the availability of the receipt. Once it's found, the loop will be terminated.

If the CI can be finished within 2s, then setting it with 7s has no side effect.

Copy link
Contributor Author

@kashitaka kashitaka Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure if this behavior is intentional or unexpected. In this line:

 	if !pendingStateHasTx(client, btx2) || !pendingStateHasTx(client, tx2) || !pendingStateHasTx(client, tx3) {

the polling loop exits almost immediately, since the receipt is available and TransactionReceipt() succeeds — just as you explained. However, in this part:

	if pendingStateHasTx(client, btx0) || pendingStateHasTx(client, tx0) || pendingStateHasTx(client, tx1) {

each call to pendingStateHasTx takes the full 2 seconds and takes 6 seconds in total, because:

receipt, err = client.TransactionReceipt(ctx, tx.Hash())

returns an error like "transaction indexing is in progress" and doesn't drop into break in the 2 sec polling, which seems to happen when querying uncommitted transactions. And that’s why I shortened the polling timeout.

Copy link
Contributor Author

@kashitaka kashitaka Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjl493456442 If you prefer a more relaxed polling interval, please let me know—I’m happy to revert that change.

deadline := time.Now().Add(200 * time.Millisecond)
for time.Now().Before(deadline) {
receipt, err = client.TransactionReceipt(ctx, tx.Hash())
if err == nil && receipt != nil {
break
}
time.Sleep(100 * time.Millisecond)
time.Sleep(5 * time.Millisecond)
}

if err != nil {
Expand Down
Loading