Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change DecryptNotesResponse to use a sparse serialization format #5049

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

andiflabs
Copy link
Contributor

Summary

In the vast majority of the cases, a DecryptNotesResponse will contain zero or close-to-zero decrypted notes. It makes sense to have the serialization of DecryptNotesResponse optimize for those cases.

This changes the serialization size of DecryptNotesResponse from O(null_notes + non_null_notes) to O(non_null_notes).

This introduces a small size overhead in the case where the are a lot of decrypted notes. We could in theory have DecryptNotesResponse choose between sparse and dense serialization based on how many decrypted notes there are, but that case is so rare, and the overhead so small, that it's not worth optimizing for it.

Testing Plan

Unit tests

Documentation

N/A

Breaking Change

N/A

@andiflabs andiflabs requested a review from a team as a code owner June 17, 2024 19:42
const notes = []

const arrayLength = reader.readU32()
const notes = Array(arrayLength).fill(null) as Array<DecryptedNote | null>
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to return the null notes at all?

maybe i'm missing something but it looks like we always filter them out

Copy link
Contributor Author

@andiflabs andiflabs Jun 17, 2024

Choose a reason for hiding this comment

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

Yes: upper-level code needs to know which nodes exactly were successfully decrypted. Here's an example: suppose you want to decrypt all notes in this block:

block:
  transaction A: [note 1, note 2, note 3, note 4]
  transaction B: [note 5, note 6]
  transaction C: [note 7, note 8]

You can submit a decrypt request with all the notes:

[note 1, note 2, note 3, note 4, note 5, note 6, note 7, note 8]

Once you get a response, you need to know which notes exactly were successfully decrypted, so that you can match them with the corresponding transactions. This is done by comparing the index of the non-null notes in the response, with the index of the notes in the request.

If this response returned only non-null values, it wouldn't be possible to decrypt an entire block at once (or even multiple blocks).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative approach that I considered is to change the input from just note to (block, transaction, note), and repeat the block/transaction information in the response. This however would greatly increase the size of requests, and that's why I didn't go that route.

@andiflabs andiflabs force-pushed the andrea/compact-decrypt-requests branch 2 times, most recently from 80b26db to 5ebbad5 Compare June 17, 2024 23:24
@andiflabs andiflabs force-pushed the andrea/compact-decrypt-responses branch from e904c6a to d9ede87 Compare June 17, 2024 23:24
@andiflabs andiflabs force-pushed the andrea/compact-decrypt-requests branch from 5ebbad5 to f475882 Compare June 18, 2024 22:12
In the vast majority of the cases, a `DecryptNotesResponse` will contain
zero or close-to-zero decrypted notes. It makes sense to have the
serialization of `DecryptNotesResponse` optimize for those cases.

This changes the serialization size of `DecryptNotesResponse` from
`O(null_notes + non_null_notes)` to `O(non_null_notes)`.

This introduces a small size overhead in the case where the are a lot of
decrypted notes. We could in theory have `DecryptNotesResponse` choose
between sparse and dense serialization based on how many decrypted notes
there are, but that case is so rare, and the overhead so small, that
it's not worth optimizing for it.
@andiflabs andiflabs force-pushed the andrea/compact-decrypt-responses branch from d9ede87 to a820f30 Compare June 18, 2024 22:13
Base automatically changed from andrea/compact-decrypt-requests to staging June 20, 2024 18:47
@andiflabs andiflabs merged commit a820f30 into staging Jun 20, 2024
10 checks passed
@andiflabs andiflabs deleted the andrea/compact-decrypt-responses branch June 20, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants