TangerineWhistle HF Support #807

holgerd77 · 2020-07-06T11:48:40Z

Initial push on TangerineWhistle support, I would assume that only the changes from EIP-161 need to be separated for this to work.

Initial test run:

1..1096
# tests 1096
# pass  832
# fail  264

Feel free to pick things up and continue on this.

…est:state:selectedForks CI script

… account creation and CREATE nonce increment (EIP-161)

holgerd77 · 2020-07-06T11:49:28Z

a2a2354 brings test failures down to 105 (separation switch for nonce increment):

1..1096
# tests 1096
# pass  991
# fail  105

Addresses the following part from the specification:

a. Account creation transactions and the CREATE operation SHALL, prior to the execution of the initialisation code, increment the nonce over and above its normal starting value by one (for normal networks, this will be simply 1, however test-nets with non-zero default starting nonces will be different).

codecov · 2020-07-06T11:49:29Z

Codecov Report

Merging #807 into master will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
+ Coverage   84.43%   84.59%   +0.16%     
==========================================
  Files          19       18       -1     
  Lines        1240     1253      +13     
  Branches      247      247              
==========================================
+ Hits         1047     1060      +13     
  Misses        125      125              
  Partials       68       68

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`80.15% <ø> (+0.18%)`	⬆️
#blockchain	`84.71% <ø> (+0.20%)`	⬆️
#common	`94.14% <ø> (ø)`
#ethash	`86.30% <ø> (+0.89%)`	⬆️
#tx	`94.02% <ø> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
packages/tx/src/index.ts
packages/ethash/src/util.ts	`97.22% <0.00%> (+0.92%)`	⬆️
packages/blockchain/src/cache.ts	`81.25% <0.00%> (+14.58%)`	⬆️

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 4539018...d43da03. Read the comment docs.

jochem-brouwer · 2020-07-06T15:46:29Z

These TangerineWhistle tests - do they also test against ChainStart/Homestead (cannot seem to find this in the tests)? Because we do not have implemented some ChainStart things, so that could also be a reason why tests fail. Changes seem to be mostly gas changes, most of them which are implemented.

holgerd77 · 2020-07-06T15:55:31Z

