From d488308db18e60cf150c14bfcad7102206ecb007 Mon Sep 17 00:00:00 2001 From: Vladyslav Lupashevskyi Date: Thu, 9 Jun 2022 02:32:56 +0200 Subject: [PATCH 1/5] Change extra data decoder to support IBFT2 block validation --- consensus/istanbul/qbft/engine/engine.go | 9 +- core/types/block.go | 3 + core/types/istanbul.go | 101 +++++++++++++++++++++-- rlp/decode.go | 13 +-- 4 files changed, 109 insertions(+), 17 deletions(-) diff --git a/consensus/istanbul/qbft/engine/engine.go b/consensus/istanbul/qbft/engine/engine.go index 3b59e6984f..6a2e464e92 100644 --- a/consensus/istanbul/qbft/engine/engine.go +++ b/consensus/istanbul/qbft/engine/engine.go @@ -2,6 +2,7 @@ package qbftengine import ( "bytes" + "encoding/binary" "fmt" "math/big" "strings" @@ -79,7 +80,9 @@ func writeCommittedSeals(committedSeals [][]byte) ApplyQBFTExtra { // writeRoundNumber writes the extra-data field of a block header with given round. func writeRoundNumber(round *big.Int) ApplyQBFTExtra { return func(qbftExtra *types.QBFTExtra) error { - qbftExtra.Round = uint32(round.Uint64()) + a := make([]byte, 4) + binary.BigEndian.PutUint32(a, uint32(round.Uint64())) + qbftExtra.Round = a return nil } } @@ -421,7 +424,7 @@ func (e *Engine) Signers(header *types.Header) ([]common.Address, error) { return []common.Address{}, err } committedSeal := extra.CommittedSeal - proposalSeal := PrepareCommittedSeal(header, extra.Round) + proposalSeal := PrepareCommittedSeal(header, binary.BigEndian.Uint32(extra.Round)) var addrs []common.Address // 1. Get committed seals from current header @@ -516,7 +519,7 @@ func getExtra(header *types.Header) (*types.QBFTExtra, error) { VanityData: vanity, Validators: []common.Address{}, CommittedSeal: [][]byte{}, - Round: 0, + Round: make([]byte, 4), Vote: nil, }, nil } diff --git a/core/types/block.go b/core/types/block.go index d613cfc57e..42f7c30a24 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -100,6 +100,9 @@ type headerMarshaling struct { func (h *Header) Hash() common.Hash { // If the mix digest is equivalent to the predefined Istanbul digest, use Istanbul // specific hash calculation. + if h.Number.Uint64() == 0 { + return rlpHash(h) + } if h.MixDigest == IstanbulDigest { // Seal is reserved in extra-data. To prove block is signed by the proposer. if istanbulHeader := FilteredHeader(h); istanbulHeader != nil { diff --git a/core/types/istanbul.go b/core/types/istanbul.go index 565100d954..5350194f34 100644 --- a/core/types/istanbul.go +++ b/core/types/istanbul.go @@ -17,6 +17,7 @@ package types import ( + "encoding/binary" "errors" "io" @@ -127,11 +128,24 @@ func IstanbulFilteredHeader(h *Header, keepSeal bool) *Header { type QBFTExtra struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote - Round uint32 + Vote *ValidatorVote `rlp:"nilString"` + Round []byte CommittedSeal [][]byte } +type QBFTExtraNoSealsNoRound struct { + VanityData []byte + Validators []common.Address + Vote *ValidatorVote `rlp:"nilString"` +} + +type QBFTExtraNoSeals struct { + VanityData []byte + Validators []common.Address + Vote *ValidatorVote `rlp:"nilString"` + Round []byte +} + type ValidatorVote struct { RecipientAddress common.Address VoteType byte @@ -139,6 +153,15 @@ type ValidatorVote struct { // EncodeRLP serializes qist into the Ethereum RLP format. func (qst *QBFTExtra) EncodeRLP(w io.Writer) error { + if qst.Vote == nil { + return rlp.Encode(w, []interface{}{ + qst.VanityData, + qst.Validators, + make([]byte, 0), + qst.Round, + qst.CommittedSeal, + }) + } return rlp.Encode(w, []interface{}{ qst.VanityData, qst.Validators, @@ -148,13 +171,45 @@ func (qst *QBFTExtra) EncodeRLP(w io.Writer) error { }) } +func (qst *QBFTExtraNoSeals) EncodeRLP(w io.Writer) error { + if qst.Vote == nil { + return rlp.Encode(w, []interface{}{ + qst.VanityData, + qst.Validators, + make([]byte, 0), + qst.Round, + }) + } + return rlp.Encode(w, []interface{}{ + qst.VanityData, + qst.Validators, + qst.Vote, + qst.Round, + }) +} + +func (qst *QBFTExtraNoSealsNoRound) EncodeRLP(w io.Writer) error { + if qst.Vote == nil { + return rlp.Encode(w, []interface{}{ + qst.VanityData, + qst.Validators, + make([]byte, 0), + }) + } + return rlp.Encode(w, []interface{}{ + qst.VanityData, + qst.Validators, + qst.Vote, + }) +} + // DecodeRLP implements rlp.Decoder, and load the QBFTExtra fields from a RLP stream. func (qst *QBFTExtra) DecodeRLP(s *rlp.Stream) error { var qbftExtra struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote `rlp:"nil"` - Round uint32 + Vote *ValidatorVote `rlp:"nilString"` + Round []byte CommittedSeal [][]byte } if err := s.Decode(&qbftExtra); err != nil { @@ -167,6 +222,11 @@ func (qst *QBFTExtra) DecodeRLP(s *rlp.Stream) error { // EncodeRLP serializes ValidatorVote into the Ethereum RLP format. func (vv *ValidatorVote) EncodeRLP(w io.Writer) error { + if vv.VoteType == 0 { + // It's easier to process this edge case in such a way in order not to introduce extra logic to RLP module for only this case + _, err := w.Write(append(append([]byte{0xd6, 0x94}, vv.RecipientAddress.Bytes()...), vv.VoteType)) + return err + } return rlp.Encode(w, []interface{}{ vv.RecipientAddress, vv.VoteType, @@ -179,9 +239,11 @@ func (vv *ValidatorVote) DecodeRLP(s *rlp.Stream) error { RecipientAddress common.Address VoteType byte } + s.IgnoreCanonIntForByte = true if err := s.Decode(&validatorVote); err != nil { return err } + s.IgnoreCanonIntForByte = false vv.RecipientAddress, vv.VoteType = validatorVote.RecipientAddress, validatorVote.VoteType return nil } @@ -202,7 +264,24 @@ func ExtractQBFTExtra(h *Header) (*QBFTExtra, error) { // are clean to fulfill the Istanbul hash rules. It returns nil if the extra-data cannot be // decoded/encoded by rlp. func QBFTFilteredHeader(h *Header) *Header { - return QBFTFilteredHeaderWithRound(h, 0) + newHeader := CopyHeader(h) + qbftExtra, err := ExtractQBFTExtra(newHeader) + if err != nil { + return nil + } + + qbftFiltered := new(QBFTExtraNoSealsNoRound) + + qbftFiltered.Validators, qbftFiltered.VanityData, qbftFiltered.Vote = qbftExtra.Validators, qbftExtra.VanityData, qbftExtra.Vote + + payload, err := rlp.EncodeToBytes(&qbftFiltered) + if err != nil { + return nil + } + + newHeader.Extra = payload + + return newHeader } // QBFTFilteredHeaderWithRound returns the copy of the header with round number set to the given round number @@ -214,10 +293,16 @@ func QBFTFilteredHeaderWithRound(h *Header, round uint32) *Header { return nil } - qbftExtra.CommittedSeal = [][]byte{} - qbftExtra.Round = round + qbftFiltered := new(QBFTExtraNoSeals) + + qbftFiltered.Validators, qbftFiltered.VanityData, qbftFiltered.Vote = qbftExtra.Validators, qbftExtra.VanityData, qbftExtra.Vote + + a := make([]byte, 4) + binary.BigEndian.PutUint32(a, round) + + qbftFiltered.Round = a - payload, err := rlp.EncodeToBytes(&qbftExtra) + payload, err := rlp.EncodeToBytes(&qbftFiltered) if err != nil { return nil } diff --git a/rlp/decode.go b/rlp/decode.go index 79b7ef0626..bab6dabd8d 100644 --- a/rlp/decode.go +++ b/rlp/decode.go @@ -554,11 +554,12 @@ type Stream struct { // auxiliary buffer for integer decoding uintbuf []byte - kind Kind // kind of value ahead - size uint64 // size of value ahead - byteval byte // value of single byte in type tag - kinderr error // error from last readKind - stack []listpos + kind Kind // kind of value ahead + size uint64 // size of value ahead + byteval byte // value of single byte in type tag + kinderr error // error from last readKind + stack []listpos + IgnoreCanonIntForByte bool } type listpos struct{ pos, size uint64 } @@ -661,7 +662,7 @@ func (s *Stream) uint(maxbits int) (uint64, error) { } switch kind { case Byte: - if s.byteval == 0 { + if !s.IgnoreCanonIntForByte && s.byteval == 0 { return 0, ErrCanonInt } s.kind = -1 // rearm Kind From 5a02b8da48756a640b1737e215a759654be8411f Mon Sep 17 00:00:00 2001 From: baptiste-b-pegasys <85155432+baptiste-b-pegasys@users.noreply.github.com> Date: Mon, 3 Oct 2022 19:10:01 +0200 Subject: [PATCH 2/5] fixes --- consensus/istanbul/qbft/engine/engine.go | 2 +- consensus/istanbul/qbft/engine/engine_test.go | 22 ++++++++++--------- consensus/istanbul/testutils/genesis.go | 2 +- core/types/istanbul.go | 2 +- core/types/istanbul_test.go | 2 +- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/consensus/istanbul/qbft/engine/engine.go b/consensus/istanbul/qbft/engine/engine.go index 6a2e464e92..7cf2433eaf 100644 --- a/consensus/istanbul/qbft/engine/engine.go +++ b/consensus/istanbul/qbft/engine/engine.go @@ -519,7 +519,7 @@ func getExtra(header *types.Header) (*types.QBFTExtra, error) { VanityData: vanity, Validators: []common.Address{}, CommittedSeal: [][]byte{}, - Round: make([]byte, 4), + Round: make([]byte, 0), Vote: nil, }, nil } diff --git a/consensus/istanbul/qbft/engine/engine_test.go b/consensus/istanbul/qbft/engine/engine_test.go index 6ee803c4a2..5bfe92334b 100644 --- a/consensus/istanbul/qbft/engine/engine_test.go +++ b/consensus/istanbul/qbft/engine/engine_test.go @@ -2,6 +2,7 @@ package qbftengine import ( "bytes" + "encoding/binary" "fmt" "math/big" "reflect" @@ -27,7 +28,7 @@ func TestPrepareExtra(t *testing.T) { validators[2] = common.BytesToAddress(hexutil.MustDecode("0x6beaaed781d2d2ab6350f5c4566a2c6eaac407a6")) validators[3] = common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")) - expectedResult := hexutil.MustDecode("0xf87aa00000000000000000000000000000000000000000000000000000000000000000f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c080c0") + expectedResult := "0xf87aa00000000000000000000000000000000000000000000000000000000000000000f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408080c0" h := &types.Header{} err := ApplyHeaderQBFTExtra( @@ -37,13 +38,12 @@ func TestPrepareExtra(t *testing.T) { if err != nil { t.Errorf("error mismatch: have %v, want: nil", err) } - if !reflect.DeepEqual(h.Extra, expectedResult) { - t.Errorf("payload mismatch: have %v, want %v", h.Extra, expectedResult) - } + result := hexutil.Encode(h.Extra) + assert.Equal(t, expectedResult, result) } func TestWriteCommittedSeals(t *testing.T) { - istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c080c0") + istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408080c0") expectedCommittedSeal := append([]byte{1, 2, 3}, bytes.Repeat([]byte{0x00}, types.IstanbulExtraSeal-3)...) expectedIstExtra := &types.QBFTExtra{ VanityData: []byte{}, @@ -54,7 +54,7 @@ func TestWriteCommittedSeals(t *testing.T) { common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), }, CommittedSeal: [][]byte{expectedCommittedSeal}, - Round: 0, + Round: make([]byte, 0), Vote: nil, } @@ -92,7 +92,9 @@ func TestWriteCommittedSeals(t *testing.T) { } func TestWriteRoundNumber(t *testing.T) { - istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c005c0") + istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408005c0") + round := make([]byte, 4) + binary.BigEndian.PutUint32(round, 5) expectedIstExtra := &types.QBFTExtra{ VanityData: []byte{}, Validators: []common.Address{ @@ -102,7 +104,7 @@ func TestWriteRoundNumber(t *testing.T) { common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), }, CommittedSeal: [][]byte{}, - Round: 5, + Round: round, Vote: nil, } @@ -133,13 +135,13 @@ func TestWriteRoundNumber(t *testing.T) { func TestWriteValidatorVote(t *testing.T) { vanity := bytes.Repeat([]byte{0x00}, types.IstanbulExtraVanity) - istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c005c0") + istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408005c0") vote := &types.ValidatorVote{RecipientAddress: common.BytesToAddress(hexutil.MustDecode("0x44add0ec310f115a0e603b2d7db9f06777123456")), VoteType: types.QBFTAuthVote} expectedIstExtra := &types.QBFTExtra{ VanityData: vanity, Validators: []common.Address{}, CommittedSeal: [][]byte{}, - Round: 0, + Round: make([]byte, 0), Vote: vote, } diff --git a/consensus/istanbul/testutils/genesis.go b/consensus/istanbul/testutils/genesis.go index ae8ff962cc..4e78a5b22b 100644 --- a/consensus/istanbul/testutils/genesis.go +++ b/consensus/istanbul/testutils/genesis.go @@ -74,7 +74,7 @@ func appendValidators(genesis *core.Genesis, addrs []common.Address) { Validators: addrs, Vote: nil, CommittedSeal: [][]byte{}, - Round: 0, + Round: make([]byte, 0), } istPayload, err := rlp.EncodeToBytes(&ist) diff --git a/core/types/istanbul.go b/core/types/istanbul.go index 5350194f34..1b886805f5 100644 --- a/core/types/istanbul.go +++ b/core/types/istanbul.go @@ -88,7 +88,7 @@ func ExtractIstanbulExtra(h *Header) (*IstanbulExtra, error) { // FilteredHeader returns a filtered header which some information (like seal, committed seals) // are clean to fulfill the Istanbul hash rules. It first check if the extradata can be extracted into IstanbulExtra if that fails, -//it extracts extradata into QBFTExtra struct +// it extracts extradata into QBFTExtra struct func FilteredHeader(h *Header) *Header { // Check if the header extradata can be decoded in IstanbulExtra, if yes, then call IstanbulFilteredHeader() // if not then call QBFTFilteredHeader() diff --git a/core/types/istanbul_test.go b/core/types/istanbul_test.go index fbd116f0a9..755c5850b4 100644 --- a/core/types/istanbul_test.go +++ b/core/types/istanbul_test.go @@ -62,7 +62,7 @@ func TestExtractToQBFTExtra(t *testing.T) { common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), }, CommittedSeal: [][]byte{}, - Round: 0, + Round: make([]byte, 0), Vote: nil, }, nil, From e13c616044cef6a0234e3047435f7d33edcbfd45 Mon Sep 17 00:00:00 2001 From: Baptiste Boussemart Date: Wed, 12 Oct 2022 15:26:21 +0200 Subject: [PATCH 3/5] fix extra decoding --- core/types/istanbul.go | 38 +++++++++++++++++++++++++++++++++---- core/types/istanbul_test.go | 18 ++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/core/types/istanbul.go b/core/types/istanbul.go index 1b886805f5..70da1e23eb 100644 --- a/core/types/istanbul.go +++ b/core/types/istanbul.go @@ -22,6 +22,7 @@ import ( "io" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/rlp" ) @@ -128,11 +129,19 @@ func IstanbulFilteredHeader(h *Header, keepSeal bool) *Header { type QBFTExtra struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote `rlp:"nilString"` + Vote *ValidatorVote `rlp:"nil"` Round []byte CommittedSeal [][]byte } +type qbftExtraFallback struct { + VanityData []byte + Validators []common.Address + Vote *ValidatorVote `rlp:"nil"` + Round uint32 + CommittedSeal [][]byte +} + type QBFTExtraNoSealsNoRound struct { VanityData []byte Validators []common.Address @@ -208,12 +217,25 @@ func (qst *QBFTExtra) DecodeRLP(s *rlp.Stream) error { var qbftExtra struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote `rlp:"nilString"` + Vote *ValidatorVote `rlp:"nil"` Round []byte CommittedSeal [][]byte } if err := s.Decode(&qbftExtra); err != nil { - return err + var qbftExtraFallback struct { + VanityData []byte + Validators []common.Address + Vote *ValidatorVote `rlp:"nil"` + Round uint32 + CommittedSeal [][]byte + } + if err := s.Decode(&qbftExtraFallback); err != nil { + return err + } + round := make([]byte, 4) + binary.BigEndian.PutUint32(round, qbftExtraFallback.Round) + qst.VanityData, qst.Validators, qst.Vote, qst.Round, qst.CommittedSeal = qbftExtraFallback.VanityData, qbftExtraFallback.Validators, qbftExtraFallback.Vote, round, qbftExtraFallback.CommittedSeal + return nil } qst.VanityData, qst.Validators, qst.Vote, qst.Round, qst.CommittedSeal = qbftExtra.VanityData, qbftExtra.Validators, qbftExtra.Vote, qbftExtra.Round, qbftExtra.CommittedSeal @@ -255,7 +277,15 @@ func ExtractQBFTExtra(h *Header) (*QBFTExtra, error) { qbftExtra := new(QBFTExtra) err := rlp.DecodeBytes(h.Extra[:], qbftExtra) if err != nil { - return nil, err + qbftExtraFallback := new(qbftExtraFallback) + err := rlp.DecodeBytes(h.Extra[:], qbftExtraFallback) + if err != nil { + return nil, err + } + log.Warn("qbft extra fallback to old version") + round := make([]byte, 4) + binary.BigEndian.PutUint32(round, qbftExtraFallback.Round) + qbftExtra.VanityData, qbftExtra.Validators, qbftExtra.Vote, qbftExtra.Round, qbftExtra.CommittedSeal = qbftExtraFallback.VanityData, qbftExtraFallback.Validators, qbftExtraFallback.Vote, round, qbftExtraFallback.CommittedSeal } return qbftExtra, nil } diff --git a/core/types/istanbul_test.go b/core/types/istanbul_test.go index 755c5850b4..ca9d855f5b 100644 --- a/core/types/istanbul_test.go +++ b/core/types/istanbul_test.go @@ -23,8 +23,26 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/stretchr/testify/assert" ) +func TestExtraFallback(t *testing.T) { + { + h := &Header{ + Extra: common.FromHex("0xf87aa00000000000000000000000000000000000000000000000000000000000000000f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408080c0"), + } + _, err := ExtractQBFTExtra(h) + assert.NoError(t, err) + } + { + h := &Header{ + Extra: common.FromHex("0xf87aa00000000000000000000000000000000000000000000000000000000000000000f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c080c0"), + } + _, err := ExtractQBFTExtra(h) + assert.NoError(t, err) + } +} + func TestHeaderHash(t *testing.T) { // 0xcefefd3ade63a5955bca4562ed840b67f39e74df217f7e5f7241a6e9552cca70 expectedExtra := common.FromHex("0x0000000000000000000000000000000000000000000000000000000000000000f89af8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440b8410000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0") From ff871bebe35bfc13c0af353987e4e5ed7cbfd93d Mon Sep 17 00:00:00 2001 From: baptiste-b-pegasys <85155432+baptiste-b-pegasys@users.noreply.github.com> Date: Thu, 13 Oct 2022 11:46:36 +0200 Subject: [PATCH 4/5] fix --- consensus/istanbul/qbft/engine/engine.go | 12 +- consensus/istanbul/qbft/engine/engine_test.go | 159 ++++++++++-------- core/types/block.go | 2 +- core/types/istanbul.go | 20 ++- 4 files changed, 109 insertions(+), 84 deletions(-) diff --git a/consensus/istanbul/qbft/engine/engine.go b/consensus/istanbul/qbft/engine/engine.go index 7cf2433eaf..13059923d3 100644 --- a/consensus/istanbul/qbft/engine/engine.go +++ b/consensus/istanbul/qbft/engine/engine.go @@ -3,8 +3,10 @@ package qbftengine import ( "bytes" "encoding/binary" + "errors" "fmt" "math/big" + "reflect" "strings" "time" @@ -533,7 +535,15 @@ func setExtra(h *types.Header, qbftExtra *types.QBFTExtra) error { if err != nil { return err } - + // security check + t := &types.QBFTExtra{} + err = rlp.DecodeBytes(payload, t) + if err != nil { + return err + } + if !reflect.DeepEqual(t, qbftExtra) { + return errors.New("encoding of QBFT extra mismatch") + } h.Extra = payload return nil } diff --git a/consensus/istanbul/qbft/engine/engine_test.go b/consensus/istanbul/qbft/engine/engine_test.go index 5bfe92334b..3de01949b6 100644 --- a/consensus/istanbul/qbft/engine/engine_test.go +++ b/consensus/istanbul/qbft/engine/engine_test.go @@ -43,93 +43,104 @@ func TestPrepareExtra(t *testing.T) { } func TestWriteCommittedSeals(t *testing.T) { - istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408080c0") - expectedCommittedSeal := append([]byte{1, 2, 3}, bytes.Repeat([]byte{0x00}, types.IstanbulExtraSeal-3)...) - expectedIstExtra := &types.QBFTExtra{ - VanityData: []byte{}, - Validators: []common.Address{ - common.BytesToAddress(hexutil.MustDecode("0x44add0ec310f115a0e603b2d7db9f067778eaf8a")), - common.BytesToAddress(hexutil.MustDecode("0x294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212")), - common.BytesToAddress(hexutil.MustDecode("0x6beaaed781d2d2ab6350f5c4566a2c6eaac407a6")), - common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), - }, - CommittedSeal: [][]byte{expectedCommittedSeal}, - Round: make([]byte, 0), - Vote: nil, - } + for _, istRawData := range [][]byte{ + hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c080c0"), + hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408080c0"), + } { + expectedCommittedSeal := append([]byte{1, 2, 3}, bytes.Repeat([]byte{0x00}, types.IstanbulExtraSeal-3)...) + expectedIstExtra := &types.QBFTExtra{ + VanityData: []byte{}, + Validators: []common.Address{ + common.BytesToAddress(hexutil.MustDecode("0x44add0ec310f115a0e603b2d7db9f067778eaf8a")), + common.BytesToAddress(hexutil.MustDecode("0x294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212")), + common.BytesToAddress(hexutil.MustDecode("0x6beaaed781d2d2ab6350f5c4566a2c6eaac407a6")), + common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), + }, + CommittedSeal: [][]byte{expectedCommittedSeal}, + Round: make([]byte, 0), + Vote: nil, + } - h := &types.Header{ - Extra: istRawData, - } + h := &types.Header{ + Extra: istRawData, + } - // normal case - err := ApplyHeaderQBFTExtra( - h, - writeCommittedSeals([][]byte{expectedCommittedSeal}), - ) - if err != nil { - t.Errorf("error mismatch: have %v, want: nil", err) - } + // normal case + err := ApplyHeaderQBFTExtra( + h, + writeCommittedSeals([][]byte{expectedCommittedSeal}), + ) + if err != nil { + t.Errorf("error mismatch: have %v, want: nil", err) + } - // verify istanbul extra-data - istExtra, err := getExtra(h) - if err != nil { - t.Errorf("error mismatch: have %v, want nil", err) - } - if !reflect.DeepEqual(istExtra, expectedIstExtra) { - t.Errorf("extra data mismatch: have %v, want %v", istExtra, expectedIstExtra) - } + // verify istanbul extra-data + istExtra, err := getExtra(h) + if err != nil { + t.Errorf("error mismatch: have %v, want nil", err) + } + if !reflect.DeepEqual(istExtra, expectedIstExtra) { + t.Errorf("extra data mismatch: have %v, want %v", istExtra, expectedIstExtra) + } - // invalid seal - unexpectedCommittedSeal := append(expectedCommittedSeal, make([]byte, 1)...) - err = ApplyHeaderQBFTExtra( - h, - writeCommittedSeals([][]byte{unexpectedCommittedSeal}), - ) - if err != istanbulcommon.ErrInvalidCommittedSeals { - t.Errorf("error mismatch: have %v, want %v", err, istanbulcommon.ErrInvalidCommittedSeals) + // invalid seal + unexpectedCommittedSeal := append(expectedCommittedSeal, make([]byte, 1)...) + err = ApplyHeaderQBFTExtra( + h, + writeCommittedSeals([][]byte{unexpectedCommittedSeal}), + ) + if err != istanbulcommon.ErrInvalidCommittedSeals { + t.Errorf("error mismatch: have %v, want %v", err, istanbulcommon.ErrInvalidCommittedSeals) + } } } func TestWriteRoundNumber(t *testing.T) { - istRawData := hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408005c0") - round := make([]byte, 4) - binary.BigEndian.PutUint32(round, 5) - expectedIstExtra := &types.QBFTExtra{ - VanityData: []byte{}, - Validators: []common.Address{ - common.BytesToAddress(hexutil.MustDecode("0x44add0ec310f115a0e603b2d7db9f067778eaf8a")), - common.BytesToAddress(hexutil.MustDecode("0x294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212")), - common.BytesToAddress(hexutil.MustDecode("0x6beaaed781d2d2ab6350f5c4566a2c6eaac407a6")), - common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), - }, - CommittedSeal: [][]byte{}, - Round: round, - Vote: nil, + testExtraData := [][]byte{ + hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b4408005c0"), + hexutil.MustDecode("0xf85a80f8549444add0ec310f115a0e603b2d7db9f067778eaf8a94294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212946beaaed781d2d2ab6350f5c4566a2c6eaac407a6948be76812f765c24641ec63dc2852b378aba2b440c005c0"), } + for _, istRawData := range testExtraData { + round := make([]byte, 4) + binary.BigEndian.PutUint32(round, 5) + expectedIstExtra := &types.QBFTExtra{ + VanityData: []byte{}, + Validators: []common.Address{ + common.BytesToAddress(hexutil.MustDecode("0x44add0ec310f115a0e603b2d7db9f067778eaf8a")), + common.BytesToAddress(hexutil.MustDecode("0x294fc7e8f22b3bcdcf955dd7ff3ba2ed833f8212")), + common.BytesToAddress(hexutil.MustDecode("0x6beaaed781d2d2ab6350f5c4566a2c6eaac407a6")), + common.BytesToAddress(hexutil.MustDecode("0x8be76812f765c24641ec63dc2852b378aba2b440")), + }, + CommittedSeal: [][]byte{}, + Round: round, + Vote: nil, + } - var expectedErr error + var expectedErr error - h := &types.Header{ - Extra: istRawData, - } + h := &types.Header{ + Extra: istRawData, + } - // normal case - err := ApplyHeaderQBFTExtra( - h, - writeRoundNumber(big.NewInt(5)), - ) - if err != expectedErr { - t.Errorf("error mismatch: have %v, want %v", err, expectedErr) - } + // normal case + err := ApplyHeaderQBFTExtra( + h, + writeRoundNumber(big.NewInt(5)), + ) + if err != expectedErr { + t.Errorf("error mismatch: have %v, want %v", err, expectedErr) + } - // verify istanbul extra-data - istExtra, err := getExtra(h) - if err != nil { - t.Errorf("error mismatch: have %v, want nil", err) - } - if !reflect.DeepEqual(istExtra, expectedIstExtra) { - t.Errorf("extra data mismatch: have %v, want %v", istExtra.VanityData, expectedIstExtra.VanityData) + // verify istanbul extra-data + istExtra, err := getExtra(h) + if err != nil { + t.Errorf("error mismatch: have %v, want nil", err) + } else if istExtra != nil && !reflect.DeepEqual(istExtra, expectedIstExtra) { + t.Errorf("extra data mismatch: have %v, want %v", istExtra.VanityData, expectedIstExtra.VanityData) + } else if istExtra == nil { + t.Errorf("extra data is nil") + } + assert.Equal(t, uint32(5), binary.BigEndian.Uint32(istExtra.Round)) } } diff --git a/core/types/block.go b/core/types/block.go index 42f7c30a24..8d01c270cc 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -100,7 +100,7 @@ type headerMarshaling struct { func (h *Header) Hash() common.Hash { // If the mix digest is equivalent to the predefined Istanbul digest, use Istanbul // specific hash calculation. - if h.Number.Uint64() == 0 { + if h.Number == nil || h.Number.Uint64() == 0 { return rlpHash(h) } if h.MixDigest == IstanbulDigest { diff --git a/core/types/istanbul.go b/core/types/istanbul.go index 70da1e23eb..29e6c69c2a 100644 --- a/core/types/istanbul.go +++ b/core/types/istanbul.go @@ -19,6 +19,7 @@ package types import ( "encoding/binary" "errors" + "fmt" "io" "github.com/ethereum/go-ethereum/common" @@ -129,7 +130,7 @@ func IstanbulFilteredHeader(h *Header, keepSeal bool) *Header { type QBFTExtra struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote `rlp:"nil"` + Vote *ValidatorVote `rlp:"nilString"` Round []byte CommittedSeal [][]byte } @@ -217,7 +218,7 @@ func (qst *QBFTExtra) DecodeRLP(s *rlp.Stream) error { var qbftExtra struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote `rlp:"nil"` + Vote *ValidatorVote `rlp:"nilString"` Round []byte CommittedSeal [][]byte } @@ -225,7 +226,7 @@ func (qst *QBFTExtra) DecodeRLP(s *rlp.Stream) error { var qbftExtraFallback struct { VanityData []byte Validators []common.Address - Vote *ValidatorVote `rlp:"nil"` + Vote *ValidatorVote `rlp:"nilString"` Round uint32 CommittedSeal [][]byte } @@ -278,13 +279,16 @@ func ExtractQBFTExtra(h *Header) (*QBFTExtra, error) { err := rlp.DecodeBytes(h.Extra[:], qbftExtra) if err != nil { qbftExtraFallback := new(qbftExtraFallback) - err := rlp.DecodeBytes(h.Extra[:], qbftExtraFallback) - if err != nil { - return nil, err + errFallback := rlp.DecodeBytes(h.Extra[:], qbftExtraFallback) + if errFallback != nil { + return nil, fmt.Errorf("got two decoding errors: 1=%w; 2=%v", err, errFallback) } log.Warn("qbft extra fallback to old version") - round := make([]byte, 4) - binary.BigEndian.PutUint32(round, qbftExtraFallback.Round) + round := make([]byte, 0) + if qbftExtraFallback.Round > 0 { + round = make([]byte, 4) + binary.BigEndian.PutUint32(round, qbftExtraFallback.Round) + } qbftExtra.VanityData, qbftExtra.Validators, qbftExtra.Vote, qbftExtra.Round, qbftExtra.CommittedSeal = qbftExtraFallback.VanityData, qbftExtraFallback.Validators, qbftExtraFallback.Vote, round, qbftExtraFallback.CommittedSeal } return qbftExtra, nil From 8c7b51772227a3ef376c189821128d5356f139a1 Mon Sep 17 00:00:00 2001 From: baptiste-b-pegasys <85155432+baptiste-b-pegasys@users.noreply.github.com> Date: Tue, 29 Nov 2022 10:29:48 +0100 Subject: [PATCH 5/5] fixes --- consensus/istanbul/qbft/engine/engine.go | 1 - consensus/istanbul/qbft/engine/engine_test.go | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/istanbul/qbft/engine/engine.go b/consensus/istanbul/qbft/engine/engine.go index 08e82642b2..efae87a2b8 100644 --- a/consensus/istanbul/qbft/engine/engine.go +++ b/consensus/istanbul/qbft/engine/engine.go @@ -7,7 +7,6 @@ import ( "fmt" "math/big" "reflect" - "strings" "time" "github.com/ethereum/go-ethereum/accounts/abi/bind" diff --git a/consensus/istanbul/qbft/engine/engine_test.go b/consensus/istanbul/qbft/engine/engine_test.go index 20cd96b8b5..7b34fa9f37 100644 --- a/consensus/istanbul/qbft/engine/engine_test.go +++ b/consensus/istanbul/qbft/engine/engine_test.go @@ -11,6 +11,7 @@ import ( "github.com/ethereum/go-ethereum/common/hexutil" istanbulcommon "github.com/ethereum/go-ethereum/consensus/istanbul/common" "github.com/ethereum/go-ethereum/core/types" + "github.com/stretchr/testify/assert" ) func TestPrepareExtra(t *testing.T) {