Remove double serialization; use software encoder for fee estimation #21966
Conversation
sipa
force-pushed
the
202105_softfloat
branch
2 times, most recently
from
f4232db to
8b0b06c
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
Strong Concept ACK |
|
While I think this is intriguing technically, I'm ~0 on this conceptually I think using floating point in places where 100% precision or portability across platforms is important is mistaken in the first place. This gives the wrong impression. It raises deeper questions for me: do we really need floating point in Bitcoin at all? Narrower: do we really need to serialize/deserialize it? |
|
Haven't checked, but I presume it is only used for fees.dat? |
|
If so, I'd prefer to find an alternative way of serializing those values specifically (as fixed-point or numerator / denominator pairs of integers) and dropping the general float/double (de)serialization. |
As MarcoFalke discovered Looks promising! :) |
|
@laanwj That"s fair. I agree that avoiding serialization of floating point values directly is much more desirable. My thinking here is that it effectively gives us a well-specified serialization with testable properties without needing to break compatibility with existing files. How about rebasing this PR on top of #21981, and making it remove serialization support for |
I was about to comment this! Let's make this code private to the single case where it is used? It keeps the current format but also prevents future serialization of these types 😄 |
sipa
changed the title
|
@laanwj Done. |
|
cr ACK 892522d: patch looks correct :) Very happy to see |
| stream >> f_deserialized; | ||
| assert(f == f_deserialized); | ||
| uint64_t encoded = EncodeDouble(d); | ||
| if constexpr (std::numeric_limits<double>::is_iec559) { |
There was a problem hiding this comment.
Note to other reviewers: We assume this to hold true in assumptions.h:
static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");
| BOOST_CHECK_EQUAL(TestDouble(785.066650390625), 0x4088888880000000ULL); | ||
|
|
||
| // Roundtrip test on IEC559-compatible systems | ||
| if (std::numeric_limits<double>::is_iec559) { |
There was a problem hiding this comment.
Note to other reviewers: We assume this to hold true in assumptions.h:
static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");
There was a problem hiding this comment.
I should remove that assumption now, it's no longer needed.
There was a problem hiding this comment.
I think we need that assumption as long as we're doing floating-point division by zero? I still think we do that in ConnectBlock, CreateTransaction and EstimateMedianVal :)
There was a problem hiding this comment.
Maybe a weaker assumption could do there. In any case, it's out of scope for this PR. Happy to leave it as it is for the foreseeable future unless anyone has good reason to work on porting bitcoind to non-IEC559 platforms.
(in which case I heartily suggest: please get rid of all floating point code. it shouldn't be necessary in financial-ish code)
|
cr re-ACK 66545da |
…r for fee estimation 66545da Remove support for double serialization (Pieter Wuille) fff1cae Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille) afd964d Convert existing float encoding tests (Pieter Wuille) bda33f9 Add unit tests for serfloat module (Pieter Wuille) 2be4cd9 Add platform-independent float encoder/decoder (Pieter Wuille) e40224d Remove unused float serialization (MarcoFalke) Pull request description: Based on bitcoin#21981. This adds a software-based platform-independent float/double encoder/decoder (platform independent in the sense that it only uses arithmetic and library calls, but never inspects the binary representation). This should strengthen our guarantee that encoded float/double values are portable across platforms. It then removes the functionality to serialize doubles from serialize.h, and replaces its only (non-test) use for fee estimation data serialization with the software encoder. At least on x86/ARM, the only difference should be how certain NaN values are encoded/decoded (but not *whether* they are NaN or not). It comes with tests that verify on is_iec559 platforms (which are the only ones we support, at least for now) that the serialized bytes exactly match the binary representation of floats in memory (for non-NaN). ACKs for top commit: laanwj: Code review re-ACK 66545da practicalswift: cr re-ACK 66545da Tree-SHA512: 62ad9adc26e28707b2eb12a919feefd4fd10cf9032652dbb1ca1cc97638ac21de89e240858e80d293d5112685c623e58affa3d316a9783ff0e6d291977a141f5
partial bitcoin#15638, bitcoin#21966, bitcoin#16889, merge bitcoin#14555, bitcoin#20499, bitcoin#14074, bitcoin#17073: util refactoring
Based on #21981.
This adds a software-based platform-independent float/double encoder/decoder (platform independent in the sense that it only uses arithmetic and library calls, but never inspects the binary representation). This should strengthen our guarantee that encoded float/double values are portable across platforms. It then removes the functionality to serialize doubles from serialize.h, and replaces its only (non-test) use for fee estimation data serialization with the software encoder.
At least on x86/ARM, the only difference should be how certain NaN values are encoded/decoded (but not whether they are NaN or not).
It comes with tests that verify on is_iec559 platforms (which are the only ones we support, at least for now) that the serialized bytes exactly match the binary representation of floats in memory (for non-NaN).