Add `/relay_tx` endpoint #1050

JesseAbram · 2024-09-12T21:15:49Z

Related #1033

Closes #899

Passes a message first to relay_tx which must be a validator not signer then has the relayer select the signers
Passes messages from signer back to relayer then to client

Points of discussion

If we want randomness in the signing session we now can have the relayer pass randomness that can be used in session id creation

…signing-relayer

ameba23

Generally all looks very good and great that this got done so quickly as its quite a big feature.

I am not done with reading over the changes to the tests so leaving this as a comment for now and will get to that later or tomorrow.

crates/client/src/client.rs

ameba23 · 2024-09-23T06:35:32Z

crates/client/src/user.rs

-    validators.truncate(threshold as usize);
+    let selected_signers: Vec<_> = {
+        let mut cloned_signers = signers.clone();
+        // TODO: temp remove dave for now until test dave is spun up correctly


After looking into this i think needing this is caused by a problem that i introduced, so sorry about that.

What i think it is the issue is that the pre-generated keyshares are being given to alice, bob and charlie, but now that we have random signer selection on jumpstart, dave is actually chosen as an initial signer but is never given a keyshare.

I have made a PR to hopefully fix that and we should be able to get rid of this pop.

#1061

crates/threshold-signature-server/src/user/api.rs

ameba23 · 2024-09-23T07:15:44Z

crates/threshold-signature-server/src/user/api.rs

@@ -524,3 +632,65 @@ pub fn check_hash_pointer_out_of_bounds(
        _ => Ok(()),
    }
 }
+
+pub async fn pre_sign_checks(


I might have missed it somewhere but i can't see a check that the RelayerSignatureRequest::validators_info are actually current signers, or that there are t of them.

I know this is comes from another TS server so should be good, but i would check anyway to avoid slashing the wrong node when things go wrong.

hmmm good point from a security checkpoint I mean like it won't sign it won't sign (im avoiding thinking of slashing since it doesn't exist yet) but I like to catch any errors first and bail out for ddos reasons, so will add that check

ameba23 · 2024-09-23T07:24:03Z

crates/client/src/user.rs

+    pub validators_info: Vec<ValidatorInfo>,
+}
+
+/// Returns a threshold of signer's ValidatorInfo from the chain
 pub async fn get_signers_from_chain(


If i've understood right, this is only ever called by the TS server, and not by the client. In which case it should probably be in entropy-tss rather than here. Using stuff like rand::thread_rng in here will restrict what platforms entropy-client can be used on.

Can you elaborate on this? What are the platforms where rand::thread_rng is not available? WASM? TDX hosts?

Yep i was thinking wasm. Although its not really clear what platforms we are trying to support for clients, its good to keep as many doors open as possible. If this was a client function, i would suggest having the rng be passed in as an argument. But since its not, i would say it just shouldn't be in this crate.

I see, thank you for the explanation. :)

ameba23 · 2024-09-23T07:47:36Z

crates/threshold-signature-server/src/user/api.rs

+                if resp.status() == 200 {
+                    let signing_result: Result<(String, Signature), String> =
+                        serde_json::from_slice(&chunk)?;
+                    send_back.push(signing_result);


As i mentioned on discord, in the happy case where signing is successful, i would do some processing here to make things simple for the client:

For each Ok((String,Signature)), validate the signatures from the TS nodes, check the strings are all identical, then send back just one response, rather than a vec of responses, signed by the relayer node rather than all signers.

In the error case it probably still makes sense to send a vec of errors, in case there is some mismatching that the client might want to investigate.

This is a 'nice to have' - i'd be happy merging this without it.

I hear what you are saying but does it make things less secure as we take multiple responses and allow the relayer to select one, I mean probably fine, that being said I think this is good for this PR and let's open an issue and maybe take JS side feedback here

I'm with Peg here, think it makes sense to cross reference all the signatures and just send back a single signature.

I hear what you are saying but does it make things less secure as we take multiple responses and allow the relayer to select one, I mean probably fine, that being said I think this is good for this PR and let's open an issue and maybe take JS side feedback here

We're already trusting the relayer by sending a message to them in the first place. Plus, if the message is signed by a signer it doesn't really matter what the relayer sends back

ameba23 · 2024-09-23T09:55:00Z

crates/threshold-signature-server/src/helpers/tests.rs

@@ -205,6 +205,8 @@ pub async fn spawn_testing_validators(
        let dave_id = PartyId::new(SubxtAccountId32(
            *get_signer(&dave_kv).await.unwrap().account_id().clone().as_ref(),
        ));
+        // TODO: fix


This should be hopefully be fixed with #1061

Ya, can you add an issue number here Jesse?

ameba23 · 2024-09-23T10:47:12Z

crates/threshold-signature-server/src/helpers/tests.rs

@@ -278,23 +280,28 @@ pub async fn jump_start_network_with_signer(
    api: &OnlineClient<EntropyConfig>,
    rpc: &LegacyRpcMethods<EntropyConfig>,
    signer: &PairSigner<EntropyConfig, sr25519::Pair>,
-) {
+) -> ValidatorName {


Maybe add to the doccomment: 'Returns the name of a validator who is not a signer.'

Also we should be aware that defaulting to alice might cause confusion if using this with a different chainspec - strictly speaking i think this should be an Option<ValidatorName>.

dvdplm · 2024-09-23T10:58:24Z

CHANGELOG.md

@@ -19,6 +19,10 @@ At the moment this project **does not** adhere to
  pallet. The genesis build config was also removed. Additionally, the `new/user/` HTTP endpoint in
  the TSS was removed since it was no longer necessary.
 - In [#1045](https://github.com/entropyxyz/entropy-core/pull/1045), `ProgramsInfo` now takes `version_number` to maintain backwards compatibility if programs runtime is updated
+- In [#1050](https://github.com/entropyxyz/entropy-core/pull/1050), the flow for signing has changed. 
+A user now sends their request to a validator that is not a signer. This will act as a realyer. 


Is it correct to say "…to any validator" here? Or is it "…to any validator that is not a signer"?

the second one is def correct, as you def can't talk straight to a signer anymore

dvdplm · 2024-09-23T11:06:08Z

crates/client/src/client.rs

+    // take only one of the responses
+    let (signature_base64, signature_of_signature) =
+        signing_results[0].clone().map_err(ClientError::SigningFailed)?;


Dumb (and paranoid) question: shouldn't we pick a random response here? So we can be sure that there's no way for an attacker to anticipate which validator will reply to us?

Also: why do you need to clone here?

mmmm ya potentially, can change this, I mean......like it is def better, however I am not sure it is more secure, anyways will change

dvdplm · 2024-09-23T11:09:54Z

crates/client/src/client.rs

+    }
+
+    if !sig_recovery_results.contains(&true) {
+        return Err(ClientError::BadSignature);


If this ever happens, how can you tell who the bad guy was? Why not bail earlier, when you're collecting the verification results?

ya I mean this relates to peg's earlier comment, I will open an issue in a bit and link it, pretty much how we return the results im open to discuss

crates/client/src/user.rs

dvdplm · 2024-09-23T11:12:20Z

crates/client/src/user.rs

+    pub validators_info: Vec<ValidatorInfo>,
+}
+
+/// Returns a threshold of signer's ValidatorInfo from the chain
 pub async fn get_signers_from_chain(


Can you elaborate on this? What are the platforms where rand::thread_rng is not available? WASM? TDX hosts?

HCastano · 2024-09-23T20:57:02Z

I'm gonna take a look at this tomorrow 🙏

crates/threshold-signature-server/src/lib.rs

ameba23

Great, and especially great for updating doccomments and adding extra validator info checks. 🌟

I think we gotta make sure client developers are aware that if they are getting an error from one of the checks made by the relayer node (eg: program fails), the right thing to do is try another random non-signing node. We could do this in entropy-client as an example.

ameba23 · 2024-09-24T08:23:09Z

crates/threshold-signature-server/src/lib.rs

+//!
+//! "[{\"Err\":\"Too many requests - wait a block\"},{\"Err\":\"Too many requests - wait a block\"}]"
+//!
+//! Curl example for `user/sign_tx`:


Suggested change

//! Curl example for `user/sign_tx`:

//! Curl example for `user/relay_tx`:

ameba23 · 2024-09-24T08:42:18Z

crates/threshold-signature-server/src/lib.rs

+//! Curl example for `user/sign_tx`:
+//! ```text
+//! curl -X POST -H "Content-Type: application/json" \
+//!   -d '{"msg" "0x174...hex encoded signedmessage...","sig":"821754409744cbb878b44bd1e3dc575a4ea721e12d781b074fcdb808fc79fd33dd1928b1a281c0b6261a30536a7c0106a102f27dad1bc3ef475b626f0e57c983","pk":[172,133,159,138,33,110,235,27,50,11,76,118,209,24,218,61,116,7,250,82,52,132,208,169,128,18,109,59,77,13,34,10],"recip":[10,192,41,240,184,83,178,59,237,101,45,109,13,230,155,124,195,141,148,249,55,50,238,252,133,181,134,30,144,247,58,34],"a":[169,94,23,7,19,184,134,70,233,117,2,84,242,135,246,95,159,14,218,125,209,191,175,89,41,196,182,96,117,5,159,98],"nonce":[114,93,158,35,209,188,96,248,85,131,95,237]}' \


Unrelated to this PR, but this example needs updating - i made #1065

ameba23 · 2024-09-24T08:44:36Z

crates/threshold-signature-server/src/lib.rs

@@ -32,17 +32,57 @@
 //! is an encrypted, signed message.
 //!
 //!
-//! #### `/user/sign_tx` - POST
+//! #### `/user/relay_tx` - POST


🤼‍♂️ 🪩

as promised you get men with bunny ears partying and disco ball for updating the API doccomments. and a medal 🥇 Well actually it seems github doesnt have men with bunny ears so you get men wrestling.

crates/threshold-signature-server/src/user/tests.rs

ameba23 · 2024-09-24T09:02:57Z

crates/threshold-signature-server/src/user/tests.rs

+}
+#[tokio::test]
+#[serial]
+async fn test_signature_requests_fail_validator_info_wrong() {


👍 Great to have these in

ameba23 · 2024-09-24T09:19:37Z

CHANGELOG.md

@@ -19,6 +19,10 @@ At the moment this project **does not** adhere to
  pallet. The genesis build config was also removed. Additionally, the `new/user/` HTTP endpoint in
  the TSS was removed since it was no longer necessary.
 - In [#1045](https://github.com/entropyxyz/entropy-core/pull/1045), `ProgramsInfo` now takes `version_number` to maintain backwards compatibility if programs runtime is updated
+- In [#1050](https://github.com/entropyxyz/entropy-core/pull/1050), the flow for signing has changed. 
+A user now sends their request to any validator that is not a siger. This will act as a realyer. 


Suggested change

A user now sends their request to any validator that is not a siger. This will act as a realyer.

A user now sends their request to any validator that is not a signer. This will act as a relayer.

Could also say that the endpoint for submitting user signature requests has changed to user/relay_tx

HCastano

Thanks for doing this!

Left lots of comments, but nothing blocking this. Would be nice if you could address or ack them though.

CHANGELOG.md

HCastano · 2024-09-24T16:16:39Z

crates/threshold-signature-server/src/lib.rs

@@ -169,6 +209,7 @@ pub fn app(app_state: AppState) -> Router {
    let mut routes = Router::new()
        .route("/generate_network_key", post(generate_network_key))
        .route("/user/sign_tx", post(sign_tx))
+        .route("/user/relay_tx", post(relay_tx))


For a follow up, but I would rename these endpoints to sign_message and relay_message to better match some of the newer endpoint names.

agree - after all 'the transaction is dead' lol

HCastano · 2024-09-24T16:27:04Z

crates/threshold-signature-server/src/user/errors.rs

+    #[error("Message not sent to a validator")]
+    NotValidator,
+    #[error("Relay message can not be sent to a signer")]
+    RelayMessageSigner,


Suggested change

RelayMessageSigner,

RelayMessageSigner,

How is this error different from NotRelayedFromValidator,?

so one is a message is sent to a signer and is not from a relayer. The other is a relayer is a validator but also a signer, which is technically different,

HCastano · 2024-09-24T16:29:30Z

crates/threshold-signature-server/src/user/api.rs

+
+    tracing::Span::current().record("request_author", signed_message.account_id().to_string());
+
+    let signers = get_signers_from_chain(&api, &rpc).await?;


Would it make sense to grab this info from the get_validators_info(..., signers) call above?

Or maybe to use get_validators_info in the implementation of get_signers_from_chain()

HCastano · 2024-09-24T16:35:51Z

crates/client/src/user.rs

-pub async fn get_signers_from_chain(
+/// Represents an unparsed transaction request coming from a relayer to a signer.
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct RelayerSignatureRequest {


We can probably just have this in the body of UserSignatureRequest, something like:

pub struct UserSignatureRequest { pub request: RelayerSignatureRequest, pub validators_info: Vec<ValidatorInfo>, }

yes but opposite

crates/threshold-signature-server/src/user/tests.rs

HCastano · 2024-09-24T17:28:47Z

crates/threshold-signature-server/src/user/tests.rs

+        get_sign_tx_data(&entropy_api, &rpc, hex::encode(PREIMAGE_SHOULD_SUCCEED), verifying_key)
+            .await;
+
+    validators_info.pop();


So here we pop an entry to trigger the "too few signers" error? Do you mind writing a comment explaining this. Can also be done as part of Peg's suggested changes

HCastano · 2024-09-24T17:30:43Z

crates/threshold-signature-server/src/user/tests.rs

+        tss_account,
+    });
+
+    let test_user_res_wrong_validator = submit_transaction_sign_tx_requests(


Would you be able to add a Test: ... annotation here too?

HCastano · 2024-09-24T17:32:10Z

crates/threshold-signature-server/src/user/tests.rs

+    .unwrap();
+
+    let signature_request_responses_fail_not_relayer = mock_client
+        .post("http://127.0.0.1:3001/user/sign_tx")


We have the validator IPs above, we can probably use that instead of hardcoding the address and port here

HCastano · 2024-09-24T17:33:57Z

crates/threshold-signature-server/src/user/tests.rs

+    let mock_client = reqwest::Client::new();

+    let signed_message = EncryptedSignedMessage::new(
+        &alice.pair(),
+        serde_json::to_vec(&signature_request.clone()).unwrap(),
+        &X25519_PUBLIC_KEYS[0],
+        &[],
+    )
+    .unwrap();
+
+    let signature_request_responses_fail_not_relayer = mock_client
+        .post("http://127.0.0.1:3001/user/sign_tx")
+        .header("Content-Type", "application/json")
+        .body(serde_json::to_string(&signed_message).unwrap())
+        .send()
+        .await;
+    assert_eq!(
+        signature_request_responses_fail_not_relayer.unwrap().text().await.unwrap(),
+        "Message sent directly to signer"
+    );


I would keep this test as strictly a happy-path test.

Could this be moved to say, test_signature_requests_fail_on_different_conditions, or even to a standalone test? Don't think we need the whole jumpstart or registration setup to trigger this error

…signing-relayer

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

Co-authored-by: peg <peg@magmacollective.org>

* master: Bump clap from 4.5.18 to 4.5.19 in the patch-dependencies group (#1091) Avoid panic by checking that we have a non-signing validator before selecting one (#1083) Fix master build (#1088) Small fixes to `test-cli` (#1084) Pregenerate keyshares sets for all possible initial signer comittees (#1073) Fix master build (#1079) Bump reqwest from 0.12.7 to 0.12.8 in the patch-dependencies group (#1082) Block tss chain when signer (#1078) Run multiple test validator (#1074) Bump tempfile from 3.12.0 to 3.13.0 (#1076) Bump axum from 0.7.6 to 0.7.7 in the patch-dependencies group (#1075) Unignore register and sign integration test, and do a non-mock jumpstart (#1070) No unbonding when signer or next signer (#1031) Add `/relay_tx` endpoint (#1050) Fix how pre-generated keyshares are added for tests (#1061) Handle Provisioning Certification Keys (PCKs) (#1051) Bump async-trait from 0.1.82 to 0.1.83 in the patch-dependencies group (#1067)

JesseAbram added 18 commits September 12, 2024 17:09

add relay tx endpoint

26e1519

Merge branch 'master' of github.com:entropyxyz/entropy-core into add-…

0c33812

…signing-relayer

working

2b2594e

redesign test

8150b14

fix sig recovery check

350e9c0

add pre sign checks

51b3353

add signer check

abe8958

add validator checks

75a1721

remove validator info from user sig req message

6256782

fixing tests

f238480

another test

e8c4858

fix user tests

0c17e6e

more tests

feef814

clean

411b0cb

test

8ab7646

clean

c89d04b

fmt

f25e7a4

fmt

2d40885

JesseAbram changed the title ~~add relay tx endpoint~~ Add relay tx endpoint Sep 19, 2024

JesseAbram added 6 commits September 19, 2024 10:40

docs

5d96f9d

tests

352f90b

Merge branch 'master' of github.com:entropyxyz/entropy-core into add-…

4739610

…signing-relayer

fmt

0216957

fix tests

5f9745d

fix test

0074333

JesseAbram marked this pull request as ready for review September 20, 2024 20:04

JesseAbram requested review from HCastano and ameba23 September 20, 2024 20:04

ameba23 reviewed Sep 23, 2024

View reviewed changes

dvdplm reviewed Sep 23, 2024

View reviewed changes

JesseAbram added 4 commits September 23, 2024 11:32

random selection

74796ee

add validator checks

13a3511

clean tests

dbed146

tests

f8a7a72

ameba23 reviewed Sep 24, 2024

View reviewed changes

crates/threshold-signature-server/src/lib.rs Outdated Show resolved Hide resolved

ameba23 approved these changes Sep 24, 2024

View reviewed changes

HCastano approved these changes Sep 24, 2024

View reviewed changes

HCastano changed the title ~~Add relay tx endpoint~~ Add /relay_tx endpoint Sep 24, 2024

JesseAbram and others added 16 commits September 25, 2024 12:42

remove poping selected signers

d545ecf

Merge branch 'master' of github.com:entropyxyz/entropy-core into add-…

f862b6e

…signing-relayer

fix

d59d6a0

Apply suggestions from code review

31afe48

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

collapse user_sig_request into relayer request

f57a5b2

Apply suggestions from code review

3d7cec4

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

clean

59906f7

clean

d423fee

clean

57485d8

clean

826af21

Update crates/threshold-signature-server/src/user/tests.rs

6682807

Co-authored-by: peg <peg@magmacollective.org>

clean

13eced6

replace charlie with dave in key creation

9abc957

fmt

0f68423

lint

2374b32

test fix

e07904e

JesseAbram merged commit 9facf6e into master Sep 26, 2024
8 checks passed

JesseAbram deleted the add-signing-relayer branch September 26, 2024 19:56

JesseAbram mentioned this pull request Sep 26, 2024

Handle test key generation better #1071

Closed

HCastano added the Breaks API label Oct 3, 2024

	//! Curl example for `user/sign_tx`:
	//! Curl example for `user/relay_tx`:

	A user now sends their request to any validator that is not a siger. This will act as a realyer.
	A user now sends their request to any validator that is not a signer. This will act as a relayer.


		tracing::Span::current().record("request_author", signed_message.account_id().to_string());

		let signers = get_signers_from_chain(&api, &rpc).await?;

Add /relay_tx endpoint #1050

Add /relay_tx endpoint #1050

Conversation

JesseAbram commented Sep 12, 2024 • 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

ameba23 Sep 23, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano commented Sep 23, 2024

ameba23 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Add `/relay_tx` endpoint #1050

Add `/relay_tx` endpoint #1050

JesseAbram commented Sep 12, 2024 •

edited

Loading

ameba23 Sep 23, 2024 •

edited

Loading

ameba23 left a comment •

edited

Loading