net: Use serialization parameters for CAddress serialization #25284

pull maflcko wants to merge 6 commits into bitcoin:master from maflcko:2206-split-consensus-net-😻 changing 26 files +594 −297
  1. maflcko commented at 1:04 pm on June 6, 2022: member

    It seems confusing that picking a wrong value for ADDRV2_FORMAT could have effects on consensus. (See the docstring of ADDRV2_FORMAT).

    Fix this by implementing #19477 (comment) .

    This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327

  2. DrahtBot added the label Refactoring on Jun 6, 2022
  3. vasild commented at 1:44 pm on June 6, 2022: contributor
    Concept ACK
  4. DrahtBot commented at 9:06 am on June 7, 2022: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan, ajtowns
    Concept ACK vasild
    Stale ACK jonatack, theuni

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26859 (fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses by vasild)
    • #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
    • #24034 (p2p: delete anchors.dat after trying to connect to that peers by brunoerg)

    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.

  5. maflcko force-pushed on Jun 11, 2022
  6. maflcko force-pushed on Jun 12, 2022
  7. luke-jr commented at 4:12 pm on June 18, 2022: member

    It seems confusing to have consensus (weakly) depend on low level BIP 155 addrv2 serialization.

    You mean the “consensus” module, right, not actual consensus?

  8. DrahtBot added the label Needs rebase on Jul 12, 2022
  9. fanquake referenced this in commit 895937edb2 on Jul 20, 2022
  10. maflcko force-pushed on Jul 22, 2022
  11. DrahtBot removed the label Needs rebase on Jul 22, 2022
  12. fanquake referenced this in commit 510ac41eac on Jul 22, 2022
  13. maflcko force-pushed on Jul 22, 2022
  14. DrahtBot added the label Needs rebase on Jul 27, 2022
  15. maflcko force-pushed on Aug 1, 2022
  16. DrahtBot removed the label Needs rebase on Aug 1, 2022
  17. DrahtBot added the label Needs rebase on Sep 16, 2022
  18. maflcko force-pushed on Dec 15, 2022
  19. DrahtBot removed the label Needs rebase on Dec 15, 2022
  20. DrahtBot added the label Needs rebase on Jan 19, 2023
  21. fanquake referenced this in commit 79e007d1d6 on Jan 26, 2023
  22. maflcko force-pushed on Jan 26, 2023
  23. DrahtBot removed the label Needs rebase on Jan 26, 2023
  24. maflcko force-pushed on Jan 26, 2023
  25. fanquake referenced this in commit 0a1d372ad0 on Jan 30, 2023
  26. sidhujag referenced this in commit 789b476bab on Jan 30, 2023
  27. maflcko force-pushed on Jan 31, 2023
  28. maflcko renamed this:
    [WIP] consensus: Remove dependency on net (BIP 155 / ADDRV2_FORMAT)
    consensus: Remove dependency on ADDRV2_FORMAT
    on Jan 31, 2023
  29. maflcko force-pushed on Jan 31, 2023
  30. maflcko renamed this:
    consensus: Remove dependency on ADDRV2_FORMAT
    net: Use serialization parameters for CAddress serialization
    on Jan 31, 2023
  31. DrahtBot renamed this:
    net: Use serialization parameters for CAddress serialization
    net: Use serialization parameters for CAddress serialization
    on Jan 31, 2023
  32. maflcko marked this as ready for review on Jan 31, 2023
  33. DrahtBot added the label Needs rebase on Jan 31, 2023
  34. maflcko force-pushed on Feb 1, 2023
  35. DrahtBot removed the label Needs rebase on Feb 1, 2023
  36. achow101 requested review from ryanofsky on Apr 25, 2023
  37. DrahtBot added the label Needs rebase on May 30, 2023
  38. maflcko force-pushed on Jul 6, 2023
  39. maflcko force-pushed on Jul 6, 2023
  40. DrahtBot added the label CI failed on Jul 6, 2023
  41. maflcko force-pushed on Jul 6, 2023
  42. DrahtBot removed the label Needs rebase on Jul 6, 2023
  43. maflcko force-pushed on Jul 6, 2023
  44. DrahtBot removed the label CI failed on Jul 6, 2023
  45. DrahtBot added the label Needs rebase on Jul 13, 2023
  46. maflcko force-pushed on Jul 14, 2023
  47. DrahtBot removed the label Needs rebase on Jul 14, 2023
  48. jonatack commented at 2:20 am on July 28, 2023: contributor
    Concept ACK, will have a look.
  49. DrahtBot added the label Needs rebase on Aug 3, 2023
  50. maflcko force-pushed on Aug 4, 2023
  51. maflcko force-pushed on Aug 4, 2023
  52. DrahtBot added the label CI failed on Aug 4, 2023
  53. DrahtBot removed the label Needs rebase on Aug 4, 2023
  54. in src/serialize.h:137 in 6a99da9fa4 outdated
    133@@ -133,12 +134,29 @@ enum
    134     SER_GETHASH         = (1 << 2),
    135 };
    136 
    137-//! Convert the reference base type to X, without changing constness or reference type.
    138-template<typename X> X& ReadWriteAsHelper(X& x) { return x; }
    139-template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
    140+/* Convert any argument to a reference to X, maintaining constness.
    


    jonatack commented at 6:49 pm on August 4, 2023:

    ef03a51

    0/** Convert any argument to a reference to X, maintaining constness.
    

    also here

    0-/* ::GetSerializeSize implementations
    1+/** ::GetSerializeSize implementations
    

    maflcko commented at 10:26 am on August 9, 2023:
    Thanks, fixed doxygen
  55. in src/test/serialize_tests.cpp:282 in 6a99da9fa4 outdated
    277+            s >> str;
    278+            assert(str.size() == 2 || (fmt == BaseFormat::DEC && str.size() == 3));
    279+            uint32_t data;
    280+            bool ok;
    281+            if (fmt == BaseFormat::DEC) {
    282+                ok = ParseUint32(str, &data);
    


    jonatack commented at 7:21 pm on August 4, 2023:

    6a99da9 If the #if directive is inversed

    0test/serialize_tests.cpp:281:22: error: use of undeclared identifier 'ParseUint32'; did you mean 'ParseUInt32'?
    1                ok = ParseUint32(str, &data);
    2                     ^~~~~~~~~~~
    3                     ParseUInt32
    4./util/strencodings.h:225:20: note: 'ParseUInt32' declared here
    5[[nodiscard]] bool ParseUInt32(std::string_view str, uint32_t *out);
    

    maflcko commented at 10:26 am on August 9, 2023:

    Good catch. The test was written by @vasild. I’ve removed the code, since there is already another unit test checking SERIALIZE_METHODS_PARAMS.

    Also, I’ve replaced DEC by RAW to remove the dependency on tinyformat for the unit test and instead only use Bitcoin Core’s own util functions.

  56. jonatack commented at 7:26 pm on August 4, 2023: contributor
    Initial tested approach ACK 6a99da9fa4a66e343df0910918da0ba9686d0fde. Quite a lot going on in these changes. The template changes in b03572d8381 are nicely simplified from the original in #19503.
  57. DrahtBot removed the label CI failed on Aug 4, 2023
  58. maflcko force-pushed on Aug 9, 2023
  59. maflcko force-pushed on Aug 9, 2023
  60. maflcko force-pushed on Aug 9, 2023
  61. in src/serialize.h:156 in 667cf7e210 outdated
    154+ *
    155+ * static_cast cannot easily be used here, as the type of Obj will be const Child&
    156+ * during serialization and Child& during deserialization. AsBase will convert to
    157+ * const Base& and Base& appropriately.
    158+ */
    159+template <typename X> X& AsBase(X& x) { return x; }
    


    theuni commented at 8:50 pm on August 22, 2023:
    Maybe constrain this with std::is_base_of and SFINAE until we get concepts?

    maflcko commented at 8:48 am on August 23, 2023:
    Would there be a reason to check std::is_base_of? IIUC the compiler already checks this internally on the argument when the function is called. And if the function call argument is a violating reinterpret_cast we can’t protect against this anyway inside this function or the function signature.

    ajtowns commented at 1:10 pm on August 23, 2023:

    It will try conversion functions, eg:

     0template<typename T> T& AsBase(T& x) { return x; }
     1
     2template<typename T, typename A, 
     3    typename = typename std::enable_if<
     4        std::is_base_of<T, A>::value, void
     5    >::type>
     6T& AsBaseSafe(A& x) { return x; }
     7
     8struct Foo { };
     9struct Bar : public Foo { };
    10
    11Bar b;
    12Foo& fb = AsBase<Foo>(b); // fine
    13Foo& fbsafe = AsBaseSafe<Foo>(b); // fine
    14
    15Bar g_foobar;
    16struct Nope { operator Foo&() { return g_foobar; }; };
    17Nope n;
    18Foo& f = AsBase<Foo>(n); // fine
    19Foo& fsafe = AsBaseSafe<Foo>(n); // error: no matching function for call to 'AsBaseSafe'
    20
    21struct Nope2 { operator Bar() { return g_foobar; }; };
    22Nope2 n2;
    23Bar& n2b = AsBase<Bar>(n2); // error: no matching function for call to 'AsBase'
    24const Bar& n2bc = AsBase<const Bar>(n2); // fine
    25const Foo& n2fc = AsBase<const Foo>(n2); // fine
    26const Bar& n2bcsafe = AsBaseSafe<const Bar>(n2); // error: no matching...
    27const Foo& n2fcsafe = AsBaseSafe<const Foo>(n2); // error: no matching...
    

    maflcko commented at 2:43 pm on August 23, 2023:
    I don’t think we use conversion functions in this codebase, but I’ve added the static_assert to turn it into a compile failure.
  62. theuni commented at 8:52 pm on August 22, 2023: member

    Concept ACK.

    Would be useful for #28327 as well.

  63. maflcko force-pushed on Aug 23, 2023
  64. theuni commented at 7:36 pm on August 23, 2023: member

    Edit: Most of the below was written with a misunderstanding of how these params actually work. I was trying (unsuccessfully) to shoe-horn params into the existing stream behavior. I’ll leave my clueless comments in case they help anyone else with the same misunderstanding. See this follow-up comment for a more reasonable take.


    What is the future of s.GetType() & SER_GETHASH and s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS ?

    I’m working on trying to switch #28327 to this approach, but I’m running into some issues.

    CBlockLocator and CDiskBlockIndex both check for SER_GETHASH and conditionally skip some serializing if we’re hashing. So i’ve introduced a bool hashing in SerParams for those classes.

    But now we have a problem in net_processing: https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/net_processing.cpp#L2630

    https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/netmessagemaker.h#L18-L24

    It’s not clear how to set !hashing for locator there.

    One solution is to have global params which could apply to any class:

     0struct GlobalParams {
     1    enum class hashing{
     2        on,
     3        off
     4    };
     5    bool is_hashing() const
     6    {
     7        return do_hashing == hashing::on;
     8    }
     9    hashing do_hashing;
    10};
    

    Then places like netmessagemaker can override for all structures:

    0 CSerializedNetMsg Make(int nFlags, std::string msg_type, Args&&... args) const 
    1 { 
    2    CSerializedNetMsg msg;
    3    msg.m_type = std::move(msg_type);
    4    CVectorWriter vecwriter{ SER_NETWORK, nFlags | nVersion, msg.data, 0};
    5    GlobalParams ser_params{GlobalParams::hashing::off};
    6    ParamsStream s{ser_params, vecwriter};
    7    ::SerializeMany(s, std::forward<Args>(args)...);
    8    return msg;
    9}
    

    That works but:

    • SER_NETWORK and GlobalParams::hashing::off are redundant
    • Now CTransaction fails to serialize because ParamsStream deletes GetVersion().

    Finally.. that could be fixed by turning SERIALIZE_TRANSACTION_NO_WITNESS into a global param as well. But that seems like it puts us back where we started, with everything jumbled up in the same place.

    So while I appreciate the approach here for CAddress, it’s not clear to me how it interacts with the last few serialization quirks.

  65. theuni commented at 8:05 pm on August 23, 2023: member

    It’s also unclear to me how this should work when the thing-to-be-serialized is a template param: https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/dbwrapper.h#L168-L177

    In that case we’ll need to set some params for some types of V but not others. How to do that generically?

  66. sipa commented at 8:28 pm on August 23, 2023: member

    @theuni This is very generic, and I haven’t tried any of this, but roughly my thinking is the following.

    For transactions (CTransaction, CMutableTransaction), I’d expect a TransactionParams with a boolean that indicates whether the witness is to be serialized or not (and whether it’s accepted on deserialization). I’d think we’d want that passed through when serializing blocks, so CBlock would also want a TransactionParams. But “higher” than that (e.g. in CNetMsgMaker or database logic) it would feel wrong to try to support having TransactionParams, because that code shouldn’t know or care about what blocks or transactions are. Instead, whenever there is code that tries to give (say) a transaction to a database writing, that code should instead serialize a WithParams(TransactionParams{.segwit=...}, tx) rather than a tx, because presumably that code knows whether witness serialization is supposed to be used. In a sense, my view is that the real problem is trying to even have a way to “globally” set witness or not - it should be confined to logic that knows what that means.

    For the distinct serializations of CBlockLocator and CDiskBlockIndex, I suspect there are similar things possible. Yes, introduce a parameter, locally for them, and we’ll find a place not much higher in the stack where the parameter value can be set, without needing to go all the way to very generic code.

  67. in src/serialize.h:1189 in fa7d40795b outdated
    1184+ * Return a wrapper around t that (de)serializes it with specified parameter params.
    1185+ *
    1186+ * See FORMATTER_METHODS_PARAMS for more information on serialization parameters.
    1187+ */
    1188+template <typename Params, typename T>
    1189+static auto WithParams(const Params& params, T&& t)
    


    sipa commented at 8:33 pm on August 23, 2023:
    I think this function could be replaced with a deduction guide for ParamsWrapper.

    maflcko commented at 6:44 am on August 26, 2023:
    Thanks, I’ll look into this style nit on the next force-push or in a follow-up.
  68. ajtowns commented at 7:28 am on August 24, 2023: contributor

    should instead give a WithParams(TransactionParams{.segwit=...}, tx) rather than tx to be serialized,

    Maybe something like:

    • replace CTransaction::Serialize(stream) with CTransaction::Serialize(stream, txseropts) – so bare CTransaction isn’t serializable anymore
    • add an automatic wrapper, so instead of stream << tx you write stream << seropt(tx, {.with_witness=true})

    In that case, I think the idea could just be to have that wrapper be something like:

     0template<typename T, typename O>
     1class serializable_with_options
     2{
     3private:
     4    T& obj;
     5    O opts;
     6public:
     7    serializable_with_options(T& obj, O&& opts) : obj{obj}, opts{std::move(opts)} { }
     8
     9    template<typename Stream>
    10    inline void Serialize(Stream& s) const { obj.Serialize(s, opts); }
    11
    12    template<typename Stream> 
    13    inline void Unserialize(Stream& s) { obj.Unserialize(s, opts); }
    14};
    15
    16template<typename T>
    17auto seropt(T& obj, typename T::SerOptions&& opts)
    18{
    19    return serializable_with_options<T, typename T::SerOptions>(obj, std::move(opts));
    20}
    

    Probably needs some further tweaking to handle constyness?

    Here’s a patch that may be fun to play with: https://github.com/ajtowns/bitcoin/commit/19cdc6b66cdf3c27d1fe895c39ab4075372bf99b . Seems workable?

    EDIT: On second thoughts, it doesn’t seem very workable – inheriting CNetAddr options through its child classes and across vectors of CNetAddrs and such kinda sucks.

  69. theuni commented at 2:00 pm on August 25, 2023: member

    For the distinct serializations of CBlockLocator and CDiskBlockIndex, I suspect there are similar things possible. Yes, introduce a parameter, locally for them, and we’ll find a place not much higher in the stack where the parameter value can be set, without needing to go all the way to very generic code. @sipa Thanks for the explanation. After playing with this some more I realized that I had a pretty significant misunderstanding, having a hard time not still thinking in terms of setting options for the entire stream. I see what you mean now about setting the param at the appropriate level in the stack.

    I took another stab at a POC for using params for CBlockLocator as you described, which I think turned out looking quite reasonable (ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).

    So while I appreciate the approach here for CAddress, it’s not clear to me how it interacts with the last few serialization quirks.

    I take that back. Having now implemented some of those quirks in the commit above, it interacts just fine.

  70. maflcko commented at 3:14 pm on August 25, 2023: member

    (ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).

    I don’t think anything does, but I am not aware of a simple way to find out at compile time. Maybe use this pull request and remove the GetType definition and confirm it only fails in CTransaction/CBlock? Slightly related, most uses of the legacy interface CHashWriter are not needed at all, see https://github.com/bitcoin/bitcoin/pull/28341

  71. maflcko force-pushed on Aug 25, 2023
  72. maflcko commented at 4:02 pm on August 25, 2023: member
    Added a commit to remove code
  73. theuni commented at 4:45 pm on August 25, 2023: member

    (ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).

    I don’t think anything does, but I am not aware of a simple way to find out at compile time. Maybe use this pull request and remove the GetType definition and confirm it only fails in CTransaction/CBlock? Slightly related, most uses of the legacy interface CHashWriter are not needed at all, see #28341

    It’s worth pointing out that, as you mentioned, it’s hard to trace this path in the current code. But a nice feature of the approach in the PR is that the params are now enforced at compile time and closer to the callsite, so it’s trivial to see what’s being used.

    So after converting CBLockLocator, compile fails for every serialization because params are missing. That’s fixed by switching CDataStream -> DataStream and handling the params. If none of the deleted CDataStream usages were SER_GETHASH, I think it’s safe to say we don’t need the hashing param at all?

    And yes, from a quick check, that’s the case everywhere but CTransaction. So specifically, I propose that we nuke the hashing checks in CDiskBlockIndex/CBLockLocator/CKeyPool. Presumably the lack of usage means that a hash has never had meaning for those. There’s still the question about whether or not to use dummy nVersions though.

  74. theuni approved
  75. theuni commented at 7:13 pm on August 25, 2023: member
    ACK fa452431db62f780abd3e4cb34ab9523214b9d52.
  76. DrahtBot requested review from jonatack on Aug 25, 2023
  77. theuni commented at 9:42 pm on August 26, 2023: member

    From #19503 :

    In this PR, the feature is only used for CAddress serialization, but I plan to also convert SERIALIZE_TRANSACTION_NO_WITNESS in a follow-up. @sipa follow-up reminder in case this gets merged :p

    Combined with #28341 and #25284 (comment), that would go a long way towards (,if not completely) eliminating the legacy flags.

  78. in src/netaddress.h:220 in fab1d5bcf2 outdated
    211@@ -220,13 +212,21 @@ class CNetAddr
    212         return IsIPv4() || IsIPv6() || IsTor() || IsI2P() || IsCJDNS();
    213     }
    214 
    215+    enum class Encoding {
    216+        V1,
    217+        V2, //!< BIP155 encoding
    218+    };
    219+    struct SerParams {
    220+        const Encoding enc;
    


    ajtowns commented at 5:48 am on August 27, 2023:

    Is this intended to default to V1? If so, be explicit with enc{V1}; if not, maybe const NoDefaultInitializer<Encoding> enc:

     0template<typename T>
     1struct NoDefaultInitializer
     2{
     3    T value;
     4    NoDefaultInitializer(const T& v) : value{v} { }
     5    NoDefaultInitializer(T&& v) : value{std::move(v)} { }
     6    NoDefaultInitializer() = delete;
     7    NoDefaultInitializer(NoDefaultInitializer&&) = default;
     8    NoDefaultInitializer(const NoDefaultInitializer&) = default;
     9    operator T&() { return value; }
    10    operator const T&() const { return value; }
    11    T& operator=(const T& v) { value = v; return value; }
    12};
    

    maflcko commented at 3:37 pm on August 28, 2023:

    SerParams a; is illegal due to const, so I guess you mean SerParams b{};. But that type of bug seems impossible to fix, because one can just write NoDefaultInitializer<Encoding> d{{}};, or SerParams b{{}};.

    So I’ll leave as is for now.


    maflcko commented at 3:39 pm on August 28, 2023:

    But that type of bug seems impossible to fix

    Or rather, this may be a clang-tidy-plugin candidate, no?


    ajtowns commented at 4:14 pm on August 28, 2023:
    I was more thinking just as a way to avoid missing entries by accident, so writing {.addrv2=true} and forgetting that you should be specifying .disk=false as well. If you’re writing .disk=NoDefaultInitializaer<bool>{{}} then you’re still at least explicitly aware of the parameter…

    maflcko commented at 5:04 pm on August 28, 2023:

    Ah, at least for my code, I already get a warning with clang-16:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
     1index 0582da423f..98699fd505 100644
     2--- a/src/net_processing.cpp
     3+++ b/src/net_processing.cpp
     4@@ -5283,7 +5283,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
     5         msg_type = NetMsgType::ADDR;
     6         ser_enc = CNetAddr::Encoding::V1;
     7     }
     8-    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
     9+    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}}, peer.m_addrs_to_send)));
    10     peer.m_addrs_to_send.clear();
    11 
    12     // we only send the big addr message once
    
    0net_processing.cpp:5286:127: warning: missing field 'fmt' initializer [-Wmissing-field-initializers]
    1    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}}, peer.m_addrs_to_send)));
    2                                                                                                                              ^
    31 warning generated.
    

    theuni commented at 5:34 pm on August 28, 2023:

    Or rather, this may be a clang-tidy-plugin candidate, no?

    Here’s a very quick POC in case this is desired: https://github.com/theuni/bitcoin/commit/caad1926ab4c4ee7a7f5bc6979228afd8500350d

    It’s not tested against all forms of init/inheritance, so we’d likely need to fix it up a bit.

    Also the max params are hard-coded because of an AST traversal quirk. That limit could be removed with a less strict match and fancier code in SerParamsCheck::check.


    maflcko commented at 7:10 pm on August 28, 2023:
    Nice, can be submitted in a follow-up pull?
  79. in src/test/addrman_tests.cpp:701 in fab1d5bcf2 outdated
    696@@ -697,16 +697,17 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
    697     auto addrman_asmap1_dup = std::make_unique<AddrMan>(netgroupman, DETERMINISTIC, ratio);
    698     auto addrman_noasmap = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, ratio);
    699 
    700-    CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
    701+    DataStream stream{};
    702+    const CAddress::SerParams ser_params{{CNetAddr::Encoding::V1}, CAddress::Format::Network};
    


    ajtowns commented at 5:53 am on August 27, 2023:

    Perhaps:

    0class CAddress {
    1    ...
    2    static constexpr SerParams network_v1{{Encoding::V1}, Format::Network};
    3};
    

    would be a bit friendlier?


    maflcko commented at 3:47 pm on August 28, 2023:
    It is just a test, so I think it is fine, no?

    ajtowns commented at 4:59 pm on August 28, 2023:
    You construct V1 params a couple of times in net_processing as well, just thought having them already done might be easier? You’d be saying WithParams(CAddress::V1_DISK, info) rather than WithParams(ser_params, info) or whatever, so it’d be slightly clearer inline as well as maybe saving a redeclaration?

    maflcko commented at 7:03 pm on August 28, 2023:

    Makes sense, pushed a change to reduce verbosity.

    The diff should be scripted-diff, roughly:

    0sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Disk}|CAddress::V1_DISK|g'                                   $(git grep -l SerParams)
    1sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V2}, CAddress::Format::Disk}|CAddress::V2_DISK|g'                                   $(git grep -l SerParams)
    2sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V2}, CAddress::Format::Network}|CAddress::V2_NETWORK|g'                             $(git grep -l SerParams)
    3sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Network}|CAddress::V1_NETWORK|g'                             $(git grep -l SerParams)
    4sed -i 's|CAddress::SerParams ser_params{{CNetAddr::Encoding::V1}, CAddress::Format::Network}|auto ser_params{CAddress::V1_NETWORK}|g' $(git grep -l SerParams)
    5sed -i 's|CAddress::SerParams ser_params{{CNetAddr::Encoding::V2}, CAddress::Format::Network}|auto ser_params{CAddress::V2_NETWORK}|g' $(git grep -l SerParams)
    6sed -i 's|CNetAddr::SerParams ser_params{CNetAddr::Encoding::V1}|auto ser_params{CNetAddr:V1}|g'                                       $(git grep -l SerParams)
    7sed -i 's|CNetAddr::SerParams{CNetAddr::Encoding::V1}|CNetAddr::V1|g'                                                                  $(git grep -l SerParams)
    8sed -i 's|CNetAddr::SerParams{CNetAddr::Encoding::V2}|CNetAddr::V2|g'                                                                  $(git grep -l SerParams)
    
  80. in src/net_processing.cpp:5278 in fab1d5bcf2 outdated
    5274@@ -5272,15 +5275,15 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
    5275     if (peer.m_addrs_to_send.empty()) return;
    5276 
    5277     const char* msg_type;
    5278-    int make_flags;
    5279+    CNetAddr::Encoding ser_params;
    


    ajtowns commented at 6:34 am on August 27, 2023:
    This is an encoding, not a ser_params

    maflcko commented at 3:47 pm on August 28, 2023:
    Thanks, changed name.
  81. in src/netaddress.h:231 in fab1d5bcf2 outdated
    225      */
    226     template <typename Stream>
    227     void Serialize(Stream& s) const
    228     {
    229-        if (s.GetVersion() & ADDRV2_FORMAT) {
    230+        if (s.GetParams().enc == Encoding::V2) {
    


    ajtowns commented at 7:09 am on August 27, 2023:

    If the stream has parameters but not the right parameters this gives a mildly confusing error. Writing it as:

    0    template <typename Stream>
    1    void Serialize(Stream& s, const SerParams& params) const
    2
    3    template <typename Stream> void Serialize(Stream& s) const { return Serialize(s, s.GetParams()); }
    

    seems a bit nicer. It seems like there should be some way to have ::Serialize() call this with the parameters directly via some template magic, but I can’t see what it would be.


    maflcko commented at 3:49 pm on August 28, 2023:
    I could also add an const SerParams& params{s.GetParams()}; in the first line of the function.

    ajtowns commented at 4:45 pm on August 28, 2023:
    Yeah; with C++20 concepts I could get ::Serialize to call obj.Serialize(s, s.GetParams()) directly which seemed nicer, but with just C++17 I got myself too confused.

    maflcko commented at 7:09 pm on August 28, 2023:
    Ok, let’s improve the warning once and if there is C++20

    maflcko commented at 11:10 am on February 9, 2024:
    Fixed in the first commit of https://github.com/bitcoin/bitcoin/pull/28929
  82. in src/test/addrman_tests.cpp:998 in fab1d5bcf2 outdated
     998+    auto ssPeers2{AddrmanToStream(addrman)};
     999 
    1000     AddrMan addrman2{EMPTY_NETGROUPMAN, !DETERMINISTIC, GetCheckRatio(m_node)};
    1001     BOOST_CHECK(addrman2.Size() == 0);
    1002-    ReadFromStream(addrman2, ssPeers2);
    1003+    ReadFromStreamUnitTests(WithParams(CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Disk}, addrman2), ssPeers2);
    


    ajtowns commented at 7:12 am on August 27, 2023:

    FWIW, I would have written:

    0WithParams(CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Disk}, addrman2)
    

    more like:

    0WithParams<CAddress>({.addrv2=false, .disk=true}, addrman2)
    

    maflcko commented at 7:09 pm on August 28, 2023:

    It is now:

    0WithParams(CAddress::V1_DISK, addrman2)
    
  83. in src/protocol.h:399 in fab1d5bcf2 outdated
    395+        Disk,
    396+        Network,
    397+    };
    398+    struct SerParams : CNetAddr::SerParams {
    399+        const Format fmt;
    400+    };
    


    ajtowns commented at 7:15 am on August 27, 2023:
    If you want to use designated initializers, a la {.addrv2=false, .disk=false}, then you can’t use inheritance here, but could instead add an operator CNetAddr::SerParams() const function for pretty much the same result.

    maflcko commented at 3:50 pm on August 28, 2023:
    Yeah, inheritance is a bit verbose, but I like that it assigns each parameter the exact class name it is used for. Maybe if you post your full diff, I can consider it?

    ajtowns commented at 4:53 pm on August 28, 2023:

    Instead of WithParams(Foo::SerParams{Foo::Bar::VALUE}, obj) I had seropt<Foo>(obj, {.var=value}); implemented something like:

     0template<typename O, typename T>
     1inline auto seropt(T& obj, const SerializationOptions<O>& opts) {
     2   return ParamsWrapper<SerializationOptions<O>,T>(opts, obj);
     3}
     4
     5template<typename T>
     6inline auto seropt(T& obj, const SerializationOptions<T>& opts) { return seropt<T,T>(obj, opts); }
     7
     8template<>
     9struct SerializationOptions<class CAddress> {
    10    bool addrv2{false};
    11    bool to_disk{false};
    12    operator SerializationOptions<class CNetAddr>() const { return {.addrv2=addrv2}; }
    13};
    

    Saves a bit of boiler plate, not sure it’s entirely enough to be worthwhile.


    maflcko commented at 7:07 pm on August 28, 2023:
    I think the last push removed most of the boilerplate, no?

    ajtowns commented at 12:02 pm on September 4, 2023:
    Yeah, looks fine
  84. ajtowns commented at 7:21 am on August 27, 2023: contributor

    Approach ACK fa452431db62f780abd3e4cb34ab9523214b9d52

    Essentially ended up reinventing this PR from scratch trying to figure out a nicer way of doing things

  85. Replace READWRITEAS macro with AsBase wrapping function
    Co-authored-by: Pieter Wuille <pieter@wuille.net>
    aaaa3fa947
  86. Rename CSerAction* to Action*
    This allows new code, added in the next commit, to conform to the coding
    guideline: No C-prefix for class names.
    fac42e9d35
  87. Support for serialization parameters
    The moved part can be reviewed with the git options
     --ignore-all-space --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
    
    (Modified by Marco Falke)
    
    Co-authored-by: Pieter Wuille <pieter@wuille.net>
    faec591d64
  88. maflcko force-pushed on Aug 28, 2023
  89. maflcko commented at 3:53 pm on August 28, 2023: member
    Rebased and changed a variable name. Should be trivial to re-ACK.
  90. maflcko force-pushed on Aug 28, 2023
  91. maflcko commented at 7:11 pm on August 28, 2023: member
    Reworked the main commit
  92. in src/net_processing.cpp:1419 in fa0b988a15 outdated
    1413@@ -1414,9 +1414,10 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
    1414     uint64_t your_services{addr.nServices};
    1415 
    1416     const bool tx_relay{!RejectIncomingTxs(pnode)};
    1417+    const auto ser_params{CNetAddr::V1};
    1418     m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
    1419-            your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
    1420-            my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
    1421+            your_services, WithParams(ser_params, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
    


    theuni commented at 8:38 pm on August 28, 2023:
    Nit: using ser_params here (and in ProcessMessage) seems less clear than just using CNetAddr::V1 directly.

    maflcko commented at 6:50 am on August 29, 2023:

    Yes, good point. It makes little sense to assign an existing short alias to a const auto alias when it is only used once or twice.

    Fixed in all files in this commit.

  93. theuni approved
  94. theuni commented at 8:41 pm on August 28, 2023: member

    Re-ACK fafca6b73310a4449918b95e3895ac434a2e096a

    Didn’t re-review tests, only the main updates.

  95. maflcko force-pushed on Aug 29, 2023
  96. in src/serialize.h:1137 in faec591d64 outdated
    1133@@ -1080,4 +1134,61 @@ size_t GetSerializeSizeMany(int nVersion, const T&... t)
    1134     return sc.size();
    1135 }
    1136 
    1137+/** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
    


    TheCharlatan commented at 11:32 am on September 1, 2023:
    Where is this GetParams defined that the method here overrides? Does this refer to the underlying type instead?

    maflcko commented at 8:19 am on September 5, 2023:

    Good question. I wonder if it makes sense to override the GetParams() function at all. Generally, no underlying stream type has a GetParams() method, so “overriding” really means “defines”.

    And if the GetParams() method is truly overridden, it should be rare, and it could make more sense to first require the stream to reveal its underlying unwrapped stream, without the GetParams() method?


    maflcko commented at 8:22 am on September 5, 2023:
    (My plan was to look into this after C++20, to avoid the risk of writing clunky C++17 code.)

    ajtowns commented at 1:48 pm on September 5, 2023:

    I think you could do:

     0#ifdef HAVE_CXX20
     1template<typename Stream>
     2concept ParamsStreamConcept = requires(Stream& s) { s.GetParams(); };
     3template<typename Stream>
     4concept PlainStreamConcept = !ParamsStreamConcept<Stream>;
     5#else
     6template<typename Stream>
     7bool ParamsStreamConcept = true;
     8template<typename Stream>
     9bool PlainStreamConcept = true;
    10#endif
    11
    12 /** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
    13 template <typename Params, typename SubStream>
    14 class ParamsStream
    15 {
    16    static_assert(PlainStreamConcept<SubStream>, "avoid wrapping a ParamsStream in a ParamsStream");
    17    SubStream& GetSubStream() { return m_substream; }
    

    or similar if you wanted, which would catch this sort of case via the C++20 CI builds. At present the only lines that violate this rule are READWRITE(WithParams(ser_params, AsBase<CService>(obj))); in protocol.h and Derived::SERIALIZE_METHOD_PARAMS in serialize_tests.cpp; you need a GetSubStream getter to fix those up.


    maflcko commented at 3:05 pm on September 5, 2023:

    Just for reference, GetSubStream was present in the original code, but only to avoid too-deeply nested templates, see https://github.com/bitcoin/bitcoin/pull/19503/files#r455147534

    Since it wasn’t enforced, I removed it. I’ll add your patch once and if there is C++20.

  97. in src/addrdb.cpp:177 in fad3f7a524 outdated
    173@@ -175,13 +174,15 @@ bool CBanDB::Read(banmap_t& banSet)
    174 bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr)
    175 {
    176     const auto pathAddr = args.GetDataDirNet() / "peers.dat";
    177-    return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
    178+    return SerializeFileDB("peers", pathAddr, WithParams(CAddress::V1_DISK, addr));
    


    sipa commented at 3:36 pm on September 1, 2023:
    This feels unnecessary. Addrman’s serialization controls how the addresses in it are serialized (only V1_DISK or V2_DISK should be used), so it shouldn’t be necessary to provide it with a CAddress parameter (and doing so is misleading, as it’d be overridden anyway).

    ajtowns commented at 11:59 am on September 4, 2023:
    +1 Patch with these changes is at https://github.com/ajtowns/bitcoin/commits/202309-serializeaddrmansimp if you want to squash it in

    maflcko commented at 7:54 am on September 5, 2023:

    I agree this is misleading, but the unit and fuzz tests would still set Network, as opposed to Disk sometimes. I agree with the cleanup, but I think this is a slight change (improvement) in what the unit and fuzz tests are testing.

    Though, as it only affects tests, I think the refactoring label can be kept on this pull request.


    maflcko commented at 8:20 am on September 5, 2023:

    +1 Patch with these changes is at https://github.com/ajtowns/bitcoin/commits/202309-serializeaddrmansimp if you want to squash it in

    Thanks for the patch! I’ve squashed it and adjusted the commit message to explain the test changes.

  98. in src/addrdb.cpp:195 in fad3f7a524 outdated
    191@@ -191,7 +192,7 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
    192     const auto start{SteadyClock::now()};
    193     const auto path_addr{args.GetDataDirNet() / "peers.dat"};
    194     try {
    195-        DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
    196+        DeserializeFileDB(path_addr, WithParams(CAddress::V1_DISK, *addrman));
    


    sipa commented at 3:39 pm on September 1, 2023:
    Same here, I think it should be possible to unserialize addrman without parameter.
  99. in src/addrman.cpp:174 in fad3f7a524 outdated
    170@@ -171,8 +171,8 @@ void AddrManImpl::Serialize(Stream& s_) const
    171      */
    172 
    173     // Always serialize in the latest version (FILE_FORMAT).
    174-
    175-    OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
    176+    const CAddress::SerParams ser_params{{CNetAddr::Encoding::V2}, s_.GetParams().fmt};
    


    sipa commented at 3:41 pm on September 1, 2023:
    Addrman is only ever serialized to disk, so the fmt here should always be disk (and thus, ser_params will only ever be V2_DISK here).

    maflcko commented at 8:01 am on September 5, 2023:

    Not in the unit tests, see #25284 (review) and:

    0test/addrman_tests.cpp(690): Entering test case "addrman_serialization"
    1test_bitcoin: addrman.cpp:175: void AddrManImpl::Serialize(Stream &) const [Stream = ParamsStream<CAddress::SerParams, DataStream>]: Assertion `s_.GetParams().fmt==CAddress::Format::Disk' failed.
    2make[3]: *** [Makefile:22464: test/addrman_tests.cpp.test] Error 1
    
  100. in src/addrman.cpp:239 in fad3f7a524 outdated
    246+        stream_enc = CNetAddr::Encoding::V2;
    247     }
    248 
    249-    OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
    250+    const CAddress::SerParams ser_params{{stream_enc}, s_.GetParams().fmt};
    251+    ParamsStream s{ser_params, s_};
    


    sipa commented at 3:42 pm on September 1, 2023:
    This line and the lines above could be simplified, as it’s only choosing between options V1_DISK and V2_DISK.
  101. Use serialization parameters for CAddress serialization
    This also cleans up the addrman (de)serialization code paths to only
    allow `Disk` serialization. Some unit tests previously forced a
    `Network` serialization, which does not make sense, because Bitcoin Core
    in production will always `Disk` serialize.
    This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
    Towns.
    
    Co-authored-by: Pieter Wuille <pieter@wuille.net>
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    fac81affb5
  102. test: add tests that exercise WithParams()
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    fafa3fc5a6
  103. Remove unused legacy CHashVerifier fa626af3ed
  104. maflcko force-pushed on Sep 5, 2023
  105. maflcko commented at 8:21 am on September 5, 2023: member
    Force pushed the simplification: #25284 (review)
  106. maflcko commented at 10:01 am on September 5, 2023: member

    Current list of linter/style questions to explore:

  107. TheCharlatan approved
  108. TheCharlatan commented at 2:53 pm on September 5, 2023: contributor
    ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
  109. DrahtBot requested review from theuni on Sep 5, 2023
  110. in src/serialize.h:960 in fac42e9d35 outdated
    959+ * Support for all macros providing or using the ser_action parameter of the SerializationOps method.
    960  */
    961-struct CSerActionSerialize
    962-{
    963+struct ActionSerialize {
    964     constexpr bool ForRead() const { return false; }
    


    ajtowns commented at 5:29 pm on September 5, 2023:

    These could be static constexpr bool ForRead() { return ...; }

    Actually, ::SerRead etc could be static member functions of these classes rather than being the global namespace too:

     0//#define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__))
     1#define READWRITE(...) (ser_action.SerReadWriteMany(s, __VA_ARGS__))
     2
     3struct ActionSerialize
     4{
     5    static constexpr bool ForRead() { return false; }
     6
     7    template<typename Stream, typename... Args>
     8    static void SerReadWriteMany(Stream& s, const Args&... args)
     9    {
    10        ::SerializeMany(s, args...);
    11    }
    

    Maybe something to consider later.


    maflcko commented at 7:40 am on September 6, 2023:
    Jup, will take look later, as this comment is on a line untouched in this pull and thus seems unrelated.
  111. in src/serialize.h:1162 in faec591d64 outdated
    1157+
    1158+/** Wrapper that serializes objects with the specified parameters. */
    1159+template <typename Params, typename T>
    1160+class ParamsWrapper
    1161+{
    1162+    static_assert(std::is_lvalue_reference<T>::value, "ParamsWrapper needs an lvalue reference type T");
    


    ajtowns commented at 6:53 pm on September 5, 2023:
    Why not write T& m_object and ParamsWrapper(.., T& obj), ParamsWrapper<.., FooObj> (instead of FooObj&) and drop this assert?

    maflcko commented at 7:41 am on September 6, 2023:
    Jup, will take a look in one of the follow-ups.
  112. ajtowns commented at 7:30 pm on September 5, 2023: contributor
    ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
  113. fanquake merged this on Sep 7, 2023
  114. fanquake closed this on Sep 7, 2023

  115. fanquake commented at 10:38 am on September 7, 2023: member

    Followups / tidy / lint to keep track of:

    #25284 (comment) #25284 (review) #25284 (review)

  116. ajtowns commented at 8:14 pm on September 7, 2023: contributor

    Since this code is mostly of the form s << WithParams(V1_DISK, data) we could shorten the common case a bit more:

    0struct SerParams {
    1   ...
    2   template <typename T>
    3   auto operator()(T& obj) const { return WithParams(*this, obj); }
    4};
    5
    6s << V1_DISK(data)
    

    That way when reading the code you’re really only seeing the info you care about.

  117. Frank-GER referenced this in commit d3985c5346 on Sep 8, 2023
  118. fanquake referenced this in commit 579c49b3a6 on Sep 9, 2023
  119. maflcko deleted the branch on Sep 11, 2023
  120. in src/net_processing.cpp:5283 in fac81affb5 outdated
    5282         msg_type = NetMsgType::ADDR;
    5283-        make_flags = 0;
    5284+        ser_enc = CNetAddr::Encoding::V1;
    5285     }
    5286-    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, peer.m_addrs_to_send));
    5287+    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
    


    maflcko commented at 12:55 pm on September 13, 2023:

    I guess the Make can be written shorter as:

    0+const bool peer_v2{peer.m_wants_addrv2};
    1-...Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send))
    2+...Make(peer_v2 ? NetMsgType::ADDRV2 : NetMsgType::ADDR, peer_v2 ? CAddress::V2_NETWORK : CAddress::V1_NETWORK, peer.m_addrs_to_send))
    

    and dropping the above if.

  121. in src/test/fuzz/deserialize.cpp:257 in fac81affb5 outdated
    256+    const auto maybe_na{ConsumeDeserializable<CNetAddr>(fdp, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp))};
    257+    if (!maybe_na) return;
    258+    const CNetAddr& na{*maybe_na};
    259     if (na.IsAddrV1Compatible()) {
    260-        AssertEqualAfterSerializeDeserialize(na);
    261+        AssertEqualAfterSerializeDeserialize(na, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp));
    


    maflcko commented at 1:25 pm on September 13, 2023:
    nit: This should be V1, because V2 is checked unconditionally below.
  122. in src/test/fuzz/deserialize.cpp:269 in fac81affb5 outdated
    274+    const auto maybe_s{ConsumeDeserializable<CService>(fdp, ser_params)};
    275+    if (!maybe_s) return;
    276+    const CService& s{*maybe_s};
    277     if (s.IsAddrV1Compatible()) {
    278-        AssertEqualAfterSerializeDeserialize(s);
    279+        AssertEqualAfterSerializeDeserialize(s, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp));
    


    maflcko commented at 1:26 pm on September 13, 2023:
    same?
  123. in src/test/fuzz/deserialize.cpp:284 in fac81affb5 outdated
    326-        AssertEqualAfterSerializeDeserialize(a, PROTOCOL_VERSION);
    327-        AssertEqualAfterSerializeDeserialize(a, 0, SER_DISK);
    328+FUZZ_TARGET(address_deserialize, .init = initialize_deserialize)
    329+{
    330+    FuzzedDataProvider fdp{buffer.data(), buffer.size()};
    331+    const auto ser_enc{ConsumeDeserializationParams<CNetAddr::SerParams>(fdp)};
    


    maflcko commented at 1:30 pm on September 13, 2023:
    nit: Could pick from CAddress::SerParams to cover a wider range?
  124. maflcko commented at 1:36 pm on September 13, 2023: member

    post-merge self-re-review, left some fuzz nits

    edit: fixed in https://github.com/bitcoin/bitcoin/pull/28470

  125. fanquake referenced this in commit 5c7cdda992 on Sep 15, 2023
  126. Frank-GER referenced this in commit e6ca965cb6 on Sep 15, 2023
  127. Frank-GER referenced this in commit d042ef9f09 on Sep 19, 2023
  128. fanquake referenced this in commit 108462139b on Nov 15, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me