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

chore: Remove default gas settings #10163

Merged
merged 5 commits into from
Nov 28, 2024
Merged

Conversation

spalladino
Copy link
Collaborator

@spalladino spalladino commented Nov 22, 2024

Deletes default fee in gas settings, and makes maxFeePerGas required for initializing a GasSettings instance.

Built on top of #10105.

@LHerskind LHerskind force-pushed the lh/9722 branch 7 times, most recently from 080c74e to efbd549 Compare November 26, 2024 09:31
@spalladino spalladino force-pushed the palla/kill-default-gas-settings branch from a95eab6 to d5ccff1 Compare November 26, 2024 15:13
@LHerskind LHerskind force-pushed the lh/9722 branch 2 times, most recently from 707f664 to 8c53565 Compare November 26, 2024 19:41
@spalladino spalladino force-pushed the palla/kill-default-gas-settings branch 2 times, most recently from d50e2f7 to d33df47 Compare November 26, 2024 19:57
@spalladino spalladino marked this pull request as ready for review November 26, 2024 19:59
@spalladino spalladino requested a review from LHerskind November 26, 2024 19:59
@spalladino
Copy link
Collaborator Author

@LHerskind this PR builds, but I'm waiting until your PR is green to start fighting the CI on this one. Feel free to start reviewing in the meantime if you want to.

@LHerskind
Copy link
Contributor

@LHerskind this PR builds, but I'm waiting until your PR is green to start fighting the CI on this one. Feel free to start reviewing in the meantime if you want to.

Understandable. The other is fighting ci on its own at the moment. Will take a look in the morning 🫡

@spalladino
Copy link
Collaborator Author

Will take a look in the morning

No rush at all. Thanks!

Base automatically changed from lh/9722 to master November 26, 2024 21:44
Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

Took a look. Will probably look again in the morning.

let gasSettings = defaultFeeOptions.gasSettings;
if (request.fee?.estimateGas) {
const feeForEstimation: FeeOptions = { paymentMethod, gasSettings };
const txRequest = await this.wallet.createTxExecutionRequest({ ...request, fee: feeForEstimation });
const simulationResult = await this.wallet.simulateTx(txRequest, true);
const { totalGas: gasLimits, teardownGas: teardownGasLimits } = getGasLimits(simulationResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

As part of #9868 I needed to estimate the gas exactly such that I could use it to check that the summing also was good. This lead me to pass along a padding argument to a couple of the functions since there was otherwise no way to get it into the getGasLimits. I added it to the SendMethodOptions to have it there, might be something to consider, as you right now cannot alter the padding that is applied.

In my case, I got semi rekt and spent way too long on trying to figure out why my estimations worked for most but not all the fee tests, until I figured out that it was that the estimate added 10% for the teardown that made the diff weird. i.e., refund of "logic" gas, meant that I was ~10% of the teardown off on estimation (~ because floating point 😬). Me an @just-mitch were talking about trying to get rid of some of the default values like that and pass things around to avoid cases where we cannot provide the options etc.

TL;DR: the default unsettable padding can be a bitch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. I took your estimateGasPad and moved it inside the new UserFeeOpts.

/** The gas settings */
gasSettings?: Partial<FieldsOf<GasSettings>>;
/** Whether to run an initial simulation of the tx with high gas limit to figure out actual gas settings. */
estimateGas?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to earlier comment, might make sense to have the padding in here 🤷? Also happy to move #9868 on top of this branch and then do it there if helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already built it into this one, no worries!

@@ -210,7 +210,7 @@ export function makeTxContext(seed: number = 1): TxContext {
* Creates a default instance of gas settings. No seed value is used to ensure we allocate a sensible amount of gas for testing.
*/
export function makeGasSettings() {
return GasSettings.default();
return GasSettings.default({ maxFeesPerGas: new GasFees(10, 10) });
Copy link
Contributor

Choose a reason for hiding this comment

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

This might make some tests explodes as soon as we start propagating and using these values more directly in part of #9868 (#9900)

protected async getDeploymentFunctionCalls(options: DeployOptions = {}): Promise<ExecutionRequestInit> {
protected async getDeploymentFunctionCalls(
options: DeployOptions = {},
): Promise<Pick<ExecutionRequestInit, 'calls' | 'authWitnesses' | 'packedArguments'>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just my typescript being bad, but why do you need the 'authWitness' | 'packedArguments' in here? Is it just to match with other things we might be "sending"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, where this is called from, we accumulate all calls along with their authwits and packed args.

@@ -34,7 +35,7 @@ export function makeBloatedProcessedTx({
db,
chainId = Fr.ZERO,
version = Fr.ZERO,
gasSettings = GasSettings.default(),
gasSettings = GasSettings.default({ maxFeesPerGas: new GasFees(10, 10) }),
Copy link
Contributor

Choose a reason for hiding this comment

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

This might make some tests explodes as soon as we start propagating and using these values more directly in part of #9868 (#9900)

@@ -126,7 +127,7 @@ export const mockTx = (
const noteEncryptedLogs = EncryptedNoteTxL2Logs.empty(); // Mock seems to have no new notes => no note logs
const encryptedLogs = hasLogs ? EncryptedTxL2Logs.random(2, 3) : EncryptedTxL2Logs.empty(); // 2 priv function invocations creating 3 encrypted logs each
const contractClassLog = hasLogs ? ContractClassTxL2Logs.random(1, 1) : ContractClassTxL2Logs.empty();
data.constants.txContext.gasSettings = GasSettings.default();
data.constants.txContext.gasSettings = GasSettings.default({ maxFeesPerGas: new GasFees(10, 10) });
Copy link
Contributor

Choose a reason for hiding this comment

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

his might make some tests explodes as soon as we start propagating and using these values more directly in part of #9868 (#9900)

@@ -135,6 +134,7 @@ export class FeesTest {
async ({ accountKeys }, { pxe, aztecNode, aztecNodeConfig }) => {
this.pxe = pxe;
this.aztecNode = aztecNode;
this.gasSettings = GasSettings.default({ maxFeesPerGas: await this.aztecNode.getCurrentBaseFees() });
Copy link
Contributor

Choose a reason for hiding this comment

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

One danger, related to the beforeEach of the others as well, is that congestion can make fees increase quickly so they could spike before some tests if many are run, leading to these gasSettings being stale and underpaying.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assumed that these tests sent few enough txs that prices would not spike. Would it be safer to set these fees to twice the current base fees, just in case?

@spalladino spalladino force-pushed the palla/kill-default-gas-settings branch from d33df47 to 7f3a096 Compare November 27, 2024 12:43
@spalladino
Copy link
Collaborator Author

@LHerskind addressed all comments but the ones on the hardcoded (10,10) fees. It's difficult to properly fix them without a big overhaul of our tests. Do you think we can set a high enough constant to get us past this?

@LHerskind
Copy link
Contributor

@LHerskind addressed all comments but the ones on the hardcoded (10,10) fees. It's difficult to properly fix them without a big overhaul of our tests. Do you think we can set a high enough constant to get us past this?

Ye think we can get away with that. The fee funder amount is huge (and easy to increase for all the test as once) so we can probably get away with even picking something that is much larger than would be sane 🤔 Since we are running from a fresh anvil etc, I think we can probably take the value from one of the test, and do a good ol' 10x or 100x of that value to have plenty leeway. The main place it could cause issues would be when there is teardowns as they would end up paying the inflated cost, but otherwise think that should cover it

@spalladino spalladino force-pushed the palla/kill-default-gas-settings branch from 8417560 to ecad2d1 Compare November 27, 2024 15:29
@spalladino spalladino enabled auto-merge (squash) November 27, 2024 15:48
Copy link
Contributor

Changes to public function bytecode sizes

Generated at commit: aba79ab4876c5f228bff633f1c1c0f9f8bc43e12, compared to commit: f28fcdb1e41aa353f0fdc2233ea66ae51ef745a4

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
AvmTest::bulk_testing +30 ❌ +0.13%
AvmTest::public_dispatch -9 ✅ -0.01%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
AvmTest::bulk_testing 23,104 (+30) +0.13%
AvmTest::public_dispatch 60,762 (-9) -0.01%

Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

🚢

@spalladino spalladino merged commit c9a4d88 into master Nov 28, 2024
69 checks passed
@spalladino spalladino deleted the palla/kill-default-gas-settings branch November 28, 2024 12:37
spalladino added a commit that referenced this pull request Nov 28, 2024
Fixes build after a bad merge from #10163.
spalladino added a commit that referenced this pull request Nov 28, 2024
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.

2 participants