Use removeRedeemers correctly (#2235)

* Use removeRedeemers correctly

* Fix topological iteration

* Some fixes

* Ignore RejectDuplicate and swallow other rule errors

* Fix typo

* Don't remove redeemers and skip mempool full revalidation

domain/consensus/model/testapi/test_consensus.go

@@ -43,6 +43,9 @@ type TestConsensus interface {
 	AddBlock(parentHashes []*externalapi.DomainHash, coinbaseData *externalapi.DomainCoinbaseData,
 		transactions []*externalapi.DomainTransaction) (*externalapi.DomainHash, *externalapi.VirtualChangeSet, error)
 
+	AddBlockOnTips(coinbaseData *externalapi.DomainCoinbaseData,
+		transactions []*externalapi.DomainTransaction) (*externalapi.DomainHash, *externalapi.VirtualChangeSet, error)
+
 	AddUTXOInvalidHeader(parentHashes []*externalapi.DomainHash) (*externalapi.DomainHash, *externalapi.VirtualChangeSet, error)
 
 	AddUTXOInvalidBlock(parentHashes []*externalapi.DomainHash) (*externalapi.DomainHash,

domain/consensus/test_consensus.go

@@ -69,6 +69,17 @@ func (tc *testConsensus) AddBlock(parentHashes []*externalapi.DomainHash, coinba
 	return consensushashing.BlockHash(block), virtualChangeSet, nil
 }
 
+func (tc *testConsensus) AddBlockOnTips(coinbaseData *externalapi.DomainCoinbaseData,
+	transactions []*externalapi.DomainTransaction) (*externalapi.DomainHash, *externalapi.VirtualChangeSet, error) {
+
+	tips, err := tc.Tips()
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return tc.AddBlock(tips, coinbaseData, transactions)
+}
+
 func (tc *testConsensus) AddUTXOInvalidHeader(parentHashes []*externalapi.DomainHash) (*externalapi.DomainHash,
 	*externalapi.VirtualChangeSet, error) {
 

domain/miningmanager/blocktemplatebuilder/blocktemplatebuilder.go

@@ -147,16 +147,12 @@ func (btb *blockTemplateBuilder) BuildBlockTemplate(
 	invalidTxsErr := ruleerrors.ErrInvalidTransactionsInNewBlock{}
 	if errors.As(err, &invalidTxsErr) {
 		log.Criticalf("consensusReference.Consensus().BuildBlock returned invalid txs in BuildBlockTemplate")
-		invalidTxs := make([]*consensusexternalapi.DomainTransaction, 0, len(invalidTxsErr.InvalidTransactions))
-		for _, tx := range invalidTxsErr.InvalidTransactions {
-			invalidTxs = append(invalidTxs, tx.Transaction)
-		}
-		err = btb.mempool.RemoveTransactions(invalidTxs, true)
+		err = btb.mempool.RemoveInvalidTransactions(&invalidTxsErr)
 		if err != nil {
-			// mempool.RemoveTransactions might return errors in situations that are perfectly fine in this context.
+			// mempool.RemoveInvalidTransactions might return errors in situations that are perfectly fine in this context.
 			// TODO: Once the mempool invariants are clear, this should be converted back `return nil, err`:
 			// https://github.com/kaspanet/kaspad/issues/1553
-			log.Criticalf("Error from mempool.RemoveTransactions: %+v", err)
+			log.Criticalf("Error from mempool.RemoveInvalidTransactions: %+v", err)
 		}
 		// We can call this recursively without worry because this should almost never happen
 		return btb.BuildBlockTemplate(coinbaseData)

domain/miningmanager/mempool/mempool.go

@@ -1,6 +1,8 @@
 package mempool
 
 import (
+	"github.com/kaspanet/kaspad/domain/consensus/ruleerrors"
+	"github.com/kaspanet/kaspad/domain/consensus/utils/consensushashing"
 	"sync"
 
 	"github.com/kaspanet/kaspad/domain/consensusreference"
@@ -151,11 +153,29 @@ func (mp *mempool) RevalidateHighPriorityTransactions() (validTransactions []*ex
 	return mp.revalidateHighPriorityTransactions()
 }
 
-func (mp *mempool) RemoveTransactions(transactions []*externalapi.DomainTransaction, removeRedeemers bool) error {
+func (mp *mempool) RemoveInvalidTransactions(err *ruleerrors.ErrInvalidTransactionsInNewBlock) error {
 	mp.mtx.Lock()
 	defer mp.mtx.Unlock()
 
-	return mp.removeTransactions(transactions, removeRedeemers)
+	for _, tx := range err.InvalidTransactions {
+		ruleErr, success := tx.Error.(ruleerrors.RuleError)
+		if !success {
+			continue
+		}
+
+		inner := ruleErr.Unwrap()
+		removeRedeemers := true
+		if _, ok := inner.(ruleerrors.ErrMissingTxOut); ok {
+			removeRedeemers = false
+		}
+
+		err := mp.removeTransaction(consensushashing.TransactionID(tx.Transaction), removeRedeemers)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 func (mp *mempool) RemoveTransaction(transactionID *externalapi.DomainTransactionID, removeRedeemers bool) error {

domain/miningmanager/mempool/remove_transaction.go

@@ -2,20 +2,9 @@ package mempool
 
 import (
 	"github.com/kaspanet/kaspad/domain/consensus/model/externalapi"
-	"github.com/kaspanet/kaspad/domain/consensus/utils/consensushashing"
 	"github.com/kaspanet/kaspad/domain/miningmanager/mempool/model"
 )
 
-func (mp *mempool) removeTransactions(transactions []*externalapi.DomainTransaction, removeRedeemers bool) error {
-	for _, transaction := range transactions {
-		err := mp.removeTransaction(consensushashing.TransactionID(transaction), removeRedeemers)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 func (mp *mempool) removeTransaction(transactionID *externalapi.DomainTransactionID, removeRedeemers bool) error {
 	if _, ok := mp.orphansPool.allOrphans[*transactionID]; ok {
 		return mp.orphansPool.removeOrphan(transactionID, true)

domain/miningmanager/mempool/revalidate_high_priority_transactions.go

@@ -7,20 +7,85 @@ import (
 )
 
 func (mp *mempool) revalidateHighPriorityTransactions() ([]*externalapi.DomainTransaction, error) {
+	type txNode struct {
+		children          map[externalapi.DomainTransactionID]struct{}
+		nonVisitedParents int
+		tx                *model.MempoolTransaction
+		visited           bool
+	}
+
 	onEnd := logger.LogAndMeasureExecutionTime(log, "revalidateHighPriorityTransactions")
 	defer onEnd()
 
+	// We revalidate transactions in topological order in case there are dependencies between them
+
+	// Naturally transactions point to their dependencies, but since we want to start processing the dependencies
+	// first, we build the opposite DAG. We initially fill `queue` with transactions with no dependencies.
+	txDAG := make(map[externalapi.DomainTransactionID]*txNode)
+
+	maybeAddNode := func(txID externalapi.DomainTransactionID) *txNode {
+		if node, ok := txDAG[txID]; ok {
+			return node
+		}
+
+		node := &txNode{
+			children:          make(map[externalapi.DomainTransactionID]struct{}),
+			nonVisitedParents: 0,
+			tx:                mp.transactionsPool.highPriorityTransactions[txID],
+		}
+		txDAG[txID] = node
+		return node
+	}
+
+	queue := make([]*txNode, 0, len(mp.transactionsPool.highPriorityTransactions))
+	for id, transaction := range mp.transactionsPool.highPriorityTransactions {
+		node := maybeAddNode(id)
+
+		parents := make(map[externalapi.DomainTransactionID]struct{})
+		for _, input := range transaction.Transaction().Inputs {
+			if _, ok := mp.transactionsPool.highPriorityTransactions[input.PreviousOutpoint.TransactionID]; !ok {
+				continue
+			}
+
+			parents[input.PreviousOutpoint.TransactionID] = struct{}{} // To avoid duplicate parents, we first add it to a set and then count it
+			maybeAddNode(input.PreviousOutpoint.TransactionID).children[id] = struct{}{}
+		}
+		node.nonVisitedParents = len(parents)
+
+		if node.nonVisitedParents == 0 {
+			queue = append(queue, node)
+		}
+	}
+
 	validTransactions := []*externalapi.DomainTransaction{}
-	for _, transaction := range mp.transactionsPool.highPriorityTransactions {
+
+	// Now we iterate the DAG in topological order using BFS
+	for len(queue) > 0 {
+		var node *txNode
+		node, queue = queue[0], queue[1:]
+
+		if node.visited {
+			continue
+		}
+		node.visited = true
+
+		transaction := node.tx
 		isValid, err := mp.revalidateTransaction(transaction)
 		if err != nil {
 			return nil, err
 		}
-		if !isValid {
-			continue
+
+		for child := range node.children {
+			childNode := txDAG[child]
+			childNode.nonVisitedParents--
+			if childNode.nonVisitedParents == 0 {
+				queue = append(queue, txDAG[child])
+			}
 		}
 
-		validTransactions = append(validTransactions, transaction.Transaction().Clone())
+		if isValid {
+			validTransactions = append(validTransactions, transaction.Transaction().Clone())
+		}
 	}
 
 	return validTransactions, nil
@@ -35,7 +100,7 @@ func (mp *mempool) revalidateTransaction(transaction *model.MempoolTransaction)
 	}
 	if len(missingParents) > 0 {
 		log.Debugf("Removing transaction %s, it failed revalidation", transaction.TransactionID())
-		err := mp.removeTransaction(transaction.TransactionID(), true)
+		err := mp.removeTransaction(transaction.TransactionID(), false)
 		if err != nil {
 			return false, err
 		}

domain/miningmanager/miningmanager_test.go

@@ -577,6 +577,72 @@ func TestRevalidateHighPriorityTransactions(t *testing.T) {
 	})
 }
 
+func TestRevalidateHighPriorityTransactionsWithChain(t *testing.T) {
+	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
+		consensusConfig.BlockCoinbaseMaturity = 0
+		factory := consensus.NewFactory()
+		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestRevalidateHighPriorityTransactions")
+		if err != nil {
+			t.Fatalf("Failed setting up TestConsensus: %+v", err)
+		}
+		defer teardown(false)
+
+		miningFactory := miningmanager.NewFactory()
+		mempoolConfig := mempool.DefaultConfig(&consensusConfig.Params)
+		tcAsConsensus := tc.(externalapi.Consensus)
+		tcAsConsensusPointer := &tcAsConsensus
+		consensusReference := consensusreference.NewConsensusReference(&tcAsConsensusPointer)
+		miningManager := miningFactory.NewMiningManager(consensusReference, &consensusConfig.Params, mempoolConfig)
+
+		const chainSize = 10
+		chain, err := createTxChain(tc, chainSize)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		_, err = miningManager.ValidateAndInsertTransaction(chain[0], true, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		blockHash, _, err := tc.AddBlockOnTips(nil, []*externalapi.DomainTransaction{chain[0].Clone()})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		block, _, err := tc.GetBlock(blockHash)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		_, err = miningManager.HandleNewBlockTransactions(block.Transactions)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		for _, transaction := range chain[1:] {
+			_, err = miningManager.ValidateAndInsertTransaction(transaction, true, false)
+			if err != nil {
+				t.Fatal(err)
+			}
+		}
+
+		_, _, err = tc.AddBlockOnTips(nil, []*externalapi.DomainTransaction{chain[1].Clone()})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		revalidated, err := miningManager.RevalidateHighPriorityTransactions()
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if len(revalidated) != chainSize-2 {
+			t.Fatalf("expected %d transactions to revalidate but instead only %d revalidated", chainSize-2, len(revalidated))
+		}
+	})
+}
+
 // TestModifyBlockTemplate verifies that modifying a block template changes coinbase data correctly.
 func TestModifyBlockTemplate(t *testing.T) {
 	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
@@ -904,40 +970,58 @@ func createArraysOfParentAndChildrenTransactions(tc testapi.TestConsensus) ([]*e
 func createParentAndChildrenTransactions(tc testapi.TestConsensus) (txParent *externalapi.DomainTransaction,
 	txChild *externalapi.DomainTransaction, err error) {
 
+	chain, err := createTxChain(tc, 2)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return chain[0], chain[1], nil
+}
+
+func createTxChain(tc testapi.TestConsensus, numTxs int) ([]*externalapi.DomainTransaction, error) {
 	// We will add two blocks by consensus before the parent transactions, in order to fund the parent transactions.
 	tips, err := tc.Tips()
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	_, _, err = tc.AddBlock(tips, nil, nil)
 	if err != nil {
-		return nil, nil, errors.Wrapf(err, "AddBlock: %v", err)
+		return nil, errors.Wrapf(err, "AddBlock: %v", err)
 	}
 
 	tips, err = tc.Tips()
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	fundingBlockHashForParent, _, err := tc.AddBlock(tips, nil, nil)
 	if err != nil {
-		return nil, nil, errors.Wrap(err, "AddBlock: ")
+		return nil, errors.Wrap(err, "AddBlock: ")
 	}
 	fundingBlockForParent, _, err := tc.GetBlock(fundingBlockHashForParent)
 	if err != nil {
-		return nil, nil, errors.Wrap(err, "GetBlock: ")
+		return nil, errors.Wrap(err, "GetBlock: ")
 	}
 	fundingTransactionForParent := fundingBlockForParent.Transactions[transactionhelper.CoinbaseTransactionIndex]
-	txParent, err = testutils.CreateTransaction(fundingTransactionForParent, 1000)
+
+	transactions := make([]*externalapi.DomainTransaction, numTxs)
+	transactions[0], err = testutils.CreateTransaction(fundingTransactionForParent, 1000)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
-	txChild, err = testutils.CreateTransaction(txParent, 1000)
-	if err != nil {
-		return nil, nil, err
+
+	txParent := transactions[0]
+	for i := 1; i < numTxs; i++ {
+		transactions[i], err = testutils.CreateTransaction(txParent, 1000)
+		if err != nil {
+			return nil, err
+		}
+
+		txParent = transactions[i]
 	}
-	return txParent, txChild, nil
+
+	return transactions, nil
 }
 
 func createChildAndParentTxsAndAddParentToConsensus(tc testapi.TestConsensus) (*externalapi.DomainTransaction, error) {

domain/miningmanager/model/interface_mempool.go

@@ -2,6 +2,7 @@ package model
 
 import (
 	"github.com/kaspanet/kaspad/domain/consensus/model/externalapi"
+	"github.com/kaspanet/kaspad/domain/consensus/ruleerrors"
 )
 
 // Mempool maintains a set of known transactions that
@@ -11,7 +12,7 @@ type Mempool interface {
 	BlockCandidateTransactions() []*externalapi.DomainTransaction
 	ValidateAndInsertTransaction(transaction *externalapi.DomainTransaction, isHighPriority bool, allowOrphan bool) (
 		acceptedTransactions []*externalapi.DomainTransaction, err error)
-	RemoveTransactions(txs []*externalapi.DomainTransaction, removeRedeemers bool) error
+	RemoveInvalidTransactions(err *ruleerrors.ErrInvalidTransactionsInNewBlock) error
 	GetTransaction(
 		transactionID *externalapi.DomainTransactionID,
 		includeTransactionPool bool,

domain/utxoindex/utxoindex.go

@@ -52,6 +52,8 @@ func (ui *UTXOIndex) Reset() error {
 	ui.mutex.Lock()
 	defer ui.mutex.Unlock()
 
+	log.Infof("Starting UTXO index reset")
+
 	err := ui.store.deleteAll()
 	if err != nil {
 		return err
@@ -88,7 +90,13 @@ func (ui *UTXOIndex) Reset() error {
 	}
 
 	// This has to be done last to mark that the reset went smoothly and no reset has to be called next time.
-	return ui.store.updateAndCommitVirtualParentsWithoutTransaction(virtualInfo.ParentHashes)
+	err = ui.store.updateAndCommitVirtualParentsWithoutTransaction(virtualInfo.ParentHashes)
+	if err != nil {
+		return err
+	}
+
+	log.Infof("Finished UTXO index reset")
+	return nil
 }
 
 func (ui *UTXOIndex) isSynced() (bool, error) {