Skip to content
40 changes: 24 additions & 16 deletions hook/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,39 +15,47 @@ import (
"github.com/ava-labs/libevm/core/types"
"github.com/ava-labs/libevm/params"

"github.com/ava-labs/strevm/blocks"
"github.com/ava-labs/strevm/gastime"
"github.com/ava-labs/strevm/intmath"
saeparams "github.com/ava-labs/strevm/params"
)

// Points define user-injected hook points.
type Points interface {
GasTarget(parent *types.Block) gas.Gas
SubSecondBlockTime(*types.Block) gas.Gas
// GasTarget returns the amount of gas per second that the chain should
// target to consume after executing the provided block.
GasTarget(*types.Header) gas.Gas
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GasTarget returns the amount of gas per second that the chain should
// target to consume after executing the provided block.
GasTarget(*types.Header) gas.Gas
// GasTargetAfter returns the amount of gas per second that the chain should
// target to consume after executing the provided block.
GasTargetAfter(*types.Header) gas.Gas

Also (nit), something about the wording of "to consume after executing the provided block" feels off and it confused me for a split second. Perhaps "target to consume in subsequent blocks."? The current phrasing suggests immediacy, not long-term horizon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworded it, lmk what you think

// SubSecondBlockTime returns the sub-second portion of the block time based
// on the provided gas rate.
//
// For example, if the block timestamp is 10.75 seconds and the gas rate is
// 100 gas/second, then this method should return 75 gas.
SubSecondBlockTime(gasRate gas.Gas, h *types.Header) gas.Gas
// BeforeBlock is called immediately prior to executing the block.
BeforeBlock(params.Rules, *state.StateDB, *types.Block) error
// AfterBlock is called immediately after executing the block.
AfterBlock(*state.StateDB, *types.Block, types.Receipts)
}

