feat: hash contract class logs with poseidon + add preimage to blob by MirandaWood · Pull Request #12061 · AztecProtocol/aztec-packages

MirandaWood · 2025-02-17T23:36:57Z

~~Quick and dirty~~ Somewhat clean implementation which:

Converts our SHA based contract class LogHashes with poseidon hashes and constrain them in the registerer.
Verifies this ^ hash in the base rollup and appends to the blob as fields, eliminating the need for any calldata.
Treats the logs as fields in ts instead of bytes.

EDIT: Added log validation to tx in this commit: cb739a2

EDIT 2: We may not need to constrain the hash in the registerer, so can revert to the oracle hashing the log and leaving the constraining to the base rollup in a future PR.

MirandaWood · 2025-02-18T13:38:16Z

noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr

+        // TODO(MW): use chunked poseidon if cheaper? Use variable poseidon with length of non-0 fields?
        let log_hash =
-            unsafe { emit_contract_class_unencrypted_log_private(contract_address, log, counter) };
+            std::hash::poseidon2::Poseidon2::hash(log_to_emit, CONTRACT_CLASS_LOG_SIZE_IN_FIELDS);
+        // Safety: TODO(MW): constraining below length in the base rollup. Is this good/safe?
+        let length = unsafe { find_index_hint_from_end(log_to_emit, |elem| !is_empty(elem)) };
+        // Safety: the below only exists to broadcast the raw log, so we can provide it to the base rollup later to be constrained.


Two Qs here:

I'm just hashing the entire (padded) log here because it's easy and clean. I could hash just the N input fields. This is a bit more expensive in the circuits (not by much, I'd use the chunk variable hash) but might be a lot faster elsewhere. I already have code to hash N of M fields, and check the remaining M - N fields are all zero.

I'm carrying the log length (N) throughout the kernels to track gas since we are only adding N things to the blob. I'm not sure this is even required atm because logs are set to cost 0? I'm also not constraining this length until the base rollup when we verify the hash. I'm not sure if this is ok.

I'd potentially go with the unpadded approach, since our recent discussions (via that hackmd) suggest that eventually (probably in a separate PR), we don't actually need to hash in this circuit at all.

We probably do need to track the length, because ultimately these logs should cost a relative fortune. And eventually (as per prev paragraph) only the Base will need to constrain this length (because we'll remove constrained hashing of this log from private-land).

because we'll remove constrained hashing of this log from private-land

Shall I just do this now, and make the oracle hash again? I don't remember now - but wasn't there some reason we weren't happy with the original oracle hash even though we verified the hash later?

Sorry for the delay; I need to sort out my notification settings.

Shall I just do this now, and make the oracle hash again?

Can do! It can either be an oracle, or just make the function unconstrained? But you messaged 14 hours ago, so maybe you've already chosen an approach, which I'll find out when I read further down this page :)

wasn't there some reason we weren't happy with the original oracle hash even though we verified the hash later?

I can't remember 😬

Just FYI Alvaro found the unpadded hash is insecure unless we also constrain the length in the registeter (which just adds gates. Fixed len hash is about the same gates). From @sirasistant:

I think that opens the attack vector:

Private chooses a shorter length

Private only hashes the shorter length

Base rollup receives the shorter preimage

Hashes match

We publish incomplete bytecode

I willl revert to a fixed length hash

yarn-project/archiver/src/archiver/kv_archiver_store/log_store.ts

yarn-project/archiver/src/archiver/memory_archiver_store/memory_archiver_store.ts

Tracking an address alongside the cc log to aid filters in the archiver. The address is always constrained to be that of the registerer, but many archiver tests use random addresses so they brick without ugly hacks

iAmMichaelConnor · 2025-02-25T14:51:49Z

👀

MirandaWood · 2025-02-26T11:46:04Z

yarn-project/end-to-end/scripts/docker-compose.yml

Sorry - this file is changed by yarn format and I forgot to remove from staged - nothing to do with PR. If the next CI run passes, I'll remove it.

iAmMichaelConnor

Wooooh I read it all! Thanks for doing all this! It's quite scary how much of the codebase needs to be touched to change a very very thin "slice" of functionality. It's essentially just changing a public input of the private context, and it blows up into thousands of lines.
Anyway, I've written comments, and a lot of them will be because this is the first time I'm looking at most of these files. :)

