core, core/state: fixed consensus issue added touch revert #3341
Conversation
|
@obscuren, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fjl, @karalabe and @Gustav-Simonsson to be potential reviewers. |
| @@ -232,6 +232,12 @@ func (c *StateObject) AddBalance(amount *big.Int) { | |||
| // EIP158: We must check emptiness for the objects such that the account | |||
| // clearing (0,0,0 objects) can take effect. | |||
| if amount.Cmp(common.Big0) == 0 && !c.empty() { | |||
| if c.empty() { | |||
There was a problem hiding this comment.
this can never be true, since the line above it is true
obscuren
force-pushed
the
touch-delete-fix
branch
3 times, most recently
from
7b414d5
to
325a359
Compare
|
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
obscuren
force-pushed
the
touch-delete-fix
branch
from
1e36fac
to
50d33ed
Compare
|
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
obscuren
force-pushed
the
touch-delete-fix
branch
2 times, most recently
from
28dc41f
to
8e8cc02
Compare
Implemented proper touch revert journal entries and copied a Parity consensus bug in order to remain in sync with the current longest chain.
obscuren
force-pushed
the
touch-delete-fix
branch
from
8e8cc02
to
12d654a
Compare
fjl
changed the title
| var ripemd = common.HexToAddress("0000000000000000000000000000000000000003") | ||
|
|
||
| func (ch touchChange) undo(s *StateDB) { | ||
| if !ch.prev && *ch.account != ripemd { |
There was a problem hiding this comment.
Why does this depend on ripemd?
There was a problem hiding this comment.
Parity has a bad implementation of EIP 161. That caused account 0000000000000000000000000000000000000003 to be deleted in block 2675119, even though the deletion should have been reverted due to an out of gas error. Geth didn't handle revertals at all (hence today's bug), but because of this, there wasn't a consensus failure 2 days ago. To avoid rewinding the chain, we added this special case for the Parity bug.
There was a problem hiding this comment.
Good question. If I have this right... :
The 00...003 was one of the accounts touched by the attacker, and also by the statesweep. When it was touched by the statesweep, the actual 'touch', in the form of a CALL, went OOG.
This was at block 2675119 , at this tx. When the geth-bug causing the consensus failure today was fixed initially, it was discovered that the Parity implementation was also flawed; the OOG actually did not revert the deletion of 0..3 (ripemd). So the original fix resulted in consensus fail at 2675119 instead.
TLDR; geth failed to revert on OOG-transactions, and Parity failed to revert OOG sub-calls, basically. And the fix above is a hack to sync up with parity.
fjl
force-pushed
the
touch-delete-fix
branch
from
2cefbf3
to
bca7bfa
Compare
The description is based on ethereum/go-ethereum#3341 (comment)
The description is based on ethereum/go-ethereum#3341 (comment)
The description is based on ethereum/go-ethereum#3341 (comment)
The description is based on ethereum/go-ethereum#3341 (comment)
The description is based on ethereum/go-ethereum#3341 (comment)
No description provided.