feat: ExecutionTaggingIndexCache
#17445
Conversation
benesjan
force-pushed
the
10-02-feat_executiontaggingindexcache
branch
from
a72fad4 to
d18c017
Compare
benesjan
changed the base branch from
graphite-base/17445
to
10-02-refactor_tagging_related_cleanup
benesjan
changed the title
ExecutionTaggingIndexCache
benesjan
force-pushed
the
10-02-feat_executiontaggingindexcache
branch
3 times, most recently
from
6e27300 to
9c9464f
Compare
benesjan
force-pushed
the
10-02-refactor_tagging_related_cleanup
branch
from
bb2d7d4 to
6aa046a
Compare
noir-projects/noir-contracts/contracts/test/test_log_contract/src/main.nr
Show resolved
Hide resolved
noir-projects/noir-contracts/contracts/test/test_log_contract/src/main.nr
Show resolved
Hide resolved
noir-projects/noir-contracts/contracts/test/test_log_contract/src/main.nr
Show resolved
Hide resolved
| * Return the tagging indexes incremented by this execution along with the directional app tagging secrets. | ||
| */ | ||
| public getIndexedTaggingSecrets(): IndexedTaggingSecret[] { | ||
| return this.taggingIndexCache.getIndexedTaggingSecrets(); | ||
| } |
There was a problem hiding this comment.
This solidified me not liking the 'indexed tagging secret' name - I just don't really know what it means. The 'tagging secret' part is particuarly annoying because it hides a lot of stuff and makes me have to think about the complexity of the tag derivation. Like, the tagging secret is not really indexed - that'd be the tag. What this is is the 'app siloed sender/recipient shared secret'. Perhaps we need to come up with a name for the thing that we combine with an index to get a tag, e.g. the 'pretag', 'tag seed', or whatever.
At any rate, I'd also change the name of this function, which is not very descriptive. Get which tagging secrets? I'd call it 'getUsedTags' or something nicer that conveys that these are the tags that this execution consumed and should be a) tracked by tx and b) never used again.
There was a problem hiding this comment.
You have a point. The IndexedTaggingSecret name is indeed bad as the point of it is not indexing a secret (indexing in a sense of it being easily obtainable from a data store) but it is either used as a preimage of a tag or as a data container to send to the db to update the next index to be used for a given directional app tagging secret.
As to the name of the function it's quite confusing here because it's not the last used indexes but it's the indexes that are to be used in a tx. This is done because it makes the tagging data provider simpler as then we never need to return undefined from the getNextIndexAsSender function as by returning next we can just return 0 in the beginning:
But anyway I think it would be a good way to change this as handling undefined in 1 place is better then having all these function confusing.
I propose the following:
- Rename
IndexedTaggingSecretasPreTag- it's a preimage of a tag so I think this name is clear, - refactor all the functions to return last used tag instead of the next one as it's clearer,
- rename this function to
getUsedPreTags.
Doing all these refactor would explode the scope of this PR so I would do that in a followup one.
Sounds good?
yarn-project/pxe/src/contract_function_simulator/oracle/private_execution_oracle.ts
Outdated
Show resolved
Hide resolved
yarn-project/pxe/src/contract_function_simulator/execution_tagging_index_cache.ts
Outdated
Show resolved
Hide resolved
benesjan
changed the base branch from
10-02-refactor_tagging_related_cleanup
to
graphite-base/17445
benesjan
force-pushed
the
10-02-feat_executiontaggingindexcache
branch
from
4de3631 to
90032df
Compare
benesjan
changed the base branch from
graphite-base/17445
to
10-02-refactor_tagging_related_cleanup
|
@nventuro I had to rebase this since I heavily modified the PR down the stack but all the changes I did since your last review are just in these commits so feel free to just check those: 
|
I started working on improving unconstrained tagging as proposed in [this forum post](https://forum.aztec.network/t/on-note-discovery-and-index-coordination/7165) and the initial step in implementing that is the following: 1. store the tagging indexes requested during execution in a cache (just like we do with notes), 2. when the execution is finished take the cache and along with a tx hash dump it into db. Given that the cache will reside in `PrivateExecutionOracle` I now need to orchestrate the sync there. When trying to do that (to be done in a [PR up the stack](#17445)) I realized that the current tagging-related API was cumbersome and this made me do the following in this PR: 1. Have the API accept directionalAppTaggingSecret on the input instead of appTaggingSecret and recipient as now I can just compute it in the `PrivateExecutionOracle` and request info based on that, 2. I moved the tagging functionality from stdlib to PXE as there was no reason to have it in stdlib, 3. I simplified a bunch of random things. This PR is a bit random so if something is not clear LMK.
AztecBot
force-pushed
the
10-02-refactor_tagging_related_cleanup
branch
from
2fb6ac9 to
cb1c968
Compare
benesjan
changed the base branch from
10-02-refactor_tagging_related_cleanup
to
graphite-base/17445
I started working on improving unconstrained tagging as proposed in [this forum post](https://forum.aztec.network/t/on-note-discovery-and-index-coordination/7165) and the initial step in implementing that is the following: 1. store the tagging indexes requested during execution in a cache (just like we do with notes), 2. when the execution is finished take the cache and along with a tx hash dump it into db. Given that the cache will reside in `PrivateExecutionOracle` I now need to orchestrate the sync there. When trying to do that (to be done in a [PR up the stack](#17445)) I realized that the current tagging-related API was cumbersome and this made me do the following in this PR: 1. Have the API accept directionalAppTaggingSecret on the input instead of appTaggingSecret and recipient as now I can just compute it in the `PrivateExecutionOracle` and request info based on that, 2. I moved the tagging functionality from stdlib to PXE as there was no reason to have it in stdlib, 3. I simplified a bunch of random things. This PR is a bit random so if something is not clear LMK.
benesjan
force-pushed
the
graphite-base/17445
branch
from
2fb6ac9 to
afc1ca4
Compare
benesjan
force-pushed
the
10-02-feat_executiontaggingindexcache
branch
from
fd39b63 to
cd31177
Compare
Fixes #15753 In this PR I implement an ExecutionTaggingIndexCache which stores the used indexes during an execution and at the end we store them in the database. By storing that info once execution is done we'll be able to have indexes associated with a transaction hash. This is a necessary step towards improving of unconstrained tagging as described in [this post](https://forum.aztec.network/t/on-note-discovery-and-index-coordination/7165). Given that once this PR is merged we will store the indexes in the database only for executions and not for simulations this PR should fix #15753.
AztecBot
force-pushed
the
10-02-feat_executiontaggingindexcache
branch
from
cd31177 to
bad2246
Compare
As [discussed](#17445 (comment)) in a PR down the stack returning the next tagging index in various places of tagging was confusing and it's better to return the last used one. In this PR I do this change. Some of the code is quite weird but I think it doesn't make sense to invest time in that as I will replace all the tagging sync it all in followup PRs.
Renamed `IndexedTaggingSecret` as `PreTag` as the original name was confusing. See [this conversation](#17445 (comment)) for more context.
Fixes #15753
In this PR I implement an ExecutionTaggingIndexCache which stores the used indexes during an execution and at the end we store them in the database. By storing that info once execution is done we'll be able to have indexes associated with a transaction hash. This is a necessary step towards improving of unconstrained tagging as described in this post.
Given that once this PR is merged we will store the indexes in the database only for executions and not for simulations this PR should fix #15753.