From d0cde5c51eadc1461b50a644b2a2b6b14f39c4f2 Mon Sep 17 00:00:00 2001 From: Liam Lai Date: Thu, 14 Apr 2022 02:07:26 -0600 Subject: [PATCH 1/3] fix new masternode bug --- common/types.go | 14 +++++++++----- consensus/XDPoS/engines/engine_v2/epochSwitch.go | 4 ++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/common/types.go b/common/types.go index 4c31429a53..d8db199883 100644 --- a/common/types.go +++ b/common/types.go @@ -279,19 +279,23 @@ func (a UnprefixedAddress) MarshalText() ([]byte, error) { // Extract validators from byte array. func RemoveItemFromArray(array []Address, items []Address) []Address { + // Create newArray to stop append change array value + newArray := make([]Address, len(array)) + copy(newArray, array) + if len(items) == 0 { - return array + return newArray } for _, item := range items { - for i := len(array) - 1; i >= 0; i-- { - if array[i] == item { - array = append(array[:i], array[i+1:]...) + for i := len(newArray) - 1; i >= 0; i-- { + if newArray[i] == item { + newArray = append(newArray[:i], newArray[i+1:]...) } } } - return array + return newArray } // Extract validators from byte array. diff --git a/consensus/XDPoS/engines/engine_v2/epochSwitch.go b/consensus/XDPoS/engines/engine_v2/epochSwitch.go index eb16de6bd5..da7efbcc6d 100644 --- a/consensus/XDPoS/engines/engine_v2/epochSwitch.go +++ b/consensus/XDPoS/engines/engine_v2/epochSwitch.go @@ -88,12 +88,12 @@ func (x *XDPoS_v2) isEpochSwitchAtRound(round utils.Round, parentHeader *types.H return true, epochNum, nil } - _, round, _, err := x.getExtraFields(parentHeader) + _, parentRound, _, err := x.getExtraFields(parentHeader) if err != nil { log.Error("[IsEpochSwitch] decode header error", "err", err, "header", parentHeader, "extra", common.Bytes2Hex(parentHeader.Extra)) return false, 0, err } - parentRound := round + epochStartRound := round - round%utils.Round(x.config.Epoch) return parentRound < epochStartRound, epochNum, nil } From 5764dbc249836cb0b80e401692c941fa1afb7a24 Mon Sep 17 00:00:00 2001 From: Liam Lai Date: Thu, 14 Apr 2022 02:17:30 -0600 Subject: [PATCH 2/3] update test for RemoveItemFromArray --- common/types_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/common/types_test.go b/common/types_test.go index d7d310576d..1ec2bfc26c 100644 --- a/common/types_test.go +++ b/common/types_test.go @@ -158,8 +158,12 @@ func BenchmarkAddressHex(b *testing.B) { func TestRemoveItemInArray(t *testing.T) { array := []Address{HexToAddress("0x0000003"), HexToAddress("0x0000001"), HexToAddress("0x0000002"), HexToAddress("0x0000003")} remove := []Address{HexToAddress("0x0000002"), HexToAddress("0x0000004"), HexToAddress("0x0000003")} - array = RemoveItemFromArray(array, remove) - if len(array) != 1 { + newArray := RemoveItemFromArray(array, remove) + + if array[0] != HexToAddress("0x0000003") || array[2] != HexToAddress("0x0000002") { + t.Error("should keep the original item from array address") + } + if len(newArray) != 1 { t.Error("fail remove item from array address") } } From 49cecaa9afced564cd0b320bf3a0bedc83b96312 Mon Sep 17 00:00:00 2001 From: wgr523 Date: Tue, 3 May 2022 11:46:55 +0800 Subject: [PATCH 3/3] XIN-176 fix (#85) * fix bug in isEpochSwitchAtRound, fix penalty test TestHookPenaltyV2Mining * fix authorised test * fix things * revert a test --- consensus/XDPoS/engines/engine_v2/epochSwitch.go | 4 ++++ .../XDPoS/engines/engine_v2/testing_utils.go | 9 +++++++++ consensus/tests/engine_v2_tests/penalty_test.go | 16 ++++++++++------ consensus/tests/engine_v2_tests/timeout_test.go | 4 ++-- 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/consensus/XDPoS/engines/engine_v2/epochSwitch.go b/consensus/XDPoS/engines/engine_v2/epochSwitch.go index da7efbcc6d..1bb195a5e9 100644 --- a/consensus/XDPoS/engines/engine_v2/epochSwitch.go +++ b/consensus/XDPoS/engines/engine_v2/epochSwitch.go @@ -93,6 +93,10 @@ func (x *XDPoS_v2) isEpochSwitchAtRound(round utils.Round, parentHeader *types.H log.Error("[IsEpochSwitch] decode header error", "err", err, "header", parentHeader, "extra", common.Bytes2Hex(parentHeader.Extra)) return false, 0, err } + if round <= parentRound { + // this round is no larger than parentRound, should return false + return false, epochNum, nil + } epochStartRound := round - round%utils.Round(x.config.Epoch) return parentRound < epochStartRound, epochNum, nil diff --git a/consensus/XDPoS/engines/engine_v2/testing_utils.go b/consensus/XDPoS/engines/engine_v2/testing_utils.go index 148652a1eb..934128199d 100644 --- a/consensus/XDPoS/engines/engine_v2/testing_utils.go +++ b/consensus/XDPoS/engines/engine_v2/testing_utils.go @@ -1,6 +1,7 @@ package engine_v2 import ( + "github.com/XinFinOrg/XDPoSChain/common" "github.com/XinFinOrg/XDPoSChain/consensus" "github.com/XinFinOrg/XDPoSChain/consensus/XDPoS/utils" ) @@ -73,3 +74,11 @@ func (x *XDPoS_v2) HygieneTimeoutPoolFaker() { func (x *XDPoS_v2) GetTimeoutPoolKeyListFaker() []string { return x.timeoutPool.PoolObjKeysList() } + +// Fake the signer address, the signing function is incompatible +func (x *XDPoS_v2) AuthorizeFaker(signer common.Address) { + x.signLock.Lock() + defer x.signLock.Unlock() + + x.signer = signer +} diff --git a/consensus/tests/engine_v2_tests/penalty_test.go b/consensus/tests/engine_v2_tests/penalty_test.go index a74c6c13b8..c4182f3e06 100644 --- a/consensus/tests/engine_v2_tests/penalty_test.go +++ b/consensus/tests/engine_v2_tests/penalty_test.go @@ -15,7 +15,7 @@ import ( func TestHookPenaltyV2Mining(t *testing.T) { config := params.TestXDPoSMockChainConfig - blockchain, _, _, signer, _, _ := PrepareXDCTestBlockChainForV2Engine(t, int(config.XDPoS.Epoch)*7, config, 0) + blockchain, _, _, _, _, _ := PrepareXDCTestBlockChainForV2Engine(t, int(config.XDPoS.Epoch)*7, config, 0) adaptor := blockchain.Engine().(*XDPoS.XDPoS) hooks.AttachConsensusV2Hooks(adaptor, blockchain, config) assert.NotNil(t, adaptor.EngineV2.HookPenalty) @@ -29,7 +29,8 @@ func TestHookPenaltyV2Mining(t *testing.T) { header6300 := blockchain.GetHeaderByNumber(config.XDPoS.Epoch * 7) penalty, err := adaptor.EngineV2.HookPenalty(blockchain, big.NewInt(int64(config.XDPoS.Epoch*7)), header6300.ParentHash, masternodes) assert.Nil(t, err) - // only 1 signer address not in the masternode list + // when we prepare the chain, we include all 5 signers as coinbase except one signer + // header6300 records 5 masternodes, so penalty contains 5-4=1 address assert.Equal(t, 1, len(penalty)) contains := false for _, mn := range common.RemoveItemFromArray(masternodes, penalty) { @@ -43,20 +44,23 @@ func TestHookPenaltyV2Mining(t *testing.T) { assert.Nil(t, err) err = adaptor.EngineV2.ProcessQCFaker(blockchain, extraField.QuorumCert) assert.Nil(t, err) + // coinbase is a faker signer headerMining := &types.Header{ ParentHash: header6300.ParentHash, Number: header6300.Number, GasLimit: params.TargetGasLimit, Time: header6300.Time, - Coinbase: signer, + Coinbase: acc1Addr, } // Force to make the node to be at its round to mine, otherwise won't pass the yourturn masternodes check - // We have 5 nodes in total and the node signer is always at the 4th(last) in the list. Hence int(config.XDPoS.Epoch)*7+4-900, the +4 means is to force to next 4 round and -900 is the relative round number to block number int(config.XDPoS.Epoch)*7 - adaptor.EngineV2.SetNewRoundFaker(blockchain, utils.Round(int(config.XDPoS.Epoch)*7+4-900), false) + // We have 19 nodes in total (20 candidates in snapshot - 1 penalty) and the fake signer is always at the 18th(last) in the list. Hence int(config.XDPoS.Epoch)*7+18-900, the +18 means is to force to next 18 round and -900 is the relative round number to block number int(config.XDPoS.Epoch)*7 + adaptor.EngineV2.SetNewRoundFaker(blockchain, utils.Round(int(config.XDPoS.Epoch)*7+18-900), false) + // The test default signer is not in the msaternodes, so we set the faker signer + adaptor.EngineV2.AuthorizeFaker(acc1Addr) err = adaptor.Prepare(blockchain, headerMining) assert.Nil(t, err) assert.Equal(t, 1, len(headerMining.Penalties)/common.AddressLength) - // 20 candidates (set by PrepareXDCTestBlockChainForV2Engine) - 3 penalty = 17 + // 20 candidates (set by PrepareXDCTestBlockChainForV2Engine) - 1 penalty = 19 assert.Equal(t, 19, len(headerMining.Validators)/common.AddressLength) } diff --git a/consensus/tests/engine_v2_tests/timeout_test.go b/consensus/tests/engine_v2_tests/timeout_test.go index 0b2345c09b..8663945662 100644 --- a/consensus/tests/engine_v2_tests/timeout_test.go +++ b/consensus/tests/engine_v2_tests/timeout_test.go @@ -16,14 +16,14 @@ import ( ) func TestCountdownTimeoutToSendTimeoutMessage(t *testing.T) { - blockchain, _, _, _, _, _ := PrepareXDCTestBlockChainForV2Engine(t, 2251, params.TestXDPoSMockChainConfig, 0) + blockchain, _, _, _, _, _ := PrepareXDCTestBlockChainForV2Engine(t, 901, params.TestXDPoSMockChainConfig, 0) engineV2 := blockchain.Engine().(*XDPoS.XDPoS).EngineV2 timeoutMsg := <-engineV2.BroadcastCh poolSize := engineV2.GetTimeoutPoolSizeFaker(timeoutMsg.(*utils.Timeout)) assert.Equal(t, poolSize, 1) assert.NotNil(t, timeoutMsg) - assert.Equal(t, uint64(1350), timeoutMsg.(*utils.Timeout).GapNumber) + assert.Equal(t, uint64(450), timeoutMsg.(*utils.Timeout).GapNumber) assert.Equal(t, utils.Round(1), timeoutMsg.(*utils.Timeout).Round) }