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.

Sign up for GitHub

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

Jump to bottom

fix(contracts): remove ExceededMaxRespondFee error #1407

Merged
merged 19 commits into from
Nov 19, 2024
Merged

fix(contracts): remove ExceededMaxRespondFee error #1407

merged 19 commits into from
Nov 19, 2024

Conversation

MarcosNicolau
Copy link
Collaborator

@MarcosNicolau MarcosNicolau commented Nov 12, 2024 •
edited
Loading

Description

Removes ExceededMaxRespondFee error in AlignedLayerServiceManager:

  • ExceededMaxRespondFee is removed from the Aligned contract and now the aggregator wallet will pay for the difference and the batcher will pay for the feeLimit he set. This case happens when the tx cost is higher than the respondToTaskFeeLimit.
  • Metrics were added for:
    • A count for the number of times the aggregator has paid for the difference.
    • An accumulated gas paid for the difference in eth.
Screenshot 2024-11-13 at 3 30 29 PM
  • Now sending respondToTask in the aggregator will return a permanent error if and only if either the aggregator or the batcher doesn't have enough funds in their wallets.

Testing

  1. Modify the bumpedGasPrice percentage to 10000 so that the txCost is bigger than the batch.RespondToTaskFeeLimit:
102: bumpedGasPrice := utils.CalculateGasPriceBumpBasedOnRetry(gasPrice, 10000, gasBumpIncrementalPercentage, I)
  1. Setup a local testnet as usual.
  2. Send a batch of proofs.
  3. Check with the metrics and logs that the aggregator is paying for the batcher.
  4. Check the Batcher and Aggregator balance before and after the aggregator sends the response, the contract should've charged him with batch.RespondToTaskFeeLimit:
    Get Agg balance before tx:
cast balance 0x15d34aaf54267db7d7c367839aaf71a00a2c6a65

Get batcher balance before transaction:
(Remember Batcher sends some funds when submitting task! so you must check the funds between the task submittion and the response)

cast call 0x851356ae760d987E095750cCeb3bC6014560891C "balanceOf(address)(uint256)" 0x7bc06c482dead17c0e297afbc32f6e63d3846650
  • Get balances after tx.
  • Aggregator should have decreased because he had to pay some of the tx:
  • The difference in Batcher should be equal to batch.RespondToTaskFeeLimit. You can identify the RespondToTaskFeeLimit value from the aggregator logs.

Another test:

  • you can undo the changes in the contract, re-deploy contracts in anvil, and check that the RespondToTask reverts.

Type of change

  • Bug fix

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

@MarcosNicolau MarcosNicolau marked this pull request as ready for review November 12, 2024 22:52
@MarcosNicolau MarcosNicolau self-assigned this Nov 13, 2024
@MarcosNicolau MarcosNicolau linked an issue Nov 13, 2024 that may be closed by this pull request
Aggregator gas fee should be able to be bumped indefinitely #1402
Open
@github-actions GitHub Actions
Copy link

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

Changes to gas cost

Generated at commit: 164b1007ac5208b6c35ca2ff23b62b3f5db16b20, compared to commit: 192a535aa39bbad8cada32b9460bf59e33081685

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
AlignedLayerServiceManager batchesState
receive		 +44 ❌
+1,858 ❌		 +6.46%
+4.14%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
AlignedLayerServiceManager 5,330,850 (+559,637) batchesState
createNewTask
disableVerifier
enableVerifier
isVerifierDisabled
receive
setDisabledVerifiers		 725 (+44)
56,109 (+2,174)
23,911 (+22)
23,803 (-67)
583 (+44)
23,318 (+2,149)
23,801 (+44)		 +6.46%
+4.03%
+0.09%
-0.28%
+8.16%
+10.15%
+0.19%		 725 (+44)
75,919 (+1,953)
35,504 (+22)
24,447 (-67)
1,249 (+44)
46,735 (+1,858)
34,808 (+44)		 +6.46%
+2.64%
+0.06%
-0.27%
+3.65%
+4.14%
+0.13%		 725 (+44)
76,219 (+2,168)
35,504 (+22)
24,447 (-67)
583 (+44)
47,202 (+2,138)
34,808 (+44)		 +6.46%
+2.93%
+0.06%
-0.27%
+8.16%
+4.74%
+0.13%		 725 (+44)
77,083 (+2,213)
47,097 (+22)
25,092 (-67)
2,583 (+44)
47,202 (+2,138)
45,815 (+44)		 +6.46%
+2.96%
+0.05%
-0.27%
+1.73%
+4.74%
+0.10%		 256 (0)
256 (0)
2 (0)
2 (0)
3 (0)
256 (0)
2 (0)

uri-99
uri-99 requested changes Nov 13, 2024
View reviewed changes
core/chainio/avs_writer.go Outdated Show resolved Hide resolved
Oppen
Oppen reviewed Nov 14, 2024
View reviewed changes
core/utils/eth_client_utils.go
@@ -51,6 +51,16 @@ func BytesToQuorumThresholdPercentages(quorumThresholdPercentagesBytes []byte) e
return quorumThresholdPercentages
}

func WeiToEth(wei *big.Int) float64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this is only used for metrics, can't the conversion be handled on the visualization side of those metrics?
This should also not be used for any other purpose other than metrics and logs, because this is a lossy conversion (floats are not good for handling money because of that).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, why don't we keep the big.Float? That might be lossless (but I'd need to check the docs).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it would be better to handle the conversion from grafana. I'll see if it is possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Oppen I agree no one likes floats, but Prometheus' Gauge accepts only float64 type

@MauroToscano MauroToscano added this pull request to the merge queue Nov 19, 2024
@MauroToscano MauroToscano removed this pull request from the merge queue due to a manual request Nov 19, 2024
@MauroToscano MauroToscano added this pull request to the merge queue Nov 19, 2024
Merged via the queue into staging with commit 698de75 Nov 19, 2024
7 checks passed
@MauroToscano MauroToscano deleted the 1402-aggregator-gas-fee-should-be-able-to-be-bumped-indefinitely branch November 19, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uri-99 uri-99 uri-99 approved these changes

@Oppen Oppen Oppen left review comments

@MauroToscano MauroToscano MauroToscano approved these changes

Assignees

@MarcosNicolau MarcosNicolau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aggregator gas fee should be able to be bumped indefinitely
4 participants
@MarcosNicolau @Oppen @MauroToscano @uri-99