From 5635bbc59a96e0b555654709d86e74bf19f0e950 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Sun, 21 Jan 2024 18:43:15 +0800 Subject: [PATCH 1/4] wire: add `Copy` method to `MsgBlock` --- wire/msgblock.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/wire/msgblock.go b/wire/msgblock.go index a9e688ab..ad4b306f 100644 --- a/wire/msgblock.go +++ b/wire/msgblock.go @@ -54,6 +54,20 @@ type MsgBlock struct { UData *UData } +// Copy creates a deep copy of MsgBlock. +func (msg *MsgBlock) Copy() *MsgBlock { + block := &MsgBlock{ + Header: msg.Header, + Transactions: make([]*MsgTx, len(msg.Transactions)), + } + + for i, tx := range msg.Transactions { + block.Transactions[i] = tx.Copy() + } + + return block +} + // AddTransaction adds a transaction to the message. func (msg *MsgBlock) AddTransaction(tx *MsgTx) error { msg.Transactions = append(msg.Transactions, tx) From d278773b84c31b661fd63b0f8d1a9c56993628c6 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Fri, 16 Feb 2024 16:15:27 +0900 Subject: [PATCH 2/4] 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 dce29f4e..2d87dba0 100644 --- a/blockchain/utxocache_test.go +++ b/blockchain/utxocache_test.go @@ -737,36 +737,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. @@ -777,15 +781,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 9bcf5e108b4f24e1df7e210fef8f3ffd60ef8d60 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Fri, 16 Feb 2024 15:49:11 +0900 Subject: [PATCH 3/4] 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 2d87dba0..37b7a05f 100644 --- a/blockchain/utxocache_test.go +++ b/blockchain/utxocache_test.go @@ -717,18 +717,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 3fc9c61c4b4052f0f4104676bd8970c119593c0f Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Fri, 19 Jul 2024 15:21:10 +0900 Subject: [PATCH 4/4] 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 | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/blockchain/utxocache.go b/blockchain/utxocache.go index 49872bde..97bff10a 100644 --- a/blockchain/utxocache.go +++ b/blockchain/utxocache.go @@ -726,10 +726,18 @@ func (b *BlockChain) flushNeededAfterPrune(earliestHeight int32) (bool, error) { if earliestHeight < 0 { return false, nil } - 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 return earliestHeight > lastFlushHeight, nil }