-
Notifications
You must be signed in to change notification settings - Fork 2
feat(sim): dedup sim cache items #74
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
Conversation
Uses an identifier to have a seen cache for items, keeping it up to date with the actual `SimItem` cache.
@@ -9,14 +15,19 @@ use signet_bundle::SignetEthBundle; | |||
pub enum SimItem { | |||
/// A bundle to be simulated. | |||
Bundle(SignetEthBundle), |
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.
qq, can't this be instantiated by downstream consumers via SimItem::Bundle(bundle)
? If so, let's remove the ability to do that? that would allow constructing SimItem
s that violate the "must have UUID" invariant
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.
will be done on a later pr, requires riffing on the least annoying way to do this since rust doesn't support this natively :D
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.
okay just a few small things left
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.
approving pending 2 nites
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.
bug 🐞
let item = item.into(); | ||
let Ok(item) = SimItem::try_from(item) else { | ||
// Skip invalid bundles |
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.
should we log here that a bundle is being discarded as invalid?
items.retain(|_, item| { | ||
// Retain only items that are not bundles or are valid in the current block. | ||
if let SimItem::Bundle(bundle) = item { | ||
let should_remove = bundle.bundle.block_number == block_number |
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.
we should remove bundles if they match the current block number? why?
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 var looks more like should_keep
, no?
- bundle block number == current block number
- bundle min timestamp <= current timestamp
- bundle max timestamp >= current timestamp
these are validity conditions
dont submit a review after merge. create a ticket instead |
Uses an identifier to have a seen cache for items, keeping it up to date with the actual
SimItem
cache. This adds additional complexity in the inner structure of the cache, but this complexity is necessarily to avoid clones on most methods.This also switches to
parking_lot
'sRwLock
, which is much faster than the standardstd::sync
one.Closes ENG-1126