Don't over-allocate invoice bytes #3494

jkczyz · 2024-12-17T23:02:08Z

When allocating space for an invoice's bytes, it was assumed that the bytes used for the invoice request signature are the same length as those used for the invoice's signature. However, fuzz testing revealed that this isn't always the case since an invoice request could contain more than one signature TLV. Account for this when determining the number of bytes to allocate for the invoice. This comes at the expense of an additional traversal of all TLVs in the invoice request through the end of SIGNATURE_TYPES (i.e., every TLV except experimental ones).

TheBlueMatt · 2024-12-18T00:27:11Z

Can we just always allocate a fixed 512/1024/2048/4096 bytes and call it a day? Over-allocating for an object that's not gonna stick around long seems fine (and over-allocating by <2x is generally pretty fine as it could reduce memory fragmentation to offset the additional allocated size.

jkczyz · 2024-12-18T14:41:43Z

Sure, though we still need to calculate the size and round to the nearest power of 2, if I understand what you're proposing.

jkczyz · 2024-12-18T18:24:44Z

FYI, I made the adjustment. The first two commits will need to be squashed if this looks good. Did the same for when building offers and refunds.

TheBlueMatt · 2024-12-19T13:37:49Z

Oh, no, I was proposing we allocate enough space for any reasonable invreq and if we under-allocate cause someone is doing something nuts that's okay.

jkczyz · 2024-12-19T15:06:32Z

Oh, no, I was proposing we allocate enough space for any reasonable invreq and if we under-allocate cause someone is doing something nuts that's okay.

Gotcha. Note that the failing fuzzer was from creating an invoice from an invreq, though.

TheBlueMatt · 2024-12-19T15:43:48Z

Err, either way yea. If this code is gonna cause problems I vote we replace with a constant. Sadly for invoices over-allocating may bloat memory somewhat as some users are likely to keep them around for a while, but invreqs much less so. If we can get the over-allocation under, like, 1.5x in the vast majority of cases (seems doable?) then I'd prefer that...

Of course if you prefer to fix it and keep it, that's okay with me, you're the one writing the PR :)

TheBlueMatt · 2025-01-12T22:05:29Z

Would be good to backport this in 0.1.1 since its currently causing fuzzers to fail on the 0.1 branch, which I'm not a particular fan of :)

vincenzopalazzo

LGTM, outside of the compilation failure but they looks trivial to fix

   --> /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/alloc/src/vec/mod.rs:422:5
    = help: items from traits can only be used if the trait is implemented and in scope
note: `WithRoundedCapacity` defines an item `with_rounded_capacity`, perhaps you need to implement it
   --> lightning/src/offers/alloc.rs:11:1
    |
11  | pub(super) trait WithRoundedCapacity {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: there is an associated function `with_capacity` with a similar name
    |
502 |         let mut experimental_bytes = Vec::with_capacity(
    |                                           ~~~~~~~~~~~~~

error[E0599]: no function or associated item named `with_rounded_capacity` found for struct `alloc::vec::Vec<_, _>` in the current scope
   --> lightning/src/offers/refund.rs:342:24
    |
342 |         let mut bytes = Vec::with_rounded_capacity($self.refund.serialized_length());
    |                              ^^^^^^^^^^^^^^^^^^^^^ function or associated item not found in `Vec<_, _>`
...
389 |     refund_builder_methods!(self, Self, Self, self, T, mut);
    |     ------------------------------------------------------- in this macro invocation
    |
note: if you're trying to build a new `alloc::vec::Vec<_, _>` consider using one of the following associated functions:

TheBlueMatt · 2025-01-28T14:59:47Z

Guess its not worth delaying 0.1.1 for this.

jkczyz · 2025-01-28T15:04:57Z

Guess its not worth delaying 0.1.1 for this.

Just let me know when you are looking to cut, and I can have it ready prior to then.

TheBlueMatt · 2025-01-28T15:08:20Z

Probably today :). But, really, there's not huge need to get this done in a given release, it just needs to be on the branch so we can fuzz the branch itself.

jkczyz · 2025-01-28T21:07:31Z

Pushed the simpler solution of a fixed-size allocation. Didn't put too much thought into the numbers.

TheBlueMatt · 2025-01-30T21:45:58Z

lightning/src/offers/invoice.rs

-				+ experimental_invoice_tlv_stream.serialized_length(),
-		);
+		const EXPERIMENTAL_TLV_ALLOCATION_SIZE: usize = 512;
+		let mut experimental_bytes = Vec::with_capacity(EXPERIMENTAL_TLV_ALLOCATION_SIZE);


Should we default to no allocation here instead? If we expect these to rarely be used that'd save us an allocation and if we only ever have one TLV then we always only do one allocation anyway, so it'd only cost us an allocation if we have multiple TLVs.

Sure, we can do that. Though the number of allocations will likely be at least two since we write the type and length before writing the value. Depends on how large the value is and how much the initial Vec allocation size is. Rust playground shows 8 bytes.

Are you sure about that? TlvStream iterates over TlvRecords which implement Writeable with a single call to write_all.

Ah, yeah, for experimental TLVs from the offer/invreq (i.e., in experimental_tlv_stream), that is true. For those that we set in the invoice (i.e., anything in experimental_invoice_tlv_stream), it would not be the case though.

Mmm, hmmm, right. We could check if any of the supported experimental fields are set and allocate based on that? I can imagine someone having paid 100k BOLT12s, loading them in memory, and us wasting 48MB plus overhead for them...Basically this would just be a comment in ExperimentalInvoiceTlvStream that we should allocate if we add fields since we don't support any fields outside of test anyway.

TheBlueMatt · 2025-01-30T22:01:25Z

lightning/src/offers/offer.rs

@@ -438,7 +438,8 @@ macro_rules! offer_builder_methods { (
 			}
 		}

-		let mut bytes = Vec::new();
+		const OFFER_ALLOCATION_SIZE: usize = 1024;


Do we need this many bytes? Most of the offers in tests fit within 256 bytes (and are generally even smaller).

Right, I was mistakenly thinking in terms of bech32 encoding and also wanted to give a little extra room. Changed to 512 for offer, refund, and invreq and 1024 for invoices.

Note with a non-compact, two-hop blinded path we are over 256 bytes before bech32 encoding. My numbers might be slightly out-of-date, but it's close enough where I think 256 would be too small.

Instead of using elaborate calculations to determine the exact amount of bytes need for a BOLT12 message are allocated, use a fixed size amount. This reduces the code complexity and potentially reduces heap fragmentation in the normal case.

TheBlueMatt · 2025-02-06T19:09:43Z

There's a number of tests that reserve bytes to pass the old assertions, I think we can remove those now, no?

Now that the previous commit removed assertions on Vec capacities for BOLT12 messages, the use of reserve_exact in tests is no longer needed.

jkczyz · 2025-02-06T20:03:06Z

There's a number of tests that reserve bytes to pass the old assertions, I think we can remove those now, no?

Yup, removed now.

TheBlueMatt

Thanks, gonna go ahead and land this after CI.

TheBlueMatt added this to the 0.1.1 milestone Jan 12, 2025

TheBlueMatt added the backport 0.1 label Jan 12, 2025

vincenzopalazzo approved these changes Jan 13, 2025

View reviewed changes

TheBlueMatt removed this from the 0.1.1 milestone Jan 28, 2025

jkczyz force-pushed the 2024-12-invoice-byte-allocation branch from b415372 to f68a6dd Compare January 28, 2025 21:06

TheBlueMatt reviewed Jan 30, 2025

View reviewed changes

jkczyz force-pushed the 2024-12-invoice-byte-allocation branch from f68a6dd to 2b74373 Compare January 31, 2025 17:16

jkczyz force-pushed the 2024-12-invoice-byte-allocation branch from 2b74373 to ffaccc0 Compare January 31, 2025 21:45

Drop use of reserve_exact in BOLT12 tests

0912b51

Now that the previous commit removed assertions on Vec capacities for BOLT12 messages, the use of reserve_exact in tests is no longer needed.

TheBlueMatt approved these changes Feb 6, 2025

View reviewed changes

TheBlueMatt merged commit f045c0e into lightningdevkit:main Feb 7, 2025
23 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't over-allocate invoice bytes #3494

Don't over-allocate invoice bytes #3494

jkczyz commented Dec 17, 2024

TheBlueMatt commented Dec 18, 2024 •

edited

Loading

jkczyz commented Dec 18, 2024

jkczyz commented Dec 18, 2024

TheBlueMatt commented Dec 19, 2024

jkczyz commented Dec 19, 2024

TheBlueMatt commented Dec 19, 2024 •

edited

Loading

TheBlueMatt commented Jan 12, 2025

vincenzopalazzo left a comment •

edited

Loading

TheBlueMatt commented Jan 28, 2025

jkczyz commented Jan 28, 2025

TheBlueMatt commented Jan 28, 2025

jkczyz commented Jan 28, 2025

TheBlueMatt Jan 30, 2025

jkczyz Jan 31, 2025

TheBlueMatt Jan 31, 2025

jkczyz Jan 31, 2025

TheBlueMatt Jan 31, 2025

jkczyz Jan 31, 2025

TheBlueMatt Jan 30, 2025

jkczyz Jan 31, 2025

TheBlueMatt commented Feb 6, 2025

jkczyz commented Feb 6, 2025

TheBlueMatt left a comment

Don't over-allocate invoice bytes #3494

Don't over-allocate invoice bytes #3494

Conversation

jkczyz commented Dec 17, 2024

TheBlueMatt commented Dec 18, 2024 • edited Loading

jkczyz commented Dec 18, 2024

jkczyz commented Dec 18, 2024

TheBlueMatt commented Dec 19, 2024

jkczyz commented Dec 19, 2024

TheBlueMatt commented Dec 19, 2024 • edited Loading

TheBlueMatt commented Jan 12, 2025

vincenzopalazzo left a comment • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Jan 28, 2025

jkczyz commented Jan 28, 2025

TheBlueMatt commented Jan 28, 2025

jkczyz commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 6, 2025

jkczyz commented Feb 6, 2025

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Dec 18, 2024 •

edited

Loading

TheBlueMatt commented Dec 19, 2024 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading