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

fix: blob fees #11029

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
8484168
fix: bump blob fees on retries
spypsy Jan 3, 2025
eb311d5
PR fixes
spypsy Jan 6, 2025
86b6693
Merge branch 'master' into spy/tx-utils-blobs
spypsy Jan 6, 2025
53b354e
Merge branch 'master' into spy/tx-utils-blobs
spypsy Jan 6, 2025
ce614fe
Try info logging to get blob fees
spypsy Jan 6, 2025
e7b3f71
log existing image in bootstrap
spypsy Jan 6, 2025
aa4e4a9
actually use calculated maxFeePerBlobGas
spypsy Jan 6, 2025
dc303b4
Merge branch 'master' into spy/tx-utils-blobs
spypsy Jan 6, 2025
258a904
min bump percentage is 100 for ALL fees
spypsy Jan 6, 2025
9981f97
Merge branch 'master' into spy/tx-utils-blobs
spypsy Jan 7, 2025
36b6582
max gwei for blob fees
spypsy Jan 7, 2025
6e9e8ac
log maxFeePerBlobGas
spypsy Jan 7, 2025
83c27f9
add metadata on speed-up attempts
spypsy Jan 7, 2025
c286d82
fix conversion
spypsy Jan 7, 2025
39de22e
add some better logging
spypsy Jan 8, 2025
a6a6308
Merge branch 'master' into spy/tx-utils-blobs
spypsy Jan 8, 2025
7b92aaf
fix import
spypsy Jan 8, 2025
d1442f7
Merge remote-tracking branch 'refs/remotes/origin/spy/tx-utils-blobs'…
spypsy Jan 8, 2025
c4783eb
fix logging, add test
spypsy Jan 8, 2025
e553adc
rm unused var
spypsy Jan 8, 2025
db53a66
disable faucet on sepolia
spypsy Jan 8, 2025
5e73f7e
actually use format fn...
spypsy Jan 8, 2025
3bb7df0
allow for fixed priority fee
spypsy Jan 8, 2025
aa0e244
truncate more log fields
spypsy Jan 9, 2025
5737628
don't use fixed blob fees
spypsy Jan 9, 2025
9a4fef2
fix maybe undefined
spypsy Jan 9, 2025
1da6523
values updates
spypsy Jan 9, 2025
11e448c
update typing
spypsy Jan 9, 2025
b04b1b1
fix test
spypsy Jan 9, 2025
379e286
try fixing logging again
spypsy Jan 9, 2025
6e95160
use formatter in sendTransaction
spypsy Jan 9, 2025
9fd9ec2
fix for <1 gwei prio fee
spypsy Jan 9, 2025
cfbe1d0
just format everywhere
spypsy Jan 9, 2025
89d1ec5
Merge branch 'master' into spy/tx-utils-blobs
spypsy Jan 9, 2025
9d1e93d
allow 2 decimals in percentages, set lower gas estimate buffer
spypsy Jan 9, 2025
7140c1e
update test
spypsy Jan 10, 2025
da68f1a
another
spypsy Jan 10, 2025
dec90b6
simplify formatting
spypsy Jan 10, 2025
45ee7f8
maintain shorter hexes
spypsy Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 64 additions & 7 deletions yarn-project/ethereum/src/l1_tx_utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Blob } from '@aztec/foundation/blob';
import { EthAddress } from '@aztec/foundation/eth-address';
import { createLogger } from '@aztec/foundation/log';
import { sleep } from '@aztec/foundation/sleep';
Expand Down Expand Up @@ -106,8 +107,9 @@ describe('GasUtils', () => {
await cheatCodes.setAutomine(false);
await cheatCodes.setIntervalMining(0);

// Ensure initial base fee is low
await cheatCodes.setNextBlockBaseFeePerGas(initialBaseFee);
spalladino marked this conversation as resolved.
Show resolved Hide resolved
// Add blob data
const blobData = new Uint8Array(131072).fill(1);
const kzg = Blob.getViemKzgInstance();

const request = {
to: '0x1234567890123456789012345678901234567890' as `0x${string}`,
Expand All @@ -119,12 +121,16 @@ describe('GasUtils', () => {

const originalMaxFeePerGas = WEI_CONST * 10n;
const originalMaxPriorityFeePerGas = WEI_CONST;
const originalMaxFeePerBlobGas = WEI_CONST * 10n;

const txHash = await walletClient.sendTransaction({
...request,
gas: estimatedGas,
maxFeePerGas: originalMaxFeePerGas,
maxPriorityFeePerGas: originalMaxPriorityFeePerGas,
blobs: [blobData],
kzg,
maxFeePerBlobGas: originalMaxFeePerBlobGas,
});

const rawTx = await cheatCodes.getRawTransaction(txHash);
Expand All @@ -142,11 +148,12 @@ describe('GasUtils', () => {
params: [rawTx],
});

// keeping auto-mining disabled to simulate a stuck transaction
// The monitor should detect the stall and create a replacement tx

// Monitor should detect stall and replace with higher gas price
const monitorFn = gasUtils.monitorTransaction(request, txHash, { gasLimit: estimatedGas });
const monitorFn = gasUtils.monitorTransaction(request, txHash, { gasLimit: estimatedGas }, undefined, {
blobs: [blobData],
kzg,
maxFeePerBlobGas: WEI_CONST * 20n,
});

await sleep(2000);
// re-enable mining
Expand All @@ -156,11 +163,12 @@ describe('GasUtils', () => {
// Verify that a replacement transaction was created
expect(receipt.transactionHash).not.toBe(txHash);

// Get details of replacement tx to verify higher gas price
// Get details of replacement tx to verify higher gas prices
const replacementTx = await publicClient.getTransaction({ hash: receipt.transactionHash });

expect(replacementTx.maxFeePerGas!).toBeGreaterThan(originalMaxFeePerGas);
expect(replacementTx.maxPriorityFeePerGas!).toBeGreaterThan(originalMaxPriorityFeePerGas);
expect(replacementTx.maxFeePerBlobGas!).toBeGreaterThan(originalMaxFeePerBlobGas);
}, 20_000);

it('respects max gas price limits during spikes', async () => {
Expand Down Expand Up @@ -299,4 +307,53 @@ describe('GasUtils', () => {
const expectedEstimate = baseEstimate + (baseEstimate * 20n) / 100n;
expect(bufferedEstimate).toBe(expectedEstimate);
});

it('correctly handles transactions with blobs', async () => {
// Create a sample blob
const blobData = new Uint8Array(131072).fill(1); // 128KB blob
const kzg = Blob.getViemKzgInstance();

const receipt = await gasUtils.sendAndMonitorTransaction(
{
to: '0x1234567890123456789012345678901234567890',
data: '0x',
value: 0n,
},
undefined,
{
blobs: [blobData],
kzg,
maxFeePerBlobGas: 10000000000n, // 10 gwei
},
);

expect(receipt.status).toBe('success');
expect(receipt.blobGasUsed).toBeDefined();
expect(receipt.blobGasPrice).toBeDefined();
}, 20_000);

it('estimates gas correctly for blob transactions', async () => {
// Create a sample blob
const blobData = new Uint8Array(131072).fill(1); // 128KB blob
const kzg = Blob.getViemKzgInstance();

const request = {
to: '0x1234567890123456789012345678901234567890' as `0x${string}`,
data: '0x' as `0x${string}`,
value: 0n,
};

// Estimate gas without blobs first
const baseEstimate = await gasUtils.estimateGas(walletClient.account!, request);

// Estimate gas with blobs
const blobEstimate = await gasUtils.estimateGas(walletClient.account!, request, undefined, {
blobs: [blobData],
kzg,
maxFeePerBlobGas: 10000000000n,
});

// Blob transactions should require more gas
expect(blobEstimate).toBeGreaterThan(baseEstimate);
Comment on lines +359 to +360
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious: why is this? Does eth_estimateGas return both the execution gas and the blob gas mixed up together?

Copy link
Member Author

@spypsy spypsy Jan 6, 2025

Choose a reason for hiding this comment

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

it seems in our estimateGas, we have to use prepareTransactionRequest to get a full estimation, which does increase a lot compared to estimateGas so I assumed that includes the blob gas

}, 20_000);
});
32 changes: 30 additions & 2 deletions yarn-project/ethereum/src/l1_tx_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ export interface L1BlobInputs {
interface GasPrice {
maxFeePerGas: bigint;
maxPriorityFeePerGas: bigint;
maxFeePerBlobGas?: bigint;
}

export class L1TxUtils {
Expand Down Expand Up @@ -301,7 +302,11 @@ export class L1TxUtils {
gasConfig,
attempts,
tx.maxFeePerGas && tx.maxPriorityFeePerGas
? { maxFeePerGas: tx.maxFeePerGas, maxPriorityFeePerGas: tx.maxPriorityFeePerGas }
? {
maxFeePerGas: tx.maxFeePerGas,
maxPriorityFeePerGas: tx.maxPriorityFeePerGas,
maxFeePerBlobGas: tx.maxFeePerBlobGas,
}
: undefined,
);

Expand Down Expand Up @@ -365,6 +370,15 @@ export class L1TxUtils {
const block = await this.publicClient.getBlock({ blockTag: 'latest' });
const baseFee = block.baseFeePerGas ?? 0n;

// Get blob base fee if available
let blobBaseFee = 0n;
try {
const blobBaseFeeHex = await this.publicClient.request({ method: 'eth_blobBaseFee' });
blobBaseFee = BigInt(blobBaseFeeHex);
Comment on lines +428 to +429
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand we should be increasing the blob base fee to 12.5% per stall time in blocks, as we do with the base fee (see here).

} catch {
// Ignore if not supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's at least log this as a warn. I'm worried about a scenario where the blobBaseFee request fails due to a connectivity error and we end up sending the tx with zero fee.

}

// Get initial priority fee from the network
let priorityFee = await this.publicClient.estimateMaxPriorityFeePerGas();
let maxFeePerGas = baseFee;
Expand Down Expand Up @@ -405,14 +419,28 @@ export class L1TxUtils {
// Ensure priority fee doesn't exceed max fee
const maxPriorityFeePerGas = priorityFee > maxFeePerGas ? maxFeePerGas : priorityFee;

if (attempt > 0 && previousGasPrice?.maxFeePerBlobGas) {
const bumpPercentage =
gasConfig.priorityFeeRetryBumpPercentage! > MIN_REPLACEMENT_BUMP_PERCENTAGE
? gasConfig.priorityFeeRetryBumpPercentage!
: MIN_REPLACEMENT_BUMP_PERCENTAGE;

blobBaseFee = (previousGasPrice.maxFeePerBlobGas * (100n + bumpPercentage)) / 100n;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we use the priorityFeeRetryBumpPercentage here? I'd expect that we just need to use the MIN_REPLACEMENT_BUMP_PERCENTAGE expected by geth (which comes from here apparently).

Also, we need to account for the current blob base fee, and do something similar to what we do for the calldata fee: use the minimum between the current chain blob base fee (bumped by 12.5% per stall time) and geth's minimum bump (10%) wrt the previous price.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for that 🙏
I was googling to find if there was such a percentage but couldn't find it so I used this patchy solution.
I should go directly to geth code from now on

Copy link
Collaborator

Choose a reason for hiding this comment

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

I should go directly to geth code from now on

Yup. I learned that the hard way. Anything related to mempool management is out of protocol, so it depends on the clients, and is awfully undocumented.

}

this.logger?.debug(`Computed gas price`, {
attempt,
baseFee: formatGwei(baseFee),
maxFeePerGas: formatGwei(maxFeePerGas),
maxPriorityFeePerGas: formatGwei(maxPriorityFeePerGas),
...(blobBaseFee && { maxFeePerBlobGas: formatGwei(blobBaseFee) }),
});

return { maxFeePerGas, maxPriorityFeePerGas };
return {
maxFeePerGas,
maxPriorityFeePerGas,
...(blobBaseFee && { maxFeePerBlobGas: blobBaseFee }),
};
}

/**
Expand Down
Loading