feat: sync tags as sender #10071
feat: sync tags as sender #10071
Conversation
sklppy88
force-pushed
the
ek/feat/9762/find-next-tag-as-sender
branch
from
c52fc20
to
cbfa05a
Compare
sklppy88
force-pushed
the
ek/feat/9762/find-next-tag-as-sender
branch
2 times, most recently
from
49ac3de
to
5435af5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
sklppy88
force-pushed
the
ek/feat/9762/find-next-tag-as-sender
branch
from
5435af5
to
8b53b70
Compare
There was a problem hiding this comment.
Hm, I'm unsure as to the overall approach. I get wanting to reuse stuff, but this feels quite odd. syncTaggedLogs returns logs and has some side effects (the indices get advanced), but here we're ignoring the retun value, and we're making it have a different set of side effects (since it's the sender indices that move forward, not the recipient ones).
syncTaggedLogs is also fairly complicated for reasons we don't care about here: it deals with multiple senders and recipients (while we just have the one), has a sliding window that moves back (we only need to move forward), etc.
It feels to me like we could just have a way simpler version in which we just read the current index in the database, queries the node for logs and moves the index forward until it doesn't find any logs, with some forward window to be able to jump over skipped indices.
| async #getAppTaggingSecretAsSender( | ||
| contractAddress: AztecAddress, | ||
| sender: AztecAddress, | ||
| recipient: AztecAddress, | ||
| ): Promise<IndexedTaggingSecret> { | ||
| const senderCompleteAddress = await this.getCompleteAddress(sender); | ||
| const senderIvsk = await this.keyStore.getMasterIncomingViewingSecretKey(sender); | ||
|
|
||
| const sharedSecret = computeTaggingSecret(senderCompleteAddress, senderIvsk, recipient); | ||
| const appTaggingSecret = poseidon2Hash([sharedSecret.x, sharedSecret.y, contractAddress]); | ||
| const [index] = await this.db.getTaggingSecretsIndexesAsSender([appTaggingSecret]); | ||
|
|
||
| return new IndexedTaggingSecret(appTaggingSecret, index); | ||
| } |
There was a problem hiding this comment.
Why reimplement all of this instead of calling calculateTaggingSecret?
There was a problem hiding this comment.
Just did it as an analog of the other function without realizing calculateTaggingSecret was the actual corresponding function, was a bit confused. Removed this in favor of calculateTaggingSecret that you pointed out.
sklppy88
force-pushed
the
ek/feat/9762/find-next-tag-as-sender
branch
4 times, most recently
from
2d60a0d
to
1778062
Compare
Ah this is a very good point. I had just wanted to maximize reuse of code, like you said. I didn't realize though that the tradeoff for this maximization was this large. I have made the changes you suggested and made it much simpler. I also did not implement a window though, because I think it would be okay to just use the first found open index for now, as I think we will take another pass at this logic when adapting it for reorgs. |
sklppy88
force-pushed
the
ek/feat/9762/find-next-tag-as-sender
branch
2 times, most recently
from
aa56f47
to
621ffab
Compare
|
I would add the |
sklppy88
force-pushed
the
ek/feat/9762/find-next-tag-as-sender
branch
from
b1c28a4
to
0aa3b96
Compare
There was a problem hiding this comment.
I also did not implement a window though, because I think it would be okay to just use the first found open index for now, as I think we will take another pass at this logic when adapting it for reorgs.
We already support reorgs as of #9913, and #10071 will trigger an index reset. If we choose the first empty index then any reverted transaction will result in the sender and recipient being desynced until the sender sends sufficient notes to cover the gap.
We must use a forward sliding window with the same size as the recipient, and only stop looking for indices once we find a full window with no tags (which is the same thing the recipient does).
| sender: AztecAddress, | ||
| recipient: AztecAddress, | ||
| ): Promise<void> { | ||
| const contractName = await this.contractDataOracle.getDebugContractName(contractAddress); |
There was a problem hiding this comment.
I'd move this below next to the log statement
This PR enables simple multi-device support by simply syncing the tagging secret indexes when fetching calculating the tag latest tag to use as a sender. The calculated tag uses the largest found index (recent), whether it be from on device, or from remote.
Resolves #9762.