Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport: merge bitcoin#19326, #20464, #23451, #20852, #20355, #22050, #23115, #23409, #18468, #23413, partial bitcoin#21560, #23438 (auxiliary backports: part 11) #5574

Merged
merged 21 commits into from
Sep 24, 2023

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Sep 9, 2023

Additional information

  • bitcoin#19326 was partially backported as part of dash#4164 to the exclusion of 2a2182c, 77c5073.
    The excluded commits have been backported, marking the pull request as fully merged.

  • bitcoin#18468 was partially backported as part of dash#4036 to the exclusion of 2676aea.
    The excluded commit has been backported, marking the pull request as fully merged.

  • bitcoin#21560 does not include any TorV3 nodes.

  • When backporting bitcoin#20464, llmq/dkgsessionhandler.cpp needs a reinterpret_cast because CHashWriter accepts a const char* while CDataStream's data() returns an unsigned const char*.

    Using reinterpret_cast is acceptable because CHashWriter::write() simply casts the const char* to unsigned const char*, reversing the previous cast (source), making this exercise solely for the purpose of fitting the pointer through the argument.

    A C-style cast wasn't used because the linter took objection to it (build) and only it, but seemed to be fine with every other C-style cast.

  • Dependency for backport: merge bitcoin#27479, #27230, #25251, partial #22934, #23383, #24792, #26691, #27445 (secp256k1 update) #5575

    • bitcoin#27479, the primary motivation for dash#5575 uses features added to Span (such as the removal of MakeSpan and the introduction of BytePtr)
    • Another backport that was primarily required as bitcoin#22570, which does away with the serialization code for CSubNet, which utilizes Span but would be incompatible with bitcoin#23413, at least on macOS (build)
      • This in turn pulled in a bunch of networking-related backports and only the minimum set required to backport bitcoin#22570 has been included.

        This has the effect of making bitcoin#17428 vestigial as the code that actually integrates it with existing logic has been excluded from the backport as it relies on larger networking backports which are far beyond the scope of this PR.

    • This in turn required backporting the contributions made before it, which revealed merge conflicts, which then needed to be added into the fold of backports included in the pull request in order to ensure the least amount of conflicts.

Warnings

  • The greater adoption of Span, removal of MakeSpan and introduction of BytePtr involved changes made to taproot and segregated witness code. This pull request may complicate Taproot backporting should that ever be on the roadmap.

  • BytePtr is later renamed to AsBytePtr due to a naming conflict with the iOS SDK (see discussion for bitcoin#24989) but in the interest of minimizing the set of changes needed to merge in dash#5575 and the fact that Dash does not target i(Pad)OS, this is considered acceptable for now.

src/test/net_tests.cpp Show resolved Hide resolved
doc/files.md Outdated Show resolved Hide resolved
src/addrdb.cpp Outdated Show resolved Hide resolved
src/hash.h Show resolved Hide resolved
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in slack, please, use Span class here instead std::vector for scriptPubKey's + Hash160

Here's possible implementation: knst@a534b5f

@PastaPastaPasta
Copy link
Member

as discussed in slack, please, use Span class here instead std::vector for scriptPubKey's + Hash160

Here's possible implementation: knst@a534b5f

This isn't needed? The purpose of using span is you can implicitly construct one from basically anything; seems fine to me to keep it as is

@knst
Copy link
Collaborator

knst commented Sep 21, 2023

This isn't needed? The purpose of using span is you can implicitly construct one from basically anything; seems fine to me to keep it as is

These 2 commits are extra context for you @PastaPastaPasta
knst@5d91e7e
knst@7d80ffb

so, this original dash's code will cause a compilation error with a new Span implementation (after all backports from this batch in this PR):

uint160 hashBytes(Hash160(prevout.scriptPubKey.begin()+1, prevout.scriptPubKey.end()-1));

There's approach in this PR (kwvg's solution):

-uint160 hashBytes(Hash160(prevout.scriptPubKey.begin()+1, prevout.scriptPubKey.end()-1));
+uint160 hashBytes(Hash160(std::vector<unsigned char>(prevout.scriptPubKey.begin()+1, prevout.scriptPubKey.end()-1)));

my patch:

-uint160 hashBytes(Hash160(prevout.scriptPubKey.begin()+1, prevout.scriptPubKey.end()-1));
+uint160 hashBytes{Hash160(Span{prevout.scriptPubKey.data()+1, prevout.scriptPubKey.size() - 2})};

my solution uses Span instead std::vector and it helps to avoid extra memory allocation, because Span do now copy data, do not own it, it is just container with pointer to data + its length (same as std::span and std::string_view), but std::vector<...> will cause memory allocation + memory copy + memory deallocation when its data no need more.

