Merge bitcoin/bitcoin#28473: refactor: Serialization parameter cleanups

fb6a2ab scripted-diff: use SER_PARAMS_OPFUNC (Anthony Towns) 5e5c8f8 serialize: add SER_PARAMS_OPFUNC (Anthony Towns) 33203f5 serialize: specify type for ParamsWrapper not ref (Anthony Towns) bf147bf serialize: move ser_action functions out of global namespace (Anthony Towns) Pull request description: Cleanups after #25284: * ser_action namespacing - bitcoin/bitcoin#25284 (comment) * make reference implicit - bitcoin/bitcoin#25284 (comment) * function notation - bitcoin/bitcoin#25284 (comment) ACKs for top commit: MarcoFalke: lgtm ACK fb6a2ab 💨 TheCharlatan: ACK fb6a2ab Tree-SHA512: aacca2ee9cfec360ade6b394606e13d1dfe05bc29c5fbdd48a4e6992bd420312d4ed0d32218d95c560646af326e9977728dc2e759990636298e326947f6f9526
bitcoin-core · Sep 15, 2023 · 5c7cdda · 5c7cdda
2 parents 717a4d8 + fb6a2ab
commit 5c7cdda
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 71 deletions.
diff --git a/src/addrdb.cpp b/src/addrdb.cpp
@@ -216,14 +216,14 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
 {
     LOG_TIME_SECONDS(strprintf("Flush %d outbound block-relay-only peer addresses to anchors.dat", anchors.size()));
-    SerializeFileDB("anchors", anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
+    SerializeFileDB("anchors", anchors_db_path, CAddress::V2_DISK(anchors));
 }
 
 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 {
     std::vector<CAddress> anchors;
     try {
-        DeserializeFileDB(anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
+        DeserializeFileDB(anchors_db_path, CAddress::V2_DISK(anchors));
         LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename())));
     } catch (const std::exception&) {
         anchors.clear();

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -1415,8 +1415,8 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
 
     const bool tx_relay{!RejectIncomingTxs(pnode)};
     m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
-            your_services, WithParams(CNetAddr::V1, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
-            my_services, WithParams(CNetAddr::V1, CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
+            your_services, CNetAddr::V1(addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
+            my_services, CNetAddr::V1(CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
             nonce, strSubVersion, nNodeStartingHeight, tx_relay));
 
     if (fLogIPs) {
@@ -3293,7 +3293,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             nTime = 0;
         }
         vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer
-        vRecv >> WithParams(CNetAddr::V1, addrMe);
+        vRecv >> CNetAddr::V1(addrMe);
         if (!pfrom.IsInboundConn())
         {
             m_addrman.SetServices(pfrom.addr, nServices);

diff --git a/src/netaddress.h b/src/netaddress.h
@@ -218,6 +218,7 @@ class CNetAddr
     };
     struct SerParams {
         const Encoding enc;
+        SER_PARAMS_OPFUNC
     };
     static constexpr SerParams V1{Encoding::V1};
     static constexpr SerParams V2{Encoding::V2};

diff --git a/src/protocol.h b/src/protocol.h
@@ -396,6 +396,7 @@ class CAddress : public CService
     };
     struct SerParams : CNetAddr::SerParams {
         const Format fmt;
+        SER_PARAMS_OPFUNC
     };
     static constexpr SerParams V1_NETWORK{{CNetAddr::Encoding::V1}, Format::Network};
     static constexpr SerParams V2_NETWORK{{CNetAddr::Encoding::V2}, Format::Network};

diff --git a/src/serialize.h b/src/serialize.h
@@ -167,9 +167,9 @@ const Out& AsBase(const In& x)
     return x;
 }
 
-#define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__))
-#define SER_READ(obj, code) ::SerRead(s, ser_action, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
-#define SER_WRITE(obj, code) ::SerWrite(s, ser_action, obj, [&](Stream& s, const Type& obj) { code; })
+#define READWRITE(...) (ser_action.SerReadWriteMany(s, __VA_ARGS__))
+#define SER_READ(obj, code) ser_action.SerRead(s, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
+#define SER_WRITE(obj, code) ser_action.SerWrite(s, obj, [&](Stream& s, const Type& obj) { code; })
 
 /**
  * Implement the Ser and Unser methods needed for implementing a formatter (see Using below).
@@ -1008,17 +1008,65 @@ void Unserialize(Stream& is, std::shared_ptr<const T>& p)
     p = std::make_shared<const T>(deserialize, is);
 }
 
+/**
+ * Support for (un)serializing many things at once
+ */
+
+template <typename Stream, typename... Args>
+void SerializeMany(Stream& s, const Args&... args)
+{
+    (::Serialize(s, args), ...);
+}
+
+template <typename Stream, typename... Args>
+inline void UnserializeMany(Stream& s, Args&&... args)
+{
+    (::Unserialize(s, args), ...);
+}
 
 /**
  * Support for all macros providing or using the ser_action parameter of the SerializationOps method.
  */
 struct ActionSerialize {
-    constexpr bool ForRead() const { return false; }
+    static constexpr bool ForRead() { return false; }
+
+    template<typename Stream, typename... Args>
+    static void SerReadWriteMany(Stream& s, const Args&... args)
+    {
+        ::SerializeMany(s, args...);
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerRead(Stream& s, Type&&, Fn&&)
+    {
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerWrite(Stream& s, Type&& obj, Fn&& fn)
+    {
+        fn(s, std::forward<Type>(obj));
+    }
 };
 struct ActionUnserialize {
-    constexpr bool ForRead() const { return true; }
-};
+    static constexpr bool ForRead() { return true; }
 
+    template<typename Stream, typename... Args>
+    static void SerReadWriteMany(Stream& s, Args&&... args)
+    {
+        ::UnserializeMany(s, args...);
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerRead(Stream& s, Type&& obj, Fn&& fn)
+    {
+        fn(s, std::forward<Type>(obj));
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerWrite(Stream& s, Type&&, Fn&&)
+    {
+    }
+};
 
 /* ::GetSerializeSize implementations
  *
@@ -1065,52 +1113,6 @@ class CSizeComputer
     int GetVersion() const { return nVersion; }
 };
 
-template <typename Stream, typename... Args>
-void SerializeMany(Stream& s, const Args&... args)
-{
-    (::Serialize(s, args), ...);
-}
-
-template <typename Stream, typename... Args>
-inline void UnserializeMany(Stream& s, Args&&... args)
-{
-    (::Unserialize(s, args), ...);
-}
-
-template<typename Stream, typename... Args>
-inline void SerReadWriteMany(Stream& s, ActionSerialize ser_action, const Args&... args)
-{
-    ::SerializeMany(s, args...);
-}
-
-template<typename Stream, typename... Args>
-inline void SerReadWriteMany(Stream& s, ActionUnserialize ser_action, Args&&... args)
-{
-    ::UnserializeMany(s, args...);
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerRead(Stream& s, ActionSerialize ser_action, Type&&, Fn&&)
-{
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerRead(Stream& s, ActionUnserialize ser_action, Type&& obj, Fn&& fn)
-{
-    fn(s, std::forward<Type>(obj));
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerWrite(Stream& s, ActionSerialize ser_action, Type&& obj, Fn&& fn)
-{
-    fn(s, std::forward<Type>(obj));
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerWrite(Stream& s, ActionUnserialize ser_action, Type&&, Fn&&)
-{
-}
-
 template<typename I>
 inline void WriteVarInt(CSizeComputer &s, I n)
 {
@@ -1161,12 +1163,11 @@ class ParamsStream
 template <typename Params, typename T>
 class ParamsWrapper
 {
-    static_assert(std::is_lvalue_reference<T>::value, "ParamsWrapper needs an lvalue reference type T");
     const Params& m_params;
-    T m_object;
+    T& m_object;
 
 public:
-    explicit ParamsWrapper(const Params& params, T obj) : m_params{params}, m_object{obj} {}
+    explicit ParamsWrapper(const Params& params, T& obj) : m_params{params}, m_object{obj} {}
 
     template <typename Stream>
     void Serialize(Stream& s) const
@@ -1190,7 +1191,20 @@ class ParamsWrapper
 template <typename Params, typename T>
 static auto WithParams(const Params& params, T&& t)
 {
-    return ParamsWrapper<Params, T&>{params, t};
+    return ParamsWrapper<Params, T>{params, t};
 }
 
+/**
+ * Helper macro for SerParams structs
+ *
+ * Allows you define SerParams instances and then apply them directly
+ * to an object via function call syntax, eg:
+ *
+ *   constexpr SerParams FOO{....};
+ *   ss << FOO(obj);
+ */
+#define SER_PARAMS_OPFUNC \
+    template <typename T> \
+    auto operator()(T&& t) const { return WithParams(*this, t); }
+
 #endif // BITCOIN_SERIALIZE_H
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
@@ -1019,7 +1019,7 @@ static auto MakeCorruptPeersDat()
     std::optional<CNetAddr> resolved{LookupHost("252.2.2.2", false)};
     BOOST_REQUIRE(resolved.has_value());
     AddrInfo info = AddrInfo(addr, resolved.value());
-    s << WithParams(CAddress::V1_DISK, info);
+    s << CAddress::V1_DISK(info);
 
     return s;
 }

diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp
@@ -83,7 +83,7 @@ CNetAddr RandAddr(FuzzedDataProvider& fuzzed_data_provider, FastRandomContext& f
         s << net;
         s << fast_random_context.randbytes(net_len_map.at(net));
 
-        s >> WithParams(CAddress::V2_NETWORK, addr);
+        s >> CAddress::V2_NETWORK(addr);
     }
 
     // Return a dummy IPv4 5.5.5.5 if we generated an invalid address.

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
@@ -850,7 +850,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     std::chrono::microseconds time_received_dummy{0};
 
     const auto msg_version =
-        msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, WithParams(CAddress::V1_NETWORK, peer_us));
+        msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us));
     CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION};
 
     m_node.peerman->ProcessMessage(
@@ -876,7 +876,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
             DataStream s{data};
             std::vector<CAddress> addresses;
 
-            s >> WithParams(CAddress::V1_NETWORK, addresses);
+            s >> CAddress::V1_NETWORK(addresses);
 
             for (const auto& addr : addresses) {
                 if (addr == expected) {

diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
@@ -561,7 +561,7 @@ BOOST_AUTO_TEST_CASE(caddress_serialize_v1)
 {
     DataStream s{};
 
-    s << WithParams(CAddress::V1_NETWORK, fixture_addresses);
+    s << CAddress::V1_NETWORK(fixture_addresses);
     BOOST_CHECK_EQUAL(HexStr(s), stream_addrv1_hex);
 }
 
@@ -570,15 +570,15 @@ BOOST_AUTO_TEST_CASE(caddress_unserialize_v1)
     DataStream s{ParseHex(stream_addrv1_hex)};
     std::vector<CAddress> addresses_unserialized;
 
-    s >> WithParams(CAddress::V1_NETWORK, addresses_unserialized);
+    s >> CAddress::V1_NETWORK(addresses_unserialized);
     BOOST_CHECK(fixture_addresses == addresses_unserialized);
 }
 
 BOOST_AUTO_TEST_CASE(caddress_serialize_v2)
 {
     DataStream s{};
 
-    s << WithParams(CAddress::V2_NETWORK, fixture_addresses);
+    s << CAddress::V2_NETWORK(fixture_addresses);
     BOOST_CHECK_EQUAL(HexStr(s), stream_addrv2_hex);
 }
 
@@ -587,7 +587,7 @@ BOOST_AUTO_TEST_CASE(caddress_unserialize_v2)
     DataStream s{ParseHex(stream_addrv2_hex)};
     std::vector<CAddress> addresses_unserialized;
 
-    s >> WithParams(CAddress::V2_NETWORK, addresses_unserialized);
+    s >> CAddress::V2_NETWORK(addresses_unserialized);
     BOOST_CHECK(fixture_addresses == addresses_unserialized);
 }
 

diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
@@ -33,9 +33,9 @@ void ConnmanTestMsg::Handshake(CNode& node,
                 Using<CustomUintFormatter<8>>(remote_services), //
                 int64_t{},                                      // dummy time
                 int64_t{},                                      // ignored service bits
-                WithParams(CNetAddr::V1, CService{}),           // dummy
+                CNetAddr::V1(CService{}),                       // dummy
                 int64_t{},                                      // ignored service bits
-                WithParams(CNetAddr::V1, CService{}),           // ignored
+                CNetAddr::V1(CService{}),                       // ignored
                 uint64_t{1},                                    // dummy nonce
                 std::string{},                                  // dummy subver
                 int32_t{},                                      // dummy starting_height