Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

refactor: Refactor evmbin CLI #10742

Merged
merged 34 commits into from
Aug 7, 2019
Merged

refactor: Refactor evmbin CLI #10742

merged 34 commits into from
Aug 7, 2019

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Jun 12, 2019

cargo build -p evmbin --release
 ./target/release/parity-evm state-test ./evmbin/res/teststate.json --only add12 --chain eip150
 ./target/release/parity-evm state-test ./evmbin/res/create2callPrecompiles.json --only create2callPrecompiles --chain Constantinople
  • Run stats:
./target/release/parity-evm stats --to "0000000000000000000000000000000000000004" --from "0000000000000000000000000000000000000003" --code "05" --input "06" --gas "21" --gas-price "2" --chain "./evmbin/res/testchain.json" --json

Output:
{"depth":1,"gas":"0x21","gasCost":"0x0","memory":"0x","op":5,"opName":"SDIV","pc":0,"stack":[],"storage":{}}
{"error":"EVM: Stack underflow SDIV 2/0","gasUsed":"0x21","time":87}

Note: operation SDIV from code: 05 does division and expects two items on the stack, try changing to code: 01 for ADD (addition). For more info do a search for "SDIV" across the codebase and read about all the different EVM bytecode instructions.

  • Build docs: cargo doc -p evmbin --release --open --no-deps --document-private-items

@ltfschoen ltfschoen added the A0-pleasereview 🤓 Pull request needs code review. label Jun 12, 2019
@parity-cla-bot
Copy link

It looks like @ltfschoen signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

good stuff here.

