fix: retry estimatefee with more blocks when getting negative value #1153
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds retry logic for fee estimation when a negative fee is returned, particularly addressing Dogecoin behavior. Introduces a helper with incremental block adjustments and updates the API handler to use it with a fixed retry limit.
- New retry helper estimateFeeRetryNegative with block increment logic
- Added max retry constant and replaced inline logic in apiEstimateFee
- Modified error wrapping when retries fail
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // estimateFeeRetryNegative tries to estimate the fee X times if the fee is negative. | ||
| // It gradually increases the number of blocks to estimate the fee for. | ||
| func (s *PublicServer) estimateFeeRetryNegative(blocks int, conservative bool, maxRetries int) (big.Int, error) { | ||
| var fee big.Int | ||
| var err error | ||
| for i := 0; i < maxRetries; i++ { | ||
| fee, err = s.chain.EstimateSmartFee(blocks, conservative) | ||
| if err == nil && fee.Sign() != -1 { | ||
| return fee, nil | ||
| } | ||
| fee, err = s.chain.EstimateFee(blocks) | ||
| if err == nil && fee.Sign() != -1 { | ||
| return fee, nil | ||
| } | ||
| blocks++ | ||
| } | ||
| return big.Int{}, fmt.Errorf("failed to estimate fee after %d retries", maxRetries) | ||
| } |
There was a problem hiding this comment.
The comment states it retries when the fee is negative, but the implementation also retries on RPC errors; additionally, 'X times' is ambiguous relative to two RPC calls per loop (EstimateSmartFee + EstimateFee). Update the comment to explicitly describe both retry conditions (negative result or error), note that up to 2 * maxRetries RPC calls are made, and clarify block increment behavior.
Copilot uses AI. Check for mistakes.
| if err == nil && fee.Sign() != -1 { | ||
| return fee, nil | ||
| } | ||
| fee, err = s.chain.EstimateFee(blocks) | ||
| if err == nil && fee.Sign() != -1 { | ||
| return fee, nil | ||
| } |
There was a problem hiding this comment.
Each retry loop performs two RPC calls, doubling network load relative to the retry count. Consider either: (a) making EstimateSmartFee the first attempt and only falling back to EstimateFee if it errors (not when it returns a negative fee), or (b) counting each RPC invocation toward the retry budget to align cost with configured maxRetries.
| if err == nil && fee.Sign() != -1 { | |
| return fee, nil | |
| } | |
| fee, err = s.chain.EstimateFee(blocks) | |
| if err == nil && fee.Sign() != -1 { | |
| return fee, nil | |
| } | |
| if err == nil { | |
| if fee.Sign() != -1 { | |
| return fee, nil | |
| } | |
| // If EstimateSmartFee returns a negative fee, do not fallback, just retry with increased blocks | |
| } else { | |
| // Only fallback to EstimateFee if EstimateSmartFee errored | |
| fee, err = s.chain.EstimateFee(blocks) | |
| if err == nil && fee.Sign() != -1 { | |
| return fee, nil | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
| } | ||
| blocks++ | ||
| } | ||
| return big.Int{}, fmt.Errorf("failed to estimate fee after %d retries", maxRetries) |
There was a problem hiding this comment.
The final error loses the underlying cause(s) (e.g., last RPC error or persistent negative fee) making debugging harder. Include the last non-nil error and optionally the last negative fee value, e.g.: return big.Int{}, fmt.Errorf("failed to estimate fee after %d retries: last error=%v last fee=%s", maxRetries, err, fee.String())
| return big.Int{}, fmt.Errorf("failed to estimate fee after %d retries", maxRetries) | |
| return big.Int{}, fmt.Errorf("failed to estimate fee after %d retries: last error=%v last fee=%s", maxRetries, err, fee.String()) |
Copilot uses AI. Check for mistakes.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return nil, api.NewAPIError(fmt.Sprintf("Failed to estimate fee: %v", err), true) |
There was a problem hiding this comment.
The wrapped API error now surfaces only the generic retry failure message from estimateFeeRetryNegative (which currently omits root causes). After improving the underlying function's error detail, consider removing the generic 'Failed to estimate fee:' prefix or wrapping with additional context using %w to preserve stack (e.g., fmt.Errorf("estimating fee: %w", err)).
| return nil, api.NewAPIError(fmt.Sprintf("Failed to estimate fee: %v", err), true) | |
| return nil, api.NewAPIError(fmt.Errorf("estimating fee: %w", err).Error(), true) |
Copilot uses AI. Check for mistakes.
1 participant
Fix for dogecoin, when we receive a negative value from estimatefee, we increase the block query by one and try again ... with specified max retries (3 now)