Add mysterious Petersburg fork #1719

cburgdorf · 2019-01-18T16:30:18Z

What was wrong?

It looks like we are going forward with another fork that is like Constantinople but without EIP-1283. Since the name is still up for debate, I'm using "Petersburg" as a placeholder for the time being.

How was it fixed?

Notice that implementation wise "Petersburg" isn't a child of Constantinople but a child of Byzantium which will make it easier to remove Constantinople entirely should we ever decide to remove support for it.

Cute Animal Picture

veox · 2019-01-19T15:03:33Z

Notice that implementation wise "Petersburg" isn't a child of Constantinople but a child of Byzantium (...)

Won't this skip a large set of upstream transition tests ("XToYAt5")?..

Those will most likely have "Byzantium to Constantinople" and "Constantinople to Petersburg" transitions, and (possibly) no explicit "Byzantium to Petersburg".

EDIT:

Although, not sure: "Byzantium to Petersburg" will be the (in-effect) transition on main-net, so it would make sense to test this, more than anything else...

I suggest waiting until the transition sets are available to pull in from upstream.

cburgdorf · 2019-01-20T11:23:52Z

On

Notice that implementation wise "Petersburg" isn't a child of Constantinople but a child of Byzantium

AFAIK, this is an implementation detail of Py-EVM that should not affect testability. For added clarity, let me refer to the Ethereum protocol rules that are currently live on Ropsten and Rinkeby as FallenConstantinopleVM and the new ones, that are planned for the Feb 27th upgrade as ConstantinopleVM.

In other words, what this PR currently adds as PetersburgVM would be ConstantinopleVM and what current master has as Constantinople should be FallenConstantinopleVM.

As far as testing goes, we can test the transition from

ByzantiumVM --> FallenConstantinopleVM --> ConstantinopleVM

which is effectively the situation on Rinkeby and Ropsten. But we can also test the transition from

ByzantiumVM --> ConstantinopleVM

Whether ConstantinopleVM derives from FallenConstantinopleVM or ByzantiumVM should not affect this testability because as far as chains are involved, these are simply an orchestration of such VMs which can be different between RopstenChain and MainnetChain.

In other words, the way I understand the plan forward, we decided that we would not hard reset Ropsten and Rinkeby, which in other words means that the Ethereum protocol does know and support a ruleset known as FallenConstantinople.

However, the Ethereum mainnet, does not support FallenConstantinople. To put it another way, at no point in time, should the Ethereum mainnet ever execute a transaction under the rules defined as FallenConstantinople. That is different from Rinkeby and Ropsten who do know and support such era (as we speak).

Now, from what I understand, as far as geth is concerned they would technically define mainnet as activating FallenConstantinople and Constantinople at the same block number. But that sounds like an implementation detail of geth. As long as we agree that mainnet should never execute a transaction under the rules of FallenConstantinople, this is effectively the same as Py-EVM not having FallenConstantinople configured for MainnetChain at all.

Now coming back to why I think ConstantinopleVM should not derive from FallenConstantinopleVM: Because, would we ever decide to throw away Rinkeby and Ropsten, then we can simple throwaway FallenConstantinopleVM as well without affecting any other code.

cburgdorf · 2019-01-31T13:40:53Z

The geth team went forward with the name Petersburg and included it in their latest release so we might want to merge this as well?

cburgdorf · 2019-01-31T14:00:04Z

Ok, after upgrading the fixtures to the latest commit geth uses this still needs some work. Seems it even fails the Byzantium tests with the new fixtures (cc @veox sounds like a riddle you would enjoy).

Also, ethereum/tests calls Petersburg ConstantinopleFix and tests for this fork still need to be configured.

veox · 2019-02-01T13:38:50Z

The failing BlockchainTests/GeneralStateTests/stRevertTest/RevertPrecompiledTouch_storage* tests (a series of four) are "new" (25 days old); this is not a regression in the strict sense of the word.

Taking a look. Will open an issue if there's too much text to fit here.

veox · 2019-02-01T13:55:10Z

