From 50008ef7a51b368123fba3756005bed69b71773d Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Tue, 16 Sep 2025 10:40:04 -0400 Subject: [PATCH 1/2] Address TODO in LastToSettleAt --- blocks/settlement.go | 21 +++++++-------------- blocks/settlement_test.go | 10 ++-------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/blocks/settlement.go b/blocks/settlement.go index 7208681..c402d9e 100644 --- a/blocks/settlement.go +++ b/blocks/settlement.go @@ -127,39 +127,32 @@ func settling(lastOfParent, lastOfCurr *Block) []*Block { // See the Example for [Block.IfChildSettles] for one usage of the returned // block. func LastToSettleAt(settleAt uint64, parent *Block) (*Block, bool) { - // These variables are only abstracted for clarity; they are not needed - // beyond the scope of the `for` loop. - var block, child *Block - block = parent // therefore `child` remains nil because it's what we're building - // The only way [Block.ParentBlock] can be nil is if `block` was already // settled (see invariant in [Block]). If a block was already settled then // only that or a later (i.e. unsettled) block can be returned by this loop, // therefore we have a guarantee that the loop update will never result in // `block==nil`. - for ; ; block, child = block.ParentBlock(), block { + var executionIsBehind bool + for block := parent; ; block = block.ParentBlock() { if startsNoEarlierThan := block.Time(); startsNoEarlierThan > settleAt { + executionIsBehind = false continue } if t := block.executionExceededSecond.Load(); t != nil && *t >= settleAt { + executionIsBehind = false continue } if e := block.execution.Load(); e != nil { if e.byGas.CompareUnix(settleAt) > 0 { // There may have been a race between this check and the // execution-exceeded one above, so we have to check again. + executionIsBehind = false continue } - return block, true + return block, !executionIsBehind } - // TODO(arr4n) more fine-grained checks are possible for scenarios where - // (a) `block` could never execute before `settleAt` so we would - // `continue`; and (b) `block` will definitely execute in time and - // `child` could never, in which case return `nil, false`. - _ = child - - return nil, false + executionIsBehind = true } } diff --git a/blocks/settlement_test.go b/blocks/settlement_test.go index a8f1eee..f396855 100644 --- a/blocks/settlement_test.go +++ b/blocks/settlement_test.go @@ -327,14 +327,8 @@ func TestLastToSettleAt(t *testing.T) { { settleAt: 9, parent: blocks[9], - // The current implementation is very coarse-grained and MAY return - // false negatives that would simply require a retry after some - // indeterminate period of time. Even though the execution time of - // `blocks[8]` guarantees that `blocks[9]` MUST finish execution - // after the settlement time, our current implementation doesn't - // check this. It is expected that this specific test case will one - // day fail, at which point it MUST be updated to want `blocks[7]`. - wantOK: false, + wantOK: true, + want: blocks[7], }, { settleAt: 15, From fc4a2360a747d79b218bf27ba28c6437fc842093 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Wed, 17 Sep 2025 07:54:28 +0100 Subject: [PATCH 2/2] refactor: use positive boolean + lock test invariant --- blocks/settlement.go | 30 ++++++++++++++++++++++++------ blocks/settlement_test.go | 5 +++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/blocks/settlement.go b/blocks/settlement.go index e0bd2e9..dfcaca7 100644 --- a/blocks/settlement.go +++ b/blocks/settlement.go @@ -156,32 +156,50 @@ func settling(lastOfParent, lastOfCurr *Block) []*Block { // See the Example for [Block.WhenChildSettles] for one usage of the returned // block. func LastToSettleAt(settleAt uint64, parent *Block) (*Block, bool) { + // A block can be the last to settle at some time i.f.f. two criteria are + // met: + // + // 1. The block has finished execution by said time and; + // + // 2. The block's child is known to have *not* finished execution or be + // unable to finish by that time. + // + // The block currently being built can never finish in time, so we start + // with criterion (2) being met. + known := true + // The only way [Block.ParentBlock] can be nil is if `block` was already // settled (see invariant in [Block]). If a block was already settled then // only that or a later (i.e. unsettled) block can be returned by this loop, // therefore we have a guarantee that the loop update will never result in // `block==nil`. - var executionIsBehind bool for block := parent; ; block = block.ParentBlock() { if startsNoEarlierThan := block.BuildTime(); startsNoEarlierThan > settleAt { - executionIsBehind = false + known = true continue } + // TODO(arr4n) more fine-grained checks are possible by computing the + // minimum possible gas consumption of blocks. For example, + // `block.BuildTime()+block.intrinsicGasSum()` can be compared against + // `settleAt`, as can the sum of a chain of blocks. if t := block.executionExceededSecond.Load(); t != nil && *t >= settleAt { - executionIsBehind = false + known = true continue } if e := block.execution.Load(); e != nil { if e.byGas.CompareUnix(settleAt) > 0 { // There may have been a race between this check and the // execution-exceeded one above, so we have to check again. - executionIsBehind = false + known = true continue } - return block, !executionIsBehind + return block, known } - executionIsBehind = true + // Note that a grandchild block having unknown execution completion time + // does not rule out knowing a child's completion time, so this could be + // set to true in a future loop iteration. + known = false } } diff --git a/blocks/settlement_test.go b/blocks/settlement_test.go index ec0f66d..2343e2f 100644 --- a/blocks/settlement_test.go +++ b/blocks/settlement_test.go @@ -272,6 +272,11 @@ func TestLastToSettleAt(t *testing.T) { requireTime(t, 13, 1) blocks[8].markExecutedForTests(t, db, tm) + require.False( + t, blocks[9].Executed(), + "Block 9 MUST remain unexecuted", // exercises lagging-execution logic when building on 9 + ) + for i, b := range blocks { // Setting interim execution time isn't required for the algorithm to // work as it just allows [LastToSettleAt] to return definitive results