Skip to content

Conversation

@hrkrshnn
Copy link
Contributor

@hrkrshnn hrkrshnn commented Jul 12, 2021

Fixes #11390

Need to fix evmone CI issues.

@axic
Copy link
Contributor

axic commented Jul 12, 2021

I really fix that just like with Berlin, this shouldn't be a single large PR, but rather:

  1. evmc/evmone
  2. london support + basefee

I would merge 1) now, but would not merge 2) before the hardfork goes live (~3 weeks from now).

@hrkrshnn
Copy link
Contributor Author

@axic I'll split this up, once I can get the CI working.

About the second part, why not add london to evm-version, while keep berlin as the default, and add basefee? We can change the default to london once the hardfork is live.

@chriseth chriseth requested a review from axic July 12, 2021 16:02
to ``abi.encodeWithSelector(bytes4(keccak256(bytes(signature)), ...)```
- ``bytes.concat(...) returns (bytes memory)``: :ref:`Concatenates variable number of
arguments to one byte array<bytes-concat>`
- ``block.basefee`` (``uint``): current block's basefee
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should explain basefee (at least mention it) in the documentation and link it here?

@axic
Copy link
Contributor

axic commented Jul 12, 2021

About the second part, why not add london to evm-version, while keep berlin as the default, and add basefee? We can change the default to london once the hardfork is live.

It worked before when we could mark those as "experimental", but you have removed that option 😅

@hrkrshnn hrkrshnn force-pushed the basefee branch 2 times, most recently from cc4041d to 2457f10 Compare July 12, 2021 17:16
@hrkrshnn hrkrshnn changed the base branch from develop to evmone-upgrade July 27, 2021 12:49
Base automatically changed from evmone-upgrade to develop July 27, 2021 14:34
@hrkrshnn hrkrshnn changed the base branch from develop to update-evmone-part2 July 27, 2021 15:10
Base automatically changed from update-evmone-part2 to develop July 27, 2021 16:18
@axic
Copy link
Contributor

axic commented Jul 27, 2021

This looks good to me, though:

  1. Would still wait the extra week until London goes live and only merge this after.
  2. Clean up the commits a tiny bit (e.g. the commit "Updated evmone version to 0.8.0 and evmc version to 9.0.0" has nothing to do with what it says)

// data_acaf3289d7b601cbd114fb36c4d29c85bbfd5e133f14cb355c3fd8d99367964f 48656c6c6f2c20576f726c6421
// Bytecode: 6006600055fe48656c6c6f2c20576f726c6421
// Opcodes: PUSH1 0x6 PUSH1 0x0 SSTORE INVALID 0x48 PUSH6 0x6C6C6F2C2057 PUSH16 0x726C6421000000000000000000000000
// Opcodes: PUSH1 0x6 PUSH1 0x0 SSTORE INVALID BASEFEE PUSH6 0x6C6C6F2C2057 PUSH16 0x726C6421000000000000000000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

:)

@chriseth
Copy link
Contributor

Why wait? As long as we do not change the default evm version it should be fine.

@axic
Copy link
Contributor

axic commented Jul 27, 2021

Why wait? As long as we do not change the default evm version it should be fine.

Because if the fork falls through there's just extra stuff to be removed. It is really one week away so makes little difference in terms of "stalled PRs" waiting that out :)

@argotorg argotorg deleted a comment from github-actions bot Jul 27, 2021
@argotorg argotorg deleted a comment from github-actions bot Jul 27, 2021
@argotorg argotorg deleted a comment from github-actions bot Jul 27, 2021
@argotorg argotorg deleted a comment from github-actions bot Jul 27, 2021
@argotorg argotorg deleted a comment from github-actions bot Jul 27, 2021
Copy link

@leonardoalt leonardoalt left a comment

Choose a reason for hiding this comment

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

Minor comment, looks good

Changelog.md Outdated
* Commandline Interface: option ``--pretty-json`` works also with ``--standard--json``.
* SMTChecker: Unproved targets are hidden by default, and the SMTChecker only states how many unproved targets there are. They can be listed using the command line option ``--model-checker-show-unproved`` or the JSON option ``settings.modelChecker.showUnproved``.
* SMTChecker: new setting to enable/disable encoding of division and modulo with slack variables. The command line option is ``--model-checker-div-mod-slacks`` and the JSON option is ``settings.modelChecker.divModWithSlacks``.
* Type System: Introduced global ``block.basefee`` for retrieving the base fee of the current block.

Choose a reason for hiding this comment

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

Maybe also add the added Yul function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a language feature instead of a compiler feature? Both of them?

leonardoalt
leonardoalt previously approved these changes Aug 9, 2021
to ``abi.encodeWithSelector(bytes4(keccak256(bytes(signature)), ...)```
- ``bytes.concat(...) returns (bytes memory)``: :ref:`Concatenates variable number of
arguments to one byte array<bytes-concat>`
- ``block.basefee`` (``uint``): current block's base fee (EIP-3198 and EIP-1559)
Copy link
Contributor

Choose a reason for hiding this comment

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

link to the EIP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added links and fixed the changelog.

Changelog.md Outdated
### 0.8.7 (unreleased)

Language Features:
* Type System: Introduced global ``block.basefee`` for retrieving the base fee of the current block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Type System: Introduced global ``block.basefee`` for retrieving the base fee of the current block.
* Introduced global ``block.basefee`` for retrieving the base fee of the current block.

(also the code generator is affected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

bool hasSelfBalance() const { return *this >= istanbul(); }
bool hasBaseFee() const { return *this >= london(); }

bool hasOpcode(evmasm::Instruction _opcode) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done in 0c8b612

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or second commit in #11763

| chainid() | | I | ID of the executing chain (EIP 1344) |
| chainid() | | I | ID of the executing chain (EIP-1344) |
+-------------------------+-----+---+-----------------------------------------------------------------+
| basefee() | | L | current block's base fee (EIP-3198 and EIP-1559) |
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc on the top also explains what these letters stand for, L is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

ekpyron
ekpyron previously approved these changes Aug 11, 2021
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Any reason for not merging #11763 into this first :-)? Strictly speaking, that'd make more sense. Not that it matters too much, if we merge all of them anyways, though.

Anyways, looks fine despite minor comments.

@hrkrshnn hrkrshnn enabled auto-merge August 11, 2021 07:55
@hrkrshnn hrkrshnn merged commit 0fc3e2d into develop Aug 11, 2021
@hrkrshnn hrkrshnn deleted the basefee branch August 11, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement support for London

7 participants