The next step of changes from #10785.
This one adds:
@ryanofsky I added another small improvement here that wasn't present in #10785, namely reusing the VectorUsing logic in the vector/prevector serializers themselves (avoiding code duplication).
24 | @@ -25,6 +25,9 @@ 25 | 26 | static const unsigned int MAX_SIZE = 0x02000000; 27 | 28 | +/** Maximum amount of memory to allocate at once when deserializing vectors. */
nit: specify units?
Done.
Code review ACK 24235e50e9d532cbe5836dcebbce0c6a04be8652
722 | @@ -673,6 +723,16 @@ inline void Unserialize(Stream& is, T&& a) 723 | a.Unserialize(is); 724 | } 725 | 726 | +/** Identity formatter. Serializes objects as themselves. */
Maybe add a comment when this is to be used, on first glance it seems like a no-op.
Done.
629 | + size_t allocated = 0; 630 | + while (allocated < size) { 631 | + // For DoS prevention, do not blindly allocate as much as the stream claims to contain. 632 | + // Instead, allocate in 5MiB batches, so that an attacker actually needs to provide 633 | + // X MiB of data to make us allocate X+5 Mib. 634 | + allocated = std::min(size, allocated + MAX_VECTOR_ALLOCATE / sizeof(typename V::value_type));
A static assert that sizeof(typename V::value_type) <= MAX_VECTOR_ALLOCATE could catch an edge case here.
Done.
635 | + v.reserve(allocated); 636 | + while (v.size() < allocated) { 637 | + typename V::value_type val; 638 | + auto wrapped = Using<Formatter>(val); 639 | + s >> wrapped; 640 | + v.push_back(std::move(val));
In commit "Add custom vector-element formatter" (741e27bfe5eb6bdc61f99b300c16f4b2f92e8ec7)
Could avoid creating the temporary:
v.emplace_back();
s >> Using<Formatter>(v.back());
Done.
98 | - // TODO: avoid reimplementing vector deserializer 99 | - uint64_t count = 0; 100 | - ::Unserialize(s, COMPACTSIZE(count)); 101 | - if (count > MAX_INPUTS_PER_BLOCK) { 102 | - throw std::ios_base::failure("Too many input undo records"); 103 | - }
In commit "Convert undo.h to new serialization framework" (24235e50e9d532cbe5836dcebbce0c6a04be8652)
Note: With this check going away, there doesn't appear to be any limit on the total size of the deserialized vprevout vector. The incremental allocation size is now limited to MAX_VECTOR_ALLOCATE, though.
Yeah, I'm aware. I don't think this is significant. The earlier code needed a sanity limit, and had custom code, so it used a custom limit. The new code is safe by default (even though the limit in practice is much higher).
608 | + * 609 | + * V is not required to be an std::vector type. It works for any class that 610 | + * exposes a value_type, size, reserve, push_back, and const iterators. 611 | + */ 612 | +template<class Formatter> 613 | +struct VectorUsing
I don't think the VectorUsing and UsingSelf names for formatters are the best. Would suggest sticking to the FooFormatter convention and renaming VectorUsing to VectorFormatter and UsingSelf to DefaultFormatter.
Aside from giving more consistent class names, it should also make usages more readable: Using<VectorUsing<UsingSelf>>(v) -> Using<VectorFormatter<DefaultFormatter>>(v)
That's indeed better. Done.
Code review ACK 24235e50e9d532cbe5836dcebbce0c6a04be8652. Left minor suggestions, feel free to ignore
This allows a very compact notation for serialization of vectors whose
elements are not serialized using their default encoding.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
628 | + size_t size = ReadCompactSize(s); 629 | + size_t allocated = 0; 630 | + while (allocated < size) { 631 | + // For DoS prevention, do not blindly allocate as much as the stream claims to contain. 632 | + // Instead, allocate in 5MiB batches, so that an attacker actually needs to provide 633 | + // X MiB of data to make us allocate X+5 Mib.
Lines 632 and 633, so the documentation doesn't go out of date if the value of MAX_VECTOR_ALLOCATE changes, perhaps replace "5 MiB" and "5 Mib" with "MAX_VECTOR_ALLOCATE" (and s/X Mib/X/ as the constant is defined in bytes).
630 | + while (allocated < size) { 631 | + // For DoS prevention, do not blindly allocate as much as the stream claims to contain. 632 | + // Instead, allocate in 5MiB batches, so that an attacker actually needs to provide 633 | + // X MiB of data to make us allocate X+5 Mib. 634 | + static_assert(sizeof(typename V::value_type) <= MAX_VECTOR_ALLOCATE, "Vector element size too large"); 635 | + allocated = std::min(size, allocated + MAX_VECTOR_ALLOCATE / sizeof(typename V::value_type));
<strike>Now that the static assert suggested in #18021 (review) has been added, possibly worthwhile (or not) to calculate sizeof(typename V::value_type) once and cache it.</strike>
Noting that the result of sizeof is always nonzero, even if applied to an empty class type, so (normally) no risk of division by zero.
Unless I'm confused, sizeof(..) is determined at compile-time, so no reason to cache it.
Oops yes, static_assert requires a bool constexpr, otherwise a compile-time error is issued.
Qualified ACK 3c94b0039d2c
Code review, built/ran tests/ran bitcoind. I am not yet highly competent wrt C++ templates but this looks like a worthwhile refactoring. Would review again. Some thoughts below, feel free to ignore.
Code review ACK 3c94b0039d2ca2a8c41fd6127ff5019a2afc304e. Changes since last review: renaming formatter classes, adding suggested static_assert, and removing temporary in VectorFormatter
code review ACK 3c94b0039d2ca2a8c41fd6127ff5019a2afc304e
