From d901d1df6e7bbd0da2865ddfda4d5de5a0c4f181 Mon Sep 17 00:00:00 2001 From: Jianrong Date: Tue, 21 Dec 2021 10:32:55 +1100 Subject: [PATCH 1/5] race condition caused by old eth bug where read before write. We fixing it by passing down header in the snapshot to avoid touching ETH code. This is just a precaution to avoid undiscovered bugs in XDC as a result of fundmental ETH implementation change --- consensus/XDPoS/engines/engine_v1/engine.go | 16 +- core/blockchain.go | 15 ++ tests/consensus/adaptor_test.go | 2 +- tests/consensus/block_signer_test.go | 32 ++-- .../blockchain_race_condition_test.go | 145 ++++++++++++++++++ tests/consensus/test_helper.go | 12 +- 6 files changed, 193 insertions(+), 29 deletions(-) create mode 100644 tests/consensus/blockchain_race_condition_test.go diff --git a/consensus/XDPoS/engines/engine_v1/engine.go b/consensus/XDPoS/engines/engine_v1/engine.go index c5e7aaf0beee..9e2146b75bf7 100644 --- a/consensus/XDPoS/engines/engine_v1/engine.go +++ b/consensus/XDPoS/engines/engine_v1/engine.go @@ -249,7 +249,7 @@ func (x *XDPoS_v1) verifyCascadingFields(chain consensus.ChainReader, header *ty when it happens we get the signers list by requesting smart contract */ // Retrieve the snapshot needed to verify this header and cache it - snap, err := x.snapshot(chain, number-1, header.ParentHash, parents) + snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, header) if err != nil { return err } @@ -336,7 +336,7 @@ func (x *XDPoS_v1) IsAuthorisedAddress(header *types.Header, chain consensus.Cha func (x *XDPoS_v1) GetSnapshot(chain consensus.ChainReader, header *types.Header) (*SnapshotV1, error) { number := header.Number.Uint64() log.Trace("get snapshot", "number", number, "hash", header.Hash()) - snap, err := x.snapshot(chain, number, header.Hash(), nil) + snap, err := x.snapshot(chain, number, header.Hash(), nil, header) if err != nil { return nil, err } @@ -435,7 +435,7 @@ func (x *XDPoS_v1) YourTurn(chain consensus.ChainReader, parent *types.Header, s } // snapshot retrieves the authorization snapshot at a given point in time. -func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash common.Hash, parents []*types.Header) (*SnapshotV1, error) { +func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash common.Hash, parents []*types.Header, currentHeader *types.Header) (*SnapshotV1, error) { // Search for a SnapshotV1 in memory or on disk for checkpoints var ( headers []*types.Header @@ -482,6 +482,8 @@ func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash com return nil, consensus.ErrUnknownAncestor } parents = parents[:len(parents)-1] + } else if currentHeader != nil && currentHeader.Hash() == hash { + header = currentHeader } else { // No explicit parents (or no more left), reach out to the database header = chain.GetHeader(hash, number) @@ -540,7 +542,7 @@ func (x *XDPoS_v1) verifySeal(chain consensus.ChainReader, header *types.Header, return utils.ErrUnknownBlock } // Retrieve the snapshot needed to verify this header and cache it - snap, err := x.snapshot(chain, number-1, header.ParentHash, parents) + snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, header) if err != nil { return err } @@ -656,7 +658,7 @@ func (x *XDPoS_v1) Prepare(chain consensus.ChainReader, header *types.Header) er number := header.Number.Uint64() // Assemble the voting snapshot to check which votes make sense - snap, err := x.snapshot(chain, number-1, header.ParentHash, nil) + snap, err := x.snapshot(chain, number-1, header.ParentHash, nil, header) if err != nil { return err } @@ -750,7 +752,7 @@ func (x *XDPoS_v1) UpdateMasternodes(chain consensus.ChainReader, header *types. number := header.Number.Uint64() log.Trace("take snapshot", "number", number, "hash", header.Hash()) // get snapshot - snap, err := x.snapshot(chain, number, header.Hash(), nil) + snap, err := x.snapshot(chain, number, header.Hash(), nil, header) if err != nil { return err } @@ -832,7 +834,7 @@ func (x *XDPoS_v1) Seal(chain consensus.ChainReader, block *types.Block, stop <- x.lock.RUnlock() // Bail out if we're unauthorized to sign a block - snap, err := x.snapshot(chain, number-1, header.ParentHash, nil) + snap, err := x.snapshot(chain, number-1, header.ParentHash, nil, block.Header()) if err != nil { return nil, err } diff --git a/core/blockchain.go b/core/blockchain.go index 9e4c84f5ca03..cf88a582a6fc 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -1344,6 +1344,21 @@ func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types. // Split same-difficulty blocks by number reorg = block.NumberU64() > currentBlock.NumberU64() } + + // if reorg { + // // Write the positional metadata for transaction and receipt lookups + // if err := WriteTxLookupEntries(batch, block); err != nil { + // return NonStatTy, err + // } + // // Write hash preimages + // if err := WritePreimages(bc.db, block.NumberU64(), state.Preimages()); err != nil { + // return NonStatTy, err + // } + // } + // if err := batch.Write(); err != nil { + // return NonStatTy, err + // } + if reorg { // Reorganise the chain if the parent is not the head block if block.ParentHash() != currentBlock.Hash() { diff --git a/tests/consensus/adaptor_test.go b/tests/consensus/adaptor_test.go index ce1b0fad3383..517574adb020 100644 --- a/tests/consensus/adaptor_test.go +++ b/tests/consensus/adaptor_test.go @@ -28,7 +28,7 @@ func TestAdaptorShouldGetAuthorForDifferentConsensusVersion(t *testing.T) { // Insert block 11 blockCoinBase := fmt.Sprintf("0x111000000000000000000000000000000%03d", 11) merkleRoot := "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block11, err := insertBlock(blockchain, 11, blockCoinBase, currentBlock, merkleRoot) + block11, err := insertBlock(blockchain, 11, blockCoinBase, currentBlock, merkleRoot, 1) if err != nil { t.Fatal(err) } diff --git a/tests/consensus/block_signer_test.go b/tests/consensus/block_signer_test.go index 8d612036d695..f15659fcca7d 100644 --- a/tests/consensus/block_signer_test.go +++ b/tests/consensus/block_signer_test.go @@ -26,7 +26,7 @@ func TestNotUpdateSignerListIfNotOnGapBlock(t *testing.T) { //Get from block validator error message merkleRoot := "46234e9cd7e85a267f7f0435b15256a794a2f6d65cc98cdbd21dcd10a01d9772" - blockA, err := insertBlockTxs(blockchain, 401, blockCoinbaseA, parentBlock, []*types.Transaction{tx}, merkleRoot) + blockA, err := insertBlockTxs(blockchain, 401, blockCoinbaseA, parentBlock, []*types.Transaction{tx}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -54,7 +54,7 @@ func TestNotChangeSingerListIfNothingProposedOrVoted(t *testing.T) { // Insert block 450 blockCoinBase := fmt.Sprintf("0x111000000000000000000000000000000%03d", 450) merkleRoot := "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block, err := insertBlock(blockchain, 450, blockCoinBase, parentBlock, merkleRoot) + block, err := insertBlock(blockchain, 450, blockCoinBase, parentBlock, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -89,7 +89,7 @@ func TestUpdateSignerListIfVotedBeforeGap(t *testing.T) { //Get from block validator error message merkleRoot := "46234e9cd7e85a267f7f0435b15256a794a2f6d65cc98cdbd21dcd10a01d9772" - block449, err := insertBlockTxs(blockchain, 449, blockCoinbaseA, parentBlock, []*types.Transaction{tx}, merkleRoot) + block449, err := insertBlockTxs(blockchain, 449, blockCoinbaseA, parentBlock, []*types.Transaction{tx}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -113,7 +113,7 @@ func TestUpdateSignerListIfVotedBeforeGap(t *testing.T) { // Now, let's mine another block to trigger the GAP block signerList update block450CoinbaseAddress := "0xaaa0000000000000000000000000000000000450" merkleRoot = "46234e9cd7e85a267f7f0435b15256a794a2f6d65cc98cdbd21dcd10a01d9772" - block450, err := insertBlock(blockchain, 450, block450CoinbaseAddress, parentBlock, merkleRoot) + block450, err := insertBlock(blockchain, 450, block450CoinbaseAddress, parentBlock, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -147,7 +147,7 @@ func TestCallUpdateM1WithSmartContractTranscation(t *testing.T) { //Get from block validator error message merkleRoot := "46234e9cd7e85a267f7f0435b15256a794a2f6d65cc98cdbd21dcd10a01d9772" - blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, currentBlock, []*types.Transaction{tx}, merkleRoot) + blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, currentBlock, []*types.Transaction{tx}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -189,7 +189,7 @@ func TestCallUpdateM1WhenForkedBlockBackToMainChain(t *testing.T) { } merkleRoot := "46234e9cd7e85a267f7f0435b15256a794a2f6d65cc98cdbd21dcd10a01d9772" - blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, currentBlock, []*types.Transaction{tx}, merkleRoot) + blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, currentBlock, []*types.Transaction{tx}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -217,7 +217,7 @@ func TestCallUpdateM1WhenForkedBlockBackToMainChain(t *testing.T) { } merkleRoot = "068dfa09d7b4093441c0cc4d9807a71bc586f6101c072d939b214c21cd136eb3" - block450B, err := insertBlockTxs(blockchain, 450, blockCoinBase450B, currentBlock, []*types.Transaction{tx}, merkleRoot) + block450B, err := insertBlockTxs(blockchain, 450, blockCoinBase450B, currentBlock, []*types.Transaction{tx}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -240,7 +240,7 @@ func TestCallUpdateM1WhenForkedBlockBackToMainChain(t *testing.T) { blockCoinBase451B := "0xbbb0000000000000000000000000000000000451" merkleRoot = "068dfa09d7b4093441c0cc4d9807a71bc586f6101c072d939b214c21cd136eb3" - block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot) + block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot, 1) if err != nil { t.Fatal(err) @@ -316,7 +316,7 @@ func TestStatesShouldBeUpdatedWhenForkedBlockBecameMainChainAtGapBlock(t *testin transferTransaction := transferTx(t, acc1Addr, 999) merkleRoot := "ea465415b60d88429f181fec9fae67c0f19cbf5a4fa10971d96d4faa57d96ffa" - blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, parentBlock, []*types.Transaction{tx, transferTransaction}, merkleRoot) + blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, parentBlock, []*types.Transaction{tx, transferTransaction}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -351,7 +351,7 @@ func TestStatesShouldBeUpdatedWhenForkedBlockBecameMainChainAtGapBlock(t *testin transferTransaction = transferTx(t, acc1Addr, 888) merkleRoot = "184edaddeafc2404248f896ae46be503ae68949896c8eb6b6ad43695581e5022" - block450B, err := insertBlockTxs(blockchain, 450, blockCoinBase450B, parentBlock, []*types.Transaction{tx, transferTransaction}, merkleRoot) + block450B, err := insertBlockTxs(blockchain, 450, blockCoinBase450B, parentBlock, []*types.Transaction{tx, transferTransaction}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -378,7 +378,7 @@ func TestStatesShouldBeUpdatedWhenForkedBlockBecameMainChainAtGapBlock(t *testin blockCoinBase451B := "0xbbb0000000000000000000000000000000000451" merkleRoot = "184edaddeafc2404248f896ae46be503ae68949896c8eb6b6ad43695581e5022" - block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot) + block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot, 1) if err != nil { t.Fatal(err) @@ -440,7 +440,7 @@ func TestVoteShouldNotBeAffectedByFork(t *testing.T) { // Insert normal blocks 450 A blockCoinBase450A := "0xaaa0000000000000000000000000000000000450" merkleRoot := "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block450A, err := insertBlock(blockchain, 450, blockCoinBase450A, parentBlock, merkleRoot) + block450A, err := insertBlock(blockchain, 450, blockCoinBase450A, parentBlock, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -453,7 +453,7 @@ func TestVoteShouldNotBeAffectedByFork(t *testing.T) { } merkleRoot = "46234e9cd7e85a267f7f0435b15256a794a2f6d65cc98cdbd21dcd10a01d9772" - block451A, err := insertBlockTxs(blockchain, 451, blockCoinbase451A, block450A, []*types.Transaction{tx}, merkleRoot) + block451A, err := insertBlockTxs(blockchain, 451, blockCoinbase451A, block450A, []*types.Transaction{tx}, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -476,21 +476,21 @@ func TestVoteShouldNotBeAffectedByFork(t *testing.T) { // Insert forked Block 450 B blockCoinBase450B := "0xbbb0000000000000000000000000000000000450" merkleRoot = "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block450B, err := insertBlock(blockchain, 450, blockCoinBase450B, parentBlock, merkleRoot) + block450B, err := insertBlock(blockchain, 450, blockCoinBase450B, parentBlock, merkleRoot, 1) if err != nil { t.Fatal(err) } blockCoinBase451B := "0xbbb0000000000000000000000000000000000451" merkleRoot = "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot) + block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot, 1) if err != nil { t.Fatal(err) } blockCoinBase452B := "0xbbb0000000000000000000000000000000000452" merkleRoot = "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block452B, err := insertBlock(blockchain, 452, blockCoinBase452B, block451B, merkleRoot) + block452B, err := insertBlock(blockchain, 452, blockCoinBase452B, block451B, merkleRoot, 1) if err != nil { t.Fatal(err) } diff --git a/tests/consensus/blockchain_race_condition_test.go b/tests/consensus/blockchain_race_condition_test.go new file mode 100644 index 000000000000..ad0e88962f4f --- /dev/null +++ b/tests/consensus/blockchain_race_condition_test.go @@ -0,0 +1,145 @@ +package consensus + +import ( + "math/big" + "testing" + + "github.com/XinFinOrg/XDPoSChain/core/types" + "github.com/XinFinOrg/XDPoSChain/params" +) + +// Snapshot try to read before blockchain is written +func TestRaceconditionOnBlockchainReadAndWrite(t *testing.T) { + + blockchain, backend, parentBlock := PrepareXDCTestBlockChain(t, GAP-1, params.TestXDPoSMockChainConfig) + + state, err := blockchain.State() + if err != nil { + t.Fatalf("Failed while trying to get blockchain state") + } + t.Logf("Account %v have balance of: %v", acc1Addr.String(), state.GetBalance(acc1Addr)) + // Check initial signer + signers, err := GetSnapshotSigner(blockchain, blockchain.CurrentBlock().Header()) + if err != nil { + t.Fatal(err) + } + if signers[acc3Addr.Hex()] != true { + debugMessage(backend, signers, t) + t.Fatalf("acc3Addr should sit in the signer list") + } + + // Insert first Block 450 A + t.Logf("Inserting block with propose and transfer at 450 A...") + blockCoinbaseA := "0xaaa0000000000000000000000000000000000450" + tx, err := voteTX(58117, 0, acc1Addr.String()) + if err != nil { + t.Fatal(err) + } + + transferTransaction := transferTx(t, acc1Addr, 999) + + merkleRoot := "ea465415b60d88429f181fec9fae67c0f19cbf5a4fa10971d96d4faa57d96ffa" + blockA, err := insertBlockTxs(blockchain, 450, blockCoinbaseA, parentBlock, []*types.Transaction{tx, transferTransaction}, merkleRoot, 1) + if err != nil { + t.Fatal(err) + } + state, err = blockchain.State() + if err != nil { + t.Fatalf("Failed while trying to get blockchain state") + } + t.Log("After transfer transaction at block 450 A, Account 1 have balance of: ", state.GetBalance(acc1Addr)) + + if state.GetBalance(acc1Addr).Cmp(new(big.Int).SetUint64(10000000999)) != 0 { + t.Fatalf("account 1 should have 10000000999 in balance") + } + + signers, err = GetSnapshotSigner(blockchain, blockA.Header()) + if err != nil { + t.Fatal(err) + } + if signers[acc1Addr.Hex()] != true { + debugMessage(backend, signers, t) + t.Fatalf("account 1 should sit in the signer list") + } + + // Insert forked Block 450 B + t.Logf("Inserting block with propose at 450 B...") + + blockCoinBase450B := "0xbbb0000000000000000000000000000000000450" + tx, err = voteTX(37117, 0, acc2Addr.String()) + if err != nil { + t.Fatal(err) + } + + transferTransaction = transferTx(t, acc1Addr, 888) + + merkleRoot = "184edaddeafc2404248f896ae46be503ae68949896c8eb6b6ad43695581e5022" + block450B, err := insertBlockTxs(blockchain, 450, blockCoinBase450B, parentBlock, []*types.Transaction{tx, transferTransaction}, merkleRoot, 2) + if err != nil { + t.Fatal(err) + } + state, err = blockchain.State() + if err != nil { + t.Fatalf("Failed while trying to get blockchain state") + } + if state.GetBalance(acc1Addr).Cmp(new(big.Int).SetUint64(10000000888)) != 0 { + t.Fatalf("account 1 should NOT have 10000000999 in balance as the block is forked, not on the main chain") + } + + signers, err = GetSnapshotSigner(blockchain, block450B.Header()) + if err != nil { + t.Fatal(err) + } + // Should run the `updateM1` for forked chain as it's now the mainchain, hence account3 NOT exit + if signers[acc3Addr.Hex()] == true { + debugMessage(backend, signers, t) + t.Fatalf("account 3 should NOT sit in the signer list as previos block result") + } + + //Insert block 451 parent is 451 B + t.Logf("Inserting block with propose at 451 B...") + + blockCoinBase451B := "0xbbb0000000000000000000000000000000000451" + merkleRoot = "184edaddeafc2404248f896ae46be503ae68949896c8eb6b6ad43695581e5022" + block451B, err := insertBlock(blockchain, 451, blockCoinBase451B, block450B, merkleRoot, 3) + + if err != nil { + t.Fatal(err) + } + + signers, err = GetSnapshotSigner(blockchain, block450B.Header()) + if err != nil { + t.Fatal(err) + } + if signers[acc2Addr.Hex()] != true { + debugMessage(backend, signers, t) + t.Fatalf("account 2 should sit in the signer list") + } + + signers, err = GetSnapshotSigner(blockchain, block451B.Header()) + if err != nil { + t.Fatal(err) + } + if signers[acc2Addr.Hex()] != true { + debugMessage(backend, signers, t) + t.Fatalf("account 2 should sit in the signer list") + } + + signers, err = GetSnapshotSigner(blockchain, blockchain.CurrentBlock().Header()) + if err != nil { + t.Fatal(err) + } + if signers[acc2Addr.Hex()] != true { + debugMessage(backend, signers, t) + t.Fatalf("acc2Addr should sit in the signer list") + } + state, err = blockchain.State() + if err != nil { + t.Fatalf("Failed while trying to get blockchain state") + } + t.Log("After transfer transaction at block 450 B and the B fork has been merged into main chain, Account 1 have balance of: ", state.GetBalance(acc1Addr)) + + if state.GetBalance(acc1Addr).Cmp(new(big.Int).SetUint64(10000000888)) != 0 { + t.Fatalf("account 1 should have 10000000888 in balance") + } +} diff --git a/tests/consensus/test_helper.go b/tests/consensus/test_helper.go index 2d587c9cc9a4..4038a09ba74f 100644 --- a/tests/consensus/test_helper.go +++ b/tests/consensus/test_helper.go @@ -236,7 +236,7 @@ func PrepareXDCTestBlockChain(t *testing.T, numOfBlocks int, chainConfig *params for i := 1; i <= numOfBlocks; i++ { blockCoinBase := fmt.Sprintf("0x111000000000000000000000000000000%03d", i) merkleRoot := "35999dded35e8db12de7e6c1471eb9670c162eec616ecebbaf4fddd4676fb930" - block, err := insertBlock(blockchain, i, blockCoinBase, currentBlock, merkleRoot) + block, err := insertBlock(blockchain, i, blockCoinBase, currentBlock, merkleRoot, 1) if err != nil { t.Fatal(err) } @@ -252,13 +252,14 @@ func PrepareXDCTestBlockChain(t *testing.T, numOfBlocks int, chainConfig *params } // insert Block without transcation attached -func insertBlock(blockchain *BlockChain, blockNum int, blockCoinBase string, parentBlock *types.Block, root string) (*types.Block, error) { +func insertBlock(blockchain *BlockChain, blockNum int, blockCoinBase string, parentBlock *types.Block, root string, difficulty int64) (*types.Block, error) { block, err := createXDPoSTestBlock( blockchain, parentBlock.Hash().Hex(), blockCoinBase, blockNum, nil, "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421", common.HexToHash(root), + difficulty, ) if err != nil { return nil, err @@ -272,13 +273,14 @@ func insertBlock(blockchain *BlockChain, blockNum int, blockCoinBase string, par } // insert Block with transcation attached -func insertBlockTxs(blockchain *BlockChain, blockNum int, blockCoinBase string, parentBlock *types.Block, txs []*types.Transaction, root string) (*types.Block, error) { +func insertBlockTxs(blockchain *BlockChain, blockNum int, blockCoinBase string, parentBlock *types.Block, txs []*types.Transaction, root string, difficulty int64) (*types.Block, error) { block, err := createXDPoSTestBlock( blockchain, parentBlock.Hash().Hex(), blockCoinBase, blockNum, txs, "0x9319777b782ba2c83a33c995481ff894ac96d9a92a1963091346a3e1e386705c", common.HexToHash(root), + difficulty, ) if err != nil { return nil, err @@ -291,7 +293,7 @@ func insertBlockTxs(blockchain *BlockChain, blockNum int, blockCoinBase string, return block, nil } -func createXDPoSTestBlock(bc *BlockChain, parentHash, coinbase string, number int, txs []*types.Transaction, receiptHash string, root common.Hash) (*types.Block, error) { +func createXDPoSTestBlock(bc *BlockChain, parentHash, coinbase string, number int, txs []*types.Transaction, receiptHash string, root common.Hash, difficulty int64) (*types.Block, error) { extraSubstring := "d7830100018358444388676f312e31342e31856c696e75780000000000000000b185dc0d0e917d18e5dbf0746be6597d3331dd27ea0554e6db433feb2e81730b20b2807d33a1527bf43cd3bc057aa7f641609c2551ebe2fd575f4db704fbf38101" // Grabbed from existing mainnet block, it does not have any meaning except for the length validation //ReceiptHash = "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421" //Root := "0xc99c095e53ff1afe3b86750affd13c7550a2d24d51fb8e41b3c3ef2ea8274bcc" @@ -304,7 +306,7 @@ func createXDPoSTestBlock(bc *BlockChain, parentHash, coinbase string, number in ReceiptHash: common.HexToHash(receiptHash), Root: root, Coinbase: common.HexToAddress(coinbase), - Difficulty: big.NewInt(int64(1)), + Difficulty: big.NewInt(difficulty), Number: big.NewInt(int64(number)), GasLimit: 1200000000, Time: big.NewInt(int64(number * 10)), From db58081b8d7f509c6cb9ed0761621d4be7ffcee9 Mon Sep 17 00:00:00 2001 From: Gerui Wang Date: Tue, 21 Dec 2021 10:19:17 +0800 Subject: [PATCH 2/5] refine race condition solution --- consensus/XDPoS/engines/engine_v1/engine.go | 14 +++++++------- tests/consensus/blockchain_race_condition_test.go | 13 ++++++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/consensus/XDPoS/engines/engine_v1/engine.go b/consensus/XDPoS/engines/engine_v1/engine.go index 9e2146b75bf7..d606c1aea3a9 100644 --- a/consensus/XDPoS/engines/engine_v1/engine.go +++ b/consensus/XDPoS/engines/engine_v1/engine.go @@ -249,7 +249,7 @@ func (x *XDPoS_v1) verifyCascadingFields(chain consensus.ChainReader, header *ty when it happens we get the signers list by requesting smart contract */ // Retrieve the snapshot needed to verify this header and cache it - snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, header) + snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, nil) if err != nil { return err } @@ -435,7 +435,7 @@ func (x *XDPoS_v1) YourTurn(chain consensus.ChainReader, parent *types.Header, s } // snapshot retrieves the authorization snapshot at a given point in time. -func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash common.Hash, parents []*types.Header, currentHeader *types.Header) (*SnapshotV1, error) { +func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash common.Hash, parents []*types.Header, selfHeader *types.Header) (*SnapshotV1, error) { // Search for a SnapshotV1 in memory or on disk for checkpoints var ( headers []*types.Header @@ -482,8 +482,8 @@ func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash com return nil, consensus.ErrUnknownAncestor } parents = parents[:len(parents)-1] - } else if currentHeader != nil && currentHeader.Hash() == hash { - header = currentHeader + } else if selfHeader != nil && selfHeader.Hash() == hash { + header = selfHeader } else { // No explicit parents (or no more left), reach out to the database header = chain.GetHeader(hash, number) @@ -542,7 +542,7 @@ func (x *XDPoS_v1) verifySeal(chain consensus.ChainReader, header *types.Header, return utils.ErrUnknownBlock } // Retrieve the snapshot needed to verify this header and cache it - snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, header) + snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, nil) if err != nil { return err } @@ -658,7 +658,7 @@ func (x *XDPoS_v1) Prepare(chain consensus.ChainReader, header *types.Header) er number := header.Number.Uint64() // Assemble the voting snapshot to check which votes make sense - snap, err := x.snapshot(chain, number-1, header.ParentHash, nil, header) + snap, err := x.snapshot(chain, number-1, header.ParentHash, nil, nil) if err != nil { return err } @@ -834,7 +834,7 @@ func (x *XDPoS_v1) Seal(chain consensus.ChainReader, block *types.Block, stop <- x.lock.RUnlock() // Bail out if we're unauthorized to sign a block - snap, err := x.snapshot(chain, number-1, header.ParentHash, nil, block.Header()) + snap, err := x.snapshot(chain, number-1, header.ParentHash, nil, nil) if err != nil { return nil, err } diff --git a/tests/consensus/blockchain_race_condition_test.go b/tests/consensus/blockchain_race_condition_test.go index ad0e88962f4f..d7c2b336d2ed 100644 --- a/tests/consensus/blockchain_race_condition_test.go +++ b/tests/consensus/blockchain_race_condition_test.go @@ -9,7 +9,7 @@ import ( ) // Snapshot try to read before blockchain is written -func TestRaceconditionOnBlockchainReadAndWrite(t *testing.T) { +func TestRaceConditionOnBlockchainReadAndWrite(t *testing.T) { blockchain, backend, parentBlock := PrepareXDCTestBlockChain(t, GAP-1, params.TestXDPoSMockChainConfig) @@ -78,22 +78,25 @@ func TestRaceconditionOnBlockchainReadAndWrite(t *testing.T) { if err != nil { t.Fatal(err) } + if blockchain.CurrentHeader().Hash() != block450B.Hash() { + t.Fatalf("the block with higher difficulty should be current header") + } state, err = blockchain.State() if err != nil { t.Fatalf("Failed while trying to get blockchain state") } if state.GetBalance(acc1Addr).Cmp(new(big.Int).SetUint64(10000000888)) != 0 { - t.Fatalf("account 1 should NOT have 10000000999 in balance as the block is forked, not on the main chain") + t.Fatalf("account 1 should have 10000000888 in balance as the block replace previous head block at number 450") } signers, err = GetSnapshotSigner(blockchain, block450B.Header()) if err != nil { t.Fatal(err) } - // Should run the `updateM1` for forked chain as it's now the mainchain, hence account3 NOT exit - if signers[acc3Addr.Hex()] == true { + // Should run the `updateM1` for forked chain as it's now the mainchain, hence account2 should exist + if signers[acc2Addr.Hex()] != true { debugMessage(backend, signers, t) - t.Fatalf("account 3 should NOT sit in the signer list as previos block result") + t.Fatalf("account 2 should sit in the signer list") } //Insert block 451 parent is 451 B From 5061dd984108fc68a50c687bbe76167901f63536 Mon Sep 17 00:00:00 2001 From: Liam Lai Date: Tue, 21 Dec 2021 08:47:23 +0300 Subject: [PATCH 3/5] add few comments in code --- consensus/XDPoS/engines/engine_v1/engine.go | 1 + core/blockchain.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/consensus/XDPoS/engines/engine_v1/engine.go b/consensus/XDPoS/engines/engine_v1/engine.go index d606c1aea3a9..2a955e0429a3 100644 --- a/consensus/XDPoS/engines/engine_v1/engine.go +++ b/consensus/XDPoS/engines/engine_v1/engine.go @@ -483,6 +483,7 @@ func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash com } parents = parents[:len(parents)-1] } else if selfHeader != nil && selfHeader.Hash() == hash { + // it prevents db doesn't have current block info, can be removed by refactor blockchain.go reorg function call. header = selfHeader } else { // No explicit parents (or no more left), reach out to the database diff --git a/core/blockchain.go b/core/blockchain.go index cf88a582a6fc..ff760d0c8b98 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -1345,6 +1345,9 @@ func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types. reorg = block.NumberU64() > currentBlock.NumberU64() } + // This is the ETH fix. We shall ultimately have this workflow, + // but due to below code has diverged significantly between ETH and XDC, and current issue we have, + // it's best to have it in a different PR with more investigations. // if reorg { // // Write the positional metadata for transaction and receipt lookups // if err := WriteTxLookupEntries(batch, block); err != nil { From 621555993ed5a787dcf0f8e64567f270a73f8d7e Mon Sep 17 00:00:00 2001 From: Anil Chinchawale Date: Tue, 21 Dec 2021 10:45:43 +0400 Subject: [PATCH 4/5] Update version.go --- params/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/params/version.go b/params/version.go index c8d19d47bf6c..70070f932a73 100644 --- a/params/version.go +++ b/params/version.go @@ -23,7 +23,7 @@ import ( const ( VersionMajor = 1 // Major version component of the current release VersionMinor = 4 // Minor version component of the current release - VersionPatch = 3 // Patch version component of the current release + VersionPatch = 4 // Patch version component of the current release VersionMeta = "stable" // Version metadata to append to the version string ) From 2969548f688ea9f93746ac0574dd0571bc0f7f56 Mon Sep 17 00:00:00 2001 From: Gerui Wang Date: Tue, 21 Dec 2021 16:06:16 +0800 Subject: [PATCH 5/5] add warn for reorg --- core/blockchain.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/blockchain.go b/core/blockchain.go index ff760d0c8b98..b90b0fe3720b 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2116,6 +2116,7 @@ func (bc *BlockChain) reorg(oldBlock, newBlock *types.Block) error { } } ) + log.Warn("Reorg", "oldBlock hash", oldBlock.Hash().Hex(), "number", oldBlock.NumberU64(), "newBlock hash", newBlock.Hash().Hex(), "number", newBlock.NumberU64()) // first reduce whoever is higher bound if oldBlock.NumberU64() > newBlock.NumberU64() { @@ -2160,7 +2161,7 @@ func (bc *BlockChain) reorg(oldBlock, newBlock *types.Block) error { } // Ensure the user sees large reorgs if len(oldChain) > 0 && len(newChain) > 0 { - logFn := log.Debug + logFn := log.Warn if len(oldChain) > 63 { logFn = log.Warn }