The RevertPrecompiledTouch_storage tests describe that precompiles at addresses from 0x01 to 0x08 have storage set.

The test performs calls via test-specific contracts to the precompiles (1 to 8). The test-specific contracts use CALL, DELEGATECALL, CALLCODE and STATICCALL. CALL and STATICCALL variants fail (have a result mismatch compared to aleth).

It then writes to the contract's storage to force OOG. The test expects pre-execution storage values to remain after OOG, including for the precompiles. py-evm clears the accounts as empty instead (part of Spurious Dragon EIP-161):

  DEBUG2  02-01 16:14:23            logging.py  COMPUTATION SUCCESS: from: 0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b | to: 0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b | value: 0 | depth: 0 | static: n | gas-used: 77588 | gas-remaining: 1220
  DEBUG2  02-01 16:14:23            logging.py  TRANSACTION REFUND: 1220 -> 0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b
  DEBUG2  02-01 16:14:23            logging.py  TRANSACTION FEE: 98780 -> 0x68795c4aa09d6f4ed3e5deddf8c2ad3049a601da
  DEBUG2  02-01 16:14:23            logging.py  CLEARING EMPTY ACCOUNT: 0x0000000000000000000000000000000000000001
<... rest of them ...>

(Full DEBUG2 EVM trace in gist.)

According to EIP-161 addendum:

On 2016-11-24, a consensus bug occurred due to two implementations having different behavior in the case of state reverts.[3] The specification was amended to clarify that empty account deletions are reverted when the state is reverted.

cburgdorf · 2019-02-18T09:57:31Z

@veox thanks for digging up all this info! Do I understand that right that our Spurious Dragon implementation is wrong then?

I'm currently knee deep in a different Trinity issue so I'm hesitant to chase down a different rabbit whole. If you have a good idea about what needs to be done and are willing to make the change that would be superb!

Otherwise, I'd propose I cut out the test upgrade for now so that we can merge this and cut a new release before the fork happens. @pipermerriam @carver @lithp

veox · 2019-02-18T14:01:32Z

Do I understand that right that our Spurious Dragon implementation is wrong then?

~~It's a little more complicated. REVERT came in Byzantium, after Spurious Dragon, - is seems like the issue is in the interaction of the two.~~

Yes, seems so.

If you have a good idea about what needs to be done and are willing to make the change that would be superb!

I'm also a bit pre-occupied right now, but will try to look into remedies later today.

It would seem that finalize_computation() of SpuriousDragonTransactionExecutor should be extended to check if the computation ended in an error, and apply EIP-161 state clearing only if not.

If this isn't supposed to be done elsewhere, of course.

Fiddling around shows it's quite related to ethereum/EIPs#716.

~~WIP~~ fix in branch pr-1719-petersborg+fix - see cburgdorf#4.

carver · 2019-02-19T21:05:57Z

Otherwise, I'd propose I cut out the test upgrade for now so that we can merge this and cut a new release before the fork happens.

Fine by me, if Spurious dragon is already broken, we don't need to delay ConstantiNOPEl implementation.

veox · 2019-02-20T14:07:49Z

