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

Proposer compare withdrawal roots #11977

Merged
merged 16 commits into from
Feb 17, 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
2 changes: 1 addition & 1 deletion beacon-chain/rpc/eth/validator/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2727,7 +2727,7 @@ func TestProduceBlindedBlock(t *testing.T) {
}
id := &enginev1.PayloadIDBytes{0x1}
v1Alpha1Server := &v1alpha1validator.Server{
ExecutionEngineCaller: &mockExecution.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &enginev1.ExecutionPayloadCapella{BlockNumber: 1}, BlockValue: big.NewInt(0)},
ExecutionEngineCaller: &mockExecution.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &enginev1.ExecutionPayloadCapella{BlockNumber: 1, Withdrawals: wds}, BlockValue: big.NewInt(0)},
BeaconDB: db,
ForkFetcher: &mockChain.ChainService{ForkChoiceStore: doublylinkedtree.New()},
TimeFetcher: &mockChain.ChainService{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ import (
"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/blocks"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/signing"
"github.com/prysmaticlabs/prysm/v3/beacon-chain/state"
fieldparams "github.com/prysmaticlabs/prysm/v3/config/fieldparams"
"github.com/prysmaticlabs/prysm/v3/config/params"
consensusblocks "github.com/prysmaticlabs/prysm/v3/consensus-types/blocks"
"github.com/prysmaticlabs/prysm/v3/consensus-types/interfaces"
"github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v3/crypto/hash"
"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
"github.com/prysmaticlabs/prysm/v3/encoding/ssz"
enginev1 "github.com/prysmaticlabs/prysm/v3/proto/engine/v1"
Expand Down Expand Up @@ -68,8 +70,13 @@ func (vs *Server) setExecutionData(ctx context.Context, blk interfaces.SignedBea
if err != nil {
log.WithError(err).Warn("Proposer: failed to get builder payload value") // Default to local if can't get builder value.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative is to add here an } else and prevent continuing if we already know that we are not going to use the builder cause the withdrawals root doesn't match


withdrawalsMatched, err := matchingWithdrawalsRoot(localPayload, builderPayload)
if err != nil {
return errors.Wrap(err, "failed to match withdrawals root")
}
// If we can't get the builder value, just use local block.
if err == nil && builderValue.Cmp(localValue) > 0 { // Builder value is higher
if builderValue.Cmp(localValue) > 0 && withdrawalsMatched { // Builder value is higher and withdrawals match.
blk.SetBlinded(true)
if err := blk.SetExecution(builderPayload); err != nil {
log.WithError(err).Warn("Proposer: failed to set builder payload")
Expand All @@ -87,6 +94,7 @@ func (vs *Server) setExecutionData(ctx context.Context, blk interfaces.SignedBea
}
}
}

}

executionData, err := vs.getExecutionPayload(ctx, slot, idx, blk.Block().ParentRoot(), headState)
Expand Down Expand Up @@ -322,3 +330,27 @@ func validateBuilderSignature(signedBid builder.SignedBid) error {
}
return signing.VerifySigningRoot(bid, bid.Pubkey(), signedBid.Signature(), d)
}

func matchingWithdrawalsRoot(local, builder interfaces.ExecutionData) (bool, error) {
wds, err := local.Withdrawals()
if err != nil {
return false, errors.Wrap(err, "could not get local withdrawals")
}
br, err := builder.WithdrawalsRoot()
if err != nil {
return false, errors.Wrap(err, "could not get builder withdrawals root")
}
wr, err := ssz.WithdrawalSliceRoot(hash.CustomSHA256Hasher(), wds, fieldparams.MaxWithdrawalsPerPayload)
if err != nil {
return false, errors.Wrap(err, "could not compute local withdrawals root")
}

if !bytes.Equal(br, wr[:]) {
log.WithFields(logrus.Fields{
"local": fmt.Sprintf("%#x", wr),
"builder": fmt.Sprintf("%#x", br),
}).Warn("Proposer: withdrawal roots don't match, using local block")
return false, nil
}
return true, nil
}
118 changes: 114 additions & 4 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/prysmaticlabs/prysm/v3/consensus-types/interfaces"
"github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v3/crypto/bls"
"github.com/prysmaticlabs/prysm/v3/crypto/hash"
"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
"github.com/prysmaticlabs/prysm/v3/encoding/ssz"
v1 "github.com/prysmaticlabs/prysm/v3/proto/engine/v1"
Expand Down Expand Up @@ -53,10 +54,15 @@ func TestServer_setExecutionData(t *testing.T) {
}))
require.NoError(t, beaconDB.SaveFeeRecipientsByValidatorIDs(context.Background(), []primitives.ValidatorIndex{0}, []common.Address{{}}))

require.NoError(t, err)
withdrawals := []*v1.Withdrawal{{
Index: 1,
ValidatorIndex: 2,
Address: make([]byte, fieldparams.FeeRecipientLength),
Amount: 3,
}}
id := &v1.PayloadIDBytes{0x1}
vs := &Server{
ExecutionEngineCaller: &powtesting.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &v1.ExecutionPayloadCapella{BlockNumber: 1}, BlockValue: big.NewInt(0)},
ExecutionEngineCaller: &powtesting.EngineClient{PayloadIDBytes: id, ExecutionPayloadCapella: &v1.ExecutionPayloadCapella{BlockNumber: 1, Withdrawals: withdrawals}, BlockValue: big.NewInt(0)},
HeadFetcher: &blockchainTest.ChainService{State: capellaTransitionState},
BeaconDB: beaconDB,
ProposerSlotIndexCache: cache.NewProposerPayloadIDsCache(),
Expand All @@ -70,8 +76,8 @@ func TestServer_setExecutionData(t *testing.T) {
require.NoError(t, err)
require.Equal(t, uint64(1), e.BlockNumber()) // Local block
})
t.Run("Builder configured. Builder Block has higher value", func(t *testing.T) {
blk, err := blocks.NewSignedBeaconBlock(util.NewBlindedBeaconBlockCapella())
t.Run("Builder configured. Builder Block has higher value. Incorrect withdrawals", func(t *testing.T) {
blk, err := blocks.NewSignedBeaconBlock(util.NewBeaconBlockCapella())
require.NoError(t, err)
require.NoError(t, vs.BeaconDB.SaveRegistrationsByValidatorIDs(ctx, []primitives.ValidatorIndex{blk.Block().ProposerIndex()},
[]*ethpb.ValidatorRegistrationV1{{FeeRecipient: make([]byte, fieldparams.FeeRecipientLength), Pubkey: make([]byte, fieldparams.BLSPubkeyLength)}}))
Expand Down Expand Up @@ -119,6 +125,59 @@ func TestServer_setExecutionData(t *testing.T) {
require.NoError(t, vs.setExecutionData(context.Background(), blk, capellaTransitionState))
e, err := blk.Block().Body().Execution()
require.NoError(t, err)
require.Equal(t, uint64(1), e.BlockNumber()) // Local block because incorrect withdrawals
})
t.Run("Builder configured. Builder Block has higher value. Correct withdrawals.", func(t *testing.T) {
blk, err := blocks.NewSignedBeaconBlock(util.NewBlindedBeaconBlockCapella())
require.NoError(t, err)
require.NoError(t, vs.BeaconDB.SaveRegistrationsByValidatorIDs(ctx, []primitives.ValidatorIndex{blk.Block().ProposerIndex()},
[]*ethpb.ValidatorRegistrationV1{{FeeRecipient: make([]byte, fieldparams.FeeRecipientLength), Pubkey: make([]byte, fieldparams.BLSPubkeyLength)}}))
ti, err := slots.ToTime(uint64(time.Now().Unix()), 0)
require.NoError(t, err)
sk, err := bls.RandKey()
require.NoError(t, err)
wr, err := ssz.WithdrawalSliceRoot(hash.CustomSHA256Hasher(), withdrawals, fieldparams.MaxWithdrawalsPerPayload)
require.NoError(t, err)
bid := &ethpb.BuilderBidCapella{
Header: &v1.ExecutionPayloadHeaderCapella{
FeeRecipient: make([]byte, fieldparams.FeeRecipientLength),
StateRoot: make([]byte, fieldparams.RootLength),
ReceiptsRoot: make([]byte, fieldparams.RootLength),
LogsBloom: make([]byte, fieldparams.LogsBloomLength),
PrevRandao: make([]byte, fieldparams.RootLength),
BaseFeePerGas: make([]byte, fieldparams.RootLength),
BlockHash: make([]byte, fieldparams.RootLength),
TransactionsRoot: bytesutil.PadTo([]byte{1}, fieldparams.RootLength),
ParentHash: params.BeaconConfig().ZeroHash[:],
Timestamp: uint64(ti.Unix()),
BlockNumber: 2,
WithdrawalsRoot: wr[:],
},
Pubkey: sk.PublicKey().Marshal(),
Value: bytesutil.PadTo([]byte{1}, 32),
}
d := params.BeaconConfig().DomainApplicationBuilder
domain, err := signing.ComputeDomain(d, nil, nil)
require.NoError(t, err)
sr, err := signing.ComputeSigningRoot(bid, domain)
require.NoError(t, err)
sBid := &ethpb.SignedBuilderBidCapella{
Message: bid,
Signature: sk.Sign(sr[:]).Marshal(),
}
vs.BlockBuilder = &builderTest.MockBuilderService{
BidCapella: sBid,
}
wb, err := blocks.NewSignedBeaconBlock(util.NewBeaconBlockBellatrix())
require.NoError(t, err)
chain := &blockchainTest.ChainService{ForkChoiceStore: doublylinkedtree.New(), Genesis: time.Now(), Block: wb}
vs.ForkFetcher = chain
vs.ForkFetcher.ForkChoicer().SetGenesisTime(uint64(time.Now().Unix()))
vs.TimeFetcher = chain
vs.HeadFetcher = chain
require.NoError(t, vs.setExecutionData(context.Background(), blk, capellaTransitionState))
e, err := blk.Block().Body().Execution()
require.NoError(t, err)
require.Equal(t, uint64(2), e.BlockNumber()) // Builder block
})
t.Run("Builder configured. Local block has higher value", func(t *testing.T) {
Expand Down Expand Up @@ -467,3 +526,54 @@ func TestServer_validateBuilderSignature(t *testing.T) {
require.NoError(t, err)
require.ErrorIs(t, validateBuilderSignature(sBid), signing.ErrSigFailedToVerify)
}

func Test_matchingWithdrawalsRoot(t *testing.T) {
t.Run("could not get local withdrawals", func(t *testing.T) {
local := &v1.ExecutionPayload{}
p, err := blocks.WrappedExecutionPayload(local)
require.NoError(t, err)
_, err = matchingWithdrawalsRoot(p, p)
require.ErrorContains(t, "could not get local withdrawals", err)
})
t.Run("could not get builder withdrawals root", func(t *testing.T) {
local := &v1.ExecutionPayloadCapella{}
p, err := blocks.WrappedExecutionPayloadCapella(local, big.NewInt(0))
require.NoError(t, err)
header := &v1.ExecutionPayloadHeader{}
h, err := blocks.WrappedExecutionPayloadHeader(header)
require.NoError(t, err)
_, err = matchingWithdrawalsRoot(p, h)
require.ErrorContains(t, "could not get builder withdrawals root", err)
})
t.Run("withdrawals mismatch", func(t *testing.T) {
local := &v1.ExecutionPayloadCapella{}
p, err := blocks.WrappedExecutionPayloadCapella(local, big.NewInt(0))
require.NoError(t, err)
header := &v1.ExecutionPayloadHeaderCapella{}
h, err := blocks.WrappedExecutionPayloadHeaderCapella(header, big.NewInt(0))
require.NoError(t, err)
matched, err := matchingWithdrawalsRoot(p, h)
require.NoError(t, err)
require.Equal(t, false, matched)
})
t.Run("withdrawals match", func(t *testing.T) {
wds := []*v1.Withdrawal{{
Index: 1,
ValidatorIndex: 2,
Address: make([]byte, fieldparams.FeeRecipientLength),
Amount: 3,
}}
local := &v1.ExecutionPayloadCapella{Withdrawals: wds}
p, err := blocks.WrappedExecutionPayloadCapella(local, big.NewInt(0))
require.NoError(t, err)
header := &v1.ExecutionPayloadHeaderCapella{}
wr, err := ssz.WithdrawalSliceRoot(hash.CustomSHA256Hasher(), wds, fieldparams.MaxWithdrawalsPerPayload)
require.NoError(t, err)
header.WithdrawalsRoot = wr[:]
h, err := blocks.WrappedExecutionPayloadHeaderCapella(header, big.NewInt(0))
require.NoError(t, err)
matched, err := matchingWithdrawalsRoot(p, h)
require.NoError(t, err)
require.Equal(t, true, matched)
})
}