-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Savanna disaster recovery unit tests. #444
Conversation
After base_tester::open() - need to reconnect the signals - need to re-initialize the node finalizers Also add a couple working tests.
} | ||
} | ||
})); | ||
} FC_LOG_AND_RETHROW() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how clean all these test cases are. Well done!
Note:start |
|
||
using namespace eosio::chain; | ||
using namespace eosio::testing; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description is very informative. Copy some high level info to the beginning of each test to describe test purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to have the test be almost as readable as the list of steps in the description above, and I feel it is better to have one source of truth.
When duplicating the code logic into comments, they often become inconsistent as the code is changed while the comments are not. This is why I did not want to add the list of steps as a block comment on top. However, let me see if I can add a more general comment describing the intent of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add the exact list of steps here, just a short sentence at the beginning would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
BOOST_REQUIRE_EQUAL(A.lib_advances_by([&]() { A.produce_blocks(4); }), 4); // lib still advances with 3 finalizers | ||
C.open(); | ||
A.push_blocks_to(C); | ||
BOOST_REQUIRE_EQUAL(A.lib_advances_by([&]() { A.produce_blocks(4); }), 4); // all 4 finalizers should be back voting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment can be improved. Here only checks LIB advancing; the next checks C is back to vote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that comment really applies to the next line, and the // let's make sure of that
implies that it is only verified on that line.
auto& A=_nodes[0]; auto& C=_nodes[2]; | ||
|
||
C.close(); | ||
BOOST_REQUIRE_EQUAL(A.lib_advances_by([&]() { A.produce_blocks(4); }), 4); // lib still advances with 3 finalizers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we verify only 3 finalizers (not including C) are voting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have an API for doing this directly... let me see if I can add one.
} | ||
|
||
void set_node_finalizers() { | ||
if (node_num_keys) | ||
t.set_node_finalizers({&key_names.at(node_first_key), node_num_keys}); | ||
t.set_node_finalizers({&key_names.at(node_first_key_idx), node_num_keys}); | ||
} | ||
|
||
// updates the finalizer_policy to the `fin_policy_size` keys starting at `first_key` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment about first_key
should be changed too. But OK not to touch them this time.
Partially resolves #380
Implemented here:
unit tests: Disaster recovery
Single finalizer goes down
[sd0] recovery when nodes go down
[sd1] Recover a killed node with old finalizer safety info
[sd2] Recover a killed node with deleted finalizer safety info
[sd3] Recover a killed node while retaining up to date finalizer safety info
All but one finalizer nodes go down
Tests are similar above, except that C is replaced by the set { B, C, D }, and lib stops advancing when { B, C, D } are shutdown
[md0] recovery when nodes go down
[md1] Recover a killed node with old finalizer safety info
[md2] Recover a killed node with deleted finalizer safety info
[md3] Recover a killed node while retaining up to date finalizer safety info
All nodes are shutdown with reversible blocks lost
[rv0] nodes shutdown with reversible blocks lost
lib_id
) and head block ID (h_id
) - the snapshot blocklib_id
's child which was lostlib_id
(because validators are locked on a reversible block which has been lost, so they cannot vote any since the claim on the lib block is just copied forward and will always be on a block with a timestamp < that the lock block in the fsi)