feat: reuse encrypted data to avoid memory allocation #242
Conversation
src/crypto/streaming.ts
Outdated
| const encrypted = chunk.subarray(i, end) | ||
| // reuse the encrypted Uint8Array to avoid memory allocation | ||
| const dst = chunk.subarray(i, end - TAG_LENGTH) | ||
| const { plaintext: decrypted, valid } = await handshake.decrypt(encrypted, handshake.session, dst) |
There was a problem hiding this comment.
I get that this is the main feature of this PR.
This seems strange though, basically using the same buffer as both the input and the output.
It seems that by writing where we're reading, the input could be modified before/during processing?
There was a problem hiding this comment.
@wemeetagain yeah agree that this looks strange, but if this is proved to be correct, for the performance reason (as shared in the PR description) I think we can go with it.
It seems that by writing where we're reading, the input could be modified before/during processing?
no, input is read byte by byte 1 time and immediately set to output, see https://github.com/StableLib/stablelib/blob/a89a438fcbf855de6b2e9faa2630f03c3f3b3a54/packages/chacha/chacha.ts#L174
in assemblyscript implementation, I have a lot of unit tests for it https://github.com/ChainSafe/as-chacha20poly1305/pull/1/files#diff-25252846b58979dcaf4e41d47b3eadd7e4f335e7fb98da6c049b1f9cd011f381R48
I will add more comments for the context
twoeths
force-pushed
the
tuyen/reuse-decrypt-allocation
branch
from
24b2de9 to
7e9e0aa
Compare
|
Testing this PR along with |
@wemeetagain I found no issue with this branch after testing on some lodestar nodes, see ChainSafe/lodestar#4787 (comment) |
Motivation
As tested in lodestar, memory allocation is expensive for
decrypt()flowDescription
dst?: Uint8Arrayparameter in its chacha20poly1305 implementation, the newas-chacha20poly1305should follow the same interface