StateManager interface #763

s1na · 2020-06-04T10:34:43Z

So I've added an interface for the StateManager. Had to make a few other changes:

The precompile activation logic was using stateManager._trie directly, replaced that with putAccount.
Also the blake2f precompile wasn't activated because the addresses were hardcoded. Fixed now.
_clearOriginalStorageCache was private, made it public. The issue is that StateManager is persistent state, but the originalStorageCache should only persist for the duration of one tx.

Should I move the docs to the interface or leave them in the implementation? Also please feel free to suggest better names for DefaultStateManager :D

codecov · 2020-06-04T10:36:56Z

Codecov Report

Merging #763 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
+ Coverage   92.01%   92.06%   +0.05%     
==========================================
  Files          47       46       -1     
  Lines        3018     3025       +7     
  Branches      472      471       -1     
==========================================
+ Hits         2777     2785       +8     
  Misses        145      145              
+ Partials       96       95       -1

Flag	Coverage Δ
#account	`94.11% <ø> (ø)`
#block	`88.36% <ø> (+0.27%)`	⬆️
#blockchain	`89.15% <ø> (+0.41%)`	⬆️
#common	`93.78% <ø> (ø)`
#tx	`94.02% <ø> (-0.14%)`	⬇️
#vm	`93.10% <100.00%> (+<0.01%)`	⬆️

Impacted Files	Coverage Δ
packages/vm/lib/runTx.ts	`91.02% <ø> (-0.12%)`	⬇️
packages/vm/lib/index.ts	`97.10% <100.00%> (+0.04%)`	⬆️
packages/vm/lib/state/index.ts	`100.00% <100.00%> (ø)`
packages/vm/lib/state/stateManager.ts	`91.45% <100.00%> (+0.08%)`	⬆️
packages/tx/src/index.ts
packages/blockchain/src/cache.ts	`100.00% <0.00%> (+20.00%)`	⬆️

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 28b8fa4...5ff3354. Read the comment docs.

s1na · 2020-06-04T11:00:43Z

Rebased master, that should hopefully fix the codecov issue? also should I rebuild the docs in the PR or is that done at release?

lgtm-com · 2020-06-04T12:17:59Z

This pull request introduces 1 alert when merging 255c51f into 61c3dca - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

evertonfraga · 2020-06-04T12:18:51Z

@s1na I got started late today, but it seems Codecov issue is gone.

And LGTM finally finished its report.

ryanio · 2020-06-04T17:12:52Z

packages/vm/lib/index.ts

-      const trie = this.stateManager._trie
-      const put = promisify(trie.put.bind(trie))
-      for (let i = 1; i <= 8; i++) {
-        await put(new BN(i).toArrayLike(Buffer, 'be', 20), new Account().serialize())
-      }
+      this.stateManager.checkpoint()
+      Object.keys(precompiles)
+        .map((k: string): Buffer => Buffer.from(k, 'hex'))
+        .forEach(async (k: Buffer) => await this.stateManager.putAccount(k, new Account()))
+      await this.stateManager.commit()


nice improvement

ryanio · 2020-06-04T17:13:49Z

packages/vm/lib/state/interface.ts

+  [key: string]: string
+}
+
+export interface StateManager {


ryanio

looks great!

ryanio · 2020-06-04T17:17:48Z

also should I rebuild the docs in the PR or is that done at release?

@s1na you can rebuild, I usually do it as a last commit.

it would be a nice improvement to ensure docs automatically rebuild prior to release, @evertonfraga and I have already discussed this so it's on our (long) wishlist :)

Should I move the docs to the interface or leave them in the implementation?

I have a feeling it would be better in the interface, but I guess it depends how TypeDoc organizes and outputs them. Feel free to try both ways and pick which one you think results in better docs.

Also please feel free to suggest better names for DefaultStateManager :D

Seems good to me! I'll keep thinking about it. How do you anticipate future StateManagers might be named? that might give some insight.

evertonfraga · 2020-06-04T22:41:17Z

yeah, IMO having the docs generated for each PR change ensures the docs reflect exactly what’s on master branch.

Thinking of people that land on the project to experiment, having tidy docs can only help them =)

