fix: pamm state overrides block context - #4691
Open
kirsanium wants to merge 2 commits into
Open
Conversation
The quote stream's frames describe the block the builder is about to build (chain head + 1), not the block a simulation runs against. The gate compared the frame's block number to the chain head, so it withheld the overrides essentially always: sampling the live stream over one persistent connection for 60s applied them once and withheld them 59 times, with the snapshot naming head + 1 in 55 samples and head + 2 in four. Relaxing the block comparison alone is not enough. The words carry a freshness stamp in their leading four bytes and the venue reverts StaleUpdate() unless it equals the block's timestamp, so a frame applied verbatim reverts just as reliably at head. Replace current() with an accessor taking the simulation context. A snapshot at or ahead of the simulated block is served with the newest frame's stamp rewritten to that block's timestamp; a snapshot behind it is withheld, because the maker has moved on from the price it carries. Only words carrying the newest frame's stamp are rewritten, so a lane that was not requoted keeps its older stamp and stays dead exactly as it would on chain. Stamps are identified by proximity to the frame's own publish time: other venues pack price data into the same leading bytes that reads as a plausible unix timestamp, and rewriting it would corrupt their quotes. Only the four stamp bytes are touched, never the maker's price next to them. Frames are also merged per storage slot rather than per account. Every venue keeps its lanes in the same shared registry account and each frame carries only its own, so inserting the account wholesale dropped the other venues' lanes. Finally, split the Stale metric into too_old, wrong_block and empty, and record the case where no frame ever arrived, so the next failure of this kind is visible rather than reading as general stream ill-health.
Gas estimation runs against pending, whose timestamp is the node's wall clock: it is unknowable when the request is built and moves between calls. Overrides restamped for a known block therefore cannot survive there, and the venue rejects them. When an override set is available and applied, run the estimate against the block it was stamped for. Without overrides the call keeps using pending exactly as before, so this only affects deployments that have opted into [simulator.state-override-stream]. This trades pending's view of the mempool for a context the overrides are valid in, which is a judgement call worth making explicitly rather than silently.
5 tasks
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes #4690
The pAMM state-override stream added in #4606 encodes an assumption about the frames it consumes that turns out to be subtly wrong, and as a result the overrides are applied approximately never.
A frame's
blockNumberis the block the builder is about to build — chain head + 1 — and its words are stamped with that block's slot timestamp. The gate compares that number to the current head, so it withholds the overrides almost every time. Driving the current client code with one long-lived connection and a real block watcher, sampling once a second for 60s:The single hit is the ~1s sliver right after a block lands, before the stream bumps on.
The same thing measured through the metric itself, by driving this crate's code against the live stream with a real WS block watcher — the driver's startup order — and sampling
current()every 100ms for 5 minutes:Same harness, same duration, same 2965 calls in both runs.
Relaxing the block comparison alone is not sufficient. The registry words carry a freshness stamp in their leading four bytes, and the venue reverts
StaleUpdate()(0x666a2814) unless that stamp equals the timestamp of the block the call runs against. Quoting 1000 USDT into WETH on the FermiSwap venue at head25646525(ts 1785424751), where the live snapshot named25646526and carried stamp1785424763— exactlyhead_ts + 12:0x666a28140x666a2814(stamp is 12s in the future)pendingcannot be the target context either. Its number is head + 1, matching the snapshot, but its timestamp is the node's wall clock and moves between calls:The value the node will use is unknowable when the request is built and moves before it executes, so restamping for
pendingis impossible.Finally, the two consumers simulate in contexts with different
block.timestamps behind one no-argument accessor — verification pins to head, gas estimation runs atpending— which is the structural reasoncurrent()cannot serve both.Changes
current()withoverrides_for(block, timestamp), taking the simulation context. A snapshot at or ahead of the simulated block is served (ahead is the normal case); a snapshot behind it is withheld, because the maker has moved on from the price it carries.max()rule would happen to still pick the right stamp today — but it classifies those words as stamps, and nothing about the encoding guarantees they stay below the live stamp. Anchoring removes the failure mode rather than relying on that margin. There is a regression test using a captured frame whose word leads with0x5d393a13(2019-07-25).0xda7afeed01fe625cf15d187a19f94b45f00b8c5f) and each frame carries only its own, soinsertdropped the other venues' lanes. In the same 90s sample, four distinct venues wrote that one account (837, 839, 528 and 106 frames respectively), each frame carrying 1–8 lanes, 19 distinct lanes merged overall.stalemetric label intotoo_old/wrong_block/empty, and record an outcome when no frame has ever arrived (previously an early return with no sample at all), so the next failure of this kind is visible instead of reading as general stream ill-health.(head, ts_head)— it already pinned to head, so this is just plumbing the timestamp through.The second commit is the one behavioural question and is deliberately separable: gas estimation currently runs at
pending, which cannot carry these overrides. It pins the estimate to the block the overrides were stamped for only when an override set is available and applied, and otherwise keepspendingbyte-for-byte as today. That confines the change to deployments that have opted into[simulator.state-override-stream]and leaves gas estimation untouched for everyone else, but it does tradepending's view of the mempool for a context the overrides are valid in.It ships with its own live test,
estimates_gas_for_pamm_call, which drivesSimulator::gasagainst the real stream and asserts three things: the estimate reverts with no overrides, succeeds with them, and — the reason for the change — that those very same overrides are rejected atpending.The alternative I did not take, and why
Rewriting the stamp produces a state that exists at no point on the timeline: at head the slot held an older stamp, at head + 1 it will hold
head_ts + 12. "Maker's newest price stamped for head" is never real. That is worth being explicit about rather than glossing.The more faithful option is to leave the venue's words untouched and move the simulation into the block they were stamped for. The settlement executes in that block, and the builder sequences the maker's update immediately before it, so that state genuinely will exist. Titan's docs point the same way: the Takers page names
eth_simulateV1alongsideeth_call, andeth_simulateV1refuses to simulate a block that does not advance time past its parent.I implemented it and verified it end to end. One real settlement produces byte-identical output through both simulation paths. It still isn't what this PR does, because the cost lands somewhere unrelated to pAMMs:
Only
eth_simulateV1can express a block context.eth_callcannot override the block it executes in, andeth_estimateGascannot either — so both simulation paths have to migrate.The gas path cannot opt out. Without the overrides the settlement reverts
StaleUpdate(),eth_estimateGasfails, and the solution is discarded, so pAMM routes would never settle.eth_simulateV1cannot answereth_estimateGas's question. Per geth'sExecutionResult,UsedGasis "total used gas, refunded gas is deducted" andMaxUsedGasis "maximum gas consumed during execution, excluding gas refunds";eth_estimateGasbinary-searches the smallest starting limit the call survives, which must also cover the 63/64 reserve at every nested call. On the Aave debt-swap replay in this repo:Gas::newtakes one number and uses it both for scoring (a cost question, wheregas_usedis right) and for the submission limit via2x(a limit question, where onlyestimate_gasis safe). Adopting the block-context approach forces those apart and changes gas semantics for every settlement the driver produces, not just pAMM ones — the submission cushion drops from 2x the true requirement to 1.16–1.35x, calibrated from a single settlement whose call depth may not be representative.Indicative of the wider surface:
eth_simulateV1defaults the simulated block's base fee to zero, so it has to be pinned explicitly or every contract in the settlement sees a different environment.Splitting
Gasinto separate cost and limit figures is the prerequisite for that migration, and it deserves its own issue rather than arriving behind a pAMM fix. Rewriting the stamp keepseth_estimateGas, and with it the meaning of the number: still the smallest starting limit the call survives, 63/64 reserve included.Gas.estimatecarries on meaning what it means today, so scoring and the submission cushion are untouched. The only change is which block the estimate runs against — head instead ofpending, and only when overrides are applied — which costspending's view of the mempool and nothing else. If the team would rather take the migration first, the working implementation exists and I'll open it instead.How to test
Unit tests alongside the existing ones in the module cover: restamping to the simulated block's timestamp; a lane carried over from an earlier block keeping its stale stamp; non-stamp words being byte-identical after restamping; a snapshot behind the simulated block being withheld; two venues' frames sharing the registry account both surviving the merge; and an unrelated word that reads as a plausible timestamp not being treated as a stamp.
There is also an
#[ignore]d live conformance test — its absence is why this shipped broken and stayed broken. It spawns the real stream, calls the new accessor at real chain head, and quotes on the FermiSwap venue, asserting that the same call reverts without the overrides and succeeds with them, so it cannot pass vacuously:Note that the router's quote function is addressed by selector (
0x300aa47f,(address,address,uint256) view, returning(amountIn, amountOut)): the contract is unverified and the symbol name isn't recoverable.A second
#[ignore]d test,serves_overrides_across_blocks, samples the accessor at chain head across several blocks with a real WS block watcher and requires virtually every sample to be served — the property the block-number gate actually broke. It is what produced the before/after table above; the old gate scores about 5% on it.Both tests install a
rustlscrypto provider before connecting (rustlsis added as a dev-dependency only). The binaries don't need this — the alloy websocket transport installs one while opening the block stream, well before this stream connects — but a test process has nothing doing that for it, and the workspace links more than one provider, so the choice has to be explicit.