Merge #6629: feat: introduce type-flexible NetInfoEntry to allow non-CService entries, use in MnNetInfo

3a72f2f evo: fast-fail `MnNetInfo::AddEntry()` if invalid characters found (Kittywhiskers Van Gogh)
e0a1c64 evo: prohibit overwriting entry in `MnNetInfo` (Kittywhiskers Van Gogh)
c69184b evo: utilize `NetInfoEntry::IsTriviallyValid()` in ProTx trivial checks (Kittywhiskers Van Gogh)
06cf4ee evo: return `MnNetInfo::GetEntries()` with `NetInfoEntry` (Kittywhiskers Van Gogh)
6286c9b evo: change internal type of `MnNetInfo` to `NetInfoEntry` (Kittywhiskers Van Gogh)
b0a634e evo: ensure the ADDRV2 serialization is always used in `NetInfoEntry` (Kittywhiskers Van Gogh)
6d97bda evo: introduce type-flexible `NetInfoEntry` to allow non-`CService` data (Kittywhiskers Van Gogh)
069583d evo: expand error codes for `netInfo` validation (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  The upcoming extended addresses specification envisions the ability to store address information _beyond_ the set of addresses that can be represented by BIP-155 (i.e. `CService`). To enable this, we need to devise a backwards-compatible way to allow storing and manipulating address information with differing serialization formats and validation rules.

  Backwards compatibility is a priority as the unique properties set (used to detect attempts at storing already-registered values) only stores a hashed representation of the value and therefore, in-place migration is not a viable option.

  With this in mind, this pull request introduces `NetInfoEntry`, which is wrapper around an `std::variant` that provides dispatching for common endpoints with `std::visit`, serialization and trivial validity enforcement between potential implementations and the ability to access the underlying type if necessary (for code that relies on backwards-compatibility, like hashing). It doesn't implement any network rules itself but requires that it must hold a valid instance of any underlying type that it supports.

  While `MnNetInfo` (the current implementation) has and always will store a `CService`, to ensure a conformity between it and `ExtNetInfo` (the upcoming extended implementation), its public functions will return a `NetInfoEntry` when applicable so that both can be abstracted by a common interface to aid with the transition.

  ## Additional Information

  * Depends on #6627

  * Depends on #6664

  * Dependency for #6665

  * ~~2e9bde0519b242d1d7aaf49344a357b90121689e in [dash#6627](#6627) incorrectly migrated validation conditions such that attempting to set a valid `addr:port` combination on mainnet would result in a `BadPort` error because the non-mainnet rules were applied _regardless_ of network.~~

    ~~This evaded testing as our unit and functional tests do not run on mainnet. To prevent this from occurring again, the whole `evo_netinfo_tests` suite now uses `BasicTestingSetup` (which advertises itself as mainnet), which comes with the added benefit of greater coverage as mainnet rules are _stricter_.~~

    ~~The port validation check for non-mainnet networks are tested _indirectly_ through tests like `evo_simplifiedmns_merkleroots`'s usage of `NetInfoInterface::*` methods ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/test/evo_simplifiedmns_tests.cpp#L25)).~~ Superseded by [dash#6664](#6664).

  * Per replies to review comments ([comment](#6627 (comment)), [comment](#6627 (comment))) from [dash#6627](#6627), reported error codes from `netInfo` interactions have been expanded to be more specific.

  * `CService` by default is serialized using ADDRV1 and utilizing ADDRV2 serialization is either explicitly opted into ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/addrman.cpp#L173-L175)) or determined on-the-fly ([source](https://github.com/dashpay/dash/blob/0f94e3e3e793925caa24a73ad54d843770b1a8c5/src/net_processing.cpp#L3960-L3965)). As we envision the ability to store Tor and I2P addresses, using ADDRV2 is mandatory.

    Though this affects (de)serialization of `NetInfoEntry`, it does not affect `MnNetInfo` as `NetInfoEntry` is only used for the sake of a consistent interface _but_ internally still (de)serializes to an ADDRV1 `CService`.

  * The introduction of fast-failing based on permitted characters is meant to mirror the upcoming extended addresses implementation where permitted characters are used as a quick way to classify the intended underlying type before running more expensive checks.

    As a side effect, this may cause inconsistency where attempting to use `MnNetInfo::AddEntry()` with, for example, an IPv6 address will result in `BadInput` as the delimiter used in IPv6 addresses are not part of the permitted characters filter _but_ validating a `MnNetInfo` with an IPv6 address _already stored_ will return a `BadType`.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 3a72f2f
  UdjinM6:
    utACK 3a72f2f

Tree-SHA512: abd84db309b6011480431b12cccd649878bab06aa44ca2c81563e9598d4424fd61888b12e2e439b9c2180bc5e0edee3431b1008ae7af4b676b164af1455fda3c

Author: PastaPastaPasta

src/evo/deterministicmns.cpp

@@ -457,11 +457,18 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
         throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate collateralOutpoint=%s", __func__,
                 dmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
     }
-    for (const CService& entry : dmn->pdmnState->netInfo.GetEntries()) {
-        if (!AddUniqueProperty(*dmn, entry)) {
+    for (const NetInfoEntry& entry : dmn->pdmnState->netInfo.GetEntries()) {
+        if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
+            const CService& service{service_opt.value()};
+            if (!AddUniqueProperty(*dmn, service)) {
+                mnUniquePropertyMap = mnUniquePropertyMapSaved;
+                throw std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s",
+                                                   __func__, dmn->proTxHash.ToString(), service.ToStringAddrPort()));
+            }
+        } else {
             mnUniquePropertyMap = mnUniquePropertyMapSaved;
-            throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s", __func__,
-                                               dmn->proTxHash.ToString(), entry.ToStringAddrPort())));
+            throw std::runtime_error(
+                strprintf("%s: Can't add a masternode %s with invalid address", __func__, dmn->proTxHash.ToString()));
         }
     }
     if (!AddUniqueProperty(*dmn, dmn->pdmnState->keyIDOwner)) {
@@ -500,28 +507,40 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
     // Using this temporary map as a checkpoint to roll back to in case of any issues.
     decltype(mnUniquePropertyMap) mnUniquePropertyMapSaved = mnUniquePropertyMap;
 
-    const auto updateNetInfo = [&]() {
-        if (oldState->netInfo != pdmnState->netInfo) {
+    auto updateNetInfo = [this](const CDeterministicMN& dmn, const MnNetInfo& oldInfo,
+                                const MnNetInfo& newInfo) -> std::string {
+        if (oldInfo != newInfo) {
             // We track each individual entry in netInfo as opposed to netInfo itself (preventing us from
             // using UpdateUniqueProperty()), so we need to successfully purge all old entries and insert
             // new entries to successfully update.
-            for (const CService& old_entry : oldState->netInfo.GetEntries()) {
-                if (!DeleteUniqueProperty(*dmn, old_entry)) {
-                    return strprintf("internal error"); // This shouldn't be possible
+            for (const NetInfoEntry& old_entry : oldInfo.GetEntries()) {
+                if (const auto& service_opt{old_entry.GetAddrPort()}) {
+                    const CService& service{service_opt.value()};
+                    if (!DeleteUniqueProperty(dmn, service)) {
+                        return "internal error"; // This shouldn't be possible
+                    }
+                } else {
+                    return "invalid address";
                 }
             }
-            for (const CService& new_entry : pdmnState->netInfo.GetEntries()) {
-                if (!AddUniqueProperty(*dmn, new_entry)) {
-                    return strprintf("duplicate (%s)", new_entry.ToStringAddrPort());
+            for (const NetInfoEntry& new_entry : newInfo.GetEntries()) {
+                if (const auto& service_opt{new_entry.GetAddrPort()}) {
+                    const CService& service{service_opt.value()};
+                    if (!AddUniqueProperty(dmn, service)) {
+                        return strprintf("duplicate (%s)", service.ToStringAddrPort());
+                    }
+                } else {
+                    return "invalid address";
                 }
             }
         }
-        return strprintf("");
-    }();
-    if (!updateNetInfo.empty()) {
+        return "";
+    };
+
+    if (auto err = updateNetInfo(*dmn, oldState->netInfo, pdmnState->netInfo); !err.empty()) {
         mnUniquePropertyMap = mnUniquePropertyMapSaved;
         throw(std::runtime_error(strprintf("%s: Can't update masternode %s with addresses, reason=%s", __func__,
-                                           oldDmn.proTxHash.ToString(), updateNetInfo)));
+                                           oldDmn.proTxHash.ToString(), err)));
     }
     if (!UpdateUniqueProperty(*dmn, oldState->keyIDOwner, pdmnState->keyIDOwner)) {
         mnUniquePropertyMap = mnUniquePropertyMapSaved;
@@ -578,11 +597,18 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash)
         throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a collateralOutpoint=%s", __func__,
                 proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
     }
-    for (const CService& entry : dmn->pdmnState->netInfo.GetEntries()) {
-        if (!DeleteUniqueProperty(*dmn, entry)) {
+    for (const NetInfoEntry& entry : dmn->pdmnState->netInfo.GetEntries()) {
+        if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
+            const CService& service{service_opt.value()};
+            if (!DeleteUniqueProperty(*dmn, service)) {
+                mnUniquePropertyMap = mnUniquePropertyMapSaved;
+                throw std::runtime_error(strprintf("%s: Can't delete a masternode %s with an address=%s", __func__,
+                                                   proTxHash.ToString(), service.ToStringAddrPort()));
+            }
+        } else {
             mnUniquePropertyMap = mnUniquePropertyMapSaved;
-            throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with an address=%s", __func__,
-                                               proTxHash.ToString(), entry.ToStringAddrPort())));
+            throw std::runtime_error(strprintf("%s: Can't delete a masternode %s with invalid address", __func__,
+                                               dmn->proTxHash.ToString()));
         }
     }
     if (!DeleteUniqueProperty(*dmn, dmn->pdmnState->keyIDOwner)) {
@@ -799,9 +825,14 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
                 }
             }
 
