Skip to content

Commit

Permalink
[EVM-736]: Quorum calculation discrepancy between SC and Edge (#1723)
Browse files Browse the repository at this point in the history
* Add NoviSad fork

* Fix test

* Update go-ibft dependecy

* Remove  unnecessary code

* Rename fork
  • Loading branch information
goran-ethernal authored Jul 20, 2023
1 parent 851b76c commit 01d7791
Show file tree
Hide file tree
Showing 14 changed files with 91 additions and 73 deletions.
60 changes: 32 additions & 28 deletions chain/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,16 @@ func (p *Params) GetEngine() string {

// predefined forks
const (
Homestead = "homestead"
Byzantium = "byzantium"
Constantinople = "constantinople"
Petersburg = "petersburg"
Istanbul = "istanbul"
London = "london"
EIP150 = "EIP150"
EIP158 = "EIP158"
EIP155 = "EIP155"
Homestead = "homestead"
Byzantium = "byzantium"
Constantinople = "constantinople"
Petersburg = "petersburg"
Istanbul = "istanbul"
London = "london"
EIP150 = "EIP150"
EIP158 = "EIP158"
EIP155 = "EIP155"
QuorumCalcAlignment = "quorumcalcalignment"
)

// Forks is map which contains all forks and their starting blocks from genesis
Expand All @@ -111,15 +112,16 @@ func (f *Forks) RemoveFork(name string) {
// At returns ForksInTime instance that shows which supported forks are enabled for the block
func (f *Forks) At(block uint64) ForksInTime {
return ForksInTime{
Homestead: f.IsActive(Homestead, block),
Byzantium: f.IsActive(Byzantium, block),
Constantinople: f.IsActive(Constantinople, block),
Petersburg: f.IsActive(Petersburg, block),
Istanbul: f.IsActive(Istanbul, block),
London: f.IsActive(London, block),
EIP150: f.IsActive(EIP150, block),
EIP158: f.IsActive(EIP158, block),
EIP155: f.IsActive(EIP155, block),
Homestead: f.IsActive(Homestead, block),
Byzantium: f.IsActive(Byzantium, block),
Constantinople: f.IsActive(Constantinople, block),
Petersburg: f.IsActive(Petersburg, block),
Istanbul: f.IsActive(Istanbul, block),
London: f.IsActive(London, block),
EIP150: f.IsActive(EIP150, block),
EIP158: f.IsActive(EIP158, block),
EIP155: f.IsActive(EIP155, block),
QuorumCalcAlignment: f.IsActive(QuorumCalcAlignment, block),
}
}

Expand Down Expand Up @@ -164,18 +166,20 @@ type ForksInTime struct {
London,
EIP150,
EIP158,
EIP155 bool
EIP155,
QuorumCalcAlignment bool
}

// AllForksEnabled should contain all supported forks by current edge version
var AllForksEnabled = &Forks{
Homestead: NewFork(0),
EIP150: NewFork(0),
EIP155: NewFork(0),
EIP158: NewFork(0),
Byzantium: NewFork(0),
Constantinople: NewFork(0),
Petersburg: NewFork(0),
Istanbul: NewFork(0),
London: NewFork(0),
Homestead: NewFork(0),
EIP150: NewFork(0),
EIP155: NewFork(0),
EIP158: NewFork(0),
Byzantium: NewFork(0),
Constantinople: NewFork(0),
Petersburg: NewFork(0),
Istanbul: NewFork(0),
London: NewFork(0),
QuorumCalcAlignment: NewFork(0),
}
2 changes: 1 addition & 1 deletion consensus/polybft/consensus_runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ func (c *consensusRuntime) FSM() error {
}

if isEndOfSprint {
commitment, err := c.stateSyncManager.Commitment()
commitment, err := c.stateSyncManager.Commitment(pendingBlockNumber)
if err != nil {
return err
}
Expand Down
12 changes: 7 additions & 5 deletions consensus/polybft/extra.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (i *Extra) ValidateFinalizedData(header *types.Header, parent *types.Header
}

// validate current block signatures
checkpointHash, err := i.Checkpoint.Hash(chainID, header.Number, header.Hash)
checkpointHash, err := i.Checkpoint.Hash(chainID, blockNumber, header.Hash)
if err != nil {
return fmt.Errorf("failed to calculate proposal hash: %w", err)
}
Expand All @@ -151,7 +151,7 @@ func (i *Extra) ValidateFinalizedData(header *types.Header, parent *types.Header
return fmt.Errorf("failed to validate header for block %d. could not retrieve block validators:%w", blockNumber, err)
}

if err := i.Committed.Verify(validators, checkpointHash, domain, logger); err != nil {
if err := i.Committed.Verify(blockNumber, validators, checkpointHash, domain, logger); err != nil {
return fmt.Errorf("failed to verify signatures for block %d (proposal hash %s): %w",
blockNumber, checkpointHash, err)
}
Expand Down Expand Up @@ -197,7 +197,8 @@ func (i *Extra) ValidateParentSignatures(blockNumber uint64, consensusBackend po
return fmt.Errorf("failed to calculate parent proposal hash: %w", err)
}

if err := i.Parent.Verify(parentValidators, parentCheckpointHash, domain, logger); err != nil {
parentBlockNumber := blockNumber - 1
if err := i.Parent.Verify(parentBlockNumber, parentValidators, parentCheckpointHash, domain, logger); err != nil {
return fmt.Errorf("failed to verify signatures for parent of block %d (proposal hash: %s): %w",
blockNumber, parentCheckpointHash, err)
}
Expand Down Expand Up @@ -256,14 +257,15 @@ func (s *Signature) UnmarshalRLPWith(v *fastrlp.Value) error {
}

// Verify is used to verify aggregated signature based on current validator set, message hash and domain
func (s *Signature) Verify(validators validator.AccountSet, hash types.Hash, domain []byte, logger hclog.Logger) error {
func (s *Signature) Verify(blockNumber uint64, validators validator.AccountSet,
hash types.Hash, domain []byte, logger hclog.Logger) error {
signers, err := validators.GetFilteredValidators(s.Bitmap)
if err != nil {
return err
}

validatorSet := validator.NewValidatorSet(validators, logger)
if !validatorSet.HasQuorum(signers.GetAddressesAsSet()) {
if !validatorSet.HasQuorum(blockNumber, signers.GetAddressesAsSet()) {
return fmt.Errorf("quorum not reached")
}

Expand Down
8 changes: 4 additions & 4 deletions consensus/polybft/extra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,10 +394,10 @@ func TestSignature_Verify(t *testing.T) {
Bitmap: bitmap,
}

err = s.Verify(validatorsMetadata, msgHash, bls.DomainCheckpointManager, hclog.NewNullLogger())
err = s.Verify(10, validatorsMetadata, msgHash, bls.DomainCheckpointManager, hclog.NewNullLogger())
signers[val.Address()] = struct{}{}

if !validatorSet.HasQuorum(signers) {
if !validatorSet.HasQuorum(10, signers) {
assert.ErrorContains(t, err, "quorum not reached", "failed for %d", i)
} else {
assert.NoError(t, err)
Expand All @@ -415,7 +415,7 @@ func TestSignature_Verify(t *testing.T) {
bmp.Set(uint64(validatorSet.Len() + 1))
s := &Signature{Bitmap: bmp}

err := s.Verify(validatorSet, types.Hash{0x1}, bls.DomainCheckpointManager, hclog.NewNullLogger())
err := s.Verify(0, validatorSet, types.Hash{0x1}, bls.DomainCheckpointManager, hclog.NewNullLogger())
require.Error(t, err)
})
}
Expand Down Expand Up @@ -483,7 +483,7 @@ func TestSignature_VerifyRandom(t *testing.T) {
Bitmap: bitmap,
}

err = s.Verify(vals.GetPublicIdentities(), msgHash, bls.DomainCheckpointManager, hclog.NewNullLogger())
err = s.Verify(1, vals.GetPublicIdentities(), msgHash, bls.DomainCheckpointManager, hclog.NewNullLogger())
assert.NoError(t, err)
}

Expand Down
6 changes: 3 additions & 3 deletions consensus/polybft/fsm.go
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ func (f *fsm) VerifyStateTransactions(transactions []*types.Transaction) error {

commitmentTxExists = true

if err = verifyBridgeCommitmentTx(tx.Hash, stateTxData, f.validators); err != nil {
if err = verifyBridgeCommitmentTx(f.Height(), tx.Hash, stateTxData, f.validators); err != nil {
return err
}
case *contractsapi.CommitEpochValidatorSetFn:
Expand Down Expand Up @@ -623,15 +623,15 @@ func (f *fsm) verifyDistributeRewardsTx(distributeRewardsTx *types.Transaction)
}

// verifyBridgeCommitmentTx validates bridge commitment transaction
func verifyBridgeCommitmentTx(txHash types.Hash,
func verifyBridgeCommitmentTx(blockNumber uint64, txHash types.Hash,
commitment *CommitmentMessageSigned,
validators validator.ValidatorSet) error {
signers, err := validators.Accounts().GetFilteredValidators(commitment.AggSignature.Bitmap)
if err != nil {
return fmt.Errorf("failed to retrieve signers for state tx (%s): %w", txHash, err)
}

if !validators.HasQuorum(signers.GetAddressesAsSet()) {
if !validators.HasQuorum(blockNumber, signers.GetAddressesAsSet()) {
return fmt.Errorf("quorum size not reached for state tx (%s)", txHash)
}

Expand Down
6 changes: 6 additions & 0 deletions consensus/polybft/fsm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,7 @@ func TestFSM_VerifyStateTransactions_StateTransactionQuorumNotReached(t *testing
validatorSet := validator.NewValidatorSet(validators.GetPublicIdentities(), hclog.NewNullLogger())

fsm := &fsm{
parent: &types.Header{Number: 2},
isEndOfEpoch: true,
isEndOfSprint: true,
validators: validatorSet,
Expand Down Expand Up @@ -598,6 +599,7 @@ func TestFSM_VerifyStateTransactions_StateTransactionInvalidSignature(t *testing
validatorSet := validator.NewValidatorSet(validators.GetPublicIdentities(), hclog.NewNullLogger())

fsm := &fsm{
parent: &types.Header{Number: 1},
isEndOfEpoch: true,
isEndOfSprint: true,
validators: validatorSet,
Expand Down Expand Up @@ -1329,6 +1331,7 @@ func TestFSM_VerifyStateTransaction_ValidBothTypesOfStateTransactions(t *testing
}

f := &fsm{
parent: &types.Header{Number: 9},
isEndOfSprint: true,
validators: validators.ToValidatorSet(),
}
Expand Down Expand Up @@ -1372,6 +1375,7 @@ func TestFSM_VerifyStateTransaction_QuorumNotReached(t *testing.T) {
validators := validator.NewTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
_, commitmentMessageSigned, _ := buildCommitmentAndStateSyncs(t, 10, uint64(3), 2)
f := &fsm{
parent: &types.Header{Number: 9},
isEndOfSprint: true,
validators: validators.ToValidatorSet(),
}
Expand Down Expand Up @@ -1400,6 +1404,7 @@ func TestFSM_VerifyStateTransaction_InvalidSignature(t *testing.T) {
validators := validator.NewTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
_, commitmentMessageSigned, _ := buildCommitmentAndStateSyncs(t, 10, uint64(3), 2)
f := &fsm{
parent: &types.Header{Number: 9},
isEndOfSprint: true,
validators: validators.ToValidatorSet(),
}
Expand Down Expand Up @@ -1437,6 +1442,7 @@ func TestFSM_VerifyStateTransaction_TwoCommitmentMessages(t *testing.T) {
validatorSet := validator.NewValidatorSet(validators.GetPublicIdentities(), hclog.NewNullLogger())

f := &fsm{
parent: &types.Header{Number: 9},
isEndOfSprint: true,
validators: validatorSet,
}
Expand Down
1 change: 1 addition & 0 deletions consensus/polybft/polybft.go
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ func (p *Polybft) PreCommitState(block *types.Block, _ *state.Transition) error
commitmentTxExists = true

if err := verifyBridgeCommitmentTx(
block.Number(),
tx.Hash,
signedCommitment,
validator.NewValidatorSet(validators, p.logger)); err != nil {
Expand Down
22 changes: 12 additions & 10 deletions consensus/polybft/state_sync_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type StateSyncProof struct {
type StateSyncManager interface {
Init() error
Close()
Commitment() (*CommitmentMessageSigned, error)
Commitment(blockNumber uint64) (*CommitmentMessageSigned, error)
GetStateSyncProof(stateSyncID uint64) (types.Proof, error)
PostBlock(req *PostBlockRequest) error
PostEpoch(req *PostEpochRequest) error
Expand All @@ -47,11 +47,13 @@ var _ StateSyncManager = (*dummyStateSyncManager)(nil)
// dummyStateSyncManager is used when bridge is not enabled
type dummyStateSyncManager struct{}

func (n *dummyStateSyncManager) Init() error { return nil }
func (n *dummyStateSyncManager) Close() {}
func (n *dummyStateSyncManager) Commitment() (*CommitmentMessageSigned, error) { return nil, nil }
func (n *dummyStateSyncManager) PostBlock(req *PostBlockRequest) error { return nil }
func (n *dummyStateSyncManager) PostEpoch(req *PostEpochRequest) error { return nil }
func (n *dummyStateSyncManager) Init() error { return nil }
func (n *dummyStateSyncManager) Close() {}
func (n *dummyStateSyncManager) Commitment(blockNumber uint64) (*CommitmentMessageSigned, error) {
return nil, nil
}
func (n *dummyStateSyncManager) PostBlock(req *PostBlockRequest) error { return nil }
func (n *dummyStateSyncManager) PostEpoch(req *PostEpochRequest) error { return nil }
func (n *dummyStateSyncManager) GetStateSyncProof(stateSyncID uint64) (types.Proof, error) {
return types.Proof{}, nil
}
Expand Down Expand Up @@ -268,7 +270,7 @@ func (s *stateSyncManager) AddLog(eventLog *ethgo.Log) error {
}

// Commitment returns a commitment to be submitted if there is a pending commitment with quorum
func (s *stateSyncManager) Commitment() (*CommitmentMessageSigned, error) {
func (s *stateSyncManager) Commitment(blockNumber uint64) (*CommitmentMessageSigned, error) {
s.lock.RLock()
defer s.lock.RUnlock()

Expand All @@ -277,7 +279,7 @@ func (s *stateSyncManager) Commitment() (*CommitmentMessageSigned, error) {
// we start from the end, since last pending commitment is the largest one
for i := len(s.pendingCommitments) - 1; i >= 0; i-- {
commitment := s.pendingCommitments[i]
aggregatedSignature, publicKeys, err := s.getAggSignatureForCommitmentMessage(commitment)
aggregatedSignature, publicKeys, err := s.getAggSignatureForCommitmentMessage(blockNumber, commitment)

if err != nil {
if errors.Is(err, errQuorumNotReached) {
Expand Down Expand Up @@ -306,7 +308,7 @@ func (s *stateSyncManager) Commitment() (*CommitmentMessageSigned, error) {

// getAggSignatureForCommitmentMessage checks if pending commitment has quorum,
// and if it does, aggregates the signatures
func (s *stateSyncManager) getAggSignatureForCommitmentMessage(
func (s *stateSyncManager) getAggSignatureForCommitmentMessage(blockNumber uint64,
commitment *PendingCommitment) (Signature, [][]byte, error) {
validatorSet := s.validatorSet

Expand Down Expand Up @@ -352,7 +354,7 @@ func (s *stateSyncManager) getAggSignatureForCommitmentMessage(
signers[types.StringToAddress(vote.From)] = struct{}{}
}

if !validatorSet.HasQuorum(signers) {
if !validatorSet.HasQuorum(blockNumber, signers) {
return Signature{}, nil, errQuorumNotReached
}

Expand Down
6 changes: 3 additions & 3 deletions consensus/polybft/state_sync_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ func TestStateSyncManager_BuildCommitment(t *testing.T) {
s.validatorSet = vals.ToValidatorSet()

// commitment is empty
commitment, err := s.Commitment()
commitment, err := s.Commitment(1)
require.NoError(t, err)
require.Nil(t, commitment)

Expand Down Expand Up @@ -262,7 +262,7 @@ func TestStateSyncManager_BuildCommitment(t *testing.T) {
require.NoError(t, s.saveVote(signedMsg1))
require.NoError(t, s.saveVote(signedMsg2))

commitment, err = s.Commitment()
commitment, err = s.Commitment(1)
require.NoError(t, err) // there is no error if quorum is not met, since its a valid case
require.Nil(t, commitment)

Expand All @@ -277,7 +277,7 @@ func TestStateSyncManager_BuildCommitment(t *testing.T) {
require.NoError(t, s.saveVote(signedMsg1))
require.NoError(t, s.saveVote(signedMsg2))

commitment, err = s.Commitment()
commitment, err = s.Commitment(1)
require.NoError(t, err)
require.NotNil(t, commitment)
}
Expand Down
Loading

0 comments on commit 01d7791

Please sign in to comment.