Add DAO support #843

jochem-brouwer · 2020-08-22T05:51:16Z

This PR adds DAO support.

~~For some reason ethereum/tests does not test if the DAO actually gets completed (!?) so~~ I added a test for this in the API test suite. (Note: DAO tests are actually tested in ethereum/tests, but our test runner currently does not verify this: see #842 )

I don't like the location of this gigantic DAO account list, maybe we just want a seperate config-like file where we add these accounts.

I was a bit unsure if we should create a new Common for DAO but this seems to be like the better option to add some more flags to VM/Block/Blockchain in order to support it. (There is an explicit requirement here that we should specify which block the fork gets activated. We do not have this support in general for Common or the VM so this is a bit of unexplored territory).

codecov · 2020-08-22T05:52:10Z

Codecov Report

Merging #843 into master will increase coverage by 0.25%.
The diff coverage is 96.66%.

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`78.43% <100.00%> (+0.85%)`	⬆️
#blockchain	`81.33% <ø> (+0.30%)`	⬆️
#common	`94.14% <ø> (ø)`
#ethash	`82.22% <ø> (ø)`
#tx	`94.16% <ø> (+0.13%)`	⬆️
#vm	`92.70% <94.44%> (+0.21%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

lgtm-com · 2020-08-22T05:56:09Z

This pull request fixes 1 alert when merging 5960ee2 into ea73164 - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-08-22T06:14:16Z

This pull request fixes 1 alert when merging 91194aa into ea73164 - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-08-22T06:22:32Z

This pull request fixes 1 alert when merging 4498862 into ea73164 - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

holgerd77 · 2020-08-24T08:47:09Z

Hi @jochem-brouwer, great that you gave this such a quick start! 😄 👍

There is no need for both of these DAOActivationBlock and DAOSupport parameters, both can be directly received by the Common instance.

You can get the activation block (the HF block, or am I missing something?) by using the Common.hardforkBlock() function, you can also explicitly provide the HF there if you want the DAO fork block even if Common is set to something else.

The integration status (is this HF applied on the respective chain) is directly integrated into the Common chain JSON files, e.g. mainnet.json. If there is an entry for the DAO with a HF block number other than null the HF is regarded as being part of the chain (this applies for all other HFs as well). The activation status can also be directly requested via Common.hardforkIsActiveOnChain().

Yeah, for these accounts I would also suggest a separate config file, maybe really do an additional lib/conf/ folder here so that we have a generic place for other future config files as well?

ryanio · 2020-08-24T19:35:57Z

@jochem-brouwer looking good!

I'd agree with @holgerd77 that we can set up a file at lib/config/dao_account_list.json to require into runBlock.ts with something like const daoAccountList = require('lib/config/dao_account_list.json').accounts. Let me know if you'd like any help with that!

Maybe dao_fork_account_list.json to be more descriptive at first glance?

lgtm-com · 2020-08-25T08:06:32Z

This pull request introduces 1 alert when merging 5c15e07 into d08ba7c - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-08-25T08:22:11Z

This pull request introduces 1 alert when merging a847ba6 into d08ba7c - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

re-lint everything [VM] fix commented out errors [VM] fix test [VM] really fix test

[Common] fix DAO activation block kovan/goerli [Block] remove unused BN [Common/VM] fix tests

jochem-brouwer · 2020-08-25T09:07:20Z

Allright this seems to be fixed and I really like it now as it is much cleaner. Thanks for the suggestions @holgerd77 @ryanio 😄 ! For some reason I was not aware of this Common functionality.

Please explicitly check the edited Goerli test, I think it makes a lot of sense to set the DAO blocks to null instead of zero (this already was the case in Rinkeby but not in Kovan/Goerli). If this is not the case then per the definition of DAO it requires the DAO transition block and the 10 blocks after the block to have the extraData field of the block set to dao-hard-fork, if we set this block to 0 in Common for Goerli then this implies that the first 10 blocks on Goerli need extraData set to dao-hard-fork (which does not make a lot of sense). Same goes for custom chains.

holgerd77 · 2020-08-25T11:13:09Z

packages/common/src/chains/goerli.json

@@ -26,7 +26,7 @@
    },
    {
      "name": "dao",
-      "block": 0,
+      "block": null,
      "forkHash": "0xa3f5ab08"


So I do get this right that DAO support is not active neither on Goerli nor on Kovan, correct?

Then this change makes a lot of sense and is technically also a bug fix along the way. Wonder if we should generally remove all the null DAO entries from Common completely since these are just HFs not being present on the respective chains (and never will be).

For null I always had the semantics "HF didn't happen yet but has some settings and will happen some time in the future" in mind, so e.g. the state of the Berlin HF right now. Technically it shouldn't make any difference though.

I do get the confusion here. But just for comparision:

geth: config="{ChainID: 1 Homestead: 1150000 DAO: 1920000 DAOSupport: true (...)

geth --goerli: config="{ChainID: 5 Homestead: 0 DAO: <nil> DAOSupport: true (...)

I just checked all 4 testnets and neither of them has any contract/code at "bf4ed7b27f1d666546e30d74d50d173d20bca754" (the DAO withdraw contract on mainnet)

This is thus also reflected in all 4 testnet common files now (they all have the DAO block set to null)

holgerd77

Hi @jochem-brouwer, super cool that you picked this up so quickly 😀 , some comments to address.

holgerd77 · 2020-08-25T14:22:09Z

packages/common/tests/hardforks.ts

@@ -136,8 +136,8 @@ tape('[Common]: Hardfork logic', function (t: tape.Test) {
    st.equal(c.activeHardforks().length, 8, msg)

    c = new Common('goerli')
-    msg = 'should return 9 active HFs for goerli'
-    st.equal(c.activeHardforks().length, 9, msg)
+    msg = 'should return 8 active HFs for goerli' // `geth --goerli` gives `DAOSupport: true` but `DAO: <nil>` (i.e. do not apply DAO state transition at any block)


I think this comment is rather confusing than helpful for future generations 🙂, I would tend to remove it.

holgerd77 · 2020-08-25T14:42:38Z

packages/block/src/header.ts

+      // verify the extraData field.
+      const blockNumber = new BN(this.number)
+      const DAOActivationBlock = new BN(this._common.hardforkBlock('dao'))
+      if (blockNumber.gte(new BN(DAOActivationBlock))) {


DAOActivationBlock is double-wrapped here in a BN.

I would generally tend to completely leave DAOActivationBlock as a number and use the *n functionality from BN.js, there is no chance that a DAOActivationBlock will exceed the allowed JavaScript integer range (I was actually wondering here along why we use BN instances for block number calculation at all, e.g. in calculateOmmerReward, but I dimly remember that there might be some reason for it, something like some End-User-Tool (Truffle?) running the VM on very high block numbers in some circumstances)

(always a problem if something like this is not documented 😕 , then on the next occasion someone else is removing stuff like that again because reasoning is not very obvious)

Regarding why we are using BNs in block numbers: I think there might be some ethereum/tests case where it could use a big number? The test cases test some pretty extreme things (which is OK of course if they are valid within the protocol).

I will keep it a BN; this allows for flexibility in case someone wants to ever use a gigantic block number for the DAO transition.

holgerd77 · 2020-08-25T14:45:58Z

packages/block/src/header.ts

@@ -13,6 +13,9 @@ import { Blockchain, BlockHeaderData, BufferLike, ChainOptions, PrefixedHexStrin
 import { Buffer } from 'buffer'
 import { Block } from './block'

+const DAO_ExtraData = Buffer.from('64616f2d686172642d666f726b', 'hex')
+const DAO_ForceExtraDataRange = 10 // force extra data be DAO_ExtraData for X blocks after DAO activation block


Where is this ExtraData thing actually coming from? I don't find this in EIP-779? 🤔

(eventually you can directly add this to the comment, maybe no URL if not a very trusted/simple one, but rather an EIP number or so if there is one)

Good point, I got this from both geth (see here) and from ethereum/tests (all DAO tests (which are only 4...) check explicitly that the extra data field is set to this, see this for instance this test comment). I don't know why it is not in the EIP. Actually I should open a PR for the EIP in order to fix this.

holgerd77 · 2020-08-25T14:52:58Z

packages/vm/lib/runBlock.ts

+      await this.stateManager.putAccount(DAORefundContractAddress, DAORefundAccount)
+    }
+  }
+


Since this is such specialized code not very relevant in the very most reads of the source file I would tend to put this into a separate function _applyDAOHardfork() and extract from runBlock(), maybe after the two if checks (so "if active & block correct", maybe these can be generally combined into one &&-concatenated check to avoid the deeper indentation level).

holgerd77 · 2020-08-25T15:04:32Z

packages/vm/tests/BlockchainTestsRunner.js

+    }
+    common = Common.forCustomChain('mainnet', {
+      hardforks: editedForks
+    }, 'homestead') // we should be on the "homestead" fork


I would love if we can avoid this code block since this is such a long code part for such a super-specialized one case application. Any idea anyone?

If we don't find an optimization I would also here tend to move the functionality (create the custom common instance, so all code within the first if clause switch) to a separate function.

Maybe we could add functionality to Common where we can specify the activation blocks of each fork? Is the Common supposed to be static after you instantiate it? Then we could just add a function where we setHardforkBlock.

holgerd77 · 2020-08-25T15:07:12Z

packages/vm/tests/api/runBlock.js

+  }
+  let common = Common.forCustomChain('mainnet', {
+    hardforks: editedForks
+  }, 'homestead') // we should be on the "homestead" fork


Ah, just seeing that this code is repeated here again. Can you then put it into tests/util.js as its own function? Then I think it is also viable to leave "as is".

Ah yes this is a good solution.

holgerd77 · 2020-08-25T15:10:18Z

packages/vm/tests/api/runBlock.js

+  let DAORefundAccount = await suite.vm.stateManager.getAccount(DAORefundAddress)
+  t.true(DAORefundAccount.balance.equals(Buffer.from('7777', 'hex'))) // verify that the refund account gets the summed balance of the original refund account + two child DAO accounts
+
+  t.end()


Nice tests, really cool that you went this extra path! 😀 Small typo in both of the // verify our funded account is now has 0 balance comments (remove the is).

holgerd77 · 2020-08-25T15:10:57Z

packages/vm/tests/tester.js

+
+  let FORK_CONFIG_VM
+  if (FORK_CONFIG.toLowerCase() == "dao") {
+    FORK_CONFIG_VM = "homestead"


This is correct? If so, can you add a comment here?

On a second look: this is just for the blockchain tests, right? Can we then move this over there and leave the simple logic here untouched? I think this is better understandable then in this context.

I should add a DAO fork to hardforks support of VM, don't know why I did this actually 😅

[Block] fix extra data range

jochem-brouwer · 2020-08-26T13:14:47Z

Tests should pass. Please re-review @holgerd77 😄

holgerd77 · 2020-08-27T08:45:22Z

@evertonfraga what is actually this codecov/patch often failing, so what does this number mean? Can we adopt the threshold there so that we will not always have failing CI runs? Have already looked at the Codecov admin to change this, but couldn't find anything. Is this some config file change or something?

holgerd77

This look good 👍 , thanks @jochem-brouwer, will add one commit (see comments) and then merge.

holgerd77 · 2020-08-27T09:02:35Z

packages/block/src/header.ts

@@ -13,6 +13,9 @@ import { Blockchain, BlockHeaderData, BufferLike, ChainOptions, PrefixedHexStrin
 import { Buffer } from 'buffer'
 import { Block } from './block'

+const DAO_ExtraData = Buffer.from('64616f2d686172642d666f726b', 'hex')
+const DAO_ForceExtraDataRange = 9 // force extra data be DAO_ExtraData for X blocks after DAO activation block (see: https://blog.slock.it/hard-fork-specification-24b889e70703)


Nice, thanks for the reference here!

holgerd77 · 2020-08-27T09:07:23Z

packages/block/src/header.ts

+          }
+        }
+      }
+    }


