Skip to content

Block validation fix for checkOldBlockIDs #52

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.

Sign up for GitHub

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 3 commits into
base: main
Choose a base branch
from
Open

Block validation fix for checkOldBlockIDs #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab13739
4172: block validation fix for checkOldBlockIDs
liam Oct 24, 2025
66de32f
Merge branch 'main' into feature/fixCheckOldBlockIds
oskarszoon Oct 24, 2025
31d35c6
Merge branch 'main' into feature/fixCheckOldBlockIds
oskarszoon Oct 24, 2025
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
109 changes: 50 additions & 59 deletions services/blockvalidation/BlockValidation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
"context"
"fmt"
"slices"
"strconv"
"strings"
"sync"
"sync/atomic"
"time"
Expand Down Expand Up @@ -1924,15 +1922,28 @@ func (u *BlockValidation) validateBlockSubtrees(ctx context.Context, block *mode
return u.subtreeValidationClient.CheckBlockSubtrees(ctx, block, baseURL)
}

// checkOldBlockIDs verifies that referenced blocks are in the current chain.
// It prevents invalid chain reorganizations and maintains chain consistency.
// checkOldBlockIDs verifies that referenced blocks exist in the block's ancestry.
// This is a critical consensus check that ensures transactions only reference UTXOs
// that exist in the block's own chain history, not from competing forks.
//
// Bitcoin consensus requires that a block can only spend UTXOs from its ancestry.
// When validating a block (including fork blocks), we must verify that all parent
// transactions exist in that block's chain, walking backward from the block being
// validated. This prevents a fork block from incorrectly spending UTXOs that exist
// on the main chain but not in the fork's history.
//
// The function fetches up to 100,000 ancestors of the block being validated and
// checks that all parent block IDs referenced by transactions exist within that
// ancestry. The 100K limit covers approximately 695 days of history, which is
// sufficient for any realistic chain reorganization scenario while maintaining
// excellent performance.
//
// Parameters:
// - ctx: Context for the operation
// - oldBlockIDsMap: Map of transaction IDs to their parent block IDs
// - block: Block to check IDs for
// - oldBlockIDsMap: Map of transaction IDs to their parent block IDs (populated by block.Valid())
// - block: Block being validated
//
// Returns an error if block verification fails.
// Returns an error if any transaction references a block not in the ancestry.
func (u *BlockValidation) checkOldBlockIDs(ctx context.Context, oldBlockIDsMap *txmap.SyncedMap[chainhash.Hash, []uint32],
block *model.Block,
) (iterationError error) {
Expand All @@ -1942,73 +1953,53 @@ func (u *BlockValidation) checkOldBlockIDs(ctx context.Context, oldBlockIDsMap *

defer deferFn()

currentChainBlockIDs, err := u.blockchainClient.GetBlockHeaderIDs(ctx, block.Hash(), 10_000)
// Fetch ancestors of the block being validated up to the configured depth limit.
// IMPORTANT: We walk backward from the block being validated, NOT from the current
// best block. This ensures fork blocks are validated against their own ancestry,
// which is required for Bitcoin consensus.
//
// The default 100K limit covers ~695 days of blockchain history (100K blocks × 10 min/block).
// Transactions referencing blocks beyond this depth are extremely rare and likely invalid.
// This limit is configurable via blockvalidation_maxAncestorDepthCheck setting.
maxAncestorDepth := u.settings.BlockValidation.MaxAncestorDepthCheck
blockAncestorIDs, err := u.blockchainClient.GetBlockHeaderIDs(ctx, block.Hash(), maxAncestorDepth)
if err != nil {
return errors.NewServiceError("[Block Validation][checkOldBlockIDs][%s] failed to get block header ids", block.String(), err)
return errors.NewServiceError("[Block Validation][checkOldBlockIDs][%s] failed to get block ancestor ids", block.String(), err)
}

currentChainBlockIDsMap := make(map[uint32]struct{}, len(currentChainBlockIDs))
for _, blockID := range currentChainBlockIDs {
currentChainBlockIDsMap[blockID] = struct{}{}
// Build a map for O(1) lookup performance
blockAncestorIDsMap := make(map[uint32]struct{}, len(blockAncestorIDs))
for _, blockID := range blockAncestorIDs {
blockAncestorIDsMap[blockID] = struct{}{}
}

currentChainLookupCache := make(map[string]bool, len(currentChainBlockIDs))

var builder strings.Builder

// range over the oldBlockIDsMap to get txID - oldBlockID pairs
// Validate that all parent block IDs exist in the block's ancestry
oldBlockIDsMap.Iterate(func(txID chainhash.Hash, blockIDs []uint32) bool {
if len(blockIDs) == 0 {
iterationError = errors.NewProcessingError("[Block Validation][checkOldBlockIDs][%s] blockIDs is empty for txID: %v", block.String(), txID)
return false
}

// check whether the blockIDs are in the current chain we just fetched
// Check if any of the parent blocks exist in this block's ancestry
foundInAncestry := false
for _, blockID := range blockIDs {
if _, ok := currentChainBlockIDsMap[blockID]; ok {
// all good, continue
return true
}
}

slices.Sort(blockIDs)

builder.Reset()

for i, id := range blockIDs {
if i > 0 {
builder.WriteString(",") // Add a separator
}

builder.WriteString(strconv.Itoa(int(id)))
}

blockIDsString := builder.String()

// check whether we already checked exactly the same blockIDs and can use a cache
if blocksPartOfCurrentChain, ok := currentChainLookupCache[blockIDsString]; ok {
if !blocksPartOfCurrentChain {
iterationError = errors.NewBlockInvalidError("[Block Validation][checkOldBlockIDs][%s] block is not valid. Transaction's (%v) parent blocks (%v) are not from current chain using cache", block.String(), txID, blockIDs)
return false
if _, ok := blockAncestorIDsMap[blockID]; ok {
foundInAncestry = true
break
}

return true
}

// Flag to check if the old blocks are part of the current chain
blocksPartOfCurrentChain, err := u.blockchainClient.CheckBlockIsInCurrentChain(ctx, blockIDs)
Copy link
Contributor

@icellan icellan Oct 24, 2025

Choose a reason for hiding this comment

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

Removing this will let all spends from parents beyond 100k blocks fail, right? Why is the database query being removed?

Copy link
Collaborator Author

@liam liam Oct 29, 2025

Choose a reason for hiding this comment

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

CheckBlockIsInCurrentChain actually checks if it's in the longest chain, not the chain we're currently on so it's wrong as far as I can see.

Copy link
Contributor

@icellan icellan Oct 29, 2025

Choose a reason for hiding this comment

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

Then the solution would be to check that it is actually on the chain we are on, not to skip the check?

// if err is not nil, log the error and continue iterating for the next transaction
if err != nil {
iterationError = errors.NewProcessingError("[Block Validation][checkOldBlockIDs][%s] failed to check if old blocks are part of the current chain", block.String(), err)
return false
}

// set the cache for the blockIDs
currentChainLookupCache[blockIDsString] = blocksPartOfCurrentChain

// if the blocks are not part of the current chain, stop iteration, set the iterationError and return false
if !blocksPartOfCurrentChain {
iterationError = errors.NewBlockInvalidError("[Block Validation][checkOldBlockIDs][%s] block is not valid. Transaction's (%v) parent blocks (%v) are not from current chain", block.String(), txID, blockIDs)
if !foundInAncestry {
// CONSENSUS RULE VIOLATION: Transaction references blocks not in this block's ancestry.
// This could mean:
// 1. Parent blocks are on a competing fork (not in this chain's history)
// 2. Parent blocks are >100K deep (extremely unlikely for valid transactions)
// 3. Parent blocks don't exist at all (malformed transaction)
slices.Sort(blockIDs)
iterationError = errors.NewBlockInvalidError(
"[Block Validation][checkOldBlockIDs][%s] block is invalid. Transaction %v references parent blocks %v that are not in this block's ancestry (checked %d ancestors)",
block.String(), txID, blockIDs, len(blockAncestorIDs),
)
return false
}

Expand Down
43 changes: 16 additions & 27 deletions services/blockvalidation/BlockValidation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1991,9 +1991,11 @@ func Test_checkOldBlockIDs(t *testing.T) {
initPrometheusMetrics()

t.Run("empty map", func(t *testing.T) {
tSettings := test.CreateBaseTestSettings(t)
blockchainMock := &blockchain.Mock{}
blockValidation := &BlockValidation{
blockchainClient: blockchainMock,
settings: tSettings,
}

oldBlockIDsMap := txmap.NewSyncedMap[chainhash.Hash, []uint32]()
Expand All @@ -2005,9 +2007,11 @@ func Test_checkOldBlockIDs(t *testing.T) {
})

t.Run("empty parents", func(t *testing.T) {
tSettings := test.CreateBaseTestSettings(t)
blockchainMock := &blockchain.Mock{}
blockValidation := &BlockValidation{
blockchainClient: blockchainMock,
settings: tSettings,
}

oldBlockIDsMap := txmap.NewSyncedMap[chainhash.Hash, []uint32]()
Expand All @@ -2024,10 +2028,12 @@ func Test_checkOldBlockIDs(t *testing.T) {
require.Contains(t, err.Error(), "blockIDs is empty for txID")
})

t.Run("simple map", func(t *testing.T) {
t.Run("parents in ancestry", func(t *testing.T) {
tSettings := test.CreateBaseTestSettings(t)
blockchainMock := &blockchain.Mock{}
blockValidation := &BlockValidation{
blockchainClient: blockchainMock,
settings: tSettings,
}

oldBlockIDsMap := txmap.NewSyncedMap[chainhash.Hash, []uint32]()
Expand All @@ -2041,52 +2047,35 @@ func Test_checkOldBlockIDs(t *testing.T) {
blockIDs = append(blockIDs, i)
}

blockchainMock.On("CheckBlockIsInCurrentChain", mock.Anything, mock.Anything).Return(true, nil)
// All parent blocks are in the ancestry - should pass
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return(blockIDs, nil).Once()

err := blockValidation.checkOldBlockIDs(t.Context(), oldBlockIDsMap, &model.Block{})
require.NoError(t, err)
})

t.Run("lookup and use cache", func(t *testing.T) {
t.Run("parents not in ancestry", func(t *testing.T) {
tSettings := test.CreateBaseTestSettings(t)
blockchainMock := &blockchain.Mock{}
blockValidation := &BlockValidation{
blockchainClient: blockchainMock,
settings: tSettings,
}

oldBlockIDsMap := txmap.NewSyncedMap[chainhash.Hash, []uint32]()

// Set transactions referencing block ID 999
for i := uint32(0); i < 100; i++ {
txHash := chainhash.HashH([]byte(fmt.Sprintf("txHash_%d", i)))
oldBlockIDsMap.Set(txHash, []uint32{1})
oldBlockIDsMap.Set(txHash, []uint32{999})
}

blockchainMock.On("CheckBlockIsInCurrentChain", mock.Anything, mock.Anything).Return(true, nil).Once()
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return([]uint32{}, nil).Once()

err := blockValidation.checkOldBlockIDs(t.Context(), oldBlockIDsMap, &model.Block{})
require.NoError(t, err)
})

t.Run("not present", func(t *testing.T) {
blockchainMock := &blockchain.Mock{}
blockValidation := &BlockValidation{
blockchainClient: blockchainMock,
}

oldBlockIDsMap := txmap.NewSyncedMap[chainhash.Hash, []uint32]()

for i := uint32(0); i < 100; i++ {
txHash := chainhash.HashH([]byte(fmt.Sprintf("txHash_%d", i)))
oldBlockIDsMap.Set(txHash, []uint32{1})
}

blockchainMock.On("CheckBlockIsInCurrentChain", mock.Anything, mock.Anything).Return(false, nil).Once()
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return([]uint32{}, nil).Once()
// Return empty ancestry (block 999 is not in ancestry)
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return([]uint32{1, 2, 3}, nil).Once()

err := blockValidation.checkOldBlockIDs(t.Context(), oldBlockIDsMap, &model.Block{})
require.Error(t, err)
require.Contains(t, err.Error(), "are not from current chain")
require.Contains(t, err.Error(), "not in this block's ancestry")
})
}

Expand Down
1 change: 1 addition & 0 deletions settings/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ type BlockValidationSettings struct {
GRPCListenAddress string
KafkaWorkers int
LocalSetTxMinedConcurrency int
MaxAncestorDepthCheck uint64
MaxPreviousBlockHeadersToCheck uint64
MissingTransactionsBatchSize int
ProcessTxMetaUsingCacheBatchSize int
Expand Down
1 change: 1 addition & 0 deletions settings/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ func NewSettings(alternativeContext ...string) *Settings {
GRPCListenAddress: getString("blockvalidation_grpcListenAddress", ":8088", alternativeContext...),
KafkaWorkers: getInt("blockvalidation_kafkaWorkers", 0, alternativeContext...),
LocalSetTxMinedConcurrency: getInt("blockvalidation_localSetTxMinedConcurrency", 8, alternativeContext...),
MaxAncestorDepthCheck: getUint64("blockvalidation_maxAncestorDepthCheck", 100_000, alternativeContext...),
MaxPreviousBlockHeadersToCheck: getUint64("blockvalidation_maxPreviousBlockHeadersToCheck", 100, alternativeContext...),
MissingTransactionsBatchSize: getInt("blockvalidation_missingTransactionsBatchSize", 5000, alternativeContext...),
ProcessTxMetaUsingCacheBatchSize: getInt("blockvalidation_processTxMetaUsingCache_BatchSize", 1024, alternativeContext...),
Expand Down