Add storable_builder helper for client side encryption #14

G8XSU · 2023-11-02T00:23:41Z

Add storable_builder helper for client side encryption
Note: All of ChaCha20Poly1305 code is copied from rust-lightning repo. (I deleted code which was specifically added for LDK)
Note: decrypt_in_place function didn't exist in chacha20poly1305, it is a new addition, reviewers should review it.

tnull

So far just had a look at the first commit. Generally checked that the files generally match (mod reformatting), but have two questions regarding the differences.

tnull · 2023-11-03T14:05:27Z

src/crypto/chacha20poly1305.rs

+			self.mac.raw_result(out_tag);
+		}
+
+		pub fn encrypt_inplace(&mut self, input_output: &mut [u8], out_tag: &mut [u8]) {


This new addition seems to be identical to the existing encrypt_full_message_in_place. Can we keep the existing name, so that we eventually know they are the same method when migrating back to rust-lightning?

i found encrypt_in_place more applicable than encrypt_full_message_in_place (i think message here refers to peer-to-peer msgs)
But i am ok with changing it.
This one is not a new addition, only decrypt_inplace is new addition.

src/crypto/chacha20poly1305.rs

jkczyz · 2023-11-03T14:52:32Z

Note: All of ChaCha20Poly1305 code is copied from rust-lightning repo. (I deleted code which was specifically added for LDK)

Note: decrypt_in_place function didn't exist in chacha20poly1305, it is a new addition, reviewers should review it.

Pardon if this has already discussed, but is it worth refactoring code into a new crate to avoid copying? (cc: @TheBlueMatt)

We had a similar issue with some code shared across crates in the rust-lightning repo, but we were able to use symlinking there. Unfortunately, the same can't be done across repos.

G8XSU · 2023-11-03T17:35:55Z

@jkczyz

We can't really depend on rust-lightning repo/crate for chacha20 encryption.

I did discuss it with Matt and Elias earlier,
if we don't want to copy code we can consider adding a dependency on some encryption library.
But it was argued that it is just 1 / 2-file and we can copy it. (and since it should never change, it is ok)

tnull · 2023-11-06T09:25:29Z

FWIW, lightningdevkit/rust-lightning#2708 will add the decrypt_in_place functionality upstream, so yo should be good just copy/pasting the files from upstream once that is merged. This has the benefit that you can just drop them entirely again when the VSS client itself is upstreamed.

jkczyz · 2023-11-06T14:59:34Z

FWIW, lightningdevkit/rust-lightning#2708 will add the decrypt_in_place functionality upstream, so yo should be good just copy/pasting the files from upstream once that is merged. This has the benefit that you can just drop them entirely again when the VSS client itself is upstreamed.

Oh, I wasn't aware that we were upstreaming this. Do you mean the entire vss-rust-client repo or something else?

tnull · 2023-11-06T15:44:10Z

Oh, I wasn't aware that we were upstreaming this. Do you mean the entire vss-rust-client repo or something else?

I thought there were plans around doing that at least eventually, i.e. have VssStore be part of rust-lightning? Or maybe I'm mistaken?

G8XSU · 2023-11-06T17:45:51Z

Yup, I just saw that we are upstreaming decrypt_in_place. :)

vss-rust-client will not be upstreamed (this PR is vss-rust-client), VssStore will be upstreamed to rust-lightning eventually.

Cargo.toml

src/util/storable_builder.rs

jkczyz · 2023-11-08T23:26:47Z

src/util/storable_builder.rs

