From cdabd85c07cc301bba7a85d3475600d5d368a903 Mon Sep 17 00:00:00 2001 From: esau <152162806+sklppy88@users.noreply.github.com> Date: Thu, 21 Nov 2024 20:58:41 +0000 Subject: [PATCH] fix: make bytecode part of artifact hash preimage again (#9771) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this PR, we are simply trying to re-introduce the bytecode hash as a part of the preimage to the function artifact hash. I had tried to reproduce the weird noir non-deterministic bytecode bug that @spalladino documented [here](https://aztecprotocol.slack.com/archives/C053490AV6V/p1713480846092319?thread_ts=1713445232.979779&cid=C053490AV6V) referenced in #5860 but was unable to do so. I see only matched bytecode hashes on local and CI: •noir-projects+build-contracts bar_far, Pasted Graphic 6 If this has been already fixed by the noir team it may make sense that this should be merged in to simply re-add the security check. --- .../circuits.js/src/contract/artifact_hash.test.ts | 4 ++-- yarn-project/circuits.js/src/contract/artifact_hash.ts | 9 ++++----- .../contract/private_function_membership_proof.test.ts | 3 +-- .../unconstrained_function_membership_proof.test.ts | 3 +-- yarn-project/pxe/src/pxe_service/pxe_service.ts | 5 +---- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/yarn-project/circuits.js/src/contract/artifact_hash.test.ts b/yarn-project/circuits.js/src/contract/artifact_hash.test.ts index 0fc093ed44b..74b7db7c485 100644 --- a/yarn-project/circuits.js/src/contract/artifact_hash.test.ts +++ b/yarn-project/circuits.js/src/contract/artifact_hash.test.ts @@ -30,7 +30,7 @@ describe('ArtifactHash', () => { for (let i = 0; i < 1000; i++) { expect(computeArtifactHash(testArtifact).toString()).toMatchInlineSnapshot( - `"0x237feccc8e34a39c0e5133c8653fc278b39275bfa3f7459e4aba07d53b752c19"`, + `"0x21070d88558fdc3906322f267cf6f0f632caf3949295520fe1f71f156fbb0d0b"`, ); } }); @@ -43,7 +43,7 @@ describe('ArtifactHash', () => { const testArtifact = loadContractArtifact(content); expect(computeArtifactHash(testArtifact).toString()).toMatchInlineSnapshot( - `"0x237feccc8e34a39c0e5133c8653fc278b39275bfa3f7459e4aba07d53b752c19"`, + `"0x21070d88558fdc3906322f267cf6f0f632caf3949295520fe1f71f156fbb0d0b"`, ); }); }); diff --git a/yarn-project/circuits.js/src/contract/artifact_hash.ts b/yarn-project/circuits.js/src/contract/artifact_hash.ts index de23fe9594d..a7bc52ae7ad 100644 --- a/yarn-project/circuits.js/src/contract/artifact_hash.ts +++ b/yarn-project/circuits.js/src/contract/artifact_hash.ts @@ -92,11 +92,10 @@ export function computeFunctionArtifactHash( | (Pick & { functionMetadataHash: Fr; selector: FunctionSelector }), ) { const selector = 'selector' in fn ? fn.selector : FunctionSelector.fromNameAndParameters(fn); - // TODO(#5860): make bytecode part of artifact hash preimage again - // const bytecodeHash = sha256Fr(fn.bytecode).toBuffer(); - // const metadataHash = 'functionMetadataHash' in fn ? fn.functionMetadataHash : computeFunctionMetadataHash(fn); - // return sha256Fr(Buffer.concat([numToUInt8(VERSION), selector.toBuffer(), metadataHash.toBuffer(), bytecodeHash])); - return sha256Fr(Buffer.concat([numToUInt8(VERSION), selector.toBuffer()])); + + const bytecodeHash = sha256Fr(fn.bytecode).toBuffer(); + const metadataHash = 'functionMetadataHash' in fn ? fn.functionMetadataHash : computeFunctionMetadataHash(fn); + return sha256Fr(Buffer.concat([numToUInt8(VERSION), selector.toBuffer(), metadataHash.toBuffer(), bytecodeHash])); } export function computeFunctionMetadataHash(fn: FunctionArtifact) { diff --git a/yarn-project/circuits.js/src/contract/private_function_membership_proof.test.ts b/yarn-project/circuits.js/src/contract/private_function_membership_proof.test.ts index 2135967d9d4..3a57e9787b2 100644 --- a/yarn-project/circuits.js/src/contract/private_function_membership_proof.test.ts +++ b/yarn-project/circuits.js/src/contract/private_function_membership_proof.test.ts @@ -31,8 +31,7 @@ describe('private_function_membership_proof', () => { expect(isValidPrivateFunctionMembershipProof(fn, contractClass)).toBeTruthy(); }); - // TODO(#5860): Re-enable this test once noir non-determinism is addressed - test.skip.each([ + test.each([ 'artifactTreeSiblingPath', 'artifactMetadataHash', 'functionMetadataHash', diff --git a/yarn-project/circuits.js/src/contract/unconstrained_function_membership_proof.test.ts b/yarn-project/circuits.js/src/contract/unconstrained_function_membership_proof.test.ts index 57495f78aea..f795c6f29b6 100644 --- a/yarn-project/circuits.js/src/contract/unconstrained_function_membership_proof.test.ts +++ b/yarn-project/circuits.js/src/contract/unconstrained_function_membership_proof.test.ts @@ -50,8 +50,7 @@ describe('unconstrained_function_membership_proof', () => { expect(isValidUnconstrainedFunctionMembershipProof(fn, contractClass)).toBeTruthy(); }); - // TODO(#5860): Re-enable this test once noir non-determinism is addressed - test.skip.each(['artifactTreeSiblingPath', 'artifactMetadataHash', 'functionMetadataHash'] as const)( + test.each(['artifactTreeSiblingPath', 'artifactMetadataHash', 'functionMetadataHash'] as const)( 'fails proof if %s is mangled', field => { const proof = createUnconstrainedFunctionMembershipProof(selector, artifact); diff --git a/yarn-project/pxe/src/pxe_service/pxe_service.ts b/yarn-project/pxe/src/pxe_service/pxe_service.ts index 1a5d83c4787..bfbfd0cf689 100644 --- a/yarn-project/pxe/src/pxe_service/pxe_service.ts +++ b/yarn-project/pxe/src/pxe_service/pxe_service.ts @@ -250,10 +250,7 @@ export class PXEService implements PXE { `Artifact does not match expected class id (computed ${contractClassId} but instance refers to ${instance.contractClassId})`, ); } - if ( - // Computed address from the instance does not match address inside instance - !computeContractAddressFromInstance(instance).equals(instance.address) - ) { + if (!computeContractAddressFromInstance(instance).equals(instance.address)) { throw new Error('Added a contract in which the address does not match the contract instance.'); }