Blockchain tests support: Do not expect tx sender always exist #684
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #684 +/- ##
==========================================
+ Coverage 97.56% 97.62% +0.06%
==========================================
Files 88 89 +1
Lines 8338 8388 +50
==========================================
+ Hits 8135 8189 +54
+ Misses 203 199 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rodiazet
changed the title
rodiazet
force-pushed
the
gas-price-0-allowed
branch
3 times, most recently
from
5e8cd03
to
e859c59
Compare
test/t8n/t8n.cpp
Outdated
| if (sender_acc_ptr == nullptr) | ||
| { | ||
| // It's possible to send tx from non-existent account before London fork. | ||
| // TODO: This should be handled in `validate_transaction` but it requires |
There was a problem hiding this comment.
I'm not sure. We should not assume the sender account exist.
Try changing in transition():
auto& sender_acc = state.get(tx.sender);to
auto& sender_acc = state.get_or_insert(tx.sender);
rodiazet
force-pushed
the
gas-price-0-allowed
branch
4 times, most recently
from
bdd54d2
to
4ab740c
Compare
rodiazet
force-pushed
the
gas-price-0-allowed
branch
from
4ab740c
to
3cfe536
Compare
chfast
force-pushed
the
gas-price-0-allowed
branch
from
3cfe536
to
aadae9a
Compare
chfast
changed the title
chfast
force-pushed
the
gas-price-0-allowed
branch
from
aadae9a
to
f7751d0
Compare
|
Added request for more tests: ethereum/tests#1286. |
test/unittests/state_transition.cpp
Outdated
|
|
||
| if (expect.tx_error != SUCCESS) | ||
| { | ||
| ASSERT_TRUE(holds_alternative<std::error_code>(res)); |
There was a problem hiding this comment.
Why is this assertion and not proper check, what if implementation mistakenly does successful transition for invalid transaction?
There was a problem hiding this comment.
There is another old check after this block which checks the other part. But probably make sense to rework this.
There was a problem hiding this comment.
Hm... I'm not sure how to do this better. Any suggestions?
There was a problem hiding this comment.
Actually I confused this with normal assertion. It think this is fine then, maybe just add a better message
ASSERT_TRUE(holds_alternative<std::error_code>(res))) << "tx expected to be invalid with error: " << expected.message();
| @@ -43,6 +44,10 @@ class state_transition : public testing::Test | |||
|
|
|||
| struct Expectation | |||
| { | |||
| /// The transaction is invalid because of the given error. | |||
| /// The rest of Expectation is ignored if the error is expected. | |||
There was a problem hiding this comment.
It could be expressed with std::variant<Expectation, ErrorCode>
There was a problem hiding this comment.
Good idea, but expect.post[tx.sender].nonce stops working because expect is variant now and does not have .post any more.
There was a problem hiding this comment.
Yeah the only solution that I can think of is adding expect() and tx_error() helpers that would set a variant when called, but perhaps this is too awkward.
| @@ -17,8 +17,26 @@ void state_transition::SetUp() | |||
|
|
|||
| void state_transition::TearDown() | |||
| { | |||
| auto& state = pre; | |||
| auto state = pre; | |||
There was a problem hiding this comment.
To compare if the state is unchanged in case of invalid transaction.
Properly handle the case when the account of transaction sender doesn't exist. In pre-Byzantium revisions such transaction could be valid with gas price 0. In any case the transaction validation should not crash. Fixes #692.
chfast
force-pushed
the
gas-price-0-allowed
branch
from
0f72852
to
e3d028c
Compare
Properly handle the case when the account of transaction sender doesn't
exist. In pre-Byzantium revisions such transaction could be valid with
gas price 0. In any case the transaction validation should not crash.
Fixes #692.