Remove hook.BeforeBlock and hook.AfterBlock #40
Conversation
ARR4N
left a comment
ARR4N
left a comment
There was a problem hiding this comment.
Only a quick pass over the fuzzing for now as my comments on your earlier PRs will affect this one.
| // Copyright (C) 2025, Ava Labs, Inc. All rights reserved. | ||
| // See the file LICENSE for licensing terms. | ||
|
|
||
| // Package gastime measures time based on the consumption of gas. |
There was a problem hiding this comment.
| // Package gastime measures time based on the consumption of gas. |
| f.Fuzz(func( | ||
| t *testing.T, | ||
| initTimestamp, initTarget, initExcess, | ||
| time0, timeFrac0, used0, limit0, target0, |
| BeforeBlock(worstcase, hook, header) | ||
| BeforeBlock(actual, hook, header) | ||
|
|
||
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) |
There was a problem hiding this comment.
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) | |
| require.LessOrEqualf(t, actual.Price(), worstcase.Price(), "actual <= worst-case %T.Price()", actual) |
|
|
||
| worstcase := New(initTimestamp, gas.Gas(initTarget), gas.Gas(initExcess)) | ||
| actual := New(initTimestamp, gas.Gas(initTarget), gas.Gas(initExcess)) | ||
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) |
There was a problem hiding this comment.
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) |
This looks like it was from an early iteration because it's repeated below and is also too lax (and kinda pointless) here.
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) | ||
|
|
||
| AfterBlock(worstcase, block.limit, hook, header) | ||
| AfterBlock(actual, block.used, hook, header) |
There was a problem hiding this comment.
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) | |
| AfterBlock(worstcase, block.limit, hook, header) | |
| AfterBlock(actual, block.used, hook, header) | |
| // The crux of this test lies in the maintaining of this inequality | |
| // through the use of `limit` instead of `used` | |
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) | |
| AfterBlock(worstcase, block.limit, hook, header) | |
| AfterBlock(actual, block.used, hook, header) |
There was a problem hiding this comment.
This precludes the hook package ever importing the blocks package. Not a problem for now, but flagging it so you're thinking about it. This will be OK if hooks only ever deal with Eth types, but that might not always be the case.
| func BeforeBlock(clock *Time, pts hook.Points, h *types.Header) { | ||
| r := clock.Rate() | ||
| toFrac := pts.SubSecondBlockTime(r, h) | ||
| clock.FastForwardTo(h.Time, toFrac) | ||
| } | ||
|
|
||
| // AfterBlock is intended to be called after processing a block, with the target | ||
| // sourced from [hook.Points] and [types.Header]. | ||
| func AfterBlock(clock *Time, used gas.Gas, pts hook.Points, h *types.Header) error { | ||
| clock.Tick(used) | ||
| target := pts.GasTarget(h) | ||
| if err := clock.SetTarget(target); err != nil { | ||
| return fmt.Errorf("%T.SetTarget() after block: %w", clock, err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
| func BeforeBlock(clock *Time, pts hook.Points, h *types.Header) { | |
| r := clock.Rate() | |
| toFrac := pts.SubSecondBlockTime(r, h) | |
| clock.FastForwardTo(h.Time, toFrac) | |
| } | |
| // AfterBlock is intended to be called after processing a block, with the target | |
| // sourced from [hook.Points] and [types.Header]. | |
| func AfterBlock(clock *Time, used gas.Gas, pts hook.Points, h *types.Header) error { | |
| clock.Tick(used) | |
| target := pts.GasTarget(h) | |
| if err := clock.SetTarget(target); err != nil { | |
| return fmt.Errorf("%T.SetTarget() after block: %w", clock, err) | |
| } | |
| return nil | |
| } | |
| func (tm *Time) BeforeBlock(hooks hook.Points, h *types.Header) { | |
| r := tm.Rate() | |
| toFrac := hooks.SubSecondBlockTime(r, h) | |
| tm.FastForwardTo(h.Time, toFrac) | |
| } | |
| // AfterBlock is intended to be called after processing a block, with the target | |
| // sourced from [hook.Points] and [types.Header]. | |
| func (tm *Time) AfterBlock(used gas.Gas, hooks hook.Points, h *types.Header) error { | |
| tm.Tick(used) | |
| target := hooks.GasTarget(h) | |
| if err := tm.SetTarget(target); err != nil { | |
| return fmt.Errorf("%T.SetTarget() after block: %w", tm, err) | |
| } | |
| return nil | |
| } |
(subjective) Once they're here, being methods seems more natural, and the change from clock to tm is to match other receivers.
(readability) The name pts made more sense when within the context of the hook package, but in gastime I think something more explicit is warranted.
(subjective, optional pedantry) In addition to the above points, which are included in the suggestion, the arguments to AfterBlock() feel like they're in the wrong order. clock.AfterBlock(e.hooks, b.Header(), gasUsed) goes from most general to most specific. But really I'm just typing this out because I want to know if I'm the only one who feels uncomfortable with weird argument orders.
In implementing the worstcase gas pricing, I want to share the gastime updating logic as much as possible. However, that should not result in calling
hook.Points.BeforeBlockorhook.Points.AfterBlock.This PR separates the gastime changes and also includes fuzzing to ensure that updating the target can not invalidate the worst case gas price assumptions.