Sorry that this didn't occur to me before, a bit analogue to other requests since all this DAO code is so special: can we extract this code block to a separate _checkDAOForkExtraData() (or similar) method since this code block is unproportionally to its importance bloating the constructor code so much right now.

(with function placement optimally also very much at the end of the file along the other _* functions)

Update: I will just add this myself along the review and then approve to streamline the process. 😄

Could you also update the Changelog? 😄

holgerd77 · 2020-08-27T09:19:12Z

packages/vm/lib/runBlock.ts

+    let account = await state.getAccount(Buffer.from(address, 'hex'))
+    DAOBalance.iadd(new BN(account.balance))
+    // clear the accounts' balance
+    account.balance = Buffer.alloc(0)


I will likely never learn these kind of things 🤨 - they just seem to not fit in my head - but is it clear here that setting the account balance to zero is done with the empty Buffer ([Buffer ]) and not the zero Buffer ([Buffer 00]))?

Will not make it a blocker though since transition test runs from ethereum/tests seem to indicate that this is correct and leads to the correct transition state roots.

This is worth checking out at some point if we would handle it correctly if we put a 1-byte zero-byte (😅 ). However to calculate the state root the actual Buffer should be empty and not Buffer.from('00','hex') (1 byte length) otherwise state root calculation gets messed up.

holgerd77 · 2020-08-27T09:22:35Z

packages/vm/package.json

@@ -16,12 +16,12 @@
    "coverage:test": "tape './tests/api/**/*.js' ./tests/tester.js --state --dist",
    "docs:build": "typedoc --options typedoc.js",
    "test:state": "ts-node ./tests/tester --state",
-    "test:state:allForks": "echo 'Homestead TangerineWhistle SpuriousDragon Byzantium Constantinople Petersburg Istanbul MuirGlacier' | xargs -n1 | xargs -I v1 npm run test:state -- --fork=v1",
+    "test:state:allForks": "echo 'Chainstart Homestead dao TangerineWhistle SpuriousDragon Byzantium Constantinople Petersburg Istanbul MuirGlacier' | xargs -n1 | xargs -I v1 npm run test:state -- --fork=v1",


The convention here for the HF names is normally to apply camel case, so at least start with a capital letter (Dao) (so but for dao it is a bit unclear to admit, could also be DAO). We shouldn't deviate on such things too much since these kind of things always have the potential to lead to some not-wanted side effects at some point.

Won't make this a review blocker though.

holgerd77 · 2020-08-27T09:23:06Z

packages/vm/tests/BlockchainTestsRunner.js

@@ -24,11 +26,11 @@ module.exports = async function runBlockchainTest(options, testData, t) {
    validate = true
  }

