From 965c2378f60758d3f8e7a86d4b4b57c9b07af2a2 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 25 Jan 2023 15:30:27 +0100 Subject: [PATCH 1/6] eth/catalyst: add error --- eth/catalyst/api.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 39dcba04f83e..14f4740c82ce 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -164,6 +164,9 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa // ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload attributes. func (api *ConsensusAPI) ForkchoiceUpdatedV2(update beacon.ForkchoiceStateV1, payloadAttributes *beacon.PayloadAttributes) (beacon.ForkChoiceResponse, error) { + if payloadAttributes != nil && payloadAttributes.Withdrawals == nil { + return beacon.STATUS_INVALID, fmt.Errorf("withdrawals have to be set to nil slice") + } return api.forkchoiceUpdated(update, payloadAttributes) } From 04ebabfcc04d1f0e0c8069411bbb1bcba0cca85d Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 25 Jan 2023 15:38:50 +0100 Subject: [PATCH 2/6] eth/catalyst: better msg --- eth/catalyst/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 14f4740c82ce..23b62e3b880a 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -165,7 +165,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa // ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload attributes. func (api *ConsensusAPI) ForkchoiceUpdatedV2(update beacon.ForkchoiceStateV1, payloadAttributes *beacon.PayloadAttributes) (beacon.ForkChoiceResponse, error) { if payloadAttributes != nil && payloadAttributes.Withdrawals == nil { - return beacon.STATUS_INVALID, fmt.Errorf("withdrawals have to be set to nil slice") + return beacon.STATUS_INVALID, fmt.Errorf("withdrawals have to be set to empty slice, not nil") } return api.forkchoiceUpdated(update, payloadAttributes) } From eedc56e8bf7d245540e86ab5e3d083c4e134af25 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 25 Jan 2023 21:40:13 +0100 Subject: [PATCH 3/6] Update eth/catalyst/api.go Co-authored-by: Felix Lange --- eth/catalyst/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 23b62e3b880a..d90a4c1adb48 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -165,7 +165,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa // ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload attributes. func (api *ConsensusAPI) ForkchoiceUpdatedV2(update beacon.ForkchoiceStateV1, payloadAttributes *beacon.PayloadAttributes) (beacon.ForkChoiceResponse, error) { if payloadAttributes != nil && payloadAttributes.Withdrawals == nil { - return beacon.STATUS_INVALID, fmt.Errorf("withdrawals have to be set to empty slice, not nil") + return beacon.STATUS_INVALID, errors.New("missing withdrawals list") } return api.forkchoiceUpdated(update, payloadAttributes) } From ed813c6cdc25522e3f64ec2bea2e2eefc294207d Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Thu, 26 Jan 2023 09:55:50 +0100 Subject: [PATCH 4/6] eth/catalyst: only error post-shanghai, add test --- eth/catalyst/api.go | 5 ++- eth/catalyst/api_test.go | 84 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index d90a4c1adb48..e4dcd735cbd2 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -165,7 +165,10 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa // ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload attributes. func (api *ConsensusAPI) ForkchoiceUpdatedV2(update beacon.ForkchoiceStateV1, payloadAttributes *beacon.PayloadAttributes) (beacon.ForkChoiceResponse, error) { if payloadAttributes != nil && payloadAttributes.Withdrawals == nil { - return beacon.STATUS_INVALID, errors.New("missing withdrawals list") + // Reject payload attributes with nil withdrawals after shanghai + if api.eth.BlockChain().Config().IsShanghai(payloadAttributes.Timestamp) { + return beacon.STATUS_INVALID, errors.New("missing withdrawals list") + } } return api.forkchoiceUpdated(update, payloadAttributes) } diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 8df48bd08e10..bd82933a07cb 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -1113,3 +1113,87 @@ func TestWithdrawals(t *testing.T) { } } } + +func TestNilWithdrawals(t *testing.T) { + genesis, blocks := generateMergeChain(10, true) + // Set shanghai time to last block + 4 seconds (first post-merge block) + time := blocks[len(blocks)-1].Time() + 4 + genesis.Config.ShanghaiTime = &time + + n, ethservice := startEthService(t, genesis, blocks) + ethservice.Merger().ReachTTD() + defer n.Close() + + api := NewConsensusAPI(ethservice) + parent := ethservice.BlockChain().CurrentHeader() + aa := common.Address{0xaa} + + type test struct { + blockParams beacon.PayloadAttributes + wantErr bool + } + tests := []test{ + { + blockParams: beacon.PayloadAttributes{ + Timestamp: parent.Time + 5, + Withdrawals: nil, + }, + wantErr: true, + }, + { + blockParams: beacon.PayloadAttributes{ + Timestamp: parent.Time + 5, + Withdrawals: make([]*types.Withdrawal, 0), + }, + wantErr: false, + }, + { + blockParams: beacon.PayloadAttributes{ + Timestamp: parent.Time + 5, + Withdrawals: []*types.Withdrawal{ + { + Index: 0, + Address: aa, + Amount: 32, + }, + }, + }, + wantErr: false, + }, + } + + fcState := beacon.ForkchoiceStateV1{ + HeadBlockHash: parent.Hash(), + } + + for _, test := range tests { + _, err := api.ForkchoiceUpdatedV2(fcState, &test.blockParams) + if test.wantErr { + if err == nil { + t.Fatal("wanted error on fcuv2 with invalid withdrawals") + } + continue + } + if err != nil { + t.Fatalf("error preparing payload, err=%v", err) + } + + // 11: verify locally build block. + payloadID := (&miner.BuildPayloadArgs{ + Parent: fcState.HeadBlockHash, + Timestamp: test.blockParams.Timestamp, + FeeRecipient: test.blockParams.SuggestedFeeRecipient, + Random: test.blockParams.Random, + }).Id() + execData, err := api.GetPayloadV2(payloadID) + if err != nil { + t.Fatalf("error getting payload, err=%v", err) + } + if status, err := api.NewPayloadV2(*execData.ExecutionPayload); err != nil { + t.Fatalf("error validating payload: %v", err) + } else if status.Status != beacon.VALID { + t.Fatalf("invalid payload") + } + } + +} From 0a01c8ccc3ea75ab8aaea8bebc117f3f6d4317e8 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Thu, 26 Jan 2023 10:28:39 +0100 Subject: [PATCH 5/6] eth/catalyst: also error on withdrawals before shanghai --- eth/catalyst/api.go | 12 +++++++++--- eth/catalyst/api_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index e4dcd735cbd2..3b931abffe45 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -164,12 +164,18 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa // ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload attributes. func (api *ConsensusAPI) ForkchoiceUpdatedV2(update beacon.ForkchoiceStateV1, payloadAttributes *beacon.PayloadAttributes) (beacon.ForkChoiceResponse, error) { - if payloadAttributes != nil && payloadAttributes.Withdrawals == nil { + if !api.eth.BlockChain().Config().IsShanghai(payloadAttributes.Timestamp) { + // Reject payload attributes with withdrawals before shanghai + if payloadAttributes != nil && payloadAttributes.Withdrawals != nil { + return beacon.STATUS_INVALID, beacon.InvalidPayloadAttributes.With(errors.New("withdrawals before shanghai")) + } + } else { // Reject payload attributes with nil withdrawals after shanghai - if api.eth.BlockChain().Config().IsShanghai(payloadAttributes.Timestamp) { - return beacon.STATUS_INVALID, errors.New("missing withdrawals list") + if payloadAttributes != nil && payloadAttributes.Withdrawals == nil { + return beacon.STATUS_INVALID, beacon.InvalidPayloadAttributes.With(errors.New("missing withdrawals list")) } } + return api.forkchoiceUpdated(update, payloadAttributes) } diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index bd82933a07cb..e23a609a924b 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -1133,6 +1133,35 @@ func TestNilWithdrawals(t *testing.T) { wantErr bool } tests := []test{ + // Before Shanghai + { + blockParams: beacon.PayloadAttributes{ + Timestamp: parent.Time + 2, + Withdrawals: nil, + }, + wantErr: false, + }, + { + blockParams: beacon.PayloadAttributes{ + Timestamp: parent.Time + 2, + Withdrawals: make([]*types.Withdrawal, 0), + }, + wantErr: true, + }, + { + blockParams: beacon.PayloadAttributes{ + Timestamp: parent.Time + 2, + Withdrawals: []*types.Withdrawal{ + { + Index: 0, + Address: aa, + Amount: 32, + }, + }, + }, + wantErr: true, + }, + // After Shanghai { blockParams: beacon.PayloadAttributes{ Timestamp: parent.Time + 5, From 9f514f2d0e379eb536e1440d1a226812f7474a47 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Fri, 27 Jan 2023 11:28:40 +0100 Subject: [PATCH 6/6] eth/catalyst: remove newline --- eth/catalyst/api_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index e23a609a924b..d9280e99d673 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -1224,5 +1224,4 @@ func TestNilWithdrawals(t *testing.T) { t.Fatalf("invalid payload") } } - }