-
Notifications
You must be signed in to change notification settings - Fork 418
feat: Add mempool drop checks to the provider heartbeat #2689
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
base: main
Are you sure you want to change the base?
Conversation
/// Transaction was dropped from the mempool. | ||
#[error("transaction was dropped from the mempool")] | ||
Dropped, |
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.
This change is technically breaking, as the enum is not marked with #[non_exhaustive]
, but I'm not sure what's the project policy in that case is.
if let Some(check_interval) = self.mempool_check_interval { | ||
if last_mempool_check.elapsed() >= check_interval { | ||
self.reap_mempool_wipes().await; | ||
last_mempool_check = Instant::now(); | ||
} | ||
} |
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.
There's a potential timing issue in the mempool check logic. Currently, last_mempool_check
is updated after the async reap_mempool_wipes()
completes, but the elapsed time check happens before starting this operation. If the mempool check takes significant time to complete, subsequent iterations might incorrectly skip checks because the elapsed time would include the previous check's duration.
Consider updating last_mempool_check
before starting the async operation or adjusting the logic to account for the time spent in the async operation. This would ensure consistent check intervals regardless of how long each check takes to complete.
if let Some(check_interval) = self.mempool_check_interval { | |
if last_mempool_check.elapsed() >= check_interval { | |
self.reap_mempool_wipes().await; | |
last_mempool_check = Instant::now(); | |
} | |
} | |
if let Some(check_interval) = self.mempool_check_interval { | |
if last_mempool_check.elapsed() >= check_interval { | |
last_mempool_check = Instant::now(); | |
self.reap_mempool_wipes().await; | |
} | |
} | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
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.
It's probably better to run these checks less often, in case e.g. some network request takes too long.
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.
this is likely a bit problematic for networks without a mempool, e.g. on op the tx is usually not retrievable until it was mined, because most providers dont maintain a mempool and strictly forward it
so this means we cant really rely on this behaviour
Hmm, interesting. Do you have any ideas for a workaround? I believe that we do need some kind of fix, because |
This isn't just an issue in |
Motivation
Fixes #2678
Solution
Adds a period check to the heartbeat to check if transactions are still present in mempool.
This turned out to be more complex than I initially expected. Reasons:
anvil
won't mine new blocks without need by default.So I introduced configurable mempool check interval on the root provider. If it's not provided, we will set it to
poll_interval * 10
(which seems to be a reasonable amount of time for transaction to appear in mempool). If it's provided and it'sSome
, we will use provided interval. For providedNone
, we will disable checks.Heartbeat integration is pretty straightforward: for any unconfirmed transaction we remember when it was added to the watch list, and start checking it only after configured interval has passed.
cc @mattsse
PR Checklist