s1na · 2020-06-05T10:55:01Z

Note to self: stateManager._touched is also cleared at the end of the tx, but by checking on commit() whether the lists of checkpoints is emtpy. Same trick can probably be used for the storage original cache.

evertonfraga · 2020-06-05T15:31:40Z

Also the blake2f precompile wasn't activated because the addresses were hardcoded. Fixed now.

Hi @s1na, before I move on with rebase & merge, I wanted to understand the aforementioned change. Pardon my ignorance (😄), but where exactly is this issue fixed, and what's the impact of the current state (eg: blake2f not operational)?

s1na · 2020-06-05T15:39:22Z

Hey @evertonfraga, no it only fixes a small edge case. Contracts that call blake2f will work. For context on what activatePrecompiles does see #575. In this case the new precompile wasn't being "activated" (confusing terminology) as you can see here https://github.com/ethereumjs/ethereumjs-vm/pull/763/files#diff-1c815e0b4a51825344018a6a6c522733L150 (note the range i = 0; i < 8; i++ which doesn't cover i = 9 which is the blake2f address).

Sorry I haven't the PR yet, my working tree for this branch is dirty because I just started working on the flat db without creating a new branch... will update now with re-built docs and dropping the clearOriginalStorageCache method.

evertonfraga · 2020-06-05T15:47:04Z

@s1na thanks for the explanation! Yes, the activated precompiles terminology have caught me. I have seen that activation behavior before, just didn't know its name or where it was represented in the code. Thanks for pointing it out.

I will now rebase and merge this PR. Sounds good?

lgtm-com · 2020-06-05T16:02:30Z

This pull request introduces 1 alert when merging 56a8731 into 6ac21f6 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

s1na · 2020-06-05T16:32:24Z

Update: I made clearOriginalStorageCache private again (so its not part of the interface), and removed it from runTx. It's now run implicitly when the highest-level checkpoint terminates. This happens when a tx has finished processing (but could happen in other cases too, hope its not confusing).

Oh and I'm not sure if this PR should be considered a breaking change due to StateManager being renamed to DefaultStateManager?

Also tried rebuilding docs but couldn't get it to work, was constantly getting an error.

If everything looks good the PR can be merged.

evertonfraga · 2020-06-05T18:18:40Z

Update: I made clearOriginalStorageCache private again (so its not part of the interface), and removed it from runTx.

Alright!

Oh and I'm not sure if this PR should be considered a breaking change due to StateManager being renamed to DefaultStateManager?

The VM will be bumped to v5.0.0 anyway, so we shouldn't worry much about it. I will, of course, make a note about this change in the release notes.

Also tried rebuilding docs but couldn't get it to work, was constantly getting an error.

I'll handle it!

lgtm-com · 2020-06-05T18:32:58Z

This pull request introduces 1 alert when merging 282fdfb into 28b8fa4 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

holgerd77 · 2020-06-05T18:34:57Z

Short note: did a branch update here, hope that was ok, so that it might be merge-ready once CI pass.

lgtm-com · 2020-06-05T18:39:15Z

This pull request introduces 1 alert when merging 5ff3354 into 28b8fa4 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

holgerd77

LGTM

evertonfraga · 2020-06-05T20:27:49Z

Thanks @holgerd77!

s1na force-pushed the statemanager-iface branch from 8cc790c to 255c51f Compare June 4, 2020 10:59

s1na requested review from ryanio and holgerd77 and removed request for ryanio June 4, 2020 11:30

ryanio reviewed Jun 4, 2020

View reviewed changes

packages/vm/lib/state/interface.ts

[key: string]: string

}

export interface StateManager {

Copy link

Contributor

ryanio Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

ryanio previously approved these changes Jun 4, 2020

View reviewed changes

ryanio added the package: vm label Jun 4, 2020

s1na added 5 commits June 5, 2020 17:50

Add interface for StateManager, rename impl to DefaultStateManager

bfb79f2

Use stateManager method to activate precompiles, not trie directly

c026506

stateManager: make clearOriginalStorageCache public

2e8ee87

Rename to clearOriginalStorageCache everywhere

050c0d4

Make clearOriginalStorageCache private, implicitly run on commit, revert

56a8731

s1na dismissed ryanio’s stale review via 56a8731 June 5, 2020 15:57

s1na force-pushed the statemanager-iface branch from 255c51f to 56a8731 Compare June 5, 2020 15:57

evertonfraga self-requested a review June 5, 2020 18:24

evertonfraga previously approved these changes Jun 5, 2020

View reviewed changes

docs: generated documentation

282fdfb

evertonfraga dismissed their stale review via 282fdfb June 5, 2020 18:27

Merge branch 'master' into statemanager-iface

5ff3354

holgerd77 approved these changes Jun 5, 2020

View reviewed changes

holgerd77 merged commit 2b8b314 into master Jun 5, 2020

holgerd77 deleted the statemanager-iface branch June 5, 2020 20:03

evertonfraga added this to the VM v5 milestone Jun 12, 2020

ryanio mentioned this pull request Jul 9, 2020

Add interface for third-party StateManager implementations #742

Closed

This was referenced Aug 9, 2020

Finalize StateManager API #503

Closed

VM v5 Release Planning #681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StateManager interface #763

StateManager interface #763

s1na commented Jun 4, 2020

codecov bot commented Jun 4, 2020 •

edited

Loading

s1na commented Jun 4, 2020

lgtm-com bot commented Jun 4, 2020

evertonfraga commented Jun 4, 2020

ryanio Jun 4, 2020

ryanio Jun 4, 2020

ryanio left a comment

ryanio commented Jun 4, 2020 •

edited

Loading

evertonfraga commented Jun 4, 2020

s1na commented Jun 5, 2020

evertonfraga commented Jun 5, 2020

s1na commented Jun 5, 2020

evertonfraga commented Jun 5, 2020

lgtm-com bot commented Jun 5, 2020

s1na commented Jun 5, 2020

evertonfraga commented Jun 5, 2020 •

edited

Loading

lgtm-com bot commented Jun 5, 2020

holgerd77 commented Jun 5, 2020

lgtm-com bot commented Jun 5, 2020

holgerd77 left a comment

evertonfraga commented Jun 5, 2020

StateManager interface #763

StateManager interface #763

Conversation

s1na commented Jun 4, 2020

codecov bot commented Jun 4, 2020 • edited Loading

Codecov Report

s1na commented Jun 4, 2020

lgtm-com bot commented Jun 4, 2020

evertonfraga commented Jun 4, 2020

ryanio Jun 4, 2020

Choose a reason for hiding this comment

ryanio Jun 4, 2020

Choose a reason for hiding this comment

ryanio left a comment

Choose a reason for hiding this comment

ryanio commented Jun 4, 2020 • edited Loading

evertonfraga commented Jun 4, 2020

s1na commented Jun 5, 2020

evertonfraga commented Jun 5, 2020

s1na commented Jun 5, 2020

evertonfraga commented Jun 5, 2020

lgtm-com bot commented Jun 5, 2020

s1na commented Jun 5, 2020

evertonfraga commented Jun 5, 2020 • edited Loading

lgtm-com bot commented Jun 5, 2020

holgerd77 commented Jun 5, 2020

lgtm-com bot commented Jun 5, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

evertonfraga commented Jun 5, 2020

codecov bot commented Jun 4, 2020 •

edited

Loading

ryanio commented Jun 4, 2020 •

edited

Loading

evertonfraga commented Jun 5, 2020 •

edited

Loading