Ayelet/mempool/naive mempool/rebase over 14 1 get ts#12368
Conversation
|
This change is |
ayeletstarkware
force-pushed
the
ayelet/mempool/naive-mempool/rebase-over-14-1-get_ts
branch
from
8c2f44b to
66cbada
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Queue is empty, return 0 | ||
| info!("Mempool get_ts (FIFO): queue empty, returning 0"); | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Empty queue returns timestamp zero instead of clock fallback
Medium Severity
When the FIFO queue is empty, get_ts() returns 0, which propagates as Ok(0) through the batcher client. In get_proposal_timestamp, Ok(ts) matches and returns 0 (Unix epoch) as the block timestamp, bypassing the clock.unix_now() fallback. The old code using state_sync_client correctly fell through to clock time when no data was available.
Additional Locations (1)
ayeletstarkware
force-pushed
the
ayelet/mempool/naive-mempool/rebase-over-14-1-get_ts
branch
from
66cbada to
1dffdf0
Compare
| // | ||
| // // Don't call get_ts() - get_txs() should return empty | ||
| // get_txs_and_assert_expected(&mut mempool, 1, &[]); | ||
| // } |
There was a problem hiding this comment.
Commented-out test code accidentally committed
Low Severity
A commented-out test test_get_txs_returns_empty_if_get_ts_not_called references create_mempool_with_mock_server, a function that doesn't exist in the codebase. This appears to be work-in-progress scaffolding that was left behind and clutters the test file.
ayeletstarkware
force-pushed
the
ayelet/mempool/naive-mempool/rebase-over-14-1-get_ts
branch
from
1dffdf0 to
4449e93
Compare
ayeletstarkware
force-pushed
the
ayelet/mempool/naive-mempool/rebase-over-14-1-get_ts
branch
2 times, most recently
from
40d0c5e to
50cb3ca
Compare
| | MempoolRequest::GetMempoolSnapshot() => RequestPriority::Normal, | ||
| | MempoolRequest::GetMempoolSnapshot() | ||
| | MempoolRequest::GetTimestamp | ||
| | MempoolRequest::UpdateTimestamps(_) => RequestPriority::Normal, |
There was a problem hiding this comment.
GetTimestamp request priority too low for block creation
Medium Severity
MempoolRequest::GetTimestamp is assigned Normal priority, but it's on the block creation critical path — get_ts must be called before get_txs in Echonet mode to set the timestamp threshold that pop_ready_chunk uses to filter transactions. Since GetTransactions and CommitBlock are High priority, under load the GetTimestamp request could be starved by high-priority requests, delaying block creation. It likely belongs alongside GetTransactions at High priority.
Additional Locations (1)
ayeletstarkware
force-pushed
the
ayelet/mempool/naive-mempool/rebase-over-14-1-get_ts
branch
from
50cb3ca to
8dcc1c4
Compare
2 participants
Note
Medium Risk
Changes the mempool’s transaction selection semantics in Echonet mode and introduces a new externally callable timestamp update endpoint, which could affect block assembly and ordering if timestamp mappings are incorrect or missing.
Overview
Adds Echonet timestamp propagation across the stack: new
POST /gateway/update_timestampsendpoint forwards aHashMap<TransactionHash, u64>throughapollo_http_server→apollo_gateway→apollo_mempoolvia newUpdateTimestampsrequests.Extends mempool’s Echonet FIFO behavior with a
get_ts/timestamp-threshold mechanism: FIFO queue tracks per-tx timestamps,get_ts()returns the first queued tx’s timestamp and sets a threshold, andget_txs()only pops contiguous transactions matching that timestamp (otherwise returns none), with tests updated/added accordingly. Also updates Grafana dashboard panels to includeget_timestamplatency metrics for batcher and mempool.Written by Cursor Bugbot for commit 8dcc1c4. This will update automatically on new commits. Configure here.