Repository navigation
fix(metrics): wire pending nonce events in mempool OTel counter (PLT-1017) #3991
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.
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
Changes from all commits
464e9b8
668bd1d
4360826
56d97bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -353,7 +353,7 @@ func (inner *txStoreInner) shouldReject(txHash types.TxHash) bool { | |
| return false | ||
| } | ||
|
|
||
| func (s *txStore) insert(inner *txStoreInner, wtx *WrappedTx) error { | ||
| func (s *txStore) insert(inner *txStoreInner, wtx *WrappedTx, recordAdded bool) error { | ||
| if _, ok := inner.byHash[wtx.Hash()]; ok { | ||
| return errDuplicateTx | ||
| } | ||
|
|
@@ -371,8 +371,11 @@ func (s *txStore) insert(inner *txStoreInner, wtx *WrappedTx) error { | |
| // Reject transactions with old nonces. | ||
| if evm.nonce < account.firstNonce { | ||
| inner.cache.Push(wtx.Hash(), utils.None[cacheEvm]()) | ||
| recordPendingNonceRejected() | ||
| return errOldNonce | ||
| } | ||
| insertedAtNextNonce := evm.nonce == account.nextNonce | ||
| newTx := wtx | ||
| inner.cache.Push(wtx.Hash(), utils.Some(cacheEvm{ | ||
| priority: wtx.priority, | ||
| address: evm.address, | ||
|
|
@@ -429,6 +432,9 @@ func (s *txStore) insert(inner *txStoreInner, wtx *WrappedTx) error { | |
| if !wtx.readyEl.IsPresent() { | ||
| s.priorityReservoir.Add(wtx.priority) | ||
| wtx.readyEl = utils.Some(s.readyTxs.PushBack(wtx.Tx())) | ||
| if !recordAdded || wtx != newTx || !insertedAtNextNonce { | ||
| recordPendingNonceAccepted() | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
|
|
@@ -442,6 +448,9 @@ func (s *txStore) insert(inner *txStoreInner, wtx *WrappedTx) error { | |
| } | ||
| inner.byHash[wtx.Hash()] = wtx | ||
| inner.state.Store(state) | ||
| if recordAdded && wtx.evm.IsPresent() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] More generally, the four labels are not a closed lifecycle: |
||
| recordPendingNonceAdded() | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -501,7 +510,7 @@ func (inner *txStoreInner) inInclusionOrder() []*WrappedTx { | |
| // txStore takes ownership of wtx. | ||
| func (s *txStore) Insert(wtx *WrappedTx) error { | ||
| for inner := range s.inner.Lock() { | ||
| if err := s.insert(inner, wtx); err != nil { | ||
| if err := s.insert(inner, wtx, true); err != nil { | ||
| return err | ||
| } | ||
| if total := inner.state.Load().total; !total.LessEqual(&inner.hardLimit) { | ||
|
|
@@ -538,7 +547,7 @@ func (s *txStore) compact(inner *txStoreInner, clearAccounts bool) { | |
| total.Inc(wtx.Size()) | ||
| limitOk := total.LessEqual(&inner.softLimit) | ||
| // NOTE: insertion is lazily evaluated here. | ||
| if !limitOk || s.insert(inner, wtx) != nil { | ||
| if !limitOk || s.insert(inner, wtx, false) != nil { | ||
| Global.RemovedTxsAt().Add(1) | ||
| Global.EvictedTxsAt().Add(1) | ||
| if el, ok := wtx.readyEl.Get(); ok { | ||
|
|
@@ -594,8 +603,12 @@ func (s *txStore) Update(spec updateSpec) { | |
| } | ||
| invalid := spec.InvalidTxs[wtx.Hash()] || wtx.check(spec.Constraints) != nil | ||
| _, executed := spec.TxResults[wtx.Hash()] | ||
| remove := invalid || executed || (expired && (s.config.RemoveExpiredTxsFromQueue || !inner.isReady(wtx))) | ||
| removedByExpiry := expired && (s.config.RemoveExpiredTxsFromQueue || !inner.isReady(wtx)) | ||
| remove := invalid || executed || removedByExpiry | ||
| if remove { | ||
| if removedByExpiry && wtx.evm.IsPresent() { | ||
| recordPendingNonceExpired() | ||
| } | ||
|
amir-deris marked this conversation as resolved.
|
||
| // KeepInvalidTxsInCache decides whether we invalidate txs removed from mempool for any reason. | ||
| // Executed txs invalidation was handled earlier. | ||
| if s.config.KeepInvalidTxsInCache && !executed { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,6 @@ var ( | |
| } | ||
|
|
||
| evmAnteMetrics = struct { | ||
| pendingNonce metric.Int64Counter | ||
| nonceMismatch metric.Int64Counter | ||
|
|
||
| // Gas price histogram | ||
|
|
@@ -26,12 +25,6 @@ var ( | |
| // Association errors | ||
| associationError metric.Int64Counter | ||
| }{ | ||
| pendingNonce: must(meter.Int64Counter( | ||
| "evm_pending_nonce", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any concern with this going away? Any alerts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there shouldn't be any concern, as this metric was already not being emitted. |
||
| metric.WithDescription("EVM pending nonce events by type (added, expired, rejected, accepted)"), | ||
| metric.WithUnit("{count}"), | ||
| )), | ||
|
|
||
| nonceMismatch: must(meter.Int64Counter( | ||
| "evm_nonce_mismatch", | ||
| metric.WithDescription("EVM nonce mismatches by cause (too_high, too_low)"), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] Unlike
addedandaccepted,rejectedis not gated byrecordAdded, so it also fires fromcompact()— whichUpdateruns every block withclearAccounts: true, re-readingfirstNoncefrom app state. A queued transaction whose nonce was consumed on chain by some other means is then re-inserted, hitserrOldNonce, and is counted as a rejection even though it is really an eviction of a stale entry. Gating this onrecordAdded(or adding a distinct label for the compaction path) would keep the series meaning "incoming transaction rejected".Conversely — and as Codex noted — this is the only rejection path counted:
errDuplicateTx(duplicate EVM hash) anderrSameNonce(replacement lost on balance or priority) return without incrementing anything, so same-nonce replacement rejections, arguably the most interesting pending-nonce rejection, never appear on the dashboard. Worth either covering them with the same label or documenting thatrejectedmeans "nonce below account nonce" only.