chore: remove role from backup operator #296

kc1212 · 2025-11-26T08:58:33Z

Description of changes

One question/discussion, should we use bc2wrap for PublicSigKey like everywhere else in the code or we should use tfhe::safe_serialization? My understanding is that the custodian CLI is more of an independent component so it should be using more more "standard" formats, that's why I went with bc2wrap. But we can easily change it into tfhe::safe_serialization if we need to.

Issue ticket number and link

Closes zama-ai/kms-internal#2826

PR Checklist

I attest that all checked items are satisfied. Any deviation is clearly justified above.

Dependency Update Questionnaire (only if deps changed or added)

Answer in the Cargo.toml next to the dependency (or here if updating):

Ownership changes or suspicious concentration?
Low popularity?
Unusual version jump?
Lacking documentation?
Missing CI?
No security / disclosure policy?
Significant size increase?

More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md

github-actions · 2025-11-26T14:58:11Z

Consolidated Tests Results 2025-12-01 - 20:32:09

Test Results

25 passed

Details

25 tests
not captured
junit-to-ctrf
kind-testing test-reporter #194
chore: remove role from backup operator #296

test-reporter: Run #194

Tests 📝	Passed ✅	Failed ❌	Skipped ⏭️	Pending ⏳	Other ❓	Flaky 🍂	Duration ⏱️
25	25	0	0	0	0	0	not captured

🎉 All tests passed!

Tests

View All Tests

Test Name	Status	Duration
identifiers::tests::request_id_ordering	✅	3ms
identifiers::tests::test_id_type_conversions	✅	3ms
identifiers::tests::test_invalid_hex_length	✅	3ms
identifiers::tests::test_invalid_id_all_zeros	✅	3ms
identifiers::tests::test_invalid_v1_request_id	✅	3ms
identifiers::tests::test_key_id_from_str	✅	3ms
identifiers::tests::test_key_id_protobuf_conversion	✅	3ms
identifiers::tests::test_request_id_random	✅	3ms
identifiers::tests::test_v1_request_id_conversion	✅	3ms
identifiers::tests::test_v1_request_id_to_u128_conversion	✅	3ms
identifiers::tests::test_v1_request_id_with_prefix	✅	3ms
identifiers::tests::test_v1_request_id_with_whitespace	✅	3ms
identifiers::tests::test_valid_hex_characters	✅	3ms
rpc_types::tests::idempotent_plaintext	✅	3ms
rpc_types::tests::test_abi_encoding_fhevm	✅	3ms
rpc_types::tests::test_eip712_verification	✅	3ms
rpc_types::tests::test_enum_default	✅	3ms
rpc_types::tests::test_old_fhe_type_enum_compatibility	✅	3ms
rpc_types::tests::test_request_id	✅	3ms
rpc_types::tests::test_request_id_raw_string	✅	3ms
rpc_types::tests::test_types_plaintext_ser	✅	3ms
full_gen_tests_default_threshld_sequential_crs	✅	2m 20s
test_threshld_insecure	✅	3m 21s
full_gen_tests_default_centralzd_sequential_crs	✅	1.8s
test_centralzd_insecure	✅	1m 57s

_{🍂 No flaky tests in this run.}

Github Test Reporter by CTRF 💚

🔄 This comment has been updated

jot2re · 2025-11-27T17:03:02Z

One question/discussion, should we use bc2wrap for PublicSigKey like everywhere else in the code or we should use tfhe::safe_serialization? My understanding is that the custodian CLI is more of an independent component so it should be using more more "standard" formats, that's why I went with bc2wrap. But we can easily change it into tfhe::safe_serialization if we need to.

Actually bc2wrap does not really follow a standard format. For now I would try to avoid having diverging ways of serializing PublicSigKey. Still, what we should probably do is to move to safe_serialization like the rest of the files in the system, but then implement the legacy_serialization interface to allow to still use the old approach to avoid compatibility issues.

An alternative approach is to use a standard key format instead of bc2wrap or safe_serialization. Still, I don't feel that is the best approach since we serialize all the data we need with versioning and out own format everywhere else. Instead we should just make it possible to do a standard export.

kc1212 · 2025-11-28T09:08:58Z

Actually bc2wrap does not really follow a standard format. For now I would try to avoid having diverging ways of serializing PublicSigKey. Still, what we should probably do is to move to safe_serialization like the rest of the files in the system, but then implement the legacy_serialization interface to allow to still use the old approach to avoid compatibility issues.

by bc2wrap I mean the sec1 format, actually. since bincode doesn't add any metadata when serializing byte vectors

jot2re

Great work! Mainly some minor comments, in particular whether there are certain places where it makes sense to use a hashmap with the verification key as the key-value instead of an index.

I see you made a todo for backwards compatibility tests, are you continuing with making a PR for that?

core-client/src/lib.rs

jot2re · 2025-11-28T09:59:18Z

core-client/src/lib.rs

+
    let mut req_tasks = JoinSet::new();
-    for (core_idx, ce) in core_endpoints.iter() {
+    for (endpoint_idx, ce) in core_endpoints.iter() {


I am not sure this double loop is the best approach to handle this. Would it not be less fragile to instead have the core_endpoint be a hashmap of the verification key to the CoreServiceEndpointClient? Then it is possible to look up the end point directly for the key in a custodian_recovery_outputs

yes I think it would be better if we remove the party ID from the core-client code completely since going forward there will be no more unique, long-term praty IDs. but I think that will need to be in another PR as that's used everywhere, in all the other core-client functions at the moment

Great, I think so too! Can I get you to make an issue for this and link it here?

added the issue link in cd1a0ef

jot2re · 2025-11-28T10:17:03Z

core/service/src/backup/tests.rs

    engine::base::derive_request_id,
 };
 use aes_prng::AesRng;
+use alloy_primitives::Address;


I am wondering if we are still relying too much on arrays in this file, since now hashmaps with verification keys might make more sense in certain places

the verification key cannot be the key for the hashmap as it does not implement Ord. I wrote it in a comment here
https://github.com/zama-ai/kms/pull/296/files#diff-d494ecdbb919122886fc25291a0e4a910989bcd9355e234ad5e0d50c4c379fbcR593

in core/service/src/backup/tests.rs

we could implement Ord I suppose but these hashmaps are only used for managing our tests, so I was hesitant to add something in an important type like PublicSigKey that will only be used for testing

Ah, I missed that.
I don't think there is an issue in adding Ord as it is just additional functionality that does not change anything with the previous code.
Alternatively .verf_key_id() can be used to get a byte array that is a unique representation of the PublicSigKey. Maybe that is the best to use as index instead of the entire key?
TBH I think either approach is totally fine

updated it to use verf_key_id here e0541e6

jot2re · 2025-11-28T10:19:15Z

core/service/src/backup/tests.rs

 ) -> (
    BTreeMap<
-        Role,
+        Address,


Why is the address used here instead of the verification key itself like in other files?
I agree both work. But I might be more inclined to the verification key since then no changes will be needed in case we change the signing scheme

see #296 (comment)

I think in this case verf_key_id() would be more suitable as it is meant to be more general than the Address

changed it here e0541e6

jot2re · 2025-11-28T10:21:21Z

core/service/src/client/tests/threshold/custodian_backup_tests.rs

 async fn run_custodian_recovery_init(
    kms_clients: &HashMap<u32, CoreServiceEndpointClient<Channel>>,
-) -> Vec<RecoveryRequest> {
+) -> Vec<(u32, RecoveryRequest)> {


Why the use of role index and not verification key here? I think this leads back to a previous comment about storing kms_clients in a hash map keyed with the verification key instead of role index

answered here #296 (comment)

I had to add this u32 to link the request to the key used in the core-client map

jot2re · 2025-11-28T10:25:34Z

core/service/src/client/tests/threshold/custodian_backup_tests.rs

-    let backup_id = recovery_requests[0].backup_id.clone().unwrap();
-    let mut outputs_for_operators = HashMap::new();
+) -> HashMap<Address, (u32, CustodianRecoveryRequest)> {
+    use kms_grpc::kms::v1::CustodianRecoveryOutput;


Nitpick: could you move the import to the top of the file

handled it in e0541e6

jot2re · 2025-11-28T10:34:18Z

core/service/src/engine/backup_operator.rs

-                                cur_op_role,
-                            );
-                    parsed_custodian_rec.insert(cur_op_role, old_val);
+                match parsed_custodian_rec.entry(output.custodian_role) {


Nitpick: I think this can be done a bit simpler using or_insert

I think i prefer the current approach since it logs a weird behaviour

jot2re · 2025-11-28T10:34:41Z

core/service/src/engine/backup_operator.rs

        CustodianRecoveryOutput {
            custodian_role,
-            operator_role,
+            operator_verification_key: bc2wrap::serialize(&operator_verification_key).unwrap(),


Maybe just add a comment that this is a legacy approach

added reference to the issue about investigating key serialization in e0541e6

jot2re · 2025-11-28T10:39:16Z

core/service/tests/integration_test.rs

        root_path: &Path,
        threshold: usize,
-        operator_role: Role,
+        operator_id: usize, // not actual operator ID, just for managing where the files go


should it maybe be a string representing a dir instead?

this operator_id isn't used for anything other than specifying the path

I think it's better like this because this ID is used for the functions later on as we need to do a nested iteration over both operator ID and custodian ID. so it's cleaner for the directory structure when both IDs are in the path

jot2re · 2025-11-28T17:15:40Z

by bc2wrap I mean the sec1 format, actually. since bincode doesn't add any metadata when serializing byte vectors

I am a bit surprised about that, because I did some experiments with the different serializations we had for verification keys and I found the result of bc2wrap to be different from the to_sec1_bytes(). But maybe you mean bc2wrap directly on the byte vector from to_sec1_bytes() and not the PublicSigKey?

kc1212 · 2025-12-01T15:45:49Z

by bc2wrap I mean the sec1 format, actually. since bincode doesn't add any metadata when serializing byte vectors

I am a bit surprised about that, because I did some experiments with the different serializations we had for verification keys and I found the result of bc2wrap to be different from the to_sec1_bytes(). But maybe you mean bc2wrap directly on the byte vector from to_sec1_bytes() and not the PublicSigKey?

you're right that the serialization is different, it was my misunderstanding. we'll track this issue in zama-ai/kms-internal/issues/2836

kc1212 requested a review from a team as a code owner November 26, 2025 08:58

cla-bot bot added the cla-signed The CLA has been signed. label Nov 26, 2025

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch 4 times, most recently from e29ccd7 to 155b31b Compare November 26, 2025 14:57

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from 155b31b to ccfb057 Compare November 26, 2025 14:58

kc1212 self-assigned this Nov 26, 2025

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from ccfb057 to c7fb7ba Compare November 26, 2025 15:28

chore: remove role from backup operator

452d1f2

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from c7fb7ba to 452d1f2 Compare November 26, 2025 16:05

kc1212 added 2 commits November 27, 2025 13:53

chore: fix test

ed9d81d

chore: disable backward compatibility tests for backup

7808453

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from 7cd1c0a to 7808453 Compare November 27, 2025 13:14

chore: fix test

558c78b

chore: fix integration test

e4d74e3

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from 2b2a601 to e4d74e3 Compare November 28, 2025 09:12

jot2re requested changes Nov 28, 2025

View reviewed changes

kc1212 added 4 commits December 1, 2025 14:43

chore: fix off by one error

08106c8

Merge branch 'main' into kc1212/chore/2826/remove-backup-operator-role

0b1ff4d

chore: address initial comments

cd1a0ef

chore: use verf_key_id instead of address

e0541e6

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from 4b7e4dc to f9d1e3c Compare December 1, 2025 18:45

chore: clippy

aaa4f33

kc1212 force-pushed the kc1212/chore/2826/remove-backup-operator-role branch from f9d1e3c to aaa4f33 Compare December 1, 2025 19:28

chore: remove role from backup operator #296

Are you sure you want to change the base?

chore: remove role from backup operator #296

Uh oh!

Conversation

kc1212 commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue ticket number and link

PR Checklist

Dependency Update Questionnaire (only if deps changed or added)

Uh oh!

github-actions bot commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Consolidated Tests Results 2025-12-01 - 20:32:09

Test Results

Details

test-reporter: Run #194

🎉 All tests passed!

Tests

Uh oh!

jot2re commented Nov 27, 2025

Uh oh!

kc1212 commented Nov 28, 2025

Uh oh!

jot2re left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jot2re commented Nov 28, 2025

Uh oh!

kc1212 commented Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

kc1212 commented Nov 26, 2025 •

edited

Loading

github-actions bot commented Nov 26, 2025 •

edited

Loading