Add state.reward processing #558
Conversation
rodiazet
force-pushed
the
t8n-state-reward
branch
from
390393f
to
3337517
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
==========================================
+ Coverage 97.00% 97.03% +0.03%
==========================================
Files 66 66
Lines 6134 6134
==========================================
+ Hits 5950 5952 +2
+ Misses 184 182 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
test/t8n/t8n.cpp
Outdated
| state.touch(block.coinbase); | ||
| else if (state_reward == std::numeric_limits<intx::uint256>::max()) | ||
| { | ||
| if (state.get_or_insert(block.coinbase).is_empty()) |
There was a problem hiding this comment.
Hm, cleaning up empty accounts is just an EIP-161 rule. Don't we have that implemented?
There was a problem hiding this comment.
For <= EIP158 coinbase account is touched even it tx is rejected. retesteth passing -1 as a stare.reward requires zero state to be removed.
test/t8n/t8n.cpp
Outdated
| else | ||
| state.get_or_insert(block.coinbase).balance += *block_reward; | ||
| } | ||
| else if (rev <= EVMC_TANGERINE_WHISTLE) |
There was a problem hiding this comment.
Don't we do the same in transaction execution?
There was a problem hiding this comment.
no, we are touching there here we are erasing.
rodiazet
force-pushed
the
t8n-state-reward
branch
from
0d06cfd
to
6dae3ed
Compare
test/t8n/t8n.cpp
Outdated
| if (block_reward.has_value()) | ||
| state.touch(block.coinbase).balance += *block_reward; | ||
| else if (rev <= EVMC_TANGERINE_WHISTLE) // This behaviour is required by retesteth | ||
| state.get_accounts().erase(block.coinbase); |
There was a problem hiding this comment.
Hm, this will remove it even when other transactions touched this account. Is that desired?
There was a problem hiding this comment.
Sorry. For some reason I removed also checking if it's empty. Fixed now
There was a problem hiding this comment.
I'm still not convinced by this quirk. It looks it replicates a bug in some client implementation. Can you provide a list of failing tests?
rodiazet
force-pushed
the
t8n-state-reward
branch
from
6dae3ed
to
677692e
Compare
test/t8n/t8n.cpp
Outdated
| @@ -54,6 +55,9 @@ int main(int argc, const char* argv[]) | |||
| output_result_file = argv[i]; | |||
| else if (arg == "--output.alloc" && ++i < argc) | |||
| output_alloc_file = argv[i]; | |||
| else if (arg == "--state.reward" && ++i < argc) | |||
| if (std::string_view(argv[i]) != "-1") | |||
There was a problem hiding this comment.
You have to combine all 3 conditions. You can be fancy by doing argv[i] != "-1"sv.
There was a problem hiding this comment.
error: no matching literal operator for call to 'operator""sv' with arguments of types 'const char *' and 'unsigned long', and no matching literal operator template
else if (arg == "--state.reward" && ++i < argc && argv[i] != "-1"sv)
test/t8n/t8n.cpp
Outdated
| if (block_reward.has_value()) | ||
| state.touch(block.coinbase).balance += *block_reward; | ||
| else if (rev <= EVMC_TANGERINE_WHISTLE) // This behaviour is required by retesteth | ||
| state.get_accounts().erase(block.coinbase); |
There was a problem hiding this comment.
I'm still not convinced by this quirk. It looks it replicates a bug in some client implementation. Can you provide a list of failing tests?
rodiazet
force-pushed
the
t8n-state-reward
branch
from
677692e
to
02de26a
Compare
No description provided.