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

Support high CHAIN_ID encoded in Transaction's v value #4625

Merged
merged 4 commits into from Nov 27, 2017

Conversation

Projects
None yet
7 participants
@gumb0
Member

gumb0 commented Oct 24, 2017

EIP155 is not clear about the possible ranges of v
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md

But other clients support high values, so we should, too.

@gumb0 gumb0 added the in progress label Oct 24, 2017

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Oct 25, 2017

Member

I don't want to deal with updating the tests right now
@winsvega or @pirapira It would be great if you could help me here

Member

gumb0 commented Oct 25, 2017

I don't want to deal with updating the tests right now
@winsvega or @pirapira It would be great if you could help me here

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 25, 2017

Contributor

does it fail on current tests?

Contributor

winsvega commented Oct 25, 2017

does it fail on current tests?

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Oct 25, 2017

Member

It does but looks like only on ZeroSig transactions, looks like I broke them and I see where, let me try to fix it...

There were more failures when I parsed v as u256 and not as int, so I thought maybe more changes in the tests needed...

Member

gumb0 commented Oct 25, 2017

It does but looks like only on ZeroSig transactions, looks like I broke them and I see where, let me try to fix it...

There were more failures when I parsed v as u256 and not as int, so I thought maybe more changes in the tests needed...

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 25, 2017

Contributor

if zero sig are supported with constantinople flag enabled then those tests should not fail.

Contributor

winsvega commented Oct 25, 2017

if zero sig are supported with constantinople flag enabled then those tests should not fail.

@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast Oct 25, 2017

Collaborator

Still broken. You should first try locally.

Collaborator

chfast commented Oct 25, 2017

Still broken. You should first try locally.

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Oct 25, 2017

Member

Ok I think we have a bug in the code for creating RLP of ZeroSig Transaction
https://github.com/ethereum/cpp-ethereum/blob/70d23560a51af2240a445370cd27640db621f20e/libethcore/TransactionBase.cpp#L163-L170

For ZeroSig m_vrs->v == m_chainId (before my modifications in this PR)
So instead of writing CHAIN_ID to RLP in this case we write CHAIN_ID + CHAIN_ID * 2 + 35 I think.

If I'm right I'll create a fix here and we will need to update Constantinople Transaction tests.

Member

gumb0 commented Oct 25, 2017

Ok I think we have a bug in the code for creating RLP of ZeroSig Transaction
https://github.com/ethereum/cpp-ethereum/blob/70d23560a51af2240a445370cd27640db621f20e/libethcore/TransactionBase.cpp#L163-L170

For ZeroSig m_vrs->v == m_chainId (before my modifications in this PR)
So instead of writing CHAIN_ID to RLP in this case we write CHAIN_ID + CHAIN_ID * 2 + 35 I think.

If I'm right I'll create a fix here and we will need to update Constantinople Transaction tests.

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Oct 25, 2017

Member

(My modifications here assigned m_vrs->v = 0 for ZeroSig and this affected RLP and this affected the transaction hash wich the tests are complaining about)

Member

gumb0 commented Oct 25, 2017

(My modifications here assigned m_vrs->v = 0 for ZeroSig and this affected RLP and this affected the transaction hash wich the tests are complaining about)

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 25, 2017

Contributor

we have to check that with other client implementations. because I think there are clients who already agree on latest transaction tests

Contributor

winsvega commented Oct 25, 2017

we have to check that with other client implementations. because I think there are clients who already agree on latest transaction tests

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Oct 25, 2017

Member

Anyway parsing Transaction RLP should be symmetrical to creating RLP

Member

gumb0 commented Oct 25, 2017

Anyway parsing Transaction RLP should be symmetrical to creating RLP

@gumb0 gumb0 requested a review from pirapira Oct 25, 2017

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Oct 25, 2017

Member

(locally running testeth.exe -t TransactionTests in this branch spits out a lot of Filler hash is different! errors, no idea why. git submodule update doesn't help)

Member

gumb0 commented Oct 25, 2017

(locally running testeth.exe -t TransactionTests in this branch spits out a lot of Filler hash is different! errors, no idea why. git submodule update doesn't help)

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 25, 2017

Contributor

that should not happen actually. filler hash is different means you have different version of tests sources and compiled tests. let me see.

Contributor

winsvega commented Oct 25, 2017

that should not happen actually. filler hash is different means you have different version of tests sources and compiled tests. let me see.

need to make sure that tests are working

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 26, 2017

Contributor

ok. the tests are failing because you changed the way transaction hash is calculated for zeroSig transactions.

Vitalik said zero signature transaction EIP is not yet confirmed. thus we could either update the tests or delete those.

Contributor

winsvega commented Oct 26, 2017

ok. the tests are failing because you changed the way transaction hash is calculated for zeroSig transactions.

Vitalik said zero signature transaction EIP is not yet confirmed. thus we could either update the tests or delete those.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 26, 2017

Contributor

run your PR on this test commit
ethereum/tests#368

Contributor

winsvega commented Oct 26, 2017

run your PR on this test commit
ethereum/tests#368

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 26, 2017

Member

Let's just update the tests. It's a waste of time to revert decisions.

Member

pirapira commented Oct 26, 2017

Let's just update the tests. It's a waste of time to revert decisions.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 26, 2017

Contributor

@pirapira
I want to place zeroSigTransaction tests into a pending PR untill we clarify the EIP realisation

Contributor

winsvega commented Oct 26, 2017

@pirapira
I want to place zeroSigTransaction tests into a pending PR untill we clarify the EIP realisation

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira
Member

pirapira commented Oct 26, 2017

@winsvega OK.

gumb0 added some commits Oct 24, 2017

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Nov 24, 2017

Member

I've updated jsontests submodule to point to ethereum/tests#368, that is tests version without ZeroSig tests.
It required to remove the test case definition from TransactionTests.cpp.
Please review.

Member

gumb0 commented Nov 24, 2017

I've updated jsontests submodule to point to ethereum/tests#368, that is tests version without ZeroSig tests.
It required to remove the test case definition from TransactionTests.cpp.
Please review.

@gumb0 gumb0 added the needs review label Nov 24, 2017

@gumb0 gumb0 removed the in progress label Nov 24, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 24, 2017

Codecov Report

Merging #4625 into develop will decrease coverage by 5.76%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #4625      +/-   ##
==========================================
- Coverage    60.36%   54.6%   -5.77%     
==========================================
  Files         1084    1611     +527     
  Lines        54947   69528   +14581     
  Branches      3569    7118    +3549     
==========================================
+ Hits         33171   37966    +4795     
- Misses       20646   30283    +9637     
- Partials      1130    1279     +149
Impacted Files Coverage Δ
test/tools/jsontests/TransactionTests.cpp 66.01% <ø> (-1%) ⬇️
libethcore/TransactionBase.h 67.74% <100%> (-0.12%) ⬇️
libethcore/TransactionBase.cpp 99.14% <100%> (+0.11%) ⬆️
...cb74/Install/include/boost/asio/detail/wait_op.hpp 0% <0%> (-100%) ⬇️
Capability.h 0% <0%> (-100%) ⬇️
Transaction.h 0% <0%> (-100%) ⬇️
...t/home/classic/iterator/impl/position_iterator.ipp 0% <0%> (-100%) ⬇️
...stall/include/boost/variant/detail/initializer.hpp 0% <0%> (-100%) ⬇️
CommonIO.h 0% <0%> (-100%) ⬇️
deps/include/jsonrpccpp/common/exception.h 0% <0%> (-100%) ⬇️
... and 838 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f82f18...e0eb27d. Read the comment docs.

codecov-io commented Nov 24, 2017

Codecov Report

Merging #4625 into develop will decrease coverage by 5.76%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #4625      +/-   ##
==========================================
- Coverage    60.36%   54.6%   -5.77%     
==========================================
  Files         1084    1611     +527     
  Lines        54947   69528   +14581     
  Branches      3569    7118    +3549     
==========================================
+ Hits         33171   37966    +4795     
- Misses       20646   30283    +9637     
- Partials      1130    1279     +149
Impacted Files Coverage Δ
test/tools/jsontests/TransactionTests.cpp 66.01% <ø> (-1%) ⬇️
libethcore/TransactionBase.h 67.74% <100%> (-0.12%) ⬇️
libethcore/TransactionBase.cpp 99.14% <100%> (+0.11%) ⬆️
...cb74/Install/include/boost/asio/detail/wait_op.hpp 0% <0%> (-100%) ⬇️
Capability.h 0% <0%> (-100%) ⬇️
Transaction.h 0% <0%> (-100%) ⬇️
...t/home/classic/iterator/impl/position_iterator.ipp 0% <0%> (-100%) ⬇️
...stall/include/boost/variant/detail/initializer.hpp 0% <0%> (-100%) ⬇️
CommonIO.h 0% <0%> (-100%) ⬇️
deps/include/jsonrpccpp/common/exception.h 0% <0%> (-100%) ⬇️
... and 838 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f82f18...e0eb27d. Read the comment docs.

else
{
if (v > 36)
m_chainId = (v - 35) / 2;
m_chainId = (v - 35) / 2;

This comment has been minimized.

@pirapira

pirapira Nov 27, 2017

Member

nitpicking: a space at the end.

@pirapira

pirapira Nov 27, 2017

Member

nitpicking: a space at the end.

@pirapira

Looks good to me.

@gumb0 gumb0 merged commit c7528cd into develop Nov 27, 2017

8 of 9 checks passed

codecov/project/tests 82.79% (-0.12%) compared to 0f82f18
Details
ci/circleci: Linux-Clang5 Your tests passed on CircleCI!
Details
ci/circleci: Linux-GCC6-Debug Your tests passed on CircleCI!
Details
ci/circleci: macOS-XCode9 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project Absolute coverage decreased by -5.76% but relative coverage increased by +39.63% compared to 0f82f18
Details
codecov/project/app Absolute coverage decreased by -5.94% but relative coverage increased by +43.51% compared to 0f82f18
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gumb0 gumb0 deleted the support-high-chainid branch Nov 27, 2017

@chriseth chriseth removed the needs review label Nov 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment