Adding "codeFile" parameter into aleth-vm options #5848

josephnicholas · 2019-11-23T17:29:33Z

Changes:

Add --codeFile parameter and accept input file.
Modified the std implementation if --codeFile has -.

Open for reviews and modifications on this PR.

Part of #4613

codecov-io · 2019-11-23T18:10:34Z

Codecov Report

Merging #5848 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5848      +/-   ##
==========================================
- Coverage   64.07%   64.05%   -0.02%     
==========================================
  Files         362      362              
  Lines       30900    30912      +12     
  Branches     3432     3433       +1     
==========================================
+ Hits        19800    19802       +2     
- Misses       9874     9881       +7     
- Partials     1226     1229       +3

aleth-vm/main.cpp

Part of ethereum#4613

aleth-vm/main.cpp

gumb0 · 2019-11-25T15:56:51Z

aleth-vm/main.cpp

    {
        if (!code.empty())
-            cerr << "--code argument overwritten by input file " << inputFile << '\n';
+            cerr << "--code argument overwritten by input file " << codeFile << '\n';


Please better in case both --code and --codefile are present, output an error and exit.

did the change.

So you need now to remove this if (!code.empty()) here, it will never be true

gumb0 · 2019-11-25T15:58:33Z

aleth-vm/main.cpp

-        if (inputFile == "-")
-            for (int i = cin.get(); i != -1; i = cin.get())
-                code.push_back(static_cast<byte>(i));
+        string codeStr = "";


I think it would be simpler to use a single string variable instead of code and codeStr.

So try changing code to string, use it for getline and change line 255 to code = contentsString(codeFile).

doing this amendment will make the try/catch block unusable so I removed it.

What do you mean? We need that block, it decodes the hex string.

We should keep it, but it should decode the string that we have read either from file or from command line.

@gumb0 will do the changes, what you said totally make sense, I was really getting of myself, my apologies.

- change placeholder to <path>. - use of single string for code. - try catch removed since ::erase might not throw. - add error message for using --code and --codefile at the same time.

josephnicholas · 2019-11-26T03:04:31Z

@gumb0 after amendments from review. I got the output below. It seems to be different from what you mentioned on gitter.

$ ./aleth-vm --log-verbosity 4 --network Istanbul trace --flat --mnemonics --codefile code.hex 
{"depth":1,"gas":"9223372036854775807","gasCost":"2","memSize":0,"memory":[],"op":54,"opName":"CALLDATASIZE","pc":0,"stack":[],"storage":{}}
{"depth":1,"gas":"9223372036854775805","gasCost":"0","memSize":0,"memory":[],"op":0,"opName":"STOP","pc":1,"stack":["0x00"]}
DEBUG 11-26 10:57:32 aleth-vm overlaydb Closing state DB

gumb0 · 2019-11-26T14:35:52Z

after amendments from review. I got the output below. It seems to be different from what you mentioned on gitter.

That's because now without hex decoding, the file is treated as binary, it tries to execute the opcode 54 which is the ascii code of the symbol 6

josephnicholas · 2019-11-27T11:23:37Z

after amendments from review. I got the output below. It seems to be different from what you mentioned on gitter.

That's because now without hex decoding, the file is treated as binary, it tries to execute the opcode 54 which is the ascii code of the symbol 6

@gumb0 output code from code form latest changes.

➜  aleth-vm git:(aleth-vm-improvement) ✗ ./aleth-vm --log-verbosity 4 --network Istanbul trace --flat --mnemonics --codefile code.hex
{"depth":1,"gas":"9223372036854775807","gasCost":"3","memSize":0,"memory":[],"op":96,"opName":"PUSH1","pc":0,"stack":[],"storage":{}}
{"depth":1,"gas":"9223372036854775804","gasCost":"3","memSize":0,"memory":[],"op":96,"opName":"PUSH1","pc":2,"stack":["0x60"]}
{"depth":1,"gas":"9223372036854775801","gasCost":"3","memSize":0,"memory":[],"op":82,"opName":"MSTORE","pc":4,"stack":["0x60","0x00"]}
{"depth":1,"gas":"9223372036854775795","gasCost":"3","memSize":32,"memory":["0000000000000000000000000000000000000000000000000000000000000060"],"op":96,"opName":"PUSH1","pc":5,"stack":[]}
{"depth":1,"gas":"9223372036854775792","gasCost":"3","memSize":32,"memory":["0000000000000000000000000000000000000000000000000000000000000060"],"op":96,"opName":"PUSH1","pc":7,"stack":["0x20"]}
{"depth":1,"gas":"9223372036854775789","gasCost":"0","memSize":32,"memory":["0000000000000000000000000000000000000000000000000000000000000060"],"op":243,"opName":"RETURN","pc":9,"stack":["0x20","0x00"]}
DEBUG 11-27 19:20:44      overlaydb Closing state DB

gumb0

Also could you please add a entry to https://github.com/ethereum/aleth/blob/master/CHANGELOG.md describing how the command line changed

gumb0 · 2019-11-27T16:20:48Z

aleth-vm/main.cpp

-            std::string strCode{reinterpret_cast<char const*>(code.data()), code.size()};
-            strCode.erase(strCode.find_last_not_of(" \t\n\r") + 1);  // Right trim.
-            code = fromHex(strCode, WhenError::Throw);
+            code.erase(code.find_last_not_of(" \t\n\r") + 1);  // Right trim.


Currently try/catch around this is useless (erase and code never throw), it was here to handle errors trown by fromHex.

It's fine that you moved fromHex into a single place for both --code and --codefile, but there needs to be hanlding of exceptions around it.

I guess you would need some new variable like bytes codeBytes and you would assign result fromHex to it inside try/catch, then pass codeBytes to setCode.

Also I find it strange that we ignored decoding errors before, so I'd say inside catch we should better output error and exit.

@gumb0 Changes pushed in the latest commit, please review if I understood your requirements perfectly. Thanks.

gumb0 · 2019-11-27T16:21:17Z

aleth-vm/main.cpp

    {
-        if (!code.empty())
-            cerr << "--code argument overwritten by input file " << inputFile << '\n';
+        if (vm.count("code"))


this would be more efficient

Suggested change

if (vm.count("code"))

if (!code.empty())

Noted on this.

@gumb0 Added on latest commit.

josephnicholas · 2019-11-27T16:31:41Z

Also could you please add a entry to https://github.com/ethereum/aleth/blob/master/CHANGELOG.md describing how the command line changed

Should this be changed on the same pull request?

gumb0 · 2019-11-27T16:43:09Z

Should this be changed on the same pull request?

Yes, exactly

- Added changelog for added parameter change. - Made some revisions based on review.

josephnicholas · 2019-11-27T18:27:08Z

@gumb0 Thanks for correcting my changes, and thanks for the review.

gumb0 · 2019-11-27T19:57:03Z

@josephnicholas thank you for the contribution.

halfalicious requested review from gumb0, chfast and halfalicious November 23, 2019 19:19

chfast reviewed Nov 23, 2019

View reviewed changes

aleth-vm/main.cpp Outdated Show resolved Hide resolved

aleth-vm/main.cpp Outdated Show resolved Hide resolved

Adding "codeFile" parameter into alet-vm options

8661e0a

Part of ethereum#4613

josephnicholas force-pushed the aleth-vm-improvement branch from 36f029e to 8661e0a Compare November 24, 2019 06:05

josephnicholas requested a review from chfast November 24, 2019 06:51

gumb0 suggested changes Nov 25, 2019

View reviewed changes

Changes from review:

f49f6f5

- change placeholder to <path>. - use of single string for code. - try catch removed since ::erase might not throw. - add error message for using --code and --codefile at the same time.

josephnicholas requested a review from gumb0 November 26, 2019 03:04

josephnicholas force-pushed the aleth-vm-improvement branch from d4bf363 to f49f6f5 Compare November 27, 2019 11:16

Revision based on review, and reverted back to original try-catch block

99528a1

gumb0 suggested changes Nov 27, 2019

View reviewed changes

Changes for this revision:

bb59c10

- Added changelog for added parameter change. - Made some revisions based on review.

josephnicholas requested a review from gumb0 November 27, 2019 17:00

gumb0 approved these changes Nov 27, 2019

View reviewed changes

Minor improvements

a4e6d35

gumb0 force-pushed the aleth-vm-improvement branch from 4ffb887 to a4e6d35 Compare November 27, 2019 18:48

gumb0 merged commit 1614343 into ethereum:master Nov 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding "codeFile" parameter into aleth-vm options #5848

Adding "codeFile" parameter into aleth-vm options #5848

josephnicholas commented Nov 23, 2019

codecov-io commented Nov 23, 2019 •

edited

Loading

gumb0 Nov 25, 2019

josephnicholas Nov 26, 2019 •

edited

Loading

gumb0 Nov 26, 2019

gumb0 Nov 25, 2019

josephnicholas Nov 26, 2019

gumb0 Nov 26, 2019

josephnicholas Nov 26, 2019

josephnicholas commented Nov 26, 2019

gumb0 commented Nov 26, 2019

josephnicholas commented Nov 27, 2019

gumb0 left a comment

gumb0 Nov 27, 2019

josephnicholas Nov 27, 2019

gumb0 Nov 27, 2019

josephnicholas Nov 27, 2019

josephnicholas Nov 27, 2019

josephnicholas commented Nov 27, 2019

gumb0 commented Nov 27, 2019 •

edited

Loading

josephnicholas commented Nov 27, 2019

gumb0 commented Nov 27, 2019

Adding "codeFile" parameter into aleth-vm options #5848

Adding "codeFile" parameter into aleth-vm options #5848

Conversation

josephnicholas commented Nov 23, 2019

Changes:

codecov-io commented Nov 23, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

josephnicholas Nov 26, 2019 • 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

Choose a reason for hiding this comment

josephnicholas commented Nov 26, 2019

gumb0 commented Nov 26, 2019

josephnicholas commented Nov 27, 2019

gumb0 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

josephnicholas commented Nov 27, 2019

gumb0 commented Nov 27, 2019 • edited Loading

josephnicholas commented Nov 27, 2019

gumb0 commented Nov 27, 2019

codecov-io commented Nov 23, 2019 •

edited

Loading

josephnicholas Nov 26, 2019 •

edited

Loading

gumb0 commented Nov 27, 2019 •

edited

Loading