Add test environment with four TS servers #1029
Conversation
| next_signers: self.mock_signer_rotate.clone().1, | ||
| confirmations: vec![], | ||
| }); | ||
| let next_signers = &mut self.mock_signer_rotate.1.clone(); |
There was a problem hiding this comment.
This hasn't changed how we interpret mock_signer_rotate - but it wasn't working for me as it was. I think it was mutating a clone of self.mock_signer_rotate but then using a different clone for next signers.
|
|
||
| /// A helper for setting up tests which starts both a set of TS servers and a chain node and returns | ||
| /// the chain API as well as IP addresses and PartyId of the started validators | ||
| pub async fn spawn_tss_nodes_and_start_chain( |
There was a problem hiding this comment.
This isn't strictly related to this PR, but since adding a way to specify which chainspec you want to use, i also added this helper which could slightly reduce boilerplate in our tests.
So this would go at the beginning of most of our tests. I haven't actually called this anywhere yet though - i wanted to run it by yous first.
| use sp_keyring::AccountKeyring; | ||
| use subxt::utils::AccountId32; | ||
| use synedrion::k256::ecdsa::VerifyingKey; | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn test_reshare() { |
There was a problem hiding this comment.
Here we try out the 4 node setup with a reshare moving the signers from alice, bob and charlie to bob, charlie and dave.
There was a problem hiding this comment.
Tbh not a fan of spinning up four nodes as part of each test, but it's one of those things that if we need to do it to get sufficient test coverage then it is what is it 🤷
Would be curious about what kind of system resources we're consuming and if this is really a concern though.
| }, | ||
| Hasher, | ||
| }; | ||
| use entropy_client::{register, sign, store_program}; |
There was a problem hiding this comment.
I'm personally a fan of the use entropy_client as test _client import since in the test it's clear that these functions are coming from the client and not part of this crate
|
|
||
| /// A helper for setting up tests which starts both a set of TS servers and a chain node and returns | ||
| /// the chain API as well as IP addresses and PartyId of the started validators | ||
| pub async fn spawn_tss_nodes_and_start_chain( |
I hear you. Having choice between development or integration test setup is not so helpful, because if i understand correctly, we need the integration test setup's force authoring thingy in order to jumpstart the network - which is pretty much all of our tests - meaning we always use four nodes. It is actually possible to use the integration test chainspec and only 3 TSS nodes. I tried it and for registering and signing, no-one complains that dave is not there. Its only for a reshare that dave is needed. But it still kinda feels wrong to have dave in the chainspec and not be running. So one option to solve this might be to add the force authoring stuff to the development chainspec, and then use that for all the tests which don't involve a reshare.
Tbh i think in comparison to the chain node it is relatively little. Especially as we don't actually run the TSS servers in separate OS processes - its just one |
For some tests it would be useful to have four TS servers - in particular the reshare tests, so that we can test adding and removing members of the signing committee during a reshare.
My personal motivation is that i am thinking to request an attestation from the new member of the signing committee before doing a reshare - and ideally i would like to test the new session handler as part of this.
This PR makes the integration test chainspec have 4 TS servers, and leaves the 'dev' chainspec with only 3 as before. Since the extra node, Dave, is not in the initial signing committee and has no initial keyshares, most of our exisiting tests which use the integration test chainspec should continue to work as they are.
This also extends the reshare test to demonstrate siging a message after a reshare. This closes #981 and partially #1010 - i say partially because although we test signing after a reshare, we mock the initial jumpstart DKG part - so its not really testing the complete process.
Im willing to close this if people think that testing with 4 TS servers is going to make our tests too heavy in terms of resources.