Skip to content

fix: retry estimatefee with more blocks when getting negative value #1153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: retry estimatefee with more blocks when getting negative value #1153

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions server/public.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ const txsInAPI = 1000

const secondaryCoinCookieName = "secondary_coin"

const maxEstimateFeeRetries = 3

const (
_ = iota
apiV1
Expand Down Expand Up @@ -1642,6 +1644,25 @@ type resultEstimateFeeAsString struct {
Result string `json:"result"`
}

// 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
}
Comment on lines +1654 to +1660
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Suggested change
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.

}
Comment on lines +1647 to +1664
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func (s *PublicServer) apiEstimateFee(r *http.Request, apiVersion int) (interface{}, error) {
var res resultEstimateFeeAsString
s.metrics.ExplorerViews.With(common.Labels{"action": "api-estimatefee"}).Inc()
Expand All @@ -1660,13 +1681,9 @@ func (s *PublicServer) apiEstimateFee(r *http.Request, apiVersion int) (interfac
return nil, api.NewAPIError("Parameter 'conservative' cannot be converted to boolean", true)
}
}
var fee big.Int
fee, err = s.chain.EstimateSmartFee(blocks, conservative)
fee, err := s.estimateFeeRetryNegative(blocks, conservative, maxEstimateFeeRetries)
if err != nil {
fee, err = s.chain.EstimateFee(blocks)
if err != nil {
return nil, err
}
return nil, api.NewAPIError(fmt.Sprintf("Failed to estimate fee: %v", err), true)
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Suggested change
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.

}
res.Result = s.chainParser.AmountToDecimalString(&fee)
return res, nil
Expand Down