Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 4 additions & 20 deletions yarn-project/end-to-end/src/e2e_mempool_limit.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { AztecAddress } from '@aztec/aztec.js/addresses';
import { NO_WAIT } from '@aztec/aztec.js/contracts';
import { TxStatus } from '@aztec/aztec.js/tx';
import { retryUntil } from '@aztec/foundation/retry';
import { TokenContract } from '@aztec/noir-contracts.js/Token';
import type { AztecNode, AztecNodeAdmin } from '@aztec/stdlib/interfaces/client';

Expand Down Expand Up @@ -70,24 +69,9 @@ describe('e2e_mempool_limit', () => {
expect.objectContaining({ status: TxStatus.PENDING }),
);

const txHash3 = await tx3.send({ wait: NO_WAIT });

const txDropped = await retryUntil(
async () => {
// one of the txs will be dropped. Which one is picked is somewhat random because all three will have the same fee
const receipts = await Promise.all([
aztecNode.getTxReceipt(txHash1),
aztecNode.getTxReceipt(txHash2),
aztecNode.getTxReceipt(txHash3),
]);
const numPending = receipts.reduce((count, r) => (r.status === TxStatus.PENDING ? count + 1 : count), 0);
return numPending < 3;
},
'Waiting for one of the txs to be evicted from the mempool',
60,
1,
);

expect(txDropped).toBe(true);
// tx3 should be rejected because pool is at capacity and its priority is not higher than existing txs
await expect(tx3.send({ wait: NO_WAIT })).rejects.toMatchObject({
data: { code: 'LOW_PRIORITY_FEE' },
});
Comment on lines -73 to +75
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous behavior of test dropped tx 3 anyways due to pending limit set above (2). Removed the retryUntil and maintained behavior of tx3 dropped.

});
});
23 changes: 23 additions & 0 deletions yarn-project/p2p/src/client/p2p_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,29 @@ describe('P2P Client', () => {
await client.stop();
});

it('throws TxPoolError with structured reason when pool rejects tx', async () => {
await client.start();
const tx1 = await mockTx();
const txHashStr = tx1.getTxHash().toString();
const errors = new Map();
errors.set(txHashStr, {
code: 'LOW_PRIORITY_FEE',
message: 'Tx does not meet minimum priority fee',
minimumPriorityFee: 101n,
txPriorityFee: 50n,
});
txPool.addPendingTxs.mockResolvedValueOnce({
accepted: [],
ignored: [tx1.getTxHash()],
rejected: [],
errors,
});

await expect(client.sendTx(tx1)).rejects.toThrow('Tx does not meet minimum priority fee');
expect(p2pService.propagate).not.toHaveBeenCalled();
await client.stop();
});

it('rejects txs after being stopped', async () => {
await client.start();
const tx1 = await mockTx();
Expand Down
17 changes: 13 additions & 4 deletions yarn-project/p2p/src/client/p2p_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import type { PeerId } from '@libp2p/interface';
import type { ENR } from '@nethermindeth/enr';

import { type P2PConfig, getP2PDefaultConfig } from '../config.js';
import { TxPoolError } from '../errors/tx-pool.error.js';
import type { AttestationPoolApi } from '../mem_pools/attestation_pool/attestation_pool.js';
import type { MemPools } from '../mem_pools/interface.js';
import type { TxPoolV2 } from '../mem_pools/tx_pool_v2/interfaces.js';
Expand Down Expand Up @@ -585,11 +586,19 @@ export class P2PClient<T extends P2PClientType = P2PClientType.Full>
const result = await this.txPool.addPendingTxs([tx], { feeComparisonOnly: true });
if (result.accepted.length === 1) {
await this.p2pService.propagate(tx);
} else {
this.log.warn(
`Tx ${tx.getTxHash()} not propagated: accepted=${result.accepted.length} ignored=${result.ignored.length} rejected=${result.rejected.length}`,
);
return;
}

const txHashStr = tx.getTxHash().toString();
const reason = result.errors?.get(txHashStr);
if (reason) {
this.log.warn(`Tx ${txHashStr} not added to pool: ${reason.message}`);
throw new TxPoolError(reason);
}

this.log.warn(
`Tx ${txHashStr} not propagated: accepted=${result.accepted.length} ignored=${result.ignored.length} rejected=${result.rejected.length}`,
);
}

/**
Expand Down
12 changes: 12 additions & 0 deletions yarn-project/p2p/src/errors/tx-pool.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import type { TxPoolRejectionError } from '../mem_pools/tx_pool_v2/eviction/interfaces.js';

/** Error thrown when a transaction is not added to the mempool. */
export class TxPoolError extends Error {
public readonly data: TxPoolRejectionError;

constructor(public readonly reason: TxPoolRejectionError) {
super(reason.message);
this.name = 'TxPoolError';
this.data = reason;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
type PoolOperations,
type PreAddPoolAccess,
type PreAddRule,
TxPoolRejectionCode,
} from './interfaces.js';

describe('EvictionManager', () => {
Expand Down Expand Up @@ -226,10 +227,15 @@ describe('EvictionManager', () => {
it('returns ignore result immediately when a rule says to ignore', async () => {
const preAddRule2 = mock<PreAddRule>({ name: 'preAddRule2' });

const testReason = {
code: 'NULLIFIER_CONFLICT' as const,
message: 'test reason',
conflictingTxHash: '0x9999',
};
preAddRule.check.mockResolvedValue({
shouldIgnore: true,
txHashesToEvict: [],
reason: 'test reason',
reason: testReason,
});
preAddRule2.check.mockResolvedValue({
shouldIgnore: false,
Expand All @@ -243,7 +249,7 @@ describe('EvictionManager', () => {
const result = await evictionManager.runPreAddRules(incomingMeta, poolAccess);

expect(result.shouldIgnore).toBe(true);
expect(result.reason).toBe('test reason');
expect(result.reason).toEqual(testReason);
expect(preAddRule.check).toHaveBeenCalledTimes(1);
// Second rule should not be called since first rule ignored
expect(preAddRule2.check).not.toHaveBeenCalled();
Expand Down Expand Up @@ -351,7 +357,9 @@ describe('EvictionManager', () => {
const result = await evictionManager.runPreAddRules(incomingMeta, poolAccess);

expect(result.shouldIgnore).toBe(true);
expect(result.reason).toContain('failingRule');
expect(result.reason).toBeDefined();
expect(result.reason!.code).toBe(TxPoolRejectionCode.INTERNAL_ERROR);
expect(result.reason!.message).toContain('failingRule');
expect(result.txHashesToEvict).toHaveLength(0);
// Second rule should not be called since first rule threw
expect(preAddRule2.check).not.toHaveBeenCalled();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
type PreAddResult,
type PreAddRule,
type TaggedEviction,
TxPoolRejectionCode,
} from './interfaces.js';

/**
Expand Down Expand Up @@ -78,7 +79,10 @@ export class EvictionManager {
return {
shouldIgnore: true,
txHashesToEvict: [],
reason: `pre-add rule ${rule.name} error: ${err}`,
reason: {
code: TxPoolRejectionCode.INTERNAL_ERROR,
message: `Pre-add rule ${rule.name} error: ${err}`,
},
};
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { type TxMetaData, stubTxMetaValidationData } from '../tx_metadata.js';
import { FeePayerBalancePreAddRule } from './fee_payer_balance_pre_add_rule.js';
import type { PreAddPoolAccess } from './interfaces.js';
import { type PreAddPoolAccess, TxPoolRejectionCode } from './interfaces.js';

describe('FeePayerBalancePreAddRule', () => {
let rule: FeePayerBalancePreAddRule;
Expand Down Expand Up @@ -63,7 +63,12 @@ describe('FeePayerBalancePreAddRule', () => {

expect(result.shouldIgnore).toBe(true);
expect(result.txHashesToEvict).toHaveLength(0);
expect(result.reason).toContain('insufficient balance');
expect(result.reason).toBeDefined();
expect(result.reason!.code).toBe(TxPoolRejectionCode.INSUFFICIENT_FEE_PAYER_BALANCE);
if (result.reason!.code === TxPoolRejectionCode.INSUFFICIENT_FEE_PAYER_BALANCE) {
expect(result.reason!.currentBalance).toBe(50n);
expect(result.reason!.feeLimit).toBe(100n);
}
});

it('accepts tx when balance exactly equals fee limit', async () => {
Expand Down Expand Up @@ -108,7 +113,8 @@ describe('FeePayerBalancePreAddRule', () => {
const result = await rule.check(incomingMeta, poolAccess);

expect(result.shouldIgnore).toBe(true);
expect(result.reason).toContain('insufficient balance');
expect(result.reason).toBeDefined();
expect(result.reason!.code).toBe(TxPoolRejectionCode.INSUFFICIENT_FEE_PAYER_BALANCE);
});

it('evicts lower-priority existing tx when high-priority tx is added', async () => {
Expand Down Expand Up @@ -263,7 +269,8 @@ describe('FeePayerBalancePreAddRule', () => {

expect(result.shouldIgnore).toBe(true);
expect(result.reason).toBeDefined();
expect(result.reason).toContain('insufficient balance');
expect(result.reason!.code).toBe(TxPoolRejectionCode.INSUFFICIENT_FEE_PAYER_BALANCE);
expect(result.reason!.message).toContain('insufficient balance');
});
});
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { createLogger } from '@aztec/foundation/log';

import { type TxMetaData, comparePriority } from '../tx_metadata.js';
import type { PreAddContext, PreAddPoolAccess, PreAddResult, PreAddRule } from './interfaces.js';
import {
type PreAddContext,
type PreAddPoolAccess,
type PreAddResult,
type PreAddRule,
TxPoolRejectionCode,
} from './interfaces.js';

/**
* Pre-add rule that checks if a fee payer has sufficient balance to cover the incoming transaction.
Expand Down Expand Up @@ -78,7 +84,13 @@ export class FeePayerBalancePreAddRule implements PreAddRule {
return {
shouldIgnore: true,
txHashesToEvict: [],
reason: `fee payer ${incomingMeta.feePayer} has insufficient balance`,
reason: {
code: TxPoolRejectionCode.INSUFFICIENT_FEE_PAYER_BALANCE,
message: `Fee payer ${incomingMeta.feePayer} has insufficient balance. Balance at transaction: ${available}, required: ${incomingMeta.feeLimit}`,
currentBalance: initialBalance,
availableBalance: available,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now returns availableBalance to user as well

feeLimit: incomingMeta.feeLimit,
},
};
} else {
// Existing tx cannot be covered after adding incoming - mark for eviction
Expand All @@ -93,7 +105,6 @@ export class FeePayerBalancePreAddRule implements PreAddRule {
return {
shouldIgnore: true,
txHashesToEvict: [],
reason: 'internal error: tx coverage not determined',
};
}

Expand Down
2 changes: 2 additions & 0 deletions yarn-project/p2p/src/mem_pools/tx_pool_v2/eviction/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ export {
type PreAddResult,
type PreAddRule,
type TaggedEviction,
TxPoolRejectionCode,
type TxPoolRejectionError,
} from './interfaces.js';

// Pre-add rules
Expand Down
32 changes: 31 additions & 1 deletion yarn-project/p2p/src/mem_pools/tx_pool_v2/eviction/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,36 @@ export interface TaggedEviction {
readonly reason: string;
}

/**
* Machine-readable rejection codes for pre-add rule rejections.
*/
export const TxPoolRejectionCode = {
LOW_PRIORITY_FEE: 'LOW_PRIORITY_FEE',
INSUFFICIENT_FEE_PAYER_BALANCE: 'INSUFFICIENT_FEE_PAYER_BALANCE',
NULLIFIER_CONFLICT: 'NULLIFIER_CONFLICT',
INTERNAL_ERROR: 'INTERNAL_ERROR',
} as const;

export type TxPoolRejectionCode = (typeof TxPoolRejectionCode)[keyof typeof TxPoolRejectionCode];

/** Structured rejection reason returned by pre-add rules. */
export type TxPoolRejectionError =
| {
code: typeof TxPoolRejectionCode.LOW_PRIORITY_FEE;
message: string;
minimumPriorityFee: bigint;
txPriorityFee: bigint;
}
| {
code: typeof TxPoolRejectionCode.INSUFFICIENT_FEE_PAYER_BALANCE;
message: string;
currentBalance: bigint;
availableBalance: bigint;
feeLimit: bigint;
}
| { code: typeof TxPoolRejectionCode.NULLIFIER_CONFLICT; message: string; conflictingTxHash: string }
| { code: typeof TxPoolRejectionCode.INTERNAL_ERROR; message: string };

/**
* Result of a pre-add check for a single transaction.
*/
Expand All @@ -84,7 +114,7 @@ export interface PreAddResult {
/** Evictions tagged with the rule name that produced them. Populated by EvictionManager. */
readonly evictions?: TaggedEviction[];
/** Optional reason for ignoring */
readonly reason?: string;
readonly reason?: TxPoolRejectionError;
}

/** Context passed to pre-add rules from addPendingTxs. */
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { type TxMetaData, comparePriority, stubTxMetaValidationData } from '../tx_metadata.js';
import type { PreAddContext, PreAddPoolAccess } from './interfaces.js';
import { type PreAddContext, type PreAddPoolAccess, TxPoolRejectionCode } from './interfaces.js';
import { LowPriorityPreAddRule } from './low_priority_pre_add_rule.js';

describe('LowPriorityPreAddRule', () => {
Expand Down Expand Up @@ -101,7 +101,12 @@ describe('LowPriorityPreAddRule', () => {

expect(result.shouldIgnore).toBe(true);
expect(result.txHashesToEvict).toHaveLength(0);
expect(result.reason).toContain('lower priority');
expect(result.reason).toBeDefined();
expect(result.reason!.code).toBe(TxPoolRejectionCode.LOW_PRIORITY_FEE);
if (result.reason!.code === TxPoolRejectionCode.LOW_PRIORITY_FEE) {
expect(result.reason!.minimumPriorityFee).toBe(101n);
expect(result.reason!.txPriorityFee).toBe(50n);
}
});

it('ignores tx when incoming has equal priority to lowest', async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
import { createLogger } from '@aztec/foundation/log';

import { type TxMetaData, comparePriority } from '../tx_metadata.js';
import type { EvictionConfig, PreAddContext, PreAddPoolAccess, PreAddResult, PreAddRule } from './interfaces.js';
import {
type EvictionConfig,
type PreAddContext,
type PreAddPoolAccess,
type PreAddResult,
type PreAddRule,
TxPoolRejectionCode,
} from './interfaces.js';

/**
* Pre-add rule that checks if the pool is at capacity and handles low-priority eviction.
Expand Down Expand Up @@ -66,7 +73,12 @@ export class LowPriorityPreAddRule implements PreAddRule {
return Promise.resolve({
shouldIgnore: true,
txHashesToEvict: [],
reason: `pool at capacity and tx has lower priority than existing transactions`,
reason: {
code: TxPoolRejectionCode.LOW_PRIORITY_FEE,
message: `Tx does not meet minimum priority fee. Required: ${lowestPriorityMeta.priorityFee + 1n}, got: ${incomingMeta.priorityFee}`,
minimumPriorityFee: lowestPriorityMeta.priorityFee + 1n,
txPriorityFee: incomingMeta.priorityFee,
},
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class NullifierConflictRule implements PreAddRule {
);

if (result.shouldIgnore) {
this.log.debug(`Ignoring tx ${incomingMeta.txHash}: ${result.reason}`);
this.log.debug(`Ignoring tx ${incomingMeta.txHash}: ${result.reason?.message}`);
}

return Promise.resolve(result);
Expand Down
3 changes: 3 additions & 0 deletions yarn-project/p2p/src/mem_pools/tx_pool_v2/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { L2Block, L2BlockId, L2BlockSource } from '@aztec/stdlib/block';
import type { WorldStateSynchronizer } from '@aztec/stdlib/interfaces/server';
import type { BlockHeader, Tx, TxHash, TxValidator } from '@aztec/stdlib/tx';

import type { TxPoolRejectionError } from './eviction/interfaces.js';
import type { TxMetaData, TxState } from './tx_metadata.js';

/**
Expand All @@ -17,6 +18,8 @@ export type AddTxsResult = {
ignored: TxHash[];
/** Transactions rejected because they failed validation (e.g., invalid proof, expired timestamp) */
rejected: TxHash[];
/** Optional rejection errors, only present when there are rejections with structured errors. */
errors?: Map<string, TxPoolRejectionError>;
};

/**
Expand Down
Loading
Loading