// BeforeBlock is intended to be called before processing a block, with the gas
// target sourced from [Points].
func BeforeBlock(pts Points, rules params.Rules, sdb *state.StateDB, b *blocks.Block, clock *gastime.Time) error {
// BeforeBlock is intended to be called before processing a block.
func BeforeBlock(pts Points, rules params.Rules, sdb *state.StateDB, b *types.Block, clock *gastime.Time) error {
r := clock.Rate()
clock.FastForwardTo(
b.BuildTime(),
pts.SubSecondBlockTime(b.EthBlock()),
b.Time(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to differentiate between the different times, much like how we do with state roots. Granted in this case it couldn't be execution (gas nor wall) time, but more generally I want to be more precise in this code base. Happy to change BuildTime() to something else if you'd like, but I'd prefer to not use raw Time() (which should probably be removed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BuildTime doesn't exist on the provided type. The function is passing in types.Block, not blocks.Block. Are you suggesting to revert the change and take in blocks.Block again?

pts.SubSecondBlockTime(r, b.Header()),
)
target := pts.GasTarget(b.ParentBlock().EthBlock())
if err := clock.SetTarget(target); err != nil {
return fmt.Errorf("%T.SetTarget() before block: %w", clock, err)
}
return pts.BeforeBlock(rules, sdb, b.EthBlock())
return pts.BeforeBlock(rules, sdb, b)
}

// AfterBlock is intended to be called after processing a block, with the gas
// sourced from [types.Block.GasUsed] or equivalent.
func AfterBlock(pts Points, sdb *state.StateDB, b *types.Block, clock *gastime.Time, used gas.Gas, rs types.Receipts) {
// AfterBlock is intended to be called after processing a block.
func AfterBlock(pts Points, sdb *state.StateDB, b *types.Block, clock *gastime.Time, used gas.Gas, rs types.Receipts) error {
clock.Tick(used)
target := pts.GasTarget(b.Header())
if err := clock.SetTarget(target); err != nil {
return fmt.Errorf("%T.SetTarget() after block: %w", clock, err)
}
pts.AfterBlock(sdb, b, rs)
return nil
}

// MinimumGasConsumption MUST be used as the implementation for the respective
Expand Down
64 changes: 64 additions & 0 deletions hook/hook_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright (C) 2025, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package hook_test

import (
"testing"

"github.com/ava-labs/avalanchego/vms/components/gas"
"github.com/ava-labs/libevm/core/types"
"github.com/ava-labs/libevm/params"
"github.com/ava-labs/strevm/gastime"
"github.com/ava-labs/strevm/hook/hookstest"
"github.com/ava-labs/strevm/saetest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

. "github.com/ava-labs/strevm/hook"
)

// TestTargetUpdateTiming verifies that the gas target is modified in AfterBlock
// rather than BeforeBlock.
func TestTargetUpdateTiming(t *testing.T) {
const (
initialTime = 42
initialTarget gas.Gas = 1_600_000
initialExcess = 1_234_567_890
)
tm := gastime.New(initialTime, initialTarget, initialExcess)

const (
newTime uint64 = initialTime + 1
newTarget = initialTarget + 100_000
)
hook := &hookstest.Stub{
Target: newTarget,
}
header := &types.Header{
Time: newTime,
}
block := types.NewBlock(header, nil, nil, nil, saetest.TrieHasher())

initialPrice := tm.Price()
require.NoError(t, BeforeBlock(hook, params.TestRules, nil, block, tm), "BeforeBlock()")
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()")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should tighten this (and the AfterBlock() equivalent) to remove the equalities. It probably requires an accompanying comment for each, to explain that the strict inequality is synthetic, but it gives us slightly stronger guarantees of directionality in the test.

if t.Failed() {
t.FailNow()
}

const (
secondsOfGasUsed = 3
initialRate = initialTarget * gastime.TargetToRate
used gas.Gas = initialRate * secondsOfGasUsed
expectedEndTime = newTime + secondsOfGasUsed
)
require.NoError(t, AfterBlock(hook, nil, block, tm, used, nil), "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()")
}
6 changes: 3 additions & 3 deletions hook/hookstest/stub.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ type Stub struct {
var _ hook.Points = (*Stub)(nil)

// GasTarget ignores its argument and always returns [Stub.Target].
func (s *Stub) GasTarget(parent *types.Block) gas.Gas {
func (s *Stub) GasTarget(*types.Header) gas.Gas {
return s.Target
}

// SubSecondBlockTime time ignores its argument and always returns 0.
func (*Stub) SubSecondBlockTime(*types.Block) gas.Gas {
// SubSecondBlockTime time ignores its arguments and always returns 0.
func (*Stub) SubSecondBlockTime(gas.Gas, *types.Header) gas.Gas {
return 0
}

Expand Down
7 changes: 3 additions & 4 deletions saexec/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (e *Executor) execute(b *blocks.Block, logger logging.Logger) error {

rules := e.chainConfig.Rules(b.Number(), true /*isMerge*/, b.BuildTime())
gasClock := parent.ExecutedByGasTime().Clone()
if err := hook.BeforeBlock(e.hooks, rules, stateDB, b, gasClock); err != nil {
if err := hook.BeforeBlock(e.hooks, rules, stateDB, b.EthBlock(), gasClock); err != nil {
return fmt.Errorf("before-block hook: %v", err)
}
perTxClock := gasClock.Time.Clone()
Expand Down Expand Up @@ -162,9 +162,8 @@ func (e *Executor) execute(b *blocks.Block, logger logging.Logger) error {
receipts[ti] = receipt
}
endTime := time.Now()
hook.AfterBlock(e.hooks, stateDB, b.EthBlock(), gasClock, blockGasConsumed, receipts)
if gasClock.Time.Compare(perTxClock) != 0 {
return fmt.Errorf("broken invariant: block-resolution clock @ %s does not match tx-resolution clock @ %s", gasClock.String(), perTxClock.String())
if err := hook.AfterBlock(e.hooks, stateDB, b.EthBlock(), gasClock, blockGasConsumed, receipts); err != nil {
return fmt.Errorf("after-block hook: %v", err)
}

logger.Debug(
Expand Down