From 8836219a0202e6d7f58d4d6d8be9fd1b4c615638 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Fri, 16 Feb 2024 16:15:27 +0900 Subject: [PATCH 1/3] blockchain: return error in db.View instead of t.Fatal Calling t.Fatal inside db.View makes the test hang on failures. Return the error and call t.Fatal outside of db.View avoids this. --- blockchain/utxocache_test.go | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/blockchain/utxocache_test.go b/blockchain/utxocache_test.go index 45116655e3..64d517b09a 100644 --- a/blockchain/utxocache_test.go +++ b/blockchain/utxocache_test.go @@ -749,36 +749,40 @@ func TestFlushOnPrune(t *testing.T) { ffldb.TstRunWithMaxBlockFileSize(chain.db, maxBlockFileSize, syncBlocks) // Function that errors out if the block that should exist doesn't exist. - shouldExist := func(dbTx database.Tx, blockHash *chainhash.Hash) { + shouldExist := func(dbTx database.Tx, blockHash *chainhash.Hash) error { bytes, err := dbTx.FetchBlock(blockHash) if err != nil { - t.Fatal(err) + return err } block, err := btcutil.NewBlockFromBytes(bytes) if err != nil { - t.Fatalf("didn't find block %v. %v", blockHash, err) + return fmt.Errorf("didn't find block %v. %v", blockHash, err) } if !block.Hash().IsEqual(blockHash) { - t.Fatalf("expected to find block %v but got %v", + return fmt.Errorf("expected to find block %v but got %v", blockHash, block.Hash()) } + + return nil } // Function that errors out if the block that shouldn't exist exists. - shouldNotExist := func(dbTx database.Tx, blockHash *chainhash.Hash) { + shouldNotExist := func(dbTx database.Tx, blockHash *chainhash.Hash) error { bytes, err := dbTx.FetchBlock(chaincfg.MainNetParams.GenesisHash) if err == nil { - t.Fatalf("expected block %s to be pruned", blockHash) + return fmt.Errorf("expected block %s to be pruned", blockHash.String()) } if len(bytes) != 0 { - t.Fatalf("expected block %s to be pruned but got %v", + return fmt.Errorf("expected block %s to be pruned but got %v", blockHash, bytes) } + + return nil } // The below code checks that the correct blocks were pruned. - chain.db.View(func(dbTx database.Tx) error { + err = chain.db.View(func(dbTx database.Tx) error { exist := false for _, block := range blocks { // Blocks up to the last flush hash should not exist. @@ -789,15 +793,23 @@ func TestFlushOnPrune(t *testing.T) { } if exist { - shouldExist(dbTx, block.Hash()) + err = shouldExist(dbTx, block.Hash()) + if err != nil { + return err + } } else { - shouldNotExist(dbTx, block.Hash()) + err = shouldNotExist(dbTx, block.Hash()) + if err != nil { + return err + } } - } return nil }) + if err != nil { + t.Fatal(err) + } } func TestInitConsistentState(t *testing.T) { From a0c9e3b384ec6c2e54f91f00da5dccfea971504a Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Fri, 16 Feb 2024 15:49:11 +0900 Subject: [PATCH 2/3] blockchain: add case for pruning stale blocks Pruning stale blocks will make the block validation fail for the block that the prune was triggered on as BlockHeightByHash will not return the height for blocks that are not in the main chain. We add a test case to ensure that the test fails in the above case. --- blockchain/utxocache_test.go | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/blockchain/utxocache_test.go b/blockchain/utxocache_test.go index 64d517b09a..2e055ea5db 100644 --- a/blockchain/utxocache_test.go +++ b/blockchain/utxocache_test.go @@ -729,18 +729,33 @@ func TestFlushOnPrune(t *testing.T) { } syncBlocks := func() { + // Modify block 1 to be a different hash. This is to artificially + // create a stale branch in the chain. + staleMsgBlock := blocks[1].MsgBlock().Copy() + staleMsgBlock.Header.Nonce = 0 + staleBlock := btcutil.NewBlock(staleMsgBlock) + + // Add the stale block here to create a chain view like so. The + // block will be the main chain at first but become stale as we + // keep adding blocks. BFNoPoWCheck is given as the pow check will + // fail. + // + // (genesis block) -> 1 -> 2 -> 3 -> ... + // \-> 1a + _, _, err = chain.ProcessBlock(staleBlock, BFNoPoWCheck) + if err != nil { + t.Fatal(err) + } + for i, block := range blocks { if i == 0 { // Skip the genesis block. continue } - isMainChain, _, err := chain.ProcessBlock(block, BFNone) + _, _, err = chain.ProcessBlock(block, BFNone) if err != nil { - t.Fatal(err) - } - - if !isMainChain { - t.Fatalf("expected block %s to be on the main chain", block.Hash()) + t.Fatalf("Failed to process block %v(%v). %v", + block.Hash().String(), block.Height(), err) } } } From f2caa8fadcb21ab7656975f750f51c4222d64e41 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Thu, 15 Feb 2024 18:13:52 +0900 Subject: [PATCH 3/3] blockchain: don't rely on BlockHeightByHash for prune height calculations Since BlockHeightByHash only returns the heights for blocks that are in the main chain, when a block that is stale gets pruned, this will cause an error in the block height lookup and cause an error in block processing. Look up the node directly from the index and if the node isn't found, just skip that node. For utxoCache.lastFlushHash, if that isn't found, just force a flush. --- blockchain/utxocache.go | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/blockchain/utxocache.go b/blockchain/utxocache.go index 07dc698a91..3261f057e5 100644 --- a/blockchain/utxocache.go +++ b/blockchain/utxocache.go @@ -617,6 +617,7 @@ func (b *BlockChain) InitConsistentState(tip *blockNode, interrupt <-chan struct // Set the last flush hash as it's the default value of 0s. s.lastFlushHash = tip.hash + s.lastFlushTime = time.Now() return err } @@ -725,22 +726,35 @@ func (b *BlockChain) InitConsistentState(tip *blockNode, interrupt <-chan struct // Example: if the last flush hash was at height 100 and one of the deleted blocks was at // height 98, this function will return true. func (b *BlockChain) flushNeededAfterPrune(deletedBlockHashes []chainhash.Hash) (bool, error) { - lastFlushHeight, err := b.BlockHeightByHash(&b.utxoCache.lastFlushHash) - if err != nil { - return false, err + node := b.index.LookupNode(&b.utxoCache.lastFlushHash) + if node == nil { + // If we couldn't find the node where we last flushed at, have the utxo cache + // flush to be safe and that will set the last flush hash again. + // + // This realistically should never happen as nodes are never deleted from + // the block index. This happening likely means that there's a hardware + // error which is something we can't recover from. The best that we can + // do here is to just force a flush and hope that the newly set + // lastFlushHash doesn't error. + return true, nil } + lastFlushHeight := node.Height() + // Loop through all the block hashes and find out what the highest block height // among the deleted hashes is. highestDeletedHeight := int32(-1) for _, deletedBlockHash := range deletedBlockHashes { - height, err := b.BlockHeightByHash(&deletedBlockHash) - if err != nil { - return false, err + node := b.index.LookupNode(&deletedBlockHash) + if node == nil { + // If we couldn't find this node, just skip it and try the next + // deleted hash. This might be a corruption in the database + // but there's nothing we can do here to address it except for + // moving onto the next block. + continue } - - if height > highestDeletedHeight { - highestDeletedHeight = height + if node.height > highestDeletedHeight { + highestDeletedHeight = node.height } }