Skip to content

Conversation

@fspreiss
Copy link
Contributor

@fspreiss fspreiss commented Dec 5, 2025

Adapts the crypto component so that the creation of basic (=Ed25519) signatures is done by directly calling the vault, rather than doing the obsolete indirection via the crypto service provider (CSP) layer.

With this PR, basic signature signature creation also no longer calculates a (private) key ID from the signing node's node signing public key stored in the registry, and thus also no longer reads this public key from the registry.

Also removes the old set of basic-signature unit tests and creates a fresh set of respective integration tests.

This also makes the BasicSigners signer: NodeId and registry_version: RegistryVersion parameters obsolete/unused, which will be removed in a follow-up PR.

The respective code for basic-signing in the CSP is kept for now, because it is still used in various tests for basic-sig verification in the CSP, and can/will be removed once we also go directly to the vault for basic-sig verification.

@fspreiss fspreiss marked this pull request as ready for review December 5, 2025 16:07
@fspreiss fspreiss requested a review from a team as a code owner December 5, 2025 16:07
@fspreiss
Copy link
Contributor Author

fspreiss commented Dec 5, 2025

@eichhorl, this PR makes the complaints::tests::test_load_transcript_failure_to_create_complaint_all_algorithms test fail. Not really sure why: could you maybe have a look what the reason could be? We can also have a look at it together next week.

@fspreiss
Copy link
Contributor Author

fspreiss commented Dec 7, 2025

@eichhorl, this PR makes the complaints::tests::test_load_transcript_failure_to_create_complaint_all_algorithms test fail. Not really sure why: could you maybe have a look what the reason could be? We can also have a look at it together next week.

@eichhorl, I found the reason. On the master branch, the test fails because of Cannot find public key registry record for node with ID [...]:

ubuntu@devenv-container:/ic$ bazel test //rs/consensus/idkg:idkg_test --test_output=all --test_arg=--nocapture --test_arg=test_load_transcript_failure_to_create_complaint_all_algorithms --cache_test_results=no
[...]
Dec 06 23:42:45.755 WARN s:/n:/ic_consensus_idkg/complaints Failed to sign complaint: transcript_id: IDkgTranscriptId { id: 4611471721867374420, source_subnet: aewsa-5mdg7-zkztu-zkzs7-yai, source_height: 6712731062086216666 }, dealer_id: 33gcm-vrdsm-dluvf-oxrja-wyxst-r3cs7-jxvgp-upb4l-22bat-f2d4a-kae, error = Cannot find public key registry record for node with ID 3jo2y-lqbaa-aaaaa-aaaap-2ai with purpose NodeSigning at registry version 8521824416584473026

With the changes in this PR, basic signature creation no longer involves reading the signing node's node-signing public key from the registry (because this public key is now just directly taken from the node's own public key store), so the signing of the complaint no longer fails and the complaint is actually created:

ubuntu@devenv-container:/ic$ bazel test //rs/consensus/idkg:idkg_test --test_output=all --test_arg=--nocapture --test_arg=test_load_transcript_failure_to_create_complaint_all_algorithms --cache_test_results=no
[...]
thread 'complaints::tests::test_load_transcript_failure_to_create_complaint_all_algorithms' panicked at rs/consensus/idkg/src/complaints.rs:2196:17:
assertion failed: `Complaints([Signed { content: IDkgComplaintContent { idkg_complaint: IDkgComplaint { transcript_id: IDkgTranscriptId { id: 14897931481665823492, source_subnet: 6dvey-rwzgr-ookif-t5mep-yai, source_height: 11047272312195686624 }, dealer_id: dc3rw-dqgby-2l2dp-sd6wr-o5rtk-ratbh-2s5a2-6ynp3-y34vw-dhwml-tqe} }, signature: BasicSignature { signature: BasicSig: 0xbc471e483e967138631e7841727f4aed3ec692cc4ba504490a815a97700fa6113aa0ef4de22c2d8cffb0b7ff0d13ea1841f39989641584292fd16542f7c2730f, signer: 3jo2y-lqbaa-aaaaa-aaaap-2ai } }])` does not match `TranscriptLoadStatus::Failure`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant