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

fix writing to memory #497

Merged
merged 4 commits into from
Oct 12, 2018
Merged

fix writing to memory #497

merged 4 commits into from
Oct 12, 2018

Conversation

ayrat555
Copy link
Member

fixes #491

Changes:

  • We weren't considering out_size parameter when we were writing to the memory after message call. If it is smaller than output's size than we should truncate output
  • Truncating logic in Memory module was wrong

@ghost ghost assigned ayrat555 Oct 12, 2018
@ghost ghost added the status: in progress label Oct 12, 2018
@ayrat555 ayrat555 added the evm label Oct 12, 2018
@@ -199,7 +199,7 @@ defmodule EVM.MessageCall do
%{machine_state | last_return_data: output}

true ->
machine_state = Memory.write(machine_state, out_offset, output)
machine_state = Memory.write(machine_state, out_offset, output, out_size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test that we could add for this? It seems like this was an important piece that was overlooked, and in terms of making sure we don't re-introduce this bug in the future, it would be nice to have a test that covers this. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@germsvel done

machine_state = %MachineState{}
updated_machine_state = Memory.write(machine_state, 256, <<>>, 0)
assert machine_state == updated_machine_state
describe "write/4" do
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I like these tests!

|> :binary.encode_unsigned()
else
original_data
cond do
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more clear what's going on here too 👍

Copy link
Contributor

@germsvel germsvel left a comment

Choose a reason for hiding this comment

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

LGTM. I think you may need to rebase or maybe I missed an AccountInterface -> AccountRepo change.

"stRevertTest/RevertOpcodeMultipleSubCalls",
"stSpecialTest/failed_tx_xcf416c53"
],
"TangerineWhistle" => ["stRevertTest/RevertOpcodeMultipleSubCalls"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! Opcode multiple subcalls 🔥


account_interface =
build(:mock_account_interface)
|> MockAccountInterface.add_account(recipient.address, %{balance: 10, code: recipient.code})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the AcountInterface stuff still in? Did I fail to remove it?

byte_value =
value
|> rem(8 * EVM.word_size())
|> :binary.encode_unsigned()
Copy link
Contributor

Choose a reason for hiding this comment

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

This was added since the last time I reviewed this. Just curious, did this fix more things?

Copy link
Member Author

Choose a reason for hiding this comment

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

@germsvel this logic was moved from Memory.write. It's needed for getting one byte from value

@@ -129,13 +129,13 @@ Ethereum common tests are created for all clients to test against. We plan to pr
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1026/1026 = 100% passing
- [x] Homestead
- [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 2231/2231 = 100% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 2059/2061 = 99.9% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 2061/2061 = 100.0% passing
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@ayrat555 ayrat555 force-pushed the ab-fix-memory-write branch from 9fcfa14 to cc89ba6 Compare October 12, 2018 13:16
@ayrat555 ayrat555 merged commit caace21 into master Oct 12, 2018
@ghost ghost removed the status: in progress label Oct 12, 2018
@ayrat555 ayrat555 deleted the ab-fix-memory-write branch October 12, 2018 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix consensus bug at block #295311 on ropsten
3 participants