Skip to content

Commit

Permalink
Remove unused Seal field from the extra data (#1379)
Browse files Browse the repository at this point in the history
* Remove unused Seal field from the extra data

* Fix tests

* Update field comments
  • Loading branch information
Stefan-Ethernal committed Apr 13, 2023
1 parent eea4fa3 commit dd7ec79
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 42 deletions.
38 changes: 11 additions & 27 deletions consensus/polybft/extra.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ var PolyBFTMixDigest = types.StringToHash("adce6e5230abe012342a44e4e9b6d05997d6f
// Extra defines the structure of the extra field for Istanbul
type Extra struct {
Validators *ValidatorSetDelta
Seal []byte
Parent *Signature
Committed *Signature
Checkpoint *CheckpointData
Expand All @@ -52,21 +51,14 @@ func (i *Extra) MarshalRLPWith(ar *fastrlp.Arena) *fastrlp.Value {
vv.Set(i.Validators.MarshalRLPWith(ar))
}

// Seal
if len(i.Seal) == 0 {
vv.Set(ar.NewNull())
} else {
vv.Set(ar.NewBytes(i.Seal))
}

// ParentSeal
// Parent Signatures
if i.Parent == nil {
vv.Set(ar.NewNullArray())
} else {
vv.Set(i.Parent.MarshalRLPWith(ar))
}

// CommittedSeal
// Committed Signatures
if i.Committed == nil {
vv.Set(ar.NewNullArray())
} else {
Expand All @@ -90,7 +82,7 @@ func (i *Extra) UnmarshalRLP(input []byte) error {

// UnmarshalRLPWith defines the unmarshal implementation for Extra
func (i *Extra) UnmarshalRLPWith(v *fastrlp.Value) error {
const expectedElements = 5
const expectedElements = 4

elems, err := v.GetElems()
if err != nil {
Expand All @@ -109,33 +101,26 @@ func (i *Extra) UnmarshalRLPWith(v *fastrlp.Value) error {
}
}

// Seal
if elems[1].Len() > 0 {
if i.Seal, err = elems[1].GetBytes(i.Seal); err != nil {
return err
}
}

// Parent
if elems[2].Elems() > 0 {
// Parent Signatures
if elems[1].Elems() > 0 {
i.Parent = &Signature{}
if err := i.Parent.UnmarshalRLPWith(elems[2]); err != nil {
if err := i.Parent.UnmarshalRLPWith(elems[1]); err != nil {
return err
}
}

// Committed
if elems[3].Elems() > 0 {
// Committed Signatures
if elems[2].Elems() > 0 {
i.Committed = &Signature{}
if err := i.Committed.UnmarshalRLPWith(elems[3]); err != nil {
if err := i.Committed.UnmarshalRLPWith(elems[2]); err != nil {
return err
}
}

// Checkpoint
if elems[4].Elems() > 0 {
if elems[3].Elems() > 0 {
i.Checkpoint = &CheckpointData{}
if err := i.Checkpoint.UnmarshalRLPWith(elems[4]); err != nil {
if err := i.Checkpoint.UnmarshalRLPWith(elems[3]); err != nil {
return err
}
}
Expand Down Expand Up @@ -689,7 +674,6 @@ func GetIbftExtraClean(extraRaw []byte) ([]byte, error) {
Parent: extra.Parent,
Validators: extra.Validators,
Checkpoint: extra.Checkpoint,
Seal: []byte{},
Committed: &Signature{},
}

Expand Down
31 changes: 19 additions & 12 deletions consensus/polybft/extra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/0xPolygon/polygon-edge/consensus/polybft/bitmap"
bls "github.com/0xPolygon/polygon-edge/consensus/polybft/signer"
"github.com/0xPolygon/polygon-edge/consensus/polybft/wallet"
"github.com/0xPolygon/polygon-edge/crypto"
"github.com/0xPolygon/polygon-edge/types"
"github.com/hashicorp/go-hclog"
"github.com/stretchr/testify/assert"
Expand All @@ -23,9 +24,17 @@ import (
func TestExtra_Encoding(t *testing.T) {
t.Parallel()

parentStr := []byte("Here is the parent signature")
committedStr := []byte("Here is the committed signature")
bitmapStr := []byte("Here are the bitmap bytes")
digest := crypto.Keccak256([]byte("Dummy content to sign"))
keys := createRandomTestKeys(t, 2)
parentSig, err := keys[0].Sign(digest)
require.NoError(t, err)

committedSig, err := keys[1].Sign(digest)
require.NoError(t, err)

bmp := bitmap.Bitmap{}
bmp.Set(1)
bmp.Set(4)

addedValidators := newTestValidatorsWithAliases(t, []string{"A", "B", "C"}).getPublicIdentities()

Expand All @@ -49,7 +58,6 @@ func TestExtra_Encoding(t *testing.T) {
{
&Extra{
Validators: &ValidatorSetDelta{},
Seal: []byte{3, 4},
},
},
{
Expand All @@ -66,7 +74,7 @@ func TestExtra_Encoding(t *testing.T) {
Validators: &ValidatorSetDelta{
Removed: removedValidators,
},
Parent: &Signature{AggregatedSignature: parentStr, Bitmap: bitmapStr},
Parent: &Signature{AggregatedSignature: parentSig, Bitmap: bmp},
Committed: &Signature{},
},
},
Expand All @@ -78,19 +86,19 @@ func TestExtra_Encoding(t *testing.T) {
Removed: removedValidators,
},
Parent: &Signature{},
Committed: &Signature{AggregatedSignature: committedStr, Bitmap: bitmapStr},
Committed: &Signature{AggregatedSignature: committedSig, Bitmap: bmp},
},
},
{
&Extra{
Parent: &Signature{AggregatedSignature: parentStr, Bitmap: bitmapStr},
Committed: &Signature{AggregatedSignature: committedStr, Bitmap: bitmapStr},
Parent: &Signature{AggregatedSignature: parentSig, Bitmap: bmp},
Committed: &Signature{AggregatedSignature: committedSig, Bitmap: bmp},
},
},
{
&Extra{
Parent: &Signature{AggregatedSignature: parentStr, Bitmap: bitmapStr},
Committed: &Signature{AggregatedSignature: committedStr, Bitmap: bitmapStr},
Parent: &Signature{AggregatedSignature: parentSig, Bitmap: bmp},
Committed: &Signature{AggregatedSignature: committedSig, Bitmap: bmp},
Checkpoint: &CheckpointData{
BlockRound: 0,
EpochNumber: 3,
Expand Down Expand Up @@ -126,7 +134,7 @@ func TestExtra_UnmarshalRLPWith_NegativeCases(t *testing.T) {

extra := &Extra{}
ar := &fastrlp.Arena{}
require.ErrorContains(t, extra.UnmarshalRLPWith(ar.NewArray()), "incorrect elements count to decode Extra, expected 5 but found 0")
require.ErrorContains(t, extra.UnmarshalRLPWith(ar.NewArray()), "incorrect elements count to decode Extra, expected 4 but found 0")
})

t.Run("Incorrect ValidatorSetDelta marshalled", func(t *testing.T) {
Expand All @@ -152,7 +160,6 @@ func TestExtra_UnmarshalRLPWith_NegativeCases(t *testing.T) {
extraMarshalled := ar.NewArray()
deltaMarshalled := new(ValidatorSetDelta).MarshalRLPWith(ar)
extraMarshalled.Set(deltaMarshalled) // ValidatorSetDelta
extraMarshalled.Set(ar.NewNull()) // Seal
extraMarshalled.Set(ar.NewBytes([]byte{})) // Parent
extraMarshalled.Set(ar.NewBytes([]byte{})) // Committed
require.Error(t, extra.UnmarshalRLPWith(extraMarshalled))
Expand Down
2 changes: 0 additions & 2 deletions consensus/polybft/hash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ func Test_setupHeaderHashFunc(t *testing.T) {
Validators: &ValidatorSetDelta{Removed: bitmap.Bitmap{1}},
Parent: createSignature(t, []*wallet.Account{generateTestAccount(t)}, types.ZeroHash, bls.DomainCheckpointManager),
Checkpoint: &CheckpointData{},
Seal: []byte{},
Committed: &Signature{},
}

Expand All @@ -28,7 +27,6 @@ func Test_setupHeaderHashFunc(t *testing.T) {
header.ExtraData = append(make([]byte, ExtraVanity), extra.MarshalRLPTo(nil)...)
notFullExtraHash := types.HeaderHash(header)

extra.Seal = []byte{1, 2, 3, 255}
extra.Committed = createSignature(t, []*wallet.Account{generateTestAccount(t)}, types.ZeroHash, bls.DomainCheckpointManager)
header.ExtraData = append(make([]byte, ExtraVanity), extra.MarshalRLPTo(nil)...)
fullExtraHash := types.HeaderHash(header)
Expand Down
1 change: 0 additions & 1 deletion consensus/polybft/polybft_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ func TestPolybft_VerifyHeader(t *testing.T) {
Parent: parentSignature,
Checkpoint: checkpointData,
Committed: &Signature{},
Seal: []byte{},
}

if extra.Checkpoint == nil {
Expand Down

0 comments on commit dd7ec79

Please sign in to comment.