Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Coming to agreement with geth on RIPEMD touch revert - another alternative solution #5664

Merged
merged 1 commit into from
Jul 11, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jul 10, 2019

(Alternative solutions were considered in #5652 and #5663)

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:

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

// 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);

This PR change this hack to behavior similar to geth, covering both not enough gas passed to CALL and OOG after CALL.

More references:

@codecov-io
Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #5664 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5664      +/-   ##
==========================================
+ Coverage      63%   63.01%   +<.01%     
==========================================
  Files         350      350              
  Lines       29908    29912       +4     
  Branches     3350     3352       +2     
==========================================
+ Hits        18845    18849       +4     
+ Misses       9848     9846       -2     
- Partials     1215     1217       +2

@gumb0 gumb0 requested a review from chfast July 11, 2019 09:35
…ree with geth.

Now precompile stays touched even after revert in the calling frame, but only for one RIPEMD precompile.
@gumb0 gumb0 force-pushed the untouch-ripemd-alternative2 branch from e4408ab to fcda8d2 Compare July 11, 2019 09:40
@gumb0
Copy link
Member Author

gumb0 commented Jul 11, 2019

Updated tests and added changelog item.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants