refactor: std::span compat fixes #31540

1. 
   maflcko commented at 12:57 pm on December 19, 2024: member

   The std::span type is already used in some parts of the codebase, and in most contexts can implicitly convert to and from Span. However, the two types are not identical in behavior and trying to use one over the other can result in compile failures in some contexts.

   Fix all those issues by allowing either Span or std::span in any part of the codebase.

   All of the changes are also required for the scripted-diff to replace Span with std::span in https://github.com/bitcoin/bitcoin/pull/31519

2. refactor: Avoid needless, unsafe c-style cast fac3a782ea
3. refactor: Replace fwd-decl with proper include

   This is fine, because the span.h include is lightweight and a proper
   include will be needed anyway when switching to std::span.

   facc4f120b
4. refactor: Simplify SpanPopBack

   Use the equivalent back() and first() member functions.

   faae6fa5f6
5. 
   DrahtBot commented at 12:57 pm on December 19, 2024: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31540.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theuni, adamandrews1, sipa, fjahr, achow101

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

6. DrahtBot added the label Refactoring on Dec 19, 2024
7. in src/script/miniscript.h:1801 in fa1be42772 outdated

   1797@@ -1798,7 +1798,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
   1798         // Get threshold
   1799         int next_comma = FindNextChar(in, ',');
   1800         if (next_comma < 1) return false;
   1801-        const auto k_to_integral{ToIntegral<int64_t>(std::string_view(in.begin(), next_comma))};
   1802+        const auto k_to_integral{ToIntegral<int64_t>(std::string_view(&in.front(), next_comma))};
   

   ---

   ---

   

   sipa commented at 1:20 pm on December 19, 2024:

   Would in.data() work here instead of &in.front()? That expresses the intent better I think, because we don’t actually just want the first element.

   ---

   

   maflcko commented at 2:54 pm on December 19, 2024:

   thx, done
8. 
   sipa commented at 1:22 pm on December 19, 2024: member

   utACK fa163e04ca62b832650c9c942ebe240ad46351c8
9. refactor: Avoid passing span iterators when data pointers are expected

   For Span, iterators are just raw data pointers. However, for std::span
   they are not.
   
   This change makes it explicit where data pointers are expected.
   
   Otherwise, there could be a compile error later on:
   
     No known conversion from 'iterator' (aka '__normal_iterator<const std::byte *, std::span<const std::byte, 18446744073709551615>>') to 'std::byte *'.

   fa86223475
10. refactor: test: Return std::span from StringBytes

    This is possible and safe, because std::span can implicitly convert into
    Span, if needed.
    
    Changing this function is required, because std::span requires the
    extent template parameter to be specified as well.
    
    Instead of explicilty specifying them, just let the compiler derive the
    template parameters correctly.
    
    Otherwise, there would be a compile error later on:
    
     src/wallet/test/db_tests.cpp:39:37: error: no matching function for call to ‘as_bytes<const char>(<brace-enclosed initializer list>)’
     ...
     /usr/include/c++/11/span:420:5: note: candidate: ...
           |     as_bytes(span<_Type, _Extent> __sp) noexcept
           |     ^~~~~~~~
     /usr/include/c++/11/span:420:5: note:   template argument deduction/substitution failed:
     src/wallet/test/db_tests.cpp:39:37: note:   couldn’t deduce template parameter ‘_Extent’
           |     return std::as_bytes<const char>({str.data(), str.size()});
           |            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~

    faa5391f77
11. Allow std::span in stream serialization

    Future code can now use std::span in stream serialization, similar to
    the existing serialization helpers for std::array or Span.

    faaf4800aa
