Replace MakeSpan helper with Span deduction guide #23413

sipa · 2021-11-01T20:36:44Z

C++17 supports user-defined deduction guides, allowing class constructors to be invoked without specifying class template arguments. Instead, the code can contain rules to infer the template arguments from the constructor argument types.

This alleviates the need for the MakeSpan helper. Convert the existing MakeSpan rules into deduction rules for Span itself, and replace all invocations of MakeSpan with just Span ones.

DrahtBot · 2021-11-01T21:04:23Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23595 (util: Add ParseHexstd::byte() helper by MarcoFalke)
#23226 (c++20: Opt-in to modeling view and borrowed_range for Span by theuni)
#17034 ([BIP 174] PSBT version, proprietary, and xpub fields by achow101)

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.

maflcko

Concept ACK on moving Span closer to std::span. Though I am wondering some deductions are omitted.

See patch:

diff --git a/src/net.cpp b/src/net.cpp
index ad558dd598..b323d17134 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -738,7 +738,7 @@ std::optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono
     if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
         LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
                  SanitizeString(msg->m_command), msg->m_message_size,
-                 HexStr(Span<uint8_t>(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)),
+                 HexStr(Span{hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE}),
                  HexStr(hdr.pchChecksum),
                  m_node_id);
         out_err_raw_size = msg->m_raw_message_size;
@@ -1556,7 +1556,7 @@ void CConnman::SocketHandler()
             if (nBytes > 0)
             {
                 bool notify = false;
-                if (!pnode->ReceiveMsgBytes(Span<const uint8_t>(pchBuf, nBytes), notify))
+                if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify))
                     pnode->CloseSocketDisconnect();
                 RecordBytesRecv(nBytes);
                 if (notify) {
diff --git a/src/netaddress.cpp b/src/netaddress.cpp
index 97ad40aa6e..201b797a0d 100644
--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -303,7 +303,7 @@ CNetAddr::CNetAddr(const struct in_addr& ipv4Addr)
 
 CNetAddr::CNetAddr(const struct in6_addr& ipv6Addr, const uint32_t scope)
 {
-    SetLegacyIPv6(Span<const uint8_t>(reinterpret_cast<const uint8_t*>(&ipv6Addr), sizeof(ipv6Addr)));
+    SetLegacyIPv6(Span{reinterpret_cast<const uint8_t*>(&ipv6Addr), sizeof(ipv6Addr)});
     m_scope_id = scope;
 }
 
diff --git a/src/pubkey.h b/src/pubkey.h
index e4e3a9560b..dcbdca8ebd 100644
--- a/src/pubkey.h
+++ b/src/pubkey.h
@@ -242,7 +242,7 @@ public:
     explicit XOnlyPubKey(Span<const unsigned char> bytes);
 
     /** Construct an x-only pubkey from a normal pubkey. */
-    explicit XOnlyPubKey(const CPubKey& pubkey) : XOnlyPubKey(Span<const unsigned char>(pubkey.begin() + 1, pubkey.begin() + 33)) {}
+    explicit XOnlyPubKey(const CPubKey& pubkey) : XOnlyPubKey(Span{pubkey.begin() + 1, pubkey.begin() + 33}) {}
 
     /** Verify a Schnorr signature against this public key.
      *
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 2d7f5f2894..c72f08a5ff 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -317,7 +317,7 @@ public:
         UniValue obj(UniValue::VOBJ);
         obj.pushKV("iswitness", true);
         obj.pushKV("witness_version", (int)id.version);
-        obj.pushKV("witness_program", HexStr(Span<const unsigned char>(id.program, id.length)));
+        obj.pushKV("witness_program", HexStr(Span{id.program, id.length}));
         return obj;
     }
 };
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 95611fc909..b023d199d5 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1251,7 +1251,7 @@ std::unique_ptr<PubkeyProvider> InferXOnlyPubkey(const XOnlyPubKey& xkey, ParseS
 std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptContext ctx, const SigningProvider& provider)
 {
     if (ctx == ParseScriptContext::P2TR && script.size() == 34 && script[0] == 32 && script[33] == OP_CHECKSIG) {
-        XOnlyPubKey key{Span<const unsigned char>{script.data() + 1, script.data() + 33}};
+        XOnlyPubKey key{Span{script.data() + 1, script.data() + 33}};
         return std::make_unique<PKDescriptor>(InferXOnlyPubkey(key, ctx, provider));
     }
 
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index eafa9840d7..2d84a28981 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1858,7 +1858,7 @@ uint256 ComputeTaprootMerkleRoot(Span<const unsigned char> control, const uint25
     uint256 k = tapleaf_hash;
     for (int i = 0; i < path_len; ++i) {
         CHashWriter ss_branch{HASHER_TAPBRANCH};
-        Span<const unsigned char> node(control.data() + TAPROOT_CONTROL_BASE_SIZE + TAPROOT_CONTROL_NODE_SIZE * i, TAPROOT_CONTROL_NODE_SIZE);
+        Span node{control.data() + TAPROOT_CONTROL_BASE_SIZE + TAPROOT_CONTROL_NODE_SIZE * i, TAPROOT_CONTROL_NODE_SIZE};
         if (std::lexicographical_compare(k.begin(), k.end(), node.begin(), node.end())) {
             ss_branch << k << node;
         } else {
@@ -1874,7 +1874,7 @@ static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, c
     assert(control.size() >= TAPROOT_CONTROL_BASE_SIZE);
     assert(program.size() >= uint256::size());
     //! The internal pubkey (x-only, so no Y coordinate parity).
-    const XOnlyPubKey p{Span<const unsigned char>{control.data() + 1, control.data() + TAPROOT_CONTROL_BASE_SIZE}};
+    const XOnlyPubKey p{Span{control.data() + 1, control.data() + TAPROOT_CONTROL_BASE_SIZE}};
     //! The output pubkey (taken from the scriptPubKey).
     const XOnlyPubKey q{program};
     // Compute the Merkle root from the leaf and the provided path.
diff --git a/src/signet.cpp b/src/signet.cpp
index aafd1999ee..68fbbc9388 100644
--- a/src/signet.cpp
+++ b/src/signet.cpp
@@ -38,7 +38,7 @@ static bool FetchAndClearCommitmentSection(const Span<const uint8_t> header, CSc
     std::vector<uint8_t> pushdata;
     while (witness_commitment.GetOp(pc, opcode, pushdata)) {
         if (pushdata.size() > 0) {
-            if (!found_header && pushdata.size() > (size_t) header.size() && Span<const uint8_t>(pushdata.data(), header.size()) == header) {
+            if (!found_header && pushdata.size() > (size_t)header.size() && Span{pushdata.data(), header.size()} == header) {
                 // pushdata only counts if it has the header _and_ some data
                 result.insert(result.end(), pushdata.begin() + header.size(), pushdata.end());
                 pushdata.erase(pushdata.begin() + header.size(), pushdata.end());
diff --git a/src/span.h b/src/span.h
index 71de70b64f..b04fdd45d1 100644
--- a/src/span.h
+++ b/src/span.h
@@ -218,16 +218,14 @@ public:
 };
 
 // Deduction guides for Span
-// For the pointer/size based constructor:
-template <typename T> Span(T*, size_t) -> Span<T>;
+// For the pointer/size based and iterator based constructor:
+template <typename T, typename EndOrSize> Span(T*, EndOrSize) -> Span<T>;
 // For the array constructor:
 template <typename T, std::size_t N> Span(T (&)[N]) -> Span<T>;
 // For the temporaries/rvalue references constructor, only supporting const output.
 template <typename T> Span(T&&) -> Span<std::enable_if_t<!std::is_lvalue_reference_v<T>, const std::remove_pointer_t<decltype(std::declval<T&&>().data())>>>;
 // For (lvalue) references, supporting mutable output.
 template <typename T> Span(T&) -> Span<std::remove_pointer_t<decltype(std::declval<T&>().data())>>;
-// For the copy constructor
-template <typename T> Span(const Span<T>&) -> Span<T>;
 
 /** Pop the last element off a span, and return a reference to that element. */
 template <typename T>
diff --git a/src/test/fuzz/asmap.cpp b/src/test/fuzz/asmap.cpp
index d402f8632c..dc37d068d9 100644
--- a/src/test/fuzz/asmap.cpp
+++ b/src/test/fuzz/asmap.cpp
@@ -49,7 +49,7 @@ FUZZ_TARGET(asmap)
     CNetAddr net_addr;
     if (ipv6) {
         assert(addr_size == ADDR_IPV6_SIZE);
-        net_addr.SetLegacyIPv6(Span<const uint8_t>(addr_data, addr_size));
+        net_addr.SetLegacyIPv6(Span{addr_data, addr_size});
     } else {
         assert(addr_size == ADDR_IPV4_SIZE);
         in_addr ipv4;
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 8d2a06f11a..d625403fa0 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -38,7 +38,7 @@ FUZZ_TARGET_INIT(utxo_snapshot, initialize_chain)
     {
         CAutoFile outfile{fsbridge::fopen(snapshot_path, "wb"), SER_DISK, CLIENT_VERSION};
         const auto file_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
-        outfile << Span<const uint8_t>{file_data};
+        outfile << Span{file_data};
     }
 
     const auto ActivateFuzzedSnapshot{[&] {
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index b1300d06ba..a97dc83a3e 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -142,13 +142,13 @@ BOOST_AUTO_TEST_CASE(util_HexStr)
         "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f");
 
     BOOST_CHECK_EQUAL(
-        HexStr(Span<const unsigned char>(
-               ParseHex_expected + sizeof(ParseHex_expected),
-               ParseHex_expected + sizeof(ParseHex_expected))),
+        HexStr(Span{
+            ParseHex_expected + sizeof(ParseHex_expected),
+            ParseHex_expected + sizeof(ParseHex_expected)}),
         "");
 
     BOOST_CHECK_EQUAL(
-        HexStr(Span<const unsigned char>(ParseHex_expected, ParseHex_expected)),
+        HexStr(Span{ParseHex_expected, ParseHex_expected}),
         "");
 
     std::vector<unsigned char> ParseHex_vec(ParseHex_expected, ParseHex_expected + 5);
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 1f13b80f3e..48ed50b9a8 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -35,7 +35,7 @@ std::string static EncodeDumpString(const std::string &str) {
     std::stringstream ret;
     for (const unsigned char c : str) {
         if (c <= 32 || c >= 128 || c == '%') {
-            ret << '%' << HexStr(Span<const unsigned char>(&c, 1));
+            ret << '%' << HexStr(Span{&c, 1});
         } else {
             ret << c;
         }

src/span.h

sipa · 2021-11-02T14:21:13Z

Thanks, @MarcoFalke! I've incorporated your suggested changes, plus a few additional simplifications in a second commit.

src/net.cpp

maflcko

review ACK 3576b93 🚛

Show signature and timestamp

Signature:

src/script/descriptor.cpp

src/test/util_tests.cpp

maflcko · 2021-11-10T18:31:23Z

I haven't tried, but to the second commit you might be able to add (after a rebase):

diff --git a/src/test/key_io_tests.cpp b/src/test/key_io_tests.cpp
index 0361618c82..02268dbcf5 100644
--- a/src/test/key_io_tests.cpp
+++ b/src/test/key_io_tests.cpp
@@ -46,7 +46,7 @@ BOOST_AUTO_TEST_CASE(key_io_valid_parse)
             privkey = DecodeSecret(exp_base58string);
             BOOST_CHECK_MESSAGE(privkey.IsValid(), "!IsValid:" + strTest);
             BOOST_CHECK_MESSAGE(privkey.IsCompressed() == isCompressed, "compressed mismatch:" + strTest);
-            BOOST_CHECK_MESSAGE(Span<const uint8_t>{privkey} == Span<const uint8_t>{exp_payload}, "key mismatch:" + strTest);
+            BOOST_CHECK_MESSAGE(Span{privkey} == Span{exp_payload}, "key mismatch:" + strTest);
 
             // Private key must be invalid public key
             destination = DecodeDestination(exp_base58string);

sipa · 2021-11-17T21:28:19Z

@MarcoFalke Done (it necessitated adding CKey::data, but that's generally useful in any case).

EDIT: nvm, the CKey::data was just added, I noticed in rebase.

sipa · 2021-11-17T21:39:41Z

Rebased and addressed some comments.

maflcko · 2021-11-24T10:44:49Z

re-ACK f90d5ae 👤

Changes:

Additional change MakeSpan -> Span in the first commit after rebase on 5ccab71
In the second commit:
- Add {} for style
- Remove now-unused template arguments from a Span constructor call
- Remove now-unused ParseHex_vec symbol from tests

Show signature and timestamp

Signature:

maflcko · 2021-11-24T11:24:22Z

message.txt:

re-ACK f90d5ae1cf5b002fe7ec1fa54e022513589721de

Changes:
* Additional change MakeSpan -> Span in the first commit after rebase on 5ccab7187b35142e03c15dd55390e45b3d233c52
* In the second commit:
  - Add {} for style
  - Remove now-unused template arguments from a Span constructor call
  - Remove now-unused ParseHex_vec symbol from tests

allowed_sig.txt:

MarcoFalke_sign_github_comments_test ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDYEowis4rvh+A4j2/2CeE7Sx/go9mzv8D7HiFRICGGl

message.txt.sig:

-----BEGIN SSH SIGNATURE-----
U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgNgSjCKziu+H4DiPb/YJ4TtLH+C
j2bO/wPseIVEgIYaUAAAAEZmlsZQAAAAAAAAAGc2hhNTEyAAAAUwAAAAtzc2gtZWQyNTUx
OQAAAEBrIf/Uab6AwMKfzBPP67zQZGpePmSDs5qtrO6w7ympI3EuiCXl655I7TIhN+Y4jN
DL5tCD8EJ5Kew8AR65PBAD
-----END SSH SIGNATURE-----

To check:

$ ssh-keygen -Y verify -f allowed_sig.txt -I MarcoFalke_sign_github_comments_test -n file -s message.txt.sig < message.txt

sipa · 2021-11-24T22:16:43Z

Rebased.

maflcko

re-ACK e4aec1e 🕙

Show signature

Signature:

src/test/util_tests.cpp

Based on suggestions by MarcoFalke <falke.marco@gmail.com>

maflcko · 2021-11-30T12:45:11Z

re-ACK 11daf6c Only change is removing a hunk in the tests 🌕

Show signature

Signature:

DrahtBot · 2021-12-03T08:37:38Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

fanquake · 2021-12-03T10:32:34Z

src/wallet/rpcdump.cpp

@@ -35,7 +35,7 @@ std::string static EncodeDumpString(const std::string &str) {
    std::stringstream ret;
    for (const unsigned char c : str) {
        if (c <= 32 || c >= 128 || c == '%') {
-            ret << '%' << HexStr(Span<const unsigned char>(&c, 1));


If anyone is confused about the bot marking this as needing a rebase, and then the merge. The conflict was in src/wallet/rpcdump.cpp. The conflicting code was moved from there, to src/wallet/rpc/backup.cpp in #23647. You can see src/wallet/rpc/backup.cpp being updated in the merge commit: 8b1de78#diff-c4eeb9bd8e84ac276465d100d6ac4c1c9b9ddc145827a03dfe11932598b028b9R38.

yeah, daily reminder to stop trusting GitHub metadata

…ion guide 1d16571 More Span simplifications (Pieter Wuille) d692560 Replace MakeSpan helper with Span deduction guide (Pieter Wuille) Pull request description: C++17 supports [user-defined deduction guides](https://en.cppreference.com/w/cpp/language/class_template_argument_deduction), allowing class constructors to be invoked without specifying class template arguments. Instead, the code can contain rules to infer the template arguments from the constructor argument types. This alleviates the need for the `MakeSpan` helper. Convert the existing MakeSpan rules into deduction rules for `Span` itself, and replace all invocations of `MakeSpan` with just `Span` ones. ACKs for top commit: MarcoFalke: re-ACK 1d16571 Only change is removing a hunk in the tests 🌕 Tree-SHA512: 10f3e82e4338f39d9b7b407cd11aac7ebe1e9191b58e3d7f4e5e338a4636c0e126b4a1d912127c7446f57ba356c8d6544482e47f97901efea6a54fffbfd7895f

@cculianu

This is a backport of bitcoin/bitcoin#23413 > C++17 supports user-defined deduction guides, allowing class constructors > to be invoked without specifying class template arguments. Instead, the > code can contain rules to infer the template arguments from the > constructor argument types. > > This alleviates the need for the MakeSpan helper. Convert the existing > MakeSpan rules into deduction rules for Span itself, and replace all > invocations of MakeSpan with just Span ones. This addresses @cculianu's [comment regarding deduction guides replacing MakeSpan](https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1411#note_798989348).

DrahtBot added Descriptors P2P RPC/REST/ZMQ TX fees and policy Wallet labels Nov 1, 2021

This was referenced Nov 1, 2021

c++20: Opt-in to modeling view and borrowed_range for Span #23226

Closed

psbt: Taproot fields for PSBT #22558

Merged

Implement BIP 370 PSBTv2 #21283

Open

maflcko added Refactoring and removed Wallet TX fees and policy RPC/REST/ZMQ P2P Descriptors labels Nov 2, 2021

DrahtBot mentioned this pull request Nov 2, 2021

[BIP 174] PSBT version, proprietary, and xpub fields #17034

Merged

maflcko reviewed Nov 2, 2021

View reviewed changes

src/span.h Outdated Show resolved Hide resolved

src/span.h Outdated Show resolved Hide resolved

sipa force-pushed the 202111_span_deduct branch from 94176a0 to a9526cf Compare November 2, 2021 14:20

sipa force-pushed the 202111_span_deduct branch 2 times, most recently from 360fa9c to 3576b93 Compare November 2, 2021 15:45

maflcko reviewed Nov 3, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

sipa mentioned this pull request Nov 4, 2021

refactor: Use spans of std::byte in serialize #23438

Merged

DrahtBot mentioned this pull request Nov 10, 2021

span: Add std::byte helpers #23451

Merged

maflcko reviewed Nov 10, 2021

View reviewed changes

src/script/descriptor.cpp Show resolved Hide resolved

src/test/util_tests.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202111_span_deduct branch from 3576b93 to 9883a03 Compare November 17, 2021 21:27

sipa force-pushed the 202111_span_deduct branch from 9883a03 to f90d5ae Compare November 17, 2021 21:37

DrahtBot mentioned this pull request Nov 23, 2021

Add external signer taproot support #23578

Merged

DrahtBot added the Needs rebase label Nov 24, 2021

sipa force-pushed the 202111_span_deduct branch from f90d5ae to e4aec1e Compare November 24, 2021 22:16

DrahtBot removed the Needs rebase label Nov 24, 2021

maflcko approved these changes Nov 25, 2021

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

fanquake requested a review from theuni November 25, 2021 08:02

DrahtBot mentioned this pull request Nov 26, 2021

util: Add ParseHex<std::byte>() helper #23595

Merged

sipa added 2 commits November 29, 2021 17:58

Replace MakeSpan helper with Span deduction guide

568dd2f

More Span simplifications

11daf6c

Based on suggestions by MarcoFalke <falke.marco@gmail.com>

sipa force-pushed the 202111_span_deduct branch from e4aec1e to 11daf6c Compare November 29, 2021 23:07

DrahtBot mentioned this pull request Dec 2, 2021

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

DrahtBot added the Needs rebase label Dec 3, 2021

maflcko merged commit 8b1de78 into bitcoin:master Dec 3, 2021

fanquake reviewed Dec 3, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 3, 2021

Merge bitcoin#23413: Replace MakeSpan helper with Span deduction guide

9f654d8

dhruv mentioned this pull request Dec 9, 2021

BIP324: Handshake prerequisites #23561

Closed

bitcoin locked and limited conversation to collaborators Dec 3, 2022

Replace MakeSpan helper with Span deduction guide #23413

Replace MakeSpan helper with Span deduction guide #23413

Uh oh!

Conversation

sipa commented Nov 1, 2021

Uh oh!

DrahtBot commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa commented Nov 2, 2021

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 10, 2021

Uh oh!

sipa commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 17, 2021

Uh oh!

maflcko commented Nov 24, 2021

Uh oh!

maflcko commented Nov 24, 2021

Uh oh!

sipa commented Nov 24, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Nov 30, 2021

Uh oh!

DrahtBot commented Dec 3, 2021

Uh oh!

fanquake Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Nov 1, 2021 •

edited

Loading

maflcko left a comment •

edited

Loading

sipa commented Nov 17, 2021 •

edited

Loading