Skip to content

chore(metrics): add log to tx store and metric for num height iterated on height sync#1071

Merged
aljo242 merged 3 commits intomainfrom
ma/num-rechecks-metircs
Mar 11, 2026
Merged

chore(metrics): add log to tx store and metric for num height iterated on height sync#1071
aljo242 merged 3 commits intomainfrom
ma/num-rechecks-metircs

Conversation

@mattac21
Copy link
Contributor

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

@greptile-apps
Copy link

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR threads a log.Logger through the HeightSync factory and both TxStore implementations (LegacyPool and CosmosTxStore), adds a log line in TxStore.Txs() reporting how many transactions were collected vs. stored, and introduces two new observability additions: a height_sync/heights meter metric in HeightSync.EndCurrentHeight() and a total counter field on TxStore to back the log.

Key changes:

  • HeightSync.reset signature updated to func(log.Logger) *Store; a logger is stored on the struct and forwarded to every new store created at height transitions.
  • legacypool.New gains a logger parameter; a tagged sub-logger (pool=legacypool) is passed to HeightSync.
  • NewTxStore (EVM) accepts and uses a logger, tagging it with txstore=evm.
  • NewCosmosTxStore accepts (and intentionally ignores) a logger to satisfy the updated interface.
  • TxStore gains a total uint64 field incremented in AddTxs and decremented in RemoveTx; the value is surfaced in the new Info-level log.
  • Two issues found: (1) the Txs() log uses Info level in a per-block hot path, risking log flooding in production; (2) AddTxs checks t.lookup for deduplication but never populates it, meaning total will overcount when the same transaction is re-submitted.

Confidence Score: 3/5

  • Safe to merge with low operational risk, but the total_in_store metric logged on every block will be inaccurate due to a missing lookup population in AddTxs, and the Info-level log may flood production logs.
  • The core logic changes (metric registration, logger threading, signature updates) are all correct and well-scoped. However, the new total counter is built on top of a pre-existing deduplication gap (lookup never written in AddTxs), making the logged value unreliable. Additionally, logging at Info in Txs() — a per-block hot path — is a meaningful operational concern for production deployments.
  • mempool/txpool/legacypool/tx_store.go — the total counter accuracy and log level both need attention before this lands in production.

Important Files Changed

Filename Overview
mempool/txpool/legacypool/tx_store.go Adds total counter and logger to TxStore; logging is at Info level in a hot path, and the total counter is inaccurate because AddTxs checks but never populates lookup, allowing duplicates to inflate the count.
mempool/internal/heightsync/heightsync.go Adds hsHeights meter metric (marked in EndCurrentHeight) and threads a log.Logger through the reset factory function; changes are straightforward and correct.
mempool/mempool.go Passes logger to legacypool.New and a tagged sub-logger to heightsync.New for the cosmos recheck pool; straightforward plumbing change.
mempool/tx_store.go Updates NewCosmosTxStore to accept (and ignore) a log.Logger to satisfy the updated reset function signature; no functional change.
mempool/txpool/legacypool/legacypool.go Adds logger cosmoslog.Logger parameter to New and passes a tagged sub-logger to heightsync.New; clean signature update with no other side effects.

Sequence Diagram

sequenceDiagram
    participant BP as Block Proposer
    participant HS as HeightSync[TxStore]
    participant TS as TxStore
    participant M as Metrics

    Note over HS: StartNewHeight(h)
    HS->>TS: reset(logger) → new TxStore
    HS->>M: (height change signalled)

    Note over HS: EndCurrentHeight()
    HS->>M: hsHeights.Mark(1)
    HS->>HS: close(done)

    BP->>HS: GetStore(ctx, h)
    HS-->>BP: *TxStore (after done / ctx timeout)

    BP->>TS: Txs(filter)
    TS->>TS: filter & convert txs
    TS->>TS: logger.Info("collected txs", total_in_store, num_selected)
    TS->>M: txsCollected.Mark(numSelected)
    TS-->>BP: map[addr][]*LazyTransaction
Loading

Comments Outside Diff (1)

  1. mempool/txpool/legacypool/tx_store.go, line 110-123 (link)

    lookup is never populated, making total inaccurate

    AddTxs checks t.lookup to skip duplicate transactions, but lookup is never written to anywhere in this file — it is only read here and deleted from in RemoveTx. Because lookup is always empty, the deduplication check on line 112 is always false and every incoming transaction is unconditionally appended. As a result, the new total counter (and the total_in_store log field) will overcount if the same transaction is submitted more than once.

    To make total accurate, AddTxs should also populate lookup for every transaction it adds:

    toAdd = append(toAdd, tx)
    t.lookup[tx.Hash()] = struct{}{}

    Without this, both the duplicate-prevention logic and the total_in_store metric are unreliable.

Last reviewed commit: 7a5b275

@codecov
Copy link

codecov bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.91%. Comparing base (bea1ab9) to head (3b49984).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
mempool/txpool/legacypool/tx_store.go 44.44% 1 Missing and 4 partials ⚠️
mempool/internal/heightsync/heightsync.go 50.00% 1 Missing and 2 partials ⚠️
mempool/tx_store.go 0.00% 0 Missing and 1 partial ⚠️
mempool/txpool/legacypool/legacypool.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1071      +/-   ##
==========================================
- Coverage   64.11%   63.91%   -0.20%     
==========================================
  Files         331      331              
  Lines       23303    23316      +13     
==========================================
- Hits        14941    14903      -38     
+ Misses       6946     6944       -2     
- Partials     1416     1469      +53     
Files with missing lines Coverage Δ
mempool/mempool.go 58.18% <100.00%> (+0.58%) ⬆️
mempool/tx_store.go 81.25% <0.00%> (-18.75%) ⬇️
mempool/txpool/legacypool/legacypool.go 64.44% <50.00%> (-1.78%) ⬇️
mempool/internal/heightsync/heightsync.go 68.91% <50.00%> (-18.23%) ⬇️
mempool/txpool/legacypool/tx_store.go 76.31% <44.44%> (-20.75%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aljo242 aljo242 enabled auto-merge March 11, 2026 21:10
@aljo242 aljo242 added this pull request to the merge queue Mar 11, 2026
Merged via the queue into main with commit e037743 Mar 11, 2026
20 of 21 checks passed
@aljo242 aljo242 deleted the ma/num-rechecks-metircs branch March 11, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants