add TestBlockEvaluator for methods to hang off of

data/pools/transactionPool.go

@@ -98,8 +98,8 @@ type TransactionPool struct {
 	// exceed the txPoolMaxSize. This flag is reset to false OnNewBlock
 	stateproofOverflowed bool
 
-	testEvalCtx   *testEvalContext
-	testEvalCtxMu deadlock.RWMutex
+	testBlockEvaluator   *eval.TestBlockEvaluator
+	testBlockEvaluatorMu deadlock.RWMutex
 
 	// shutdown is set to true when the pool is being shut down. It is checked in exported methods
 	// to prevent pool operations like remember and recomputing the block evaluator
@@ -109,7 +109,6 @@ type TransactionPool struct {
 
 // BlockEvaluator defines the block evaluator interface exposed by the ledger package.
 type BlockEvaluator interface {
-	TestTransactionGroup(txgroup []transactions.SignedTxn) error
 	Round() basics.Round
 	PaySetSize() int
 	TransactionGroup(txads []transactions.SignedTxnWithAD) error
@@ -118,30 +117,33 @@ type BlockEvaluator interface {
 	ResetTxnBytes()
 }
 
-// testEvalContext implements the eval.TestEvalContext interface.
+// testEvalContext implements the eval.TestEvalContext interface. It allows for concurrent
+// calls to TestTransactionGroup for candidate transactions calling transactionPool.Test.
 type testEvalContext struct {
 	ledger   *ledger.Ledger
 	block    bookkeeping.Block
 	proto    config.ConsensusParams
 	specials transactions.SpecialAddresses
 }
 
-func newTestEvalCtx(ledger *ledger.Ledger, block bookkeeping.Block) *testEvalContext {
-	return &testEvalContext{
-		ledger: ledger,
-		block:  block,
-		proto:  config.Consensus[block.CurrentProtocol],
-		specials: transactions.SpecialAddresses{
-			FeeSink:     block.FeeSink,
-			RewardsPool: block.RewardsPool,
-		},
-	}
+func newTestBlockEvaluator(ledger *ledger.Ledger, block bookkeeping.Block) *eval.TestBlockEvaluator {
+	return &eval.TestBlockEvaluator{
+		TestEvalContext: &testEvalContext{
+			ledger: ledger,
+			block:  block,
+			proto:  config.Consensus[block.CurrentProtocol],
+			specials: transactions.SpecialAddresses{
+				FeeSink:     block.FeeSink,
+				RewardsPool: block.RewardsPool,
+			},
+		}}
 }
 
 func (c *testEvalContext) Proto() config.ConsensusParams           { return c.proto }
 func (c *testEvalContext) Specials() transactions.SpecialAddresses { return c.specials }
 func (c *testEvalContext) TxnContext() transactions.TxnContext     { return c.block }
 func (c *testEvalContext) CheckDup(firstValid, lastValid basics.Round, txid transactions.Txid, txl ledgercore.Txlease) error {
+	// will call txTail.checkDup, which uses an RLock for concurrent access.
 	return c.ledger.CheckDup(c.proto, c.block.BlockHeader.Round, firstValid, lastValid, txid, txl)
 }
 
@@ -421,19 +423,20 @@ func (pool *TransactionPool) checkSufficientFee(txgroup []transactions.SignedTxn
 
 // Test performs basic duplicate detection and well-formedness checks
 // on a transaction group without storing the group.
+// It may be called concurrently.
 func (pool *TransactionPool) Test(txgroup []transactions.SignedTxn) error {
 	if err := pool.checkPendingQueueSize(txgroup); err != nil {
 		return err
 	}
 
-	pool.testEvalCtxMu.RLock()
-	defer pool.testEvalCtxMu.RUnlock()
+	pool.testBlockEvaluatorMu.RLock()
+	defer pool.testBlockEvaluatorMu.RUnlock()
 
-	if pool.testEvalCtx == nil {
+	if pool.testBlockEvaluator == nil {
 		return fmt.Errorf("Test: testEvalCtx is nil")
 	}
 
-	return eval.TestTransactionGroup(pool.testEvalCtx, txgroup)
+	return pool.testBlockEvaluator.TestTransactionGroup(txgroup)
 }
 
 type poolIngestParams struct {
@@ -780,9 +783,9 @@ func (pool *TransactionPool) recomputeBlockEvaluator(committedTxIDs map[transact
 		return
 	}
 
-	pool.testEvalCtxMu.Lock()
-	pool.testEvalCtx = newTestEvalCtx(pool.ledger, next)
-	pool.testEvalCtxMu.Unlock()
+	pool.testBlockEvaluatorMu.Lock()
+	pool.testBlockEvaluator = newTestBlockEvaluator(pool.ledger, next)
+	pool.testBlockEvaluatorMu.Unlock()
 
 	var asmStats telemetryspec.AssembleBlockMetrics
 	asmStats.StartCount = len(txgroups)

ledger/eval/eval.go

@@ -932,12 +932,19 @@ func (eval *BlockEvaluator) CheckDup(firstValid, lastValid basics.Round, txid tr
 // on a transaction group, but does not actually add the transactions to the block
 // evaluator, or modify the block evaluator state in any other visible way.
 func (eval *BlockEvaluator) TestTransactionGroup(txgroup []transactions.SignedTxn) error {
-	return TestTransactionGroup(eval, txgroup)
+	return TestBlockEvaluator{eval}.TestTransactionGroup(txgroup)
+}
+
+// TestBlockEvaluator uses a TestEvalContext to perform basic checks on transactions.
+type TestBlockEvaluator struct {
+	TestEvalContext
 }
 
 // TestTransactionGroup performs basic duplicate detection and well-formedness checks
-// on a transaction group.
-func TestTransactionGroup(eval TestEvalContext, txgroup []transactions.SignedTxn) error {
+// on a transaction group. It is passed a TestEvalContext, which prevents it from adding
+// the transactions to the block evaluator, or modifying the block evaluator state in any
+// other visible way.
+func (eval TestBlockEvaluator) TestTransactionGroup(txgroup []transactions.SignedTxn) error {
 	// Nothing to do if there are no transactions.
 	if len(txgroup) == 0 {
 		return nil
@@ -952,7 +959,7 @@ func TestTransactionGroup(eval TestEvalContext, txgroup []transactions.SignedTxn
 
 	var group transactions.TxGroup
 	for gi, txn := range txgroup {
-		err := TestTransaction(eval, txn)
+		err := eval.TestTransaction(txn)
 		if err != nil {
 			return err
 		}
@@ -993,16 +1000,9 @@ func TestTransactionGroup(eval TestEvalContext, txgroup []transactions.SignedTxn
 	return nil
 }
 
-// TestTransaction performs basic duplicate detection and well-formedness checks
-// on a single transaction, but does not actually add the transaction to the block
-// evaluator, or modify the block evaluator state in any other visible way.
-func (eval *BlockEvaluator) TestTransaction(txn transactions.SignedTxn) error {
-	return TestTransaction(eval, txn)
-}
-
 // TestTransaction performs basic duplicate detection and well-formedness checks
 // on a single transaction.
-func TestTransaction(eval TestEvalContext, txn transactions.SignedTxn) error {
+func (eval TestBlockEvaluator) TestTransaction(txn transactions.SignedTxn) error {
 	// Transaction valid (not expired)?
 	err := txn.Txn.Alive(eval.TxnContext())
 	if err != nil {
@@ -1016,6 +1016,7 @@ func TestTransaction(eval TestEvalContext, txn transactions.SignedTxn) error {
 	}
 
 	// Transaction already in the ledger?
+	// BlockEvaluator.transaction will check again using cow.checkDup later, if the pool tries to add this transaction to the block.
 	txid := txn.ID()
 	err = eval.CheckDup(txn.Txn.First(), txn.Txn.Last(), txid, ledgercore.Txlease{Sender: txn.Txn.Sender, Lease: txn.Txn.Lease})
 	if err != nil {
@@ -1199,6 +1200,7 @@ func (eval *BlockEvaluator) transaction(txn transactions.SignedTxn, evalParams *
 		}
 
 		// Transaction already in the ledger?
+		// this checks against the txns added to this evaluator; testTransaction currently only checks against committed txns.
 		err = cow.checkDup(txn.Txn.First(), txn.Txn.Last(), txid, ledgercore.Txlease{Sender: txn.Txn.Sender, Lease: txn.Txn.Lease})
 		if err != nil {
 			return err