eval: split TestTransactionGroup from BlockEvaluator using TestEvalContext

[cce]

Summary

eval.TestTransactionGroup (and TestTransaction) need only read-only access to a few details about the next pending block. This introduces an interface TestEvalContext that is used by the transaction pool, so that there can be parallel TX backlog workers checking if transactions are well-formed and move them from the backlogQueue to the postVerificationQueue.

Test Plan

Updated TxHandler test that was checking Remember() vs Test() and counting metrics bumped by different kinds of err returns.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 56.17%. Comparing base (3b3b5ce) to head (38edb5c).
Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
data/pools/transactionPool.go	58.33%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5818      +/-   ##
==========================================
+ Coverage   54.28%   56.17%   +1.88%     
==========================================
  Files         494      494              
  Lines       69936    69963      +27     
==========================================
+ Hits        37968    39303    +1335     
+ Misses      29246    27978    -1268     
+ Partials     2722     2682      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gmalouf]

Why 2 backlog workers vs say 4?

[jannotti]

I have no particular thought on the best number, and often wonder what the best way to manage worker numbers across many different places in the code base that might have some static choice.

I do wonder if all the static choice ought to be written as a fraction of the total cores available, rather than an actual numeric constant,

[jannotti]

Seems like a good idea, I had small comments, but willing to approve as is.

[jannotti]

I have no particular thought on the best number, and often wonder what the best way to manage worker numbers across many different places in the code base that might have some static choice.

I do wonder if all the static choice ought to be written as a fraction of the total cores available, rather than an actual numeric constant,

[jannotti]

I don't follow why the eval.TestBlockEvaluator struct exists. Doesn't it just contain a TestEvalContext and uses it to implement two more methods? Could they be methods on theTestEvalContext interface (and therefore, I suppose implemented by the normal BlockEvaluator)? Is the idea that this is a pattern that allows one implementation of those methods, despite multiple implementations of TestEvalContext? Are there multiple implementation of TestEvalContext?

If not, I do like the name TestBlockEvaluator, but it could be the current TestEvalContext interface, with the addition of the two extra methods.

[cce]

I'm trying to make it so you don't need a BlockEvaluator to run TestTransactionGroup, to make it so you can provide the needed state as lightweight as possible for parallel checks.

There are two implementations of TestEvalContext — BlockEvaluator is itself an implementation of TestEvalContext (you can see it instantiated as one in BlockEvaluator.TestTransactionGroup and eval.go in this PR) and the transactionPool.go implementation testEvalContext is another.

before the latest commit on this PR to add a TestBlockEvaluator type for it to hang off of, they both shared a global function instead of a new TestBlockEvaluator type:

func TestTransactionGroup(eval TestEvalContext, txgroup []transactions.SignedTxn) error {

but it seemed to me that maybe it should be

func (eval SomethingNew) TestTransactionGroup(txgroup []transactions.SignedTxn) error {

Even though TestBlockEvaluator doesn't need to hold onto anything except the pointers to Proto(), CheckDup(), etc that it needs to operate.