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

Coming to agreement with geth on RIPEMD touch revert #5652

Closed
gumb0 wants to merge 2 commits into from
Closed

Coming to agreement with geth on RIPEMD touch revert #5652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cdea49
Change the logic around reverting touched Precompiled contracts to ag…
gumb0 Jul 4, 2019
a5a73ff
Add changelog item.
gumb0 Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
- Fixed: [#5627](https://github.com/ethereum/aleth/pull/5627) Correct testeth --help log output indentation.
- Fixed: [#5644](https://github.com/ethereum/aleth/pull/5644) Avoid attempting to sync with disconnected peers.
- Fixed: [#5647](https://github.com/ethereum/aleth/pull/5647) test_importRawBlock RPC method correctly fails in case of import failure.
- Fixed: [#5652](https://github.com/ethereum/aleth/pull/5652) Behavior in corner case tests about touching empty Precompiles now agrees with geth's results.


## [1.6.0] - 2019-04-16
Expand Down
17 changes: 15 additions & 2 deletions libethereum/State.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ using namespace dev;
using namespace dev::eth;
namespace fs = boost::filesystem;

namespace
{
Address const c_RipemdPrecompiledAddress{0x03};
}

State::State(u256 const& _accountStartNonce, OverlayDB const& _db, BaseState _bs):
m_db(_db),
m_state(&m_db),
Expand Down Expand Up @@ -589,8 +594,16 @@ void State::rollback(size_t _savepoint)
account.resetCode();
break;
case Change::Touch:
account.untouch();
m_unchangedCacheEntries.emplace_back(change.address);
// Empty RIPEMD contract needs to be deleted even in case of exception.
// This doesn't affect main net, but is needed for some consensus tests.
// https://github.com/ethereum/go-ethereum/pull/3341/files#diff-2433aa143ee4772026454b8abd76b9dd
// https://github.com/ethereum/EIPs/issues/716
// https://github.com/ethereum/aleth/pull/5652
if (change.address != c_RipemdPrecompiledAddress)
Copy link
Copy Markdown
Member

@chfast chfast Jul 9, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@gumb0 gumb0 Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@gumb0 gumb0 Jul 10, 2019

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Member Author

@gumb0 gumb0 Jul 10, 2019

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Member Author

@gumb0 gumb0 Jul 10, 2019

Choose a reason for hiding this comment

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

#5664

{
account.untouch();
m_unchangedCacheEntries.emplace_back(change.address);
}
break;
}
m_changeLog.pop_back();
Expand Down
2 changes: 1 addition & 1 deletion test/jsontests
Open in desktop
Submodule jsontests updated 6291 files