apollo_batcher: Add get_ts from mempool for echonet#12456
Closed
ron-starkware wants to merge 1 commit intoayelet/mempool/naive-mempool/rebase-over-14-1-get_tsfrom
Closed
apollo_batcher: Add get_ts from mempool for echonet#12456ron-starkware wants to merge 1 commit intoayelet/mempool/naive-mempool/rebase-over-14-1-get_tsfrom
ron-starkware wants to merge 1 commit intoayelet/mempool/naive-mempool/rebase-over-14-1-get_tsfrom
Conversation
| Ok(ts) => return ts, | ||
| Err(err) => { | ||
| warn!("Failed to get timestamp from batcher, falling back to clock time: {err:?}"); | ||
| } |
There was a problem hiding this comment.
Zero timestamp used instead of clock fallback
Medium Severity
When the mempool queue is empty and no prior get_ts call was made, batcher.get_ts() returns Ok(0). The get_proposal_timestamp function treats this as a valid timestamp and returns 0 (Unix epoch) instead of falling back to clock.unix_now(). The old code using state sync would fall back to clock time when no data was available, since the if let Ok(Some(block_header)) pattern wouldn't match on Ok(None). The new code has no equivalent fallback because the mempool call always succeeds — the "no data" case is encoded as Ok(0) rather than as an error.
Additional Locations (1)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Note
Medium Risk
Touches consensus proposal timestamp selection and adds a new cross-component request/response variant; incorrect timestamps or integration mismatches could affect block info and downstream validation, though changes are scoped and include tests for the new FIFO behavior.
Overview
Adds a new batcher RPC/trait method
get_ts(wired throughBatcherRequest::GetTimestamp/BatcherResponse::GetTimestamp) that returns a mempool-sourced timestamp.Updates consensus proposal building to optionally use this batcher-provided “original” timestamp in Echonet mode (renaming the flag to
use_original_timestamp), falling back to local clock time on errors.Adjusts Echonet FIFO mempool timestamp behavior so
get_ts()returns the last returned timestamp when the queue is empty (instead of0), addsTransactionQueueTrait::get_last_returned_timestamp, and includes a regression test for the empty-queue-after-drain case.Written by Cursor Bugbot for commit 5ac1dd9. This will update automatically on new commits. Configure here.