-
Notifications
You must be signed in to change notification settings - Fork 1
Remove hook.BeforeBlock and hook.AfterBlock #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
8b038bf
af596f7
178724f
19c3fb9
3b7e488
6851d5a
0d8cb33
d8e67f3
b4450bb
f34665a
0373d0a
94e1fb9
5c4a6e1
16badd0
abe0d6d
8a8b9e2
537bc52
ad66e07
2339b16
73f2f6c
cb79047
f206ac4
67171b5
6a11f2a
b7f4ce2
694b7f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,32 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
StephenButtolph marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package gastime | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/ava-labs/avalanchego/vms/components/gas" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/ava-labs/libevm/core/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/ava-labs/strevm/hook" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // BeforeBlock is intended to be called before processing a block, with the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // timestamp sourced from [hook.Points] and [types.Header]. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I think that we are equally pedantic but with different optimization functions lol.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| // Copyright (C) 2025, Ava Labs, Inc. All rights reserved. | ||
| // See the file LICENSE for licensing terms. | ||
|
|
||
| package gastime | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/ava-labs/avalanchego/vms/components/gas" | ||
| "github.com/ava-labs/libevm/core/types" | ||
| "github.com/ava-labs/strevm/hook/hookstest" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestTargetUpdateTiming verifies that the gas target is modified in AfterBlock | ||
| // rather than BeforeBlock. | ||
| func TestTargetUpdateTiming(t *testing.T) { | ||
StephenButtolph marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const ( | ||
| initialTime = 42 | ||
| initialTarget gas.Gas = 1_600_000 | ||
| initialExcess = 1_234_567_890 | ||
| ) | ||
| tm := New(initialTime, initialTarget, initialExcess) | ||
|
|
||
| const ( | ||
| newTime uint64 = initialTime + 1 | ||
| newTarget = initialTarget + 100_000 | ||
| ) | ||
| hook := &hookstest.Stub{ | ||
| Target: newTarget, | ||
| } | ||
| header := &types.Header{ | ||
| Time: newTime, | ||
| } | ||
|
|
||
| initialPrice := tm.Price() | ||
| BeforeBlock(tm, hook, header) | ||
| assert.Equal(t, newTime, tm.Unix(), "Unix time advanced by BeforeBlock()") | ||
| assert.Equal(t, initialTarget, tm.Target(), "Target not changed by BeforeBlock()") | ||
|
|
||
| enforcedPrice := tm.Price() | ||
| assert.LessOrEqual(t, enforcedPrice, initialPrice, "Price should not increase in BeforeBlock()") | ||
| if t.Failed() { | ||
| t.FailNow() | ||
| } | ||
|
|
||
| const ( | ||
| secondsOfGasUsed = 3 | ||
| initialRate = initialTarget * TargetToRate | ||
| used gas.Gas = initialRate * secondsOfGasUsed | ||
| expectedEndTime = newTime + secondsOfGasUsed | ||
| ) | ||
| require.NoError(t, AfterBlock(tm, used, hook, header), "AfterBlock()") | ||
| assert.Equal(t, expectedEndTime, tm.Unix(), "Unix time advanced by AfterBlock()") | ||
| assert.Equal(t, newTarget, tm.Target(), "Target updated by AfterBlock()") | ||
| assert.GreaterOrEqual(t, tm.Price(), enforcedPrice, "Price should not decrease in AfterBlock()") | ||
| } | ||
|
|
||
| func FuzzWorstCasePrice(f *testing.F) { | ||
| f.Fuzz(func( | ||
| t *testing.T, | ||
| initTimestamp, initTarget, initExcess, | ||
| time0, timeFrac0, used0, limit0, target0, | ||
StephenButtolph marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| time1, timeFrac1, used1, limit1, target1, | ||
| time2, timeFrac2, used2, limit2, target2, | ||
| time3, timeFrac3, used3, limit3, target3 uint64, | ||
| ) { | ||
| initTarget = max(initTarget, 1) | ||
|
|
||
| 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()) | ||
StephenButtolph marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| blocks := []struct { | ||
| time uint64 | ||
| timeFrac gas.Gas | ||
| used gas.Gas | ||
| limit gas.Gas | ||
| target gas.Gas | ||
| }{ | ||
| { | ||
| time: time0, | ||
| timeFrac: gas.Gas(timeFrac0), | ||
| used: gas.Gas(used0), | ||
| limit: gas.Gas(limit0), | ||
| target: gas.Gas(target0), | ||
| }, | ||
| { | ||
| time: time1, | ||
| timeFrac: gas.Gas(timeFrac1), | ||
| used: gas.Gas(used1), | ||
| limit: gas.Gas(limit1), | ||
| target: gas.Gas(target1), | ||
| }, | ||
| { | ||
| time: time2, | ||
| timeFrac: gas.Gas(timeFrac2), | ||
| used: gas.Gas(used2), | ||
| limit: gas.Gas(limit2), | ||
| target: gas.Gas(target2), | ||
| }, | ||
| { | ||
| time: time3, | ||
| timeFrac: gas.Gas(timeFrac3), | ||
| used: gas.Gas(used3), | ||
| limit: gas.Gas(limit3), | ||
| target: gas.Gas(target3), | ||
| }, | ||
| } | ||
| for _, block := range blocks { | ||
| block.limit = max(block.used, block.limit) | ||
| block.target = clampTarget(max(block.target, 1)) | ||
| block.timeFrac %= rateOf(block.target) | ||
|
|
||
| header := &types.Header{ | ||
| Time: block.time, | ||
| } | ||
| hook := &hookstest.Stub{ | ||
| Target: block.target, | ||
| SubSecondTime: block.timeFrac, | ||
| } | ||
|
|
||
| BeforeBlock(worstcase, hook, header) | ||
| BeforeBlock(actual, hook, header) | ||
|
|
||
| require.LessOrEqual(t, actual.Price(), worstcase.Price()) | ||
StephenButtolph marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| AfterBlock(worstcase, block.limit, hook, header) | ||
| AfterBlock(actual, block.used, hook, header) | ||
StephenButtolph marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This precludes the
hookpackage ever importing theblockspackage. 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.