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: fee foresight support #10262

Merged
merged 3 commits into from
Nov 28, 2024
Merged

feat: fee foresight support #10262

merged 3 commits into from
Nov 28, 2024

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Nov 27, 2024

Fixes #10261.

Allow passing a timestamp for foresight. Also updates the getCurrentBaseFees to really return the fees that will be used at the next block. Essentially, what you currently see as needed to be included. I think keeping the name as is, is simpler to understand, as it is what you want to use if you are sending the tx "currently".

To see tests related to this, we have updated test_FeeModelEquivalence() such that it is predicting the fee up front and checking that it match (showcase foresight). And a separate test test__FeeModelPrune which will compute the fees with and without a future prune, showing that the foresight also considers the pruning.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@LHerskind LHerskind marked this pull request as ready for review November 27, 2024 22:04
@LHerskind LHerskind requested a review from spalladino November 27, 2024 22:10
public async getCurrentBaseFees(): Promise<GasFees> {
return new GasFees(Fr.ZERO, new Fr(await this.rollupContract.read.getManaBaseFee([true])));
const timestamp = BigInt((await this.publicClient.getBlock()).timestamp + BigInt(this.ethereumSlotDuration));
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
const timestamp = BigInt((await this.publicClient.getBlock()).timestamp + BigInt(this.ethereumSlotDuration));
const timestamp = BigInt((await this.publicClient.getBlock()).timestamp + BigInt(this.aztecSlotDuration));

Shouldn't we be moving to the next Aztec slot, as opposed to the next Ethereum one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If using the aztecSlotDuration then if you are "in" a slot that don't have a block, you will be using the timestamp of the next slot, even if the next block could be within this slot.

Neither of them is really "perfect" for this, it should probably look at the next possible block, e.g., could be the current slot or the next if the current is already filled etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made an update I think should handle it better. Now fetching the last block, and ensuring that we are at least at the next block from that. Ensuring that we won't get the fee for the already published block, but for the next possible. Only needed for the end-user, as some of these checks are already done earlier in the run from sequencing.

Copy link
Contributor

github-actions bot commented Nov 28, 2024

Changes to circuit sizes

Generated at commit: 272fa983efdaed9e7b7d82e20415fff2718f5387, compared to commit: 9440c1cf3834eea380014d55eef6e81cff8ffee8

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_reset_4_4_4_4_4_4_4_4_1 +4,719 ❌ +14.57% +19,236 ❌ +22.24%
private_kernel_reset +5,633 ❌ +6.70% +19,440 ❌ +3.15%
rollup_block_root -168 ✅ -3.74% -144 ✅ -0.01%
parity_root -252 ✅ -5.01% -216 ✅ -0.01%
rollup_merge -168 ✅ -4.91% -144 ✅ -0.01%
rollup_block_merge -465 ✅ -3.87% -451 ✅ -0.02%
rollup_root -465 ✅ -3.88% -451 ✅ -0.02%
private_kernel_tail -6 ✅ -0.15% -6 ✅ -0.05%
parity_base -252 ✅ -5.86% -216 ✅ -0.70%
rollup_base_private -28,501 ✅ -14.42% -41,353 ✅ -1.56%
private_kernel_inner -785 ✅ -2.09% -1,034 ✅ -1.82%
private_kernel_init -741 ✅ -3.46% -986 ✅ -2.86%
rollup_base_public -91,453 ✅ -20.45% -129,514 ✅ -3.24%
private_kernel_tail_to_public -2,133 ✅ -12.27% -3,044 ✅ -10.32%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_reset_4_4_4_4_4_4_4_4_1 37,106 (+4,719) +14.57% 105,730 (+19,236) +22.24%
private_kernel_reset 89,662 (+5,633) +6.70% 637,479 (+19,440) +3.15%
rollup_block_root 4,322 (-168) -3.74% 2,739,521 (-144) -0.01%
parity_root 4,782 (-252) -5.01% 3,636,536 (-216) -0.01%
rollup_merge 3,252 (-168) -4.91% 1,826,915 (-144) -0.01%
rollup_block_merge 11,544 (-465) -3.87% 1,858,073 (-451) -0.02%
rollup_root 11,528 (-465) -3.88% 1,858,059 (-451) -0.02%
private_kernel_tail 4,127 (-6) -0.15% 12,509 (-6) -0.05%
parity_base 4,049 (-252) -5.86% 30,482 (-216) -0.70%
rollup_base_private 169,174 (-28,501) -14.42% 2,617,309 (-41,353) -1.56%
private_kernel_inner 36,767 (-785) -2.09% 55,848 (-1,034) -1.82%
private_kernel_init 20,668 (-741) -3.46% 33,520 (-986) -2.86%
rollup_base_public 355,674 (-91,453) -20.45% 3,867,937 (-129,514) -3.24%
private_kernel_tail_to_public 15,253 (-2,133) -12.27% 26,461 (-3,044) -10.32%

@LHerskind
Copy link
Contributor Author

Updated the artifact hash since it had changed after the noir sync

@LHerskind LHerskind merged commit 9e19244 into master Nov 28, 2024
68 checks passed
@LHerskind LHerskind deleted the lh/10261 branch November 28, 2024 10:41
ludamad pushed a commit that referenced this pull request Nov 28, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.65.2</summary>

##
[0.65.2](aztec-package-v0.65.1...aztec-package-v0.65.2)
(2024-11-28)


### Features

* New proving broker
([#10174](#10174))
([6fd5fc1](6fd5fc1))
</details>

<details><summary>barretenberg.js: 0.65.2</summary>

##
[0.65.2](barretenberg.js-v0.65.1...barretenberg.js-v0.65.2)
(2024-11-28)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>aztec-packages: 0.65.2</summary>

##
[0.65.2](aztec-packages-v0.65.1...aztec-packages-v0.65.2)
(2024-11-28)


### Features

* Fee foresight support
([#10262](#10262))
([9e19244](9e19244))
* New proving broker
([#10174](#10174))
([6fd5fc1](6fd5fc1))
* Sequential insertion in indexed trees
([#10111](#10111))
([bfd9fa6](bfd9fa6))
* Swap polys to facilitate dynamic trace overflow
([#9976](#9976))
([b7b282c](b7b282c))


### Bug Fixes

* Don't store indices of zero leaves.
([#10270](#10270))
([c22be8b](c22be8b))
* Expect proper duplicate nullifier error patterns in e2e tests
([#10256](#10256))
([4ee8344](4ee8344))


### Miscellaneous

* Check artifact consistency
([#10271](#10271))
([6a49405](6a49405))
* Dont import things that themselves import jest in imported functions
([#10260](#10260))
([9440c1c](9440c1c))
* Fix bad merge in integration l1 publisher
([#10272](#10272))
([b5a6aa4](b5a6aa4))
* Fixing sol warnings
([#10276](#10276))
([3d113b2](3d113b2))
* Pull out sync changes
([#10274](#10274))
([391a6b7](391a6b7))
* Pull value merger code from sync
([#10080](#10080))
([3392629](3392629))
* Remove default gas settings
([#10163](#10163))
([c9a4d88](c9a4d88))
* Replace relative paths to noir-protocol-circuits
([654d801](654d801))
* Teardown context in prover coordination test
([#10257](#10257))
([7ea3888](7ea3888))
</details>

<details><summary>barretenberg: 0.65.2</summary>

##
[0.65.2](barretenberg-v0.65.1...barretenberg-v0.65.2)
(2024-11-28)


### Features

* Sequential insertion in indexed trees
([#10111](#10111))
([bfd9fa6](bfd9fa6))
* Swap polys to facilitate dynamic trace overflow
([#9976](#9976))
([b7b282c](b7b282c))


### Bug Fixes

* Don't store indices of zero leaves.
([#10270](#10270))
([c22be8b](c22be8b))


### Miscellaneous

* Pull value merger code from sync
([#10080](#10080))
([3392629](3392629))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Nov 29, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.65.2</summary>

##
[0.65.2](AztecProtocol/aztec-packages@aztec-package-v0.65.1...aztec-package-v0.65.2)
(2024-11-28)


### Features

* New proving broker
([#10174](AztecProtocol/aztec-packages#10174))
([6fd5fc1](AztecProtocol/aztec-packages@6fd5fc1))
</details>

<details><summary>barretenberg.js: 0.65.2</summary>

##
[0.65.2](AztecProtocol/aztec-packages@barretenberg.js-v0.65.1...barretenberg.js-v0.65.2)
(2024-11-28)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>aztec-packages: 0.65.2</summary>

##
[0.65.2](AztecProtocol/aztec-packages@aztec-packages-v0.65.1...aztec-packages-v0.65.2)
(2024-11-28)


### Features

* Fee foresight support
([#10262](AztecProtocol/aztec-packages#10262))
([9e19244](AztecProtocol/aztec-packages@9e19244))
* New proving broker
([#10174](AztecProtocol/aztec-packages#10174))
([6fd5fc1](AztecProtocol/aztec-packages@6fd5fc1))
* Sequential insertion in indexed trees
([#10111](AztecProtocol/aztec-packages#10111))
([bfd9fa6](AztecProtocol/aztec-packages@bfd9fa6))
* Swap polys to facilitate dynamic trace overflow
([#9976](AztecProtocol/aztec-packages#9976))
([b7b282c](AztecProtocol/aztec-packages@b7b282c))


### Bug Fixes

* Don't store indices of zero leaves.
([#10270](AztecProtocol/aztec-packages#10270))
([c22be8b](AztecProtocol/aztec-packages@c22be8b))
* Expect proper duplicate nullifier error patterns in e2e tests
([#10256](AztecProtocol/aztec-packages#10256))
([4ee8344](AztecProtocol/aztec-packages@4ee8344))


### Miscellaneous

* Check artifact consistency
([#10271](AztecProtocol/aztec-packages#10271))
([6a49405](AztecProtocol/aztec-packages@6a49405))
* Dont import things that themselves import jest in imported functions
([#10260](AztecProtocol/aztec-packages#10260))
([9440c1c](AztecProtocol/aztec-packages@9440c1c))
* Fix bad merge in integration l1 publisher
([#10272](AztecProtocol/aztec-packages#10272))
([b5a6aa4](AztecProtocol/aztec-packages@b5a6aa4))
* Fixing sol warnings
([#10276](AztecProtocol/aztec-packages#10276))
([3d113b2](AztecProtocol/aztec-packages@3d113b2))
* Pull out sync changes
([#10274](AztecProtocol/aztec-packages#10274))
([391a6b7](AztecProtocol/aztec-packages@391a6b7))
* Pull value merger code from sync
([#10080](AztecProtocol/aztec-packages#10080))
([3392629](AztecProtocol/aztec-packages@3392629))
* Remove default gas settings
([#10163](AztecProtocol/aztec-packages#10163))
([c9a4d88](AztecProtocol/aztec-packages@c9a4d88))
* Replace relative paths to noir-protocol-circuits
([654d801](AztecProtocol/aztec-packages@654d801))
* Teardown context in prover coordination test
([#10257](AztecProtocol/aztec-packages#10257))
([7ea3888](AztecProtocol/aztec-packages@7ea3888))
</details>

<details><summary>barretenberg: 0.65.2</summary>

##
[0.65.2](AztecProtocol/aztec-packages@barretenberg-v0.65.1...barretenberg-v0.65.2)
(2024-11-28)


### Features

* Sequential insertion in indexed trees
([#10111](AztecProtocol/aztec-packages#10111))
([bfd9fa6](AztecProtocol/aztec-packages@bfd9fa6))
* Swap polys to facilitate dynamic trace overflow
([#9976](AztecProtocol/aztec-packages#9976))
([b7b282c](AztecProtocol/aztec-packages@b7b282c))


### Bug Fixes

* Don't store indices of zero leaves.
([#10270](AztecProtocol/aztec-packages#10270))
([c22be8b](AztecProtocol/aztec-packages@c22be8b))


### Miscellaneous

* Pull value merger code from sync
([#10080](AztecProtocol/aztec-packages#10080))
([3392629](AztecProtocol/aztec-packages@3392629))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

bug: fee estimation don't have foresight
2 participants