No, they only tests against TangerineWhistle. The thing which I also had to understand on the SpuriousDragon implementation is that these backports are always more about the one-above HF - so in this case the SpuriousDragon HF - and the EIPs from there, since the main task is to create the delimination towards this HF so that the rules from SpuriousDragon are not yet active on TangerineWhistle as well (the EIP linked in the initial comment is actually a SpuriousDragon HF where we need to make sure here that these rules are not yet active on TangerineWhistle.

Makes sense? 😄

…n or later (EIP-161)

holgerd77 · 2020-07-07T08:27:48Z

2f9ed3c is bringing test failures down to 99:

1..1096
# tests 1096
# pass  997
# fail  99

Following part of the specification:

d. At the end of the transaction, any account touched by the execution of that transaction which is now empty SHALL instead become non-existent (i.e. deleted).

holgerd77 · 2020-07-07T08:48:30Z

I am struggling a lot with:

b. Whereas CALL and SUICIDE would charge 25,000 gas when the destination is non-existent, now the charge SHALL only be levied if the operation transfers more than zero value and the destination account is dead.

respectively the definition of a dead account as being non-existent or empty (no code, zero nonce, zero balance) and the way the related StateManager API is set up.

This is not very consistent and in parts badly named or at least commented, following potential problems or potentially problematic design decisions:

StateManager.getAccount() implicitly returns an empty account via getOrLoad for a non-existent account, so there is no way to distinguish between the two states (non-existent, empty)
StateManager.accountIsEmpty() is wrongly named since it actually gives an answer to accountIsEmptyOrNonExistent() ( by calling StateManager.getAccount() first hand, see 1.)
Same for the corresponding isAccountEmtpy() function in the EEI

First question: is this analysis correct?

My impression is - based on this - that if we overlay this chaos with pre-SpuriousDragon functionality, understanding of the code respectively these empty states get even worse.

Suggestion here would be that we do a separate PR on this and:

Give getAccount() back the somewhat expected behavior and return undefined when the account doesn't exist, the emptiness of the account can still checked separately with account.isEmtpy()
Change the semantics of the StateManager and EEI isEmpty() functions to plainly answer that question
Add two additional accountExists() functions on StateManager and EEI

Does this make sense? Other suggestions on this?

//cc @s1na @jochem-brouwer ,all others

s1na · 2020-07-07T11:01:08Z

Hm yeah you're right that the API is not ideal for distinguishing between those states.

My first impression is that changing getAccount to return undefined would require quite a few changes here and there in the VM. So maybe the easier option for now is to add a new method like getExistentAccount (for lack of a better name) which can be used for the specific logic you want to implement.

isAccountEmtpy can also be modified to take an Account as a param instead of the address.

…costs, new accountExists StateManager function

holgerd77 · 2020-07-08T11:41:48Z

@s1na ok, have added a new StateManager.accountExists() function

lgtm-com · 2020-07-08T11:45:47Z

This pull request introduces 1 alert when merging 6bb93d8 into 195be3b - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

holgerd77 · 2020-07-08T11:54:22Z

Hi @jochem-brouwer, do you think you might find some time to work a bit as well during the next 2 weeks or so? I likely won't find so many free hours to continue here. Task is mainly to have a look a the old SpuriousDragon working branch #147 and make sure that the changes done there won't get activated here.

jochem-brouwer · 2020-07-08T14:47:41Z

@holgerd77 Sure, I will pick this up! 😄

lgtm-com · 2020-07-09T18:45:51Z

This pull request introduces 2 alerts when merging f09a36b into 195be3b - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

jochem-brouwer · 2020-07-09T18:57:03Z

1..1096
# tests 1096
# pass  1082
# fail  14

Some notes: had to dig up an older version of the Ethereum Yellow Paper to actually make sense of this problem. In <= TangerineWhistle all accounts start as null (they are not in the MPT). If you call an account you also create it, you actually put an EMPTY (code hash is the empty hash, nonce is 0, balance is 0). What is really weird about this EMPTY account definition is that I cannot find out what happens if you have a contract with non-empty storage but with the empty code hash, 0 nonce, 0 balance. SpuriousDragon actually cleans up all these "touched" accounts if it turns out they are empty, i.e. in a chain where you start off with SpuriousDragon it is impossible to have EMPTY accounts (i.e. new Account()) in the trie. For clarity, DEAD accounts are either non-existent (so they are not in the Trie) or they are EMPTY.

I'll have to think about it a bit more but I think we should create functions which have these names and thus follow the specification from the Yellow Paper.

To cleanup these empty accounts from the Trie, Sweeper.sol was deployed on mainnet, but this ironically lead to a consensus bug between geth/parity.

Failing tests

ok 1 the state roots should match
# file: CrashingTransaction test: CrashingTransaction
not ok 2 the state roots should match
--
# file: CREATE_ContractSuicideDuringInit_WithValueToItself test: CREATE_ContractSuicideDuringInit_WithValueToItself
not ok 65 the state roots should match
--
# file: CREATE_EmptyContractAndCallIt_0wei test: CREATE_EmptyContractAndCallIt_0wei
not ok 72 the state roots should match
--
# file: CREATE_EmptyContractAndCallIt_1wei test: CREATE_EmptyContractAndCallIt_1wei
not ok 73 the state roots should match
--
not ok 76 the state roots should match
--
not ok 77 the state roots should match
--
# file: NonZeroValue_CALL_ToEmpty test: NonZeroValue_CALL_ToEmpty
not ok 312 the state roots should match
--
# file: NonZeroValue_CALL_ToOneStorageKey test: NonZeroValue_CALL_ToOneStorageKey
not ok 314 the state roots should match
--
# file: RevertPrefoundEmptyCall test: RevertPrefoundEmptyCall
not ok 736 the state roots should match
--
not ok 798 the state roots should match
--
# file: doubleSelfdestructTest test: doubleSelfdestructTest
not ok 853 the state roots should match
--
# file: doubleSelfdestructTest2 test: doubleSelfdestructTest2
not ok 854 the state roots should match
--
# file: ZeroValue_CALL_ToEmpty test: ZeroValue_CALL_ToEmpty
not ok 1042 the state roots should match
--
# file: ZeroValue_CALL_ToOneStorageKey test: ZeroValue_CALL_ToOneStorageKey
not ok 1044 the state roots should match

jochem-brouwer · 2020-07-10T14:37:38Z

# file: doubleSelfdestructTest test: doubleSelfdestructTest
not ok 853 the state roots should match
--
# file: doubleSelfdestructTest2 test: doubleSelfdestructTest2
not ok 854 the state roots should match

lgtm-com · 2020-07-10T14:42:10Z

This pull request introduces 2 alerts when merging e2f5371 into 195be3b - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

jochem-brouwer · 2020-07-13T17:50:25Z

Final test for TangerineWhistle now passes. This was very confusing - it was a test which had to do with SELFDESTRUCT, the problem was not in SELFDESTRUCT but actually the fact that the miner gets 0 gas fees; in Tangerine Whistle this miner stays in the Trie while in SpuriousDragon it gets removed.

However, now's the task to fix the Spurious Dragon tests:

1..1221
# tests 1221
# pass  1125
# fail  96

lgtm-com · 2020-07-13T17:52:37Z

This pull request introduces 2 alerts when merging 46d9cd6 into 4539018 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

jochem-brouwer · 2020-07-13T18:11:20Z

Can't request a review for some reason from you @holgerd77, but tests pass and this seems ready (tests pass locally)

lgtm-com · 2020-07-13T18:14:03Z

This pull request introduces 2 alerts when merging a0bd87b into 4539018 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-07-13T19:26:44Z

This pull request introduces 2 alerts when merging d43da03 into 4539018 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

ryanio · 2020-07-14T01:20:05Z

packages/vm/lib/state/stateManager.ts

@@ -111,7 +115,7 @@ export default class DefaultStateManager implements StateManager {
    const codeHash = keccak256(value)

    if (codeHash.equals(KECCAK256_NULL)) {
-      return
+      //return


a note that this is commented out

@jochem-brouwer can you clarify why this should be commented out?

It should not be commented out, this is a remainder from toying around in order to resolve those failing tests. It will not change anything though; the only call is from evm.ts which does not call putContractCode if the code to put is the empty string.

ryanio · 2020-07-14T01:21:33Z

packages/vm/lib/state/cache.ts

@@ -54,24 +54,30 @@ export default class Cache {
  /**
   * Looks up address in underlying trie.
   * @param address - Address of account
+   * @param create - Create emtpy account if non-existent


Suggested change

* @param create - Create emtpy account if non-existent

* @param create - Create empty account if non-existent

ryanio · 2020-07-14T01:34:08Z

packages/vm/lib/state/stateManager.ts

@@ -71,7 +71,7 @@ export default class DefaultStateManager implements StateManager {
   * @param address - Address of the `account` to get
   */
  async getAccount(address: Buffer): Promise<Account> {
-    const account = await this._cache.getOrLoad(address)
+    const account = (await this._cache.getOrLoad(address)) as Account


this is cast with as Account because getOrLoad was updated to return with optional undefined (Promise<Account | undefined>), so it may be better to update the return signature of the function here instead of casting.

holgerd77

This looks good now, thanks Jochem, really great! 😄 Will do an exception and admin-merge here, since socio-technically 😋 this is the PR from Jochem (I opened initially).

s1na

Congrats on getting this through the finish line! I started reviewing yesterday but had to leave in the middle. I see it's merged now but I completed the review for future PRs. Just a few minor code quality comments.

s1na · 2020-07-14T12:12:28Z

packages/vm/lib/evm/opFns.ts

+    if (runState._common.gteHardfork('spuriousDragon')) {
+      // We are at or after Spurious Dragon
+      // Call new account gas: account is DEAD and we transfer nonzero value
+      if ((await runState.stateManager.accountIsEmpty(toAddressBuf)) && !value.isZero()) {


Can you add these methods to EEI and use those here instead? We want to make sure the EVM has access only to what's exposed from the EEI and nothing else.

I am not sure what you mean here. You mean the calls to the stateManager? I.e. put accountIsEmpty and accountExists in EEI?

Yeah exactly. isAccountEmtpy is already in the EEI and can be used. accountExists needs to be added.
To clarify the reasoning, we want to isolate the execution of EVM code as much as possible. EEI is supposed to be the only external interface evm code has access to.

Cool, makes sense!

s1na · 2020-07-14T12:14:48Z

packages/vm/lib/evm/opFns.ts

+    } else if (!(await runState.stateManager.accountExists(toAddressBuf))) {
+      // We are before Spurious Dragon
+      // Call new account gas: account does not exist (it is not in the state trie, not even as an "empty" account)
+      const accountDoesNotExist = !(await runState.stateManager.accountExists(toAddressBuf))


No need to call accountExists twice

s1na · 2020-07-15T11:51:55Z

packages/vm/lib/state/stateManager.ts

@@ -111,7 +115,7 @@ export default class DefaultStateManager implements StateManager {
    const codeHash = keccak256(value)

    if (codeHash.equals(KECCAK256_NULL)) {
-      return
+      //return


@jochem-brouwer can you clarify why this should be commented out?

s1na · 2020-07-15T12:02:07Z

packages/vm/lib/state/interface.ts

@@ -10,7 +10,8 @@ export interface StorageDump {
 export interface StateManager {
  copy(): StateManager
  getAccount(address: Buffer): Promise<Account>
-  putAccount(address: Buffer, account: Account): Promise<void>
+  putAccount(address: Buffer, account: Account | null): Promise<void>


I don't see the interface of DefaultStateManager.putAccount changing. Is this change still valid or maybe deleteAccount suffices here?

Yup I should revert the changes to the interface, thanks!

s1na · 2020-07-15T12:05:38Z

packages/vm/lib/state/cache.ts

   */
-  async getOrLoad(key: Buffer): Promise<Account> {
+  async getOrLoad(key: Buffer, create: boolean = true): Promise<Account | undefined> {


I don't see any invocation of getOrLoad with create=false. Are these changes deprecated? in that case can you revert them? (also _lookupAccount)

Yup you are right, I will remove this.

jochem-brouwer · 2020-07-15T14:04:09Z

I see the PR is merged already but there are some good code quality comments above (thanks @s1na and @ryanio), I'll create a new PR to resolve these comments.

holgerd77 added 2 commits July 6, 2020 13:30

vm -> tangerineWhistle: added HF and alias to VM and tests, adopted t…

f15f9d2

…est:state:selectedForks CI script

vm -> tangerineWhistle: added spuriousDragon HF separation switch for…

a2a2354

… account creation and CREATE nonce increment (EIP-161)

holgerd77 added the PR state: WIP label Jul 6, 2020

vm -> tangerineWhistle: only delete touched accounts on spuriousDrago…

2f9ed3c

…n or later (EIP-161)

vm -> tangerineWhistle: added separation on EIP-161 SELFDESTRUCT gas …

6bb93d8

…costs, new accountExists StateManager function

jochem-brouwer self-assigned this Jul 9, 2020

[VM] TangerineWhistle: fix some SELFDESTRUCT tests

f09a36b

jochem-brouwer marked this pull request as draft July 10, 2020 10:17

[VM] fix call new account gas for pre-Spurious Dragon

e2f5371

jochem-brouwer mentioned this pull request Jul 13, 2020

EIP-150 tests with wrong stateRoot? ethereum/tests#714

Closed

[VM] fix case where miner account gets 0 gas fees

46d9cd6

jochem-brouwer added 2 commits July 13, 2020 20:08

[VM] fix SpuriousDragon and lint

04eaff3

[VM] add tangerineWhistle to state all forks test

a0bd87b

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Jul 13, 2020

jochem-brouwer marked this pull request as ready for review July 13, 2020 18:10

jochem-brouwer requested a review from s1na July 13, 2020 18:10

Merge branch 'master' into add-tangerine-whistle-support

d43da03

ryanio reviewed Jul 14, 2020

View reviewed changes

holgerd77 commented Jul 15, 2020

View reviewed changes

holgerd77 merged commit b276092 into master Jul 15, 2020

holgerd77 deleted the add-tangerine-whistle-support branch July 15, 2020 08:19

s1na reviewed Jul 15, 2020

View reviewed changes

jochem-brouwer mentioned this pull request Jul 15, 2020

[VM] cleanup some changes made in the TangerineWhistle PR #814

Merged

jochem-brouwer mentioned this pull request Jul 31, 2020

Backporting old HF rules #652

Closed

5 tasks

holgerd77 mentioned this pull request Aug 9, 2020

VM v5 Release Planning #681

Closed

27 tasks

holgerd77 added the package: vm label Sep 4, 2020

	* @param create - Create emtpy account if non-existent
	* @param create - Create empty account if non-existent

TangerineWhistle HF Support #807

TangerineWhistle HF Support #807

Conversation

holgerd77 commented Jul 6, 2020

holgerd77 commented Jul 6, 2020 • edited Loading

codecov bot commented Jul 6, 2020 • edited Loading

Codecov Report

jochem-brouwer commented Jul 6, 2020

holgerd77 commented Jul 6, 2020

holgerd77 commented Jul 7, 2020

holgerd77 commented Jul 7, 2020

s1na commented Jul 7, 2020

holgerd77 commented Jul 8, 2020

lgtm-com bot commented Jul 8, 2020

holgerd77 commented Jul 8, 2020

jochem-brouwer commented Jul 8, 2020

lgtm-com bot commented Jul 9, 2020

jochem-brouwer commented Jul 9, 2020

jochem-brouwer commented Jul 10, 2020

lgtm-com bot commented Jul 10, 2020

jochem-brouwer commented Jul 13, 2020

lgtm-com bot commented Jul 13, 2020

jochem-brouwer commented Jul 13, 2020

lgtm-com bot commented Jul 13, 2020

lgtm-com bot commented Jul 13, 2020

ryanio Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

s1na left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Jul 15, 2020

holgerd77 commented Jul 6, 2020 •

edited

Loading

codecov bot commented Jul 6, 2020 •

edited

Loading

ryanio Jul 14, 2020 •

edited

Loading