-  const hardfork = options.forkConfigVM
-
+  const common = (options.forkConfigTestSuite == "HomesteadToDaoAt5") ? getDAOCommon(5) : new Common('mainnet', options.forkConfigVM)


Ah, nice. 😄

holgerd77 · 2020-08-27T09:26:00Z

packages/vm/tests/util.js

+  const DAOCommon = Common.forCustomChain('mainnet', {
+    hardforks: editedForks
+  }, 'dao') 
+  return DAOCommon


No blocker either, but just for awareness if this gets necessary on future work: this can be generalized to a getCommonWithCustomHFBlock() function by also passing in the HF name. Then this can be used potentially for other use cases or HF transitions as well.

This will be addressed in #849 😄

…two DAO HF extra data block creation tests

jochem-brouwer · 2020-08-27T10:11:03Z

Thanks for these changers, @holgerd77 😄

Only thing left to do before merging is updating the Changelog + ReadMe! Could you do that?

axic · 2020-08-27T10:13:51Z

.github/workflows/vm-pr.yml

@@ -85,7 +85,7 @@ jobs:
        # This is the most fair division among 1 directory vs. everything else
        args: ['--excludeDir=stTimeConsuming', '--dir=GeneralStateTests/stTimeConsuming']
        # Run specific fork tests
