diff --git a/blockchain/README.md b/blockchain/README.md index e51bece252..0d50973e75 100644 --- a/blockchain/README.md +++ b/blockchain/README.md @@ -37,10 +37,6 @@ is by no means exhaustive: transaction amounts, script complexity, and merkle root calculations - Compare the block against predetermined checkpoints for expected timestamps and difficulty based on elapsed time since the checkpoint - - Save the most recent orphan blocks for a limited time in case their parent - blocks become available - - Stop processing if the block is an orphan as the rest of the processing - depends on the block's position within the block chain - Perform a series of more thorough checks that depend on the block's position within the block chain such as verifying block difficulties adhere to difficulty retarget rules, timestamps are after the median of the last diff --git a/blockchain/chain.go b/blockchain/chain.go index 986f9ca2fd..1e5d2ec7de 100644 --- a/blockchain/chain.go +++ b/blockchain/chain.go @@ -26,10 +26,6 @@ import ( ) const ( - // maxOrphanBlocks is the maximum number of orphan blocks that can be - // queued. - maxOrphanBlocks = 500 - // minMemoryNodes is the minimum number of consecutive nodes needed // in memory in order to perform all necessary validation. It is used // to determine when it's safe to prune nodes from memory without @@ -77,14 +73,6 @@ func panicf(format string, args ...interface{}) { // [17a 16a 15 14 13 12 11 10 9 8 7 6 4 genesis] type BlockLocator []*chainhash.Hash -// orphanBlock represents a block that we don't yet have the parent for. It -// is a normal block plus an expiration time to prevent caching the orphan -// forever. -type orphanBlock struct { - block *dcrutil.Block - expiration time.Time -} - // BestState houses information about the current best block and other info // related to the state of the main chain as it exists from the point of view of // the current best block. @@ -138,10 +126,10 @@ func newBestState(node *blockNode, blockSize, numTxns, totalTxns uint64, } } -// BlockChain provides functions for working with the Decred block chain. -// It includes functionality such as rejecting duplicate blocks, ensuring blocks -// follow all rules, orphan handling, checkpoint handling, and best chain -// selection with reorganization. +// BlockChain provides functions for working with the Decred block chain. It +// includes functionality such as rejecting duplicate blocks, ensuring blocks +// follow all rules, checkpoint handling, and best chain selection with +// reorganization. type BlockChain struct { // The following fields are set when the instance is created and can't // be changed afterwards, so there is no need to protect them with a @@ -181,13 +169,6 @@ type BlockChain struct { index *blockIndex bestChain *chainView - // These fields are related to handling of orphan blocks. They are - // protected by a combination of the chain lock and the orphan lock. - orphanLock sync.RWMutex - orphans map[chainhash.Hash]*orphanBlock - prevOrphans map[chainhash.Hash][]*orphanBlock - oldestOrphan *orphanBlock - // The block cache for main chain blocks to facilitate faster chain reorgs // and more efficient recent block serving. mainChainBlockCacheLock sync.RWMutex @@ -360,11 +341,11 @@ func (b *BlockChain) DisableVerify(disable bool) { // HaveBlock returns whether or not the chain instance has the block represented // by the passed hash. This includes checking the various places a block can -// be like part of the main chain, on a side chain, or in the orphan pool. +// be like part of the main chain or on a side chain. // // This function is safe for concurrent access. func (b *BlockChain) HaveBlock(hash *chainhash.Hash) bool { - return b.index.HaveBlock(hash) || b.IsKnownOrphan(hash) + return b.index.HaveBlock(hash) } // ChainWork returns the total work up to and including the block of the @@ -378,136 +359,6 @@ func (b *BlockChain) ChainWork(hash *chainhash.Hash) (*big.Int, error) { return node.workSum, nil } -// IsKnownOrphan returns whether the passed hash is currently a known orphan. -// Keep in mind that only a limited number of orphans are held onto for a -// limited amount of time, so this function must not be used as an absolute -// way to test if a block is an orphan block. A full block (as opposed to just -// its hash) must be passed to ProcessBlock for that purpose. However, calling -// ProcessBlock with an orphan that already exists results in an error, so this -// function provides a mechanism for a caller to intelligently detect *recent* -// duplicate orphans and react accordingly. -// -// This function is safe for concurrent access. -func (b *BlockChain) IsKnownOrphan(hash *chainhash.Hash) bool { - // Protect concurrent access. Using a read lock only so multiple - // readers can query without blocking each other. - b.orphanLock.RLock() - _, exists := b.orphans[*hash] - b.orphanLock.RUnlock() - - return exists -} - -// GetOrphanRoot returns the head of the chain for the provided hash from the -// map of orphan blocks. -// -// This function is safe for concurrent access. -func (b *BlockChain) GetOrphanRoot(hash *chainhash.Hash) *chainhash.Hash { - // Protect concurrent access. Using a read lock only so multiple - // readers can query without blocking each other. - b.orphanLock.RLock() - defer b.orphanLock.RUnlock() - - // Keep looping while the parent of each orphaned block is - // known and is an orphan itself. - orphanRoot := hash - prevHash := hash - for { - orphan, exists := b.orphans[*prevHash] - if !exists { - break - } - orphanRoot = prevHash - prevHash = &orphan.block.MsgBlock().Header.PrevBlock - } - - return orphanRoot -} - -// removeOrphanBlock removes the passed orphan block from the orphan pool and -// previous orphan index. -func (b *BlockChain) removeOrphanBlock(orphan *orphanBlock) { - // Protect concurrent access. - b.orphanLock.Lock() - defer b.orphanLock.Unlock() - - // Remove the orphan block from the orphan pool. - orphanHash := orphan.block.Hash() - delete(b.orphans, *orphanHash) - - // Remove the reference from the previous orphan index too. An indexing - // for loop is intentionally used over a range here as range does not - // reevaluate the slice on each iteration nor does it adjust the index - // for the modified slice. - prevHash := &orphan.block.MsgBlock().Header.PrevBlock - orphans := b.prevOrphans[*prevHash] - for i := 0; i < len(orphans); i++ { - hash := orphans[i].block.Hash() - if hash.IsEqual(orphanHash) { - copy(orphans[i:], orphans[i+1:]) - orphans[len(orphans)-1] = nil - orphans = orphans[:len(orphans)-1] - i-- - } - } - b.prevOrphans[*prevHash] = orphans - - // Remove the map entry altogether if there are no longer any orphans - // which depend on the parent hash. - if len(b.prevOrphans[*prevHash]) == 0 { - delete(b.prevOrphans, *prevHash) - } -} - -// addOrphanBlock adds the passed block (which is already determined to be -// an orphan prior calling this function) to the orphan pool. It lazily cleans -// up any expired blocks so a separate cleanup poller doesn't need to be run. -// It also imposes a maximum limit on the number of outstanding orphan -// blocks and will remove the oldest received orphan block if the limit is -// exceeded. -func (b *BlockChain) addOrphanBlock(block *dcrutil.Block) { - // Remove expired orphan blocks. - for _, oBlock := range b.orphans { - if time.Now().After(oBlock.expiration) { - b.removeOrphanBlock(oBlock) - continue - } - - // Update the oldest orphan block pointer so it can be discarded - // in case the orphan pool fills up. - if b.oldestOrphan == nil || - oBlock.expiration.Before(b.oldestOrphan.expiration) { - b.oldestOrphan = oBlock - } - } - - // Limit orphan blocks to prevent memory exhaustion. - if len(b.orphans)+1 > maxOrphanBlocks { - // Remove the oldest orphan to make room for the new one. - b.removeOrphanBlock(b.oldestOrphan) - b.oldestOrphan = nil - } - - // Protect concurrent access. This is intentionally done here instead - // of near the top since removeOrphanBlock does its own locking and - // the range iterator is not invalidated by removing map entries. - b.orphanLock.Lock() - defer b.orphanLock.Unlock() - - // Insert the block into the orphan map with an expiration time - // 1 hour from now. - expiration := time.Now().Add(time.Hour) - oBlock := &orphanBlock{ - block: block, - expiration: expiration, - } - b.orphans[*block.Hash()] = oBlock - - // Add to previous hash lookup index for faster dependency lookups. - prevHash := &block.MsgBlock().Header.PrevBlock - b.prevOrphans[*prevHash] = append(b.prevOrphans[*prevHash], oBlock) -} - // TipGeneration returns the entire generation of blocks stemming from the // parent of the current tip. // @@ -570,14 +421,6 @@ func (b *BlockChain) fetchBlockByNode(node *blockNode) (*dcrutil.Block, error) { return block, nil } - // Check orphan cache. - b.orphanLock.RLock() - orphan, existsOrphans := b.orphans[node.hash] - b.orphanLock.RUnlock() - if existsOrphans { - return orphan.block, nil - } - // Load the block from the database. err := b.db.View(func(dbTx database.Tx) error { var err error @@ -2163,8 +2006,6 @@ func New(ctx context.Context, config *Config) (*BlockChain, error) { subsidyCache: subsidyCache, index: newBlockIndex(config.DB), bestChain: newChainView(nil), - orphans: make(map[chainhash.Hash]*orphanBlock), - prevOrphans: make(map[chainhash.Hash][]*orphanBlock), mainChainBlockCache: make(map[chainhash.Hash]*dcrutil.Block), deploymentCaches: newThresholdCaches(params), isVoterMajorityVersionCache: make(map[[stakeMajorityCacheKeySize]byte]bool), diff --git a/blockchain/chain_test.go b/blockchain/chain_test.go index a4be7ac49a..fe423ca43d 100644 --- a/blockchain/chain_test.go +++ b/blockchain/chain_test.go @@ -101,7 +101,7 @@ func TestBlockchainFunctions(t *testing.T) { t.Errorf("NewBlockFromBytes error: %v", err.Error()) } - _, _, err = chain.ProcessBlock(bl, BFNone) + _, err = chain.ProcessBlock(bl, BFNone) if err != nil { t.Fatalf("ProcessBlock error at height %v: %v", i, err.Error()) } diff --git a/blockchain/common_test.go b/blockchain/common_test.go index 1a7cb64950..a0c14f07b2 100644 --- a/blockchain/common_test.go +++ b/blockchain/common_test.go @@ -343,25 +343,20 @@ func (g *chaingenHarness) AcceptBlock(blockName string) { g.t.Logf("Testing block %s (hash %s, height %d)", blockName, block.Hash(), blockHeight) - forkLen, isOrphan, err := g.chain.ProcessBlock(block, BFNone) + forkLen, err := g.chain.ProcessBlock(block, BFNone) if err != nil { g.t.Fatalf("block %q (hash %s, height %d) should have been accepted: %v", blockName, block.Hash(), blockHeight, err) } - // Ensure the main chain and orphan flags match the values specified in the - // test. - isMainChain := !isOrphan && forkLen == 0 + // Ensure the block was accepted to the main chain as indicated by a fork + // length of zero. + isMainChain := forkLen == 0 if !isMainChain { g.t.Fatalf("block %q (hash %s, height %d) unexpected main chain flag "+ "-- got %v, want true", blockName, block.Hash(), blockHeight, isMainChain) } - if isOrphan { - g.t.Fatalf("block %q (hash %s, height %d) unexpected orphan flag -- "+ - "got %v, want false", blockName, block.Hash(), blockHeight, - isOrphan) - } } // AcceptTipBlock processes the current tip block associated with the harness @@ -383,7 +378,7 @@ func (g *chaingenHarness) RejectBlock(blockName string, code ErrorCode) { g.t.Logf("Testing block %s (hash %s, height %d)", blockName, block.Hash(), blockHeight) - _, _, err := g.chain.ProcessBlock(block, BFNone) + _, err := g.chain.ProcessBlock(block, BFNone) if err == nil { g.t.Fatalf("block %q (hash %s, height %d) should not have been accepted", blockName, block.Hash(), blockHeight) @@ -440,25 +435,20 @@ func (g *chaingenHarness) AcceptedToSideChainWithExpectedTip(tipName string) { g.t.Logf("Testing block %s (hash %s, height %d)", g.TipName(), block.Hash(), blockHeight) - forkLen, isOrphan, err := g.chain.ProcessBlock(block, BFNone) + forkLen, err := g.chain.ProcessBlock(block, BFNone) if err != nil { g.t.Fatalf("block %q (hash %s, height %d) should have been accepted: %v", g.TipName(), block.Hash(), blockHeight, err) } - // Ensure the main chain and orphan flags match the values specified in - // the test. - isMainChain := !isOrphan && forkLen == 0 + // Ensure the block was accepted to a side chain as indicated by a non-zero + // fork length. + isMainChain := forkLen == 0 if isMainChain { g.t.Fatalf("block %q (hash %s, height %d) unexpected main chain flag "+ "-- got %v, want false", g.TipName(), block.Hash(), blockHeight, isMainChain) } - if isOrphan { - g.t.Fatalf("block %q (hash %s, height %d) unexpected orphan flag -- "+ - "got %v, want false", g.TipName(), block.Hash(), blockHeight, - isOrphan) - } g.ExpectTip(tipName) } diff --git a/blockchain/doc.go b/blockchain/doc.go index 46f01ac1ee..2ff1812f49 100644 --- a/blockchain/doc.go +++ b/blockchain/doc.go @@ -15,13 +15,12 @@ extremely important that fully validating nodes agree on all rules. At a high level, this package provides support for inserting new blocks into the block chain according to the aforementioned rules. It includes functionality such as rejecting duplicate blocks, ensuring blocks and transactions follow all -rules, orphan handling, and best chain selection along with reorganization. +rules, and best chain selection along with reorganization. Since this package does not deal with other Decred specifics such as network communication or wallets, it provides a notification system which gives the caller a high level of flexibility in how they want to react to certain events -such as orphan blocks which need their parents requested and newly connected -main chain blocks which might result in wallet updates. +such as newly connected main chain blocks which might result in wallet updates. Decred Chain Processing Overview @@ -36,10 +35,6 @@ is by no means exhaustive: transaction amounts, script complexity, and merkle root calculations - Compare the block against predetermined checkpoints for expected timestamps and difficulty based on elapsed time since the checkpoint - - Save the most recent orphan blocks for a limited time in case their parent - blocks become available - - Stop processing if the block is an orphan as the rest of the processing - depends on the block's position within the block chain - Perform a series of more thorough checks that depend on the block's position within the block chain such as verifying block difficulties adhere to difficulty retarget rules, timestamps are after the median of the last diff --git a/blockchain/error.go b/blockchain/error.go index 14940aa181..c53f790379 100644 --- a/blockchain/error.go +++ b/blockchain/error.go @@ -634,3 +634,10 @@ func (e RuleError) Error() string { func ruleError(c ErrorCode, desc string) RuleError { return RuleError{ErrorCode: c, Description: desc} } + +// IsErrorCode returns whether or not the provided error is a rule error with +// the provided error code. +func IsErrorCode(err error, c ErrorCode) bool { + e, ok := err.(RuleError) + return ok && e.ErrorCode == c +} diff --git a/blockchain/error_test.go b/blockchain/error_test.go index fec5a7be40..7c29ba7f26 100644 --- a/blockchain/error_test.go +++ b/blockchain/error_test.go @@ -162,3 +162,46 @@ func TestRuleError(t *testing.T) { } } } + +// TestIsErrorCode ensures IsErrorCode works as intended. +func TestIsErrorCode(t *testing.T) { + tests := []struct { + name string + err error + code ErrorCode + want bool + }{{ + name: "ErrUnexpectedDifficulty testing for ErrUnexpectedDifficulty", + err: ruleError(ErrUnexpectedDifficulty, ""), + code: ErrUnexpectedDifficulty, + want: true, + }, { + name: "ErrHighHash testing for ErrHighHash", + err: ruleError(ErrHighHash, ""), + code: ErrHighHash, + want: true, + }, { + name: "ErrHighHash error testing for ErrUnexpectedDifficulty", + err: ruleError(ErrHighHash, ""), + code: ErrUnexpectedDifficulty, + want: false, + }, { + name: "ErrHighHash error testing for unknown error code", + err: ruleError(ErrHighHash, ""), + code: 0xffff, + want: false, + }, { + name: "nil error testing for ErrUnexpectedDifficulty", + err: nil, + code: ErrUnexpectedDifficulty, + want: false, + }} + for _, test := range tests { + result := IsErrorCode(test.err, test.code) + if result != test.want { + t.Errorf("%s: unexpected result -- got: %v want: %v", test.name, + result, test.want) + continue + } + } +} diff --git a/blockchain/example_test.go b/blockchain/example_test.go index 48044b7f72..7c7cc5f50d 100644 --- a/blockchain/example_test.go +++ b/blockchain/example_test.go @@ -62,15 +62,14 @@ func ExampleBlockChain_ProcessBlock() { // cause an error by trying to process the genesis block which already // exists. genesisBlock := dcrutil.NewBlock(mainNetParams.GenesisBlock) - forkLen, isOrphan, err := chain.ProcessBlock(genesisBlock, + forkLen, err := chain.ProcessBlock(genesisBlock, blockchain.BFNone) if err != nil { fmt.Printf("Failed to create chain instance: %v\n", err) return } - isMainChain := !isOrphan && forkLen == 0 + isMainChain := forkLen == 0 fmt.Printf("Block accepted. Is it on the main chain?: %v", isMainChain) - fmt.Printf("Block accepted. Is it an orphan?: %v", isOrphan) // This output is dependent on the genesis block, and needs to be // updated if the mainnet genesis block is updated. diff --git a/blockchain/fullblocks_test.go b/blockchain/fullblocks_test.go index 43046749cb..8c6d928a54 100644 --- a/blockchain/fullblocks_test.go +++ b/blockchain/fullblocks_test.go @@ -140,12 +140,16 @@ func TestFullBlocks(t *testing.T) { t.Logf("Testing block %s (hash %s, height %d)", item.Name, block.Hash(), blockHeight) - forkLen, isOrphan, err := chain.ProcessBlock(block, - blockchain.BFNone) + var isOrphan bool + forkLen, err := chain.ProcessBlock(block, blockchain.BFNone) + if blockchain.IsErrorCode(err, blockchain.ErrMissingParent) { + isOrphan = true + err = nil + } if err != nil { - t.Fatalf("block %q (hash %s, height %d) should "+ - "have been accepted: %v", item.Name, - block.Hash(), blockHeight, err) + t.Fatalf("block %q (hash %s, height %d) should have "+ + "been accepted: %v", item.Name, block.Hash(), + blockHeight, err) } // Ensure the main chain and orphan flags match the values @@ -174,7 +178,7 @@ func TestFullBlocks(t *testing.T) { t.Logf("Testing block %s (hash %s, height %d)", item.Name, block.Hash(), blockHeight) - _, _, err := chain.ProcessBlock(block, blockchain.BFNone) + _, err := chain.ProcessBlock(block, blockchain.BFNone) if err == nil { t.Fatalf("block %q (hash %s, height %d) should not "+ "have been accepted", item.Name, block.Hash(), @@ -231,9 +235,11 @@ func TestFullBlocks(t *testing.T) { t.Logf("Testing block %s (hash %s, height %d)", item.Name, block.Hash(), blockHeight) - _, isOrphan, err := chain.ProcessBlock(block, blockchain.BFNone) + _, err := chain.ProcessBlock(block, blockchain.BFNone) if err != nil { - // Ensure the error code is of the expected type. + // Ensure the error code is of the expected type. Note + // that orphans are rejected with ErrMissingParent, so + // this check covers both conditions. if _, ok := err.(blockchain.RuleError); !ok { t.Fatalf("block %q (hash %s, height %d) "+ "returned unexpected error type -- "+ @@ -242,12 +248,6 @@ func TestFullBlocks(t *testing.T) { err) } } - - if !isOrphan { - t.Fatalf("block %q (hash %s, height %d) was accepted, "+ - "but is not considered an orphan", item.Name, - block.Hash(), blockHeight) - } } // testExpectedTip ensures the current tip of the blockchain is the diff --git a/blockchain/process.go b/blockchain/process.go index 3131dcc881..87e617b660 100644 --- a/blockchain/process.go +++ b/blockchain/process.go @@ -10,7 +10,6 @@ import ( "time" "github.com/decred/dcrd/blockchain/standalone" - "github.com/decred/dcrd/chaincfg/chainhash" "github.com/decred/dcrd/dcrutil/v3" ) @@ -34,79 +33,21 @@ const ( BFNone BehaviorFlags = 0 ) -// processOrphans determines if there are any orphans which depend on the passed -// block hash (they are no longer orphans if true) and potentially accepts them. -// It repeats the process for the newly accepted blocks (to detect further -// orphans which may no longer be orphans) until there are no more. -// -// The flags do not modify the behavior of this function directly, however they -// are needed to pass along to maybeAcceptBlock. -// -// This function MUST be called with the chain state lock held (for writes). -func (b *BlockChain) processOrphans(hash *chainhash.Hash, flags BehaviorFlags) error { - // Start with processing at least the passed hash. Leave a little room - // for additional orphan blocks that need to be processed without - // needing to grow the array in the common case. - processHashes := make([]*chainhash.Hash, 0, 10) - processHashes = append(processHashes, hash) - for len(processHashes) > 0 { - // Pop the first hash to process from the slice. - processHash := processHashes[0] - processHashes[0] = nil // Prevent GC leak. - processHashes = processHashes[1:] - - // Look up all orphans that are parented by the block we just - // accepted. This will typically only be one, but it could - // be multiple if multiple blocks are mined and broadcast - // around the same time. The one with the most proof of work - // will eventually win out. An indexing for loop is - // intentionally used over a range here as range does not - // reevaluate the slice on each iteration nor does it adjust the - // index for the modified slice. - for i := 0; i < len(b.prevOrphans[*processHash]); i++ { - orphan := b.prevOrphans[*processHash][i] - if orphan == nil { - log.Warnf("Found a nil entry at index %d in the "+ - "orphan dependency list for block %v", i, - processHash) - continue - } - - // Remove the orphan from the orphan pool. - orphanHash := orphan.block.Hash() - b.removeOrphanBlock(orphan) - i-- - - // Potentially accept the block into the block chain. - _, err := b.maybeAcceptBlock(orphan.block, flags) - if err != nil { - return err - } - - // Add this block to the list of blocks to process so - // any orphan blocks that depend on this block are - // handled too. - processHashes = append(processHashes, orphanHash) - } - } - return nil -} - // ProcessBlock is the main workhorse for handling insertion of new blocks into // the block chain. It includes functionality such as rejecting duplicate -// blocks, ensuring blocks follow all rules, orphan handling, and insertion into -// the block chain along with best chain selection and reorganization. +// blocks, ensuring blocks follow all rules, and insertion into the block chain +// along with best chain selection and reorganization. +// +// It is up to the caller to ensure the blocks are processed in order since +// orphans are rejected. // // When no errors occurred during processing, the first return value indicates // the length of the fork the block extended. In the case it either extended // the best chain or is now the tip of the best chain due to causing a -// reorganize, the fork length will be 0. The second return value indicates -// whether or not the block is an orphan, in which case the fork length will -// also be zero as expected, because it, by definition, does not connect to the -// best chain. +// reorganize, the fork length will be 0. // // This function is safe for concurrent access. -func (b *BlockChain) ProcessBlock(block *dcrutil.Block, flags BehaviorFlags) (int64, bool, error) { +func (b *BlockChain) ProcessBlock(block *dcrutil.Block, flags BehaviorFlags) (int64, error) { b.chainLock.Lock() defer b.chainLock.Unlock() @@ -124,19 +65,13 @@ func (b *BlockChain) ProcessBlock(block *dcrutil.Block, flags BehaviorFlags) (in // The block must not already exist in the main chain or side chains. if b.index.HaveBlock(blockHash) { str := fmt.Sprintf("already have block %v", blockHash) - return 0, false, ruleError(ErrDuplicateBlock, str) - } - - // The block must not already exist as an orphan. - if _, exists := b.orphans[*blockHash]; exists { - str := fmt.Sprintf("already have block (orphan) %v", blockHash) - return 0, false, ruleError(ErrDuplicateBlock, str) + return 0, ruleError(ErrDuplicateBlock, str) } // Perform preliminary sanity checks on the block and its transactions. err := checkBlockSanity(block, b.timeSource, flags, b.chainParams) if err != nil { - return 0, false, err + return 0, err } // Find the previous checkpoint and perform some additional checks based @@ -148,7 +83,7 @@ func (b *BlockChain) ProcessBlock(block *dcrutil.Block, flags BehaviorFlags) (in blockHeader := &block.MsgBlock().Header checkpointNode, err := b.findPreviousCheckpoint() if err != nil { - return 0, false, err + return 0, err } if checkpointNode != nil { // Ensure the block timestamp is after the checkpoint timestamp. @@ -157,7 +92,7 @@ func (b *BlockChain) ProcessBlock(block *dcrutil.Block, flags BehaviorFlags) (in str := fmt.Sprintf("block %v has timestamp %v before "+ "last checkpoint timestamp %v", blockHash, blockHeader.Timestamp, checkpointTime) - return 0, false, ruleError(ErrCheckpointTimeTooOld, str) + return 0, ruleError(ErrCheckpointTimeTooOld, str) } if !fastAdd { @@ -175,39 +110,28 @@ func (b *BlockChain) ProcessBlock(block *dcrutil.Block, flags BehaviorFlags) (in str := fmt.Sprintf("block target difficulty of %064x "+ "is too low when compared to the previous "+ "checkpoint", currentTarget) - return 0, false, ruleError(ErrDifficultyTooLow, str) + return 0, ruleError(ErrDifficultyTooLow, str) } } } - // Handle orphan blocks. + // This function should never be called with orphans or the genesis block. prevHash := &blockHeader.PrevBlock if !b.index.HaveBlock(prevHash) { - log.Infof("Adding orphan block %v with parent %v", blockHash, - prevHash) - b.addOrphanBlock(block) - // The fork length of orphans is unknown since they, by definition, do // not connect to the best chain. - return 0, true, nil + str := fmt.Sprintf("previous block %s is not known", prevHash) + return 0, ruleError(ErrMissingParent, str) } // The block has passed all context independent checks and appears sane // enough to potentially accept it into the block chain. forkLen, err := b.maybeAcceptBlock(block, flags) if err != nil { - return 0, false, err - } - - // Accept any orphan blocks that depend on this block (they are no - // longer orphans) and repeat for those accepted blocks until there are - // no more. - err = b.processOrphans(blockHash, flags) - if err != nil { - return 0, false, err + return 0, err } log.Debugf("Accepted block %v", blockHash) - return forkLen, false, nil + return forkLen, nil } diff --git a/blockchain/process_test.go b/blockchain/process_test.go index ca8efe91e7..2504324225 100644 --- a/blockchain/process_test.go +++ b/blockchain/process_test.go @@ -11,7 +11,6 @@ import ( "github.com/decred/dcrd/blockchain/v3/chaingen" "github.com/decred/dcrd/chaincfg/chainhash" "github.com/decred/dcrd/chaincfg/v2" - "github.com/decred/dcrd/dcrutil/v3" "github.com/decred/dcrd/wire" ) @@ -24,38 +23,6 @@ func TestProcessOrder(t *testing.T) { g, teardownFunc := newChaingenHarness(t, params, "processordertest") defer teardownFunc() - // Define additional convenience helper function to process the current tip - // block associated with the generator. - // - // orphaned expects the block to be accepted as an orphan. - orphaned := func() { - msgBlock := g.Tip() - blockHeight := msgBlock.Header.Height - block := dcrutil.NewBlock(msgBlock) - t.Logf("Testing orphan block %s (hash %s, height %d)", g.TipName(), - block.Hash(), blockHeight) - - forkLen, isOrphan, err := g.chain.ProcessBlock(block, BFNone) - if err != nil { - g.t.Fatalf("block %q (hash %s, height %d) not accepted: %v", - g.TipName(), block.Hash(), blockHeight, err) - } - - // Ensure the main chain and orphan flags match the values specified in - // the test. - isMainChain := !isOrphan && forkLen == 0 - if isMainChain { - g.t.Fatalf("block %q (hash %s, height %d) unexpected main chain "+ - "flag -- got %v, want true", g.TipName(), block.Hash(), - blockHeight, isMainChain) - } - if !isOrphan { - g.t.Fatalf("block %q (hash %s, height %d) unexpected orphan flag "+ - "-- got %v, want false", g.TipName(), block.Hash(), blockHeight, - isOrphan) - } - } - // Shorter versions of useful params for convenience. coinbaseMaturity := params.CoinbaseMaturity stakeValidationHeight := params.StakeValidationHeight @@ -115,7 +82,7 @@ func TestProcessOrder(t *testing.T) { g.NextBlock("borphan0", outs[1], ticketOuts[1], func(b *wire.MsgBlock) { b.Header.PrevBlock = chainhash.Hash{} }) - orphaned() + g.RejectTipBlock(ErrMissingParent) // Create valid orphan block. // @@ -124,13 +91,20 @@ func TestProcessOrder(t *testing.T) { g.SetTip("b1") g.NextBlock("borphanbase", outs[1], ticketOuts[1]) g.NextBlock("borphan1", outs[2], ticketOuts[2]) - orphaned() + g.RejectTipBlock(ErrMissingParent) // Ensure duplicate orphan blocks are rejected. - g.RejectTipBlock(ErrDuplicateBlock) + g.RejectTipBlock(ErrMissingParent) // --------------------------------------------------------------------- // Out-of-order forked reorg to invalid block tests. + // + // NOTE: These tests have been modified to be processed in order despite + // the comments for now due to the removal of orphan processing. They + // are otherwise left intact because future commits will allow headers + // to be processed independently from the block data which will allow + // out of order processing of data again so long as the headers are + // processed in order. // --------------------------------------------------------------------- // Create a fork that ends with block that generates too much proof-of-work @@ -146,14 +120,14 @@ func TestProcessOrder(t *testing.T) { g.SetTip("b1") g.NextBlock("bpw1", outs[1], ticketOuts[1]) + g.AcceptedToSideChainWithExpectedTip("b2") g.NextBlock("bpw2", outs[2], ticketOuts[2]) - orphaned() g.NextBlock("bpw3", outs[3], ticketOuts[3], func(b *wire.MsgBlock) { // Increase the first proof-of-work coinbase subsidy. b.Transactions[0].TxOut[2].Value += 1 }) - orphaned() - g.RejectBlock("bpw1", ErrBadCoinbaseValue) + g.AcceptBlock("bpw2") + g.RejectBlock("bpw3", ErrBadCoinbaseValue) g.ExpectTip("bpw2") // Create a fork that ends with block that generates too much dev-org @@ -164,13 +138,13 @@ func TestProcessOrder(t *testing.T) { // (bdc1 added last) g.SetTip("bpw1") g.NextBlock("bdc1", outs[2], ticketOuts[2]) + g.AcceptedToSideChainWithExpectedTip("bpw2") g.NextBlock("bdc2", outs[3], ticketOuts[3]) - orphaned() g.NextBlock("bdc3", outs[4], ticketOuts[4], func(b *wire.MsgBlock) { // Increase the proof-of-work dev subsidy by the provided amount. b.Transactions[0].TxOut[0].Value += 1 }) - orphaned() - g.RejectBlock("bdc1", ErrNoTax) + g.AcceptBlock("bdc2") + g.RejectBlock("bdc3", ErrNoTax) g.ExpectTip("bdc2") } diff --git a/blockchain/validate_test.go b/blockchain/validate_test.go index 241d1f31c5..d6e8db0a77 100644 --- a/blockchain/validate_test.go +++ b/blockchain/validate_test.go @@ -89,11 +89,11 @@ func TestBlockchainSpendJournal(t *testing.T) { t.Fatalf("NewBlockFromBytes error: %v", err.Error()) } - forkLen, isOrphan, err := chain.ProcessBlock(bl, BFNone) + forkLen, err := chain.ProcessBlock(bl, BFNone) if err != nil { t.Fatalf("ProcessBlock error at height %v: %v", i, err.Error()) } - isMainChain := !isOrphan && forkLen == 0 + isMainChain := forkLen == 0 if !isMainChain { t.Fatalf("block %s (height %d) should have been "+ "accepted to the main chain", bl.Hash(), diff --git a/blockmanager.go b/blockmanager.go index ad70139fa2..d56d380f50 100644 --- a/blockmanager.go +++ b/blockmanager.go @@ -33,6 +33,10 @@ const ( // more. minInFlightBlocks = 10 + // maxOrphanBlocks is the maximum number of orphan blocks that can be + // queued. + maxOrphanBlocks = 500 + // blockDbNamePrefix is the prefix for the block database name. The // database type is appended to this value to form the full block // database name. @@ -298,6 +302,14 @@ type peerSyncState struct { requestedBlocks map[chainhash.Hash]struct{} } +// orphanBlock represents a block for which the parent is not yet available. It +// is a normal block plus an expiration time to prevent caching the orphan +// forever. +type orphanBlock struct { + block *dcrutil.Block + expiration time.Time +} + // blockManager provides a concurrency safe block manager for handling all // incoming blocks. type blockManager struct { @@ -320,6 +332,13 @@ type blockManager struct { startHeader *list.Element nextCheckpoint *chaincfg.Checkpoint + // These fields are related to handling of orphan blocks. They are + // protected by the orphan lock. + orphanLock sync.RWMutex + orphans map[chainhash.Hash]*orphanBlock + prevOrphans map[chainhash.Hash][]*orphanBlock + oldestOrphan *orphanBlock + // lotteryDataBroadcastMutex is a mutex protecting the map // that checks if block lottery data has been broadcasted // yet for any given block, so notifications are never @@ -755,6 +774,226 @@ func (b *blockManager) handleTxMsg(tmsg *txMsg) { b.cfg.PeerNotifier.AnnounceNewTransactions(acceptedTxs) } +// isKnownOrphan returns whether the passed hash is currently a known orphan. +// Keep in mind that only a limited number of orphans are held onto for a +// limited amount of time, so this function must not be used as an absolute way +// to test if a block is an orphan block. A full block (as opposed to just its +// hash) must be passed to ProcessBlock for that purpose. This function +// provides a mechanism for a caller to intelligently detect *recent* duplicate +// orphans and react accordingly. +// +// This function is safe for concurrent access. +func (b *blockManager) isKnownOrphan(hash *chainhash.Hash) bool { + // Protect concurrent access. Using a read lock only so multiple readers + // can query without blocking each other. + b.orphanLock.RLock() + _, exists := b.orphans[*hash] + b.orphanLock.RUnlock() + return exists +} + +// orphanRoot returns the head of the chain for the provided hash from the map +// of orphan blocks. +// +// This function is safe for concurrent access. +func (b *blockManager) orphanRoot(hash *chainhash.Hash) *chainhash.Hash { + // Protect concurrent access. Using a read lock only so multiple + // readers can query without blocking each other. + b.orphanLock.RLock() + defer b.orphanLock.RUnlock() + + // Keep looping while the parent of each orphaned block is known and is an + // orphan itself. + orphanRoot := hash + prevHash := hash + for { + orphan, exists := b.orphans[*prevHash] + if !exists { + break + } + orphanRoot = prevHash + prevHash = &orphan.block.MsgBlock().Header.PrevBlock + } + + return orphanRoot +} + +// removeOrphanBlock removes the passed orphan block from the orphan pool and +// previous orphan index. +func (b *blockManager) removeOrphanBlock(orphan *orphanBlock) { + // Protect concurrent access. + b.orphanLock.Lock() + defer b.orphanLock.Unlock() + + // Remove the orphan block from the orphan pool. + orphanHash := orphan.block.Hash() + delete(b.orphans, *orphanHash) + + // Remove the reference from the previous orphan index too. An indexing + // for loop is intentionally used over a range here as range does not + // reevaluate the slice on each iteration nor does it adjust the index + // for the modified slice. + prevHash := &orphan.block.MsgBlock().Header.PrevBlock + orphans := b.prevOrphans[*prevHash] + for i := 0; i < len(orphans); i++ { + hash := orphans[i].block.Hash() + if hash.IsEqual(orphanHash) { + copy(orphans[i:], orphans[i+1:]) + orphans[len(orphans)-1] = nil + orphans = orphans[:len(orphans)-1] + i-- + } + } + b.prevOrphans[*prevHash] = orphans + + // Remove the map entry altogether if there are no longer any orphans + // which depend on the parent hash. + if len(b.prevOrphans[*prevHash]) == 0 { + delete(b.prevOrphans, *prevHash) + } +} + +// addOrphanBlock adds the passed block (which is already determined to be an +// orphan prior calling this function) to the orphan pool. It lazily cleans up +// any expired blocks so a separate cleanup poller doesn't need to be run. It +// also imposes a maximum limit on the number of outstanding orphan blocks and +// will remove the oldest received orphan block if the limit is exceeded. +func (b *blockManager) addOrphanBlock(block *dcrutil.Block) { + // Remove expired orphan blocks. + for _, oBlock := range b.orphans { + if time.Now().After(oBlock.expiration) { + b.removeOrphanBlock(oBlock) + continue + } + + // Update the oldest orphan block pointer so it can be discarded + // in case the orphan pool fills up. + if b.oldestOrphan == nil || + oBlock.expiration.Before(b.oldestOrphan.expiration) { + b.oldestOrphan = oBlock + } + } + + // Limit orphan blocks to prevent memory exhaustion. + if len(b.orphans)+1 > maxOrphanBlocks { + // Remove the oldest orphan to make room for the new one. + b.removeOrphanBlock(b.oldestOrphan) + b.oldestOrphan = nil + } + + // Protect concurrent access. This is intentionally done here instead + // of near the top since removeOrphanBlock does its own locking and + // the range iterator is not invalidated by removing map entries. + b.orphanLock.Lock() + defer b.orphanLock.Unlock() + + // Insert the block into the orphan map with an expiration time + // 1 hour from now. + expiration := time.Now().Add(time.Hour) + oBlock := &orphanBlock{ + block: block, + expiration: expiration, + } + b.orphans[*block.Hash()] = oBlock + + // Add to previous hash lookup index for faster dependency lookups. + prevHash := &block.MsgBlock().Header.PrevBlock + b.prevOrphans[*prevHash] = append(b.prevOrphans[*prevHash], oBlock) +} + +// processOrphans determines if there are any orphans which depend on the passed +// block hash (they are no longer orphans if true) and potentially accepts them. +// It repeats the process for the newly accepted blocks (to detect further +// orphans which may no longer be orphans) until there are no more. +// +// The flags do not modify the behavior of this function directly, however they +// are needed to pass along to maybeAcceptBlock. +func (b *blockManager) processOrphans(hash *chainhash.Hash, flags blockchain.BehaviorFlags) error { + // Start with processing at least the passed hash. Leave a little room for + // additional orphan blocks that need to be processed without needing to + // grow the array in the common case. + processHashes := make([]*chainhash.Hash, 0, 10) + processHashes = append(processHashes, hash) + for len(processHashes) > 0 { + // Pop the first hash to process from the slice. + processHash := processHashes[0] + processHashes[0] = nil // Prevent GC leak. + processHashes = processHashes[1:] + + // Look up all orphans that are parented by the block we just accepted. + // This will typically only be one, but it could be multiple if multiple + // blocks are mined and broadcast around the same time. The one with + // the most proof of work will eventually win out. An indexing for loop + // is intentionally used over a range here as range does not reevaluate + // the slice on each iteration nor does it adjust the index for the + // modified slice. + for i := 0; i < len(b.prevOrphans[*processHash]); i++ { + orphan := b.prevOrphans[*processHash][i] + if orphan == nil { + bmgrLog.Warnf("Found a nil entry at index %d in the orphan "+ + "dependency list for block %v", i, processHash) + continue + } + + // Remove the orphan from the orphan pool. + orphanHash := orphan.block.Hash() + b.removeOrphanBlock(orphan) + i-- + + // Potentially accept the block into the block chain. + _, err := b.cfg.Chain.ProcessBlock(orphan.block, flags) + if err != nil { + return err + } + + // Add this block to the list of blocks to process so any orphan + // blocks that depend on this block are handled too. + processHashes = append(processHashes, orphanHash) + } + } + return nil +} + +// processBlockAndOrphans processes the provided block using the internal chain +// instance while keeping track of orphan blocks and also processing any orphans +// that depend on the passed block to potentially accept as well. +// +// When no errors occurred during processing, the first return value indicates +// the length of the fork the block extended. In the case it either extended +// the best chain or is now the tip of the best chain due to causing a +// reorganize, the fork length will be 0. The second return value indicates +// whether or not the block is an orphan, in which case the fork length will +// also be zero as expected, because it, by definition, does not connect to the +// best chain. +func (b *blockManager) processBlockAndOrphans(block *dcrutil.Block, flags blockchain.BehaviorFlags) (int64, bool, error) { + // Process the block to include validation, best chain selection, etc. + // + // Also, keep track of orphan blocks in the block manager when the error + // returned indicates the block is an orphan. + blockHash := block.Hash() + forkLen, err := b.cfg.Chain.ProcessBlock(block, flags) + if blockchain.IsErrorCode(err, blockchain.ErrMissingParent) { + bmgrLog.Infof("Adding orphan block %v with parent %v", blockHash, + block.MsgBlock().Header.PrevBlock) + b.addOrphanBlock(block) + + // The fork length of orphans is unknown since they, by definition, do + // not connect to the best chain. + return 0, true, nil + } + if err != nil { + return 0, false, err + } + + // Accept any orphan blocks that depend on this block (they are no longer + // orphans) and repeat for those accepted blocks until there are no more. + if err := b.processOrphans(blockHash, flags); err != nil { + return 0, false, err + } + + return forkLen, false, nil +} + // current returns true if we believe we are synced with our peers, false if we // still have blocks to check func (b *blockManager) current() bool { @@ -827,8 +1066,7 @@ func (b *blockManager) handleBlockMsg(bmsg *blockMsg) { // Process the block to include validation, best chain selection, orphan // handling, etc. - forkLen, isOrphan, err := b.cfg.Chain.ProcessBlock(bmsg.block, - behaviorFlags) + forkLen, isOrphan, err := b.processBlockAndOrphans(bmsg.block, behaviorFlags) if err != nil { // When the error is a rule error, it means the block was simply // rejected as opposed to something actually going wrong, so log @@ -856,7 +1094,7 @@ func (b *blockManager) handleBlockMsg(bmsg *blockMsg) { // Request the parents for the orphan block from the peer that sent it. onMainChain := !isOrphan && forkLen == 0 if isOrphan { - orphanRoot := b.cfg.Chain.GetOrphanRoot(blockHash) + orphanRoot := b.orphanRoot(blockHash) blkLocator, err := b.cfg.Chain.LatestBlockLocator() if err != nil { bmgrLog.Warnf("Failed to get block locator for the "+ @@ -1127,9 +1365,10 @@ func (b *blockManager) handleHeadersMsg(hmsg *headersMsg) { func (b *blockManager) haveInventory(invVect *wire.InvVect) (bool, error) { switch invVect.Type { case wire.InvTypeBlock: - // Ask chain if the block is known to it in any form (main - // chain, side chain, or orphan). - return b.cfg.Chain.HaveBlock(&invVect.Hash), nil + // Determine if the block is known in any form (main chain, side + // chain, or orphan). + hash := &invVect.Hash + return b.isKnownOrphan(hash) || b.cfg.Chain.HaveBlock(hash), nil case wire.InvTypeTx: // Ask the transaction memory pool if the transaction is known @@ -1253,11 +1492,11 @@ func (b *blockManager) handleInvMsg(imsg *invMsg) { // resending the orphan block as an available block // to signal there are more missing blocks that need to // be requested. - if b.cfg.Chain.IsKnownOrphan(&iv.Hash) { + if b.isKnownOrphan(&iv.Hash) { // Request blocks starting at the latest known // up to the root of the orphan that just came // in. - orphanRoot := b.cfg.Chain.GetOrphanRoot(&iv.Hash) + orphanRoot := b.orphanRoot(&iv.Hash) blkLocator, err := b.cfg.Chain.LatestBlockLocator() if err != nil { bmgrLog.Errorf("PEER: Failed to get block "+ @@ -1451,8 +1690,8 @@ out: } case processBlockMsg: - forkLen, isOrphan, err := b.cfg.Chain.ProcessBlock( - msg.block, msg.flags) + forkLen, isOrphan, err := b.processBlockAndOrphans(msg.block, + msg.flags) if err != nil { msg.reply <- processBlockResponse{ forkLen: forkLen, @@ -2044,10 +2283,8 @@ func (b *blockManager) requestFromPeer(p *peerpkg.Peer, blocks, txs []*chainhash continue } - // Check to see if we already have this block, too. - // If so, skip. - exists := b.cfg.Chain.HaveBlock(bh) - if exists { + // Skip the block when it is already known. + if b.isKnownOrphan(bh) || b.cfg.Chain.HaveBlock(bh) { continue } @@ -2219,6 +2456,8 @@ func newBlockManager(config *blockManagerConfig) (*blockManager, error) { headerList: list.New(), AggressiveMining: !cfg.NonAggressive, quit: make(chan struct{}), + orphans: make(map[chainhash.Hash]*orphanBlock), + prevOrphans: make(map[chainhash.Hash][]*orphanBlock), } best := bm.cfg.Chain.BestSnapshot() diff --git a/cmd/addblock/import.go b/cmd/addblock/import.go index 7e3bcacc86..62fe3b1c04 100644 --- a/cmd/addblock/import.go +++ b/cmd/addblock/import.go @@ -127,20 +127,20 @@ func (bi *blockImporter) processBlock(serializedBlock []byte) (bool, error) { // Ensure the blocks follows all of the chain rules and match up to the // known checkpoints. - forkLen, isOrphan, err := bi.chain.ProcessBlock(block, - blockchain.BFFastAdd) + forkLen, err := bi.chain.ProcessBlock(block, blockchain.BFFastAdd) if err != nil { + if blockchain.IsErrorCode(err, blockchain.ErrMissingParent) { + return false, fmt.Errorf("import file contains an orphan block: %v", + blockHash) + } + return false, err } - isMainChain := !isOrphan && forkLen == 0 + isMainChain := forkLen == 0 if !isMainChain { return false, fmt.Errorf("import file contains a block that "+ "does not extend the main chain: %v", blockHash) } - if isOrphan { - return false, fmt.Errorf("import file contains an orphan "+ - "block: %v", blockHash) - } return true, nil }