This is probably the most involved change in the sequence of changes extracted from #10785.
In order to implement the differential encoding of BIP152, this change changes VectorFormatter to permit a stateful sub-formatter, which is then used by DifferenceFormatter. A CustomUintFormatter is added as well to do the 48-bit serialization of short ids.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
557- explicit CCompactSize(uint64_t& nIn) : n(nIn) { }
558+ template<typename Stream, typename I>
559+ void Unser(Stream& s, I& v)
560+ {
561+ static_assert(std::is_unsigned<I>::value, "CompactSize only supported for unsigned integers");
562+ static_assert(std::numeric_limits<I>::max() <= std::numeric_limits<uint64_t>::max(), "CompactSize only supports 64-bit integers and below");
In commit “Convert CCompactSize to proper formatter” (8b8ebf016c1de3c3827beff7666c0adb628355bf)
I’d consider dropping the static asserts here, because they don’t really make sense for deserializing (they do make sense in the Ser function below). I don’t thinks a reason to disallow deserializing into variables wider than 64 bits or signed variables if the value fits.
563
564- template<typename Stream>
565- void Serialize(Stream &s) const {
566- WriteCompactSize<Stream>(s, n);
567+ uint64_t n = ReadCompactSize<Stream>(s);
568+ if (n > std::numeric_limits<I>::max()) {
In commit “Convert CCompactSize to proper formatter” (8b8ebf016c1de3c3827beff7666c0adb628355bf)
It’s unlikely to make a difference in practice unless I is some crazy type that starts above 0, but I’d add || n < std::numeric_limits<I>::min() to be complete here
10@@ -11,17 +11,60 @@
11 class CTxMemPool;
12
13 // Dumb helper to handle CTransaction compression at serialize-time
14-struct TransactionCompressor {
15-private:
16- CTransactionRef& tx;
17-public:
18- explicit TransactionCompressor(CTransactionRef& txIn) : tx(txIn) {}
19+struct TransactionCompression
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
I think you could drop this class and use DefaultFormatter below instead
25+struct Uint48Formatter
26+{
27+ template <typename Stream, typename I> void Ser(Stream& s, I v)
28+ {
29+ static_assert(std::is_unsigned<I>::value, "Uint48Formatter needs an unsigned integer");
30+ static_assert(sizeof(I) >= 6, "Uint48Formatter needs a 48+ bit type");
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
Would drop these lines. There should be no problem serializing a value smaller than 48 bits as 48 bits (or serializing a signed value value that’s positive). I think a runtime check would be more appropriate:
0if (v >> 48 || v < 0) {
1 throw std::ios_base::failure("Uint48Formatter value out of range");
2}
36
37- ADD_SERIALIZE_METHODS;
38+ template <typename Stream, typename I> void Unser(Stream& s, I& v)
39+ {
40+ static_assert(std::is_unsigned<I>::value, "Uint48Formatter needs an unsigned integer");
41+ static_assert(sizeof(I) >= 6, "Uint48Formatter needs a 48+ bit type");
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
These checks seem too strict. Could just check for a big enough range and not care about the format
0static_assert(std::numeric_limits<I>::max() >= 0xffffffffffff && std::numeric_limits<I>::min() <= 0, "Uint48Formatter needs a 48+ bit type");
54+{
55+ uint16_t m_subtract = 0;
56+
57+ uint16_t operator()(uint16_t val)
58+ {
59+ uint16_t tmp = val - m_subtract;
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
Should this raise an error if val <= tmp? It seems like the serializer shouldn’t create a sequence that the deserializer would reject.
I think the code is written assuming that the data fed to the serializer is sane.
I’ve merged the two differential transforms into one; I think this is more readable too.
52+
53+struct DifferentialSerTransform
54+{
55+ uint16_t m_subtract = 0;
56+
57+ uint16_t operator()(uint16_t val)
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
Maybe not worth extra effort or complexity, but I think the transforms would be safer and actually more understandable if they were templated. Safer because by hardcoding uint16_t as the sequence and diff types, lets c++ get away with truncating wider values to 16 bits with no checking. I think the ideal would look something like:
0template<typename DT> struct DifferentialSerTransform {
1 template<typename T> DT operator()(T val);
2};
3
4template<typename T> struct DifferentialUnserTransform {
5 template<typename DT> T operator()(DT diff);
6};
VectorFormatter needs the input and output of the transform to be identical (or at least compatible)… suspect there is a way to make it infer what the input type to the transform on deserialization is, but right now, it creates a V::value_type to deserialize into.
114- inline void SerializationOp(Stream& s, Operation ser_action) {
115- uint64_t idx = index;
116- READWRITE(COMPACTSIZE(idx));
117- if (idx > std::numeric_limits<uint16_t>::max())
118+ template<typename Stream>
119+ void Unserialize(Stream& s)
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
There should be no need to have custom serialize and deserialize methods, and do overflow checking with a temporary variable now that CompactSizeFormatter has the same checking. The following seems to be sufficient:
0SERIALIZE_METHODS(PrefilledTransaction, obj) { READWRITE(COMPACTSIZE(obj.index), obj.tx); }
197- if (BlockTxCount() > std::numeric_limits<uint16_t>::max())
198+ template <typename Stream>
199+ inline void Unserialize(Stream& s)
200+ {
201+ static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids serialization assumes 6-byte shorttxids");
202+ s >> header >> nonce >> Using<VectorFormatter<Uint48Formatter>>(shorttxids) >> prefilledtxn;
In commit “Convert blockencodings.h to new serialization framework” (d46dfeccaf1992ba92bff7e0d4988ae3f66b327a)
Maybe a matter of preference, but it might be nice to avoid needing to repeat s << header... ss >> header... serialization order. Could do:
0SERIALIZE_METHODS(CBlockHeaderAndShortTxIDs, obj)
1{
2 static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids serialization assumes 6-byte shorttxids");
3 READWRITE(obj.header, obj.nonce, Using<VectorFormatter<Uint48Formatter>>(obj.shorttxids), obj.prefilledtxn);
4 if (ser_action.ForRead()) {
5 if (obj.BlockTxCount() > std::numeric_limits<uint16_t>::max()) {
6 throw std::ios_base::failure("indexes overflowed 16 bits");
7 }
8 obj.FillShortTxIDSelector();
9 }
10}
Ah indeed, that works because shorttxidkX are mutable, which they probably shouldn’t be. But no need to fix that here.
Done.
Code review ACK 40fdd5faefe835a9868be1a667511780811a814e. Changes since last review were just suggestions tweaking asserts and switching to SERIALIZE_METHODS. Also, there’s a new combined DifferenceTransform class.
I think you could go further and make DifferenceTransform into a formatter: f7778799685a0d3d70cf6e18d0911e676cc02b90 (branch). This is nice because it collapses transforming and formatting into one function and does more size/overflow checking
@ryanofsky That’s amazing, it looks so much cleaner than my approach. I’ve taken your code and restructured it as commits in this PR.
I’ve also replaced Uint48Formatter with a CustomUintFormatter in serialize.h instead, as a separate commit.
519+ static_assert(Bytes > 0 && Bytes <= 8, "CustomUintFormatter Bytes out of range");
520+ static constexpr uint64_t MASK = 0xffffffffffffffff >> (8 * (8 - Bytes));
521+
522+ template <typename Stream, typename I> void Ser(Stream& s, I v)
523+ {
524+ if (v < 0 || (v & ~I(MASK)) != 0) throw std::ios_base::failure("CustomUintFormatter value out of range");
In commit “Add CustomUintFormatter” (96b18829d750baea8577cf409d85c0cd5186b67c)
Could replace (v & ~I(MASK)) != 0 with v <= MASK to be more consistent with other assert and avoid making an assumption about representation of v.
520+ static constexpr uint64_t MAX = 0xffffffffffffffff >> (8 * (8 - Bytes));
521+
522+ template <typename Stream, typename I> void Ser(Stream& s, I v)
523+ {
524+ if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
525+ uint64_t raw = htole64(v);
Can you give an example of how it would fail?
htole64 converts a number in host native representation (= the value the uint64_t input has in C) to 64-bit LE representation. On LE systems htole64 is a no-op. On BE systems it performs a byteswap on the representation of the number.