chore: Validate RPC inputs #9672

spalladino · 2024-11-01T23:01:12Z

Adds schemas for every API. Every API exposed via JSON RPC now requires a zod schema (see #9656 for more context on the rationale for this change). All schemas are in circuit-types/interfaces, and look like:

aztec-packages/yarn-project/circuit-types/src/interfaces/prover-node.ts

Lines 33 to 45 in 3e78ec7

    
           /** Schemas for prover node API functions. */ 
        
           export const ProverNodeApiSchema: ApiSchemaFor<ProverNodeApi> = { 
        
             getJobs: z 
        
               .function() 
        
               .args() 
        
               .returns(z.array(z.object({ uuid: z.string(), status: z.enum(EpochProvingJobState) }))), 
        
             startProof: z.function().args(schemas.Integer).returns(z.void()), 
        
             prove: z.function().args(schemas.Integer).returns(z.void()), 
        
             sendEpochProofQuote: z.function().args(EpochProofQuote.schema).returns(z.void()), 
        
           };

These schemas are type-checked against the interface via the ApiSchemaFor utility type, so if the interface changes, schemas are required by the compiler to change as well. Schemas are now used in the JSON RPC server to 1) identify which methods are exposed (so we no longer need the method disallowlist) and 2) parse their arguments. The JSON RPC server, once it has identified the method to be called, grabs the arguments schema and funnels the result of a vanilla JSON parse through it.

Every type or struct that is exposed via an interface now has an associated schema, which is referenced in the API for parsing. Schemas both validate input and hydrate instances. This means that we no longer set a type property to identify how to hydrate each object in a request during deserialization, which was a security risk.

aztec-packages/yarn-project/circuit-types/src/l2_block.ts

Lines 24 to 32 in 3e78ec7

    
           static get schema() { 
        
             return z 
        
               .object({ 
        
                 archive: AppendOnlyTreeSnapshot.schema, 
        
                 header: Header.schema, 
        
                 body: Body.schema, 
        
               }) 
        
               .transform(({ archive, header, body }) => new L2Block(archive, header, body)); 
        
           }

Schemas are also used in the JSON RPC client for deserializing the result types. Again, this lets us remove the type parameter from all serialized entities, though this is still present in since it is required by the TypeRegistry (still to be removed) which is only used in the snapshot manager.

All schemas are tested via mini integration tests. These tests define a mock implementation for each service, use it for setting up a JSON RPC server, starting it in a free port, and test calling every method through JSON RPC.

aztec-packages/yarn-project/circuit-types/src/interfaces/prover-node.test.ts

