Skip to content

Process block headers #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions blockchain/accept.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,23 +62,29 @@ func (b *BlockChain) maybeAcceptBlock(block *btcutil.Block, flags BehaviorFlags)
return false, err
}

// Create a new block node for the block and add it to the node index. Even
// if the block ultimately gets connected to the main chain, it starts out
// on a side chain.
blockHeader := &block.MsgBlock().Header
newNode := newBlockNode(blockHeader, prevNode)
newNode.status = statusDataStored

b.index.AddNode(newNode)
err = b.index.flushToDB()
if err != nil {
return false, err
// Lookup for the block node that was created at the time of the header
// validation. If the block node is not existent, then create a new
// block node for the block and add it to the node index. Even if
// the block ultimately gets connected to the main chain, it starts
// out on a side chain.
blockHash := block.Hash()
node := b.index.LookupNode(blockHash)
if node == nil {
blockHeader := &block.MsgBlock().Header
newNode := newBlockNode(blockHeader, prevNode)
b.index.AddNode(newNode)
err = b.index.flushToDB()
if err != nil {
return false, err
}
node = newNode
}
node.status = statusDataStored

// Connect the passed block to the chain while respecting proper chain
// selection according to the chain with the most proof of work. This
// also handles validation of the transaction scripts.
isMainChain, err := b.connectBestChain(newNode, block, flags)
isMainChain, err := b.connectBestChain(node, block, flags)
if err != nil {
return false, err
}
Expand Down
32 changes: 32 additions & 0 deletions blockchain/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,38 @@ func (b *BlockChain) HaveBlock(hash *chainhash.Hash) (bool, error) {
return exists || b.IsKnownOrphan(hash), nil
}

// HaveBlockWithData returns whether or not the chain instance has the block
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff --git a/blockchain/chain.go b/blockchain/chain.go
index 2ea09665..4bb1416f 100644
--- a/blockchain/chain.go
+++ b/blockchain/chain.go
@@ -229,7 +229,7 @@ func (b *BlockChain) HaveBlock(hash *chainhash.Hash) (bool, error) {
 }
 
 // HaveBlockWithData returns whether or not the chain instance has the block
-// represented by the passed hash This includes checking the various places
+// 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. In addition to this the function also checks for presence of block
 // data along with presence of node.

// 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. In addition to this, the function also checks for the presence of block
// data along with the presence of a node.
//
// This function is safe for concurrent access.
func (b *BlockChain) HaveBlockWithData(hash *chainhash.Hash) (bool, error) {
// Check if the node exists in blockIndex or database.
exists, err := b.blockExists(hash)
if err != nil {
return false, err
}
if exists {
// Retrieving the node from the blockIndex to check the node status.
node := b.IndexLookupNode(hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b.blockExists is calling b.HaveBlock which is just look up the blockhash in the map.

index.LookupNode is calling b.index.LookupNode which is also looking up the blockhash in the map.

Since they're both looking up the same map, there's no way the node can be nil here unless if it's an orphan.

Maybe not handling orphans and the blocks in the same branch would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got your point, that checking for block data for an orphan block has no point, since we can't have an orphan header.
So I can just return true if the block is an orphan.

// If the returned node is nil, this implies that the block is stored
// in the database and not present in block index, so we return true
// here as presence of block in database means that we already have
// its block data.
if node == nil {
return true, nil
}
// If the block data is present then we return true, otherwise we
// return false.
return node.status.HaveData(), nil
}
// If the block is not present in blockIndex or in database then we return
// false, but if the hash is a known orphan then we return true.
return false || b.IsKnownOrphan(hash), 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
Expand Down
73 changes: 73 additions & 0 deletions blockchain/fullblocktests/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"

"github.com/utreexo/utreexod/blockchain"
"github.com/utreexo/utreexod/btcutil"
)

type chaingenHarness struct {
Expand Down Expand Up @@ -130,3 +131,75 @@ func (g *chaingenHarness) RejectHeader(blockName string, code blockchain.ErrorCo
"marked as known invalid", blockName, blockHash)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, the code here is taken from TestFullBlocks() is fullblocktests right?

Having duplicate code that performs the same functionality isn't great for maintainability. I need to take a better look at the original Generate() function but it does seem doable to integrate the headers tests there instead of making a whole another set of tests.

Ok seems like a lot of work from a glance. Still something desirable.

// testAcceptedBlock attempts to process the block in the provided test
// instance and ensures that it was accepted according to the flags
// specified in the test.
func (g *chaingenHarness) AcceptBlock(item AcceptedBlock) {
g.t.Helper()

blockHeight := item.Height
block := btcutil.NewBlock(item.Block)
block.SetHeight(blockHeight)
g.t.Logf("Testing block %s (hash %s, height %d)",
item.Name, block.Hash(), blockHeight)

isMainChain, isOrphan, err := g.chain.ProcessBlock(block,
blockchain.BFNone)
if err != nil {
g.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
// specified in the test.
if isMainChain != item.IsMainChain {
g.t.Fatalf("block %q (hash %s, height %d) unexpected main "+
"chain flag -- got %v, want %v", item.Name,
block.Hash(), blockHeight, isMainChain,
item.IsMainChain)
}
if isOrphan != item.IsOrphan {
g.t.Fatalf("block %q (hash %s, height %d) unexpected "+
"orphan flag -- got %v, want %v", item.Name,
block.Hash(), blockHeight, isOrphan,
item.IsOrphan)
}
}

// testRejectedBlock attempts to process the block in the provided test
// instance and ensures that it was rejected with the reject code
// specified in the test.
func (g *chaingenHarness) RejectBlock(item RejectedBlock) {
g.t.Helper()

blockHeight := item.Height
block := btcutil.NewBlock(item.Block)
block.SetHeight(blockHeight)
g.t.Logf("Testing block %s (hash %s, height %d)",
item.Name, block.Hash(), blockHeight)

_, _, err := g.chain.ProcessBlock(block, blockchain.BFNone)
if err == nil {
g.t.Fatalf("block %q (hash %s, height %d) should not "+
"have been accepted", item.Name, block.Hash(),
blockHeight)
}

// Ensure the error code is of the expected type and the reject
// code matches the value specified in the test instance.
rerr, ok := err.(blockchain.RuleError)
if !ok {
g.t.Fatalf("block %q (hash %s, height %d) returned "+
"unexpected error type -- got %T, want "+
"blockchain.RuleError", item.Name, block.Hash(),
blockHeight, err)
}
if rerr.ErrorCode != item.RejectCode {
g.t.Fatalf("block %q (hash %s, height %d) does not have "+
"expected reject code -- got %v, want %v",
item.Name, block.Hash(), blockHeight,
rerr.ErrorCode, item.RejectCode)
}
}
101 changes: 97 additions & 4 deletions blockchain/fullblocktests/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -2214,7 +2214,15 @@ func GenerateHeaders() (generator *testGenerator, tests [][]TestInstance) {
blockHeight := g.blockHeights[blockName]
return RejectedHeader{blockName, blockHeader, blockHeight, code}
}

acceptBlock := func(blockName string, block *wire.MsgBlock, isMainChain, isOrphan bool) TestInstance {
blockHeight := g.blockHeights[blockName]
return AcceptedBlock{blockName, block, blockHeight, isMainChain,
isOrphan}
}
rejectBlock := func(blockName string, block *wire.MsgBlock, code blockchain.ErrorCode) TestInstance {
blockHeight := g.blockHeights[blockName]
return RejectedBlock{blockName, block, blockHeight, code}
}
// Define some convenience helper functions to populate the tests slice
// with test instances that have the described characteristics.
//
Expand All @@ -2224,6 +2232,13 @@ func GenerateHeaders() (generator *testGenerator, tests [][]TestInstance) {
//
// rejected creates and appends a single rejectHeader test instance for
// the current tip.
//
// acceptedBlock creates and appends a single acceptBlock test instance for
// the current tip which expects the block to be accepted to the main
// chain.
//
// rejectedBlock creates and appends a single rejectBlock test instance for
// the current tip.
accepted := func() {
tests = append(tests, []TestInstance{
acceptHeader(g.tipName, g.tip),
Expand All @@ -2234,6 +2249,16 @@ func GenerateHeaders() (generator *testGenerator, tests [][]TestInstance) {
rejectHeader(g.tipName, g.tip, code),
})
}
acceptedBlock := func() {
tests = append(tests, []TestInstance{
acceptBlock(g.tipName, g.tip, true, false),
})
}
rejectedBlock := func(code blockchain.ErrorCode) {
tests = append(tests, []TestInstance{
rejectBlock(g.tipName, g.tip, code),
})
}
// ---------------------------------------------------------------------
// Generate enough blocks to have mature coinbase outputs to work with.
//
Expand Down Expand Up @@ -2317,11 +2342,79 @@ func GenerateHeaders() (generator *testGenerator, tests [][]TestInstance) {
g.updateBlockState("b3", origHash, "b3", b3)
}
rejected(blockchain.ErrUnexpectedDifficulty)
// Adding a block with valid header
// Adding a block with valid header but invalid spend
//
// ... -> b0() -> b4(1)
// ... -> b0() -> b4(2)
g.setTip("b0")
g.nextBlock("b4", outs[1])
g.nextBlock("b4", outs[2])
accepted()
// Adding a block with valid header and valid spend, but invalid parent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is wrong here. In headers-first you don't accept a header with an invalid parent. The tip is never reset to "b0" and thus it has a valid parent of "b4".

In general, checkpointed headers are assumed to be correct as it's hardcoded into the binary and it's actually a consensus rule (whether or not this is good is besides the point). Once you accept headers, the block must match with the header. If it doesn't I believe the node just hangs.

So we can have tests for rejecting headers but once we accept the headers, it's assumed as correct so the blockdata corresponding to the headers should never be rejected.

//
// ... -> b0() -> b5(1)
g.nextBlock("b5", outs[1])
accepted()
// Adding a block with valid header and valid spend and valid parent
//
// ... -> b0() -> b6(1)
g.setTip("b0")
g.nextBlock("b6", outs[1])
// Accepting/Rejecting the blocks for the headers that were
// accepted/rejected
testInstances = make([]TestInstance, 0)
for i := uint16(0); i < coinbaseMaturity; i++ {
blockName := fmt.Sprintf("bm%d", i)
g.setTip(blockName)
testInstances = append(testInstances, acceptBlock(g.tipName,
g.tip, true, false))
}
tests = append(tests, testInstances)
// Accepting the block b0
//
// ... -> b0
g.setTip("b0")
acceptedBlock()
// Rejecting the block data for b1 because of high hash
// ... -> b0()
// \-> b1(1)
g.setTip("b1")
rejectedBlock(blockchain.ErrHighHash)
// Acccept the block as orphan
//
// -> b1a(1)
g.setTip("b1a")
tests = append(tests, []TestInstance{
acceptBlock(g.tipName, g.tip, false, true),
})
// Reject the block with invalid proof of work
//
// ... -> b0()
// \-> b2(1)
g.setTip("b2")
rejectedBlock(blockchain.ErrUnexpectedDifficulty)
// Reject the block with invalid negative proof of work
//
// ... -> b0()
// \-> b3(1)
g.setTip("b3")
rejectedBlock(blockchain.ErrUnexpectedDifficulty)
// Rejecting the block with valid header but invalid spend
//
// ... -> b0()
// \-> b4(2)
g.setTip("b4")
rejectedBlock(blockchain.ErrImmatureSpend)
// Since the block is rejected, so rejecting the header also
// which was earlier accepted.
rejected(blockchain.ErrKnownInvalidBlock)
// Rejecting a block with valid header and valid spend, but invalid parent
//
// -> b4(2) -> b5(1)
g.setTip("b5")
rejectedBlock(blockchain.ErrInvalidAncestorBlock)
// Accepting the block
//
// ... -> b0() -> b6(1)
g.setTip("b6")
acceptedBlock()
return &g, tests
}
4 changes: 4 additions & 0 deletions blockchain/fullblocktests/process_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ func TestProcessLogic(t *testing.T) {
harness.AcceptHeader(item.Name)
case RejectedHeader:
harness.RejectHeader(item.Name, item.RejectCode)
case AcceptedBlock:
harness.AcceptBlock(item)
case RejectedBlock:
harness.RejectBlock(item)
default:
t.Fatalf("test #%d, item #%d is not one of "+
"the supported test instance types -- "+
Expand Down
28 changes: 26 additions & 2 deletions blockchain/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ const (
// not be performed.
BFNoPoWCheck

// BFHeaderValidated indicates the header has already been validated
// and the header validation can be skipped when block validation is
// being performed.
BFHeaderValidated

// BFNone is a convenience value to specifically indicate no flags.
BFNone BehaviorFlags = 0
)
Expand Down Expand Up @@ -200,13 +205,25 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, flags BehaviorFlags) (bo

blockHash := block.Hash()
log.Tracef("Processing block %v", blockHash)

// The block must not already exist in the main chain or side chains.
// Check if the block is present in main chain or any of the side chains.
exists, err := b.blockExists(blockHash)
if err != nil {
return false, false, err
}
// If the block node is present, it means that the header is already verified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff --git a/blockchain/process.go b/blockchain/process.go
index f8a06d71..01551514 100644
--- a/blockchain/process.go
+++ b/blockchain/process.go
@@ -212,7 +212,7 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, flags BehaviorFlags) (bo
        }
        // If the block node is present, it means that the header is already verified.
        // So we add this to the behavioural flag and the flag is then passed to further
-       // functions, where we don't need to reavalidate the header.
+       // functions so that we don't re-validate the header.
        if exists {
                flags |= BFHeaderValidated
        }
@@ -242,6 +242,7 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, flags BehaviorFlags) (bo
                        return false, false, err
                }
        }
+
        // Perform preliminary sanity checks on the block and its transactions.a
        err = checkBlockSanity(block, b.chainParams.PowLimit, b.timeSource, flags)
        if err != nil {

// So we add this to the behavioural flag and the flag is then passed to further
// functions, so that we don't have to re-validate the header.
if exists {
flags |= BFHeaderValidated
}
// Look up the node and check if the block data is already stored.
node := b.index.LookupNode(blockHash)
haveData := false
if node != nil {
haveData = node.status.HaveData()
}
// Return an error if the block data is already present.
if exists && haveData {
str := fmt.Sprintf("already have block %v", blockHash)
return false, false, ruleError(ErrDuplicateBlock, str)
}
Expand All @@ -217,6 +234,13 @@ func (b *BlockChain) ProcessBlock(block *btcutil.Block, flags BehaviorFlags) (bo
return false, false, ruleError(ErrDuplicateBlock, str)
}

// Reject blocks that are already known to be invalid immediately to avoid
// additional work when possible.
if node != nil {
if err := b.checkKnownInvalidBlock(node); err != nil {
return false, false, err
}
}
// Perform preliminary sanity checks on the block and its transactions.
err = checkBlockSanity(block, b.chainParams.PowLimit, b.timeSource, flags)
if err != nil {
Expand Down
24 changes: 18 additions & 6 deletions blockchain/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -467,9 +467,15 @@ func checkBlockHeaderSanity(header *wire.BlockHeader, powLimit *big.Int, timeSou
func checkBlockSanity(block *btcutil.Block, powLimit *big.Int, timeSource MedianTimeSource, flags BehaviorFlags) error {
msgBlock := block.MsgBlock()
header := &msgBlock.Header
err := checkBlockHeaderSanity(header, powLimit, timeSource, flags)
if err != nil {
return err

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff --git a/blockchain/validate.go b/blockchain/validate.go
index 65cdee93..5a45e589 100644
--- a/blockchain/validate.go
+++ b/blockchain/validate.go
@@ -468,8 +468,8 @@ func checkBlockSanity(block *btcutil.Block, powLimit *big.Int, timeSource Median
        msgBlock := block.MsgBlock()
        header := &msgBlock.Header
 
-       // If the BFHeaderValidated flag is set, then it means that the header
-       // is already been validated, so we don't need to revalidate it. So we
+       // If the BFHeaderValidated flag is set, it means that the header has
+       // already been validated and we don't need to re-validate it. Only
        // check block header sanity if we BFHeaderValidated is not set.
        if flags&BFHeaderValidated != BFHeaderValidated {
                err := checkBlockHeaderSanity(header, powLimit, timeSource, flags)
@@ -733,9 +733,9 @@ func (b *BlockChain) checkBlockContext(block *btcutil.Block, prevNode *blockNode
        // Perform all block header related validation checks.
        header := &block.MsgBlock().Header
 
-       // If BFHeaderValidated flag is set, then it means that the header is
-       // already been validated, so we don't need to revalidate it. So we check
-       // block header context, only when BFHeaderValidated flag is not set.
+       // If the BFHeaderValidated flag is set, it means that the header has
+       // already been validated and we don't need to re-validate it. Only
+       // check block header sanity if we BFHeaderValidated is not set.
        if flags&BFHeaderValidated != BFHeaderValidated {
                err := b.checkBlockHeaderContext(header, prevNode, flags)
                if err != nil {

// If the BFHeaderValidated flag is set, it means that the header
// has already been validated, so we don't need to re-validate it. Only
// check block header sanity if BFHeaderValidated is not set.
if flags&BFHeaderValidated != BFHeaderValidated {
err := checkBlockHeaderSanity(header, powLimit, timeSource, flags)
if err != nil {
return err
}
}

// A block must have at least one transaction.
Expand Down Expand Up @@ -726,9 +732,15 @@ func (b *BlockChain) checkBlockHeaderContext(header *wire.BlockHeader, prevNode
func (b *BlockChain) checkBlockContext(block *btcutil.Block, prevNode *blockNode, flags BehaviorFlags) error {
// Perform all block header related validation checks.
header := &block.MsgBlock().Header
err := b.checkBlockHeaderContext(header, prevNode, flags)
if err != nil {
return err

// If BFHeaderValidated flag is set, then it means that the header has
// already been validated and we don't need to re-validate it. We check
// block header context, only when BFHeaderValidated flag is not set.
if flags&BFHeaderValidated != BFHeaderValidated {
err := b.checkBlockHeaderContext(header, prevNode, flags)
if err != nil {
return err
}
}

fastAdd := flags&BFFastAdd == BFFastAdd
Expand Down
Loading