Coming to agreement with geth on RIPEMD touch revert

[gumb0]

figured cpp does a very complicated thing. It's so complicated that I don't want to describe it.

-- Yoichi Hirai

This is the old issue, originated that time when both Geth and Parity had incorrect revert behavior, and then it was decided to introduce a hack into consensus rules instead of doing huge chain reorg.

The buggy behavior that is now a rule is basically: when precompiled contract with address 0x03 (RIPEMD160) is touched, then exception happens and state changes are reverted, this address should be still considered touched in the end, so that "remove empty touched accounts" rule (EIP-158) deletes it from the state at the end of transaction.

There are several related tests created after this incident:

• GeneralStateTests/stSpecialTest/tx_xcf416c53 - emulates the transaction where this special behavior took place on the main net. Here the gas given to CALL to RIPEMD is not enough, so the CALL itself fails, but then transaction proceeds further till success. RIPEMD stays touched and then is deleted. All clients pass this test (otherwise they wouldn't be able to do full sync)
• GeneralStateTests/stRevertTest/RevertPrecompiledTouch and GeneralStateTests/stRevertTest/RevertPrecompiledTouch_storage - here CALL to RIPEMD succeeds, but later execution runs out of gas. Changes to the state are reverted, but RIPEMD stays touched and then is deleted. In these tests we have different results than geth.

Dimitriy now has regenerated these latter tests via geth, and now aleth doesn't pass them.

In geth all of these cases look to be handled by just one hack https://github.com/ethereum/go-ethereum/blob/7527215a68eaac8a880be83f3f1ded0ed82f6650/core/state/state_object.go#L145-L149

Our current hack covers only the first tx_xcf416c53 case, because for us failing CALL to precompile is a special case, that doesn't invoke any rollback

aleth/libethereum/Executive.cpp

Lines 313 to 318 in 007d89a

	// Empty precompiled contracts need to be deleted even in case of OOG
	// because the bug in both Geth and Parity led to deleting RIPEMD precompiled in this case
	// see https://github.com/ethereum/go-ethereum/pull/3341/files#diff-2433aa143ee4772026454b8abd76b9dd
	// We mark the account as touched here, so that is can be removed among other touched empty accounts (after tx finalization)
	if (m_envInfo.number() >= m_sealEngine.chainParams().EIP158ForkBlock)
	m_s.addBalance(_p.codeAddress, 0);

Now for us to cover the case of OOG in caller's frame, it looks like we unfortunately have to add another similar hack in revert logic.

From the point of view of our code it feels like an unnecessary hack, not related to original problem, but I guess from geth's perspective to replicate our behavior they would need to do some "unnecessary" hacks in their code. Oh well.

More references:

• Original geth's fix https://github.com/ethereum/go-ethereum/pull/3341/files#diff-2433aa143ee4772026454b8abd76b9dd
• More detailed description Clarification about when touchedness is reverted during state clearance EIPs#716
• Recent mention in AllCoreDevs chat https://gitter.im/ethereum/AllCoreDevs?at=5ce3bd9863dea422b4bc9b91
• Yellow Paper mentions this anomaly in Appendix K https://ethereum.github.io/yellowpaper/paper.pdf

cc @holiman @winsvega

[gumb0]

Looks now like the revert tests generated with old testeth are failing, and the tests generated with geth are passing.
Fixed now

[codecov-io]

Codecov Report

Merging #5652 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5652      +/-   ##
==========================================
- Coverage   62.83%   62.81%   -0.02%     
==========================================
  Files         349      348       -1     
  Lines       29722    29709      -13     
  Branches     3346     3347       +1     
==========================================
- Hits        18675    18662      -13     
  Misses       9837     9837              
  Partials     1210     1210

[gumb0]

Note that I don't see any difference in test results, if I change our original hack (the one covering failed CALL) to handle only RIPEMD and not other precompiles.

This suggests that tests might not cover the cases of CALL to other empty precompiles with not enough gas and leaving them touched/untouched.
(so similar to tx_xcf416c53, but with other precompiles that are empty and not enough gas passed in CALL params)
Not sure if we should care about this coverage.

@holiman @winsvega

[chfast]

Uff, this check is actually placed in some hot part of the code...

[gumb0]

Maybe we could try to introduce some new kind of touch, that is un-revertable and set only for precompiles...

[gumb0]

@chfast Ok, here's the alternative solution #5663
No hacks in rollback() at the expense of additional flag in Account class.

I think I like it better, at least it allows to have one central piece of weirdness only in Executive.

[gumb0]

Actually now I have a third idea, to keep the list of unrevertably touched accounts in State, instead of putting the flag in each Account.
Third PR is coming...

[gumb0]

#5664

[winsvega]

tx_xcf416c53

added more tests around touch on precompiled addresses and OOG / NO OOG within the same call, not within the same transaction

ethereum/tests#610

have aleth argue with geth on Byzantium no OOG touch to 0x0000....008 empty account

[gumb0]

have aleth argue with geth on Byzantium no OOG touch to 0x0000....008 empty account

have a new test case for this
ethereum/tests#610

This branch and untouch-ripemd-alternative2 branch pass it, did you try with aleth's master?

[winsvega]

did you try with aleth's master

no

This branch and untouch-ripemd-alternative2 branch pass it,

good. merging the tests then

[gumb0]

Closing in favor of #5664