apollo_mempool: fetch timestamps in echonet mode#12850
apollo_mempool: fetch timestamps in echonet mode#12850ayeletstarkware merged 1 commit intomain-v0.14.2from
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
39d5cf6 to
ba917fb
Compare
ba917fb to
9a5a3fc
Compare
ab52141 to
2466bfa
Compare
9a5a3fc to
5ba5a55
Compare
2466bfa to
cc87e5a
Compare
5ba5a55 to
6a3d616
Compare
6a3d616 to
c2e542f
Compare
cc87e5a to
a1546f9
Compare
c2e542f to
859c833
Compare
859c833 to
97080c0
Compare
a1546f9 to
98f8f25
Compare
e5f0d51 to
1ef60f5
Compare
1ef60f5 to
21674b7
Compare
0d78cce to
b8b509f
Compare
90b9b4e to
a837dfb
Compare
b8b509f to
269bda9
Compare
a837dfb to
f8cb984
Compare
269bda9 to
7eb9c20
Compare
matanl-starkware
left a comment
There was a problem hiding this comment.
@matanl-starkware reviewed 11 files and all commit messages, and made 6 comments.
Reviewable status: 11 of 12 files reviewed, 7 unresolved discussions (waiting on ayeletstarkware and ron-starkware).
crates/apollo_mempool/src/communication.rs line 54 at r4 (raw file):
mempool_p2p_propagator_client: SharedMempoolP2pPropagatorClient, config_manager_client: SharedConfigManagerClient, http_client: reqwest::Client,
What's the motivation for keeping this as a member?
Is it to preserve the connection opened?
Code quote:
http_client: reqwest::Client,crates/apollo_mempool/src/communication.rs line 109 at r4 (raw file):
let tx_hash = args_wrapper.args.tx.tx_hash(); if !self.fetch_and_update_timestamp(tx_hash).await { warn!("Failed to fetch timestamp for tx {}, skipping transaction", tx_hash);
Is it printed as hex by default?
Code quote:
{}crates/apollo_mempool/src/communication.rs line 189 at r4 (raw file):
} } }
Consider using reqwest_retry crate.
See example here:
Code quote:
for attempt in 0..MAX_RETRIES {
match self.try_fetch_timestamp(&url, attempt).await {
Ok(timestamp) => {
self.mempool.update_timestamp(tx_hash, timestamp);
return true;
}
Err(e) => {
if attempt + 1 >= MAX_RETRIES {
warn!("Failed to fetch timestamp for tx {}: {}", tx_hash, e);
return false;
}
sleep(std::time::Duration::from_millis(RETRY_DELAY_MS)).await;
}
}
}crates/apollo_mempool/src/communication.rs line 197 at r4 (raw file):
&self, url: &reqwest::Url, attempt: usize,
IMO this method should not care about attempts.
The caller may print the attempt number if Err is returned.
Code quote:
attempt: usize,crates/apollo_mempool/src/mempool.rs line 293 at r4 (raw file):
pub(crate) fn update_timestamp(&mut self, tx_hash: TransactionHash, timestamp: UnixTimestamp) { if !self.is_fifo() { debug!(
This should never happen, unless something is broken.
In fact, I suggest:
Suggestion (i):
panicCode snippet (ii):
assert!(self.is_fifo(), "Update timestamp called for tx {} in non-echonet mode")crates/apollo_node/resources/config_schema.json line 3602 at r4 (raw file):
"value": 1000000 } }
Missing a new line
7eb9c20 to
023d9f7
Compare
f8cb984 to
938bbec
Compare
ayeletstarkware
left a comment
There was a problem hiding this comment.
@ayeletstarkware made 6 comments.
Reviewable status: 11 of 12 files reviewed, 7 unresolved discussions (waiting on matanl-starkware and ron-starkware).
crates/apollo_mempool/src/communication.rs line 54 at r4 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
What's the motivation for keeping this as a member?
Is it to preserve the connection opened?
stale
crates/apollo_mempool/src/communication.rs line 109 at r4 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Is it printed as hex by default?
yes
crates/apollo_mempool/src/communication.rs line 189 at r4 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Consider using reqwest_retry crate.
See example here:
Done.
crates/apollo_mempool/src/communication.rs line 197 at r4 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
IMO this method should not care about attempts.
The caller may print the attempt number if Err is returned.
Done.
crates/apollo_mempool/src/mempool.rs line 293 at r4 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
This should never happen, unless something is broken.
In fact, I suggest:
Done.
crates/apollo_node/resources/config_schema.json line 3602 at r4 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Missing a new line
Done.
938bbec to
fa47fa1
Compare
023d9f7 to
5f97a73
Compare
matanl-starkware
left a comment
There was a problem hiding this comment.
@matanl-starkware reviewed 6 files and all commit messages, made 3 comments, and resolved 6 discussions.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on ayeletstarkware and ron-starkware).
crates/apollo_mempool/src/communication.rs line 58 at r5 (raw file):
mempool_p2p_propagator_client: SharedMempoolP2pPropagatorClient, config_manager_client: SharedConfigManagerClient, client: ClientWithMiddleware,
Too generic IMO.
Maybe echonet_client?
Code quote:
clientcrates/apollo_mempool/src/communication.rs line 68 at r5 (raw file):
) -> Self { const MIN_RETRY_INTERVAL: Duration = Duration::from_millis(50); const MAX_RETRY_INTERVAL: Duration = Duration::from_millis(50);
Suggestion:
const MAX_RETRY_INTERVAL: Duration = Duration::from_millis(500);crates/apollo_mempool/src/communication.rs line 180 at r5 (raw file):
// Returns true if successful, false if failed after all retries. pub(crate) async fn fetch_and_update_timestamp(&mut self, tx_hash: TransactionHash) -> bool { let recorder_url = &self.mempool.config.static_config.recorder_url;
Lets add a comment explaining why in EchoNet setup we fetch the timestamp from the recorder...
Code quote:
.recorder_urlfa47fa1 to
1eab5c8
Compare
ayeletstarkware
left a comment
There was a problem hiding this comment.
@ayeletstarkware made 3 comments.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on matanl-starkware and ron-starkware).
crates/apollo_mempool/src/communication.rs line 58 at r5 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Too generic IMO.
Maybeechonet_client?
Done.
crates/apollo_mempool/src/communication.rs line 180 at r5 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Lets add a comment explaining why in EchoNet setup we fetch the timestamp from the recorder...
Done.
crates/apollo_mempool/src/communication.rs line 68 at r5 (raw file):
) -> Self { const MIN_RETRY_INTERVAL: Duration = Duration::from_millis(50); const MAX_RETRY_INTERVAL: Duration = Duration::from_millis(50);
Done. just to confirm:
Previously, with min=max=50ms, there’s no exponential backoff, the client retries every 50ms for up to 10s (~200 attempts).
With min=50ms, max=500ms, exponential backoff is enabled (50, 100,... 500ms), resulting in ~30 retries over 10s with increasing delays.
matanl-starkware
left a comment
There was a problem hiding this comment.
@matanl-starkware reviewed 4 files and all commit messages, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ron-starkware).

Note
Medium Risk
Adds external HTTP dependency and changes transaction ingestion behavior in Echonet mode (transactions may be skipped on recorder failures), which can affect replay correctness and test stability.
Overview
In
apollo_mempool,add_txnow conditionally fetches a transaction’s original mainnet timestamp from a configured recorder endpoint in Echonet mode (with request timeout + exponential backoff retries) and skips the transaction if the timestamp cannot be fetched.This introduces a new static config field
mempool_config.static_config.recorder_url(plumbed into node config pointers and schema), adds timestamp update plumbing (update_timestamp) on the mempool/queue trait surface, and includes ignored integration tests with anaxummock recorder server.Written by Cursor Bugbot for commit 1eab5c8. This will update automatically on new commits. Configure here.