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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

🎉

- [x] EIP150
- [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 1275/1275 = 100% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1109/1113 = 99.6% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1113/1113 = 100.0% passing
- [x] EIP158
- [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 1232/1233 = 99.9% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1176/1181 = 99.6% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1180/1181 = 99.9% passing
- [x] Byzantium
- [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 4915/4959 = 99.1% passing
- [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 4758/4784 = 99.5% passing
Expand Down
17 changes: 5 additions & 12 deletions apps/blockchain/test/blockchain/state_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,11 @@ defmodule Blockchain.StateTest do
"stReturnDataTest/returndatacopy_following_call",
"stReturnDataTest/returndatacopy_following_revert",
"stReturnDataTest/returndatacopy_following_revert_in_create",
"stRevertTest/PythonRevertTestTue201814-1430",
"stRevertTest/RevertInCallCode",
"stRevertTest/RevertInCreateInInit",
"stRevertTest/RevertInDelegateCall",
"stRevertTest/RevertOpcodeInCreateReturns",
"stRevertTest/RevertOpcodeMultipleSubCalls",
"stSpecialTest/failed_tx_xcf416c53",
"stStaticCall/static_Call1024PreCalls2"
"stSpecialTest/failed_tx_xcf416c53"
],
"Constantinople" => [
"stCreate2/CREATE2_ContractSuicideDuringInit_ThenStoreThenReturn",
Expand Down Expand Up @@ -58,16 +55,12 @@ defmodule Blockchain.StateTest do
"stCreate2/returndatacopy_0_0_following_successful_create",
"stCreate2/returndatacopy_afterFailing_create",
"stCreate2/returndatacopy_following_revert_in_create",
"stCreate2/returndatasize_following_successful_create",
"stRevertTest/RevertOpcodeMultipleSubCalls"
"stCreate2/returndatasize_following_successful_create"
],
"Frontier" => [],
"Homestead" => ["stRevertTest/RevertOpcodeMultipleSubCalls"],
"SpuriousDragon" => [
"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 🔥

"Homestead" => [],
"SpuriousDragon" => ["stSpecialTest/failed_tx_xcf416c53"],
"TangerineWhistle" => []
}

@fifteen_minutes 1000 * 60 * 15
Expand Down
18 changes: 11 additions & 7 deletions apps/evm/lib/evm/memory.ex
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,17 @@ defmodule EVM.Memory do

def write(machine_state = %MachineState{}, offset_bytes, original_data, size) do
data =
if size do
original_data
|> :binary.decode_unsigned()
|> rem(size * EVM.word_size())
|> :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 👍

is_nil(size) ->
original_data

size > byte_size(original_data) ->
original_data

true ->
<<data::binary-size(size), _tail::bitstring>> = original_data

data
end

memory_size = byte_size(machine_state.memory)
Expand Down
2 changes: 1 addition & 1 deletion apps/evm/lib/evm/message_call.ex
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,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 | last_return_data: output}
end
Expand Down
7 changes: 6 additions & 1 deletion apps/evm/lib/evm/operation/stack_memory_storage_and_flow.ex
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ defmodule EVM.Operation.StackMemoryStorageAndFlow do
"""
@spec mstore8(Operation.stack_args(), Operation.vm_map()) :: Operation.op_result()
def mstore8([offset, value], %{machine_state: machine_state}) do
machine_state = Memory.write(machine_state, offset, value, EVM.byte_size())
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


machine_state = Memory.write(machine_state, offset, byte_value)

%{machine_state: machine_state}
end
Expand Down
30 changes: 26 additions & 4 deletions apps/evm/test/evm/memory_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,31 @@ defmodule EVM.MemoryTest do
use ExUnit.Case, async: true
doctest EVM.Memory

test "returns the machine_state unchanged if data size is zero" do
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!

test "returns the machine_state unchanged if data size is zero" do
machine_state = %MachineState{}
updated_machine_state = Memory.write(machine_state, 256, <<>>, 0)
assert machine_state == updated_machine_state
end

test "truncates input if the size parameter is smaller than input's size" do
machine_state = %MachineState{}
input = Enum.reduce(1..60, <<>>, fn i, acc -> <<i>> <> acc end)

updated_machine_state = Memory.write(machine_state, 0, input, 2)

assert updated_machine_state.memory == <<60, 59>>
assert updated_machine_state.active_words == 1
end

test "does not truncate input if the size parameter is bigger than input's size" do
machine_state = %MachineState{}
input = Enum.reduce(1..60, <<>>, fn i, acc -> <<i>> <> acc end)

updated_machine_state = Memory.write(machine_state, 0, input, 61)

assert updated_machine_state.memory == input
assert updated_machine_state.active_words == 2
end
end
end
38 changes: 36 additions & 2 deletions apps/evm/test/evm/message_call_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ defmodule EVM.MessageCallTest do
current_machine_state: pre_machine_state,
recipient: recipient.address,
code_owner: recipient.address,
execution_value: 100
execution_value: 100,
output_params: {0, 256}
)

%{machine_state: machine_state, exec_env: _exec_env, sub_state: _sub_state} =
Expand All @@ -47,7 +48,7 @@ defmodule EVM.MessageCallTest do
# Code was to add 1 + 3 = 4 and store in memory.
assert machine_state.memory == <<0x4::256>>
assert machine_state.gas == message_call.execution_value - 24
assert machine_state.active_words == pre_machine_state.active_words + 1
assert machine_state.active_words == 8
assert machine_state.stack == pre_machine_state.stack ++ [1]
end

Expand Down Expand Up @@ -146,4 +147,37 @@ defmodule EVM.MessageCallTest do

assert active_words == 0
end

test "truncates message call's output if output's byte size is bigger than output_params" do
code = build(:machine_code, operations: [:push1, 1, :push1, 3, :add, :push1, 0x00, :mstore])

recipient = %{address: <<0x9::160>>, code: code}

account_repo =
build(:mock_account_repo)
|> MockAccountRepo.add_account(recipient.address, %{balance: 10, code: recipient.code})

pre_exec_env =
build(:exec_env,
account_repo: account_repo
)

pre_machine_state = build(:machine_state)

message_call =
build(:message_call,
current_exec_env: pre_exec_env,
current_machine_state: pre_machine_state,
recipient: recipient.address,
code_owner: recipient.address,
execution_value: 100,
output_params: {0, 2}
)

%{machine_state: machine_state, exec_env: _exec_env, sub_state: _sub_state} =
MessageCall.call(message_call)

assert machine_state.memory == <<0, 0>>
assert machine_state.active_words == 1
end
end