-            for (const CService& entry : proTx.netInfo.GetEntries()) {
-                if (newList.HasUniqueProperty(entry)) {
-                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
+            for (const NetInfoEntry& entry : proTx.netInfo.GetEntries()) {
+                if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
+                    const CService& service{service_opt.value()};
+                    if (newList.HasUniqueProperty(service)) {
+                        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
+                    }
+                } else {
+                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-netinfo-entry");
                 }
             }
             if (newList.HasUniqueProperty(proTx.keyIDOwner) || newList.HasUniqueProperty(proTx.pubKeyOperator)) {
@@ -830,10 +861,15 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload");
             }
 
-            for (const CService& entry : opt_proTx->netInfo.GetEntries()) {
-                if (newList.HasUniqueProperty(entry) &&
-                    newList.GetUniquePropertyMN(entry)->proTxHash != opt_proTx->proTxHash) {
-                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
+            for (const NetInfoEntry& entry : opt_proTx->netInfo.GetEntries()) {
+                if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
+                    const CService& service{service_opt.value()};
+                    if (newList.HasUniqueProperty(service) &&
+                        newList.GetUniquePropertyMN(service)->proTxHash != opt_proTx->proTxHash) {
+                        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
+                    }
+                } else {
+                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-netinfo-entry");
                 }
             }
 
@@ -1188,12 +1224,22 @@ template <typename ProTx>
 static bool CheckService(const ProTx& proTx, TxValidationState& state)
 {
     switch (proTx.netInfo.Validate()) {
-    case NetInfoStatus::BadInput:
-        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo");
+    case NetInfoStatus::BadAddress:
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr");
     case NetInfoStatus::BadPort:
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-port");
+    case NetInfoStatus::BadType:
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr-type");
+    case NetInfoStatus::NotRoutable:
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr-unroutable");
+    case NetInfoStatus::Malformed:
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad");
     case NetInfoStatus::Success:
         return true;
+    // Shouldn't be possible during self-checks
+    case NetInfoStatus::BadInput:
+    case NetInfoStatus::MaxLimit:
+        assert(false);
     } // no default case, so the compiler can warn about missing cases
     assert(false);
 }
@@ -1356,10 +1402,15 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl:
         auto mnList = dmnman.GetListForBlock(pindexPrev);
 
         // only allow reusing of addresses when it's for the same collateral (which replaces the old MN)
-        for (const CService& entry : opt_ptx->netInfo.GetEntries()) {
-            if (mnList.HasUniqueProperty(entry) &&
-                mnList.GetUniquePropertyMN(entry)->collateralOutpoint != collateralOutpoint) {
-                return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
+        for (const NetInfoEntry& entry : opt_ptx->netInfo.GetEntries()) {
+            if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
+                const CService& service{service_opt.value()};
+                if (mnList.HasUniqueProperty(service) &&
+                    mnList.GetUniquePropertyMN(service)->collateralOutpoint != collateralOutpoint) {
+                    return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
+                }
+            } else {
+                return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-entry");
             }
         }
 
@@ -1429,9 +1480,14 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
     }
 
     // don't allow updating to addresses already used by other MNs
-    for (const CService& entry : opt_ptx->netInfo.GetEntries()) {
-        if (mnList.HasUniqueProperty(entry) && mnList.GetUniquePropertyMN(entry)->proTxHash != opt_ptx->proTxHash) {
-            return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
+    for (const NetInfoEntry& entry : opt_ptx->netInfo.GetEntries()) {
+        if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
+            const CService& service{service_opt.value()};
+            if (mnList.HasUniqueProperty(service) && mnList.GetUniquePropertyMN(service)->proTxHash != opt_ptx->proTxHash) {
+                return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
+            }
+        } else {
+            return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-entry");
         }
     }
 

src/evo/deterministicmns.h

@@ -402,6 +402,7 @@ class CDeterministicMNList
     static_assert(!std::is_same_v<std::decay_t<T>, name>, "GetUniquePropertyHash cannot be templated against " #name)
         DMNL_NO_TEMPLATE(CBLSPublicKey);
         DMNL_NO_TEMPLATE(MnNetInfo);
+        DMNL_NO_TEMPLATE(NetInfoEntry);
 #undef DMNL_NO_TEMPLATE
         return ::SerializeHash(v);
     }