From e53e69e25e88562f8c62d16a51c1383b3a60650b Mon Sep 17 00:00:00 2001 From: Simon Ordish <71426+ordishs@users.noreply.github.com> Date: Tue, 28 Apr 2026 19:57:10 +0200 Subject: [PATCH] fix(validator): require BlockIDs+!Conflicting+!Locked before ErrTxNotFound shortcut (refs #4568) --- services/validator/Validator.go | 18 ++- services/validator/Validator_coverage_test.go | 129 +++++++++++++++++- stores/utxo/mock.go | 8 +- 3 files changed, 146 insertions(+), 9 deletions(-) diff --git a/services/validator/Validator.go b/services/validator/Validator.go index 4b74906cb0..453a6a53b5 100644 --- a/services/validator/Validator.go +++ b/services/validator/Validator.go @@ -646,14 +646,20 @@ func (v *Validator) validateInternal(ctx context.Context, tx *bt.Tx, blockHeight return txMetaData, err } } else if errors.Is(err, errors.ErrTxNotFound) { - // the parent transaction was not found, this can happen when the parent tx has been DAH'd and removed from - // the utxo store. We can check whether the tx already exists, which means it has been validated and - // blessed. In this case we can just return early. + // The parent transaction was not found. This can legitimately happen when the parent has been DAH-evicted + // long after the child was mined. Only short-circuit if the stored metadata confirms prior full validation: + // - tx has been included in at least one block (BlockIDs non-empty), AND + // - tx is NOT marked conflicting, AND + // - tx is NOT locked + // Otherwise, surface the original ErrTxNotFound — a "tx exists in store" alone is not proof of validation + // (a re-org or DAH window could expose a stale or mid-flight record). txMetaData = &meta.Data{} - if err = v.utxoStore.GetMeta(decoupledCtx, tx.TxIDChainHash(), txMetaData); err == nil { - v.logger.Warnf("[Validate][%s] parent tx not found, but tx already exists in store, assuming already blessed", txID) + if metaErr := v.utxoStore.GetMeta(decoupledCtx, tx.TxIDChainHash(), txMetaData); metaErr == nil { + if len(txMetaData.BlockIDs) > 0 && !txMetaData.Conflicting && !txMetaData.Locked { + v.logger.Warnf("[Validate][%s] parent tx DAH-evicted, child already mined and not conflicting/locked, assuming blessed (BlockIDs=%v)", txID, txMetaData.BlockIDs) - return txMetaData, nil + return txMetaData, nil + } } } diff --git a/services/validator/Validator_coverage_test.go b/services/validator/Validator_coverage_test.go index c363accb56..a856b4f011 100644 --- a/services/validator/Validator_coverage_test.go +++ b/services/validator/Validator_coverage_test.go @@ -814,17 +814,142 @@ func TestValidator_ValidateInternal_TxNotFoundError_ExistingTx(t *testing.T) { // Mock spendUtxos to return TxNotFound error mockStore.On("Spend", mock.Anything, tx, mock.Anything, mock.Anything).Return([]*utxo.Spend{}, errors.NewTxNotFoundError("tx not found")) - // Mock GetMeta to return existing tx (blessed scenario) - mockStore.On("GetMeta", mock.Anything, mock.Anything, mock.Anything).Return(nil) + // Mock GetMeta to return existing tx already mined and not flagged — legitimate DAH-evicted-parent case. + existingMeta := &meta.Data{Tx: tx, BlockIDs: []uint32{1, 2}} + mockStore.On("GetMeta", mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + data := args.Get(2).(*meta.Data) + *data = *existingMeta + }). + Return(nil) options := &Options{} txMetaData, err := v.validateInternal(ctx, tx, 100, options) assert.NoError(t, err) assert.NotNil(t, txMetaData) + assert.Equal(t, []uint32{1, 2}, txMetaData.BlockIDs) mockStore.AssertExpectations(t) } +// TestValidate_TxNotFoundShortcut verifies that when spendUtxos returns ErrTxNotFound (parent missing), +// the validator only short-circuits with (meta, nil) when the stored metadata genuinely confirms prior +// full validation: tx has been mined (BlockIDs non-empty), is not Conflicting, and is not Locked. +// In all other cases the original ErrTxNotFound must surface to the caller. This guards against a reorg +// or DAH-eviction window where a stale or mid-flight record could otherwise be accepted as "blessed". +func TestValidate_TxNotFoundShortcut(t *testing.T) { + makeTxAndParent := func(t *testing.T) (*bt.Tx, *bt.Tx) { + privateKey, publicKey := bec.PrivateKeyFromBytes([]byte("THIS_IS_A_DETERMINISTIC_PRIVATE_KEY")) + coinbaseTx := transactions.Create(t, + transactions.WithCoinbaseData(100, "/Test miner/"), + transactions.WithP2PKHOutputs(1, 50e8, publicKey), + ) + tx := transactions.Create(t, + transactions.WithPrivateKey(privateKey), + transactions.WithInput(coinbaseTx, 0), + transactions.WithP2PKHOutputs(1, 1000), + transactions.WithChangeOutput(), + ) + return tx, coinbaseTx + } + + setupValidator := func(t *testing.T, tx *bt.Tx, coinbaseTx *bt.Tx, getMetaResult *meta.Data, getMetaErr error) (*Validator, *utxo.MockUtxostore) { + ctx := context.Background() + logger := ulogger.TestLogger{} + mockStore := &utxo.MockUtxostore{} + settings := test.CreateBaseTestSettings(t) + + validator, err := New(ctx, logger, settings, mockStore, nil, nil, nil, nil) + require.NoError(t, err) + v := validator.(*Validator) + + parentTxMeta := &meta.Data{Tx: coinbaseTx, BlockHeights: []uint32{}} + mockStore.On("Get", mock.Anything, mock.Anything, mock.Anything).Return(parentTxMeta, nil) + mockStore.On("GetBlockState").Return(utxo.BlockState{Height: 100, MedianTime: 1000000000}) + mockStore.On("Spend", mock.Anything, tx, mock.Anything, mock.Anything).Return([]*utxo.Spend{}, errors.NewTxNotFoundError("tx not found")) + if getMetaErr != nil { + mockStore.On("GetMeta", mock.Anything, mock.Anything, mock.Anything).Return(getMetaErr) + } else { + mockStore.On("GetMeta", mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + data := args.Get(2).(*meta.Data) + *data = *getMetaResult + }). + Return(nil) + } + + return v, mockStore + } + + t.Run("shortcut allowed when mined and not flagged", func(t *testing.T) { + tx, coinbaseTx := makeTxAndParent(t) + existingMeta := &meta.Data{Tx: tx, BlockIDs: []uint32{1, 2}, Conflicting: false, Locked: false} + v, mockStore := setupValidator(t, tx, coinbaseTx, existingMeta, nil) + + txMetaData, err := v.validateInternal(context.Background(), tx, 100, &Options{}) + + require.NoError(t, err) + require.NotNil(t, txMetaData) + require.Equal(t, []uint32{1, 2}, txMetaData.BlockIDs) + mockStore.AssertExpectations(t) + }) + + t.Run("shortcut denied when not yet mined (BlockIDs empty)", func(t *testing.T) { + tx, coinbaseTx := makeTxAndParent(t) + notYetMined := &meta.Data{Tx: tx, BlockIDs: nil, Conflicting: false, Locked: false} + v, mockStore := setupValidator(t, tx, coinbaseTx, notYetMined, nil) + + txMetaData, err := v.validateInternal(context.Background(), tx, 100, &Options{}) + + require.Error(t, err) + require.Nil(t, txMetaData) + require.True(t, errors.Is(err, errors.ErrTxNotFound), "expected wrapped ErrTxNotFound, got: %v", err) + require.Contains(t, err.Error(), "error spending utxos") + mockStore.AssertExpectations(t) + }) + + t.Run("shortcut denied when conflicting", func(t *testing.T) { + tx, coinbaseTx := makeTxAndParent(t) + conflicting := &meta.Data{Tx: tx, BlockIDs: []uint32{1}, Conflicting: true, Locked: false} + v, mockStore := setupValidator(t, tx, coinbaseTx, conflicting, nil) + + txMetaData, err := v.validateInternal(context.Background(), tx, 100, &Options{}) + + require.Error(t, err) + require.Nil(t, txMetaData) + require.True(t, errors.Is(err, errors.ErrTxNotFound), "expected wrapped ErrTxNotFound, got: %v", err) + require.Contains(t, err.Error(), "error spending utxos") + mockStore.AssertExpectations(t) + }) + + t.Run("shortcut denied when locked", func(t *testing.T) { + tx, coinbaseTx := makeTxAndParent(t) + locked := &meta.Data{Tx: tx, BlockIDs: []uint32{1}, Conflicting: false, Locked: true} + v, mockStore := setupValidator(t, tx, coinbaseTx, locked, nil) + + txMetaData, err := v.validateInternal(context.Background(), tx, 100, &Options{}) + + require.Error(t, err) + require.Nil(t, txMetaData) + require.True(t, errors.Is(err, errors.ErrTxNotFound), "expected wrapped ErrTxNotFound, got: %v", err) + require.Contains(t, err.Error(), "error spending utxos") + mockStore.AssertExpectations(t) + }) + + t.Run("shortcut denied when GetMeta itself fails", func(t *testing.T) { + tx, coinbaseTx := makeTxAndParent(t) + v, mockStore := setupValidator(t, tx, coinbaseTx, nil, errors.NewTxNotFoundError("meta not found")) + + txMetaData, err := v.validateInternal(context.Background(), tx, 100, &Options{}) + + require.Error(t, err) + require.Nil(t, txMetaData) + require.True(t, errors.Is(err, errors.ErrTxNotFound), "expected wrapped ErrTxNotFound, got: %v", err) + require.Contains(t, err.Error(), "error spending utxos") + mockStore.AssertExpectations(t) + }) +} + func TestValidator_ValidateInternal_GeneralSpendError(t *testing.T) { ctx := context.Background() logger := ulogger.TestLogger{} diff --git a/stores/utxo/mock.go b/stores/utxo/mock.go index 678523cd19..6251ffa433 100644 --- a/stores/utxo/mock.go +++ b/stores/utxo/mock.go @@ -71,10 +71,16 @@ func (m *MockUtxostore) GetSpend(ctx context.Context, spend *Spend) (*SpendRespo // GetMeta mocks the retrieval of complete transaction metadata from the UTXO store. // Returns the configured mock response for full metadata lookup operations. +// +// Accepts either Return(*meta.Data) for the success path (data populated, no error), +// Return(nil) for an empty metadata result, or Return(error) to surface a lookup failure. func (m *MockUtxostore) GetMeta(ctx context.Context, hash *chainhash.Hash, data *meta.Data) error { args := m.Called(ctx, hash, data) if result := args.Get(0); result != nil { - *data = *result.(*meta.Data) + if md, ok := result.(*meta.Data); ok { + *data = *md + return nil + } } return args.Error(0) }