blockchain, fullblocktests, workmath, testhelper: add InvalidateBlock() method to BlockChain #2155

kcalvinalvin · 2024-04-02T09:06:51Z

Depends on #2153

All of the code except for the last two commits are for refactoring code. The vast majority is just moving code from fullblocktests to testhelper so that the testing code for InvalidateBlock() can call the functions instead of having to copy over and duplicate functions.

The InvalidateBlock() method is operational and is tested but is not revealed through rpc in this PR.

coveralls · 2024-04-02T09:24:01Z

Pull Request Test Coverage Report for Build 8891501246

Details

292 of 315 (92.7%) changed or added relevant lines in 5 files are covered.
10 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.1%) to 56.978%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
blockchain/fullblocktests/generate.go	41	42	97.62%
blockchain/internal/testhelper/common.go	105	113	92.92%
blockchain/chain.go	76	90	84.44%

Files with Coverage Reduction	New Missed Lines	%
connmgr/connmanager.go	1	86.27%
peer/peer.go	9	73.72%

Totals
Change from base Build 8881487645:	0.1%
Covered Lines:	29567
Relevant Lines:	51892

💛 - Coveralls

Roasbeef · 2024-04-03T00:32:43Z

blockchain/testhelper/README.md

@@ -0,0 +1,16 @@
+testhelper


We could also make this an internal package if we don't envision any external usage (hidden from default godoc, can't be imported outside the project).

Addressed in the latest pushes. Made an internal package inside the package blockchain

Roasbeef · 2024-04-03T00:37:05Z

blockchain/common_test.go

@@ -14,6 +14,7 @@ import (
 	"strings"
 	"time"

+	"github.com/btcsuite/btcd/blockchain/testhelper"


Commit references invalidateblock and reconsiderblock, but only the latter was added in this PR.

I think for cases like the issue with the pruning bug w/ the utxo set cache, we actually want reconsiderblock, as invalidateblock marks all the dependents as invalid, so it won't attempt to reprocess anything after the target block.

I'll add it in as another PR so that it doesn't add on to the needed reviews on this PR.

Was holding it off because the test for reconsiderblock utilizes invalidateblock

Roasbeef · 2024-04-03T00:41:42Z

blockchain/chain.go

+		}
+
+		if writeErr := b.index.flushToDB(); writeErr != nil {
+			log.Warnf("Error flushing block index changes to disk: %v", writeErr)


Hmm, perhaps we should just hard fail here instead? As possible that we have unclean state in the db at this point, and accepting further blocks may cause cascading issues.

Addressed in the latest pushes.

Roasbeef · 2024-04-03T00:42:52Z

blockchain/chain.go

+		b.index.UnsetStatusFlags(n, statusValid)
+		detachNodes.PushBack(n)
+	}
+	// Push back the block node being invalidated.


Style nit: missing space above.

Addressed in the latest pushes.

Roasbeef · 2024-04-03T00:49:08Z

blockchain/chain_test.go

