Skip to content

feat(avm): tx hint init#13218

Merged
fcarreiro merged 1 commit intomasterfrom
fc/avm-tx-hint
Apr 3, 2025
Merged

feat(avm): tx hint init#13218
fcarreiro merged 1 commit intomasterfrom
fc/avm-tx-hint

Conversation

@fcarreiro
Copy link
Contributor

Please read contributing guidelines and remove this line.

Copy link
Contributor Author

fcarreiro commented Apr 1, 2025

@fcarreiro fcarreiro requested a review from dbanks12 April 1, 2025 16:48
@fcarreiro fcarreiro changed the title feat(avm): tx hint feat(avm): tx hint init Apr 1, 2025
@fcarreiro fcarreiro force-pushed the fc/avm-sequentialinsert-hints branch from a0c5866 to 2965864 Compare April 1, 2025 17:17
@fcarreiro fcarreiro force-pushed the fc/avm-sequentialinsert-hints branch from 2965864 to 6592cea Compare April 2, 2025 10:26
@fcarreiro fcarreiro force-pushed the fc/avm-tx-hint branch 2 times, most recently from 0ae6700 to b1b0287 Compare April 2, 2025 11:54
@fcarreiro fcarreiro force-pushed the fc/avm-sequentialinsert-hints branch 2 times, most recently from 1aa0ebd to 748972a Compare April 2, 2025 14:24
@fcarreiro fcarreiro force-pushed the fc/avm-sequentialinsert-hints branch from 748972a to a5282e1 Compare April 2, 2025 14:51
@fcarreiro fcarreiro force-pushed the fc/avm-sequentialinsert-hints branch from a5282e1 to b354130 Compare April 2, 2025 15:45
@fcarreiro fcarreiro force-pushed the fc/avm-tx-hint branch 2 times, most recently from b164182 to fb6cf57 Compare April 2, 2025 16:14
Base automatically changed from fc/avm-sequentialinsert-hints to master April 2, 2025 17:53
@fcarreiro fcarreiro marked this pull request as ready for review April 2, 2025 17:56
@fcarreiro fcarreiro enabled auto-merge April 2, 2025 17:56
Copy link
Contributor

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

LGTM. Few comments, but none blocking a merge

