Validate proactive refresh endpoint #483

JesseAbram · 2023-11-07T20:33:02Z

Adds validation to proactive refresh endpoint that makes sure messages are reflected on chain and are not repeated

vercel · 2023-11-07T20:33:08Z

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 8, 2023 4:18pm

HCastano

So I don't really know what this proactive refresh feature is all about, can you give me a quick rundown? I think it would be helpful to have a bit more context here

crypto/server/src/signing_client/api.rs

HCastano · 2023-11-07T21:56:22Z

crypto/server/src/signing_client/api.rs

+    if u32::from_be_bytes(
+        last_block_number_recorded
+            .try_into()
+            .map_err(|_| ProtocolErr::Conversion("Block number conversion"))?,


This really shouldn't fail though, right? And what would a caller even be able to do in this case? My gut says that we panic here indicating a bug

idk even though I agree this should never fail persay, I like keeping error handling consistent through the codebase, introducing panics is a code design change on error handling and idk should probably be talked about

@JesseAbram I wouldn't worry about keeping things consistent in this case.

The key question here is "what can the calling code do in case of an error"? In this case it seems like nothing because it's a bug (we stored something in the db that couldn't be deserialized into a block number), so it would be acceptable to panic instead of moving forward like there was something to handle

HCastano · 2023-11-07T21:58:30Z

crypto/server/src/signing_client/api.rs

+        .await?
+        .ok_or_else(|| ProtocolErr::OptionUnwrapError("Failed to get block number".to_string()))?
+        .number;
+    // prevents multiple repeated messages being sent


Why are we checking for future block numbers here instead of simply checking for the existence for a key in the db?

@HCastano I'm not sure what you mean. How would that prevent the same proactive refresh being triggered more than once?

@ameba23 so from my understanding it seems like this can only be triggered once, and we'd have a record of that in the DB. So could we not check that key directly instead of checking for some future block number?

But I guess this can be triggered multiple times, so it's fine?

crypto/server/src/signing_client/api.rs

pallets/staking/src/lib.rs

HCastano · 2023-11-07T22:10:35Z

crypto/shared/src/types.rs

@@ -29,8 +29,18 @@ pub enum KeyVisibility {
 }

 /// Information from the validators in signing party
-#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
-#[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, TypeInfo)]
+#[cfg(not(feature = "wasm"))]


Why did you change this from being std to not(wasm)?

@HCastano this was probably the only thing which would compile.

For context, this crate is used in three different environments, which makes things complicated:

The runtime uses it on the wasm platform without std

server uses it natively with std

entropy-protocol uses it with std on either native or wasm

frame-support doesn't work on wasm with std

I double checked that entropy-protocol will compile to wasm on this branch, and i am gonna add a check to CI for it.

crypto/server/src/signing_client/tests.rs

HCastano · 2023-11-07T22:20:10Z

crypto/server/src/helpers/tests.rs

@@ -36,21 +36,22 @@ use crate::{
    AppState,
 };

