Weaken serfloat tests #29192

pull sipa wants to merge 2 commits into bitcoin:master from sipa:202401_serfloat_weaken_test changing 1 files +69 −47
  1. sipa commented at 7:53 pm on January 5, 2024: member

    Closes #28941.

    Our current tests for serfloat verify two distinct properties:

    1. Whether they roundtrip double->uint64_t->double (excluding NaN values) on all systems.
    2. Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.

    #28941 seems to show that the second property doesn’t always hold, but just for “subnormal” numbers (below $2^{-1021}$). Since we don’t care about encoding these numbers, we could exclude such subnormal numbers from the hardware-identical representation test, but this PR goes further and just drops the second property entirely, as I don’t think we care about edge-case compatibility with pre-v22.0 code for fee_estimates.dat (the only place it is used).

  2. DrahtBot commented at 7:53 pm on January 5, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK glozow, fanquake

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. in src/test/serfloat_tests.cpp:52 in 9c144154bd outdated 
    52-    if (std::numeric_limits<double>::is_iec559) {
53-        BOOST_CHECK_EQUAL(sizeof(double), 8U);
54-        BOOST_CHECK_EQUAL(sizeof(uint64_t), 8U);
55-        // Test extreme values
56+    // Test extreme values on systems where they are guaranteed to exist.
57+    if constexpr (std::numeric_limits<double>::is_iec559) {
    maflcko commented at 8:05 pm on January 5, 2024:

    side-note: This should always be true, according to the static_assert:

    0src/compat/assumptions.h:static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float assumed");
1src/compat/assumptions.h:static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");
    sipa commented at 8:17 pm on January 5, 2024:
    Fair enough. The check doesn’t hurt though, and perhaps functions as documentation to clarify the conditions due to which these tests are possible?
    maflcko commented at 8:22 pm on January 5, 2024:
    Sure, no need to change anything.
    sipa commented at 7:30 pm on March 12, 2024:
    I’ve reworked this; the tests just assume is_iec559 now.
  4. maflcko commented at 8:24 pm on January 5, 2024: member
    I think the fuzz tests are also checking the exact hardware representation? So they should be failing as well, but I guess no one is running them through homebrew (or whatever the affected system is).
  5. sipa commented at 8:27 pm on January 5, 2024: member
    @maflcko I don’t think they are affected, as the fuzz tests always start from a double (even if one obtained by decoding raw memory) and test roundtripping double->uint64_t->double of that. The issue only seems to appear when starting from raw memory, then encoding + decoding, and comparing with the raw memory started with.
  6. sipa commented at 8:44 pm on January 5, 2024: member
    @maflcko I’ve added a commit to verify that… it adds a hardware-representation equivalence test again, but starting from a double like the fuzz tests do.
  7. in src/test/serfloat_tests.cpp:36 in d5cd5a7ee9 outdated 
    29@@ -30,6 +30,14 @@ uint64_t TestDouble(double f) {
30         uint64_t i2 = EncodeDouble(f2);
31         BOOST_CHECK_EQUAL(f, f2);
32         BOOST_CHECK_EQUAL(i, i2);
33+        // On IEEE754 systems, the encoding is expected to match the hardware representation.
34+        if constexpr (std::numeric_limits<double>::is_iec559) {
35+            static_assert(sizeof(double) == 8);
36+            static_assert(sizeof(uint64_t) == 8);
    maflcko commented at 8:51 pm on January 5, 2024:
    I think in C++20 a static_assert is evaluated even in a non-instantiated context, so could either make this a global static_assert (which is equivalent), or use the “template-hack” to not evaluate it if the context isn’t instantiated either.
    sipa commented at 8:57 pm on January 5, 2024:

    Right.

    I’ll address this (and update the description of the PR which is now outdated with the last commit) when @fanquake reports this fixes #28941.

    maflcko commented at 11:37 am on January 6, 2024:
    (Just a nit, no need to change anything, because is_iec559 is already asserted globally, so the two other asserts can also happen globally. In any case, this will “fix itself” with C++23 at some far point in the future :sweat_smile: )
    sipa commented at 7:31 pm on March 12, 2024:
    Done.
  8. maflcko approved
  9. maflcko commented at 8:51 pm on January 5, 2024: member
    lgtm
  10. fanquake referenced this in commit da3404a3c7 on Jan 8, 2024
  11. fanquake commented at 11:59 am on January 8, 2024: member

    Pushed up the patches into my PR, it has fixed some of the test failures, but not all: https://github.com/Homebrew/homebrew-core/actions/runs/7446626418/job/20257419440?pr=156745#step:4:82.

     0  ==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/26.0/bin/test_bitcoin
 1  Running 585 test cases...
 2  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [1 != 0]
 3  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9223372036854775809 != 9223372036854775808]
 4  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9227078312698333629 != 9223372036854775808]
 5  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9227541302258710164 != 9223372036854775808]
 6  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9226320440948298264 != 9223372036854775808]
 7  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9224296035333843957 != 9223372036854775808]
 8  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [3342972290171876 != 0]
 9  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [455962168228056 != 0]
