Skip to content
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

SpuriousDragon HF Support #791

Merged
merged 6 commits into from
Jul 2, 2020
Merged

SpuriousDragon HF Support #791

merged 6 commits into from
Jul 2, 2020

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Jun 25, 2020

Part of #652

References

TODO

Byzantium HF Activation Switches

Byzantium Meta Issue: #209

Check that all EIPs from the respective newer HF get disabled/reverted, taking in the list from #161 here:

SpuriousDragon HF EIPs - Recap and Activation Switch Checks

Rather for preparing older HF integration, technically not needed.

  • EIP-155 (Simple replay attack protection), implementation in the Tx library Support spurious dragon and later ethereumjs-tx#132 with HF switch in place
  • EIP-160 (EXP cost increase), done by having the EXP gas cost changes integrated in the Common library
  • EIP-161 (State trie clearing), will leave for now easier to adopt on TangerineWhistle integration
  • EIP-170 (Contract code size limit), added 7ccb6c3 as a check, but this is rather for older HF support and won't affect the SpuriousDragon test output directly

Test Situation

Test run with HF activated and no changes applied:

1..1221
# tests 1221
# pass  1126
# fail  95

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   84.19%   84.44%   +0.25%     
==========================================
  Files          16       18       +2     
  Lines        1221     1241      +20     
  Branches      246      246              
==========================================
+ Hits         1028     1048      +20     
  Misses        125      125              
  Partials       68       68              
Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 80.18% <ø> (+0.20%) ⬆️
#blockchain 84.50% <ø> (ø)
#common 93.60% <ø> (ø)
#ethash 86.30% <ø> (+0.89%) ⬆️
#tx 94.16% <ø> (+0.13%) ⬆️
Impacted Files Coverage Δ
packages/block/src/index.ts 100.00% <0.00%> (ø)
packages/tx/src/index.ts 100.00% <0.00%> (ø)
packages/ethash/src/util.ts 97.22% <0.00%> (+0.92%) ⬆️
packages/block/src/header-from-rpc.ts 87.50% <0.00%> (+20.83%) ⬆️

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 cf82c4e...2e9572f. Read the comment docs.

@holgerd77
Copy link
Member Author

Ok, this is actually very much doable. Not completely there yet but state tests are passing already. 😄 The TODO list from above has still to be completed.

Was just working on re-adding pre-Byzantium EIP-658 (Tx receipts) when running out of time. This is the change commit here and I came up with the following not yet working test code for the runBlock.js API tests:

async function runWithHf(hardfork) {
  const vm = setupVM({ hardfork: hardfork })
  const suite = setup(vm)
  
  const block = new Block(util.rlp.decode(suite.data.blocks[0].rlp))
  
  await setupPreConditions(suite.vm.stateManager._trie, suite.data)
  
  let res = await suite.p.runBlock({
    block,
    generate: true,
    skipBlockValidation: true,
  })
  return res
}
  
tape('should return correct HF receipts', async (t) => {
  const res = await runWithHf('byzantium')
  t.equal(res.receipts[0].status, 1, 'should return correct post-Byzantium receipt format')
    
  res = await runWithHf('spuriousDragon')
  console.log(res)
  //t.equal(res.receipts[0].stateRoot, 1, 'should return correct pre-Byzantium receipt format')
  
  t.end()
})

Feel free everyone to pick this up - didn't want to commit incomplete here - and generally continue working on this (SpuriousDragon support). Shouldn't be too much work left.

"test:state:allForks": "npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
"test:state:selectedForks": "npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
"test:state:allForks": "npm run test:state -- --fork=SpuriousDragon && npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
"test:state:selectedForks": "npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=SpuriousDragon",
Copy link
Contributor

Choose a reason for hiding this comment

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

State tests are our bottleneck, I was thinking in splitting state tests in two jobs, so instead of a 12min job, it could be two jobs of 6:30s each. Let me know if that would speed up your work @holgerd77, so I can implement it right into this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the offer, not necessary here on this round, but definitely useful to split these up. We can do test:state:selectedLatestForks and test:state:selectedNonLatestForks to give this some (somewhat blurry) semantics for better orientation, rather that test:state:selectedForks1 and ...2 or something.

Just a first-shot idea, feel free to take on or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about an experiment re state tests, and that'd be to have the test runner accept multiple forks, prepare everything once and run each test for all the forks consecutively. It probably won't yield much of a speed-up, but might be worth trying out if the effort required isn't that high.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you re-add Istanbul and MuirGlacier to seletedForks state test since those are run by CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

My suggestion was to leave this to Everton to readd (see comment below) along splitting the state test run CI command into two (for performance reasons). Could you live with that as well?

