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

Possible msg.data or transaction data regression from v2.3.1 to v2.3.2+ #283

Closed
sohkai opened this issue Jan 20, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@sohkai
Copy link

commented Jan 20, 2019

It appears there may be a regression related to msg.data (opcode: CALLDATASIZE) or transaction data-related JSON-RPC behaviour in contracts between v2.3.1 and v2.3.2 of ganache-core.

In particular, the following tests failed when upgrading from ganache-cli@6.2.3 to ganache-cli@6.2.4-beta.0 and higher, where the only related thing that changed was the upgrade to ganache-core from 2.3.1 to 2.3.2-beta.4. The following tests are failing on non-beta versions of ganache-cli@6.2.4 and ganache-cli@6.2.5 (see passing CI when pinned to 6.2.3 and failing CI when upgraded to 6.2.5).

Test

Test code

What this test checks for:

We have a DepositableDelegateProxy that allows a sender to send ETH to a delegate proxy if less than 10k gas and no data is sent with the transaction (FWD_GAS_LIMIT is defined in a base contract, DelegateProxy).

This test tries to assert the data condition: a ETH transfer should fail if data is sent as part of the transaction, even if less than 10k gas is used. On ganache-cli@6.2.3, this test passes by causing a revert to occur. However, on ganache-cli@6.2.4 and higher, the test fails because the transaction completes.

Upon looking into the behaviour (logging both the gasleft() and msg.data values), it appears that msg.data is the culprint; on ganache-cli@6.2.3, this test sends a msg.data of length 1 whereas on ganache-cli@6.2.4, it thinks msg.data is of length 0.

Expected Behavior

msg.data should have the correct data filled in.

Current Behavior

msg.data seems to not have the correct data in recent versions of ganache-core.

Possible Solution

Not sure if it's the CALLDATASIZE opcode or a JSON-RPC endpoint. Maybe this was caused due to an undocumented API change (we're using truffle@4.1.14 so it could be related to how truffle-contract expects to send the data although all other transactions except for those sent with contract.sendTransaction() seem to work fine).

Steps to Reproduce (for bugs)

See above

Context

Would like our tests to pass with newer ganache versions :)

Your Environment

@davidmurdoch

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Thanks, @sohkai for filing this issue! It seems like it could be related to some changes made in ganache-core v2.3.2 related to the way we handle transaction data. You could try using input instead of data when creating the transaction (in your sendTransaction call); these two fields are aliases, and I'm wondering if the aliasing isn't working for some reason. Let me know if that works for your or not.

@davidmurdoch davidmurdoch self-assigned this Jan 22, 2019

sohkai referenced this issue in aragon/aragonOS Jan 23, 2019

@sohkai

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

@davidmurdoch I've tried using input in aragon/aragonOS#471 but without luck :(.

@sohkai

This comment has been minimized.

Copy link
Author

commented Feb 7, 2019

Just tested with ganache-cli@6.3.0 (using ganache-core@2.4.0) and our tests are still failing.

@davidmurdoch

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@sohkai thanks for following up. I'll let you know ASAP when a release with the fix has shipped!

@davidmurdoch davidmurdoch removed their assignment Feb 22, 2019

@eshaben eshaben self-assigned this Feb 25, 2019

@eshaben

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

Thanks again @sohkai for filing this issue! The PR that addresses this issue has been merged into develop and will be available in our next release, so I am going to go ahead and close this issue.

@eshaben eshaben closed this Mar 7, 2019

@sohkai

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

Looking forward to it @eshaben!

@davidmurdoch

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@sohkai The actual bug @eshaben fixed here goes against the JSON-RPC spec in order to match with the behavior shown in geth. Passing 0x1 in a transaction's data field is actually invalid; the value should be 0x01 (since it should be an UNFORMATTED DATA type). In order to align with geth we now 0 pad these data fields when necessary.

What was happening in ganache-core is that we were creating a native Buffer from the given hex value, "1". Buffer then checks the length of this value and determines it to be 0, creates a 0-length Buffer instance, then writes the "1" to it, which obviously doesn't work. There is a node issue and comment about Buffer.from's ambiguity here: nodejs/node#8569 (comment), which you may find interesting.

@sohkai

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

Sounds like we should also update the test to use 0x01 (I just assumed the left padding wouldn't be required, but I can see it also being strict), although node's behaviour is hilariously awful.

sohkai added a commit to aragon/aragonOS that referenced this issue Mar 20, 2019

test: update bytes and address constants (#492)
Updates the tests to use `EMPTY_BYTES` and `ZERO_ADDR` constants (should move these out to a shared lib sometime).

Also fixes a few instances where we sent an invalid number of hex bytes (usually 1-length bytes like `0x1`; see trufflesuite/ganache-core#283 (comment)).

sohkai added a commit to aragon/aragonOS that referenced this issue Apr 5, 2019

test: update bytes and address constants (#492)
Updates the tests to use `EMPTY_BYTES` and `ZERO_ADDR` constants (should move these out to a shared lib sometime).

Also fixes a few instances where we sent an invalid number of hex bytes (usually 1-length bytes like `0x1`; see trufflesuite/ganache-core#283 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.