refactor(gas_limiter): canonical + overlay design with epoch tag#488
refactor(gas_limiter): canonical + overlay design with epoch tag#488julio4 wants to merge 1 commit into
Conversation
julio4
force-pushed
the
refactor/gas-limiter-overlay
branch
from
b951e01 to
f7c1bf1
Compare
|
This conflicts hugely with my existing PR (#487). Can you describe to me
Then we can build on top of each other and streamline the work properly |
Yes that's why I extracted it from continuous PR, so that we can merge it first and we can update 487 with this foundation. There's detailed "What was missing" and "What this fixes for continuous mode" sections in the description upper, but tldr we need cheap copy/overlay as only the winning candidate should consume budgets. |
julio4
force-pushed
the
refactor/gas-limiter-overlay
branch
3 times, most recently
from
643d608 to
c7f0ea2
Compare
julio4
force-pushed
the
refactor/gas-limiter-overlay
branch
from
c7f0ea2 to
595ab27
Compare
| self.base.len() | ||
| } | ||
|
|
||
| /// Fork an overlay sharing this canonical's base. O(1). |
There was a problem hiding this comment.
Rename methods for clarity:
refresh->refill_bucketsso it can't be confused with the overlay conceptfork->beginfold_overlay->commit
Makes it more clear this is basically a db transaction
| if let Some(inner) = self.gas_limiter.as_ref() { | ||
| inner.refresh(block_number) | ||
| /// Snapshot the current overlay state for later [`Self::restore`]. | ||
| pub fn checkpoint(&self) -> AddressLimiterCheckpoint { |
There was a problem hiding this comment.
I don't think you need to include checkpoint and restore methods in the public api. If you want to introduce a snapshot system then the AddressBuckets type at the bottom should be able to manage its own snapshots
| /// In-flight overlay. Reads/consumes go through here with `&self`; on | ||
| /// commit the accumulated deltas are folded back into the canonical. | ||
| #[derive(Debug)] | ||
| pub(super) struct GasLimiterOverlay { |
There was a problem hiding this comment.
Can you work on following the generic pattern from bucket.rs when possible? It looks like there don't need to be separate GasLimiterOverlay and ComputeLimiterOverlay definitions
| /// back into the canonical when this ctx is dropped. Shadows the | ||
| /// `address_limiter` field reached via `Deref` to `OpPayloadBuilderCtx`, | ||
| /// so `self.address_limiter` inside this ctx always hits the overlay. | ||
| pub address_limiter: AddressLimiterOverlay, |
There was a problem hiding this comment.
An overlay is something that sits on top of something else to provide more information or context, but this is really a set of changes that are supposed to be atomically applied (or discarded) like a database transaction. I think naming it that way would help a lot with comprehension but since transaction is already taken can you rename the suffix to maybe Guard, Scope, Lease, or something else?
| /// tracks how much was overdrawn. | ||
| #[derive(Debug, Clone)] | ||
| struct TokenBucket<T> { | ||
| pub(super) struct TokenBucket<T> { |
There was a problem hiding this comment.
Other modules shouldn't need to carry around and know about TokenBuckets, just the higher up AddressBuckets and the session type, please restrict this
2 participants
Refactor the per-address gas/compute limiter to use per-payload overlays over a shared canonical limiter.
Each payload job now records limiter usage in its own overlay and commits it back when the job finishes. This keeps in-flight payload builds isolated while preserving persistent sender debt across jobs. The
limiter also rejects stale overlay commits when the canonical state has already advanced.
Changes