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 { |
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
Transaction retry (pkg/transaction)
New SendWithRetry on the transaction service: EIP-1559 txs with fee estimation via eth_feeHistory (initial tip = market / 50th percentile), then +20% tip per retry step (configurable). Replacement transactions reuse the same nonce; maxFeePerGas is derived as 2 × baseFee + tip. Persists RetryState in the state store and resumes in-flight retries after restart. Stops on non-retryable errors (revert, insufficient funds, sign failure, redistribution contract errors, nonce too low, etc.).
Structured logging: Info on each broadcast, Debug on fee suggestion / state updates, Error on critical failures and when all attempts are exhausted.
Configuration (cmd/bee, pkg/node)
CLI flags (defaults: 5 attempts, 1 min delay, 20% increase):
--transaction-retry-max-retries
--transaction-retry-delay
--transaction-retry-gas-increase-percent
--transaction-retry-max-tx-price-wei
Call sites
API
New HTTP header Disable-Retry (parsed in gasConfigMiddleware together with Gas-Price / Gas-Limit).
When Disable-Retry: true, postage falls back to the existing Send + WaitForReceipt path.
Header is included in CORS allow-list.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5114
Screenshots (if appropriate):
AI Disclosure