-pub async fn setup_client() {
+pub async fn setup_client() -> KvManager {


So was there no way to access the kvdb from the tests before?

I'm thinking there might be a better way to provide access than to pass this out from the setup function

well I mean normally we don't need it and the kv manager can be accessed with our unsafe endpoints but that is way less clean and will bloat the code imo

ameba23

Looks great! (obviously pending CI passing)

Seems a shame there isn't a simpler way of checking that the HTTP request is made by the TSS server's corresponding chain node, as that is the only one who should be making these requests. But this works!

ameba23 · 2023-11-08T07:35:19Z

crypto/shared/src/types.rs

@@ -29,8 +29,18 @@ pub enum KeyVisibility {
 }

 /// Information from the validators in signing party
-#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
-#[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, TypeInfo)]
+#[cfg(not(feature = "wasm"))]


@HCastano this was probably the only thing which would compile.

For context, this crate is used in three different environments, which makes things complicated:

The runtime uses it on the wasm platform without std

server uses it natively with std

entropy-protocol uses it with std on either native or wasm

frame-support doesn't work on wasm with std

I double checked that entropy-protocol will compile to wasm on this branch, and i am gonna add a check to CI for it.

ameba23 · 2023-11-08T08:27:17Z

crypto/server/src/signing_client/api.rs

+        .await?
+        .ok_or_else(|| ProtocolErr::OptionUnwrapError("Failed to get block number".to_string()))?
+        .number;
+    // prevents multiple repeated messages being sent


@HCastano I'm not sure what you mean. How would that prevent the same proactive refresh being triggered more than once?

crypto/server/src/signing_client/api.rs

ameba23 · 2023-11-08T08:41:06Z

crypto/server/src/user/api.rs

@@ -544,8 +543,9 @@ pub async fn validate_new_user(
    if verifying_data_hash != chain_data_hash {
        return Err(UserErr::InvalidData);
    }
-    kv_manager.kv().delete("LATEST_BLOCK_NUMBER").await?;
-    let reservation = kv_manager.kv().reserve_key("LATEST_BLOCK_NUMBER".to_string()).await?;
+    kv_manager.kv().delete("LATEST_BLOCK_NUMBER_NEW_USER").await?;


I think that reserve_key will fail if the key exists. TBH i don't understand the reasoning behind having a reservation system rather than a put method which returns an error if the key already exists, as sled has some pretty good thread safety features out of the box. But thats not relevant to this PR.

pallets/staking/src/lib.rs

ameba23 · 2023-11-08T08:51:04Z

pallets/propagation/src/lib.rs

            // We construct the request
            // important: the header->Content-Type must be added and match that of the receiving
            // party!!
-            let pending = http::Request::post(url, vec![validators_info.encode()]) // scheint zu klappen
+            let pending = http::Request::post(url, vec![req_body.encode()]) // scheint zu klappen


In case anyone was wondering, this comment says 'seems to work', lol

oh this is really old lol, ill remove, long story im not gonna explain here

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

JesseAbram · 2023-11-08T14:14:28Z

Looks great! (obviously pending CI passing)

Seems a shame there isn't a simpler way of checking that the HTTP request is made by the TSS server's corresponding chain node, as that is the only one who should be making these requests. But this works!

we can def look into having the TSS key maybe be held by the substrate node and sign off on messages, that would simplify this function but make the launching of a node harder and wouldn't really reduce computations much so idk if it is worth it

JesseAbram · 2023-11-08T15:22:27Z

So I don't really know what this proactive refresh feature is all about, can you give me a quick rundown? I think it would be helpful to have a bit more context here

It is a way to rotate keys in an MPC situation where the new keys still resolve to the same signature address, in entropy the goal is to have the network trigger this and to be done in the background

HCastano · 2023-11-08T19:09:08Z

It is a way to rotate keys in an MPC situation where the new keys still resolve to the same signature address, in entropy the goal is to have the network trigger this and to be done in the background

@JesseAbram and why would we want to trigger this? What's the benefit of rotating keys here? And which keys are being rotated?

JesseAbram · 2023-11-08T19:21:01Z

It is a way to rotate keys in an MPC situation where the new keys still resolve to the same signature address, in entropy the goal is to have the network trigger this and to be done in the background

@JesseAbram and why would we want to trigger this? What's the benefit of rotating keys here? And which keys are being rotated?

The keys are the entropy shards "the 1 of n" key shard that you do the MPC with. This is because as the validator set rotates and old validators are booted for new validators, their key shards will be invalid

* 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 requested review from ameba23 and HCastano November 7, 2023 20:33

JesseAbram added 6 commits November 7, 2023 15:38

add to validate proactive refresh endpoint

2cd6cc4

working tests

5b519f9

clean

413155f

docs

fc0e54e

fmt

17088d0

fmt

7c24d51

JesseAbram force-pushed the validate-proactive-refresh-endpoint branch from c82236f to 7c24d51 Compare November 7, 2023 20:38

metadata

41c4f5e

vercel bot deployed to Preview November 7, 2023 21:16 View deployment

HCastano reviewed Nov 7, 2023

View reviewed changes

ameba23 approved these changes Nov 8, 2023

View reviewed changes

JesseAbram and others added 4 commits November 8, 2023 06:06

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

23997d0

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

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

a66271a

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

Update pallets/staking/src/lib.rs

c1030c2

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

Update crypto/server/src/signing_client/tests.rs

5e52ed4

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

JesseAbram added 2 commits November 8, 2023 09:28

refactors

9f3fa3f

fix test

b42c7e8

JesseAbram added 2 commits November 8, 2023 10:24

docs

887514a

refactor

9069323

JesseAbram merged commit 1696407 into master Nov 8, 2023
5 checks passed

JesseAbram deleted the validate-proactive-refresh-endpoint branch November 8, 2023 16:13

vercel bot deployed to Preview November 8, 2023 16:18 View deployment

JesseAbram mentioned this pull request Nov 14, 2023

Validate proactive refresh endpoint #454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate proactive refresh endpoint #483

Validate proactive refresh endpoint #483

JesseAbram commented Nov 7, 2023

vercel bot commented Nov 7, 2023 •

edited

Loading

HCastano left a comment

HCastano Nov 7, 2023

JesseAbram Nov 8, 2023

HCastano Nov 8, 2023

HCastano Nov 7, 2023

ameba23 Nov 8, 2023

HCastano Nov 8, 2023

HCastano Nov 7, 2023

ameba23 Nov 8, 2023

HCastano Nov 7, 2023

JesseAbram Nov 8, 2023

ameba23 left a comment

ameba23 Nov 8, 2023

ameba23 Nov 8, 2023

ameba23 Nov 8, 2023

ameba23 Nov 8, 2023

JesseAbram Nov 8, 2023 •

edited

Loading

JesseAbram commented Nov 8, 2023

JesseAbram commented Nov 8, 2023

HCastano commented Nov 8, 2023

JesseAbram commented Nov 8, 2023

Validate proactive refresh endpoint #483

Validate proactive refresh endpoint #483

Conversation

JesseAbram commented Nov 7, 2023

vercel bot commented Nov 7, 2023 • edited Loading

HCastano 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

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

JesseAbram Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

JesseAbram commented Nov 8, 2023

JesseAbram commented Nov 8, 2023

HCastano commented Nov 8, 2023

JesseAbram commented Nov 8, 2023

vercel bot commented Nov 7, 2023 •

edited

Loading

JesseAbram Nov 8, 2023 •

edited

Loading