Comment on lines +197 to +200
struct AccumulatedData {
// TODO: add as needed.
std::vector<FF> noteHashes;
std::vector<FF> nullifiers;
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO meaning "add the remaining side effects as needed?"

Copy link
Contributor

Choose a reason for hiding this comment

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

And do we need two variations of AccumulatedData like we have in public inputs?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll respond in the other thread.

Comment on lines +207 to +223
// We are currently using this structure as the input to TX simulation.
// That's why I'm not calling it TxHint. We can reconsider if the inner types seem to dirty.
struct Tx {
AccumulatedData nonRevertibleAccumulatedData;
AccumulatedData revertibleAccumulatedData;
std::vector<EnqueuedCallHint> setupEnqueuedCalls;
std::vector<EnqueuedCallHint> appLogicEnqueuedCalls;
std::optional<EnqueuedCallHint> teardownEnqueuedCall;

bool operator==(const Tx& other) const = default;

MSGPACK_FIELDS(nonRevertibleAccumulatedData,
revertibleAccumulatedData,
setupEnqueuedCalls,
appLogicEnqueuedCalls,
teardownEnqueuedCall);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this eventually become a complete mirror of the TS TX? Or this will be some subset including only what is needed for our purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK. It looked like we didn't need everything from the TX object in TS, so I guess this could be a subset? Also, if I wanted to respect the same structure (or if I want to serialize the whole TX) then I have to go in TS and implement (de)serialization/schemas for everything that is in it, even if we don't use it.

For the moment I've opted to add what we need, but it can be refactored later if needed. Modifying serialization is really not that difficult now with MP/zod, but it's easier to do it incrementally, I find. Also it lets us look at the structure and see what we already solved and what not (of course not completely true since we don't use note hashes now ;)).

Comment on lines +84 to +85
context.hints.tx = AvmTxHint.fromTx(tx);

Copy link
Contributor

Choose a reason for hiding this comment

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

nice :)

// The reason we need enqueued hints at all (and cannot just use the public inputs) is
// because they don't have the actual calldata, just the hash of it.
// If/when we pass the whole TX to C++, we can remove this class of hints.
stateManager.traceEnqueuedCall(callRequest.request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... i guess we still need to trace the enqueued call because tracing it means "hint that this enqueued call actually happened"? Otherwise, the TX should include everything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep tracing is for PIs I guess? Didn't look too much in detail, maybe it can be removed.

@fcarreiro fcarreiro added this pull request to the merge queue Apr 3, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 3, 2025
@fcarreiro fcarreiro added this pull request to the merge queue Apr 3, 2025
Merged via the queue into master with commit 60a1a92 Apr 3, 2025
8 checks passed
@fcarreiro fcarreiro deleted the fc/avm-tx-hint branch April 3, 2025 23:15
github-merge-queue bot pushed a commit that referenced this pull request Apr 4, 2025
🤖 I have created a new Aztec Packages release
---


##
[0.84.0](v0.83.1...v0.84.0)
(2025-04-04)


### ⚠ BREAKING CHANGES

* `UnsconstrainedContext` --> `UtilityContext`
([#13246](#13246))
* `#[utility]` function
([#13243](#13243))
* Validate public setup fns and gas in p2p
([#13154](#13154))

### Features

* `#[utility]` function
([#13243](#13243))
([945ffa2](945ffa2))
* **avm:** tx hint init
([#13218](#13218))
([60a1a92](60a1a92))
* Remove 4 byte metadata from bb-produced proof
([#13231](#13231))
([0dcc915](0dcc915))
* To enable better ci dashboard.
([#13272](#13272))
([61c6375](61c6375))


### Bug Fixes

* **avm:** fix lookup builder and FF hashing
([#13263](#13263))
([2633856](2633856))
* ci3-external concurrency bug, reduce grind set
([2c5e830](2c5e830)),
closes
[#13285](#13285)
* ci3-external.yml
([#13291](#13291))
([6ad68ed](6ad68ed))
* Validate public setup fns and gas in p2p
([#13154](#13154))
([1ef4add](1ef4add)),
closes
[#10958](#10958)


### Miscellaneous

* `UnsconstrainedContext` --&gt; `UtilityContext`
([#13246](#13246))
([69df86f](69df86f))
* add some PrivateSet tests
([#13270](#13270))
([bd9e690](bd9e690))
* bump full prover test to 32 cores. hoping to boost speed.
([#13293](#13293))
([c8e95dd](c8e95dd))
* deflake p2p reqresp test
([#13271](#13271))
([b9164fa](b9164fa))
* don't dump on fail. click the link instead.
([#13292](#13292))
([ba0fb4d](ba0fb4d))
* flake
([#13277](#13277))
([62c32eb](62c32eb))
* make rahul happy with migration notes
([#13255](#13255))
([3dd75a6](3dd75a6))
* minor simulator utils cleanup
([#13250](#13250))
([8a622c9](8a622c9))
* move a couple of `SharedMutableValues` functions outside of impl
([#13283](#13283))
([df9a40c](df9a40c))
* nuking debug-only logger and various unused functionality in
`foundation`
([#13187](#13187))
([2d38e60](2d38e60))
* prevent eth devnet config contention in ci
([#13260](#13260))
([1581836](1581836))
* renaming unconstrained function as utility in TS
([#13249](#13249))
([34d03bb](34d03bb))
* replace relative paths to noir-protocol-circuits
([b5b99f8](b5b99f8))
* Speed up note hashes test
([#13282](#13282))
([ad23358](ad23358))
* update gov and proposer configs
([#13281](#13281))
([e1a5be3](e1a5be3))
* update slashing test port
([#13274](#13274))
([9a1ddc5](9a1ddc5))
* Want to fail fast on test runs and the wider ci run.
([#13258](#13258))
([f0553b8](f0553b8))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants