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

Add automatic gas-fee retry for Ethereum transactions in Bee. Transactions that stay unconfirmed are re-broadcast with the same nonce and an escalated priority fee, using dynamic fees from eth_feeHistory. Retry behaviour is configurable, survives node restarts, and is used for redistribution and postage operations.

Diagram in miro

What changed

Transaction retry (pkg/transaction)
New SendWithRetry on the transaction service: sends EIP-1559 transactions with dynamic fee estimation from eth_feeHistory and automatic escalation across priority tiers. Requires automatic gas pricing: a request carrying an explicit Gas-Price is rejected.

Fees are derived from fresh fee history (default window: last 100 blocks) at percentiles 10 / 50 / 90, mapped to tiers low / market / aggressive. maxFeePerGas = 2 × baseFee + tip.

Sending walks a tier range [start_tier … end_tier]. Each tier gets 2 broadcast attempts (internal constant). If no receipt arrives within the retry delay, the tx is rebroadcast — reusing the same nonce — at the same tier (with fresh fee history) or escalated to the next tier once the per-tier budget is spent.

Persists retry state in the state store and resumes in-flight retries after restart, skipping nonces already confirmed on-chain.

Stops on non-retryable errors.

Configuration (cmd/bee, pkg/node)
CLI flags (defaults: start market, ceiling aggressive, 1 min delay, no price cap):

--transaction-fee-priority — starting fee tier (low / market / aggressive), default market
--transaction-fee-priority-max — escalation ceiling tier, default aggressive
--transaction-retry-delay — wait for a receipt before escalating, default 1m
--transaction-fee-max-tx-price-wei — max maxFeePerGas in wei, 0 = no limit
--fee-history-block-count — fee-history window depth, default 100

Constraint: priority <= priority-max, otherwise the node fails to start. Number of attempts per tier and the +15% bump are internal, not exposed as flags.

Call sites
Redistribution (pkg/storageincentives/redistribution): commit / reveal / claim use SendWithRetry via sendAndWait. Config-only — no header overrides.

Postage (pkg/postage/postagecontract): batch create, top-up, dilute, approve use SendWithRetry when retry is not disabled; otherwise fall back to Send + WaitForReceipt.

More detailed description is in doc

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?

gacevicljubisa · 2026-06-08T12:25:38Z

+	if previousTip != nil && previousTip.Sign() > 0 {
+		bumpedTip := applyMempoolBump(previousTip)
+		if tip.Cmp(bumpedTip) < 0 {
+			tip = bumpedTip
+		}
+	}


The same floor should apply to the fee cap as well, not only for the tip?

gacevicljubisa · 2026-06-08T12:27:21Z

+	gasLimit, err := t.estimateGasLimit(ctx, request)
+	if err != nil {
+		return nil, err
+	}


Previous attempt for gas limit as well can be reused

gacevicljubisa · 2026-06-08T12:30:15Z

+	if state.LastTxHash != (common.Hash{}) {
+		if !keepLast {
+			_ = t.store.Delete(pendingTransactionKey(state.LastTxHash))
+		}


if keepLast, should we call t.waitForPendingTx(state.LastTxHash) because the last one may still be mined?

acud · 2026-06-08T21:02:16Z

-	loggerName      = "redistributionContract"
+	loggerName = "redistributionContract"
+	// BoostTipPercent is used where the node sends transactions without retry.
 	BoostTipPercent = 50


this is now just used in one place (agent.go) and i wonder if that callsite also needs to be migrated or if it is covered in a subsequent PR

acud · 2026-06-08T21:04:57Z

+		vals = append(vals, new(big.Int).Set(row[idx]))
+	}
+	if len(vals) == 0 {
+		return big.NewInt(0)


not to slow things down about this, but if there's no entries here, does it mean that all subsequent transactions sent using this quantile's tip value will fail because the value they're using is 0?

acud · 2026-06-08T21:08:56Z

+
+// ParseFeePriority validates a fee priority tier name (for API headers and config).
+func ParseFeePriority(s string) (string, error) {
+	t, err := parseFeeTier(s)


what's this function needed for? why can't you just inline the inner unexported function here?

acud · 2026-06-08T21:16:07Z

+// SendWithRetry sends an EIP-1559 transaction using fee-history tiers with automatic
+// 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) {


nit: i have a feeling this is a little bit misplaced. also, the fact that another metrics collector has been created here (which also partially overlaps with the metrics collector in the wrapped package smells like it should really be there alongside, and you should just reuse the existing metrics collector there.

acud · 2026-06-08T21:21:27Z

+
+	gasFeeCap, gasTipCap, err := t.suggestGasFeeForTier(ctx, tier, previousTip)
+	if err != nil {
+		return nil, err


so if the suggest gas fee failed for whatever reason, then the whole transaction is fail and not even sent? there is a soft-fail there which fails when fh is empty

acud · 2026-06-08T21:26:16Z

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


danger... this will only unlock once the whole method runs to completion.
see example here: https://go.dev/play/p/Cg6j6EpNtKl

acud · 2026-06-08T21:30:16Z

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


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-06-08T21:31:15Z

+				"description", request.Description)
+		}
+
+		monitorLast := errors.Is(terminateTxErr, ErrTxMaxPriceExceeded) || errors.Is(terminateTxErr, ErrAllAttemptsExhausted)


don't understand what is monitorLast... it is just a bool, what does it mean

acud · 2026-06-08T21:34:23Z

+		}
+
+		monitorLast := errors.Is(terminateTxErr, ErrTxMaxPriceExceeded) || errors.Is(terminateTxErr, ErrAllAttemptsExhausted)
+		t.deleteRetryStateAndPending(txRetryKey, txState, monitorLast)


are you sure you're not ending up with stale zombie db records here? for me this bool toggle is a bit tricky since the downstream logic may or may not remove some of the database records and that may lead to statestore growth. i see that there is some unit test coverage for db entry assertion (for deletion) but it's another 1000 lines of machine written code to go through

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: fix log

8c744de

gacevicljubisa reviewed Jun 8, 2026

View reviewed changes

acud requested changes Jun 8, 2026

View reviewed changes

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!

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbackend123 commented May 18, 2026 •

edited

Loading

sbackend123 Jun 7, 2026 •

edited

Loading