iAmMichaelConnor · 2025-02-25T20:03:54Z

noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr

+        let log_hash = poseidon2_hash_subarray_variable(log_to_emit, length);
+        // Safety: the below only exists to broadcast the raw log, so we can provide it to the base rollup later to be constrained.
+        unsafe {
+            emit_contract_class_unencrypted_log_private(contract_address, log_to_emit, counter);


Forewarning, I still don't really know my way around the codebase or our naming conventions.
Forewarning, sometimes my review comments are pedantic.
I think I've seen a naming pattern for other oracles which just inform typescript about something (e.g. when lining up a hint for some later kernel / rollup circuit): the word "notify" is used. Perhaps this oracle function should be called notify_....

I've not really touched the oracle that much, but since we actually add state in ts which is different to the state tracked here (raw log vs hashed log), maybe emit still stands. I think the notify is different but unsure?

iAmMichaelConnor · 2025-02-25T20:06:11Z

noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr

@@ -217,13 +220,18 @@ pub contract ContractClassRegisterer {
        // cannot prove non-registration. Therefore, it is possible that a malicious oracle might prevent sequencers


I think some of this comment is out of date now, since your special poseidon2_hash_subarray_variable function is able to hash the whole thing in-circuit, so the purpose of the oracle is now just to inform typescript, rather than its previous purpose as a hack to compute the log_hash.

True, I was hoping the reviewer would give me some guidance on this. If we move the hash to the oracle / unconstrained as planned this comment is still correct isn't it? Shall I change it then change it back?

noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr

yarn-project/noir-protocol-circuits-types/src/conversion/server.ts

iAmMichaelConnor · 2025-02-26T12:00:33Z

yarn-project/stdlib/src/logs/contract_class_log.ts

+    // TODO(MW): Lazily used /2 instead of CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS, because this keeps overfilling block blobs
+    // Below line gives error 'Type instantiation is excessively deep and possibly infinite. ts(2589)'
+    // makeTuple(CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS, Fr.random);
+    const fields = Array.from({ length: Math.ceil(CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS / 2) }, () => Fr.random());
+    return new ContractClassLog(
+      await AztecAddress.random(),
+      fields.concat(Array.from({ length: Math.floor(CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS / 2) }, () => Fr.ZERO)),
+    );


Maybe comment something like "Only half of the contract class log's fields get filled with random values here (ideally all of the fields would be filled with random values), because...". It took my brain a sec to understand this.

Can we not ignore the error, like I saw you do elsewhere?

No - it's a nightmare because they are tuples, not arrays. I couldn't get tuples defined in our usual way working at all

iAmMichaelConnor · 2025-02-26T12:03:30Z

yarn-project/stdlib/src/logs/contract_class_log.ts

+  }
+
+  async hash() {
+    return await poseidon2Hash(this.getEmittedFields());


Maybe comment that this is one of two approaches to hashing this data, to disambiguate from the weird AVM way of hashing the data.

I don't think these logs are ever hashed in the AVM way at all? Information about functions is hashed in the AVM way, but not these logs AFAIK

iAmMichaelConnor · 2025-02-26T12:05:02Z

yarn-project/stdlib/src/tx/tx.ts


+  /**
+   * Gets either revertible or non revertible contract class logs emitted by this tx.
+   * @param revertible - true for revertible only logs, false for non reverible only logs.


Oh gosh, I didn't even pay attention to any logic that discerned between rev and non-rev in the .nr files...

I didn't need to change it because before this PR we use a log hash in the kernels. After this PR we... use a log hash in the kernels :)

LeilaWang

Agree with some of Mike's comments, but maybe they can be addressed in followup prs. It looks great to me!

After this: #12061 we were overflowing the trace on contract class registrations. This was not caught by tests due to: - Insufficient tests with full proving (and the ones we have deploy without proving!) - Insufficient WASM testing: this overflow caused the overflowing trace to go over 4GB

After this: AztecProtocol/aztec-packages#12061 we were overflowing the trace on contract class registrations. This was not caught by tests due to: - Insufficient tests with full proving (and the ones we have deploy without proving!) - Insufficient WASM testing: this overflow caused the overflowing trace to go over 4GB

After this: #12061 we were overflowing the trace on contract class registrations. This was not caught by tests due to: - Insufficient tests with full proving (and the ones we have deploy without proving!) - Insufficient WASM testing: this overflow caused the overflowing trace to go over 4GB

MirandaWood added 2 commits February 17, 2025 23:35

feat: hash cc log w poseidon, add raw log to rollup + ts

2031c58

feat: final changes, fix hash, cleanup

2160e38

MirandaWood marked this pull request as ready for review February 18, 2025 11:32

MirandaWood added 3 commits February 18, 2025 11:55

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

a00ab47

chore: aaaa safety doc change again

03adfc7

chore: aaaa safety doc change again again

6057a35

MirandaWood commented Feb 18, 2025

View reviewed changes

yarn-project/archiver/src/archiver/kv_archiver_store/log_store.ts Outdated Show resolved Hide resolved

MirandaWood commented Feb 18, 2025

View reviewed changes

yarn-project/archiver/src/archiver/memory_archiver_store/memory_archiver_store.ts Outdated Show resolved Hide resolved

MirandaWood added 13 commits February 19, 2025 09:57

feat: add cc log validation to tx

cb739a2

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

f8fafa0

chore: post merge fixes, fmt

b6d797a

chore: fixtures + more import fixing

ccb7be7

feat: use unpadded variable hash for cc logs

6d2349d

fix: update tests

56b8cd1

feat: add .contractaddr to contract class log (#12197)

c2ebf15

Tracking an address alongside the cc log to aid filters in the archiver. The address is always constrained to be that of the registerer, but many archiver tests use random addresses so they brick without ugly hacks

chore: move cc log to /logs/ to prep for conflicts

7cd0932

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

2dcc661

chore: post merge fmt and fixes

edf15af

chore: aaa lint

36edf5a

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

fee44dc

chore: post merge fmt

5bc9634

MirandaWood requested a review from iAmMichaelConnor February 25, 2025 13:15

MirandaWood added 4 commits February 26, 2025 09:02

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

9685104

chore: post merge fixes, fmt

9764a0e

chore: remove docker fmt i guess

6a4a0da

chore: more fixes (bootstrap no longer full build)

7593562

MirandaWood commented Feb 26, 2025

View reviewed changes

iAmMichaelConnor reviewed Feb 26, 2025

View reviewed changes

MirandaWood added 3 commits February 26, 2025 18:11

feat: address some comments

81920fd

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

e26bb79

chore: post merge fixes

530f013

LeilaWang approved these changes Feb 26, 2025

View reviewed changes

MirandaWood mentioned this pull request Feb 27, 2025

chore: contract class log refactor follow up & cleanup #12325

Closed

MirandaWood and others added 2 commits February 27, 2025 16:51

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

34a43bf

Merge remote-tracking branch 'origin/master' into mw/cc-logs-pos-hash

799d842

LeilaWang enabled auto-merge (squash) February 27, 2025 18:18

MirandaWood added 2 commits February 27, 2025 18:53

chore: fix merges and empty array lens

a9f8424

fix: i am stupid

3e70e8a

LeilaWang merged commit 44c07a3 into master Feb 27, 2025
9 checks passed

LeilaWang deleted the mw/cc-logs-pos-hash branch February 27, 2025 19:34

MirandaWood mentioned this pull request Mar 3, 2025

feat: exploring contract class logs as fields #11632

Closed

Thunkar mentioned this pull request Mar 24, 2025

fix: increased poseidon gates #12973

Merged

		@@ -217,13 +220,18 @@ pub contract ContractClassRegisterer {
		// cannot prove non-registration. Therefore, it is possible that a malicious oracle might prevent sequencers

Conversation

MirandaWood commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iAmMichaelConnor commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeilaWang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MirandaWood commented Feb 17, 2025 •

edited

Loading