Aggregator single signatures buffer #1934

Alenar · 2024-09-13T14:15:16Z

Content

This PR introduce a buffer mechanism in aggregator to make it able to store signatures that come from signers that are ahead of it, making easier to have signers that handle their signing rhythm themself are they won't need to rely on the aggregator certificate pending to issue single signatures.

When a aggregator receive a single signature it will now:

Try to register it as before.
If registration failed because an open message isn't open for the associated signed entity:
a. Check the signature against its embedded signed message and the current stake distribution.
b. If check fail check it using next epoch stake distribution (as the signer can already be at the next epoch when crossing epoch boundary).
c. If any of the two previous succeed store the signature in a store, a new table in our main sqlite file, alongside it's associated signed entity discriminant.

When a aggregator create a new open message it will now:

Check if for the new open message signed entity discriminant there's buffered signatures.
Try to register immediately the buffered signatures.
If a registration fails because of an invalid signature, skip it and continue to register the signatures (as there's an high chance that the failure is due to the signature being for another message with the same discriminant).
Delete successfully registered buffered signatures from the buffer store.

Pre-submit checklist

Branch
- Tests are provided (if possible)
- Crates versions are updated (if relevant)
- CHANGELOG file is updated (if relevant)
- Commit sequence broadly makes sense
- Key commits have useful messages
PR
- No clippy warnings in the CI
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested
Documentation
- Update README file (if relevant)
- Update documentation website (if relevant)
- Add dev blog post (if relevant)

Comments

Issue(s)

Closes #1900

mithril-common/src/entities/single_signatures.rs

github-actions · 2024-09-13T14:22:57Z

Test Results

4 files ± 0 54 suites +1 9m 39s ⏱️ -56s
1 290 tests +29 1 290 ✅ +29 0 💤 ±0 0 ❌ ±0
1 501 runs +29 1 501 ✅ +29 0 💤 ±0 0 ❌ ±0

Results for commit 5b578c7. ± Comparison against base commit 62dbe88.

This pull request removes 16 and adds 45 tests. Note that renamed tests count towards both.

mithril-aggregator ‑ http_server::routes::signatures_routes::tests::test_register_signatures_post_ok
mithril-aggregator ‑ services::certifier::tests::should_clean_epoch_when_inform_epoch
mithril-aggregator ‑ services::certifier::tests::should_create_certificate_when_multi_signature_produced
mithril-aggregator ‑ services::certifier::tests::should_mark_open_message_expired_when_exists
mithril-aggregator ‑ services::certifier::tests::should_not_create_certificate_for_open_message_already_certified
mithril-aggregator ‑ services::certifier::tests::should_not_create_certificate_for_open_message_not_created
mithril-aggregator ‑ services::certifier::tests::should_not_create_certificate_when_no_multi_signature_produced
mithril-aggregator ‑ services::certifier::tests::should_not_mark_open_message_expired_when_does_not_expire
mithril-aggregator ‑ services::certifier::tests::should_not_mark_open_message_expired_when_has_not_expired_yet
mithril-aggregator ‑ services::certifier::tests::should_not_register_invalid_single_signature
…

mithril-aggregator ‑ database::query::buffered_single_signature::delete_buffered_single_signature::tests::test_delete_buffered_single_signature_records_by_discriminant_and_party_ids
mithril-aggregator ‑ database::query::buffered_single_signature::get_buffered_single_signature::tests::test_get_all
mithril-aggregator ‑ database::query::buffered_single_signature::get_buffered_single_signature::tests::test_get_buffered_single_signature_records_by_discriminant
mithril-aggregator ‑ database::query::buffered_single_signature::insert_or_replace_buffered_single_signature::tests::allow_to_insert_record_with_different_party_id_and_discriminant_but_different_signature
mithril-aggregator ‑ database::query::buffered_single_signature::insert_or_replace_buffered_single_signature::tests::insert_records_in_empty_db
mithril-aggregator ‑ database::query::buffered_single_signature::insert_or_replace_buffered_single_signature::tests::inserting_record_with_same_party_id_and_discriminant_replace_previous_record
mithril-aggregator ‑ database::query::buffered_single_signature::insert_or_replace_buffered_single_signature::tests::inserting_same_record_twice_should_replace_first_insert
mithril-aggregator ‑ database::record::buffered_single_signature_record::tests::building_fake_generate_different_protocol_single_signature
mithril-aggregator ‑ database::record::buffered_single_signature_record::tests::test_convert_single_signatures
mithril-aggregator ‑ database::repository::buffered_single_signature_repository::tests::remove_buffered_signatures
…

♻️ This comment has been updated with latest results.

sfauvel

LGTM

...-aggregator/src/database/query/buffered_single_signature/delete_buffered_single_signature.rs

sfauvel · 2024-09-16T08:08:00Z

mithril-aggregator/src/http_server/routes/signatures_routes.rs

@@ -146,12 +147,45 @@ mod tests {
        .unwrap();
    }

+    #[tokio::test]
+    async fn test_register_signatures_post_ok_202() {


I wonder if it's a good thing to speak about code 202 in test name when we don't see it in the test even if It seems normal that it is like this

Yes, it's implicit since in the code and test we use enum variant with the explicit name of the http code.

Until now this is how we have named those tests has it has the benefit of clarity as long as you have some knowledge of the http code numbers. A requirement that can be though as reasonable since this whole part is an http api.

openapi.yaml

mithril-aggregator/src/services/certifier/buffered_certifier.rs

mithril-aggregator/src/multi_signer.rs

mithril-aggregator/src/services/certifier/certifier_service.rs

mithril-aggregator/src/services/certifier/interface.rs

mithril-aggregator/src/services/certifier/buffered_certifier.rs

mithril-aggregator/tests/create_certificate_with_buffered_signatures.rs

Alenar · 2024-09-16T09:22:45Z

mithril-aggregator/tests/test_extensions/runtime_tester.rs

+        // Todo: Refactor this code to avoid code duplication by making the signable_builder_service
+        // able to retrieve the next avk by itself.


We should open an issue to plan that refactor

jpraynaud

Overall looks good 👍

I'm a bit worried that signature entities without message will not be buffered.
I also left few comments and questions.

mithril-aggregator/src/database/repository/buffered_single_signature_repository.rs

mithril-aggregator/src/http_server/routes/signatures_routes.rs

mithril-aggregator/src/services/certifier/buffered_certifier.rs

mithril-aggregator/tests/create_certificate_with_buffered_signatures.rs

mithril-aggregator/tests/test_extensions/aggregator_observer.rs

mithril-common/src/entities/protocol_message.rs

mithril-common/src/entities/single_signatures.rs

By computing the Signed Entity Type and the protocol message without having to get the current open message. Required to be able to buffer incoming single signatures as an open message is not yet opened.

Since a certifier decorator will be added, this will avoid stacking too much files or too much the certifier file.

- `interface`: contains the trait & error definition - `certifier_service`: the implementation

Straigth to the point for now.

…ssage For the message type, the buffered signatures will be removed from the buffer.

If check using the current one fails, as a signer may detect epoch change before the aggregator and issue single signatures.

This allow caller to know if the signature was registered for immediate use or buffered.

In order to remove the InMemory store. Doing so required to be more precise with the tests signatures used. IE most"register_signature" tests needs a single signature that have a signed message but when reading them from the store the store doesn't keep the signed message leading to problems with some assert_eq.

As it has no more use.

It will be used to know if a single signature can be buffered or not.

Instead of validating their `signed_message` as this will be handled by another component beforehand.

As it has now no more use.

For retrocompatibility with previously stored signatures (with open messages in aggregator).

…ed_message is unauthenticated As there's no value to send it to the certifier since it will reject it anyway.

As we won't be able to always set this value (ie: with the upcoming p2p registration). In order to do so an refactor have to be done in `mithril-signer` so the `RegisterSignatureMessage::signed_message` property can be set.

And the trait method from `message_string` to `to_message`.

* Simpify some code docs. * Enhance `delete_buffered_single_signature` tests * Make some naming more clear

jpraynaud

LGTM 👍

I left minor comments below.

mithril-common/src/crypto_helper/tests_setup.rs

.../src/database/query/buffered_single_signature/insert_or_replace_buffered_single_signature.rs

mithril-common/src/entities/single_signatures.rs

mithril-aggregator/src/tools/single_signature_authenticator.rs

mithril-aggregator/src/dependency_injection/builder.rs

mithril-aggregator/src/http_server/routes/signatures_routes.rs

mithril-aggregator/src/services/certifier/interface.rs

* Mithril-aggregator from `0.5.63` to `0.5.64` * Mithril-signer from `0.2.183` to `0.2.184` * Mithril-common from `0.4.51` to `0.4.52` * OpenApi from `0.1.29` to `0.1.30`

Alenar requested review from sfauvel, jpraynaud and dlachaume September 13, 2024 14:15

github-advanced-security bot found potential problems Sep 13, 2024

View reviewed changes

mithril-common/src/entities/single_signatures.rs Fixed Show fixed Hide fixed

Alenar temporarily deployed to testing-preview September 13, 2024 14:23 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 13, 2024 14:23 — with GitHub Actions Inactive

Alenar force-pushed the ensemble/1900/aggregator_signatures_buffer branch from 43f9ace to d7e9138 Compare September 13, 2024 16:25

Alenar temporarily deployed to testing-preview September 13, 2024 16:33 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 13, 2024 16:33 — with GitHub Actions Inactive

sfauvel approved these changes Sep 16, 2024

View reviewed changes

dlachaume approved these changes Sep 16, 2024

View reviewed changes

Alenar commented Sep 16, 2024

View reviewed changes

jpraynaud reviewed Sep 16, 2024

View reviewed changes

Alenar temporarily deployed to testing-preview September 17, 2024 10:44 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 17, 2024 10:44 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-preview September 17, 2024 15:50 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 17, 2024 15:50 — with GitHub Actions Inactive

Alenar force-pushed the ensemble/1900/aggregator_signatures_buffer branch 2 times, most recently from 335512b to b308cc1 Compare September 18, 2024 15:42

Alenar temporarily deployed to testing-preview September 18, 2024 15:52 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 18, 2024 15:52 — with GitHub Actions Inactive

Alenar added 8 commits September 19, 2024 10:32

Scaffold integration test for signing with buffered signatures

e02968d

Uncouple signatures registration from certifier in integration tests

b038620

By computing the Signed Entity Type and the protocol message without having to get the current open message. Required to be able to buffer incoming single signatures as an open message is not yet opened.

Promote aggregator certifier module to directory

33bd027

Since a certifier decorator will be added, this will avoid stacking too much files or too much the certifier file.

Rearrange certifier module

047e26b

- `interface`: contains the trait & error definition - `certifier_service`: the implementation

Scaffold BufferedCertifierService decorator

10af4a0

Define a store for buffered signatures with an in-memory impl

c6b0d45

Straigth to the point for now.

Make buffer certifier buffer signature if no open message is available

4c203a5

Make buffered certifier add buffered signatures when opening a new me…

fa55f44

…ssage For the message type, the buffered signatures will be removed from the buffer.

Alenar added 16 commits September 19, 2024 10:32

Check signature against message using next stake distribution

c7f0878

If check using the current one fails, as a signer may detect epoch change before the aggregator and issue single signatures.

Add a status to certifier signature registration

a903d92

This allow caller to know if the signature was registered for immediate use or buffered.

Use sqlite buffer store everywhere and remove in memory version

34a7219

As it has no more use.

Add authentication_status to SingleSignatures entity

76ffeae

It will be used to know if a single signature can be buffered or not.

Buffer signature if they're authenticated

a9ba1ff

Instead of validating their `signed_message` as this will be handled by another component beforehand.

Remove multi_signer dependency from buffered certifier service

c859a5a

As it has now no more use.

Skip (de)serialization of single signature authentication_status

c19f497

For retrocompatibility with previously stored signatures (with open messages in aggregator).

Make buffer integration test send authenticated signatures

5d81f67

Add SingleSignatureAuthenticator in aggregator

ebdea24

Wire SingleSignatureAuthenticator in aggregator dependencies

f51ac9a

Make aggregator register_signatures route check signature authenticity

2d6e9eb

Aggregator /register-signatures return 404 when signature with sign…

4c32c7b

…ed_message is unauthenticated As there's no value to send it to the certifier since it will reject it anyway.

Remove signed_message from SingleSignatures entity

eeb23cf

As we won't be able to always set this value (ie: with the upcoming p2p registration). In order to do so an refactor have to be done in `mithril-signer` so the `RegisterSignatureMessage::signed_message` property can be set.

refactor: rename AsMessage to ToMessage

d06bb6e

And the trait method from `message_string` to `to_message`.

refactor+style: Implement changes asked in PR reviews

ad4d1db

* Simpify some code docs. * Enhance `delete_buffered_single_signature` tests * Make some naming more clear

Alenar force-pushed the ensemble/1900/aggregator_signatures_buffer branch from b308cc1 to ad4d1db Compare September 19, 2024 08:56

Alenar temporarily deployed to testing-preview September 19, 2024 09:05 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 19, 2024 09:05 — with GitHub Actions Inactive

jpraynaud approved these changes Sep 19, 2024

View reviewed changes

dlachaume approved these changes Sep 19, 2024

View reviewed changes

mithril-aggregator/src/services/certifier/interface.rs Outdated Show resolved Hide resolved

Alenar added 2 commits September 19, 2024 12:27

style: naming & comments adjustements from pr reviews

90f7c20

chore: bump crates & openapi versions

f62785c

* Mithril-aggregator from `0.5.63` to `0.5.64` * Mithril-signer from `0.2.183` to `0.2.184` * Mithril-common from `0.4.51` to `0.4.52` * OpenApi from `0.1.29` to `0.1.30`

Alenar temporarily deployed to testing-preview September 19, 2024 10:41 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 19, 2024 10:41 — with GitHub Actions Inactive

docs: update CHANGELOG

5b578c7

Alenar temporarily deployed to testing-preview September 19, 2024 13:11 — with GitHub Actions Inactive

Alenar temporarily deployed to testing-sanchonet September 19, 2024 13:11 — with GitHub Actions Inactive

Alenar merged commit 27fdfa0 into main Sep 19, 2024
39 of 40 checks passed

Alenar deleted the ensemble/1900/aggregator_signatures_buffer branch September 19, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregator single signatures buffer #1934

Aggregator single signatures buffer #1934

Alenar commented Sep 13, 2024 •

edited

Loading

github-actions bot commented Sep 13, 2024 •

edited

Loading

sfauvel left a comment

sfauvel Sep 16, 2024

Alenar Sep 19, 2024

Alenar Sep 16, 2024

jpraynaud left a comment

jpraynaud left a comment

		// Todo: Refactor this code to avoid code duplication by making the signable_builder_service
		// able to retrieve the next avk by itself.

Aggregator single signatures buffer #1934

Aggregator single signatures buffer #1934

Conversation

Alenar commented Sep 13, 2024 • edited Loading

Content

Pre-submit checklist

Comments

Issue(s)

github-actions bot commented Sep 13, 2024 • edited Loading

Test Results

sfauvel left a comment

Choose a reason for hiding this comment

sfauvel Sep 16, 2024

Choose a reason for hiding this comment

Alenar Sep 19, 2024

Choose a reason for hiding this comment

Alenar Sep 16, 2024

Choose a reason for hiding this comment

jpraynaud left a comment

Choose a reason for hiding this comment

jpraynaud left a comment

Choose a reason for hiding this comment

Alenar commented Sep 13, 2024 •

edited

Loading

github-actions bot commented Sep 13, 2024 •

edited

Loading