Skip to content
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

Fix test for rejecting a TSS node who is not part of the signing committe #1028

Conversation

ameba23
Copy link
Contributor

@ameba23 ameba23 commented Aug 26, 2024

This is based off #1026 and fixes the test which checks that a TSS server who is not a member of the expected signing committee is unable to participate in the signing protocol.

This test was actually pretty broken before - and i am to blame. It was sending the signature request to one validator, and then attempting a bogus connection to another validator. Which correctly failed because they were not expecting a signing session at all. But what we want to test here is that when they are expecting a signing session, they will only allow members of the signing committee to connect.

@@ -186,8 +185,7 @@ async fn handle_initial_incoming_ws_message(
// Make the signing process fail, since one of the commitee has misbehaved
listeners.remove(&msg.session_id);
return Err(SubscribeErr::Decryption(
"Public key does not match that given in UserTransactionRequest or register \
transaction"
"Public key does not match any of those expected for this protocol session"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this error message and the comment above because they mentioned registration which no longer involves a protocol session.

for res in test_user_bad_connection_res {
assert_eq!(
res.unwrap().text().await.unwrap(),
"{\"Err\":\"Timed out waiting for remote party\"}"
"{\"Err\":\"Oneshot timeout error: channel closed\"}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what error we get here is not too important - the main thing is that the connection attempt fails in the assertion below.

@@ -780,20 +779,16 @@ async fn test_fails_to_sign_if_non_signing_group_participants_are_used() {
signature_request.signature_verifying_key = verifying_key.to_vec();

let test_user_bad_connection_res = submit_transaction_requests(
vec![validator_ips_and_keys[1].clone()],
vec![validator_ips_and_keys[0].clone()],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the bit that was wrong before. We should be sending the signature request to the same TS server that we attempt to make a connection to.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@HCastano HCastano merged commit a47156c into hc/update-registration-tests-on-tss Aug 26, 2024
8 checks passed
@HCastano HCastano deleted the peg/fix-non-signing-committee-participant-test branch August 26, 2024 16:29
HCastano added a commit that referenced this pull request Aug 26, 2024
* Remove signing integration test

The `register_and_sign` integration test covers the same functionality now.

* Update ETH integration test to use new registration flow

* Get program config test working again

* Delete `test_store_share` test

This isn't relevant in the context of registration anymore. The functionality
of this test was mostly moved to `test_jumpstart_network` anyways.

* Fix `test_fail_infinite_program`

* Fix `test_device_key_proxy` test

* Get `test_sign_tx_no_chain` mostly passing

* Move request limit tests to their own test

* Rename failing signing test

* Move a failing connection test out of `test_sign_tx_no_chain`

* Ignore failing test

Not sure if we still want this, or tbh how to fix it

* Update test name to better reflect what it covers

* Rename `generic_msg` to `signature_request`

This better reflects what the variable actually is

* Fix test for rejecting a TSS node who is not part of the signing committe (#1028)

* Update error message for unexpected participant

* Fix test

* Fix used import and variable warnings

* Use `test_client` for registration in all non-registration tests

* Rustfmt

* Move `update_programs` call to relevant location

---------

Co-authored-by: peg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants