Merge bitcoin/bitcoin#27978: refactor: Drop unsafe AsBytePtr function

7c853619ee refactor: Drop unsafe AsBytePtr function (Ryan Ofsky)

Pull request description:

  Replace calls to `AsBytePtr` with calls to `AsBytes` or `reinterpret_cast`. `AsBytePtr` is just a wrapper around `reinterpret_cast`. It accepts any type of pointer as an argument and uses `reinterpret_cast` to cast the argument to a `std::byte` pointer.

  Despite taking any type of pointer as an argument, it is not useful to call `AsBytePtr` on most types of pointers, because byte representations of most types will be platform specific or undefined. Also, because it is named similarly to the `AsBytes` function, `AsBytePtr` looks safer than it actually is. Both `AsBytes` and `AsBytePtr` call reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called on `Span` objects, while `AsBytePtr` can be called on any pointer argument.

  The change was motivated by discussion on #27973 and #27927 and is compatible with those PRs

ACKs for top commit:
  jonatack:
    re-ACK 7c853619ee
  sipa:
    utACK 7c853619ee
  achow101:
    ACK 7c853619ee

Tree-SHA512: 200d858b1d4d579f081a7f9a14d488a99713b4918b4564ac3dd5c18578d927dbd6426e62e02f49f04a3fa73ca02ff7109c495cb0b92bec43c27d9b74e2f95757

src/bench/ellswift.cpp

@@ -17,12 +17,12 @@ static void EllSwiftCreate(benchmark::Bench& bench)
     uint256 entropy = GetRandHash();
 
     bench.batch(1).unit("pubkey").run([&] {
-        auto ret = key.EllSwiftCreate(AsBytes(Span{entropy}));
+        auto ret = key.EllSwiftCreate(MakeByteSpan(entropy));
         /* Use the first 32 bytes of the ellswift encoded public key as next private key. */
         key.Set(UCharCast(ret.data()), UCharCast(ret.data()) + 32, true);
         assert(key.IsValid());
         /* Use the last 32 bytes of the ellswift encoded public key as next entropy. */
-        std::copy(ret.begin() + 32, ret.begin() + 64, AsBytePtr(entropy.data()));
+        std::copy(ret.begin() + 32, ret.begin() + 64, MakeWritableByteSpan(entropy).begin());
     });
 
     ECC_Stop();

src/serialize.h

@@ -473,10 +473,10 @@ struct CustomUintFormatter
         if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
         if (BigEndian) {
             uint64_t raw = htobe64(v);
-            s.write({AsBytePtr(&raw) + 8 - Bytes, Bytes});
+            s.write(AsBytes(Span{&raw, 1}).last(Bytes));
         } else {
             uint64_t raw = htole64(v);
-            s.write({AsBytePtr(&raw), Bytes});
+            s.write(AsBytes(Span{&raw, 1}).first(Bytes));
         }
     }
 
@@ -486,10 +486,10 @@ struct CustomUintFormatter
         static_assert(std::numeric_limits<U>::max() >= MAX && std::numeric_limits<U>::min() <= 0, "Assigned type too small");
         uint64_t raw = 0;
         if (BigEndian) {
-            s.read({AsBytePtr(&raw) + 8 - Bytes, Bytes});
+            s.read(AsWritableBytes(Span{&raw, 1}).last(Bytes));
             v = static_cast<I>(be64toh(raw));
         } else {
-            s.read({AsBytePtr(&raw), Bytes});
+            s.read(AsWritableBytes(Span{&raw, 1}).first(Bytes));
             v = static_cast<I>(le64toh(raw));
         }
     }

src/span.h

@@ -243,21 +243,16 @@ T& SpanPopBack(Span<T>& span)
     return back;
 }
 
-//! Convert a data pointer to a std::byte data pointer.
-//! Where possible, please use the safer AsBytes helpers.
-inline const std::byte* AsBytePtr(const void* data) { return reinterpret_cast<const std::byte*>(data); }
-inline std::byte* AsBytePtr(void* data) { return reinterpret_cast<std::byte*>(data); }
-
 // From C++20 as_bytes and as_writeable_bytes
 template <typename T>
 Span<const std::byte> AsBytes(Span<T> s) noexcept
 {
-    return {AsBytePtr(s.data()), s.size_bytes()};
+    return {reinterpret_cast<const std::byte*>(s.data()), s.size_bytes()};
 }
 template <typename T>
 Span<std::byte> AsWritableBytes(Span<T> s) noexcept
 {
-    return {AsBytePtr(s.data()), s.size_bytes()};
+    return {reinterpret_cast<std::byte*>(s.data()), s.size_bytes()};
 }
 
 template <typename V>

