backport: merge bitcoin#21817, #21966, #21824, #21969, #23653, #23438, #24190, #24253, #24231, #26258, #28012, partial bitcoin#25001, #25296, #23595, #27927 (serialization updates)

[kwvg]

Additional Information

• Dependency for backport: merge bitcoin#27985, #27993, #28008, #28267, #28374, #28100 (p2p primitives) #5902

• bitcoin#24231 is merged after bitcoin#24253 due to a MinGW bug (comment)

• bitcoin#25001 is listed as unmerged despite being committed upstream as bitcoin@132d5f8

• bitcoin#25296 is listed as unmerged despite being committed upstream as bitcoin@79e007d

• bitcoin#21966 was partially backported in dash#4197 as f946c68, including only 2be4cd9.
  The excluded commits have been backported, marking the pull request as fully merged.

• bitcoin#23438 was partially backported in dash#5574 as de54b87, including only fa65bbf.
  The excluded commits have been backported, marking the pull request as fully merged.

• bitcoin#27927 opened a fresh can of hell thanks to being (possibly?) the first pull request to include std::byte BOOST_CHECK's to the unit test suite. For reasons still unbeknownst to me, it refused to compile, despite being perfectly happy when checked-out as a commit directly and built as-is from upstream.

  The compile error was like this (edited for brevity):

    CXX      test/test_dash-serialize_tests.o
  In file included from test/serialize_tests.cpp:13:
  In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/unit_test.hpp:18:
  In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/test_tools.hpp:46:
  In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/old/impl.hpp:24:
  /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/detail/print_helper.hpp:53:13: error: static assertion failed due to requirement 'boost::has_left_shift<std::basic_ostream<char, std::char_traits<char>>, std::byte, boost::binary_op_detail::dont_care>::value': Type has to implement operator<< to be printable
              BOOST_STATIC_ASSERT_MSG( (boost::has_left_shift<std::ostream,T>::value),
              ^                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  [...]
  <scratch space>:206:1: note: expanded from here
  BOOST_PP_REPEAT_1
  ^
  test/serialize_tests.cpp:347:9: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, std::byte, std::byte>' requested here
          BOOST_CHECK_EQUAL(out.at(0), std::byte{'a'});
          ^
  [...]
  In file included from test/serialize_tests.cpp:13:
  In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/unit_test.hpp:18:
  In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/test_tools.hpp:46:
  In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/old/impl.hpp:24:
  /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/detail/print_helper.hpp:55:18: error: invalid operands to binary expression ('std::ostream' (aka 'basic_ostream<char>') and 'const std::byte')
              ostr << t;
              ~~~~ ^  ~
  /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/cstddef:130:5: note: candidate function template not viable: no known conversion from 'std::ostream' (aka 'basic_ostream<char>') to 'byte' for 1st argument
      operator<<(byte __b, _IntegerType __shift) noexcept
      ^
  [...]
  5 warnings and 2 errors generated.
  make[2]: *** [Makefile:17842: test/test_dash-serialize_tests.o] Error 1
  make[2]: *** Waiting for unfinished jobs....
  make[2]: Leaving directory '/src/dash/src'
  make[1]: *** [Makefile:18525: all-recursive] Error 1
  make[1]: Leaving directory '/src/dash/src'
  make: *** [Makefile:802: all-recursive] Error 1
  

  • No such error was present on Bitcoin.

    It is true, that no std::ostream& operator<<(std::ostream& a, const std::byte& b) is present by default but attempting to grep for any specializations didn't show anything relevant that Dash didn't have. Searching on GitHub didn't help either.

  • Then, assuming that perhaps Boost's assertion logic may have changed, upgraded the version of Boost to match the pull request at the time, Boost 1.81. That also did not do anything (and actually, caused a trailing slashes unit test to fail but doesn't cause any problem in Bitcoin because they got rid of their boost::filesystem usage by then).

  • If that isn't it, then let's try building Bitcoin with Dash's depends. It built successfully, ran successfully. The problem isn't in the dependency, it's in the codebase.

  • Since it seemed to be std::byte related, pull requests that are related to std::byte serialization were backported and std::byte serialization related changes needed for dash#5902 were cherry-picked. That's why this pull request came to be. But it didn't help this particular issue (though it did smooth out the cherry-picks).

  • Running out of ideas, gdb is used to step through serialize_tests's class_methods and understand why BOOST_CHECK_EQUAL(out.at(0), std::byte{'a'}); is valid in Bitcoin but not Dash by finding the elusive operator<<. This is where things go from bad to worse.

    Turns out, when you build with clang, gdb loses the ability to do breakpoints by file. So, an attempt is made to use lldb (which btw, is called lldb-16, running lldb with yield an error if you're using the develop container) and it refuses to work, erroring personality set failed: Operation not permitted.

    Turns out, the docker-compose.yml needs the following additions:

      cap_add:
        - SYS_PTRACE
      security_opt:
        - "seccomp:unconfined"
    

    After making these changes, lldb works and then we resume trying to find operator<<. After too many hours and nimbly alternating between next and step, tried making a return to gdb (compiling with gcc this time with the appropriate CXXFLAGS) hoping for different results and a while later, realized that it cannot step through Boost's headers (it doesn't recognize the filenames) and then recompile it with clang and return to lldb.

    This was a wild goose chase.

  • After a lot of futile efforts to find the operator by stepping through BOOST_CHECK_EQUAL, a basic example addressing the static assertion (that a left shift operator must exist of <type> (here std::byte) for std::ostream) was added in

    std::ostream& ostr = std::cout;
    ostr << std::byte{'a'};

    ...and it compiled in both codebases.

    So the left shift that Boost is asserting doesn't exist does exist but it isn't being detected for some reason. Upon hovering the <<, VSCode highlighted the source of the definition as setup_common.h thanks to the comment above it.

    Diffing between Bitcoin and Dash revealed the secret, the operator<< definition was placed under namespace std by bitcoin#23497 in bitcoin@f7086fd (see change).

    That change has now been made in a separate commit.

Breaking Changes

Changes in serialization APIs will make backports predating bitcoin#23438 annoying but will not change how data is stored on disk.

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas (note: N/A)
• I have added or updated relevant unit/integration/functional/e2e tests (note: N/A)
• I have made corresponding changes to the documentation (note: N/A)
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[UdjinM6]

utACK

[github-actions]

This pull request has conflicts, please rebase.

[UdjinM6]

re-utACK

[PastaPastaPasta]

utACK for merging via merge commit