Skip to content
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

op-service: withdrawals typing fixes #7767

Merged
merged 3 commits into from
Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions op-e2e/actions/l2_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ func TestL2EngineAPIBlockBuilding(gt *testing.T) {

nextBlockTime := eth.Uint64Quantity(parent.Time) + 2

var w *eth.Withdrawals
var w *types.Withdrawals
if sd.RollupCfg.IsCanyon(uint64(nextBlockTime)) {
w = &eth.Withdrawals{}
w = &types.Withdrawals{}
}

// Now let's ask the engine to build a block
Expand Down
25 changes: 23 additions & 2 deletions op-e2e/e2eutils/geth/fakepos.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package geth

import (
"math/rand"
protolambda marked this conversation as resolved.
Show resolved Hide resolved
"time"

"github.com/ethereum-optimism/optimism/op-service/clock"
"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/beacon/engine"
"github.com/ethereum/go-ethereum/common"
Expand All @@ -12,6 +12,9 @@ import (
"github.com/ethereum/go-ethereum/eth/catalyst"
"github.com/ethereum/go-ethereum/event"
"github.com/ethereum/go-ethereum/log"

"github.com/ethereum-optimism/optimism/op-service/clock"
"github.com/ethereum-optimism/optimism/op-service/testutils"
)

// fakePoS is a testing-only utility to attach to Geth,
Expand All @@ -22,6 +25,8 @@ type fakePoS struct {
log log.Logger
blockTime uint64

withdrawalsIndex uint64

finalizedDistance uint64
safeDistance uint64

Expand All @@ -33,6 +38,7 @@ func (f *fakePoS) Start() error {
if advancing, ok := f.clock.(*clock.AdvancingClock); ok {
advancing.Start()
}
withdrawalsRNG := rand.New(rand.NewSource(450368975843)) // avoid generating the same address as any test
f.sub = event.NewSubscription(func(quit <-chan struct{}) error {
// poll every half a second: enough to catch up with any block time when ticks are missed
t := f.clock.NewTicker(time.Second / 2)
Expand Down Expand Up @@ -64,6 +70,17 @@ func (f *fakePoS) Start() error {
// We're a long way behind, let's skip some blocks...
newBlockTime = uint64(f.clock.Now().Unix())
}
// create some random withdrawals
withdrawals := make([]*types.Withdrawal, withdrawalsRNG.Intn(4))
for i := 0; i < len(withdrawals); i++ {
withdrawals[i] = &types.Withdrawal{
Index: f.withdrawalsIndex + uint64(i),
Validator: withdrawalsRNG.Uint64() % 100_000_000, // 100 million fake validators
Address: testutils.RandomAddress(withdrawalsRNG),
// in gwei, consensus-layer quirk. withdraw non-zero value up to 50 ETH
Amount: uint64(withdrawalsRNG.Intn(50_000_000_000) + 1),
}
}
res, err := f.engineAPI.ForkchoiceUpdatedV2(engine.ForkchoiceStateV1{
HeadBlockHash: head.Hash(),
SafeBlockHash: safe.Hash(),
Expand All @@ -72,7 +89,7 @@ func (f *fakePoS) Start() error {
Timestamp: newBlockTime,
Random: common.Hash{},
SuggestedFeeRecipient: head.Coinbase,
Withdrawals: make([]*types.Withdrawal, 0),
Withdrawals: withdrawals,
})
if err != nil {
f.log.Error("failed to start building L1 block", "err", err)
Expand Down Expand Up @@ -109,6 +126,10 @@ func (f *fakePoS) Start() error {
f.log.Error("failed to make built L1 block canonical", "err", err)
continue
}
// Increment global withdrawals index in the CL.
// The EL doesn't really care about the value,
// but it's nice to mock something consistent with the CL specs.
f.withdrawalsIndex += uint64(len(withdrawals))
case <-quit:
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions op-e2e/op_geth.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ func (d *OpGeth) CreatePayloadAttributes(txs ...*types.Transaction) (*eth.Payloa
txBytes = append(txBytes, bin)
}

var withdrawals *eth.Withdrawals
var withdrawals *types.Withdrawals
if d.L2ChainConfig.IsCanyon(uint64(timestamp)) {
withdrawals = &eth.Withdrawals{}
withdrawals = &types.Withdrawals{}
}

attrs := eth.PayloadAttributes{
Expand Down
2 changes: 1 addition & 1 deletion op-e2e/op_geth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ func TestCanyon(t *testing.T) {

b, err := opGeth.AddL2Block(ctx)
require.NoError(t, err)
assert.Equal(t, *b.Withdrawals, eth.Withdrawals{})
assert.Equal(t, *b.Withdrawals, types.Withdrawals{})

l1Block, err := opGeth.L2Client.BlockByNumber(ctx, nil)
require.Nil(t, err)
Expand Down
4 changes: 2 additions & 2 deletions op-node/rollup/derive/attributes.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ func (ba *FetchingAttributesBuilder) PreparePayloadAttributes(ctx context.Contex
txs = append(txs, l1InfoTx)
txs = append(txs, depositTxs...)

var withdrawals *eth.Withdrawals
var withdrawals *types.Withdrawals
if ba.cfg.IsCanyon(nextL2Time) {
withdrawals = &eth.Withdrawals{}
withdrawals = &types.Withdrawals{}
}

return &eth.PayloadAttributes{
Expand Down
4 changes: 2 additions & 2 deletions op-node/rollup/derive/engine_consolidate.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func AttributesMatchBlock(attrs *eth.PayloadAttributes, parentHash common.Hash,
return nil
}

func checkWithdrawalsMatch(attrWithdrawals *eth.Withdrawals, blockWithdrawals *eth.Withdrawals) error {
func checkWithdrawalsMatch(attrWithdrawals *types.Withdrawals, blockWithdrawals *types.Withdrawals) error {
if attrWithdrawals == nil && blockWithdrawals == nil {
return nil
}
Expand All @@ -67,7 +67,7 @@ func checkWithdrawalsMatch(attrWithdrawals *eth.Withdrawals, blockWithdrawals *e
for idx, expected := range *attrWithdrawals {
actual := (*blockWithdrawals)[idx]

if expected != actual {
if *expected != *actual {
return fmt.Errorf("expected withdrawal %d to be %v, actual %v", idx, expected, actual)
}
}
Expand Down
23 changes: 12 additions & 11 deletions op-node/rollup/derive/engine_consolidate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package derive
import (
"testing"

"github.com/ethereum-optimism/optimism/op-service/eth"
"github.com/stretchr/testify/require"

"github.com/ethereum/go-ethereum/core/types"
)

func TestWithdrawalsMatch(t *testing.T) {
tests := []struct {
attrs *eth.Withdrawals
block *eth.Withdrawals
attrs *types.Withdrawals
block *types.Withdrawals
shouldMatch bool
}{
{
Expand All @@ -19,36 +20,36 @@ func TestWithdrawalsMatch(t *testing.T) {
shouldMatch: true,
},
{
attrs: &eth.Withdrawals{},
attrs: &types.Withdrawals{},
block: nil,
shouldMatch: false,
},
{
attrs: nil,
block: &eth.Withdrawals{},
block: &types.Withdrawals{},
shouldMatch: false,
},
{
attrs: &eth.Withdrawals{},
block: &eth.Withdrawals{},
attrs: &types.Withdrawals{},
block: &types.Withdrawals{},
shouldMatch: true,
},
{
attrs: &eth.Withdrawals{
attrs: &types.Withdrawals{
{
Index: 1,
},
},
block: &eth.Withdrawals{},
block: &types.Withdrawals{},
shouldMatch: false,
},
{
attrs: &eth.Withdrawals{
attrs: &types.Withdrawals{
{
Index: 1,
},
},
block: &eth.Withdrawals{
block: &types.Withdrawals{
{
Index: 2,
},
Expand Down
12 changes: 6 additions & 6 deletions op-program/client/l2/engineapi/test/l2_engine_api_tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ func RunEngineAPITests(t *testing.T, createBackend func(t *testing.T) engineapi.

nextBlockTime := eth.Uint64Quantity(genesis.Time + 1)

var w *eth.Withdrawals
var w *types.Withdrawals
if api.backend.Config().IsCanyon(uint64(nextBlockTime)) {
w = &eth.Withdrawals{}
w = &types.Withdrawals{}
}

result, err := api.engine.ForkchoiceUpdatedV2(api.ctx, &eth.ForkchoiceState{
Expand Down Expand Up @@ -111,9 +111,9 @@ func RunEngineAPITests(t *testing.T, createBackend func(t *testing.T) engineapi.
t.Run("RejectInvalidBlockHash", func(t *testing.T) {
api := newTestHelper(t, createBackend)

var w *eth.Withdrawals
var w *types.Withdrawals
if api.backend.Config().IsCanyon(uint64(0)) {
w = &eth.Withdrawals{}
w = &types.Withdrawals{}
}

// Invalid because BlockHash won't be correct (among many other reasons)
Expand Down Expand Up @@ -385,9 +385,9 @@ func (h *testHelper) startBlockBuilding(head *types.Header, newBlockTimestamp et
}

canyonTime := h.backend.Config().CanyonTime
var w *eth.Withdrawals
var w *types.Withdrawals
if canyonTime != nil && *canyonTime <= uint64(newBlockTimestamp) {
w = &eth.Withdrawals{}
w = &types.Withdrawals{}
}

result, err := h.engine.ForkchoiceUpdatedV2(h.ctx, &eth.ForkchoiceState{
Expand Down
18 changes: 10 additions & 8 deletions op-service/eth/ssz.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"io"
"math"
"sync"

"github.com/ethereum/go-ethereum/core/types"
)

type BlockVersion int
Expand Down Expand Up @@ -180,16 +182,16 @@ func (payload *ExecutionPayload) MarshalSSZ(w io.Writer) (n int, err error) {
offset += transactionSize
// dyanmic value 3: Withdrawals
if payload.Withdrawals != nil {
marshalWithdrawals(buf[offset:], payload.Withdrawals)
marshalWithdrawals(buf[offset:], *payload.Withdrawals)
}

return w.Write(buf)
}

func marshalWithdrawals(out []byte, withdrawals *Withdrawals) {
func marshalWithdrawals(out []byte, withdrawals types.Withdrawals) {
offset := uint32(0)

for _, withdrawal := range *withdrawals {
for _, withdrawal := range withdrawals {
binary.LittleEndian.PutUint64(out[offset:offset+8], withdrawal.Index)
offset += 8
binary.LittleEndian.PutUint64(out[offset:offset+8], withdrawal.Validator)
Expand Down Expand Up @@ -305,14 +307,14 @@ func (payload *ExecutionPayload) UnmarshalSSZ(version BlockVersion, scope uint32
if err != nil {
return fmt.Errorf("failed to unmarshal withdrawals list: %w", err)
}
payload.Withdrawals = withdrawals
payload.Withdrawals = &withdrawals
}

return nil
}

func unmarshalWithdrawals(in []byte) (*Withdrawals, error) {
result := &Withdrawals{}
func unmarshalWithdrawals(in []byte) (types.Withdrawals, error) {
result := types.Withdrawals{} // empty list by default, intentionally non-nil

if len(in)%withdrawalSize != 0 {
return nil, errors.New("invalid withdrawals data")
Expand All @@ -327,7 +329,7 @@ func unmarshalWithdrawals(in []byte) (*Withdrawals, error) {
offset := 0

for i := 0; i < withdrawalCount; i++ {
withdrawal := Withdrawal{}
withdrawal := &types.Withdrawal{}

withdrawal.Index = binary.LittleEndian.Uint64(in[offset : offset+8])
offset += 8
Expand All @@ -341,7 +343,7 @@ func unmarshalWithdrawals(in []byte) (*Withdrawals, error) {
withdrawal.Amount = binary.LittleEndian.Uint64(in[offset : offset+8])
offset += 8

*result = append(*result, withdrawal)
result = append(result, withdrawal)
}

return result, nil
Expand Down
28 changes: 15 additions & 13 deletions op-service/eth/ssz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import (
"math"
"testing"

"github.com/ethereum/go-ethereum/common"
"github.com/google/go-cmp/cmp"
"github.com/holiman/uint256"
"github.com/stretchr/testify/require"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
)

// FuzzExecutionPayloadUnmarshal checks that our SSZ decoding never panics
Expand Down Expand Up @@ -138,9 +140,9 @@ func FuzzExecutionPayloadMarshalUnmarshalV2(f *testing.F) {
}

wCount = wCount % maxWithdrawalsPerPayload
withdrawals := make(Withdrawals, wCount)
withdrawals := make(types.Withdrawals, wCount)
for i := 0; i < int(wCount); i++ {
withdrawals[i] = Withdrawal{
withdrawals[i] = &types.Withdrawal{
Index: a,
Validator: b,
Address: common.BytesToAddress(data[:20]),
Expand Down Expand Up @@ -228,10 +230,10 @@ func TestOPB04(t *testing.T) {

tests := []struct {
version BlockVersion
withdrawals *Withdrawals
withdrawals *types.Withdrawals
}{
{BlockV1, nil},
{BlockV2, &Withdrawals{}},
{BlockV2, &types.Withdrawals{}},
}

for _, test := range tests {
Expand All @@ -246,7 +248,7 @@ func TestOPB04(t *testing.T) {

}

func createPayloadWithWithdrawals(w *Withdrawals) *ExecutionPayload {
func createPayloadWithWithdrawals(w *types.Withdrawals) *ExecutionPayload {
return &ExecutionPayload{
ParentHash: common.HexToHash("0x123"),
FeeRecipient: common.HexToAddress("0x456"),
Expand All @@ -267,27 +269,27 @@ func createPayloadWithWithdrawals(w *Withdrawals) *ExecutionPayload {
}

func TestMarshalUnmarshalWithdrawals(t *testing.T) {
emptyWithdrawal := &Withdrawals{}
withdrawals := &Withdrawals{
emptyWithdrawal := &types.Withdrawals{}
withdrawals := &types.Withdrawals{
{
Index: 987,
Validator: 654,
Address: common.HexToAddress("0x898"),
Amount: 321,
},
}
maxWithdrawals := make(Withdrawals, maxWithdrawalsPerPayload)
maxWithdrawals := make(types.Withdrawals, maxWithdrawalsPerPayload)
for i := 0; i < maxWithdrawalsPerPayload; i++ {
maxWithdrawals[i] = Withdrawal{
maxWithdrawals[i] = &types.Withdrawal{
Index: 987,
Validator: 654,
Address: common.HexToAddress("0x898"),
Amount: 321,
}
}
tooManyWithdrawals := make(Withdrawals, maxWithdrawalsPerPayload+1)
tooManyWithdrawals := make(types.Withdrawals, maxWithdrawalsPerPayload+1)
for i := 0; i < maxWithdrawalsPerPayload+1; i++ {
tooManyWithdrawals[i] = Withdrawal{
tooManyWithdrawals[i] = &types.Withdrawal{
Index: 987,
Validator: 654,
Address: common.HexToAddress("0x898"),
Expand All @@ -299,7 +301,7 @@ func TestMarshalUnmarshalWithdrawals(t *testing.T) {
name string
version BlockVersion
hasError bool
withdrawals *Withdrawals
withdrawals *types.Withdrawals
}{
{"ZeroWithdrawalsSucceeds", BlockV2, false, emptyWithdrawal},
{"ZeroWithdrawalsFailsToDeserialize", BlockV1, true, emptyWithdrawal},
Expand Down
Loading