Repository navigation
fix: covering balance cache - #5610
Conversation
|
|
||
| if amount.Cmp(big.NewInt(0).Sub(availableBalance, s.totalIssuedReserved)) > 0 { | ||
| s.metrics.OutOfFunds.Inc() | ||
| return nil, ErrOutOfFunds |
There was a problem hiding this comment.
"On apparent shortfall, force one refresh before returning ErrOutOfFunds." Without it, a node that runs dry keeps refusing cheques for up to 5 min after funds arrive. That covers both our own deposits (see the comment on L126) and external top-ups. This line is unchanged, but availableBalance is now cached, so it can refuse based on a balance up to 5 min old.
| return common.Hash{}, err | ||
| } | ||
|
|
||
| s.invalidateCoveringBalance() |
There was a problem hiding this comment.
This runs right after the transfer is broadcast, before it's mined. POST /chequebook/deposit never calls WaitForDeposit. So the next Issue (usually within milliseconds on a busy node) reloads the pre-deposit balance and caches it for 5 minutes.
Reproduced with a simulated chain:
Empty chequebook.
Deposit 100 via Deposit.
The tx gets mined.
Issue(10) still returns ErrOutOfFunds.
At the same time, GET /chequebook/balance shows totalBalance: 100, availableBalance: 0. On master, the same Issue succeeds.
| defer s.lock.Unlock() | ||
|
|
||
| if s.coveringBalance != nil { | ||
| s.coveringBalance.Sub(s.coveringBalance, amount) |
There was a problem hiding this comment.
The deduction only lives in the cached value, so any reload throws it away while the withdrawal is still pending. A reload happens on deposit invalidation, on TTL expiry, or on the shortfall refresh from L219.
Repro:
Balance 100, issue 10.
Withdraw(90) (pending).
Deposit(1) (pending).
Issue(50) succeeds.
After both txs are mined: 60 issued against 11 covering.
| } | ||
|
|
||
| if amount.Cmp(big.NewInt(0).Sub(availableBalance, s.totalIssuedReserved)) > 0 { | ||
| s.metrics.OutOfFunds.Inc() |
There was a problem hiding this comment.
Before refusing here, could we reload the covering balance once? Something like:
go
if amount.Cmp(new(big.Int).Sub(availableBalance, s.totalIssuedReserved)) > 0 {
s.coveringBalance = nil // force a reload: an external top-up may not be cached yet
if availableBalance, err = s.availableBalance(ctx); err != nil {
return nil, err
}
if amount.Cmp(new(big.Int).Sub(availableBalance, s.totalIssuedReserved)) > 0 {
s.metrics.OutOfFunds.Inc()
return nil, ErrOutOfFunds
}
}
That covers top-ups made outside the node, which invalidateCoveringBalance never sees.
| } | ||
|
|
||
| func (s *service) getOrLoadCoveringBalance(ctx context.Context) (*big.Int, error) { | ||
| now := time.Now().UTC() |
There was a problem hiding this comment.
nit: .UTC() isn't needed. now.Sub(s.coveringBalanceAt) already uses the monotonic clock reading in time.Time, and .UTC() only changes the time zone used for display. Plain time.Now() reads more clearly. Same at line 403.
| func (s *service) getOrLoadCoveringBalance(ctx context.Context) (*big.Int, error) { | ||
| now := time.Now().UTC() | ||
| if s.coveringBalance != nil && now.Sub(s.coveringBalanceAt) < coveringBalanceCacheValidity { | ||
| s.metrics.CoveringBalanceCacheHit.Inc() |
There was a problem hiding this comment.
nit: hit and miss counters are fine. For operators, a gauge of the current covering balance, or of the available balance after each cheque, would probably be more useful when diagnosing out_of_funds.
…ring-balance-cache
CI builds the PR merge, so tests that arrived with the base still called chequebook.New without blockTime. Co-authored-by: Cursor <cursoragent@cursor.com>
| if errors.Is(err, transaction.ErrTransactionCancelled) { | ||
| s.logger.Warning("withdrawal transaction was not mined", "tx", txHash, "amount", amount, "error", err) | ||
| s.lock.Lock() | ||
| delete(s.pendingWithdrawals, txHash) |
There was a problem hiding this comment.
Should we have instead of these 3 lines just s.releaseMinedWithdrawal(txHash) ?
Checklist
Description
Adds an in-memory cache for the chequebook covering balance (balance + totalPaidOut) so cheque issuance no longer does two live RPC calls under the global mutex on every Issue.
Known corner cases
a) Deposit may stay invisible until the cache expires
If the cache is loaded (or reloaded) before a deposit is mined, the node can keep using a stale covering balance for up to 5 minutes and refuse to issue cheques (ErrOutOfFunds). Impact is temporary under-issuance only. In practice the node is already struggling to settle when the chequebook is empty; an extra few minutes is unlikely to change the outcome meaningfully.
b) Withdrawal + cache refresh can allow an uncovered cheque
After Withdraw, the cache is reduced locally. An uncovered cheque is possible only if the cache is refreshed from chain after the withdrawal is broadcast but before it is mined, so the pending withdrawal is missing from the reloaded value.
That requires either:
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5570
Screenshots (if appropriate):
Results:
AI Disclosure