Skip to content

Commit 1dd47db

Browse files
oskarszoonoskarszoon
committed
Merge PR #52: Block validation fix for checkOldBlockIDs
2 parents 3c18f98 + 31d35c6 commit 1dd47db

File tree

4 files changed

+68
-86
lines changed

4 files changed

+68
-86
lines changed

‎services/blockvalidation/BlockValidation.go‎

Lines changed: 50 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@ import (
1818
"context"
1919
"fmt"
2020
"slices"
21-
"strconv"
22-
"strings"
2321
"sync"
2422
"sync/atomic"
2523
"time"
@@ -1924,15 +1922,28 @@ func (u *BlockValidation) validateBlockSubtrees(ctx context.Context, block *mode
19241922
return u.subtreeValidationClient.CheckBlockSubtrees(ctx, block, baseURL)
19251923
}
19261924

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

19431954
defer deferFn()
19441955

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

1950-
currentChainBlockIDsMap := make(map[uint32]struct{}, len(currentChainBlockIDs))
1951-
for _, blockID := range currentChainBlockIDs {
1952-
currentChainBlockIDsMap[blockID] = struct{}{}
1970+
// Build a map for O(1) lookup performance
1971+
blockAncestorIDsMap := make(map[uint32]struct{}, len(blockAncestorIDs))
1972+
for _, blockID := range blockAncestorIDs {
1973+
blockAncestorIDsMap[blockID] = struct{}{}
19531974
}
19541975

1955-
currentChainLookupCache := make(map[string]bool, len(currentChainBlockIDs))
1956-
1957-
var builder strings.Builder
1958-
1959-
// range over the oldBlockIDsMap to get txID - oldBlockID pairs
1976+
// Validate that all parent block IDs exist in the block's ancestry
19601977
oldBlockIDsMap.Iterate(func(txID chainhash.Hash, blockIDs []uint32) bool {
19611978
if len(blockIDs) == 0 {
19621979
iterationError = errors.NewProcessingError("[Block Validation][checkOldBlockIDs][%s] blockIDs is empty for txID: %v", block.String(), txID)
19631980
return false
19641981
}
19651982

1966-
// check whether the blockIDs are in the current chain we just fetched
1983+
// Check if any of the parent blocks exist in this block's ancestry
1984+
foundInAncestry := false
19671985
for _, blockID := range blockIDs {
1968-
if _, ok := currentChainBlockIDsMap[blockID]; ok {
1969-
// all good, continue
1970-
return true
1971-
}
1972-
}
1973-
1974-
slices.Sort(blockIDs)
1975-
1976-
builder.Reset()
1977-
1978-
for i, id := range blockIDs {
1979-
if i > 0 {
1980-
builder.WriteString(",") // Add a separator
1981-
}
1982-
1983-
builder.WriteString(strconv.Itoa(int(id)))
1984-
}
1985-
1986-
blockIDsString := builder.String()
1987-
1988-
// check whether we already checked exactly the same blockIDs and can use a cache
1989-
if blocksPartOfCurrentChain, ok := currentChainLookupCache[blockIDsString]; ok {
1990-
if !blocksPartOfCurrentChain {
1991-
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)
1992-
return false
1986+
if _, ok := blockAncestorIDsMap[blockID]; ok {
1987+
foundInAncestry = true
1988+
break
19931989
}
1994-
1995-
return true
19961990
}
19971991

1998-
// Flag to check if the old blocks are part of the current chain
1999-
blocksPartOfCurrentChain, err := u.blockchainClient.CheckBlockIsInCurrentChain(ctx, blockIDs)
2000-
// if err is not nil, log the error and continue iterating for the next transaction
2001-
if err != nil {
2002-
iterationError = errors.NewProcessingError("[Block Validation][checkOldBlockIDs][%s] failed to check if old blocks are part of the current chain", block.String(), err)
2003-
return false
2004-
}
2005-
2006-
// set the cache for the blockIDs
2007-
currentChainLookupCache[blockIDsString] = blocksPartOfCurrentChain
2008-
2009-
// if the blocks are not part of the current chain, stop iteration, set the iterationError and return false
2010-
if !blocksPartOfCurrentChain {
2011-
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)
1992+
if !foundInAncestry {
1993+
// CONSENSUS RULE VIOLATION: Transaction references blocks not in this block's ancestry.
1994+
// This could mean:
1995+
// 1. Parent blocks are on a competing fork (not in this chain's history)
1996+
// 2. Parent blocks are >100K deep (extremely unlikely for valid transactions)
1997+
// 3. Parent blocks don't exist at all (malformed transaction)
1998+
slices.Sort(blockIDs)
1999+
iterationError = errors.NewBlockInvalidError(
2000+
"[Block Validation][checkOldBlockIDs][%s] block is invalid. Transaction %v references parent blocks %v that are not in this block's ancestry (checked %d ancestors)",
2001+
block.String(), txID, blockIDs, len(blockAncestorIDs),
2002+
)
20122003
return false
20132004
}
20142005

‎services/blockvalidation/BlockValidation_test.go‎

Lines changed: 16 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1991,9 +1991,11 @@ func Test_checkOldBlockIDs(t *testing.T) {
19911991
initPrometheusMetrics()
19921992

19931993
t.Run("empty map", func(t *testing.T) {
1994+
tSettings := test.CreateBaseTestSettings(t)
19941995
blockchainMock := &blockchain.Mock{}
19951996
blockValidation := &BlockValidation{
19961997
blockchainClient: blockchainMock,
1998+
settings: tSettings,
19971999
}
19982000

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

20072009
t.Run("empty parents", func(t *testing.T) {
2010+
tSettings := test.CreateBaseTestSettings(t)
20082011
blockchainMock := &blockchain.Mock{}
20092012
blockValidation := &BlockValidation{
20102013
blockchainClient: blockchainMock,
2014+
settings: tSettings,
20112015
}
20122016

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

2027-
t.Run("simple map", func(t *testing.T) {
2031+
t.Run("parents in ancestry", func(t *testing.T) {
2032+
tSettings := test.CreateBaseTestSettings(t)
20282033
blockchainMock := &blockchain.Mock{}
20292034
blockValidation := &BlockValidation{
20302035
blockchainClient: blockchainMock,
2036+
settings: tSettings,
20312037
}
20322038

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

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

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

2051-
t.Run("lookup and use cache", func(t *testing.T) {
2057+
t.Run("parents not in ancestry", func(t *testing.T) {
2058+
tSettings := test.CreateBaseTestSettings(t)
20522059
blockchainMock := &blockchain.Mock{}
20532060
blockValidation := &BlockValidation{
20542061
blockchainClient: blockchainMock,
2062+
settings: tSettings,
20552063
}
20562064

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

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

2064-
blockchainMock.On("CheckBlockIsInCurrentChain", mock.Anything, mock.Anything).Return(true, nil).Once()
2065-
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return([]uint32{}, nil).Once()
2066-
2067-
err := blockValidation.checkOldBlockIDs(t.Context(), oldBlockIDsMap, &model.Block{})
2068-
require.NoError(t, err)
2069-
})
2070-
2071-
t.Run("not present", func(t *testing.T) {
2072-
blockchainMock := &blockchain.Mock{}
2073-
blockValidation := &BlockValidation{
2074-
blockchainClient: blockchainMock,
2075-
}
2076-
2077-
oldBlockIDsMap := txmap.NewSyncedMap[chainhash.Hash, []uint32]()
2078-
2079-
for i := uint32(0); i < 100; i++ {
2080-
txHash := chainhash.HashH([]byte(fmt.Sprintf("txHash_%d", i)))
2081-
oldBlockIDsMap.Set(txHash, []uint32{1})
2082-
}
2083-
2084-
blockchainMock.On("CheckBlockIsInCurrentChain", mock.Anything, mock.Anything).Return(false, nil).Once()
2085-
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return([]uint32{}, nil).Once()
2073+
// Return empty ancestry (block 999 is not in ancestry)
2074+
blockchainMock.On("GetBlockHeaderIDs", mock.Anything, mock.Anything, mock.Anything).Return([]uint32{1, 2, 3}, nil).Once()
20862075

20872076
err := blockValidation.checkOldBlockIDs(t.Context(), oldBlockIDsMap, &model.Block{})
20882077
require.Error(t, err)
2089-
require.Contains(t, err.Error(), "are not from current chain")
2078+
require.Contains(t, err.Error(), "not in this block's ancestry")
20902079
})
20912080
}
20922081

‎settings/interface.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ type BlockValidationSettings struct {
250250
GRPCListenAddress string
251251
KafkaWorkers int
252252
LocalSetTxMinedConcurrency int
253+
MaxAncestorDepthCheck uint64
253254
MaxPreviousBlockHeadersToCheck uint64
254255
MissingTransactionsBatchSize int
255256
ProcessTxMetaUsingCacheBatchSize int

‎settings/settings.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,7 @@ func NewSettings(alternativeContext ...string) *Settings {
241241
GRPCListenAddress: getString("blockvalidation_grpcListenAddress", ":8088", alternativeContext...),
242242
KafkaWorkers: getInt("blockvalidation_kafkaWorkers", 0, alternativeContext...),
243243
LocalSetTxMinedConcurrency: getInt("blockvalidation_localSetTxMinedConcurrency", 8, alternativeContext...),
244+
MaxAncestorDepthCheck: getUint64("blockvalidation_maxAncestorDepthCheck", 100_000, alternativeContext...),
244245
MaxPreviousBlockHeadersToCheck: getUint64("blockvalidation_maxPreviousBlockHeadersToCheck", 100, alternativeContext...),
245246
MissingTransactionsBatchSize: getInt("blockvalidation_missingTransactionsBatchSize", 5000, alternativeContext...),
246247
ProcessTxMetaUsingCacheBatchSize: getInt("blockvalidation_processTxMetaUsingCache_BatchSize", 1024, alternativeContext...),

0 commit comments

Comments
 (0)