Current failures are from tests already in INCORRECT_UPSTREAM_TESTS (of PR #1224 fame); except they're not marked xfail for ConstantinopleFix.

I'll submit a quick fix.

veox · 2019-02-20T15:05:53Z

tests/json-fixtures/test_blockchain.py

 # All four variants have been specifically added to test a collision type
 # like the above; therefore, they fail in the same manner.
 # * https:/ethereum/py-evm/pull/1579#issuecomment-446591118
 ('GeneralStateTests/stSStoreTest/InitCollision_d0g0v0.json', 'InitCollision_d0g0v0_Constantinople'), # noqa: E501
 ('GeneralStateTests/stSStoreTest/InitCollision_d1g0v0.json', 'InitCollision_d1g0v0_Constantinople'), # noqa: E501
 ('GeneralStateTests/stSStoreTest/InitCollision_d2g0v0.json', 'InitCollision_d2g0v0_Constantinople'), # noqa: E501
 ('GeneralStateTests/stSStoreTest/InitCollision_d3g0v0.json', 'InitCollision_d3g0v0_Constantinople'), # noqa: E501
+ ('GeneralStateTests/stSStoreTest/InitCollision_d0g0v0.json', 'InitCollision_d0g0v0_ConstantinopleFix'), # noqa: E501
+ ('GeneralStateTests/stSStoreTest/InitCollision_d1g0v0.json', 'InitCollision_d1g0v0_ConstantinopleFix'), # noqa: E501
+ ('GeneralStateTests/stSStoreTest/InitCollision_d3g0v0.json', 'InitCollision_d3g0v0_ConstantinopleFix'), # noqa: E501


NB: InitCollision_d2g0v0 not present here. It seems in that particular case, the failure relates indirectly to net gas metering of EIP-1283. Can find no other explanation why xfail for Constantinople but pass for Petersburg.

For some reason, it no longer surfaces specifically for the d2 variant.

cburgdorf · 2019-02-20T15:06:41Z

if Spurious dragon is already broken, we don't need to delay ConstantiNOPEl implementation.

Better! @veox fixed Spurious Dragon with this commit

This is now complete with CI tests etc and can be given a full review.

carver

LGTM!

eth/vm/forks/petersburg/computation.py

carver · 2019-02-20T21:23:39Z

eth/vm/forks/petersburg/headers.py

+)
+
+
+compute_petersburg_difficulty = compute_difficulty(5000000)


Maybe compute_difficulty(bomb_delay=5000000) as a way to document what this magic number is.

For some reason, this fails the tests with:

TypeError: compute_difficulty() got multiple values for argument 'bomb_delay' (CI job)

Leaving it as is for now.

tests/core/tester/test_generate_vm_configuration.py

carver · 2019-02-20T21:38:55Z

tox.ini

@@ -28,6 +28,7 @@ commands=
 native-blockchain-spurious_dragon: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork EIP158}
 native-blockchain-byzantium: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork Byzantium}
 native-blockchain-constantinople: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork Constantinople}
+ native-blockchain-petersburg: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork ConstantinopleFix}


Hm, seems unfortunate to call it petersburg in one place and ConstantinopleFix in another. Obviously this isn't the first fork to have that problem, but I can't think of a good reason we'd want to continue the divergence.

It's unfortunate that we didn't manage to reach consensus among all dev teams how to name this fork. Geth and Py-EVM both call it Petersburg but ethereum/tests calls it ConstantinopleFix which is why we have ConstantinopleFix here. I believe this should be fixed upstream because ConstantinopleFix imho is visually too close to Constantinople and makes Constantinople less grepable.

cc @holgerd77 and @winsvega what do you two think about changing the name.

@cburgdorf you might surface this in the ethereum/tests repo as an issue

I'd even prefer calling the old one ConstantiNOPEl and the new one Constantinople 😄 -- caps make it visually apparent, and it's greppable

@carver I remember I proposed in the AllCoreDevs that we should consider renaming the failed fork to free up the name Constantinople for the new fork but there were concerns that we can't do that since it is already live on various nets including xDai. Technically, we can do whatever we want in this code base but I guess deciding on a name in ethereum/tests opens up a broader discussion again ;)

If the issue is the CLI arg being named as-upstream instead of as-codebase, could there be a "convenience" mapping of names for the --fork arg?..

@veox we could do that but it just moves the ConstantinopleFix to another place. With geth, EthereumJS, Py-EVM all using Petersburg as a name, maybe an upstream change would be appropriate.

…tation had error. Previously, "nested" computations would be recursed into even if they had an error; whereas "origin" computation would be recursed into on success only. This commit effectively makes so that "nested" comp-ns will be recursed into only if they are successful (that is, same logic as for "origin" comp-n). The rest of the changes are comments, heredoc brush-ups, and similar fluff from the debugging session.

…leFix.

winsvega · 2019-02-21T19:21:47Z

Didn't we decide on a core dev call to name Constantinople and ConstantinopleFix ?

