Add storable_builder helper for client side encryption

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.

[G8XSU]

• 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]

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?

[G8XSU]

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.

[jkczyz]

• 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]

@jkczyz

1. 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]

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]

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]

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]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[jkczyz]

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.

[jkczyz]

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.

[G8XSU]

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.)

[jkczyz]

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.

[G8XSU]

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)

[jkczyz]

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.