+		Storable {
+			data: data_blob,
+			encryption_metadata: Option::from(EncryptionMetadata {
+				nonce: nonce.to_vec(),
+				tag: tag.to_vec(),
+				cipher_format: CHACHA20_CIPHER_NAME.to_string(),
+			}),


Is this result essentially immediately serialized and written to disk? From an allocation perspective, it's a bit unfortunate we have an intermediary object requiring a few Vecs if the alternative is to use something like Writeable.

almost in most cases yes.
main constituent of it is data_blob, which is encrypted_in_place and decrypted_in_place, and then used as it is in storable.
others are just 96bytes and 16bytes. from allocation perspective, i don't think it will allocate unless we encode it. it will just hold references to it.

iiuc, even if it was writeable,
it would be serializing vec's one after other into one big VecWriter just before writing when encode() is called.

almost in most cases yes. main constituent of it is data_blob, which is encrypted_in_place and decrypted_in_place, and then used as it is in storable. others are just 96bytes and 16bytes. from allocation perspective, i don't think it will allocate unless we encode it. it will just hold references to it.

Any Vec creation other than an empty one results in a heap allocation. So there are two when you use to_vec here and another heap allocation for to_string on CHACHA20_CIPHER_NAME.

iiuc, even if it was writeable, it would be serializing vec's one after other into one big VecWriter just before writing when encode() is called.

If we didn't use Storable, then we could serialize nonce, tag, and CHACHA20_CIPHER_NAME without creating any new Vecs or Strings . True that it would ultimately put everything into a Vec, so at least one allocation is required.

If we used a rust struct implementing Writeable instead of a proto, we could avoid the unnecessary allocations.

src/util/storable_builder.rs

jkczyz · 2023-11-09T15:36:38Z

src/util/storable_builder.rs

+		Storable {
+			data: data_blob,
+			encryption_metadata: Option::from(EncryptionMetadata {
+				nonce: nonce.to_vec(),
+				tag: tag.to_vec(),
+				cipher_format: CHACHA20_CIPHER_NAME.to_string(),
+			}),


almost in most cases yes. main constituent of it is data_blob, which is encrypted_in_place and decrypted_in_place, and then used as it is in storable. others are just 96bytes and 16bytes. from allocation perspective, i don't think it will allocate unless we encode it. it will just hold references to it.

Any Vec creation other than an empty one results in a heap allocation. So there are two when you use to_vec here and another heap allocation for to_string on CHACHA20_CIPHER_NAME.

iiuc, even if it was writeable, it would be serializing vec's one after other into one big VecWriter just before writing when encode() is called.

If we didn't use Storable, then we could serialize nonce, tag, and CHACHA20_CIPHER_NAME without creating any new Vecs or Strings . True that it would ultimately put everything into a Vec, so at least one allocation is required.

If we used a rust struct implementing Writeable instead of a proto, we could avoid the unnecessary allocations.

src/util/storable_builder.rs

jkczyz · 2023-11-10T14:18:08Z

src/util/storable_builder.rs

-				nonce: nonce.to_vec(),
-				tag: tag.to_vec(),
+				nonce: Vec::from(nonce),
+				tag: Vec::from(tag),


These still result in heap allocations. A new array is created on the heap in Vec within a Box type and then the data is copied in because arrays implement Copy. Ultimately, the nonce and tag memory is already stack allocated.

Other than foregoing the use of the proto altogether or using an arena (if possible), the only way to avoid excessive heap allocations would be to allocate one Storable as a member of StorableBuilder and reuse its Vec fields by taking slices to use with fill_bytes and encrypt_inplace. Then you would only need to allocate one String for cipher_format, too. But the caller would need to also reuse StorableBuilder, and build couldn't return a Storable any more. Instead, the interface would be in terms of bytes and Storable would be an implementation detail.

Hmm.. yeah, missed the box::new inside of implementation.

Storable as a member of StorableBuilder

Yeah but that introduces a state which mutates per request, making it non-thread safe. I think it will complicate everything.

But what you suggested gave me idea for another approach.
Basically it will avoid Copy and array allocation but not vec allocation. We will create vec's in the first place and use references for fill_bytes and encrypt_inplace, similar to what you suggest.

Since these vec's are fixed size pre-allocated, i think it should be fine?

(Overall i think it shouldn't be a big concern since it is around 28bytes and we allocate much more during just a single gossip msg forward)

(From my limited understanding of glibc allocator, the optimization achieved by using a single Storable as field is easily achieved at allocator level i think. Repeated small allocations will reuse the same small-bins/fast-bins/tcache. It might not avoid alloc traffic but those operations are meant to be fast.
As long as number of small allocations don't keep on growing it is fine.)

Yeah but that introduces a state which mutates per request, making it non-thread safe. I think it will complicate everything.

You could use a Storable per thread, FWIW.

But what you suggested gave me idea for another approach. Basically it will avoid Copy and array allocation but not vec allocation. We will create vec's in the first place and use references for fill_bytes and encrypt_inplace, similar to what you suggest.

Since these vec's are fixed size pre-allocated, i think it should be fine?

Eh, it removes the stack allocations, which are essentially free, and the copy, yeah.

(Overall i think it shouldn't be a big concern since it is around 28bytes and we allocate much more during just a single gossip msg forward)

The larger concern is not the allocation size but rather any resulting heap fragmentation. I guess your argument is that in comparison to other parts of the code (e.g., gossip) this is trivial or at least similar, IIUC. Most gossip messages are indeed larger but are allocated largely on the stack. Not entirely though as they do contain a Vec field here and there (e.g., features), so I'd buy your argument that perhaps this isn't too much to worry about from a frequency of small allocation perspective.

(From my limited understanding of glibc allocator, the optimization achieved by using a single Storable as field is easily achieved at allocator level i think. Repeated small allocations will reuse the same small-bins/fast-bins/tcache. It might not avoid alloc traffic but those operations are meant to be fast. As long as number of small allocations don't keep on growing it is fine.)

I'm not knowledgable enough here to say what would happen in practice. Essentially, we'll have three small, fixed-size heap allocations for the Vecs and String, which are free'ed whenever the Storeable is freed. Presumably the caller will call encode immediately to pass the value to the KVStore. Afterwards, the Storeable is freed upon drop and thus so are the small allocations.

The question is, will the allocator reuse those small pieces of memory the next time a Storable is created? And will there be much heap fragmentation in the interim?

@TheBlueMatt I'm indifferent on keeping the proto. Seems it's not a whole lot different from other places in the code where there may be comparable allocation patterns (e.g., gossip forwarding). But let me know if I'm missing anything.

Most gossip messages are indeed larger but are allocated largely on the stack.

No they are heap allocated, MessageBuf is just an abstraction over Vec link

I did a small instrumentation, for 2000,000 (2million) back to back 16-byte vector allocations, there are only 530 real allocations, 530 bins/slots that get re-used. (Those are 16byte mallocs again and again)

At no point in time did real memory cross those 530 alloc worth space. (so no frag risk i guess, even though there was alloc traffic)

No they are heap allocated, MessageBuf is just an abstraction over Vec link

FWIW, I was referring to the gossip messages after they have been deserialized from a MessageBuf. Those structs' fields are mostly (but not entirely) primitives or other structs that don't require heap allocation.

jkczyz

Just some minor comments. Feel free to squash fixups and any changes addressing these comments.

src/util/storable_builder.rs

G8XSU force-pushed the crypto branch 2 times, most recently from b68759d to 8d14179 Compare November 2, 2023 00:29

G8XSU requested a review from jkczyz November 2, 2023 00:29

G8XSU marked this pull request as draft November 2, 2023 00:51

G8XSU force-pushed the crypto branch 2 times, most recently from 513f7d3 to 3017e9a Compare November 2, 2023 01:11

G8XSU marked this pull request as ready for review November 2, 2023 01:19

G8XSU changed the title ~~Add Storable_Builder helper for client side encryption~~ Add storable_builder helper for client side encryption Nov 2, 2023

tnull reviewed Nov 3, 2023

View reviewed changes

jkczyz reviewed Nov 8, 2023

View reviewed changes

Add ChaCha20Poly1305 encryption helper

232415b

G8XSU force-pushed the crypto branch from 3017e9a to 3aba612 Compare November 9, 2023 03:10

jkczyz reviewed Nov 9, 2023

View reviewed changes

jkczyz reviewed Nov 10, 2023

View reviewed changes

G8XSU requested a review from jkczyz November 14, 2023 01:57

jkczyz reviewed Nov 14, 2023

View reviewed changes

Add storable_builder helper

10630c1

G8XSU force-pushed the crypto branch from 921f4f1 to 10630c1 Compare November 14, 2023 21:30

G8XSU requested a review from jkczyz November 14, 2023 21:31

jkczyz approved these changes Nov 15, 2023

View reviewed changes

G8XSU merged commit 3a26735 into lightningdevkit:main Nov 16, 2023

G8XSU mentioned this pull request Nov 28, 2023

Patch release 0.1.2 #18

Merged

jkczyz mentioned this pull request Dec 1, 2023

Trivial ChaCha cleanups lightningdevkit/rust-lightning#2764

Merged

Add storable_builder helper for client side encryption #14

Add storable_builder helper for client side encryption #14

Uh oh!

Conversation

G8XSU commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz commented Nov 3, 2023

Uh oh!

G8XSU commented Nov 3, 2023

Uh oh!

tnull commented Nov 6, 2023

Uh oh!

jkczyz commented Nov 6, 2023

Uh oh!

tnull commented Nov 6, 2023

Uh oh!

G8XSU commented Nov 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

G8XSU commented Nov 2, 2023 •

edited

Loading

tnull left a comment •

edited

Loading

G8XSU Nov 3, 2023 •

edited

Loading

G8XSU Nov 10, 2023 •

edited

Loading