Skip to content

Commit 7325e06

Browse files
PastaPastaPastaPastaPastaPasta
committed
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
2 parents c40c4b4 + 3a72f2f commit 7325e06

File tree

9 files changed

+563
-73
lines changed

9 files changed

+563
-73
lines changed

‎src/evo/deterministicmns.cpp‎

Lines changed: 92 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -457,11 +457,18 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
457457
throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate collateralOutpoint=%s", __func__,
458458
dmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
459459
}
460-
for (const CService& entry : dmn->pdmnState->netInfo.GetEntries()) {
461-
if (!AddUniqueProperty(*dmn, entry)) {
460+
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo.GetEntries()) {
461+
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
462+
const CService& service{service_opt.value()};
463+
if (!AddUniqueProperty(*dmn, service)) {
464+
mnUniquePropertyMap = mnUniquePropertyMapSaved;
465+
throw std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s",
466+
__func__, dmn->proTxHash.ToString(), service.ToStringAddrPort()));
467+
}
468+
} else {
462469
mnUniquePropertyMap = mnUniquePropertyMapSaved;
463-
throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s", __func__,
464-
dmn->proTxHash.ToString(), entry.ToStringAddrPort())));
470+
throw std::runtime_error(
471+
strprintf("%s: Can't add a masternode %s with invalid address", __func__, dmn->proTxHash.ToString()));
465472
}
466473
}
467474
if (!AddUniqueProperty(*dmn, dmn->pdmnState->keyIDOwner)) {
@@ -500,28 +507,40 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
500507
// Using this temporary map as a checkpoint to roll back to in case of any issues.
501508
decltype(mnUniquePropertyMap) mnUniquePropertyMapSaved = mnUniquePropertyMap;
502509

503-
const auto updateNetInfo = [&]() {
504-
if (oldState->netInfo != pdmnState->netInfo) {
510+
auto updateNetInfo = [this](const CDeterministicMN& dmn, const MnNetInfo& oldInfo,
511+
const MnNetInfo& newInfo) -> std::string {
512+
if (oldInfo != newInfo) {
505513
// We track each individual entry in netInfo as opposed to netInfo itself (preventing us from
506514
// using UpdateUniqueProperty()), so we need to successfully purge all old entries and insert
507515
// new entries to successfully update.
508-
for (const CService& old_entry : oldState->netInfo.GetEntries()) {
509-
if (!DeleteUniqueProperty(*dmn, old_entry)) {
510-
return strprintf("internal error"); // This shouldn't be possible
516+
for (const NetInfoEntry& old_entry : oldInfo.GetEntries()) {
517+
if (const auto& service_opt{old_entry.GetAddrPort()}) {
518+
const CService& service{service_opt.value()};
519+
if (!DeleteUniqueProperty(dmn, service)) {
520+
return "internal error"; // This shouldn't be possible
521+
}
522+
} else {
523+
return "invalid address";
511524
}
512525
}
513-
for (const CService& new_entry : pdmnState->netInfo.GetEntries()) {
514-
if (!AddUniqueProperty(*dmn, new_entry)) {
515-
return strprintf("duplicate (%s)", new_entry.ToStringAddrPort());
526+
for (const NetInfoEntry& new_entry : newInfo.GetEntries()) {
527+
if (const auto& service_opt{new_entry.GetAddrPort()}) {
528+
const CService& service{service_opt.value()};
529+
if (!AddUniqueProperty(dmn, service)) {
530+
return strprintf("duplicate (%s)", service.ToStringAddrPort());
531+
}
532+
} else {
533+
return "invalid address";
516534
}
517535
}
518536
}
519-
return strprintf("");
520-
}();
521-
if (!updateNetInfo.empty()) {
537+
return "";
538+
};
539+
540+
if (auto err = updateNetInfo(*dmn, oldState->netInfo, pdmnState->netInfo); !err.empty()) {
522541
mnUniquePropertyMap = mnUniquePropertyMapSaved;
523542
throw(std::runtime_error(strprintf("%s: Can't update masternode %s with addresses, reason=%s", __func__,
524-
oldDmn.proTxHash.ToString(), updateNetInfo)));
543+
oldDmn.proTxHash.ToString(), err)));
525544
}
526545
if (!UpdateUniqueProperty(*dmn, oldState->keyIDOwner, pdmnState->keyIDOwner)) {
527546
mnUniquePropertyMap = mnUniquePropertyMapSaved;
@@ -578,11 +597,18 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash)
578597
throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a collateralOutpoint=%s", __func__,
579598
proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
580599
}
581-
for (const CService& entry : dmn->pdmnState->netInfo.GetEntries()) {
582-
if (!DeleteUniqueProperty(*dmn, entry)) {
600+
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo.GetEntries()) {
601+
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
602+
const CService& service{service_opt.value()};
603+
if (!DeleteUniqueProperty(*dmn, service)) {
604+
mnUniquePropertyMap = mnUniquePropertyMapSaved;
605+
throw std::runtime_error(strprintf("%s: Can't delete a masternode %s with an address=%s", __func__,
606+
proTxHash.ToString(), service.ToStringAddrPort()));
607+
}
608+
} else {
583609
mnUniquePropertyMap = mnUniquePropertyMapSaved;
584-
throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with an address=%s", __func__,
585-
proTxHash.ToString(), entry.ToStringAddrPort())));
610+
throw std::runtime_error(strprintf("%s: Can't delete a masternode %s with invalid address", __func__,
611+
dmn->proTxHash.ToString()));
586612
}
587613
}
588614
if (!DeleteUniqueProperty(*dmn, dmn->pdmnState->keyIDOwner)) {
@@ -799,9 +825,14 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
799825
}
800826
}
801827

802-
for (const CService& entry : proTx.netInfo.GetEntries()) {
803-
if (newList.HasUniqueProperty(entry)) {
804-
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
828+
for (const NetInfoEntry& entry : proTx.netInfo.GetEntries()) {
829+
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
830+
const CService& service{service_opt.value()};
831+
if (newList.HasUniqueProperty(service)) {
832+
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
833+
}
834+
} else {
835+
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-netinfo-entry");
805836
}
806837
}
807838
if (newList.HasUniqueProperty(proTx.keyIDOwner) || newList.HasUniqueProperty(proTx.pubKeyOperator)) {
@@ -830,10 +861,15 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
830861
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload");
831862
}
832863

833-
for (const CService& entry : opt_proTx->netInfo.GetEntries()) {
834-
if (newList.HasUniqueProperty(entry) &&
835-
newList.GetUniquePropertyMN(entry)->proTxHash != opt_proTx->proTxHash) {
836-
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
864+
for (const NetInfoEntry& entry : opt_proTx->netInfo.GetEntries()) {
865+
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
866+
const CService& service{service_opt.value()};
867+
if (newList.HasUniqueProperty(service) &&
868+
newList.GetUniquePropertyMN(service)->proTxHash != opt_proTx->proTxHash) {
869+
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
870+
}
871+
} else {
872+
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-netinfo-entry");
837873
}
838874
}
839875

@@ -1188,12 +1224,22 @@ template <typename ProTx>
11881224
static bool CheckService(const ProTx& proTx, TxValidationState& state)
11891225
{
11901226
switch (proTx.netInfo.Validate()) {
1191-
case NetInfoStatus::BadInput:
1192-
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo");
1227+
case NetInfoStatus::BadAddress:
1228+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr");
11931229
case NetInfoStatus::BadPort:
11941230
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-port");
1231+
case NetInfoStatus::BadType:
1232+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr-type");
1233+
case NetInfoStatus::NotRoutable:
1234+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr-unroutable");
1235+
case NetInfoStatus::Malformed:
1236+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad");
11951237
case NetInfoStatus::Success:
11961238
return true;
1239+
// Shouldn't be possible during self-checks
1240+
case NetInfoStatus::BadInput:
1241+
case NetInfoStatus::MaxLimit:
1242+
assert(false);
11971243
} // no default case, so the compiler can warn about missing cases
11981244
assert(false);
11991245
}
@@ -1356,10 +1402,15 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl:
13561402
auto mnList = dmnman.GetListForBlock(pindexPrev);
13571403

13581404
// only allow reusing of addresses when it's for the same collateral (which replaces the old MN)
1359-
for (const CService& entry : opt_ptx->netInfo.GetEntries()) {
1360-
if (mnList.HasUniqueProperty(entry) &&
1361-
mnList.GetUniquePropertyMN(entry)->collateralOutpoint != collateralOutpoint) {
1362-
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
1405+
for (const NetInfoEntry& entry : opt_ptx->netInfo.GetEntries()) {
1406+
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
1407+
const CService& service{service_opt.value()};
1408+
if (mnList.HasUniqueProperty(service) &&
1409+
mnList.GetUniquePropertyMN(service)->collateralOutpoint != collateralOutpoint) {
1410+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
1411+
}
1412+
} else {
1413+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-entry");
13631414
}
13641415
}
13651416

@@ -1429,9 +1480,14 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
14291480
}
14301481

14311482
// don't allow updating to addresses already used by other MNs
1432-
for (const CService& entry : opt_ptx->netInfo.GetEntries()) {
1433-
if (mnList.HasUniqueProperty(entry) && mnList.GetUniquePropertyMN(entry)->proTxHash != opt_ptx->proTxHash) {
1434-
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
1483+
for (const NetInfoEntry& entry : opt_ptx->netInfo.GetEntries()) {
1484+
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
1485+
const CService& service{service_opt.value()};
1486+
if (mnList.HasUniqueProperty(service) && mnList.GetUniquePropertyMN(service)->proTxHash != opt_ptx->proTxHash) {
1487+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
1488+
}
1489+
} else {
1490+
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-entry");
14351491
}
14361492
}
14371493

‎src/evo/deterministicmns.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,7 @@ class CDeterministicMNList
402402
static_assert(!std::is_same_v<std::decay_t<T>, name>, "GetUniquePropertyHash cannot be templated against " #name)
403403
DMNL_NO_TEMPLATE(CBLSPublicKey);
404404
DMNL_NO_TEMPLATE(MnNetInfo);
405+
DMNL_NO_TEMPLATE(NetInfoEntry);
405406
#undef DMNL_NO_TEMPLATE
406407
return ::SerializeHash(v);
407408
}

0 commit comments

Comments
 (0)