@@ -102,13 +102,21 @@ pub fn run_action<T: Informant>(

/// Execute given Transaction and verify resulting state root.
pub fn run_transaction<T: Informant>(
// Chain specification name associated with the transaction
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be doc-comments? I'm unsure of how the docs are rendered for private fields of a public struct.

Copy link
Contributor Author

@ltfschoen ltfschoen Jun 14, 2019

Choose a reason for hiding this comment

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

Thanks. I'm looking into it. If I update main.rs so that info.rs is public: pub mod info; then it shows that module in the rustdocs (it currently isn't at the moment), but if I then make this a doc comment (i.e. /// Chain ...) and then build the docs with cargo doc -p evmbin --release --open --no-deps i get the following error.

error: documentation comments cannot be applied to method arguments
   --> evmbin/./src/info.rs:105:2
    |
105 |     /// Chain specification name associated with the transaction
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doc comments are not allowed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dvdplm I tried to make the input parameters into this run_transaction into a struct called InputData, since we can use doc comments on structs. The approach that I tried has been pushed in this commit 8a6e644 (a separate branch to try it out), however I'm not sure how to overcome the ^^ undeclared lifetime error. In the InputData struct i'm not sure how to declare the lifetime parameter in addition to the generic type T (i.e. change pub struct InputData<T> { to pub struct InputData<'a, T> { or something?), and the existing code mut informant: T, is giving the following error and i'm not sure how to overcome that either:

122 |     mut informant: T,
    |     ^^^ expected identifier, found keyword
help: you can escape reserved keywords to use them as identifiers
    |
122 |     r#mut informant: T,

Copy link
Collaborator

Choose a reason for hiding this comment

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

See 8a6e644#r33959733 – but I admit I'm not sure of the context here and why we need lifetimes on these values (prolly me being ignorant).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tips! I somehow managed to get it to compile and I've created a separate PR #10769. However there's quite a bit of code duplication and I haven't figured out how to approach resolving yet.

evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Show resolved Hide resolved
evmbin/src/main.rs Show resolved Hide resolved
ltfschoen added a commit that referenced this pull request Jun 14, 2019
evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
dvdplm added a commit that referenced this pull request Jul 3, 2019
…lly typed serde serialization using Rust structs (#10657)

* fix: Replace multirust with rustup wince multirust is deprecated

* docs: Update evmbin Rust docs and code comments

* WIP: Add Response struct. Initial step using serde to serialize instead of hardcoding with JSON

* fix: Update Response struct types to be string after formatting

* fix: Fix move out of borrowed content error by cloning informant

* refactor: Change from camelcase to snake case to fix linting errors

* restore: Restore some code since now covered in separate PR #10658

* restore: Restore original Rustdocs of evmbin

* WIP

* add Clone type

* add newlines to end of json files

* remove uml file that was unintentionally commited

* rename chain spec to state test JSON fle

* remove log. fix indentation

* revert: Restore indentation now handled by separate PR #10740

* remove state test json files as moved to PR #10742

* revert changes in info.rs since covered in PR #10742

* revert changes to main.rs since covered in PR #10742

* revert newlines back to master

* revert newlines back to master2

* refactor: Rename Response to TraceData

* fix: Remove Clone and replace with lifetimes. Update tests since not ordered

* refactor: Change all json! to typed serde

* docs: Update rustdocs. Remove fixme

* fix: Add missing semicolons from printf

* fix: Change style from unwrap to expect in evmbin/src/display/json.rs

Co-Authored-By: Andronik Ordian <write@reusable.software>

* fix: Change style from unwrap to expect in evmbin/src/display/std_json.rs

Co-Authored-By: Andronik Ordian <write@reusable.software>

* revert updating module comments as will be done in separate PR #10742 instead

* review-fix: Remove useless reference

* Remove unncessary use of format macro

* Update evmbin/src/display/json.rs

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/json.rs with serialization in set_gas success

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/json.rs with serialization in set_gas failure

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization in finish for state root

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization in before_test

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization for state root

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization for finish success

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Update evmbin/src/display/std_json.rs with serialization for finish failure

Co-Authored-By: Andronik Ordian <write@reusable.software>

* refactor: Rename structs and variables. Remove space. Simplify MessageInitial struct

* refactor: Captialize expect message

* revert to previous struct name TraceDataStateRoot

* refactor: Simplify variable for consistency

* Update accounts/ethstore/src/json/crypto.rs

Co-Authored-By: David <dvdplm@gmail.com>
… and make doc comments of its fields (#10769)

* Question

* refactor: Further changes for doc comments to be part of public struct

* refactor: Rename InputData to TxInput for clarity in docs

* docs: Change String to fixed length str

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* refactor: Update evmbin/src/info.rs moving mut into fn declaration

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* refactor: Update evmbin/src/info.rs moving mut into fn declaration part 2

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* review-fix: Add missing docs to TxInput transaction and trie_spec

* docs: Improve grammar

* review-fix: Destructure tx_input

* WIP

* review-fix: Rename variables of InputTx

* rename `spec_from_json` to `fork_spec_from_json`
* rename `name` to `state_test_name`
* rename `spec` to `fork_spec_name`
* rename `spec_checked` to `fork_spec`

* review-fix: Rename idx to tx_index
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 19, 2019
@ordian ordian added this to the 2.7 milestone Jul 19, 2019
@ordian
Copy link
Collaborator

ordian commented Jul 19, 2019

@ltfschoen a merge with master would fix cargo audit failure (and resolve the merge conflict).

@debris debris added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 19, 2019
debris
debris previously requested changes Jul 19, 2019
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

I merged this pr with master. It looks good so far, but the tests are not finished. @ltfschoen can you please fix the tests so we can merge it in?

evmbin/src/main.rs Show resolved Hide resolved
evmbin/src/main.rs Show resolved Hide resolved
evmbin/src/main.rs Outdated Show resolved Hide resolved
ltfschoen and others added 7 commits July 22, 2019 20:26
* master:
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated security@parity.io key (#10939)
  Change the return type of step_inner function. (#10940)
  get rid of hidden mutability of Spec (#10904)
  simplify BlockReward::reward implementation (#10906)
  Kaspersky AV whitelisting (#10919)
  additional arithmetic EVM opcode benchmarks (#10916)
  [Cargo.lock] cargo update -p crossbeam-epoch (#10921)
  Fixes incorrect comment. (#10913)
  Add file path to disk map write/read warnings (#10911)
@dvdplm dvdplm dismissed debris’s stale review August 7, 2019 14:50

concerns addressed

@dvdplm dvdplm merged commit c689495 into master Aug 7, 2019
@dvdplm dvdplm deleted the luke-ethjson-refactor branch August 7, 2019 14:51
dvdplm added a commit that referenced this pull request Aug 7, 2019
* master:
  unify loading spec && further spec cleanups (#10948)
  refactor: Refactor evmbin CLI (#10742)
  journaldb changes (#10929)
@debris
Copy link
Collaborator

debris commented Aug 8, 2019

@dvdplm not entirely, tests in evmbin/src/main.rs are still not finished

@dvdplm
Copy link
Collaborator

dvdplm commented Aug 8, 2019

@dvdplm not entirely, tests in evmbin/src/main.rs are still not finished

Ouch, sorry about that. @ordian can you take a look at that please? :/

ordian added a commit that referenced this pull request Aug 15, 2019
* master:
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
  Extract Machine from ethcore (#10949)
  removed redundant state_root function from spec, improve spec error types (#10955)
  Add support for Energy Web Foundation's new chains (#10957)
  [evmbin] add more tests to main.rs (#10956)
  Fix compiler warnings in util/io and upgrade to edition 2018 Upgrade mio to latest (#10953)
  unify loading spec && further spec cleanups (#10948)
  refactor: Refactor evmbin CLI (#10742)
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated security@parity.io key (#10939)
  Change the return type of step_inner function. (#10940)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants