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.

Sign up for GitHub

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

Jump to bottom

Support for EIP-1344 CHAINID opcode #375

Merged
merged 4 commits into from
Aug 21, 2019
Merged

Support for EIP-1344 CHAINID opcode #375

merged 4 commits into from
Aug 21, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jul 30, 2019 •
edited
Loading

axic
axic reviewed Jul 30, 2019
View reviewed changes
examples/example_host.cpp Outdated
@@ -129,6 +129,8 @@ class ExampleHost : public evmc::Host
(void)topics;
(void)topics_count;
}

evmc_bytes32 get_chain_id() noexcept final { return 0_bytes32; }
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go into the tx_context header.

Copy link
Member Author

Choose a reason for hiding this comment

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

My line of thought was: 1. it's probably not going to be used very often, so why copy it with every get_tx_context call; 2. it's not "transaction context" conceptually, but rather more global chain constant.
But I don't insist, can change.

@gumb0 gumb0 force-pushed the chainid branch 3 times, most recently from e5aa7a2 to 8ae8d7e Compare July 31, 2019 13:29
@gumb0
Copy link
Member Author

gumb0 commented Jul 31, 2019

Not sure what happens to bindings-rust-asan-combined build.

@gumb0 gumb0 force-pushed the chainid branch 2 times, most recently from 7051c32 to 1ab538f Compare July 31, 2019 14:02
@gumb0
Copy link
Member Author

gumb0 commented Jul 31, 2019

@axic I'm bumping the version here, is that ok?

@gumb0 gumb0 removed the in progress label Jul 31, 2019
chfast
chfast reviewed Aug 5, 2019
View reviewed changes
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

Yes, looks good. Except we need to start 7.0 branch separately.

For reference, we consider always providing tx_context: #120
Now I believe this is pretty good idea - more in the issue.

@gumb0 gumb0 mentioned this pull request Aug 7, 2019
Closed
18 tasks
@gumb0 gumb0 marked this pull request as ready for review August 15, 2019 08:11
@gumb0 gumb0 requested review from axic and chfast August 15, 2019 08:12
axic
axic reviewed Aug 15, 2019
View reviewed changes
include/evmc/evmc.h
@@ -153,6 +153,7 @@ struct evmc_tx_context
int64_t block_timestamp; /**< The block timestamp. */
int64_t block_gas_limit; /**< The block gas limit. */
evmc_uint256be block_difficulty; /**< The block difficulty. */
evmc_uint256be chain_id; /**< The blockchain's ChainID. */
Copy link
Member

Choose a reason for hiding this comment

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

This part is contentious. There is a long discussion on https://ethereum-magicians.org/t/eip-1344-add-chain-id-opcode/1131/56

However many say it should be a hash, e.g. bytes32, however the calculation done the transaction format requires it to be a number.

Geth uses 256-bit, Parity 64-bit and Pantheon 32-bit numbers for chain id.

@gumb0
Copy link
Member Author

gumb0 commented Aug 20, 2019

Rebased.

chfast
chfast requested changes Aug 20, 2019
View reviewed changes
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

Please rebase and squash the first 2 commits.

@gumb0
Copy link
Member Author

gumb0 commented Aug 20, 2019

Rebased.

chfast
chfast requested changes Aug 20, 2019
View reviewed changes
CHANGELOG.md Outdated
@@ -5,6 +5,10 @@ Documentation of all notable changes to the **EVMC** project.
The format is based on [Keep a Changelog],
and this project adheres to [Semantic Versioning].

## [7.0.0] - unreleased
- Added: [[#375](https://github.com/ethereum/evmc/pull/375)]
Copy link
Member

Choose a reason for hiding this comment

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

Please update to new format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@chfast
Copy link
Member

chfast commented Aug 20, 2019

@axic Can you check Rust CI? It is failing from some days.

@axic
Copy link
Member

axic commented Aug 20, 2019

Can you check Rust CI? It is failing from some days.

@chfast I tried to fix it (#408), but it seems the unstable distro link has broken packages right now, and msan needs llvm8.

@chfast chfast merged commit 0280907 into master Aug 21, 2019
@chfast chfast deleted the chainid branch August 21, 2019 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast approved these changes

@axic axic axic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @chfast @axic