feat: transactions with retry by sbackend123 · Pull Request #5464 · ethersphere/bee

sbackend123 · 2026-05-18T22:33:25Z

Checklist

I have read the coding guide.
My change requires a documentation update, and I have done it.
I have added tests to cover my changes.
I have filled out the description and linked the related issues.

Description

This PR adds an automatic retry for transactions, with fee escalation.

If a transaction does not get into a block, it is sent again with the same nonce and a higher fee. The fee comes from eth_feeHistory. The session is stored on disk and continues after a restart. If the request sets GasPrice, the call returns an error immediately: the node chooses the price.

Over the last 100 blocks (the depth is configurable) eth_feeHistory takes the priority fee at the 10th, 50th and 90th percentiles. These are the low, market and aggressive tiers. At first maxFeePerGas = 2 * baseFee + tip. On a replacement the tip and the fee cap are at least 15% above the previous ones, otherwise the mempool will not accept the replacement.

The node goes from the starting tier up to the maximum. Each tier gets 2 attempts. Between them it waits for the nonce to show up in a block. If that does not happen within time window, it goes on next iteration. When the tier runs out of attempts, it moves to the next tier.

Once the nonce is in a block, escalation stops and the node looks for a receipt. If there is none, the session is cancelled only after cancellationDepth blocks. Insufficient balance, a revert during estimation and other fatal errors do not continue the retry.

If the nonce is already confirmed by the time the node restarts, nothing is sent to the network again: it waits for the receipt and clears the session. If the attempts run out while the transaction may still be in the mempool, it keeps waiting in the background.

Flags:

--transaction-fee-priority sets the starting tier. Default is market.
--transaction-fee-priority-max sets the ceiling. Default is aggressive.
--transaction-retry-delay sets how long to wait for the nonce to get into a block. Default is 1 minute. This is not a wait for a receipt.
--transaction-fee-max-tx-price-wei sets the maxFeePerGas ceiling. 0 means no limit.
--fee-history-block-count sets the window depth. Default is 100.
If the starting tier is above the ceiling, the node does not start. The number of attempts per tier and the 15% bump are not exposed as flags.

commit, reveal and claim in redistribution always go through transactions with retry mechanism.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

#5114

Screenshots (if appropriate):

AI Disclosure

This PR contains code that has been generated by an LLM.
I have reviewed the AI generated code thoroughly.
I possess the technical expertise to responsibly review the code generated in this PR.

# Conflicts: # cmd/bee/cmd/cmd.go

acud

lots and lots of code here, i think some complexity can be reduced and readability can be improved

acud · 2026-05-25T16:50:45Z

+	WhitelistedWithdrawalAddress       []string
+}
+
+func txRetryConfigFromOptions(o *Options) transaction.TransactionsRetryConfig {


since we already have the same logic in the cmd package (which precedes this package execution on runtime), wouldn't it make sense to build the config just once and pass it to this package as the right type?

acud · 2026-05-25T19:51:13Z

+	}
+
+	var rewardPerc []float64
+	if len(feeHistoryRewardPercentiles) >= 3 {


i find this a bit confusing. why do we need this? why not just inject the default value and eliminate the choices here?

acud · 2026-05-25T20:01:14Z

+	return new(big.Int).Div(new(big.Int).Mul(new(big.Int).Set(tip), big.NewInt(int64(100+increasePct))), big.NewInt(100))
+}
+
+// suggestGasFeeGasTipCapWithHistory returns maxFeePerGas (gasFeeCap) and maxPriorityFeePerGas (gasTipCap)


slight headache from this method: long variable names and this comment that tries to explain it all makes it really difficult to read for the amount of lines that it has. i'd rather convert this comment to inline comments explaining the branching, and reduce the variable/method names in the PR in general (adding more words to the method name doesn't necessarily add value when reading it)

acud · 2026-05-25T20:07:18Z

+		_ = t.store.Delete(pendingTransactionKey(state.LastTxHash))
+	}
+}
+func (t *transactionService) retry(ctx context.Context, txRetryKey string, request *TxRequest) (common.Hash, *types.Receipt, error) {


this is really hard to review. i'm not sure about this. also, i'm not sure i fully understand why statestore persistence is needed. do we really need to persist all of the data every time we submit a transaction? apart from the nonce and the transaction data, there's no guarantee that the extra data persisted around a transaction will be relevant once a node restarts (it can just be again inferred via RPC calls and configuration)

We discussed this briefly on the call, but leaving it here for the record:

I think persistence is needed because:

We choose the next transaction nonce based in part on the transactions we are currently tracking. In this case, we cannot avoid persisting the transaction altogether. And we cannot persist it only once — with each rebroadcast the hash changes because the fees change, and from a monitoring perspective we would end up with several “stuck” transactions sharing the same nonce.

With the default 1 min delay, in the worst case — even with just two adjacent tiers (low → market, market → aggressive) and a price spike — retries can take 3–4 minutes. If the node is shut down during that window, the transaction may already be in the mempool and even mined, but user would have no way to know. As a result, user would send it again and lose fees, and that is exactly the scenario we are trying to avoid in the first place.

the mining while node is shut down is a separate use case that needs to be handled separately by the startup logic. what i meant is that if the node is offline, the fee pricing fee goes "out-of-fashion" just because the fee snapshot may be completely different with different values once you start the node next time.

acud · 2026-05-25T20:13:16Z

+	gasFeeCapWithEscalatedTip := new(big.Int).Add(new(big.Int).Set(gasFeeCap), escalatedGasTip)
+	gasFeeCapWithPreviousTip := new(big.Int).Add(new(big.Int).Set(gasFeeCap), prevGasTipCap)
+
+	t.logger.V(1).Register().Debug("suggest gas fees for retry",


why do we need the register call? why can't you just use t.logger

acud · 2026-05-25T20:14:00Z

+		signedTx, err := t.broadcastTx(ctx, request, nonce, txState.PreviousTip, attempt)
+		if err != nil {
+			if isErrCritical(err) {
+				t.logger.Error(err,


why are you using t.logger here and not loggerV1, not sure why both usages are needed

acud · 2026-05-25T20:16:27Z

+	}
+
+	exhaustionErr := fmt.Errorf("transaction failed after %d attempts (nonce=%d, description=%s)", t.txMaxRetries, txState.Nonce, txState.Description)
+	t.logger.Error(exhaustionErr,


quite a few cases here of log and return error which is against the style guide. in general a bit too much logging i'd say, not sure if this is needed for merging into trunk but ok for now if it is needed for debugging purposes

acud · 2026-05-25T20:19:12Z

+	return nil
+}
+
+func isErrCritical(err error) bool {


notRetriable?

# Conflicts: # pkg/transaction/backendmock/backend.go

martinconic · 2026-06-05T08:37:35Z

+// escalation. Each tier gets attemptsPerTier broadcast rounds with fresh eth_feeHistory
+// data. A +15% mempool bump floor is applied to ensure replacement transactions are accepted.
+func (t *transactionService) SendWithRetry(ctx context.Context, request *TxRequest) (txHash common.Hash, receipt *types.Receipt, err error) {
+	if request.GasPrice != nil {


Gas-Price header is broken for postage stamp ops. SendWithRetry rejects any request with an explicit GasPrice (send_tx_with_retry.go:51-56), but the postage call sites (postagecontract/contract.go:168 and :212) still copy GasPrice from the header and route to SendWithRetry by default. So POST /stamps/... with Gas-Price set and no Disable-Retry always fails and since the error isn't a matched sentinel, it surfaces as an opaque HTTP 500 instead of a 400. This is a regression (the old Send path honored Gas-Price) and it affects bee-js.

Thanks for catching that.

I see three possible approaches:

Make the error explicit and add Disable-Retry usage in bee-js when Gas-Price is set. This is a breaking change, but on the other hand we are introducing new functionality.

If Gas-Price is set, automatically route to Send without retry.

Keep the old behavior as the default, and instead of a Disable-Retry flag, introduce the opposite — e.g. a With-Retry opt-in flag.

Given existing clients, option 3 seems the most logical to me. I don’t like option 2 because it’s implicit behavior, and we may end up having to revisit it in a later release anyway — which would likely bring us back to option 1.

@gacevicljubisa WDYT?

Gas-Price is a spend ceiling, retry contradicts it, and it should act as a client override. I would go with option 2. I would even consider droping the Disable-Retry, if you set Gas-Price you've already opted out of retry. If Gas-Price is not used, pricing is given to the node and retry is just part of that. But we should document this in Swarm.yaml that Gas-Price disables escalation.

martinconic

This PR adds new API headers like Disable-Retry and Fee-Priority, but they're not documented in the OpenAPI spec. Please add all necessary parameters in the corresponding yaml files

martinconic · 2026-06-05T10:10:24Z

+				"tx_hash", txState.LastTxHash,
+				"nonce", txState.Nonce,
+				"to", request.To.String(),
+				"value", request.Value.String(),


Both To and Value and will cause a panic if nil, right?

acud

looking way better, i still have a few comments but i think we're almost there. thanks for you patience

acud · 2026-06-12T22:58:58Z

-				GasLimit uint64   `map:"Gas-Limit"`
+				GasPrice    *big.Int `map:"Gas-Price"`
+				GasLimit    uint64   `map:"Gas-Limit"`
+				FeePriority string   `map:"Fee-Priority"`


this is not in the openapi spec

acud · 2026-06-12T23:06:03Z

+		if err != nil {
+			return nil, fmt.Errorf("fee priority: %w", err)
+		}
+		if parsed > t.endTier {


if there's an override then you're clamping it? not sure this makes much sense no? if there's an override from the API then i'd kinda expect it to win here

We do not allow an override above the maximum threshold configured on the node, so node operator would keep control over tiers.

acud · 2026-06-12T23:07:12Z

+		nonce = *fixedNonce
+	} else {
+		t.lock.Lock()
+		defer t.lock.Unlock()


then at least put a very visible comment that this is intentional...

acud · 2026-06-12T23:08:37Z

+
+		n, err := t.nextNonce(ctx)
+		if err != nil {
+			return nil, err


error instrumentation is missing across this file and probably worth to go through the entire changeset for this - please instrument the errors whenever a function can return more than one error, otherwise we cannot know where it came from once it lands in the log

acud · 2026-06-12T23:10:48Z

+		return nil, fmt.Errorf("%w: %w", ErrSignTransaction, err)
+	}
+
+	t.logger.Info("send with retry: broadcast",


as a general comment: this is fine while the branch is in development but i'd be leaning towards having more concise logging. no need to log so much data. the Send method in this package is a good example for this

acud · 2026-06-12T23:20:27Z

+}
+
+// attemptResult is the outcome of a single broadcast+wait cycle.
+type attemptResult struct {


not sure why is this type needed. usually i'd say this is needed when you want to pass multiple values in one go, normally as a result of an operation over a channel. here there are no channels, and go supports multiple return values, which i think would be nicer to use here

acud · 2026-06-12T23:21:09Z

+	}
+
+	terminateTxErr = ErrAllAttemptsExhausted
+	return rs.lastTxHash, nil, terminateTxErr


why care about the last tx hash when there's an error?

EDIT: because of the defer statement. usually if you have an error there's no need to return the other values, but here it does because of the defer. i would lean away from the defer in this case

acud · 2026-06-12T23:23:23Z

+		nonce          *uint64
+	)
+	attempt := 1
+	defer func() { t.finishRetry(rs, request, attempt, terminateTxErr) }()


this is not so idiomatic, and mostly just making everything here way more complicated. i would go with explicitly calling this method wherever needed and then explicitly returning, instead of returning and implicitly calling this always. it may save on a few lines but it makes the code less linear to read imo

# Conflicts: # cmd/bee/cmd/cmd.go # pkg/transaction/wrapped/metrics.go # pkg/transaction/wrapped/wrapped_test.go

akrem-chabchoub · 2026-09-30T08:22:56Z

+			return receipt
+		}
+	}
+	return nil


Could an RPC error here turn a mined tx into cancelled?

For now the only check is err == nil && receipt != nil, so a 429 or timeout on the mined hash seems to be handled the same as NotFound.

Then we'd return ErrTransactionCancelled and finishRetry would delete the records, while the tx could actually went through ?

# Conflicts: # cmd/bee/cmd/cmd.go # pkg/transaction/transaction.go

martinconic · 2026-10-21T07:59:00Z

 	}

-	pending := t.filterPendingTransactions(t.ctx, pendingTxs)
+	pending := t.filterPendingTransactions(ctx, pendingTxs)


nextNonce can hand out a nonce that an active retry session currently owns.

In filterPendingTransactions, if TransactionByHash returns ethereum.NotFound (for instance, if the previous attempt dropped from the mempool or a load-balanced RPC node hasn't seen it yet), it deletes pendingTransactionKey(txHash) from disk and omits it from pending. When a legacy Send (like a chequebook cashout or stake transaction) calls nextNonce, maxNonce is computed without the retry session's nonce and allocates the same nonce $N$.

When the retry session escalates to the next attempt, it prepares a replacement with nonce $N$ and a +15% fee, silently replacing and canceling the other transaction.

Made a fix, but not really happy with it, so opened follow up issue

martinconic · 2026-10-21T08:00:19Z

+func (t *transactionService) attempt(ctx context.Context, rs *RetriedTransaction, request *TxRequest, previousTip, previousFeeCap *big.Int, tier feeTier, nonceWatch *retryNonceWatch) (*types.Transaction, *types.Receipt, error) {
+	signedTx, broadCastErr := t.sendAndRecord(ctx, rs, request, tier, previousTip, previousFeeCap)
+	switch {
+	case isNonceTooLow(broadCastErr):


If the very first broadcast fails with nonce too low (e.g. stale nonce served by a lagging RPC), nothing was ever sent to the network and persistReplaceTx was not called. This causes two critical bugs:

rs.NonceAssigned is still false and rs.allHashes() is empty. attempt calls pollReceiptAfterNonceMined, which stalls for cancellationDepth (12 blocks / ~1 minute) and then returns ErrTransactionCancelled for a transaction that was never broadcast.
finishRetry passes rs to clearRetrySession, which executes _ = t.store.Delete(retryStateKey(rs.Nonce)). Because rs.Nonce was never initialized, it defaults to 0, deleting transaction_retry_00000000000000000000 from disk and wiping out an unrelated retry session that legitimately owned nonce 0.

martinconic · 2026-10-21T08:00:57Z

+	if err != nil {
+		return nil, err
+	}
+	low, market, aggressive, err := suggestedFeesFromFeeHistoryResult(fh)


In the legacy fee path (

fee.go:66-70
), gasTipCap is clamped against b.minimumGasTipCap.

Here, low, market, and aggressive tips are returned straight from fee history without checking b.minimumGasTipCap. This is a regression: operators configuring --minimum-gas-tip-cap will have their minimum tip setting silently ignored for all postage stamp operations and redistribution transactions.

Please clamp each tier tip against b.minimumGasTipCap.

sbackend123 added 15 commits May 12, 2026 22:22

feat: fee history

d6f4499

feat: draft retry logic with

b03a321

feat: clean up retry logic + fixes

97db71f

feat: clean up2 + debugging

4e3d724

feat: tests, improvement, debugging

006bb44

feat: logging, more tests, clean up

48b637e

fix: metrics

9dad7f5

fix: debugging + tests

b015979

Merge branch 'master' into feat/new-gas-estimation

30aa5c7

# Conflicts: # cmd/bee/cmd/cmd.go

fix: cleanup, fix namings

f681124

fix: fix assigning

32551cb

fix: fix old tests

dda24dd

fix: fix mock

279359b

fix: fix contract tests

e57022e

fix: fix data race

5517301

acud reviewed May 25, 2026

View reviewed changes

sbackend123 added 5 commits May 28, 2026 19:06

fix: refactoring

185b86c

Merge branch 'master' into feat/new-gas-estimation

aa5b45d

# Conflicts: # pkg/transaction/backendmock/backend.go

fix: move corner cases logic from branch

e94aefc

fix: make linter happy

19cd2db

fix: make linter happy + renaming + test

80f7edb

sbackend123 self-assigned this Jun 3, 2026

sbackend123 marked this pull request as ready for review June 3, 2026 23:07

sbackend123 requested review from acud, gacevicljubisa, janos and martinconic June 3, 2026 23:07

martinconic reviewed Jun 5, 2026

View reviewed changes

martinconic requested changes Jun 5, 2026

View reviewed changes

martinconic reviewed Jun 5, 2026

View reviewed changes

fix: simplify resuming transactions

42f6a97

acud requested changes Jun 12, 2026

View reviewed changes

sbackend123 added 3 commits September 15, 2026 13:37

Merge branch 'master' into feat/new-gas-estimation

094e979

# Conflicts: # cmd/bee/cmd/cmd.go # pkg/transaction/wrapped/metrics.go # pkg/transaction/wrapped/wrapped_test.go

fix: keep multiple tx hashes

ef1f0b5

feat: experiment with WatchTransaction

af2c942

sbackend123 marked this pull request as draft September 21, 2026 14:41

sbackend123 added 4 commits September 22, 2026 15:20

feat: experiment with WatchTransaction 2

492c438

fix: testing + debugging + cleanup

ae2f922

fix: fix tests

29c58ff

fix: address review comments

7b35fe2

sbackend123 requested review from acud, gacevicljubisa and martinconic September 29, 2026 20:52

sbackend123 marked this pull request as ready for review September 29, 2026 20:53

fix: make linter happy

0c8531f

akrem-chabchoub reviewed Sep 30, 2026

View reviewed changes

sbackend123 added 4 commits October 13, 2026 23:07

fix: clean up + refactor + documenting

dd52f4f

fix: test audit

5d23620

fix: monitoring with cancellation depth

9f5b4c5

fix: nonce race

f8d0097

ethersphere deleted a comment from sbackend Oct 19, 2026

sbackend123 added 4 commits October 19, 2026 13:23

Merge branch 'master' into feat/new-gas-estimation

16f8fdc

# Conflicts: # cmd/bee/cmd/cmd.go # pkg/transaction/transaction.go

fix: test

b62b49f

fix: docs + state error corner case

4ee2f98

fix: comment

5652af2

martinconic reviewed Oct 21, 2026

View reviewed changes

fix: address comments

3ded69c

sbackend123 mentioned this pull request Oct 25, 2026

A dropped transaction can leave a nonce gap that blocks every later transaction #5678

Open

sbackend123 requested review from akrem-chabchoub and martinconic October 25, 2026 22:41

Conversation

sbackend123 commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

AI Disclosure

Uh oh!

acud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbackend123 Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinconic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

sbackend123 commented May 18, 2026 •

edited

Loading

sbackend123 Jun 7, 2026 •

edited

Loading