10  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9226157041067996541 != 9223372036854775808]
11  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9225839089286856993 != 9223372036854775808]
12  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9225901867614656042 != 9223372036854775808]
13  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9227430019473490172 != 9223372036854775808]
14  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9224285440633669494 != 9223372036854775808]
15  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9224305768567079357 != 9223372036854775808]
16  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9223409755742115848 != 9223372036854775808]
17  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9224058209782467602 != 9223372036854775808]
18  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [2599738216344523 != 0]
19  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9227049248406766111 != 9223372036854775808]
20  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9226155215443275398 != 9223372036854775808]
21  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [4045757423585278 != 0]
22  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9225916179490235579 != 9223372036854775808]
23  test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [9227295753411824867 != 9223372036854775808]
24  
25  *** 22 failures are detected in the test module "Bitcoin Core Test Suite"

    If anyone wants to reproduce the failures locally (against master), they can do:

     0docker pull docker.io/homebrew/brew:latest
 1docker run -it homebrew/brew
 2
 3brew install bitcoin --head
 4brew test bitcoin
 5
 6==> Testing bitcoin
 7==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/HEAD-04b9df0/bin/test_bitcoin
 8Last 15 lines from /home/linuxbrew/.cache/Homebrew/Logs/bitcoin/test.01.test_bitcoin:
 9test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [602650259075964 != 0]
10test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [9225028085348500152 != 9223372036854775808]
11test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [225556757220273 != 0]
12test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [808287137814849 != 0]
13test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1601366725718274 != 0]
14test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1803809784509295 != 0]
15test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [9224578429555196467 != 9223372036854775808]
16test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [2210598722220679 != 0]
17test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [869264799447506 != 0]
18test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1193579134805228 != 0]
19test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1018605001172173 != 0]
20test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [9226505921543298662 != 9223372036854775808]
21
22*** 219 failures are detected in the test module "Bitcoin Core Test Suite"
  12. sipa commented at 1:51 pm on January 8, 2024: member
    @fanquake What if you drop the last commit here?
  13. fanquake commented at 2:43 pm on January 8, 2024: member

    What if you drop the last commit here?

    Tests are passing: https://github.com/Homebrew/homebrew-core/actions/runs/7448770957/job/20264045705?pr=156745.

  14. sipa force-pushed on Jan 8, 2024
  15. sipa commented at 3:39 pm on January 8, 2024: member
    Dropped the last commit, and improved the code changes a bit. For backport, if needed, the first commit suffices.
  16. fanquake added the label Needs backport (26.x) on Jan 8, 2024
  17. DrahtBot added the label CI failed on Jan 15, 2024
  18. luke-jr commented at 9:12 pm on January 23, 2024: member
    Does dropping 2 lose the test that we haven’t broken the serialization format somehow? Maybe it should be comparing a fixed mapping of some values at least?
  19. in src/test/serfloat_tests.cpp:103 in e5202418cb outdated 
    104+    TestDouble(-std::numeric_limits<double>::quiet_NaN());
105+    TestDouble(std::numeric_limits<double>::signaling_NaN());
106+    TestDouble(-std::numeric_limits<double>::signaling_NaN());
107+    TestDouble(std::numeric_limits<double>::denorm_min());
108+    TestDouble(-std::numeric_limits<double>::denorm_min());
109+    // Construct doubles to test from the encoding.
    glozow commented at 11:43 am on January 26, 2024:
    IIUC pseudocode: Use values of x to codify all possibilities of 9 bits, with 1000 iters for each setting. So x=0..(1000*2^9)-1 for x_pos, v_pos in enumerate([0, 1, 50, 51, 52, 53, 61, 62, 63]):     v starts as 64 random bits     reset v’s v_posth bit to what x ’s x_pos bit is     turn v into double f and TestDouble(f)
  20. in src/test/serfloat_tests.cpp:67 in e5202418cb outdated 
    107+    TestDouble(std::numeric_limits<double>::denorm_min());
108+    TestDouble(-std::numeric_limits<double>::denorm_min());
109+    // Construct doubles to test from the encoding.
110+    static_assert(sizeof(double) == 8);
111+    static_assert(sizeof(uint64_t) == 8);
112+    for (int x = 0; x < 51200; ++x) {
    glozow commented at 11:57 am on January 26, 2024:
    Is this doing 100 iterations per setting, when it was 1000 before?
    sipa commented at 11:35 am on February 20, 2024:
    Indeed, reverted (and also stuck closer to the former code).
  21. glozow commented at 1:52 pm on February 2, 2024: member

    ACK ea60c95be36a68ba98d1d23018587aa4d4d6bb1a, based on https://github.com/Homebrew/homebrew-core/pull/156745 and #29192 (comment) it looks like this fixes the issue.

    re #29192 (comment), the values that were failing the tests are irrelevant in the context we’re using this, and 21.0 has been eol for a while now - also see #28941 (comment)

    second commit diff looks correct to me though I’m unsure if it’s necessary (ACK e5202418cbec34ffd0269ce66293d9c41b20709b)

  22. serfloat: do not test encode(bits)=bits anymore b45f1f5658
  23. serfloat: improve/simplify tests 6e873df347
  24. sipa force-pushed on Feb 20, 2024
  25. sipa commented at 11:36 am on February 20, 2024: member
    @luke-jr There were a few expected encoding value tests, but I’ve expanded them significantly now.
  26. DrahtBot removed the label CI failed on Feb 20, 2024
  27. glozow commented at 3:31 pm on February 28, 2024: member
    ACK 6e873df3478f3ab8f67d1b9339c7e990ae90e95b
  28. glozow removed the label Needs backport (26.x) on Feb 28, 2024
  29. sipa commented at 3:50 pm on March 17, 2024: member
    Anything left to do here?
  30. glozow assigned fanquake on Mar 18, 2024
  31. fanquake approved
  32. fanquake commented at 5:04 pm on March 19, 2024: member
    ACK 6e873df3478f3ab8f67d1b9339c7e990ae90e95b - It’s not as much of a priority, but I think we could still backport this.
  33. fanquake merged this on Mar 19, 2024
  34. fanquake closed this on Mar 19, 2024
  35. fanquake referenced this in commit f4be4d7447 on Mar 21, 2024
  36. fanquake referenced this in commit 5d381cfb6f on Mar 21, 2024
  37. fanquake commented at 10:59 am on March 25, 2024: member
    Backported to 27.x in #29693.


sipa DrahtBot maflcko fanquake luke-jr glozow

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me