Integration test for validator joining network #493

JesseAbram · 2023-11-10T14:45:38Z

adds an integration test for joining and syncing a validator

vercel · 2023-11-10T14:45:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
entropy-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 13, 2023 9:30pm

ameba23

The test looks great.

A lot of my comments relate to the sync_validator function which isn't really relevant to this PR since that code was already there, this just moves it.

One other thing while i am looking at this - get_and_store_values will fail if it encounters a keyshare which is already in the db, as reserve_key will return an error. So sync_validator can only be run once when starting for the first time. If for whatever reason it fails to complete and we get some keyshares but not all, we can't run it again. For this case it might be better to ignore the error from reserve_key and keep getting the other keys in case there's some we don't have. I can make this into an issue.

ameba23 · 2023-11-13T14:59:02Z

crypto/server/src/helpers/launch.rs

-pub async fn setup_mnemonic(kv: &KvManager, is_alice: bool, is_bob: bool) -> Result<(), KvError> {
+pub async fn setup_mnemonic(
+    kv: &KvManager,
+    is_alice: bool,


An enum would be neater but im happy with leaving it like this

ameba23 · 2023-11-13T15:02:35Z

crypto/server/src/validator/api.rs

+        // if not in subgroup retry until you are
+        let mut my_subgroup = get_subgroup(&api, &rpc, &signer).await;
+        while my_subgroup.is_err() {
+            println!("you are not currently a validator, retrying");


replace with log::warn or are we happy with printlns in server?

Im ok with print line in server, unless other's are not, but currently we only do log in the substrate portions, gonna leave it here but if you feel strongly create an issue and card and we can overhaul all println in server

ameba23 · 2023-11-13T15:09:21Z

crypto/server/src/validator/api.rs

+            let health = rpc.system_health().await.expect("Issue checking chain health");
+            is_syncing = health.is_syncing;
+            if is_syncing {
+                println!("chain syncing, retrying {is_syncing:?}");


use log::info or log::debug?

ameba23 · 2023-11-13T15:21:57Z

crypto/server/src/validator/api.rs

+            }
+        }
+        // TODO: find a proper batch size
+        let batch_size = 10;


Probably not relevant to this PR as i think this was already here, but could this be a constant rather than needing to be passed between functions?

ameba23 · 2023-11-13T16:31:28Z

crypto/server/src/validator/api.rs

+            String::from_utf8(key_server_info.endpoint).expect("failed to parse IP address.");
+        let recip_key = x25519_dalek::PublicKey::from(key_server_info.x25519_public_key);
+        let all_keys = get_all_keys(&api, &rpc).await.expect("failed to get all keys.");
+        let _ = get_and_store_values(


Just to check this does what you intended - get_and_store_values returns a result, so if there is an error we ignore it, and go ahead with tell_chain_syncing_is_done

this handles a failure in a reservation of the kvdb but does it in a bad way, ill add an exist check here to fix

ameba23 · 2023-11-13T16:45:25Z

crypto/server/src/validator/api.rs

+    dev: bool,
+    endpoint: &str,
+    kv_store: &KvManager,
+) -> Result<(), ()> {


Seems strange to have a Result that never errors. I guess as this is called when launching server we are happy with panicking if something goes wrong. But i think id prefer to make this function return an error, and call it with expect from main.rs.

anyhow is quite good for this - you can also make a main function return an anyhow::Result and it will display the error nicely and give a non-zero process exit code.

But for now i think panicking is fine

ya will remove the result, leave the panics I mean we can remove them but idk if it is worth it but can talk about it for another PR

ameba23 · 2023-11-13T16:48:46Z

crypto/server/src/validator/api.rs

+        }
+        // if not in subgroup retry until you are
+        let mut my_subgroup = get_subgroup(&api, &rpc, &signer).await;
+        while my_subgroup.is_err() {


should we have a maximum number of tries?

no so in this case this is like npos stuff, like you can set up your validator while you are trying to get into the validator rotation and kinda just keep it running

as in like a validator can keep this running as long as they are willing to pay the compute cost while they try to get into the validator set

ameba23 · 2023-11-13T16:51:24Z

crypto/server/src/validator/api.rs

+        let mut is_syncing = true;
+        let sleep_time = Duration::from_secs(20);
+        // wait for chain to be fully synced before starting key swap
+        while is_syncing {


add a maximum number of tries? when we are deploying a lot of these it might be better to assume no-one is reading the logs and can see why its just hanging.

no I mean, in that case it is just a validator running in its own machine, let's say a year from now the chain could get long and it could take time to sync..... there is a linear relation between how long this will take and how long the chain is

JesseAbram · 2023-11-13T17:10:45Z

The test looks great.

A lot of my comments relate to the sync_validator function which isn't really relevant to this PR since that code was already there, this just moves it.

One other thing while i am looking at this - get_and_store_values will fail if it encounters a keyshare which is already in the db, as reserve_key will return an error. So sync_validator can only be run once when starting for the first time. If for whatever reason it fails to complete and we get some keyshares but not all, we can't run it again. For this case it might be better to ignore the error from reserve_key and keep getting the other keys in case there's some we don't have. I can make this into an issue.

agreed imma make an issue and a card for this

HCastano · 2023-11-13T16:56:08Z

crypto/server/src/helpers/launch.rs

+    is_alice: bool,
+    is_bob: bool,
+    is_charlie: bool,


What if we took an enum here instead and called the function a few times from the respective tests?

E.g

enum Keyring { Alice, Bob, ... } pub async fn setup_mnemonic(kv: &KvManager, account: Keyring) { ... } #[test] fn example() { setup_mnemonic(&kv, Keyring::Alice); setup_mnemonic(&kv, Keyring::Bob); }

crypto/server/src/validator/api.rs

crypto/testing-utils/src/substrate_context.rs

HCastano · 2023-11-13T17:25:20Z

crypto/server/src/validator/tests.rs

+    let rpc = get_rpc(&ctx.ws_url).await.unwrap();
+    let values = vec![vec![10], vec![11], vec![12]];
+    let keys = vec![
+        "5CiPPseXPECbkjWCa6MnjNokrgYjMqmKndv2rSnekmSK2DjL".to_string(),


Would be nice to have these as part of a keyring enum or sets of consts

HCastano · 2023-11-13T17:27:02Z

crypto/server/src/validator/api.rs

+    sync: bool,
+    dev: bool,
+    endpoint: &str,
+    kv_store: &KvManager,


We can also move these into a SyncConfig struct or something to make it easier to understand what's going on from the call site

HCastano · 2023-11-13T17:28:22Z

crypto/server/src/validator/api.rs

+        // if not in subgroup retry until you are
+        let mut my_subgroup = get_subgroup(&api, &rpc, &signer).await;
+        while my_subgroup.is_err() {
+            println!("you are not currently a validator, retrying");


Do we use any sort of logging library yet (e.g log)? If not we should make an issue to use that instead of println!s

we do not, I feel like we continued this in discord iirc

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* master: Integration test for validator joining network (#493) Add `ENTROPY_NODE` environment variable (#497) Change `SocketAddr` type for `String` (#496) Allow big protocol messages (#495) Add test for `EncryptedConnection` (#494) Use bincode rather than JSON for protocol and subscribe messages (#492) Remark on possible need to address TSS server from the Docker host. (#490) no proactive refresh on private key visibility (#485) Auxiliary data for program evaluation (#475) Validate proactive refresh endpoint (#483)

JesseAbram added 2 commits November 9, 2023 15:01

sync validator full test

7b8eee6

check sync state

6443c66

JesseAbram added 2 commits November 10, 2023 09:48

docs

312a62a

fmt

8a72e03

vercel bot deployed to Preview November 10, 2023 15:29 View deployment

fmt

9443960

JesseAbram marked this pull request as ready for review November 10, 2023 17:40

JesseAbram requested a review from ameba23 November 10, 2023 17:40

vercel bot deployed to Preview November 10, 2023 18:04 View deployment

JesseAbram requested review from jakehemmerle and HCastano November 10, 2023 19:19

ameba23 approved these changes Nov 13, 2023

View reviewed changes

refactors

f3c6f56

HCastano reviewed Nov 13, 2023

View reviewed changes

vercel bot deployed to Preview November 13, 2023 17:51 View deployment

refactor

d8331f2

HCastano mentioned this pull request Nov 13, 2023

Improve TSS logging and instrumentation #498

Closed

4 tasks

vercel bot deployed to Preview November 13, 2023 19:33 View deployment

JesseAbram and others added 5 commits November 13, 2023 15:51

create validator name enum

bef7b0f

Update crypto/server/src/validator/api.rs

fe4ca2f

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Update crypto/server/src/validator/api.rs

fdeb496

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Update crypto/testing-utils/src/substrate_context.rs

c1571c3

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

refactor

63292f4

vercel bot deployed to Preview November 13, 2023 21:30 View deployment

JesseAbram merged commit 46a3602 into master Nov 13, 2023
5 checks passed

JesseAbram deleted the integration-test-validator-joining branch November 13, 2023 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration test for validator joining network #493

Integration test for validator joining network #493

JesseAbram commented Nov 10, 2023 •

edited

Loading

vercel bot commented Nov 10, 2023 •

edited

Loading

ameba23 left a comment

ameba23 Nov 13, 2023

ameba23 Nov 13, 2023

JesseAbram Nov 13, 2023

ameba23 Nov 13, 2023

ameba23 Nov 13, 2023

ameba23 Nov 13, 2023

JesseAbram Nov 13, 2023

ameba23 Nov 13, 2023

JesseAbram Nov 13, 2023

ameba23 Nov 13, 2023

JesseAbram Nov 13, 2023 •

edited

Loading

JesseAbram Nov 13, 2023

ameba23 Nov 13, 2023

JesseAbram Nov 13, 2023

JesseAbram commented Nov 13, 2023

HCastano Nov 13, 2023

HCastano Nov 13, 2023

HCastano Nov 13, 2023

HCastano Nov 13, 2023 •

edited

Loading

JesseAbram Nov 13, 2023

Integration test for validator joining network #493

Integration test for validator joining network #493

Conversation

JesseAbram commented Nov 10, 2023 • edited Loading

vercel bot commented Nov 10, 2023 • edited Loading

ameba23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram commented Nov 10, 2023 •

edited

Loading

vercel bot commented Nov 10, 2023 •

edited

Loading

JesseAbram Nov 13, 2023 •

edited

Loading

HCastano Nov 13, 2023 •

edited

Loading