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

feat(aggregator): Aggregator Retry improvements #1425

Closed
wants to merge 39 commits into from

Conversation

PatStiles
Copy link
Contributor

Retry interface improvements:

closes #1401 #1414 #1391

Description

Description of the pull request changes and motivation.

Type of change

Please delete options that are not relevant.

  • Refactor

Checklist

  • Linked to Github Issue

@PatStiles PatStiles marked this pull request as draft November 14, 2024 18:15
@PatStiles PatStiles marked this pull request as ready for review November 19, 2024 13:03
Copy link
Contributor

@MauroToscano MauroToscano left a comment

Choose a reason for hiding this comment

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

DefaultRetryConfig may be confusing. We should use the same names as in the batcher, to split the chain and non chain constants

Copy link
Collaborator

@MarcosNicolau MarcosNicolau left a comment

Choose a reason for hiding this comment

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

Did the basics tests on macos and everything worked fine. Minor comments with respect to the fallbacks I need to check if the times match.

aggregator/pkg/server.go Outdated Show resolved Hide resolved
core/chainio/avs_writer.go Outdated Show resolved Hide resolved
core/chainio/avs_writer.go Outdated Show resolved Hide resolved
core/chainio/retryable.go Show resolved Hide resolved
core/chainio/retryable.go Show resolved Hide resolved
core/chainio/retryable.go Outdated Show resolved Hide resolved
core/chainio/retryable.go Outdated Show resolved Hide resolved
core/chainio/retryable.go Outdated Show resolved Hide resolved
@PatStiles
Copy link
Contributor Author

PatStiles commented Nov 19, 2024

DefaultRetryConfig may be confusing. We should use the same names as in the batcher, to split the chain and non chain constants

The Batcher uses the following naming scheme for the constants:

/// Ethereum calls retry constants
pub const ETHEREUM_CALL_MIN_RETRY_DELAY: u64 = 500; // milliseconds
pub const ETHEREUM_CALL_MAX_RETRIES: usize = 5;
pub const ETHEREUM_CALL_BACKOFF_FACTOR: f32 = 2.0;
pub const ETHEREUM_CALL_MAX_RETRY_DELAY: u64 = 3600; // seconds

/// Ethereum transaction retry constants
pub const BUMP_MIN_RETRY_DELAY: u64 = 500; // milliseconds
pub const BUMP_MAX_RETRIES: usize = 33; // ~ 1 day
pub const BUMP_BACKOFF_FACTOR: f32 = 2.0;
pub const BUMP_MAX_RETRY_DELAY: u64 = 3600; // seconds

Which indicate the parameters for performing an eth call vs eth transaction. In the operator/aggregator we are differentiating between on-chain vs not on-chain. @MauroToscano I'm not sure if there is overlap as the eth tx retry factors into bumping the tx fee. @avilagaston9 would welcome your thoughts.

@MauroToscano
Copy link
Contributor

DefaultRetryConfig may be confusing. We should use the same names as in the batcher, to split the chain and non chain constants

The Batcher uses the following naming scheme for the constants:

/// Ethereum calls retry constants
pub const ETHEREUM_CALL_MIN_RETRY_DELAY: u64 = 500; // milliseconds
pub const ETHEREUM_CALL_MAX_RETRIES: usize = 5;
pub const ETHEREUM_CALL_BACKOFF_FACTOR: f32 = 2.0;
pub const ETHEREUM_CALL_MAX_RETRY_DELAY: u64 = 3600; // seconds

/// Ethereum transaction retry constants
pub const BUMP_MIN_RETRY_DELAY: u64 = 500; // milliseconds
pub const BUMP_MAX_RETRIES: usize = 33; // ~ 1 day
pub const BUMP_BACKOFF_FACTOR: f32 = 2.0;
pub const BUMP_MAX_RETRY_DELAY: u64 = 3600; // seconds

Which indicate the parameters for performing an eth call vs eth transaction. In the operator/aggregator we are differentiating between on-chain vs not on-chain. @MauroToscano I'm not sure if there is overlap as the eth tx retry factors into bumping the tx fee. @avilagaston9 would welcome your thoughts.

We can use ETHEREUM_CALL as name for consistency if we don't have the other ones.

Default is the strangest one, It's not clear what a default is or why we need it

core/chainio/avs_writer.go Outdated Show resolved Hide resolved
core/retry_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@JulianVentura JulianVentura left a comment

Choose a reason for hiding this comment

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

Code looks fine, just a nit comment. I'll test later

core/chainio/retryable.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@JulianVentura JulianVentura left a comment

Choose a reason for hiding this comment

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

Seems to work fine locally, unit tests are passing too.

aggregator/pkg/aggregator.go Outdated Show resolved Hide resolved
@PatStiles PatStiles force-pushed the 1401-1414-aggregator-retry-improvements branch from 2bc53a2 to 4ecde3c Compare November 21, 2024 15:02
@MauroToscano
Copy link
Contributor

We will re do this PR in parts

@JuArce JuArce deleted the 1401-1414-aggregator-retry-improvements branch January 2, 2025 15:15
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.

5 participants