Renaming the fork require client code changes, test file changes (most of it)
In other words its pretty expensive. Changing the code could lead to potential bugs also. I suggest we put to YP fork naming convention

cburgdorf · 2019-02-21T19:29:10Z

@winsvega Geth is more ambiguous than I thought. It uses both terms ConstantinopleFix and Petersburg https:/holiman/go-ethereum/blob/bb75b656929e5688d2e6756461a51b7aecb66c9d/cmd/puppeth/genesis.go#L352-L354

The announcement calls it Petersburg though.

holgerd77 · 2019-02-22T13:48:26Z

We are also using Petersburg since I read out of the conversations that this would be the settled name. For the moment we do some not to beautiful name mapping from constantinopleFix -> petersburg, so I would be supportive on renaming.

pipermerriam · 2019-02-22T15:24:21Z

@winsvega I'm strongly in favor of renaming to Petersburg. I don't have a full grasp of the cost but with advent of ethereum/tests publishing releases (and release notes) I think we have a framework in place to let people know this change happened and I suspect every client has some form of their own internal mapping from the ethereum/tests names to their own internal names.

Would explicit approval from Geth and Parity teams be sufficient for you to be comfortable making this change? cc @karalabe and @5chdn

karalabe · 2019-02-22T15:31:02Z

We just use ConstantinopleFix as a log label. I'd gladly drop it and use Petersburg evertwhere. Our chain config API's use Petersburg in the genesis spec, do that's our current "official" stance.

…

On Fri, Feb 22, 2019, 17:24 Piper Merriam ***@***.***> wrote: @winsvega <https:/winsvega> I'm strongly in favor of renaming to Petersburg. I don't have a full grasp of the *cost* but with advent of ethereum/tests publishing releases (and release notes) I think we have a framework in place to let people know this change happened and I suspect every client has some form of their own internal mapping from the ethereum/tests names to their own internal names. Would explicit approval from Geth and Parity teams be sufficient for you to be comfortable making this change? cc @karalabe <https:/karalabe> and @5chdn <https:/5chdn> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1719 (comment)>, or mute the thread <https:/notifications/unsubscribe-auth/AAH6GbezmGrFS8F_dtRo7HgCLAxIlAB-ks5vQAupgaJpZM4aIO6g> .

pipermerriam · 2019-02-22T15:34:38Z

cc @folsen

holiman · 2019-02-22T18:10:25Z

Yeah I used ConstantinopleFix internally, since it's quite obvious for us what it means. I considered it totally useless for non-developers though, since it's highly confusing unless you know the full backstory. Hence put 'Petersburg' in the genesis spec.

Didn't we decide on a core dev call to name Constantinople and ConstantinopleFix ?

No, I'm 99% certain we never did any such thing (I would have strongly objected). I think we said "let's decide over allcoredev gitter channel".

I don't really care what we call it, but I think ConstantinopleFix is a horrible 'official' name, and in the name fo consistency it might be better to use 'Petersburg" everywhere.

cburgdorf force-pushed the christoph/feat/petersburg-fork branch from 4ca2145 to 221b8bc Compare January 18, 2019 16:35

cburgdorf force-pushed the christoph/feat/petersburg-fork branch from 221b8bc to cec7dd8 Compare January 31, 2019 13:35

veox mentioned this pull request Feb 19, 2019

eth/vm/state: recurse collect_touched_accounts() always, unless computation had error. cburgdorf/py-evm#4

Closed

cburgdorf force-pushed the christoph/feat/petersburg-fork branch 5 times, most recently from bf80b88 to 673e4df Compare February 20, 2019 13:07

veox mentioned this pull request Feb 20, 2019

tests/blockchain: duplicate INCORRECT_UPSTREAM_TESTS for ConstantinopleFix cburgdorf/py-evm#5

Closed

cburgdorf force-pushed the christoph/feat/petersburg-fork branch from e56e38a to d41e805 Compare February 20, 2019 14:52

veox reviewed Feb 20, 2019

View reviewed changes

cburgdorf assigned carver Feb 20, 2019

carver approved these changes Feb 20, 2019

View reviewed changes

cburgdorf force-pushed the christoph/feat/petersburg-fork branch from d41e805 to 08726b6 Compare February 21, 2019 07:51

cburgdorf and others added 5 commits February 21, 2019 09:08

Add Petersburg fork aka Constantinople that works

c977794

Make test more DRY

b74d07f

Activate tests for Petersburg fork

7c8dd1a

tests/blockchain: duplicate INCORRECT_UPSTREAM_TESTS for Constantinop…

92b6585

…leFix.

cburgdorf force-pushed the christoph/feat/petersburg-fork branch from 08726b6 to 92b6585 Compare February 21, 2019 08:14

cburgdorf merged commit 42360e4 into ethereum:master Feb 21, 2019

veox mentioned this pull request Oct 4, 2019

Tracking: some upstream test fixtures fail when updating to v7.0.0-beta.1 #1857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mysterious Petersburg fork #1719

Add mysterious Petersburg fork #1719

cburgdorf commented Jan 18, 2019 •

edited

Loading

veox commented Jan 19, 2019 •

edited

Loading

cburgdorf commented Jan 20, 2019

cburgdorf commented Jan 31, 2019

cburgdorf commented Jan 31, 2019

veox commented Feb 1, 2019 •

edited

Loading

veox commented Feb 1, 2019 •

edited

Loading

cburgdorf commented Feb 18, 2019

veox commented Feb 18, 2019 •

edited

Loading

carver commented Feb 19, 2019

veox commented Feb 20, 2019 •

edited

Loading

veox Feb 20, 2019 •

edited

Loading

cburgdorf commented Feb 20, 2019 •

edited

Loading

carver left a comment

carver Feb 20, 2019

cburgdorf Feb 21, 2019

carver Feb 20, 2019

cburgdorf Feb 21, 2019

pipermerriam Feb 21, 2019

carver Feb 21, 2019

cburgdorf Feb 21, 2019 •

edited

Loading

veox Feb 21, 2019

cburgdorf Feb 22, 2019

winsvega commented Feb 21, 2019 •

edited

Loading

cburgdorf commented Feb 21, 2019

holgerd77 commented Feb 22, 2019

pipermerriam commented Feb 22, 2019

karalabe commented Feb 22, 2019 via email

pipermerriam commented Feb 22, 2019

holiman commented Feb 22, 2019

		)


		compute_petersburg_difficulty = compute_difficulty(5000000)

Add mysterious Petersburg fork #1719

Add mysterious Petersburg fork #1719

Conversation

cburgdorf commented Jan 18, 2019 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

veox commented Jan 19, 2019 • edited Loading

cburgdorf commented Jan 20, 2019

cburgdorf commented Jan 31, 2019

cburgdorf commented Jan 31, 2019

veox commented Feb 1, 2019 • edited Loading

veox commented Feb 1, 2019 • edited Loading

cburgdorf commented Feb 18, 2019

veox commented Feb 18, 2019 • edited Loading

carver commented Feb 19, 2019

veox commented Feb 20, 2019 • edited Loading

veox Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

cburgdorf commented Feb 20, 2019 • edited Loading

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega commented Feb 21, 2019 • edited Loading

cburgdorf commented Feb 21, 2019

holgerd77 commented Feb 22, 2019

pipermerriam commented Feb 22, 2019

karalabe commented Feb 22, 2019 via email

pipermerriam commented Feb 22, 2019

holiman commented Feb 22, 2019

cburgdorf commented Jan 18, 2019 •

edited

Loading

veox commented Jan 19, 2019 •

edited

Loading

veox commented Feb 1, 2019 •

edited

Loading

veox commented Feb 1, 2019 •

edited

Loading

veox commented Feb 18, 2019 •

edited

Loading

veox commented Feb 20, 2019 •

edited

Loading

veox Feb 20, 2019 •

edited

Loading

cburgdorf commented Feb 20, 2019 •

edited

Loading

cburgdorf Feb 21, 2019 •

edited

Loading

winsvega commented Feb 21, 2019 •

edited

Loading