src/wallet/bdb.cpp

@@ -31,6 +31,10 @@
 
 namespace wallet {
 namespace {
+Span<const std::byte> SpanFromDbt(const SafeDbt& dbt)
+{
+    return {reinterpret_cast<const std::byte*>(dbt.get_data()), dbt.get_size()};
+}
 
 //! Make sure database has a unique fileid within the environment. If it
 //! doesn't, throw an error. BDB caches do not work properly when more than one
@@ -702,7 +706,7 @@ DatabaseCursor::Status BerkeleyCursor::Next(DataStream& ssKey, DataStream& ssVal
         return Status::FAIL;
     }
 
-    Span<const std::byte> raw_key = {AsBytePtr(datKey.get_data()), datKey.get_size()};
+    Span<const std::byte> raw_key = SpanFromDbt(datKey);
     if (!m_key_prefix.empty() && std::mismatch(raw_key.begin(), raw_key.end(), m_key_prefix.begin(), m_key_prefix.end()).second != m_key_prefix.end()) {
         return Status::DONE;
     }
@@ -711,7 +715,7 @@ DatabaseCursor::Status BerkeleyCursor::Next(DataStream& ssKey, DataStream& ssVal
     ssKey.clear();
     ssKey.write(raw_key);
     ssValue.clear();
-    ssValue.write({AsBytePtr(datValue.get_data()), datValue.get_size()});
+    ssValue.write(SpanFromDbt(datValue));
     return Status::MORE;
 }
 
@@ -796,7 +800,7 @@ bool BerkeleyBatch::ReadKey(DataStream&& key, DataStream& value)
     int ret = pdb->get(activeTxn, datKey, datValue, 0);
     if (ret == 0 && datValue.get_data() != nullptr) {
         value.clear();
-        value.write({AsBytePtr(datValue.get_data()), datValue.get_size()});
+        value.write(SpanFromDbt(datValue));
         return true;
     }
     return false;

src/wallet/sqlite.cpp

@@ -24,6 +24,12 @@
 namespace wallet {
 static constexpr int32_t WALLET_SCHEMA_VERSION = 0;
 
+static Span<const std::byte> SpanFromBlob(sqlite3_stmt* stmt, int col)
+{
+    return {reinterpret_cast<const std::byte*>(sqlite3_column_blob(stmt, col)),
+            static_cast<size_t>(sqlite3_column_bytes(stmt, col))};
+}
+
 static void ErrorLogCallback(void* arg, int code, const char* msg)
 {
     // From sqlite3_config() documentation for the SQLITE_CONFIG_LOG option:
@@ -434,10 +440,8 @@ bool SQLiteBatch::ReadKey(DataStream&& key, DataStream& value)
         return false;
     }
     // Leftmost column in result is index 0
-    const std::byte* data{AsBytePtr(sqlite3_column_blob(m_read_stmt, 0))};
-    size_t data_size(sqlite3_column_bytes(m_read_stmt, 0));
     value.clear();
-    value.write({data, data_size});
+    value.write(SpanFromBlob(m_read_stmt, 0));
 
     sqlite3_clear_bindings(m_read_stmt);
     sqlite3_reset(m_read_stmt);
@@ -527,12 +531,8 @@ DatabaseCursor::Status SQLiteCursor::Next(DataStream& key, DataStream& value)
     value.clear();
 
     // Leftmost column in result is index 0
-    const std::byte* key_data{AsBytePtr(sqlite3_column_blob(m_cursor_stmt, 0))};
-    size_t key_data_size(sqlite3_column_bytes(m_cursor_stmt, 0));
-    key.write({key_data, key_data_size});
-    const std::byte* value_data{AsBytePtr(sqlite3_column_blob(m_cursor_stmt, 1))};
-    size_t value_data_size(sqlite3_column_bytes(m_cursor_stmt, 1));
-    value.write({value_data, value_data_size});
+    key.write(SpanFromBlob(m_cursor_stmt, 0));
+    value.write(SpanFromBlob(m_cursor_stmt, 1));
     return Status::MORE;
 }