+		chainGen func() (*BlockChain, []*chainhash.Hash, func())
+	}{
+		{
+			name: "One branch, invalidate once",


Should start with a lower case character for uniformity.

Addressed in the latest pushes.

Roasbeef · 2024-04-03T00:49:37Z

blockchain/chain_test.go

+				source := rand.NewSource(time.Now().UnixNano())
+				rand := rand.New(source)
+
+				chain, params, tearDown := utxoCacheTestChain("TestInvalidateBlock-one-branch-invalidate-once")


I know we don't have the full linter active in this project, but can we wrap all this to 80 char columns to match most of the rest of the codebase?

Addressed in the latest pushes.

Roasbeef · 2024-04-03T00:50:19Z

blockchain/chain_test.go

+						randIdx := rand.Intn(len(allSpends))
+
+						spend := allSpends[randIdx]                                       // get
+						allSpends = append(allSpends[:randIdx], allSpends[randIdx+1:]...) // delete


https://pkg.go.dev/slices#Delete

Roasbeef · 2024-04-03T00:51:00Z

blockchain/chain_test.go

+				var invalidateHash *chainhash.Hash
+
+				// Create a chain with 11 blocks.
+				for b := 0; b < 10; b++ {


Can consolidate into a helper func based on the other instance above where we make the same 11 block chain.

Will finish this promptly

Addressed in the latest push

Roasbeef · 2024-04-03T00:51:43Z

blockchain/chain_test.go

+				}
+
+				// Create a side chain with 7 blocks that builds on block 1.
+				var altSpends []*testhelper.SpendableOut


Same here re helper func for the 7 block chain.

Roasbeef · 2024-04-03T00:52:01Z

blockchain/chain_test.go

+
+	for _, test := range tests {
+		chain, invalidateHashes, tearDown := test.chainGen()
+		func() {


Why the closure func that gets immediately executed?

We can also use t.Run here, then that way the test name gets printed during execution, and we can run just one of the sub-tests by name.

Was doing this so that even when the test fails, we still run the teardown

Crypt-iQ

looks good, think the tests could be cleaned up a little and have more comments

Crypt-iQ · 2024-04-05T17:12:03Z

blockchain/chain_test.go

+					allSpends = append(allSpends, newSpendableOuts...)
+
+					var nextSpendsTmp []*testhelper.SpendableOut
+					for i := 0; i < len(allSpends); i++ {


A comment explaining what this is doing would be very helpful

Addressed in the latest pushes.

instead of randomly grabbing, doesn't this grab all of the utxos from allSpends?

Oh you're right. Fixed the code in the latest push.

Crypt-iQ · 2024-04-05T17:12:36Z

blockchain/chain_test.go

+	}{
+		{
+			name: "One branch, invalidate once",
+			chainGen: func() (*BlockChain, []*chainhash.Hash, func()) {


think this should be functions below rather than explicitly in full form here

Could you clarify what you mean here?

I just mean moving the chainGen function into a function outside of the test so you just call chainGen:funcHere for readability. Just my preference, but it's non blocking

Roasbeef · 2024-04-10T03:11:19Z

Dependent PR merged, needs a rebase!

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.

Crypt-iQ

changes look good to me, only thing is whether reconsiderblock should be implemented instead

Crypt-iQ · 2024-04-24T16:24:57Z

blockchain/chain_test.go

+					allSpends = append(allSpends, newSpendableOuts...)
+
+					var nextSpendsTmp []*testhelper.SpendableOut
+					for i := 0; i < len(allSpends); i++ {


instead of randomly grabbing, doesn't this grab all of the utxos from allSpends?

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.

Roasbeef requested changes Apr 3, 2024

View reviewed changes

Crypt-iQ reviewed Apr 5, 2024

View reviewed changes

kcalvinalvin force-pushed the 2024-04-02-invalidate-block branch 2 times, most recently from 6b3238e to 7e9b796 Compare April 22, 2024 17:19

kcalvinalvin added 9 commits April 23, 2024 02:35

blockchain: add block generating functions in test code

ea39fe0

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.

kcalvinalvin force-pushed the 2024-04-02-invalidate-block branch from 7e9b796 to 2960f40 Compare April 22, 2024 17:53

Crypt-iQ reviewed Apr 24, 2024

View reviewed changes

kcalvinalvin force-pushed the 2024-04-02-invalidate-block branch from 2960f40 to fa7e3d8 Compare April 30, 2024 07:30

blockchain: Add InvalidateBlock() method to BlockChain

635ae68

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.

kcalvinalvin force-pushed the 2024-04-02-invalidate-block branch from fa7e3d8 to 635ae68 Compare April 30, 2024 08:13

This was referenced Apr 30, 2024

blcokchain: Add ReconsiderBlock() #2181

Open

main, rpcclient, integration: add rpccalls for invalidate and reconsiderblock #2183

Open

kcalvinalvin requested review from Roasbeef and Crypt-iQ May 7, 2024 03:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockchain, fullblocktests, workmath, testhelper: add InvalidateBlock() method to BlockChain #2155

blockchain, fullblocktests, workmath, testhelper: add InvalidateBlock() method to BlockChain #2155

kcalvinalvin commented Apr 2, 2024

coveralls commented Apr 2, 2024 •

edited

Roasbeef Apr 3, 2024

kcalvinalvin Apr 22, 2024

Roasbeef Apr 3, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 30, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 22, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 22, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 22, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 22, 2024

Roasbeef Apr 3, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 22, 2024

kcalvinalvin Apr 30, 2024

Roasbeef Apr 3, 2024

Roasbeef Apr 3, 2024

Roasbeef Apr 3, 2024

kcalvinalvin Apr 30, 2024

Crypt-iQ left a comment

Crypt-iQ Apr 5, 2024

kcalvinalvin Apr 22, 2024

Crypt-iQ Apr 24, 2024

kcalvinalvin Apr 30, 2024

Crypt-iQ Apr 5, 2024

kcalvinalvin Apr 22, 2024

Crypt-iQ Apr 23, 2024

Roasbeef commented Apr 10, 2024

Crypt-iQ left a comment

Crypt-iQ Apr 24, 2024

blockchain, fullblocktests, workmath, testhelper: add InvalidateBlock() method to BlockChain #2155

Are you sure you want to change the base?

blockchain, fullblocktests, workmath, testhelper: add InvalidateBlock() method to BlockChain #2155

Conversation

kcalvinalvin commented Apr 2, 2024

coveralls commented Apr 2, 2024 • edited

Pull Request Test Coverage Report for Build 8891501246

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Apr 10, 2024

Crypt-iQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 2, 2024 •

edited