blockchain: Add ReconsiderBlock() #2181
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some of the functions in difficulty.go are not dependent on any external functions and they are needed to introduce testing code for the invalidateblock and reconsiderblock methods that are to be added on in later commits. Having the workmath package let's us reuse the code and avoid dependency cycles. The existing functions that were exported already (HashToBig, CompactToBig, BigToCompact, CalcWork) are still kept in difficulty.go to avoid breaking external code that depends on those exported functions.
spendableOut and the functions related to it are is moved to package testhelper and are exported. This is done to make the code reusable without introducing an import cycle when the testing code for invalidateblock and reconsiderblock are added in follow up commits.
solveBlock is moved to testhelper and is exported. This is done so that the code can be reused without introducing import cycles. The testing code to be added in alter commits for invalidateblock and reconsider block will use SolveBlock.
The variables are moved to testhelper so that they can be reused in the blockchain package without introducing an import cycle. The testing code for invalidateblock and reconsiderblock that will be added in later commits will be using these variables.
uniqueOpReturnScript is moved to testhelper and is exported so that the code and be reused in package blockchain without introducing import cycles. The test code for invalidateblock and reconsiderblock that are gonna be added in later commits uses the functions.
standardCoinbaseScript is moved to testhelper and is exported. This allows test code in package blockchain to reuse the code without introducing an import cycle. This code is used in the testing code for invalidateblock and reconsiderblock that's added in the later commits.
createCoinbaseTx's code is refactored out and placed in testhelper package and is exported so that callers in package blockchain can reuse the code without introducing import cycles. The test code for invalidateblock and reconsiderblock that'll be added in later commits make use of this code.
createSpendTx is moved to testhelper so that the function can be used for callers in package blockchain without introducing import cycles. The test code for invalidateblock and reconsiderblock that are going to be added in later commits make use of this code.
The block generating functions here allow for a test to create mock blocks. This is useful for testing invalidateblock and reconsiderblock methods on blockchain that will be added in later commits.
InvalidateBlock() invalidates a given block and marks all its descendents as invalid as well. The active chain tip changes if the invalidated block is part of the best chain.
reorganizeChain() used to handle the following: 1: That the blocknodes being disconnected/connected indeed to connect properly without errors. 2: Perform the actual disconnect/connect of the blocknodes. The functionality of 1, the validation that the disconnects/connects can happen without errors are now refactored out into verifyReorganizationValidity. This is an effort made so that ReconsiderBlock() can call verifyReorganizationValidity and set the block status of the reconsidered chain and return nil even when an error returns as it's ok to get an error when reconsidering an invalid branch.
ReconsiderBlock reconsiders the validity of the block for the passed in blockhash. The behavior of the function mimics that of Bitcoin Core. The invalid status of the block nodes are reset and if the chaintip that is being reconsidered has more cumulative work, then we'll validate the blocks and reorganize to it. If the cumulative work is lesser than the current active chain tip, then nothing else will be done.
Pull Request Test Coverage Report for Build 8893534348Details
💛 - Coveralls |
|
I just want to note that the InvalidateBlock/ReconsiderBlock in lbcd and and bchd is incorrect. See gcash/bchd#512 |
|
Can be rebased now! |
This is a completely independent implementation by me. I didn't even know they were implemented there. |
Roasbeef
changed the title
Roasbeef
reviewed
May 23, 2024
|
|
||
| // Update the view to unspend all of the spent txos and remove | ||
| // the utxos created by the block. | ||
| err = view.disconnectTransactions(b.db, block, |
There was a problem hiding this comment.
Style nit, can reflow as:
err = view.disconnectTransactions(
b.db, block, detachSpentTxOuts[i],
)|
|
||
| // Nothing to do if the given block is already valid. | ||
| if node.status.KnownValid() { | ||
| return nil |
There was a problem hiding this comment.
Can we add a log statament here? Something along the lines of:
log.Infof("block_hash=%x is valid, nothing to reconsider", hash[:])| b.chainLock.Lock() | ||
| defer b.chainLock.Unlock() | ||
|
|
||
| node := b.index.LookupNode(hash) |
There was a problem hiding this comment.
Naming suggestion: reconsiderNode.
| // being reconsidered. | ||
| for n := tip; n != nil && n != node; n = n.parent { | ||
| b.index.UnsetStatusFlags(n, statusInvalidAncestor) | ||
| } |
There was a problem hiding this comment.
Can't we break after this?
Also isn't it possible that the block we want to reconsider is a descendent of several of the inactive chain tips? If this is the case, then we'll continually re-assign reconsiderTip for each of them.
Consider a scenario where we had 3 inactive chain tips {A, B, C}, block D that we want to reconsider is a descendent of all 3 chain tips, but they later on fork further down the line produce distinct chain tips (that we thought were invalid in the past).
There was a problem hiding this comment.
Actually on second thought, block hash chains are unique, so if D is a descendent of all 3 chain tips, then the chain tips should themselves be identical.
| // If we errored out during the verification of the reorg branch, | ||
| // it's ok to return nil as we reconsidered the block and determined | ||
| // that it's invalid. | ||
| return nil |
Roasbeef
requested changes
May 23, 2024
| func (b *BlockChain) ReconsiderBlock(hash *chainhash.Hash) error { | ||
| b.chainLock.Lock() | ||
| defer b.chainLock.Unlock() | ||
|
|
There was a problem hiding this comment.
log.Infof("Reconsidering block_hash=%x", hash[:])
| // Grab all the tips. | ||
| tips := b.index.InactiveTips(b.bestChain) | ||
| tips = append(tips, b.bestChain.Tip()) | ||
|
|
There was a problem hiding this comment.
log.Debugf("Examining %v inactive chain tips for reconsideration")
|
|
||
| // Compare the cumulative work for the branch being reconsidered. | ||
| if reconsiderTip.workSum.Cmp(b.bestChain.Tip().workSum) <= 0 { | ||
| return nil |
There was a problem hiding this comment.
log.Debugf("Tip to reconsider has less cumulative work than current chain tip: %v vs %v", reconsiderTip.worksum, bestChain.Tip().workSum)
| // to it after checking the validity of the nodes. | ||
| detachNodes, attachNodes := b.getReorganizeNodes(reconsiderTip) | ||
|
|
||
| // We're checking if the reorganization that'll happen is actually valid. |
There was a problem hiding this comment.
log.Infof("Re-org to reconsider_block=%x size %v (num_add=%v, num_detach=%v)", reconsiderTip.hash[;], len(detchNodes)+len(attachNodes), len(detchNodes), len(attachNodes))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds
ReconsiderBlock()on blockchain that will clear up any block validation statuses and re-validate the block. If the block is part of a longer chain, then the chain will reorganize to that chain.Depends on #2155