Lines 12 to 31 in 3e78ec7

    
           beforeEach(async () => { 
        
             handler = new MockProverNode(); 
        
             context = await createJsonRpcTestSetup<ProverNodeApi>(handler, ProverNodeApiSchema); 
        
           }); 
        
           afterEach(() => { 
        
             tested.add(/^ProvingNodeApiSchema\s+([^(]+)/.exec(expect.getState().currentTestName!)![1]); 
        
             context.httpServer.close(); 
        
           }); 
        
           afterAll(() => { 
        
             const all = Object.keys(ProverNodeApiSchema); 
        
             expect([...tested].sort()).toEqual(all.sort()); 
        
           }); 
        
           it('getJobs', async () => { 
        
             const jobs = await context.client.getJobs(); 
        
             const expected = await handler.getJobs(); 
        
             expect(jobs).toEqual(expected); 
        
           });

These changes prompted other changes. For instance, we introduced the following changes to APIs:

ProvingJobSource.rejectProvingJob now accepts a reason string instead of an Error type
PXE.getEvents(type) is removed in favor of PXE.getEncryptedEvents and PXE.getUnencryptedEvents since both methods required different arguments

We also removed service-management methods (ie stop) from interfaces. We were inadvertently calling stop on remote instances over http when we shouldn't have. We also typed some previously untyped interfaces, such as the TXE's.

Fixes #9455

Some processes may prefer IPv6 for localhost. Like the nargo external oracle resolver when run in earthly.

ludamad · 2024-11-10T03:57:26Z

yarn-project/aztec.js/src/fee/private_fee_payment_method.ts

@@ -65,7 +65,7 @@ export class PrivateFeePaymentMethod implements FeePaymentMethod {
      caller: this.paymentContract,
      action: {
        name: 'setup_refund',
-        args: [this.feeRecipient, this.wallet.getAddress(), maxFee, nonce],
+        args: [this.feeRecipient.toField(), this.wallet.getAddress().toField(), maxFee, nonce],


Very happy to get rid of the inheritance here

ludamad · 2024-11-10T04:30:16Z

yarn-project/circuit-types/src/interfaces/archiver.test.ts

+import { TxEffect } from '../tx_effect.js';
+import { type ArchiverApi, ArchiverApiSchema } from './archiver.js';
+
+describe('ArchiverApiSchema', () => {


ludamad · 2024-11-10T04:31:26Z

yarn-project/circuit-types/src/interfaces/aztec-node.ts

   * @returns Whether the message is synced and ready to be included in a block.
   */
-  isL1ToL2MessageSynced(l1ToL2Message: Fr, startL2BlockNumber: number): Promise<boolean>;


Reason this isn't needed anymore?

ludamad · 2024-11-10T04:35:10Z

yarn-project/circuit-types/src/tx/tx.ts

@@ -121,6 +124,21 @@ export class Tx extends Gossipable {
    ]);
  }

+  static get schema() {
+    // TODO(palla/schemas): Use the nested objects schemas as opposed to the toBuffers


noting no issue number

ludamad · 2024-11-10T04:35:48Z

yarn-project/circuits.js/src/contract/artifact_hash.test.ts

@@ -16,7 +16,7 @@ describe('ArtifactHash', () => {
      notes: {},
    };
    expect(computeArtifactHash(emptyArtifact).toString()).toMatchInlineSnapshot(
-      `"0x0dea64e7fa0688017f77bcb7075485485afb4a5f1f8508483398869439f82fdf"`,


Interesting, why the change?

ludamad · 2024-11-10T04:44:06Z

yarn-project/protocol-contracts/src/protocol_contract_data.ts

@@ -53,11 +53,11 @@ export const ProtocolContractLeaf = {
  AuthRegistry: Fr.fromString('0x295f52c40413b660d817ceea60d07154322a035d28d18927b2ca84e8ec3b2115'),
  ContractInstanceDeployer: Fr.fromString('0x00113ad28d270a493266484d733f73a56b98c74b4e2cdf9fc040b5d3a6560f2d'),
  ContractClassRegisterer: Fr.fromString('0x1d591819cccc4031cc18a7865321c54f6344ae42a205782874d1f72648df2034'),
-  MultiCallEntrypoint: Fr.fromString('0x20a2e7e882045d27b3aa9e36188b8e45483b3c11652d4a46406699e5eb4efa9b'),


Curious why these change?

ludamad · 2024-11-10T04:45:12Z

yarn-project/foundation/src/json-rpc/server/json_rpc_server.ts

@@ -1,337 +0,0 @@
-import cors from '@koa/cors';
-import http from 'http';


ludamad · 2024-11-10T04:49:13Z

LGTM.

Review methodology:

look at methodology, agree with the approach
every line of diff needs to hit my eyeballs
consider large parts of it as low risk because of 1. type-checked schema 2. has tests 3. written by Palla
look closer at parts that stand out like hash changes, package.json changes etc
say LGTM

This reverts commit 6554122.

Reverts #9672 as it broke some e2e tests and boxes

* master: (182 commits) feat(avm): mem specific range check (#9828) refactor: remove public kernel inner (#9865) chore: Revert "chore: Validate RPC inputs" (#9875) Revert "fix: deploy l2 contracts fails on 48 validator" (#9871) fix: deploy l2 contracts fails on 48 validator (#9860) chore: Validate RPC inputs (#9672) fix: fixing devcontainers to use the sandbox docker-compose file (#9782) fix: Revert changes to ci.yml (#9863) chore: Move epoch and slot durations to config (#9861) git subrepo push --branch=master noir-projects/aztec-nr git_subrepo.sh: Fix parent in .gitrepo file. [skip ci] chore: replace relative paths to noir-protocol-circuits git subrepo push --branch=master barretenberg fix: tree heights that last past 3 days (#9760) fix(build): l1-contracts .rebuild_patterns did not cover test files (#9862) fix: bench prover test (#9856) fix: Fix mac build by calling `count` on durations (#9855) feat: zk shplemini (#9830) feat: domain separate block proposals and attestations (#9842) chore: bump runner cache disk size (#9849) ...

Second attempt at #9672 after it broke some e2e tests on master.

spalladino force-pushed the palla/validate-rpc-inputs branch 2 times, most recently from 7260917 to 8b713f0 Compare November 5, 2024 15:26

spalladino changed the title ~~wip: Validate RPC inputs~~ chore: Validate RPC inputs Nov 5, 2024

spalladino marked this pull request as ready for review November 5, 2024 15:50

spalladino force-pushed the palla/validate-rpc-inputs branch 4 times, most recently from 072ad42 to 90c592c Compare November 7, 2024 20:01

spalladino requested review from fcarreiro and dbanks12 as code owners November 7, 2024 20:01

fcarreiro removed request for dbanks12 and fcarreiro November 8, 2024 09:54

spalladino self-assigned this Nov 8, 2024

spalladino force-pushed the palla/validate-rpc-inputs branch 3 times, most recently from 3e78ec7 to d9d94e3 Compare November 8, 2024 23:18

spalladino added 14 commits November 8, 2024 23:43

Schema for proving job source

df5e611

Schema test for proving job source

3a81faa

Test for prover node schema

419d95b

Fix for parsing coerced schemas

be74193

Schema for archiver

fb2843a

PXE schema

8135429

Add ZodFor type

41442a5

Schema for aztec node

f599158

Schema for bot runner

bcd164f

Schema for prover agent api

a9659d0

Schema for txe dispatcher api

cdfa8cb

Add service interface to remove stop method from apis

b09a6bb

Switch to safe json rpc server and client and delete old ones

bd3a935

Clean up class conversion code

255fe5d

spalladino added 9 commits November 8, 2024 23:44

Ignore artifact metadata until 6021 lands

124ff85

Use a nullable optional type for optional parameters

00320a9

Handle serialized objects in abi encoder

94fe1a9

Fix tests

62995ae

Update protocol contracts

d0c04d4

Fix pxe setup

3e66c1e

Minor fixes

153762b

Fix wrong PXE call that has been broken for over a year

24eb6f8

Fixes after another rebase

369fb71

spalladino force-pushed the palla/validate-rpc-inputs branch from 16b649c to 369fb71 Compare November 9, 2024 02:47

Do not default listen addr to IPv4 if not set on startHttpServer

51f104d

Some processes may prefer IPv6 for localhost. Like the nargo external oracle resolver when run in earthly.

spalladino enabled auto-merge (squash) November 9, 2024 19:52

ludamad reviewed Nov 10, 2024

View reviewed changes

ludamad approved these changes Nov 10, 2024

View reviewed changes

spalladino merged commit 6554122 into master Nov 10, 2024
66 checks passed

spalladino deleted the palla/validate-rpc-inputs branch November 10, 2024 04:49

AztecBot mentioned this pull request Nov 10, 2024

chore(master): Release 0.63.0 #9651

Merged

ludamad added a commit that referenced this pull request Nov 11, 2024

Revert "chore: Validate RPC inputs (#9672)"

63492d1

This reverts commit 6554122.

ludamad mentioned this pull request Nov 11, 2024

chore: Revert "chore: Validate RPC inputs" #9875

Merged

ludamad added a commit that referenced this pull request Nov 11, 2024

chore: Revert "chore: Validate RPC inputs" (#9875)

dd83d52

Reverts #9672 as it broke some e2e tests and boxes

spalladino mentioned this pull request Nov 11, 2024

chore: Validate RPC inputs reloaded #9878

Merged

spalladino added a commit that referenced this pull request Nov 11, 2024

chore: Validate RPC inputs reloaded (#9878)

70ab7c4

Second attempt at #9672 after it broke some e2e tests on master.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Validate RPC inputs #9672

chore: Validate RPC inputs #9672

spalladino commented Nov 1, 2024 •

edited

Loading

ludamad Nov 10, 2024

ludamad Nov 10, 2024

ludamad Nov 10, 2024

ludamad Nov 10, 2024

ludamad Nov 10, 2024

ludamad Nov 10, 2024

ludamad Nov 10, 2024

ludamad commented Nov 10, 2024 •

edited

Loading

	/** Schemas for prover node API functions. */
	export const ProverNodeApiSchema: ApiSchemaFor<ProverNodeApi> = {
	getJobs: z
	.function()
	.args()
	.returns(z.array(z.object({ uuid: z.string(), status: z.enum(EpochProvingJobState) }))),

	startProof: z.function().args(schemas.Integer).returns(z.void()),

	prove: z.function().args(schemas.Integer).returns(z.void()),

	sendEpochProofQuote: z.function().args(EpochProofQuote.schema).returns(z.void()),
	};

	static get schema() {
	return z
	.object({
	archive: AppendOnlyTreeSnapshot.schema,
	header: Header.schema,
	body: Body.schema,
	})
	.transform(({ archive, header, body }) => new L2Block(archive, header, body));
	}

	beforeEach(async () => {
	handler = new MockProverNode();
	context = await createJsonRpcTestSetup<ProverNodeApi>(handler, ProverNodeApiSchema);
	});

	afterEach(() => {
	tested.add(/^ProvingNodeApiSchema\s+([^(]+)/.exec(expect.getState().currentTestName!)![1]);
	context.httpServer.close();
	});

	afterAll(() => {
	const all = Object.keys(ProverNodeApiSchema);
	expect([...tested].sort()).toEqual(all.sort());
	});

	it('getJobs', async () => {
	const jobs = await context.client.getJobs();
	const expected = await handler.getJobs();
	expect(jobs).toEqual(expected);
	});

		@@ -1,337 +0,0 @@
		import cors from '@koa/cors';
		import http from 'http';

chore: Validate RPC inputs #9672

chore: Validate RPC inputs #9672

Conversation

spalladino commented Nov 1, 2024 • edited Loading

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad Nov 10, 2024

Choose a reason for hiding this comment

ludamad commented Nov 10, 2024 • edited Loading

spalladino commented Nov 1, 2024 •

edited

Loading

ludamad commented Nov 10, 2024 •

edited

Loading