@PastaPastaPasta
Copy link
Member

@knst thanks for the context! I didn't see that current code was allocating a vector.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for merging via merge commit

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kwvg and others added 9 commits September 24, 2023 09:50
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
The three object variant was removed in dash#3276 through the backport
of bitcoin#11385, leaving behind the four, five and six variant options.
They aren't used anywhere in the codebase and aren't present in Bitcoin
Core currently, and so, have been removed.
includes:
- 2a2182c
- 77c5073

completion of partial merge done in dash#4164 as 56f1b2d (blocker
bitcoin#17938 was merged in dash#5246 as 00802bb)
@PastaPastaPasta PastaPastaPasta merged commit 7bff85e into dashpay:develop Sep 24, 2023
8 of 10 checks passed
@UdjinM6 UdjinM6 added this to the 20 milestone Oct 5, 2023
PastaPastaPasta added a commit that referenced this pull request Feb 28, 2024
, bitcoin#21969, bitcoin#23653, bitcoin#23438, bitcoin#24190, bitcoin#24253, bitcoin#24231, bitcoin#26258, bitcoin#28012, partial bitcoin#25001, bitcoin#25296, bitcoin#23595, bitcoin#27927 (serialization updates)

2b26a87 merge bitcoin#28012: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization (Kittywhiskers Van Gogh)
4eeafa2 partial bitcoin#27927: Allow std::byte and char Span serialization (Kittywhiskers Van Gogh)
cb2fa83 test: place the std::ostream operator<< definition in namespace std (Kittywhiskers Van Gogh)
cf4522f partial bitcoin#23595: Add ParseHex<std::byte>() helper (Kittywhiskers Van Gogh)
e4091aa partial bitcoin#25296: Add DataStream without ser-type and ser-version (Kittywhiskers Van Gogh)
eab031a merge bitcoin#26258: Remove unused CDataStream::rdbuf method (Kittywhiskers Van Gogh)
95b5850 partial bitcoin#25001: Modernize util/strencodings and util/string: string_view and optional (Kittywhiskers Van Gogh)
5fe72bb merge bitcoin#24231: Fix read-past-the-end and integer overflows (Kittywhiskers Van Gogh)
24af372 merge bitcoin#24253: Remove broken and unused CDataStream methods (Kittywhiskers Van Gogh)
baf8dd6 merge bitcoin#24190: Fix sanitizer suppresions in streams_tests (Kittywhiskers Van Gogh)
e933d78 merge bitcoin#23438: Use spans of std::byte in serialize (Kittywhiskers Van Gogh)
d3b2822 merge bitcoin#23653: Generalize/simplify VectorReader into SpanReader (Kittywhiskers Van Gogh)
2c32a09 merge bitcoin#21969: Switch serialize to uint8_t (Kittywhiskers Van Gogh)
0a08dbf merge bitcoin#21824: Replace deprecated char with uint8_t in serialization (Kittywhiskers Van Gogh)
d0b4e56 merge bitcoin#21966: Remove double serialization; use software encoder for fee estimation (Kittywhiskers Van Gogh)
1d6aafe merge bitcoin#21817: Replace &foo[0] with foo.data() (Kittywhiskers Van Gogh)
d9a8ce2 trivial: move GetSerializeSize away from Stream (Un)serialize functions (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #5902

  * [bitcoin#24231](bitcoin#24231) is merged after [bitcoin#24253](bitcoin#24253) due to a MinGW bug ([comment](bitcoin#24231 (comment)))

  * [bitcoin#25001](bitcoin#25001) is listed as unmerged despite being committed upstream as bitcoin@132d5f8

  * [bitcoin#25296](bitcoin#25296) is listed as unmerged despite being committed upstream as bitcoin@79e007d

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

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

  * [bitcoin#27927](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](#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`](https://github.com/dashpay/dash/blob/37f43e4e56de0d510110a7f790df34ea77748dc9/contrib/containers/develop/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

      ```c++
      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`](https://github.com/dashpay/dash/blob/96ac317c2714afcac1ff4e2df309f17149f42776/src/test/util/setup_common.h#L27-L32) 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](bitcoin#23497) in bitcoin@f7086fd (see [change](https://github.com/bitcoin/bitcoin/blame/f7086fd8ff084ab0dd656d75b7485e59263bdfd8/src/test/util/setup_common.h#L29-L35)).

      That change has now been made in a separate commit.

  ## Breaking Changes

  Changes in serialization APIs will make backports predating [bitcoin#23438](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._
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: 1d7c67f5fe282f78e24cb720828e5f1f48b6926006b903c28399938532cc5c470c175b00c8b80e9662c4467c1201e09ae6d1cd9b95e8b20ace5e4410c72c472e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants