From 9adf8ad738fad103dca1c51c5fe8bbff9cc267ea Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 14 Nov 2018 14:10:33 +0100 Subject: [PATCH 1/6] Remove restriction that forced use of same addresses for payout and collateral --- src/evo/providertx.cpp | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index 0db1149e4..f57022637 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -101,12 +101,6 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid if (tx.vout[ptx.collateralOutpoint.n].nValue != 1000 * COIN) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral"); } - - // This is a temporary restriction that will be lifted later - // It is required while we are transitioning from the old MN list to the deterministic list - if (tx.vout[ptx.collateralOutpoint.n].scriptPubKey != ptx.scriptPayout) { - return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-collateral"); - } } if (ptx.keyIDOwner.IsNull() || !ptx.pubKeyOperator.IsValid() || ptx.keyIDVoting.IsNull()) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-key-null"); @@ -295,15 +289,10 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-reuse"); } - // This is a temporary restriction that will be lifted later - // It is required while we are transitioning from the old MN list to the deterministic list Coin coin; if (!GetUTXOCoin(dmn->collateralOutpoint, coin)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payee-collateral"); } - if (coin.out.scriptPubKey != ptx.scriptPayout) { - return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-collateral"); - } if (mnList.HasUniqueProperty(ptx.pubKeyOperator)) { auto otherDmn = mnList.GetUniquePropertyMN(ptx.pubKeyOperator); From dc404e7550c59e58e59156125c35bf2813409cd9 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 14 Nov 2018 14:31:56 +0100 Subject: [PATCH 2/6] Allow P2SH for payout scripts --- src/evo/providertx.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index f57022637..c781da630 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -105,8 +105,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid if (ptx.keyIDOwner.IsNull() || !ptx.pubKeyOperator.IsValid() || ptx.keyIDVoting.IsNull()) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-key-null"); } - // we may support P2SH later, but restrict it for now (while in transitioning phase from old MN list to deterministic list) - if (!ptx.scriptPayout.IsPayToPublicKeyHash()) { + if (!ptx.scriptPayout.IsPayToPublicKeyHash() && !ptx.scriptPayout.IsPayToScriptHash()) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee"); } @@ -266,8 +265,7 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal if (!ptx.pubKeyOperator.IsValid() || ptx.keyIDVoting.IsNull()) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-key-null"); } - // we may support P2SH later, but restrict it for now (while in transitioning phase from old MN list to deterministic list) - if (!ptx.scriptPayout.IsPayToPublicKeyHash()) { + if (!ptx.scriptPayout.IsPayToPublicKeyHash() && !ptx.scriptPayout.IsPayToScriptHash()) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee"); } From 826e7d063aa105dd71251de94049589e5ab7ee63 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 14 Nov 2018 14:49:32 +0100 Subject: [PATCH 3/6] Move internal collateral check to the else branch of the external collateral check --- src/evo/providertx.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index c781da630..8f0bc7e0d 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -93,15 +93,6 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid return state.DoS(100, false, REJECT_INVALID, "bad-protx-mode"); } - // when collateralOutpoint refers to an external collateral, we check it further down - if (ptx.collateralOutpoint.hash.IsNull()) { - if (ptx.collateralOutpoint.n >= tx.vout.size()) { - return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-index"); - } - if (tx.vout[ptx.collateralOutpoint.n].nValue != 1000 * COIN) { - return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral"); - } - } if (ptx.keyIDOwner.IsNull() || !ptx.pubKeyOperator.IsValid() || ptx.keyIDVoting.IsNull()) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-key-null"); } @@ -152,6 +143,12 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid collateralOutpoint = ptx.collateralOutpoint; } else { + if (ptx.collateralOutpoint.n >= tx.vout.size()) { + return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-index"); + } + if (tx.vout[ptx.collateralOutpoint.n].nValue != 1000 * COIN) { + return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral"); + } collateralOutpoint = COutPoint(tx.GetHash(), ptx.collateralOutpoint.n); } From 927e8bd796e65bbd79559b6876def9833d8c8b3e Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 14 Nov 2018 14:53:41 +0100 Subject: [PATCH 4/6] Also forbid reusing collateral key for owner/voting keys We previously only checked for the payee key. --- src/evo/providertx.cpp | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index 8f0bc7e0d..ab9022f11 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -105,7 +105,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid // should not happen as we checked script types before return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-dest"); } - // don't allow reuse of collateral key for other keys (don't allow people to put the collateral key onto an online server) + // don't allow reuse of payout key for other keys (don't allow people to put the payee key onto an online server) if (payoutDest == CTxDestination(ptx.keyIDOwner) || payoutDest == CTxDestination(ptx.keyIDVoting)) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-reuse"); } @@ -120,6 +120,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-reward"); } + CTxDestination collateralTxDest; CKeyID keyForPayloadSig; COutPoint collateralOutpoint; @@ -129,15 +130,13 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral"); } - CTxDestination txDest; - if (!ExtractDestination(coin.out.scriptPubKey, txDest)) { + if (!ExtractDestination(coin.out.scriptPubKey, collateralTxDest)) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-dest"); } // Extract key from collateral. This only works for P2PK and P2PKH collaterals and will fail for P2SH. // Issuer of this ProRegTx must prove ownership with this key by signing the ProRegTx - CBitcoinAddress txAddr(txDest); - if (!txAddr.GetKeyID(keyForPayloadSig)) { + if (!CBitcoinAddress(collateralTxDest).GetKeyID(keyForPayloadSig)) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-pkh"); } @@ -149,9 +148,20 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid if (tx.vout[ptx.collateralOutpoint.n].nValue != 1000 * COIN) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral"); } + + if (!ExtractDestination(tx.vout[ptx.collateralOutpoint.n].scriptPubKey, collateralTxDest)) { + return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-dest"); + } + collateralOutpoint = COutPoint(tx.GetHash(), ptx.collateralOutpoint.n); } + // don't allow reuse of collateral key for other keys (don't allow people to put the collateral key onto an online server) + // this check applies to internal and external collateral, but internal collaterals are not necessarely a P2PKH + if (collateralTxDest == CTxDestination(ptx.keyIDOwner) || collateralTxDest == CTxDestination(ptx.keyIDVoting)) { + return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-reuse"); + } + if (pindexPrev) { auto mnList = deterministicMNManager->GetListForBlock(pindexPrev->GetBlockHash()); @@ -279,14 +289,24 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal return state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); } - // don't allow reuse of collateral key for other keys (don't allow people to put the collateral key onto an online server) + // don't allow reuse of payee key for other keys (don't allow people to put the payee key onto an online server) if (payoutDest == CTxDestination(dmn->pdmnState->keyIDOwner) || payoutDest == CTxDestination(ptx.keyIDVoting)) { return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-reuse"); } Coin coin; if (!GetUTXOCoin(dmn->collateralOutpoint, coin)) { - return state.DoS(100, false, REJECT_INVALID, "bad-protx-payee-collateral"); + // this should never happen (there would be no dmn otherwise) + return state.DoS(100, false, REJECT_INVALID, "bad-protx-collateral"); + } + + // don't allow reuse of collateral key for other keys (don't allow people to put the collateral key onto an online server) + CTxDestination collateralTxDest; + if (!ExtractDestination(coin.out.scriptPubKey, collateralTxDest)) { + return state.DoS(100, false, REJECT_INVALID, "bad-protx-collateral-dest"); + } + if (collateralTxDest == CTxDestination(dmn->pdmnState->keyIDOwner) || collateralTxDest == CTxDestination(ptx.keyIDVoting)) { + return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-reuse"); } if (mnList.HasUniqueProperty(ptx.pubKeyOperator)) { From 5ccf556f3a987cb439888ecac56d4070f15922fe Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 14 Nov 2018 14:57:58 +0100 Subject: [PATCH 5/6] GetMasternodeInfo with payee argument should do nothing when DIP3 is active Also change ComputeBlockVersion to directly use mnList.GetMNPayee() --- src/masternodeman.cpp | 26 +++++++++++++++--------- src/validation.cpp | 46 +++++++++++++++++++++++++------------------ 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/masternodeman.cpp b/src/masternodeman.cpp index 90722cf77..03f9a2f3b 100644 --- a/src/masternodeman.cpp +++ b/src/masternodeman.cpp @@ -605,18 +605,26 @@ bool CMasternodeMan::GetMasternodeInfo(const CKeyID& keyIDOperator, masternode_i bool CMasternodeMan::GetMasternodeInfo(const CScript& payee, masternode_info_t& mnInfoRet) { - CTxDestination dest; - if (!ExtractDestination(payee, dest) || !boost::get(&dest)) + if (deterministicMNManager->IsDeterministicMNsSporkActive()) { + // we can't reliably search by payee as there might be duplicates. Also, keyIDCollateralAddress is not + // always the payout address as DIP3 allows using different keys for collateral and payouts + // this method is only used from ComputeBlockVersion, which has a different logic for deterministic MNs + // this method won't be reimplemented when removing the compatibility code return false; - CKeyID keyId = *boost::get(&dest); - LOCK(cs); - for (const auto& mnpair : mapMasternodes) { - if (mnpair.second.keyIDCollateralAddress == keyId) { - mnInfoRet = mnpair.second.GetInfo(); - return true; + } else { + CTxDestination dest; + if (!ExtractDestination(payee, dest) || !boost::get(&dest)) + return false; + CKeyID keyId = *boost::get(&dest); + LOCK(cs); + for (const auto& mnpair : mapMasternodes) { + if (mnpair.second.keyIDCollateralAddress == keyId) { + mnInfoRet = mnpair.second.GetInfo(); + return true; + } } + return false; } - return false; } bool CMasternodeMan::Has(const COutPoint& outpoint) diff --git a/src/validation.cpp b/src/validation.cpp index 5ec27ca16..8971cff84 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1807,26 +1807,34 @@ int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Para ThresholdState state = VersionBitsState(pindexPrev, params, pos, versionbitscache); const struct BIP9DeploymentInfo& vbinfo = VersionBitsDeploymentInfo[pos]; if (vbinfo.check_mn_protocol && state == THRESHOLD_STARTED && fCheckMasternodesUpgraded) { - std::vector voutMasternodePayments; - masternode_info_t mnInfo; - if (!mnpayments.GetBlockTxOuts(pindexPrev->nHeight + 1, 0, voutMasternodePayments)) { - // no votes for this block - continue; - } - bool mnKnown = false; - for (const auto& txout : voutMasternodePayments) { - if (mnodeman.GetMasternodeInfo(txout.scriptPubKey, mnInfo)) { - mnKnown = true; - break; + if (deterministicMNManager->IsDeterministicMNsSporkActive()) { + auto mnList = deterministicMNManager->GetListForBlock(pindexPrev->GetBlockHash()); + auto payee = mnList.GetMNPayee(); + if (!payee) { + continue; + } + } else { + std::vector voutMasternodePayments; + masternode_info_t mnInfo; + if (!mnpayments.GetBlockTxOuts(pindexPrev->nHeight + 1, 0, voutMasternodePayments)) { + // no votes for this block + continue; + } + bool mnKnown = false; + for (const auto& txout : voutMasternodePayments) { + if (mnodeman.GetMasternodeInfo(txout.scriptPubKey, mnInfo)) { + mnKnown = true; + break; + } + } + if (!mnKnown) { + // unknown masternode + continue; + } + if (mnInfo.nProtocolVersion < DMN_PROTO_VERSION) { + // masternode is not upgraded yet + continue; } - } - if (!mnKnown) { - // unknown masternode - continue; - } - if (mnInfo.nProtocolVersion < DMN_PROTO_VERSION) { - // masternode is not upgraded yet - continue; } } if (state == THRESHOLD_LOCKED_IN || state == THRESHOLD_STARTED) { From f5864254cfaec10d7475a6f91a6e3eae58b637fb Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 14 Nov 2018 15:39:40 +0100 Subject: [PATCH 6/6] Do not use keyIDCollateralAddress anymore when spork15 is active --- src/masternode.cpp | 6 +----- src/masternode.h | 2 +- src/rpc/masternode.cpp | 38 +++++++++++++++++++++++++++++++------- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/masternode.cpp b/src/masternode.cpp index 82a39b5d5..9cce8bf7b 100644 --- a/src/masternode.cpp +++ b/src/masternode.cpp @@ -56,13 +56,9 @@ CMasternode::CMasternode(const CMasternodeBroadcast& mnb) : CMasternode::CMasternode(const uint256 &proTxHash, const CDeterministicMNCPtr& dmn) : masternode_info_t{ MASTERNODE_ENABLED, DMN_PROTO_VERSION, GetAdjustedTime(), - dmn->collateralOutpoint, dmn->pdmnState->addr, CKeyID(), dmn->pdmnState->keyIDOwner, dmn->pdmnState->pubKeyOperator, dmn->pdmnState->keyIDVoting}, + dmn->collateralOutpoint, dmn->pdmnState->addr, CKeyID() /* not valid with DIP3 */, dmn->pdmnState->keyIDOwner, dmn->pdmnState->pubKeyOperator, dmn->pdmnState->keyIDVoting}, fAllowMixingTx(true) { - CTxDestination dest; - if (!ExtractDestination(dmn->pdmnState->scriptPayout, dest) || !boost::get(&dest)) - assert(false); // should not happen (previous verification forbids non p2pkh/p2pk - keyIDCollateralAddress = *boost::get(&dest); } // diff --git a/src/masternode.h b/src/masternode.h index 4e60c9dea..bbba83c9d 100644 --- a/src/masternode.h +++ b/src/masternode.h @@ -129,7 +129,7 @@ struct masternode_info_t CService addr{}; CPubKey pubKeyCollateralAddress{}; // this will be invalid/unset when the network switches to deterministic MNs (luckely it's only important for the broadcast hash) CPubKey pubKeyMasternode{}; // this will be invalid/unset when the network switches to deterministic MNs (luckely it's only important for the broadcast hash) - CKeyID keyIDCollateralAddress{}; + CKeyID keyIDCollateralAddress{}; // this is only used in compatibility code and won't be used when spork15 gets activated CKeyID keyIDOwner{}; CKeyID legacyKeyIDOperator{}; CBLSPublicKey blsPubKeyOperator; diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 158f7d222..ebdcd7419 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -283,13 +283,22 @@ UniValue GetNextMasternodeForPayment(int heightShift) nHeight = pindex->nHeight + heightShift; mnodeman.UpdateLastPaid(pindex); + CScript payeeScript; if (deterministicMNManager->IsDeterministicMNsSporkActive()) { auto payee = deterministicMNManager->GetListAtChainTip().GetMNPayee(); if (!payee || !mnodeman.GetMasternodeInfo(payee->proTxHash, mnInfo)) return "unknown"; + payeeScript = payee->pdmnState->scriptPayout; } else { if (!mnodeman.GetNextMasternodeInQueueForPayment(nHeight, true, nCount, mnInfo)) return "unknown"; + payeeScript = GetScriptForDestination(mnInfo.keyIDCollateralAddress); + } + + CTxDestination payeeDest; + CBitcoinAddress payeeAddr; + if (ExtractDestination(payeeScript, payeeDest)) { + payeeAddr = CBitcoinAddress(payeeDest); } UniValue obj(UniValue::VOBJ); @@ -298,7 +307,7 @@ UniValue GetNextMasternodeForPayment(int heightShift) obj.push_back(Pair("IP:port", mnInfo.addr.ToString())); obj.push_back(Pair("protocol", mnInfo.nProtocolVersion)); obj.push_back(Pair("outpoint", mnInfo.outpoint.ToStringShort())); - obj.push_back(Pair("payee", CBitcoinAddress(mnInfo.keyIDCollateralAddress).ToString())); + obj.push_back(Pair("payee", payeeAddr.IsValid() ? payeeAddr.ToString() : "UNKNOWN")); obj.push_back(Pair("lastseen", mnInfo.nTimeLastPing)); obj.push_back(Pair("activeseconds", mnInfo.nTimeLastPing - mnInfo.sigTime)); return obj; @@ -877,6 +886,23 @@ UniValue masternodelist(const JSONRPCRequest& request) for (const auto& mnpair : mapMasternodes) { CMasternode mn = mnpair.second; std::string strOutpoint = mnpair.first.ToStringShort(); + + CScript payeeScript; + if (deterministicMNManager->IsDeterministicMNsSporkActive()) { + auto dmn = deterministicMNManager->GetListAtChainTip().GetMNByCollateral(mn.outpoint); + if (dmn) { + payeeScript = dmn->pdmnState->scriptPayout; + } + } else { + payeeScript = GetScriptForDestination(mn.keyIDCollateralAddress); + } + + CTxDestination payeeDest; + std::string payeeStr = "UNKOWN"; + if (ExtractDestination(payeeScript, payeeDest)) { + payeeStr = CBitcoinAddress(payeeDest).ToString(); + } + if (strMode == "activeseconds") { if (strFilter !="" && strOutpoint.find(strFilter) == std::string::npos) continue; obj.push_back(Pair(strOutpoint, (int64_t)(mn.lastPing.sigTime - mn.sigTime))); @@ -900,7 +926,7 @@ UniValue masternodelist(const JSONRPCRequest& request) streamFull << std::setw(18) << mn.GetStatus() << " " << mn.nProtocolVersion << " " << - CBitcoinAddress(mn.keyIDCollateralAddress).ToString() << " " << + payeeStr << " " << (int64_t)mn.lastPing.sigTime << " " << std::setw(8) << (int64_t)(mn.lastPing.sigTime - mn.sigTime) << " " << std::setw(10) << mn.GetLastPaidTime() << " " << std::setw(6) << @@ -915,7 +941,7 @@ UniValue masternodelist(const JSONRPCRequest& request) streamInfo << std::setw(18) << mn.GetStatus() << " " << mn.nProtocolVersion << " " << - CBitcoinAddress(mn.keyIDCollateralAddress).ToString() << " " << + payeeStr << " " << (int64_t)mn.lastPing.sigTime << " " << std::setw(8) << (int64_t)(mn.lastPing.sigTime - mn.sigTime) << " " << mn.lastPing.GetSentinelString() << " " << @@ -964,11 +990,9 @@ UniValue masternodelist(const JSONRPCRequest& request) if (strFilter !="" && strOutpoint.find(strFilter) == std::string::npos) continue; obj.push_back(Pair(strOutpoint, (int64_t)mn.lastPing.sigTime)); } else if (strMode == "payee") { - CBitcoinAddress address(mn.keyIDCollateralAddress); - std::string strPayee = address.ToString(); - if (strFilter !="" && strPayee.find(strFilter) == std::string::npos && + if (strFilter !="" && payeeStr.find(strFilter) == std::string::npos && strOutpoint.find(strFilter) == std::string::npos) continue; - obj.push_back(Pair(strOutpoint, strPayee)); + obj.push_back(Pair(strOutpoint, payeeStr)); } else if (strMode == "protocol") { if (strFilter !="" && strFilter != strprintf("%d", mn.nProtocolVersion) && strOutpoint.find(strFilter) == std::string::npos) continue;