12. refactor: Specify const in std::span constructor, where needed

    The std::span constructor requires std::ranges::borrowed_range, which
    tries to protect against dangling references.
    
    One way to disable the check is to specify the std::span's element type
    as const in the constructor call.
    
    Otherwise, a compile error will look like:
    
     include/c++/span: note: candidate constructor not viable: no known conversion from 'std::vector<unsigned char>' to 'const span<unsigned char>' for 1st argument
          |       span(const span&) noexcept = default;
          |       ^    ~~~~~~~~~~~
     ...
     include/c++/span: note: candidate template ignored: constraints not satisfied [with _Range = std::vector<unsigned char>]
          |         span(_Range&& __range)
          |         ^
     include/c++/span: note: because 'std::vector<unsigned char>' does not satisfy 'borrowed_range'
          |           && (ranges::borrowed_range<_Range> || is_const_v<element_type>)
          |               ^
     include/c++/bits/ranges_base.h: note: because 'std::vector<unsigned char>' does not satisfy '__maybe_borrowed_range'
          |       = range<_Tp> && __detail::__maybe_borrowed_range<_Tp>;
          |                       ^
     include/c++/bits/ranges_base.h: note: because 'is_lvalue_reference_v<std::vector<unsigned char> >' evaluated to false
          |         = is_lvalue_reference_v<_Tp>
          |           ^
     include/c++/bits/ranges_base.h: note: and 'enable_borrowed_range<remove_cvref_t<vector<unsigned char, allocator<unsigned char> > > >' evaluated to false
          |           || enable_borrowed_range<remove_cvref_t<_Tp>>;
          |              ^
     include/c++/span: note: and 'is_const_v<element_type>' evaluated to false
          |           && (ranges::borrowed_range<_Range> || is_const_v<element_type>)
          |                                                 ^

    fa494a1d53
13. maflcko force-pushed on Dec 19, 2024
14. in src/base58.cpp:142 in fac3a782ea outdated

    138@@ -139,7 +139,7 @@ std::string EncodeBase58Check(Span<const unsigned char> input)
    139     // add 4-byte hash check to the end
    140     std::vector<unsigned char> vch(input.begin(), input.end());
    141     uint256 hash = Hash(vch);
    142-    vch.insert(vch.end(), (unsigned char*)&hash, (unsigned char*)&hash + 4);
    

    ---

    ---

    

    theuni commented at 3:37 pm on December 19, 2024:

    😬
15. in src/span.h:251 in faae6fa5f6 outdated

    247@@ -248,9 +248,8 @@ template <typename T>
    248 T& SpanPopBack(Span<T>& span)
    249 {
    250     size_t size = span.size();
    251-    ASSERT_IF_DEBUG(size > 0);
    

    ---

    ---

    

    theuni commented at 3:42 pm on December 19, 2024:

    Why drop the assert? Though I have no idea who/what tests with -DDEBUG anyway.

    ---

    

    maflcko commented at 4:10 pm on December 19, 2024:

    The assert is not dropped. In fact, there are now two asserts:

    • https://github.com/bitcoin/bitcoin/blob/fa494a1d53f3f030fafe7b533d72b2200428a0fd/src/span.h#L184
    • https://github.com/bitcoin/bitcoin/blob/fa494a1d53f3f030fafe7b533d72b2200428a0fd/src/span.h#L207

    The first one is the one you left a comment on. And the second one is left for the compiler to drop, if it wants to.

16. theuni approved
17. 
    theuni commented at 3:58 pm on December 19, 2024: member

    utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd.

    I haven’t verified that this assortment of changes is all that’s needed for the conversion, but they all look good to me.

18. DrahtBot requested review from sipa on Dec 19, 2024
19. adamandrews1 approved
20. 
    adamandrews1 commented at 4:03 pm on December 19, 2024: none

    utACK https://github.com/bitcoin/bitcoin/commit/fa494a1d53f3f030fafe7b533d72b2200428a0fd
21. 
    sipa commented at 3:31 pm on December 23, 2024: member

    utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
22. 
    fjahr commented at 12:01 pm on December 30, 2024: contributor

    Code review ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
23. 
    achow101 commented at 6:19 pm on December 30, 2024: member

    ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
24. achow101 merged this on Dec 30, 2024
25. achow101 closed this on Dec 30, 2024

    ---

26. maflcko deleted the branch on Jan 2, 2025

Contributors
maflcko DrahtBot sipa theuni adamandrews1 fjahr achow101

Review Requested
sipa

Labels
Refactoring