-        fork: ['MuirGlacier', 'Istanbul', 'Homestead', 'Chainstart']
+        fork: ['MuirGlacier', 'Istanbul', 'Homestead', 'Chainstart', 'dao']


Does Chaintstart refer to Frontier ("the genesis") here?

holgerd77

Hi @jochem-brouwer, went along and don't want to trigger a new CI run just for a one-line text addition. Let's do these CHANGELOG updates along some dedicated PRs until we caught up and can just add and switch to our new mode.

Will merge this in here for now.

jochem-brouwer marked this pull request as draft August 22, 2020 06:01

jochem-brouwer force-pushed the dao-support branch 2 times, most recently from 00fca2d to 91194aa Compare August 22, 2020 06:10

jochem-brouwer force-pushed the dao-support branch from 91194aa to 4498862 Compare August 22, 2020 06:16

jochem-brouwer marked this pull request as ready for review August 22, 2020 06:25

jochem-brouwer added the PR state: needs review label Aug 22, 2020

jochem-brouwer force-pushed the dao-support branch 2 times, most recently from 45eaf37 to ccca2fb Compare August 25, 2020 08:53

jochem-brouwer added 3 commits August 25, 2020 10:54

[VM] add DAO support

39e90f6

[VM] add explicit DAO test and lint

ae9d39a

re-lint everything [VM] fix commented out errors [VM] fix test [VM] really fix test

[VM] cleanup DAO fork to use correctly use Common

ecb7bdf

[Common] fix DAO activation block kovan/goerli [Block] remove unused BN [Common/VM] fix tests

jochem-brouwer force-pushed the dao-support branch from ccca2fb to ecb7bdf Compare August 25, 2020 08:54

holgerd77 reviewed Aug 25, 2020

View reviewed changes

holgerd77 requested changes Aug 25, 2020

View reviewed changes

[VM/Block/Common] cleanup DAO support

c035e03

[Block] fix extra data range

jochem-brouwer force-pushed the dao-support branch from 299bdb7 to c035e03 Compare August 26, 2020 13:07

Merge branch 'master' into dao-support

52a6017

holgerd77 reviewed Aug 27, 2020

View reviewed changes

block: moved DAO extra data check logic to dedicated function, added …

e513ecd

…two DAO HF extra data block creation tests

axic reviewed Aug 27, 2020

View reviewed changes

holgerd77 approved these changes Aug 27, 2020

View reviewed changes

holgerd77 merged commit 37b7185 into master Aug 27, 2020

holgerd77 deleted the dao-support branch August 27, 2020 10:21

holgerd77 added package: block package: vm labels Sep 4, 2020

Add DAO support #843

Add DAO support #843

Conversation

jochem-brouwer commented Aug 22, 2020 • edited Loading

codecov bot commented Aug 22, 2020 • edited Loading

Codecov Report

lgtm-com bot commented Aug 22, 2020

lgtm-com bot commented Aug 22, 2020

lgtm-com bot commented Aug 22, 2020

holgerd77 commented Aug 24, 2020

ryanio commented Aug 24, 2020

lgtm-com bot commented Aug 25, 2020

lgtm-com bot commented Aug 25, 2020

jochem-brouwer commented Aug 25, 2020

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

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

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 Aug 26, 2020

holgerd77 commented Aug 27, 2020

holgerd77 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

Choose a reason for hiding this comment

jochem-brouwer commented Aug 27, 2020

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

jochem-brouwer commented Aug 22, 2020 •

edited

Loading

codecov bot commented Aug 22, 2020 •

edited

Loading