...abstractTxReceipt,
} as PreByzantiumTxReceipt
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, @s1na @alcuadrado please have a look if you are satisfied with this implementation, I thought it is not worth to update the checkpointing mechanism here, since this is a feature (which we then doesn't fully provide for pre-Byzantium) rather than consensus critical code.

We can alternatively also just fill in an empty Buffer and avoid changing the StateManager interface (might generally be a useful addition, not sure though). 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Some working note: just tested to run the blockchain tests on older HFs with node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Byzantium.

These are currently NOT working and throwing errors like invalid block stateRoot and invalid receiptTrie. I can confirm though that this is also happening on master so this is not induced by the changes here and rather need some separate investigation. @evertonfraga We should also run - I would suggest - 1 additional fork config on the blockchain tests on the nightly test runs.

The Istanbul tests are running ('node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Istanbul'). The older tests not running doesn't necessarily mean that this originates in one or several bugs in our code but can also have its origin on the tests side and eventually the older HF blockchain tests don't get accurately generated any more respectively have been at the state of the testssnapshot.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather suggest not to do this investigation here but do this separately. Otherwise the scope here gets too big.

Copy link
Contributor

Choose a reason for hiding this comment

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

The receiptTrie field in the block header is a commitment to the tx receipts, so I wouldn't be surprised if they were incorrect. But I agree we can probably handle this separately because as you said it needs deeper changes. I'd suggest however till then to keep the StateManager interface unchanged and if the intermediate state root is incorrect anyway then just return a dummy value until we dig deeper.

@holgerd77
Copy link
Member Author

Had another check, the difficulty code in BlockHeader.canonicalDifficulty() is actually already HF-aware (down to Frontier 😄 ), so there was just EIP-658 missing which I now added, please have a look there if you are satisfied with this since this is somewhat of a stub implementation, see reasoning along the code comment.

Gave this a last rebase, this is now ready for review.

@evertonfraga For practical reasons and to see the spuriousDragon state test results here, I would suggest to leave the changed test:state:selectedForks "as is" for this PR and you can then give this another update (likely without SpuriousDragon again ?) on your task split.

@holgerd77
Copy link
Member Author

Rebased this on top of #787

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Great PR overall and thanks for taking initiative on this Holger! Just had a few minor comments.

@@ -401,9 +401,15 @@ export const handlers: { [k: string]: OpHandler } = {
runState.stack.push(new BN(keccak256(code)))
},
RETURNDATASIZE: function (runState: RunState) {
if (!runState._common.gteHardfork('byzantium')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a side-note: we won't be needing these checks anymore after getOpcodesForHF supports all the forks.

Copy link
Member

Choose a reason for hiding this comment

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

Yup these can be removed as these changes are in master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this will be another PR.

Can you approve here again so that we can merge? Sina already did but I dismissed by doing the rebase. 😋

packages/vm/lib/evm/opcodes.ts Outdated Show resolved Hide resolved
packages/vm/lib/runBlock.ts Outdated Show resolved Hide resolved
packages/vm/lib/runBlock.ts Show resolved Hide resolved
packages/vm/lib/runBlock.ts Show resolved Hide resolved
...abstractTxReceipt,
} as PreByzantiumTxReceipt
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The receiptTrie field in the block header is a commitment to the tx receipts, so I wouldn't be surprised if they were incorrect. But I agree we can probably handle this separately because as you said it needs deeper changes. I'd suggest however till then to keep the StateManager interface unchanged and if the intermediate state root is incorrect anyway then just return a dummy value until we dig deeper.

@holgerd77
Copy link
Member Author

This is now ready for re-review. //cc @s1na

packages/vm/lib/evm/evm.ts Outdated Show resolved Hide resolved
packages/vm/tests/api/runBlock.js Show resolved Hide resolved
packages/vm/tests/config.js Show resolved Hide resolved
@holgerd77
Copy link
Member Author

@ryanio thanks for the branch update, @s1na (or @evertonfraga if still awake 😛 , but don't stay up for this), can one of you give this another look?

@holgerd77
Copy link
Member Author

Update: please wait with merging this in here, I will do some other reviews this morning and then do a rebase here on top.

@holgerd77
Copy link
Member Author

Ok, rebased this. This is now open for another look.

@holgerd77
Copy link
Member Author

(also feel free to directly merge in upon review)

s1na
s1na previously approved these changes Jul 1, 2020
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Holger!

@holgerd77
Copy link
Member Author

Rebased this, this would need a re-approval now.

@holgerd77 holgerd77 merged commit dacc853 into master Jul 2, 2020
@holgerd77 holgerd77 deleted the spuriousDragon branch July 2, 2020 07:26
@jochem-brouwer jochem-brouwer mentioned this pull request Jul 31, 2020
5 tasks
@holgerd77 holgerd77 mentioned this pull request Aug 9, 2020
27 tasks
CASABECI

This comment was marked as abuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants