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

refactor: replacing use of L2Tx with TxEffect #4876

Merged
merged 19 commits into from
Mar 1, 2024
Merged
8 changes: 4 additions & 4 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ jobs:
command: cond_spot_run_compose end-to-end 4 ./scripts/docker-compose.yml TEST=e2e_inclusion_proofs_contract.test.ts
aztec_manifest_key: end-to-end

e2e-pending-commitments-contract:
e2e-pending-note-hashes-contract:
docker:
- image: aztecprotocol/alpine-build-image
resource_class: small
Expand All @@ -902,7 +902,7 @@ jobs:
- *setup_env
- run:
name: "Test"
command: cond_spot_run_compose end-to-end 4 ./scripts/docker-compose.yml TEST=e2e_pending_commitments_contract.test.ts
command: cond_spot_run_compose end-to-end 4 ./scripts/docker-compose.yml TEST=e2e_pending_note_hashes_contract.test.ts
aztec_manifest_key: end-to-end

e2e-ordering:
Expand Down Expand Up @@ -1493,7 +1493,7 @@ workflows:
- e2e-account-contracts: *e2e_test
- e2e-escrow-contract: *e2e_test
- e2e-inclusion-proofs-contract: *e2e_test
- e2e-pending-commitments-contract: *e2e_test
- e2e-pending-note-hashes-contract: *e2e_test
- e2e-ordering: *e2e_test
- e2e-counter: *e2e_test
- e2e-private-voting: *e2e_test
Expand Down Expand Up @@ -1539,7 +1539,7 @@ workflows:
- e2e-account-contracts
- e2e-escrow-contract
- e2e-inclusion-proofs-contract
- e2e-pending-commitments-contract
- e2e-pending-note-hashes-contract
- e2e-ordering
- e2e-counter
- e2e-private-voting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ ImportTestContractArtifact
InclusionProofsContractArtifact
LendingContractArtifact
ParentContractArtifact
PendingCommitmentsContractArtifact
PendingNoteHashesContractArtifact
PriceFeedContractArtifact
SchnorrAccountContractArtifact
SchnorrHardcodedAccountContractArtifact
Expand Down
2 changes: 1 addition & 1 deletion noir-projects/noir-contracts/Nargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ members = [
"contracts/inclusion_proofs_contract",
"contracts/lending_contract",
"contracts/parent_contract",
"contracts/pending_commitments_contract",
"contracts/pending_note_hashes_contract",
"contracts/price_feed_contract",
"contracts/schnorr_account_contract",
"contracts/schnorr_hardcoded_account_contract",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[package]
name = "pending_commitments_contract"
name = "pending_note_hashes_contract"
authors = [""]
compiler_version = ">=0.18.0"
type = "contract"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// read (eventually even nullified) in the same TX. This contract
// also contains some "bad" test cases to ensure that notes cannot
// be read/nullified before their creation etc.
contract PendingCommitments {
contract PendingNoteHashes {
// Libs
use dep::std::option::Option;
use dep::value_note::{balance_utils, filter::filter_notes_min_sum, value_note::{VALUE_NOTE_LEN, ValueNote}};
Expand Down Expand Up @@ -47,7 +47,7 @@ contract PendingCommitments {
note0.value
}

// Confirm cannot access commitments inserted later in same function
// Confirm cannot access note hashes inserted later in same function
#[aztec(private)]
fn test_bad_get_then_insert_flat(amount: Field, owner: AztecAddress) -> Field {
let owner_balance = storage.balances.at(owner);
Expand Down Expand Up @@ -108,7 +108,7 @@ contract PendingCommitments {
}

// Test pending note hashes with note insertion done in a nested call
// and "read" / get of that pending note/commitment in another nested call
// and "read" / get of that pending note hash in another nested call
// Realistic way to describe this test is "Mint note A, then burn note A in the same transaction"
#[aztec(private)]
fn test_insert_then_get_then_nullify_all_in_nested_calls(
Expand Down Expand Up @@ -248,7 +248,7 @@ contract PendingCommitments {
// that is created/inserted later in execution but in the parent.
// NOTE: This test is especially important in an end-to-end context because the parent call
// (and therefore the insertion) will be processed in an earlier kernel iteration, but the
// nested call (later kernel iteration) should not be able to read the commitment despite
// nested call (later kernel iteration) should not be able to read the note hash despite
// it being present at that stage in the kernel.
// If we can somehow force the simulator to allow execution to succeed can ensure that this test fails in the kernel
// #[aztec(private)]
Expand Down
11 changes: 8 additions & 3 deletions yarn-project/archiver/src/archiver/archiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import {
L2BlockL2Logs,
L2BlockSource,
L2LogsSource,
L2Tx,
LogFilter,
LogType,
TxEffect,
TxHash,
TxReceipt,
UnencryptedL2Log,
} from '@aztec/circuit-types';
import { ContractClassRegisteredEvent, FunctionSelector } from '@aztec/circuits.js';
Expand Down Expand Up @@ -434,8 +435,12 @@ export class Archiver implements ArchiveSource {
return blocks.length === 0 ? undefined : blocks[0];
}

public getL2Tx(txHash: TxHash): Promise<L2Tx | undefined> {
return this.store.getL2Tx(txHash);
public getTxEffect(txHash: TxHash): Promise<TxEffect | undefined> {
return this.store.getTxEffect(txHash);
}

public getSettledTxReceipt(txHash: TxHash): Promise<TxReceipt | undefined> {
return this.store.getSettledTxReceipt(txHash);
}

/**
Expand Down
18 changes: 13 additions & 5 deletions yarn-project/archiver/src/archiver/archiver_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ import {
L1ToL2Message,
L2Block,
L2BlockL2Logs,
L2Tx,
LogFilter,
LogType,
TxEffect,
TxHash,
TxReceipt,
} from '@aztec/circuit-types';
import { Fr } from '@aztec/circuits.js';
import { AztecAddress } from '@aztec/foundation/aztec-address';
Expand Down Expand Up @@ -63,11 +64,18 @@ export interface ArchiverDataStore {
getBlocks(from: number, limit: number): Promise<L2Block[]>;

/**
* Gets an l2 tx.
* @param txHash - The txHash of the l2 tx.
* @returns The requested L2 tx.
* Gets a tx effect.
* @param txHash - The txHash of the tx corresponding to the tx effect.
* @returns The requested tx effect (or undefined if not found).
*/
getL2Tx(txHash: TxHash): Promise<L2Tx | undefined>;
getTxEffect(txHash: TxHash): Promise<TxEffect | undefined>;

/**
* Gets a receipt of a settled tx.
* @param txHash - The hash of a tx we try to get the receipt for.
* @returns The requested tx receipt (or undefined if not found).
*/
getSettledTxReceipt(txHash: TxHash): Promise<TxReceipt | undefined>;

/**
* Append new logs to the store's list.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export function describeArchiverDataStore(testName: string, getStore: () => Arch
});
});

describe('getL2Tx', () => {
describe('getTxEffect', () => {
beforeEach(async () => {
await Promise.all(
blocks.map(block => store.addLogs(block.body.encryptedLogs, block.body.unencryptedLogs, block.number)),
Expand All @@ -173,12 +173,12 @@ export function describeArchiverDataStore(testName: string, getStore: () => Arch
() => blocks[1].getTx(0),
])('retrieves a previously stored transaction', async getExpectedTx => {
const expectedTx = getExpectedTx();
const actualTx = await store.getL2Tx(expectedTx.txHash);
const actualTx = await store.getTxEffect(expectedTx.txHash);
expect(actualTx).toEqual(expectedTx);
});

it('returns undefined if tx is not found', async () => {
await expect(store.getL2Tx(new TxHash(Fr.random().toBuffer()))).resolves.toBeUndefined();
await expect(store.getTxEffect(new TxHash(Fr.random().toBuffer()))).resolves.toBeUndefined();
});
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { INITIAL_L2_BLOCK_NUM, L2Block, L2Tx, TxHash } from '@aztec/circuit-types';
import { INITIAL_L2_BLOCK_NUM, L2Block, TxEffect, TxHash, TxReceipt, TxStatus } from '@aztec/circuit-types';
import { AppendOnlyTreeSnapshot, AztecAddress, Header } from '@aztec/circuits.js';
import { createDebugLogger } from '@aztec/foundation/log';
import { AztecKVStore, AztecMap, Range } from '@aztec/kv-store';
Expand Down Expand Up @@ -117,12 +117,12 @@ export class BlockStore {
}

/**
* Gets an l2 tx.
* @param txHash - The txHash of the l2 tx.
* @returns The requested L2 tx.
* Gets a tx effect.
* @param txHash - The txHash of the tx corresponding to the tx effect.
* @returns The requested tx effect (or undefined if not found).
*/
getL2Tx(txHash: TxHash): L2Tx | undefined {
const [blockNumber, txIndex] = this.getL2TxLocation(txHash) ?? [];
getTxEffect(txHash: TxHash): TxEffect | undefined {
const [blockNumber, txIndex] = this.getTxLocation(txHash) ?? [];
if (typeof blockNumber !== 'number' || typeof txIndex !== 'number') {
return undefined;
}
Expand All @@ -132,11 +132,26 @@ export class BlockStore {
}

/**
* Looks up which block included the requested L2 tx.
* @param txHash - The txHash of the l2 tx.
* Gets a receipt of a settled tx.
* @param txHash - The hash of a tx we try to get the receipt for.
* @returns The requested tx receipt (or undefined if not found).
*/
getSettledTxReceipt(txHash: TxHash): TxReceipt | undefined {
const [blockNumber, txIndex] = this.getTxLocation(txHash) ?? [];
if (typeof blockNumber !== 'number' || typeof txIndex !== 'number') {
return undefined;
}

const block = this.getBlock(blockNumber)!;
return new TxReceipt(txHash, TxStatus.MINED, '', block.hash().toBuffer(), block.number);
}

/**
* Looks up which block included the requested tx effect.
* @param txHash - The txHash of the tx.
* @returns The block number and index of the tx.
*/
getL2TxLocation(txHash: TxHash): [blockNumber: number, txIndex: number] | undefined {
getTxLocation(txHash: TxHash): [blockNumber: number, txIndex: number] | undefined {
return this.#txIndex.get(txHash.toString());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ import {
L1ToL2Message,
L2Block,
L2BlockL2Logs,
L2Tx,
LogFilter,
LogType,
TxEffect,
TxHash,
TxReceipt,
} from '@aztec/circuit-types';
import { Fr } from '@aztec/circuits.js';
import { AztecAddress } from '@aztec/foundation/aztec-address';
Expand Down Expand Up @@ -115,12 +116,21 @@ export class KVArchiverDataStore implements ArchiverDataStore {
}

/**
* Gets an l2 tx.
* @param txHash - The txHash of the l2 tx.
* @returns The requested L2 tx.
* Gets a tx effect.
* @param txHash - The txHash of the tx corresponding to the tx effect.
* @returns The requested tx effect (or undefined if not found).
*/
getL2Tx(txHash: TxHash): Promise<L2Tx | undefined> {
return Promise.resolve(this.#blockStore.getL2Tx(txHash));
getTxEffect(txHash: TxHash): Promise<TxEffect | undefined> {
return Promise.resolve(this.#blockStore.getTxEffect(txHash));
}

/**
* Gets a receipt of a settled tx.
* @param txHash - The hash of a tx we try to get the receipt for.
* @returns The requested tx receipt (or undefined if not found).
*/
getSettledTxReceipt(txHash: TxHash): Promise<TxReceipt | undefined> {
return Promise.resolve(this.#blockStore.getSettledTxReceipt(txHash));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export class LogStore {
throw new Error('Missing txHash');
}

const [blockNumber, txIndex] = this.blockStore.getL2TxLocation(filter.txHash) ?? [];
const [blockNumber, txIndex] = this.blockStore.getTxLocation(filter.txHash) ?? [];
if (typeof blockNumber !== 'number' || typeof txIndex !== 'number') {
return { logs: [], maxLogsHit: false };
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import {
L2Block,
L2BlockContext,
L2BlockL2Logs,
L2Tx,
LogFilter,
LogId,
LogType,
TxEffect,
TxHash,
TxReceipt,
TxStatus,
UnencryptedL2Log,
} from '@aztec/circuit-types';
import { Fr, NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP } from '@aztec/circuits.js';
Expand All @@ -38,9 +40,9 @@ export class MemoryArchiverStore implements ArchiverDataStore {
private l2BlockBodies: Map<string, Body> = new Map();

/**
* An array containing all the L2 Txs in the L2 blocks that have been fetched so far.
* An array containing all the the tx effects in the L2 blocks that have been fetched so far.
*/
private l2Txs: L2Tx[] = [];
private txEffects: TxEffect[] = [];

/**
* An array containing all the encrypted logs that have been fetched so far.
Expand Down Expand Up @@ -120,7 +122,7 @@ export class MemoryArchiverStore implements ArchiverDataStore {
*/
public addBlocks(blocks: L2Block[]): Promise<boolean> {
this.l2BlockContexts.push(...blocks.map(block => new L2BlockContext(block)));
this.l2Txs.push(...blocks.flatMap(b => b.getTxs()));
this.txEffects.push(...blocks.flatMap(b => b.getTxs()));
return Promise.resolve(true);
}

Expand Down Expand Up @@ -271,13 +273,31 @@ export class MemoryArchiverStore implements ArchiverDataStore {
}

/**
* Gets an l2 tx.
* @param txHash - The txHash of the l2 tx.
* @returns The requested L2 tx.
* Gets a tx effect.
* @param txHash - The txHash of the tx effect.
* @returns The requested tx effect.
*/
public getL2Tx(txHash: TxHash): Promise<L2Tx | undefined> {
const l2Tx = this.l2Txs.find(tx => tx.txHash.equals(txHash));
return Promise.resolve(l2Tx);
public getTxEffect(txHash: TxHash): Promise<TxEffect | undefined> {
const txEffect = this.txEffects.find(tx => tx.txHash.equals(txHash));
return Promise.resolve(txEffect);
}

/**
* Gets a receipt of a settled tx.
* @param txHash - The hash of a tx we try to get the receipt for.
* @returns The requested tx receipt (or undefined if not found).
*/
public getSettledTxReceipt(txHash: TxHash): Promise<TxReceipt | undefined> {
for (const blockContext of this.l2BlockContexts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fairly inefficient but I guess it is just as we don't actually have the index to do the lookup in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but given that the MemoryArchiveStore is not used anymore (there is KVArchiverDataStore now) I think it doesn't matter.

for (const currentTxHash of blockContext.getTxHashes()) {
if (currentTxHash.equals(txHash)) {
return Promise.resolve(
new TxReceipt(txHash, TxStatus.MINED, '', blockContext.block.hash().toBuffer(), blockContext.block.number),
);
}
}
}
return Promise.resolve(undefined);
}

/**
Expand Down
3 changes: 2 additions & 1 deletion yarn-project/archiver/src/rpc/archiver_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
L1ToL2Message,
L2Block,
L2BlockL2Logs,
TxReceipt,
} from '@aztec/circuit-types';
import { EthAddress, Fr } from '@aztec/circuits.js';
import { createJsonRpcClient, makeFetch } from '@aztec/foundation/json-rpc/client';
Expand All @@ -26,7 +27,7 @@ export const createArchiverClient = (url: string, fetch = makeFetch([1, 2, 3], t
L2Block,
L2BlockL2Logs,
},
{},
{ TxReceipt },
false,
'archiver',
fetch,
Expand Down
5 changes: 4 additions & 1 deletion yarn-project/archiver/src/rpc/archiver_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
L1ToL2Message,
L2Block,
L2BlockL2Logs,
TxEffect,
TxReceipt,
} from '@aztec/circuit-types';
import { EthAddress, Fr } from '@aztec/circuits.js';
import { JsonRpcServer } from '@aztec/foundation/json-rpc/server';
Expand All @@ -30,8 +32,9 @@ export function createArchiverRpcServer(archiverService: Archiver): JsonRpcServe
L1ToL2Message,
L2Block,
L2BlockL2Logs,
TxEffect,
},
{},
{ TxReceipt },
['start', 'stop'],
);
}