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 bump fee if transaction was not included #1286

Conversation

MarcosNicolau
Copy link
Collaborator

@MarcosNicolau MarcosNicolau commented Oct 17, 2024

Before merging this pr, pr #1304 should be merged first, as it depends on the retry logic.

Changes

Bumps the gasPrice in the aggregator when sending a responded task:

  • After a timeout period the aggregator will send a new ReponseToTask with a higher bumped gas price.
  • Configurations fields have been added to easily change the behavior:
    • gas_base_bump_percentage: how much to bump the gas price. Configured to 10%
    • gas_bump_incremental_percentage: an extra percentage to bump every retry i*5. Configured to 2%
    • time_to_wait_before_bump: the time to wait for the receipt once we sent the tx. Configured to 36s
  • Traces for tasks: a new endpoint in the telemetry to log when the gas price is bumped
Screenshot 2024-11-11 at 11 34 02 AM
  • Time series in grafana: shows the number of times the gas was bumped when sending task response across time.
image

Considerations:

For every retry, the gas price is bumped a 10% + 2% * i, where i is the iteration number. i is only incremented when the receipt timeout has passed. That is, if the rpc node fails the fee i won't be incremented. This will run infinitely until the transaction gets accepted unless the checkRespondToTaskFeeLimit fails. In that case, we stop retrying and leave the tx in the mempool. Issue #1389 has been created to cover this case.

Another note is that waitForTransactionReceipt has been refactored. Now it does not have a maxTries anymore (set to 0) and instead receives a new parameter waitTimeout which defines the retries based on a timeout. Also, the maxInterval has been modified to 2 seconds because we can't reliably measure the specific time the tx will be included in a block. Setting a higher value will imply doing fewer retries across the waitTimeout so we might lose the receipt.

Test

Setup a local testnet as usual and introduce the following lines in the avs_writer.go file:

125:   if i < 2 {
126:	    i++
127:	    return nil, fmt.Errorf("error")
130:   }

You should check through the aggregator logs, telemetry traces, and grafana charts, that the gas price was indeed bumped.

@MarcosNicolau MarcosNicolau mentioned this pull request Oct 18, 2024
10 tasks
@MarcosNicolau MarcosNicolau linked an issue Oct 21, 2024 that may be closed by this pull request
2 tasks
@MarcosNicolau MarcosNicolau marked this pull request as ready for review October 21, 2024 20:29
@MarcosNicolau MarcosNicolau self-assigned this Oct 21, 2024
core/connection.go Outdated Show resolved Hide resolved
core/connection.go Outdated Show resolved Hide resolved
core/connection.go Outdated Show resolved Hide resolved
core/connection.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.

Looks good and works on my machine

Base automatically changed from feat/aggregator_retries to staging November 12, 2024 03:49
@uri-99
Copy link
Contributor

uri-99 commented Nov 12, 2024

There is something odd with the bumper, i added the lines of code in the description to make it fail, it bumped correctly but then skyrocketed on because it had no delay?
image

@MarcosNicolau
Copy link
Collaborator Author

There is something odd with the bumper, i added the lines of code in the description to make it fail, it bumped correctly but then skyrocketed on because it had no delay?

I tried to reproduce the behavior but I couldn't. Would you try again with the latest changes pls.

Comment on lines +75 to +77
// Sends AggregatedResponse and waits for the receipt for three blocks, if not received
// it will try again bumping the last tx gas price based on `CalculateGasPriceBump`
// This process happens indefinitely until the transaction is included.
Copy link
Collaborator

@avilagaston9 avilagaston9 Nov 13, 2024

Choose a reason for hiding this comment

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

Maybe for a separate PR, but we need to add some numbers to graph the retry logic. See:

pub async fn cancel_create_new_task_tx(&self, old_tx_gas_price: U256) {

core/utils/eth_client_utils.go Outdated Show resolved Hide resolved
core/utils/eth_client_utils.go Outdated Show resolved Hide resolved
core/utils/eth_client_utils.go Outdated Show resolved Hide resolved
aggregator/pkg/aggregator.go Show resolved Hide resolved
aggregator/pkg/aggregator.go Outdated Show resolved Hide resolved
// the currentGasPrice, a base bump percentage, a retry percentage, and the retry count.
// Formula: currentGasPrice + (currentGasPrice * (baseBumpPercentage + retryCount * incrementalRetryPercentage) / 100)
func CalculateGasPriceBumpBasedOnRetry(currentGasPrice *big.Int, baseBumpPercentage uint, retryAttemptPercentage uint, retryCount int) *big.Int {
// Incremental percentage increase for each retry attempt (i*5%)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Incremental percentage increase for each retry attempt (i*5%)
// Incremental percentage increase for each retry attempt (i*2%)

Copy link
Collaborator

@avilagaston9 avilagaston9 left a comment

Choose a reason for hiding this comment

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

LGTM! Left some nit comments

txOpts.GasPrice = bumpedGasPrice
} else {
// bump the last tx gas price a little by `gasBumpIncrementalPercentage` to replace it.
txOpts.GasPrice = utils.CalculateGasPriceBumpBasedOnRetry(txOpts.GasPrice, gasBumpIncrementalPercentage, 0, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add comments explaining how we are using this function. We are using different bump percentages depending on which price that we are bumping.

@MauroToscano MauroToscano added this pull request to the merge queue Nov 13, 2024
Merged via the queue into staging with commit 44339f7 Nov 13, 2024
4 checks passed
@MauroToscano MauroToscano deleted the 311-aggregator-wait-for-receipt-for-1-minute-if-not-bump-the-fee-v2 branch November 13, 2024 17:23
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.

( aggregator ) Bump fees
8 participants