Skip to content

Commit

Permalink
Merge pull request #153 from XinFinOrg/fix-block-write-read-race-cond…
Browse files Browse the repository at this point in the history 
…ition

race condition caused by old eth bug where read before write. We fixi…
  • Loading branch information
@wjrjerome
wjrjerome authored Dec 22, 2021
2 parents f9fa3a8 + 2969548 commit 69cadcc
Show file tree
Hide file tree
Showing 7 changed files with 203 additions and 31 deletions.
17 changes: 10 additions & 7 deletions consensus/XDPoS/engines/engine_v1/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, nil)
if err != nil {
return err
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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, selfHeader *types.Header) (*SnapshotV1, error) {
// Search for a SnapshotV1 in memory or on disk for checkpoints
var (
headers []*types.Header
Expand Down Expand Up @@ -482,6 +482,9 @@ func (x *XDPoS_v1) snapshot(chain consensus.ChainReader, number uint64, hash com
return nil, consensus.ErrUnknownAncestor
}
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
header = chain.GetHeader(hash, number)
Expand Down Expand Up @@ -540,7 +543,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, nil)
if err != nil {
return err
}
Expand Down Expand Up @@ -656,7 +659,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, nil)
if err != nil {
return err
}
Expand Down Expand Up @@ -750,7 +753,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
}
Expand Down Expand Up @@ -832,7 +835,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, nil)
if err != nil {
return nil, err
}
Expand Down
21 changes: 20 additions & 1 deletion core/blockchain.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1344,6 +1344,24 @@ func (bc *BlockChain) WriteBlockWithState(block *types.Block, receipts []*types.
// Split same-difficulty blocks by number
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 {
// 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() {
Expand Down Expand Up @@ -2098,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() {
Expand Down Expand Up @@ -2142,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
}
Expand Down
2 changes: 1 addition & 1 deletion params/version.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down
2 changes: 1 addition & 1 deletion tests/consensus/adaptor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
32 changes: 16 additions & 16 deletions tests/consensus/block_signer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down
Loading

0 comments on commit 69cadcc

Please sign in to comment.