Code review feedback

op-node/p2p/sync.go

@@ -621,8 +621,10 @@ func (s *SyncClient) doRequest(ctx context.Context, id peer.ID, expectedBlockNum
 		return fmt.Errorf("failed to read response: %w", err)
 	}
 
+	expectedBlockTime := s.cfg.TimestampForBlock(expectedBlockNum)
+
 	blockVersion := eth.BlockV1
-	if s.cfg.IsCanyon(expectedBlockNum) {
+	if s.cfg.IsCanyon(expectedBlockTime) {
 		blockVersion = eth.BlockV2
 	}
 	var res eth.ExecutionPayload

op-node/rollup/derive/engine_consolidate.go

@@ -41,6 +41,37 @@ func AttributesMatchBlock(attrs *eth.PayloadAttributes, parentHash common.Hash,
 	if *attrs.GasLimit != block.GasLimit {
 		return fmt.Errorf("gas limit does not match. expected %d. got: %d", *attrs.GasLimit, block.GasLimit)
 	}
+	if withdrawalErr := checkWithdrawalsMatch(attrs.Withdrawals, block.Withdrawals); withdrawalErr != nil {
+		return withdrawalErr
+	}
+	return nil
+}
+
+func checkWithdrawalsMatch(attrWithdrawals *eth.Withdrawals, blockWithdrawals *eth.Withdrawals) error {
+	if attrWithdrawals == nil && blockWithdrawals == nil {
+		return nil
+	}
+
+	if attrWithdrawals == nil && blockWithdrawals != nil {
+		return fmt.Errorf("expected withdrawals in block to be nil, actual %v", *blockWithdrawals)
+	}
+
+	if attrWithdrawals != nil && blockWithdrawals == nil {
+		return fmt.Errorf("expected withdrawals in block to be non-nil %v, actual nil", *attrWithdrawals)
+	}
+
+	if len(*attrWithdrawals) != len(*blockWithdrawals) {
+		return fmt.Errorf("expected withdrawals in block to be %d, actual %d", len(*attrWithdrawals), len(*blockWithdrawals))
+	}
+
+	for idx, expected := range *attrWithdrawals {
+		actual := (*blockWithdrawals)[idx]
+
+		if expected != actual {
+			return fmt.Errorf("expected withdrawal %d to be %v, actual %v", idx, expected, actual)
+		}
+	}
+
 	return nil
 }
 

op-node/rollup/derive/engine_consolidate_test.go

+package derive
+
+import (
+	"testing"
+
+	"github.com/ethereum-optimism/optimism/op-service/eth"
+	"github.com/stretchr/testify/require"
+)
+
+func TestWithdrawalsMatch(t *testing.T) {
+	tests := []struct {
+		attrs       *eth.Withdrawals
+		block       *eth.Withdrawals
+		shouldMatch bool
+	}{
+		{
+			attrs:       nil,
+			block:       nil,
+			shouldMatch: true,
+		},
+		{
+			attrs:       &eth.Withdrawals{},
+			block:       nil,
+			shouldMatch: false,
+		},
+		{
+			attrs:       nil,
+			block:       &eth.Withdrawals{},
+			shouldMatch: false,
+		},
+		{
+			attrs:       &eth.Withdrawals{},
+			block:       &eth.Withdrawals{},
+			shouldMatch: true,
+		},
+		{
+			attrs: &eth.Withdrawals{
+				{
+					Index: 1,
+				},
+			},
+			block:       &eth.Withdrawals{},
+			shouldMatch: false,
+		},
+		{
+			attrs: &eth.Withdrawals{
+				{
+					Index: 1,
+				},
+			},
+			block: &eth.Withdrawals{
+				{
+					Index: 2,
+				},
+			},
+			shouldMatch: false,
+		},
+	}
+
+	for _, test := range tests {
+		err := checkWithdrawalsMatch(test.attrs, test.block)
+
+		if test.shouldMatch {
+			require.NoError(t, err)
+		} else {
+			require.Error(t, err)
+		}
+	}
+}

op-node/rollup/types.go

@@ -125,6 +125,10 @@ func (cfg *Config) ValidateL2Config(ctx context.Context, client L2Client) error
 	return nil
 }
 
+func (cfg *Config) TimestampForBlock(blockNumber uint64) uint64 {
+	return cfg.Genesis.L2Time + ((blockNumber - cfg.Genesis.L2.Number) * cfg.BlockTime)
+}
+
 func (cfg *Config) TargetBlockNumber(timestamp uint64) (num uint64, err error) {
 	// subtract genesis time from timestamp to get the time elapsed since genesis, and then divide that
 	// difference by the block time to get the expected L2 block number at the current time. If the

op-node/rollup/types_test.go

@@ -396,3 +396,54 @@ func TestConfig_Check(t *testing.T) {
 		})
 	}
 }
+
+func TestTimestampForBlock(t *testing.T) {
+	config := randConfig()
+
+	tests := []struct {
+		name              string
+		genesisTime       uint64
+		genesisBlock      uint64
+		blockTime         uint64
+		blockNum          uint64
+		expectedBlockTime uint64
+	}{
+		{
+			name:              "FirstBlock",
+			genesisTime:       100,
+			genesisBlock:      0,
+			blockTime:         2,
+			blockNum:          0,
+			expectedBlockTime: 100,
+		},
+		{
+			name:              "SecondBlock",
+			genesisTime:       100,
+			genesisBlock:      0,
+			blockTime:         2,
+			blockNum:          1,
+			expectedBlockTime: 102,
+		},
+		{
+			name:              "NBlock",
+			genesisTime:       100,
+			genesisBlock:      0,
+			blockTime:         2,
+			blockNum:          25,
+			expectedBlockTime: 150,
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(fmt.Sprintf("TestTimestampForBlock_%s", test.name), func(t *testing.T) {
+			config.Genesis.L2Time = test.genesisTime
+			config.Genesis.L2.Number = test.genesisBlock
+			config.BlockTime = test.blockTime
+
+			timestamp := config.TimestampForBlock(test.blockNum)
+			assert.Equal(t, timestamp, test.expectedBlockTime)
+		})
+	}
+
+}

op-program/client/l2/engineapi/l2_engine_api.go

@@ -355,7 +355,7 @@ func toGethWithdrawals(payload *eth.ExecutionPayload) []*types.Withdrawal {
 		return nil
 	}
 
-	result := []*types.Withdrawal{}
+	result := make([]*types.Withdrawal, 0, len(*payload.Withdrawals))
 
 	for _, w := range *payload.Withdrawals {
 		result = append(result, &types.Withdrawal{

op-service/eth/ssz.go

@@ -26,7 +26,7 @@ const blockV1FixedPart = 32 + 20 + 32 + 32 + 256 + 32 + 8 + 8 + 8 + 8 + 4 + 32 +
 // V1 + Withdrawals offset
 const blockV2FixedPart = blockV1FixedPart + 4
 
-const withdrawalSize = 8 + 8 + 32 + 8
+const withdrawalSize = 8 + 8 + 20 + 8
 
 // MAX_TRANSACTIONS_PER_PAYLOAD in consensus spec
 // https://github.com/ethereum/consensus-specs/blob/ef434e87165e9a4c82a99f54ffd4974ae113f732/specs/bellatrix/beacon-chain.md#execution
@@ -194,8 +194,8 @@ func marshalWithdrawals(out []byte, withdrawals *Withdrawals) {
 		offset += 8
 		binary.LittleEndian.PutUint64(out[offset:offset+8], withdrawal.Validator)
 		offset += 8
-		copy(out[offset:offset+32], withdrawal.Address[:])
-		offset += 32
+		copy(out[offset:offset+20], withdrawal.Address[:])
+		offset += 20
 		binary.LittleEndian.PutUint64(out[offset:offset+8], withdrawal.Amount)
 		offset += 8
 	}
@@ -335,8 +335,8 @@ func unmarshalWithdrawals(in []byte) (*Withdrawals, error) {
 		withdrawal.Validator = binary.LittleEndian.Uint64(in[offset : offset+8])
 		offset += 8
 
-		copy(withdrawal.Address[:], in[offset:offset+32])
-		offset += 32
+		copy(withdrawal.Address[:], in[offset:offset+20])
+		offset += 20
 
 		withdrawal.Amount = binary.LittleEndian.Uint64(in[offset : offset+8])
 		offset += 8