feat: LastToSettleAt() supports unknown grandchild execution time
#19
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a TODO comment in the LastToSettleAt function by implementing more fine-grained execution time checks. The implementation now tracks when execution is behind the settlement time and uses this information to provide more accurate results.
- Removes coarse-grained implementation that returned false negatives
- Adds execution timing tracking with
executionIsBehindflag - Updates test expectations to reflect improved accuracy
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| blocks/settlement.go | Implements fine-grained execution time checking with executionIsBehind tracking |
| blocks/settlement_test.go | Updates test case to expect accurate results instead of false negatives |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
blocks/settlement.go
Outdated
| continue | ||
| } | ||
| return block, true | ||
| return block, !executionIsBehind |
There was a problem hiding this comment.
The logic for returning !executionIsBehind appears incorrect. When execution data is available (e != nil) and execution time is within the settlement window (e.byGas.CompareUnix(settleAt) <= 0), the block should be valid regardless of the executionIsBehind flag state. The executionIsBehind flag tracks whether we've encountered blocks that can't execute in time, but if we have actual execution data showing completion within the window, that should always return true.
| return block, !executionIsBehind | |
| return block, true |
blocks/settlement.go
Outdated
| var executionIsBehind bool | ||
| for block := parent; ; block = block.ParentBlock() { | ||
| if startsNoEarlierThan := block.BuildTime(); startsNoEarlierThan > settleAt { | ||
| executionIsBehind = false |
There was a problem hiding this comment.
Setting executionIsBehind = false when startsNoEarlierThan > settleAt is incorrect logic. If a block's build time is after the settlement time, it indicates the block cannot execute before settlement, which should set executionIsBehind = true, not false.
| executionIsBehind = false | |
| executionIsBehind = true |
ARR4N
changed the title
LastToSettleAt() supports unknown grandchild execution time
ARR4N
left a comment
ARR4N
left a comment
There was a problem hiding this comment.
Nice! Thank you. I pushed a slight refactoring and updated the PR title to reflect what's happening.
Was thinking about how best to convey what I expected to happen here and realized it would be easiest to just make a PR.