feat: transactions with retry#5464
Conversation
# Conflicts: # cmd/bee/cmd/cmd.go
acud
left a comment
acud
left a comment
There was a problem hiding this comment.
lots and lots of code here, i think some complexity can be reduced and readability can be improved
| WhitelistedWithdrawalAddress []string | ||
| } | ||
|
|
||
| func txRetryConfigFromOptions(o *Options) transaction.TransactionsRetryConfig { |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| var rewardPerc []float64 | ||
| if len(feeHistoryRewardPercentiles) >= 3 { |
There was a problem hiding this comment.
i find this a bit confusing. why do we need this? why not just inject the default value and eliminate the choices here?
| 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) |
There was a problem hiding this comment.
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)
| _ = t.store.Delete(pendingTransactionKey(state.LastTxHash)) | ||
| } | ||
| } | ||
| func (t *transactionService) retry(ctx context.Context, txRetryKey string, request *TxRequest) (common.Hash, *types.Receipt, error) { |
There was a problem hiding this comment.
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)
| 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", |
There was a problem hiding this comment.
why do we need the register call? why can't you just use t.logger
| signedTx, err := t.broadcastTx(ctx, request, nonce, txState.PreviousTip, attempt) | ||
| if err != nil { | ||
| if isErrCritical(err) { | ||
| t.logger.Error(err, |
There was a problem hiding this comment.
why are you using t.logger here and not loggerV1, not sure why both usages are needed
| } | ||
|
|
||
| exhaustionErr := fmt.Errorf("transaction failed after %d attempts (nonce=%d, description=%s)", t.txMaxRetries, txState.Nonce, txState.Description) | ||
| t.logger.Error(exhaustionErr, |
There was a problem hiding this comment.
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
| return nil | ||
| } | ||
|
|
||
| func isErrCritical(err error) bool { |
# Conflicts: # pkg/transaction/backendmock/backend.go
2 participants
Checklist
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
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.
CLI flags (defaults: start
market, ceilingaggressive, 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.
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