Skip to content

Feat: implement signature publisher decorators in signer #2468

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented May 6, 2025

Content

This PR includes the implementation of signature publisher decorators in the signer node, introducing a retrier publisher and a delayer publisher to facilitate future integration with a DMQ-based diffusion mechanism.

Additionally, signature publisher parameters have been made configurable to support execution in both end-to-end test and production environments. The default values are:
SIGNATURE_PUBLISHER_RETRY_ATTEMPTS: 3 attempts
SIGNATURE_PUBLISHER_RETRY_DELAY_MS: 2,000 milliseconds
SIGNATURE_PUBLISHER_DELAY_MS: 10,000 milliseconds

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
    • All check jobs of the CI have succeeded
    • 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)
    • Add ADR blog post or Dev ADR entry (if relevant)
    • No new TODOs introduced

Issue(s)

Closes #2461

@dlachaume dlachaume self-assigned this May 6, 2025
@dlachaume dlachaume changed the title Dlachaume/2461/signer signature publisher decorators Feat: implement signer signature publisher decorators May 6, 2025
@dlachaume dlachaume changed the title Feat: implement signer signature publisher decorators Feat: implement signature publisher decorators in signer May 6, 2025
Copy link

github-actions bot commented May 6, 2025

Test Results

    3 files  ±0     57 suites  ±0   11m 52s ⏱️ +6s
1 905 tests +9  1 905 ✅ +9  0 💤 ±0  0 ❌ ±0 
2 379 runs  +9  2 379 ✅ +9  0 💤 ±0  0 ❌ ±0 

Results for commit 2b40cb3. ± Comparison against base commit 346b447.

♻️ This comment has been updated with latest results.

@dlachaume dlachaume force-pushed the dlachaume/2461/signer-signature-publisher-decorators branch from 1d4e2d3 to d62bf6b Compare May 7, 2025 08:51
@dlachaume dlachaume temporarily deployed to testing-preview May 7, 2025 14:40 — with GitHub Actions Inactive
@dlachaume dlachaume force-pushed the dlachaume/2461/signer-signature-publisher-decorators branch 2 times, most recently from 91119b5 to 324823e Compare May 7, 2025 14:45
@dlachaume dlachaume marked this pull request as ready for review May 7, 2025 14:54
@dlachaume dlachaume requested review from jpraynaud and turmelclem May 7, 2025 14:54
@dlachaume dlachaume temporarily deployed to testing-preview May 7, 2025 14:54 — with GitHub Actions Inactive
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -259,3 +259,6 @@ Here is a list of the available parameters:
| `transactions_import_block_chunk_size` | - | - | `TRANSACTIONS_IMPORT_BLOCK_CHUNK_SIZE` | Chunk size for importing transactions, combined with transaction pruning it reduces the storage footprint of the signer by reducing the number of transactions stored on disk at any given time. | `1500` | - | - |
| `cardano_transactions_block_streamer_max_roll_forwards_per_poll` | - | - | `CARDANO_TRANSACTIONS_BLOCK_STREAMER_MAX_ROLL_FORWARDS_PER_POLL` | The maximum number of roll forwards during a poll of the block streamer when importing transactions. | `1000` | - | - |
| `preloading_refresh_interval_in_seconds` | `--preloading-refresh-interval-in-seconds` | - | `PRELOADING_REFRESH_INTERVAL_IN_SECONDS` | The preloading refresh interval in seconds. | `7200` | - | - |
| `signature_publisher_retry_attempts` | `--signature-publisher-retry-attempts` | - | `SIGNATURE_PUBLISHER_RETRY_ATTEMPTS` | Number of retry attempts when publishing the signature. | `3` | - | - |
| `signature_publisher_retry_delay_ms` | `--signature-publisher-retry-delay-ms` | - | `SIGNATURE_PUBLISHER_RETRY_DELAY_MS` | Delay (in milliseconds) between two retry attempts when publishing the signature. | `2000` | - | - |
| `signature_publisher_delay_ms` | `--signature-publisher-delay-ms` | - | `SIGNATURE_PUBLISHER_DELAY_MS` | Delay (in milliseconds) between two separate signature publishers being called. | `10000` | - | - |
Copy link
Member

Choose a reason for hiding this comment

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

The name is not specific enough, it would be more clear like this:

Suggested change
| `signature_publisher_delay_ms` | `--signature-publisher-delay-ms` | - | `SIGNATURE_PUBLISHER_DELAY_MS` | Delay (in milliseconds) between two separate signature publishers being called. | `10000` | - | - |
| `signature_publisher_delayer_delay_ms` | `--signature-publisher-delayer-delay-ms` | - | `SIGNATURE_PUBLISHER_DELAY_MS` | Delay (in milliseconds) between two separate publications done by the delayer signature publisher. | `10000` | - | - |

@@ -121,6 +121,15 @@ pub struct Configuration {

/// Preloading refresh interval in seconds
pub preloading_refresh_interval_in_seconds: u64,

/// Number of retry attempts when publishing the signature
pub signature_publisher_retry_attempts: usize,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is enough?

Suggested change
pub signature_publisher_retry_attempts: usize,
pub signature_publisher_retry_attempts: u8,

@@ -386,7 +387,30 @@ impl<'a> DependenciesBuilder<'a> {
self.root_logger(),
));

let signature_publisher: Arc<dyn SignaturePublisher> = aggregator_client.clone();
let signature_publisher = {
// Temporary no-op publisher used to prepare the delayer for a future DMQ-based diffusion.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Temporary no-op publisher used to prepare the delayer for a future DMQ-based diffusion.
// Temporary no-op publisher before a DMQ-based implementation is available.


use super::SignaturePublisher;

/// A decorator of [SignaturePublisher] that delays the publishing of signatures to a second publisher
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// A decorator of [SignaturePublisher] that delays the publishing of signatures to a second publisher
/// A decorator of [SignaturePublisher] that publishes right away on a first publisher and with a delay on the second publisher.

.publish(signed_entity_type, signature, protocol_message)
.await
{
warn!(self.logger, "Failed to publish signature: {e}");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warn!(self.logger, "Failed to publish signature: {e}");
error!(self.logger, "Delayer failed to publish first signature: {e}");


tokio::time::sleep(self.delay_between_publish).await;

self.second_publisher
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can also add an error log here?

Comment on lines 10 to 11
/// It is useful to prepare the coexistence phase of the DMQ and HTTP implementations,
/// with the introduction of delay logic.
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed IMO

Copy link
Member

Choose a reason for hiding this comment

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

The naming Upload should be replaced by Publish or similar in this module

@dlachaume dlachaume force-pushed the dlachaume/2461/signer-signature-publisher-decorators branch from 324823e to 2b40cb3 Compare May 7, 2025 16:45
@dlachaume dlachaume deployed to testing-preview May 7, 2025 16:54 — with GitHub Actions Active
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.

Implement delayer and retrier decorators of the signature publisher in signer
2 participants