chore: settle on the capture taken for the settle time - #1434
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesSettlement Capture Selection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change implements the intended earliest-capture settlement behavior without a supported merge-blocking issue. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Time Submission Status
Submit or update total time with: Add time on top of previous submission with: See available commands to help comply with our Guidelines. |
|
@holdex pr submit-time 4h |
A market can carry more than one capture.
request_attestationfreezes the queryresult in the block it lands in and a validator signs it in a later one, so a poll
falling between those two blocks sees nothing signed and asks for another. On
mainnet 205 of the 1,073 markets that carry a capture carry two, taken 6 to
220 blocks apart with a mean of 30.
settle_marketchose between them with:That is signing order, which has nothing to do with the market. Worse, for 128
of the 205 both rows were signed in the same block, so the ordering is a tie and
the row the database happens to return first decides the outcome. The two captures
read the query against different chain state, so they are not interchangeable.
Why the earliest
Straight from the requirement: "capture the state of the query as close to
settle_timeas possible", and a ladder "resolves against the value that existedat
settle_time". The settlement path only captures a market once it has fallendue, so the first capture is the closest it got. Every later one is further away,
and whether it exists at all depends on how long signing took.
Markets already settled under the old ordering are not revisited. #1430 puts
remediation out of scope and asks for it to be tracked separately.
The pre-capture gap
request_attestationis a general primitive: it mentions neither markets norsettle_time, it isPUBLIC, and a live market's query components are readablefrom
ob_queries. So a capture can be taken beforesettle_time, over awindow that has not closed, and under this ordering it would win.
Never observed: 0 of 1,319 captures on mainnet predate their market's creation,
minimum gap +34 blocks. And the ordering being replaced has the same exposure
pointing the other way — capture after watching the value move, win if signed
last — which is the worse of the two, because it can be played with hindsight.
Closing it properly means bounding a capture to the settlement window, which needs
a timestamp on
attestations. That table stores heights only, so it is a schemachange and not this PR.
What changed
settle_marketnow selectsORDER BY created_height ASCamong rows that carry asignature, rather than the most recently signed row of any kind.
The two failure messages are also separated. Selecting only signed rows means a
market with an unsigned capture would otherwise report "attestation not found",
which sends an operator looking for the wrong thing, so the not-found branch now
distinguishes "never captured" from "captured, not yet signed". Both were reachable
before; only the wording changes.
No schema change.
settle_marketcarries no.prod.sqloverride, confirmed byre-running
scripts/generate_prod_migrations.py.Tests
One integration test, mutation-checked against real Postgres: two signed captures
of one market, at heights 10 and 20, reading values that give opposite outcomes.
Settlement must resolve TRUE, the earlier one. Reverting the
ORDER BYfails itwith exactly that message.
Getting it to mean anything took two corrections worth recording:
value = 0for the earlier capture.insert_recordsdrops zeros unless the stream opts in (
046-allow-zeros-config.sql), so therecord was silently discarded and the capture came back with an empty payload.
The test now calls
set_allow_zerosfirst.same value, so reverting the
ORDER BYstill passed. Only the mutation checkcaught it.
Scope
This does not stop a market acquiring two captures in the first place. That is:
It also does not touch ladders. A ladder whose books were captured in different
blocks still settles book by book, and holding one back needs a ladder identity on
ob_queriesthat does not exist yet — an uncaptured book has noattestationsrow,so the grouping cannot be derived at settlement time.