From 3d9316da20758fcb875995475e939a2c71b09f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= <93934272+Stefan-Ethernal@users.noreply.github.com> Date: Mon, 20 Mar 2023 13:52:22 +0100 Subject: [PATCH] Provide validators in the same order when initializing `CheckpointManager` SC and genesis (#1309) * Order validators by addresses when creating manifest * Print genesis validators to CLI output * Remove sorting validators when writing genesis delta * Fix manifest file validators sort * Get rid off sorting and apply delta when calculating next validator set in the fsm * Add comments * Remove outputting validators from genesis and rootchain contracts commands * Fix register validator E2E test * Add logs * Address comments --- command/genesis/genesis.go | 2 +- command/genesis/polybft_params.go | 19 ++--- .../rootchain/initcontracts/init_contracts.go | 41 +++++++---- consensus/polybft/extra.go | 2 +- consensus/polybft/fsm.go | 39 +++++++++-- consensus/polybft/polybft_config.go | 6 ++ consensus/polybft/system_state.go | 8 --- consensus/polybft/validator_metadata.go | 10 +-- consensus/polybft/validators_snapshot.go | 4 +- e2e-polybft/e2e/consensus_test.go | 69 +++++++++++++------ e2e-polybft/e2e/txpool_test.go | 4 +- e2e-polybft/framework/test-cluster.go | 8 +-- e2e-polybft/grpc_validation_test.go | 2 +- 13 files changed, 141 insertions(+), 73 deletions(-) diff --git a/command/genesis/genesis.go b/command/genesis/genesis.go index 33643372ec..eb187498a2 100644 --- a/command/genesis/genesis.go +++ b/command/genesis/genesis.go @@ -249,7 +249,7 @@ func runCommand(cmd *cobra.Command, _ []string) { var err error if params.isPolyBFTConsensus() { - err = params.generatePolyBftChainConfig() + err = params.generatePolyBftChainConfig(outputter) } else { err = params.generateGenesis() } diff --git a/command/genesis/polybft_params.go b/command/genesis/polybft_params.go index 21f59db202..f9b8dc2f1b 100644 --- a/command/genesis/polybft_params.go +++ b/command/genesis/polybft_params.go @@ -1,11 +1,9 @@ package genesis import ( - "bytes" "errors" "fmt" "math/big" - "sort" "time" "github.com/0xPolygon/polygon-edge/consensus/polybft/contractsapi" @@ -49,7 +47,7 @@ var ( ) // generatePolyBftChainConfig creates and persists polybft chain configuration to the provided file path -func (p *genesisParams) generatePolyBftChainConfig() error { +func (p *genesisParams) generatePolyBftChainConfig(o command.OutputFormatter) error { // load manifest file manifest, err := polybft.LoadManifest(p.manifestPath) if err != nil { @@ -74,6 +72,16 @@ func (p *genesisParams) generatePolyBftChainConfig() error { bridge.EventTrackerStartBlocks = eventTrackerStartBlock } + if _, err := o.Write([]byte("[GENESIS VALIDATORS]\n")); err != nil { + return err + } + + for _, v := range manifest.GenesisValidators { + if _, err := o.Write([]byte(fmt.Sprintf("%v\n", v))); err != nil { + return err + } + } + polyBftConfig := &polybft.PolyBFTConfig{ InitialValidatorSet: manifest.GenesisValidators, BlockTime: p.blockTime, @@ -262,11 +270,6 @@ func generateExtraDataPolyBft(validators []*polybft.ValidatorMetadata) ([]byte, Removed: bitmap.Bitmap{}, } - // Order validators based on its addresses - sort.Slice(delta.Added, func(i, j int) bool { - return bytes.Compare(delta.Added[i].Address[:], delta.Added[j].Address[:]) < 0 - }) - extra := polybft.Extra{Validators: delta, Checkpoint: &polybft.CheckpointData{}} return append(make([]byte, polybft.ExtraVanity), extra.MarshalRLPTo(nil)...), nil diff --git a/command/rootchain/initcontracts/init_contracts.go b/command/rootchain/initcontracts/init_contracts.go index 0045c436a8..0a2ca3f617 100644 --- a/command/rootchain/initcontracts/init_contracts.go +++ b/command/rootchain/initcontracts/init_contracts.go @@ -1,10 +1,8 @@ package initcontracts import ( - "bytes" "fmt" "math/big" - "sort" "strings" "github.com/spf13/cobra" @@ -331,7 +329,7 @@ func deployContracts(outputter command.OutputFormatter, client *jsonrpc.Client, } // init CheckpointManager - if err := initializeCheckpointManager(txRelayer, manifest, deployerKey); err != nil { + if err := initializeCheckpointManager(outputter, txRelayer, manifest, deployerKey); err != nil { return err } @@ -362,10 +360,11 @@ func deployContracts(outputter command.OutputFormatter, client *jsonrpc.Client, // initializeCheckpointManager invokes initialize function on "CheckpointManager" smart contract func initializeCheckpointManager( + o command.OutputFormatter, txRelayer txrelayer.TxRelayer, manifest *polybft.Manifest, deployerKey ethgo.Key) error { - validatorSet, err := validatorSetToABISlice(manifest.GenesisValidators) + validatorSet, err := validatorSetToABISlice(o, manifest.GenesisValidators) if err != nil { return fmt.Errorf("failed to convert validators to map: %w", err) } @@ -451,27 +450,39 @@ func sendTransaction(txRelayer txrelayer.TxRelayer, txn *ethgo.Transaction, cont // validatorSetToABISlice converts given validators to generic map // which is used for ABI encoding validator set being sent to the rootchain contract -func validatorSetToABISlice(validators []*polybft.Validator) ([]*contractsapi.Validator, error) { - genesisValidators := make([]*polybft.Validator, len(validators)) - copy(genesisValidators, validators) - sort.Slice(genesisValidators, func(i, j int) bool { - return bytes.Compare(genesisValidators[i].Address.Bytes(), genesisValidators[j].Address.Bytes()) < 0 - }) +func validatorSetToABISlice(o command.OutputFormatter, + validators []*polybft.Validator) ([]*contractsapi.Validator, error) { + accSet := make(polybft.AccountSet, len(validators)) + + if _, err := o.Write([]byte(fmt.Sprintf("%s [VALIDATORS]\n", contractsDeploymentTitle))); err != nil { + return nil, err + } - accSet := make(polybft.AccountSet, len(genesisValidators)) + for i, validator := range validators { + if _, err := o.Write([]byte(fmt.Sprintf("%v\n", validator))); err != nil { + return nil, err + } - for i, validatorInfo := range genesisValidators { - blsKey, err := validatorInfo.UnmarshalBLSPublicKey() + blsKey, err := validator.UnmarshalBLSPublicKey() if err != nil { return nil, err } accSet[i] = &polybft.ValidatorMetadata{ - Address: validatorInfo.Address, + Address: validator.Address, BlsKey: blsKey, - VotingPower: new(big.Int).Set(validatorInfo.Balance), + VotingPower: new(big.Int).Set(validator.Balance), } } + hash, err := accSet.Hash() + if err != nil { + return nil, err + } + + if _, err := o.Write([]byte(fmt.Sprintf("%s Validators hash: %s\n", contractsDeploymentTitle, hash))); err != nil { + return nil, err + } + return accSet.ToAPIBinding(), nil } diff --git a/consensus/polybft/extra.go b/consensus/polybft/extra.go index 8a95696be8..35311ed785 100644 --- a/consensus/polybft/extra.go +++ b/consensus/polybft/extra.go @@ -417,7 +417,7 @@ func (d *ValidatorSetDelta) Copy() *ValidatorSetDelta { // fmt.Stringer interface implementation func (d *ValidatorSetDelta) String() string { - return fmt.Sprintf("Added %v Removed %v Updated %v", d.Added, d.Removed, d.Updated) + return fmt.Sprintf("Added: \n%v Removed: %v\n Updated: \n%v\n", d.Added, d.Removed, d.Updated) } // Signature represents aggregated signatures of signers accompanied with a bitmap diff --git a/consensus/polybft/fsm.go b/consensus/polybft/fsm.go index ac136509ec..0c3e12b051 100644 --- a/consensus/polybft/fsm.go +++ b/consensus/polybft/fsm.go @@ -139,6 +139,11 @@ func (f *fsm) BuildProposal(currentRound uint64) ([]byte, error) { extra.Validators = validatorsDelta f.logger.Trace("[FSM Build Proposal]", "Validators Delta", validatorsDelta) + + nextValidators, err = f.getValidatorsTransition(validatorsDelta) + if err != nil { + return nil, err + } } currentValidatorsHash, err := f.validators.Accounts().Hash() @@ -159,6 +164,9 @@ func (f *fsm) BuildProposal(currentRound uint64) ([]byte, error) { EventRoot: f.exitEventRootHash, } + f.logger.Debug("[Build Proposal]", "Current validators hash", currentValidatorsHash, + "Next validators hash", nextValidatorsHash) + stateBlock, err := f.blockBuilder.Build(func(h *types.Header) { h.ExtraData = append(make([]byte, ExtraVanity), extra.MarshalRLPTo(nil)...) h.MixHash = PolyBFTMixDigest @@ -205,6 +213,28 @@ func (f *fsm) stateTransactions() []*types.Transaction { return txns } +// getValidatorsTransition applies delta to the current validators, +// as ChildValidatorSet SC returns validators in different order than the one kept on the Edge +func (f *fsm) getValidatorsTransition(delta *ValidatorSetDelta) (AccountSet, error) { + nextValidators, err := f.validators.Accounts().ApplyDelta(delta) + if err != nil { + return nil, err + } + + if f.logger.IsDebug() { + var buf bytes.Buffer + for _, v := range nextValidators { + if _, err := buf.WriteString(fmt.Sprintf("%s\n", v.String())); err != nil { + return nil, err + } + } + + f.logger.Debug("getValidatorsTransition", "Next validators", buf.String()) + } + + return nextValidators, nil +} + // createCommitEpochTx create a StateTransaction, which invokes ValidatorSet smart contract // and sends all the necessary metadata to it. func (f *fsm) createCommitEpochTx() (*types.Transaction, error) { @@ -295,6 +325,11 @@ func (f *fsm) Validate(proposal []byte) error { return err } + nextValidators, err = f.getValidatorsTransition(extra.Validators) + if err != nil { + return err + } + return extra.Checkpoint.Validate(parentExtra.Checkpoint, currentValidators, nextValidators) } @@ -522,10 +557,6 @@ func (f *fsm) getCurrentValidators(pendingBlockState *state.Transition) (Account return nil, fmt.Errorf("failed to retrieve validator set for current block: %w", err) } - if f.logger.IsDebug() { - f.logger.Debug("getCurrentValidators", "Validator set", newValidators.String()) - } - return newValidators, nil } diff --git a/consensus/polybft/polybft_config.go b/consensus/polybft/polybft_config.go index a80df23cc8..48c6b77ab8 100644 --- a/consensus/polybft/polybft_config.go +++ b/consensus/polybft/polybft_config.go @@ -184,6 +184,12 @@ func (v *Validator) ToValidatorMetadata() (*ValidatorMetadata, error) { return metadata, nil } +// String implements fmt.Stringer interface +func (v *Validator) String() string { + return fmt.Sprintf("Address=%s; Balance=%d; P2P Multi addr=%s; BLS Key=%s;", + v.Address, v.Balance, v.MultiAddr, v.BlsKey) +} + // RootchainConfig contains information about rootchain contract addresses // as well as rootchain admin account address type RootchainConfig struct { diff --git a/consensus/polybft/system_state.go b/consensus/polybft/system_state.go index 6edce9a803..81b9243995 100644 --- a/consensus/polybft/system_state.go +++ b/consensus/polybft/system_state.go @@ -1,11 +1,9 @@ package polybft import ( - "bytes" "encoding/json" "fmt" "math/big" - "sort" "github.com/0xPolygon/polygon-edge/consensus/polybft/contractsapi" bls "github.com/0xPolygon/polygon-edge/consensus/polybft/signer" @@ -110,12 +108,6 @@ func (s *SystemStateImpl) GetValidatorSet() (AccountSet, error) { res = append(res, val) } - // It is important to keep validator ordered by addresses, because of internal storage on SC - // which changes original ordering of sent validators - sort.Slice(res, func(i, j int) bool { - return bytes.Compare(res[i].Address[:], res[j].Address[:]) < 0 - }) - return res, nil } diff --git a/consensus/polybft/validator_metadata.go b/consensus/polybft/validator_metadata.go index 39e0364538..d81d490352 100644 --- a/consensus/polybft/validator_metadata.go +++ b/consensus/polybft/validator_metadata.go @@ -1,13 +1,13 @@ package polybft import ( + "bytes" "encoding/hex" "encoding/json" "errors" "fmt" "math/big" "reflect" - "strings" "github.com/0xPolygon/polygon-edge/consensus/polybft/contractsapi" bls "github.com/0xPolygon/polygon-edge/consensus/polybft/signer" @@ -142,12 +142,12 @@ func (as AccountSet) Equals(other AccountSet) bool { // fmt.Stringer implementation func (as AccountSet) String() string { - metadataString := make([]string, len(as)) - for i, v := range as { - metadataString[i] = v.String() + var buf bytes.Buffer + for _, v := range as { + buf.WriteString(fmt.Sprintf("%s\n", v.String())) } - return strings.Join(metadataString, "\n") + return buf.String() } // GetAddresses aggregates addresses for given AccountSet diff --git a/consensus/polybft/validators_snapshot.go b/consensus/polybft/validators_snapshot.go index 08fbc113aa..8189004abb 100644 --- a/consensus/polybft/validators_snapshot.go +++ b/consensus/polybft/validators_snapshot.go @@ -195,9 +195,9 @@ func (v *validatorsSnapshotCache) computeSnapshot( return nil, fmt.Errorf("failed to apply delta to the validators snapshot, block#%d: %w", header.Number, err) } - v.logger.Trace("Computed snapshot", + v.logger.Debug("Computed snapshot", "blockNumber", nextEpochEndBlockNumber, - "snapshot", snapshot, + "snapshot", snapshot.String(), "delta", extra.Validators) return &validatorSnapshot{ diff --git a/e2e-polybft/e2e/consensus_test.go b/e2e-polybft/e2e/consensus_test.go index 8e18b856af..680a4b116e 100644 --- a/e2e-polybft/e2e/consensus_test.go +++ b/e2e-polybft/e2e/consensus_test.go @@ -107,11 +107,13 @@ func TestE2E_Consensus_RegisterValidator(t *testing.T) { premineBalance = "0x1A784379D99DB42000000" // 2M native tokens (so that we have enough balance to fund new validator) ) - // new validator data - firstValidatorDataDir := fmt.Sprintf("test-chain-%d", validatorSize+1) // directory where the first validator secrets will be stored - secondValidatorDataDir := fmt.Sprintf("test-chain-%d", validatorSize+2) // directory where the second validator secrets will be stored - newValidatorInitBalance := "500000000000000000000000" // 500k - balance which will be transferred to the new validator - newValidatorStakeRaw := "0x8AC7230489E80000" // 10 native tokens - amout which will be staked by the new validator + var ( + firstValidatorDataDir = fmt.Sprintf("test-chain-%d", validatorSize+1) // directory where the first validator secrets will be stored + secondValidatorDataDir = fmt.Sprintf("test-chain-%d", validatorSize+2) // directory where the second validator secrets will be stored + newValidatorInitBalance = "500000000000000000000000" // 500k - balance which will be transferred to the new validator + newValidatorStakeRaw = "0x8AC7230489E80000" // 10 native tokens - amount which will be staked by the new validator + ) + newValidatorStake, err := types.ParseUint256orHex(&newValidatorStakeRaw) require.NoError(t, err) @@ -156,7 +158,16 @@ func TestE2E_Consensus_RegisterValidator(t *testing.T) { // wait for consensus to start require.NoError(t, cluster.WaitForBlock(1, 10*time.Second)) - // owner enlists both new validators + genesisBlock, err := owner.JSONRPC().Eth().GetBlockByNumber(0, false) + require.NoError(t, err) + + extra, err := polybft.GetIbftExtra(genesisBlock.ExtraData) + require.NoError(t, err) + + // on genesis block all validators are marked as added, which makes initial validator set + initialValidators := extra.Validators.Added + + // owner whitelists both new validators require.NoError(t, owner.WhitelistValidator(firstValidatorAddr.String(), ownerSecrets)) require.NoError(t, owner.WhitelistValidator(secondValidatorAddr.String(), ownerSecrets)) @@ -172,22 +183,22 @@ func TestE2E_Consensus_RegisterValidator(t *testing.T) { require.True(t, ok) // send some tokens from the owner to the first validator so that the first validator can register and stake - receipt1, err := txRelayer.SendTransaction(ðgo.Transaction{ + receipt, err := txRelayer.SendTransaction(ðgo.Transaction{ From: ownerAcc.Ecdsa.Address(), To: &firstValidatorAddr, Value: initialBalance, }, ownerAcc.Ecdsa) require.NoError(t, err) - require.Equal(t, uint64(types.ReceiptSuccess), receipt1.Status) + require.Equal(t, uint64(types.ReceiptSuccess), receipt.Status) // send some tokens from the owner to the second validator so that the second validator can register and stake - receipt2, err := txRelayer.SendTransaction(ðgo.Transaction{ + receipt, err = txRelayer.SendTransaction(ðgo.Transaction{ From: ownerAcc.Ecdsa.Address(), To: &secondValidatorAddr, Value: initialBalance, }, ownerAcc.Ecdsa) require.NoError(t, err) - require.Equal(t, uint64(types.ReceiptSuccess), receipt2.Status) + require.Equal(t, uint64(types.ReceiptSuccess), receipt.Status) // collect the first and the second validator from the cluster firstValidator := cluster.Servers[validatorSize] @@ -213,30 +224,44 @@ func TestE2E_Consensus_RegisterValidator(t *testing.T) { require.NoError(t, secondValidator.Stake(newValidatorStake.Uint64())) validators := polybft.AccountSet{} - // assert that new validator is among validator set - require.NoError(t, cluster.WaitUntil(20*time.Second, func() bool { + require.NoError(t, cluster.WaitUntil(20*time.Second, 1*time.Second, func() bool { // query validators validators, err = systemState.GetValidatorSet() require.NoError(t, err) - return validators.ContainsAddress((types.Address(firstValidatorAddr))) && validators.ContainsAddress((types.Address(secondValidatorAddr))) + return validators.ContainsAddress((types.Address(firstValidatorAddr))) && + validators.ContainsAddress((types.Address(secondValidatorAddr))) })) - // assert that validators hash is correct + // assert that next validators hash is correct block, err := owner.JSONRPC().Eth().GetBlockByNumber(ethgo.Latest, false) require.NoError(t, err) t.Logf("Block Number=%d\n", block.Number) - // unmarshal header extra data - extra, err := polybft.GetIbftExtra(block.ExtraData) - require.NoError(t, err) - require.NotNil(t, extra.Checkpoint) + // apply all deltas from epoch ending blocks + nextValidators := initialValidators + + // apply all the deltas to the initial validator set + latestEpochEndBlock := block.Number - (block.Number % epochSize) + for blockNum := uint64(epochSize); blockNum <= latestEpochEndBlock; blockNum += epochSize { + block, err = owner.JSONRPC().Eth().GetBlockByNumber(ethgo.BlockNumber(blockNum), false) + require.NoError(t, err) + + extra, err = polybft.GetIbftExtra(block.ExtraData) + require.NoError(t, err) + require.NotNil(t, extra.Checkpoint) + + t.Logf("Block Number: %d, Delta: %v\n", blockNum, extra.Validators) + + nextValidators, err = nextValidators.ApplyDelta(extra.Validators) + require.NoError(t, err) + } // assert that correct validators hash gets submitted - validatorsHash, err := validators.Hash() + nextValidatorsHash, err := nextValidators.Hash() require.NoError(t, err) - require.Equal(t, extra.Checkpoint.NextValidatorsHash, validatorsHash) + require.Equal(t, extra.Checkpoint.NextValidatorsHash, nextValidatorsHash) // query the first validator firstValidatorInfo, err := sidechain.GetValidatorInfo(firstValidatorAddr, txRelayer) @@ -264,7 +289,7 @@ func TestE2E_Consensus_RegisterValidator(t *testing.T) { require.NoError(t, err) // check if the first validator has signed any prposals - firstSealed, err := firstValidator.HasValidatorSealed(currentBlock, currentBlock+3*epochSize, validators, firstValidatorAddr) + firstSealed, err := firstValidator.HasValidatorSealed(currentBlock, currentBlock+3*epochSize, nextValidators, firstValidatorAddr) require.NoError(t, err) if firstSealed { @@ -278,7 +303,7 @@ func TestE2E_Consensus_RegisterValidator(t *testing.T) { require.NoError(t, err) // check if the second validator has signed any prposals - secondSealed, err := secondValidator.HasValidatorSealed(currentBlock, currentBlock+3*epochSize, validators, secondValidatorAddr) + secondSealed, err := secondValidator.HasValidatorSealed(currentBlock, currentBlock+3*epochSize, nextValidators, secondValidatorAddr) require.NoError(t, err) if secondSealed { diff --git a/e2e-polybft/e2e/txpool_test.go b/e2e-polybft/e2e/txpool_test.go index 7b72f87224..58e7d423cc 100644 --- a/e2e-polybft/e2e/txpool_test.go +++ b/e2e-polybft/e2e/txpool_test.go @@ -65,7 +65,7 @@ func TestE2E_TxPool_Transfer(t *testing.T) { wg.Wait() - err = cluster.WaitUntil(2*time.Minute, func() bool { + err = cluster.WaitUntil(2*time.Minute, 2*time.Second, func() bool { for _, receiver := range receivers { balance, err := client.GetBalance(receiver, ethgo.Latest) if err != nil { @@ -100,7 +100,7 @@ func TestE2E_TxPool_Transfer_Linear(t *testing.T) { require.NoError(t, err) waitUntilBalancesChanged := func(acct ethgo.Address) error { - err := cluster.WaitUntil(30*time.Second, func() bool { + err := cluster.WaitUntil(30*time.Second, 2*time.Second, func() bool { balance, err := client.GetBalance(acct, ethgo.Latest) if err != nil { return true diff --git a/e2e-polybft/framework/test-cluster.go b/e2e-polybft/framework/test-cluster.go index 65b1e1a78a..db03d76b33 100644 --- a/e2e-polybft/framework/test-cluster.go +++ b/e2e-polybft/framework/test-cluster.go @@ -468,8 +468,8 @@ func (c *TestCluster) Stats(t *testing.T) { } } -func (c *TestCluster) WaitUntil(dur time.Duration, handler func() bool) error { - timer := time.NewTimer(dur) +func (c *TestCluster) WaitUntil(timeout, pollFrequency time.Duration, handler func() bool) error { + timer := time.NewTimer(timeout) defer timer.Stop() for { @@ -478,7 +478,7 @@ func (c *TestCluster) WaitUntil(dur time.Duration, handler func() bool) error { return fmt.Errorf("timeout") case <-c.failCh: return c.executionErr - case <-time.After(2 * time.Second): + case <-time.After(pollFrequency): } if handler() { @@ -526,7 +526,7 @@ func (c *TestCluster) WaitForBlock(n uint64, timeout time.Duration) error { // WaitForGeneric waits until all running servers returns true from fn callback or timeout defined by dur occurs func (c *TestCluster) WaitForGeneric(dur time.Duration, fn func(*TestServer) bool) error { - return c.WaitUntil(dur, func() bool { + return c.WaitUntil(dur, 2*time.Second, func() bool { for _, srv := range c.Servers { // query only running servers if srv.isRunning() && !fn(srv) { diff --git a/e2e-polybft/grpc_validation_test.go b/e2e-polybft/grpc_validation_test.go index 5777dbbcc9..238fbbe71e 100644 --- a/e2e-polybft/grpc_validation_test.go +++ b/e2e-polybft/grpc_validation_test.go @@ -25,7 +25,7 @@ func TestE2E_GRPCRequestValidationTriggering(t *testing.T) { defer cluster.Stop() ctx := context.Background() - err := cluster.WaitUntil(testTimeout, func() bool { + err := cluster.WaitUntil(testTimeout, 2*time.Second, func() bool { peerList, err := cluster.Servers[0].Conn().PeersList(ctx, &emptypb.Empty{}) return err == nil && len(peerList.GetPeers()) > 0