ledger: refactor evaluator into an internal package #2983

tsachiherman · 2021-10-01T22:59:02Z

Summary

This PR reorganize the files inside the ledger package by move the evaluator related files into its own internal package.
The files in the internal package cannot access the root ledger files, and therefore using the shared ledgercore as a place to share interfaces.

ledger/
├── apply/
├── internal/
├── ledgercore/
└── testing/

Test Plan

use existing test, and update existing tests.

tsachiherman · 2021-10-04T14:02:09Z

fyi: @algorandskiy

data/basics/compactcert.go

ledger/ledger.go

ledger/accountdb_test.go

algorandskiy · 2021-10-04T16:02:45Z

ledger/apptxn_test.go


+// newTestLedger creates a in memory Ledger that is as realistic as
+// possible.  It has Rewards and FeeSink properly configured.
+func newTestLedger(t testing.TB, balances bookkeeping.GenesisBalances) *Ledger {


looks like this also need to go into testing since these functions are generic and not app-specific

We can't do that due to circular dependencies between the "testing" and the "ledger".

looks like the only problem is `Ledger at

type testingEvaluator struct { *internal.BlockEvaluator ledger *Ledger }

and ledger. genesisProto, ledger.lookup usages. The former is implemented as GenesisProto() in #2984, the latter most likely is Lookup* function. So defining a new LedgerForTesting (well, I know) interface might make the trick.

ledger/evalIndexer.go

ledger/internal/eval.go

algorandskiy · 2021-10-04T23:46:47Z

btw, maybe move block eval into eval subpackage?

tsachiherman · 2021-10-05T00:01:00Z

btw, maybe move block eval into eval subpackage?

I think that doing so would be much easier after this PR is done.

codecov-commenter · 2021-10-05T22:54:25Z

Codecov Report

Merging #2983 (f9e8a46) into master (ff8139a) will decrease coverage by 0.20%.
The diff coverage is 34.14%.

❗ Current head f9e8a46 differs from pull request most recent head b645bac. Consider uploading reports for the commit b645bac to get more accurate results

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   43.83%   43.62%   -0.21%     
==========================================
  Files         385      390       +5     
  Lines       86754    86750       -4     
==========================================
- Hits        38026    37849     -177     
- Misses      42716    42875     +159     
- Partials     6012     6026      +14

Impacted Files	Coverage Δ
catchup/service.go	`69.35% <0.00%> (ø)`
cmd/tealdbg/localLedger.go	`62.67% <ø> (ø)`
compactcert/worker.go	`90.00% <ø> (ø)`
crypto/compactcert/builder.go	`63.63% <ø> (ø)`
crypto/compactcert/msgp_gen.go	`39.47% <ø> (-0.79%)`	⬇️
crypto/compactcert/structs.go	`100.00% <ø> (ø)`
daemon/algod/api/server/v2/dryrun.go	`66.56% <ø> (ø)`
data/basics/ccertpart.go	`0.00% <0.00%> (ø)`
ledger/internal/appcow.go	`80.71% <ø> (ø)`
ledger/internal/applications.go	`36.75% <ø> (ø)`
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff8139a...b645bac. Read the comment docs.

algorandskiy

Looks good to me but I have some opinionated remarks.

algorandskiy · 2021-10-07T00:12:02Z

ledger/evalindexer_test.go

 	"github.com/algorand/go-algorand/data/transactions"
 	"github.com/algorand/go-algorand/data/txntest"
 	"github.com/algorand/go-algorand/ledger/ledgercore"
+	ledgertesting "github.com/algorand/go-algorand/ledger/testing"


nit: ledgertesting looks way too long, maybe ltesting or something?

algorandskiy · 2021-10-07T00:19:22Z

ledger/apptxn_test.go


+// newTestLedger creates a in memory Ledger that is as realistic as
+// possible.  It has Rewards and FeeSink properly configured.
+func newTestLedger(t testing.TB, balances bookkeeping.GenesisBalances) *Ledger {


looks like the only problem is `Ledger at

type testingEvaluator struct { *internal.BlockEvaluator ledger *Ledger }

and ledger. genesisProto, ledger.lookup usages. The former is implemented as GenesisProto() in #2984, the latter most likely is Lookup* function. So defining a new LedgerForTesting (well, I know) interface might make the trick.

algorandskiy · 2021-10-07T00:22:37Z

ledger/ledger.go

+}
+
+// MakeDebugBalances creates a ledger suitable for dryrun and debugger
+func MakeDebugBalances(l ledgercore.LedgerForCowBase, round basics.Round, proto protocol.ConsensusVersion, prevTimestamp int64) apply.Balances {


thoughts/comments?

( not all the unit tests are ready yet )

## Summary This PR addresses a regression introduced in #2983. The culprit is that an interface might contain a nil pointer, which makes it insufficient to test the interface pointer itself. Fail cases: https://app.circleci.com/pipelines/github/algorand/go-algorand/2451/workflows/8807ced7-89ae-4b6b-96b0-1bc5bdf9d84c/jobs/27259/tests#failed-test-0 ## Test Plan Use existing unit testing. In particular TestCatchupOverGossip.

This PR reorganize the files inside the ledger package by move the evaluator related files into its own `internal` package. The files in the internal package cannot access the root ledger files, and therefore using the shared `ledgercore` as a place to share interfaces. ``` ledger/ ├── apply/ ├── internal/ ├── ledgercore/ └── testing/ ``` use existing test, and update existing tests.

## Summary This PR addresses a regression introduced in algorand#2983. The culprit is that an interface might contain a nil pointer, which makes it insufficient to test the interface pointer itself. Fail cases: https://app.circleci.com/pipelines/github/algorand/go-algorand/2451/workflows/8807ced7-89ae-4b6b-96b0-1bc5bdf9d84c/jobs/27259/tests#failed-test-0 ## Test Plan Use existing unit testing. In particular TestCatchupOverGossip.

## Summary This PR reorganize the files inside the ledger package by move the evaluator related files into its own `internal` package. The files in the internal package cannot access the root ledger files, and therefore using the shared `ledgercore` as a place to share interfaces. ``` ledger/ ├── apply/ ├── internal/ ├── ledgercore/ └── testing/ ``` ## Test Plan use existing test, and update existing tests.

tsachiherman added 2 commits October 1, 2021 18:52

stage ( still has testing errors )

9ae8d24

few more compilation issues.

7cda907

tsachiherman added the Team Carbon-11 label Oct 1, 2021

tsachiherman self-assigned this Oct 1, 2021

add missing file.

e96f672

tsachiherman changed the title ~~ledger: refactor evaluator~~ ledger: refactor evaluator into an internal package Oct 4, 2021

algorandskiy reviewed Oct 4, 2021

View reviewed changes

tsachiherman added 2 commits October 4, 2021 16:41

Merge branch 'master' into tsachi/ledger_refactor_step1

01ca84a

few reviewdog comments

894773d

few reviewdog fixes.

6396e90

tsachiherman added 9 commits October 5, 2021 16:21

update

7716f07

fix test

7c3fab7

Fix testing & extract benchmarks.

b8ddc7d

few more updates.

db60bfe

update

bd090ec

fix test

6d0b139

address reviewdog issues

6880503

modify the linter to dump the errors.

4943923

fix lint.

78ef4a3

update

0c34bcf

tsachiherman marked this pull request as ready for review October 6, 2021 16:02

tsachiherman requested a review from algorandskiy October 6, 2021 16:02

algorandskiy reviewed Oct 7, 2021

View reviewed changes

algorandskiy requested a review from cce October 7, 2021 00:25

tsachiherman added 2 commits October 7, 2021 16:28

Merge branch 'master' into tsachi/ledger_refactor_step1

9e8a300

update per review feedback.

7ba295a

tsachiherman added 5 commits October 8, 2021 09:37

Merge branch 'master' into tsachi/ledger_refactor_step1

f9e8a46

( not all the unit tests are ready yet )

fix tests.

925707f

enable few more unit tests.

b645bac

restore unit test

d6fa00f

Merge branch 'master' into tsachi/ledger_refactor_step1

daebe0f

tsachiherman requested a review from algorandskiy October 8, 2021 19:34

algorandskiy approved these changes Oct 8, 2021

View reviewed changes

tsachiherman merged commit 239921a into algorand:master Oct 8, 2021

tsachiherman deleted the tsachi/ledger_refactor_step1 branch October 8, 2021 21:30

tsachiherman mentioned this pull request Oct 9, 2021

ledger: fix regression in transaction pool #3029

Merged

egieseke mentioned this pull request Nov 23, 2021

go-algorand 3.2.0-beta #3231

Merged

algobarb mentioned this pull request Dec 7, 2021

go-algorand 3.2.1-stable #3279

Merged

cce mentioned this pull request Apr 28, 2023

testing: fix consensus version inconsistency in newTestLedger #5342

Merged

ledger: refactor evaluator into an internal package #2983

ledger: refactor evaluator into an internal package #2983

Uh oh!

Conversation

tsachiherman commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

tsachiherman commented Oct 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

algorandskiy Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

tsachiherman Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

algorandskiy commented Oct 4, 2021

Uh oh!

tsachiherman commented Oct 5, 2021

Uh oh!

codecov-commenter commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

algorandskiy Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tsachiherman commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 5, 2021 •

edited

Loading