-
Notifications
You must be signed in to change notification settings - Fork 524
Chore: Eliminate BlockEvaluator.Transaction #6515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6515 +/- ##
==========================================
- Coverage 47.86% 47.67% -0.19%
==========================================
Files 662 655 -7
Lines 87936 87849 -87
==========================================
- Hits 42087 41881 -206
- Misses 43085 43188 +103
- Partials 2764 2780 +16 ☔ View full report in Codecov by Sentry. |
The existence of a public BlockEvaluator.Transaction perpetuates the
myth that it's reasonable to evaluate transactions one at a time. No
"real" code used this method, only tests, which actually meant
"construct a one element Group and evaluate it. To eliminate it, and
keep the test callers from looking cluttered, TransactionGroup() was
made variadic, and WithAD() was added as a convenient way to construct
a SignedTransactionWithAD from SignedTransaction.
This means that called to TransactionGroup(x) become
TransactionGroup(x...), but note that this does not allocate or copy.
Go Spec says this about variadic functions:
```
If the final argument is assignable to a slice type []T and is followed by ..., it is passed unchanged as the value for a ...T parameter. In this case no new slice is created.
Given the slice s and call
s := []string{"James", "Jasmine"}
Greeting("goodbye:", s...)
within Greeting, who will have the same value as s with the same underlying array.
```
e54abf0 to
5d3f79b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes the BlockEvaluator.Transaction method to discourage evaluating transactions individually rather than as groups. The method is replaced by making TransactionGroup variadic and adding a WithAD() helper method to SignedTxn for convenient conversion to SignedTxnWithAD. Additionally, the internal test helper TestTransaction is renamed to testTransaction to reflect its private scope.
- Changed
TransactionGroupfrom accepting a slice to accepting variadic arguments - Added
WithAD()method toSignedTxnfor creatingSignedTxnWithADwith emptyApplyData - Replaced all
Transaction()calls withTransactionGroup()using variadic syntax
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/block-generator/generator/generator_ledger.go | Updated to use variadic TransactionGroup with spread operator |
| ledger/txnbench_test.go | Replaced Transaction call with TransactionGroup using WithAD() helper |
| ledger/simulation/testing/utils.go | Replaced Transaction call with TransactionGroup using WithAD() helper |
| ledger/simulation/simulator.go | Updated to use variadic TransactionGroup with spread operator |
| ledger/simple_test.go | Replaced Transaction calls with TransactionGroup using WithAD() and spread operator |
| ledger/ledger_perf_test.go | Replaced Transaction calls with TransactionGroup using WithAD() helper |
| ledger/fullblock_perf_test.go | Replaced Transaction call with TransactionGroup using WithAD() helper |
| ledger/evalbench_test.go | Replaced Transaction calls with TransactionGroup using WithAD() helper |
| ledger/eval_simple_test.go | Replaced multiple Transaction calls with TransactionGroup using WithAD() and spread operator |
| ledger/eval/txntracer_test.go | Updated to use variadic TransactionGroup and simplified single-transaction group construction |
| ledger/eval/prefetcher/prefetcher_alignment_test.go | Updated to use variadic TransactionGroup with spread operator |
| ledger/eval/evalindexer.go | Updated to use variadic TransactionGroup with spread operator |
| ledger/eval/eval_test.go | Replaced Transaction calls with TransactionGroup using WithAD() and updated to variadic syntax |
| ledger/eval/eval.go | Removed Transaction method, made TransactionGroup variadic, and renamed TestTransaction to testTransaction |
| data/transactions/signedtxn.go | Added WithAD() helper method to create SignedTxnWithAD from SignedTxn |
| data/pools/transactionPool_test.go | Replaced Transaction calls with TransactionGroup using WithAD() helper |
| data/pools/transactionPool.go | Updated BlockEvaluator interface to use variadic TransactionGroup and removed Transaction method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
algorandskiy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some random sampling old vs new allocs (-benchmem) - no changes.
The existence of a public BlockEvaluator.Transaction perpetuates the myth that it's reasonable to evaluate transactions one at a time. No "real" code used this method, only tests, which actually meant "construct a one element Group and evaluate it. To eliminate it, and keep the test callers from looking cluttered, TransactionGroup() was made variadic, and WithAD() was added as a convenient way to construct a SignedTransactionWithAD from SignedTransaction.
This means that called to TransactionGroup(x) become TransactionGroup(x...), but note that this does not allocate or copy. Go Spec says this about variadic functions:
TestTransactionwas also needlessly exported. It's nowtestTransaction. (It was the conflicting meaning for TestTransaction and Transaction that really bugged me - TestTransaction was in internal method used by TestTransactionGroup, but Transaction was a wrapper around TransactionGroup that constructed a singleton group and called it.)Summary
Test Plan