From 6302119cdff4a023b222998481f8fb59cb43af38 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 18 Aug 2020 23:22:26 -0500 Subject: [PATCH 1/5] Harden Spork6 Spork6 was previously activated on testnet, but we then developed an alternative fix for the issue and never activated spork6 on mainnet. At this point, Spork6 will not be activated on mainnet. As such, it makes sense to harden Spork6 and ensure that spork6 will never be activated on mainnet. So, we just change from checking Spork6 to checking if we are on testnet. If we are on testnet, use spork6 logic, else don't. Signed-off-by: pasta --- src/governance/governance-vote.cpp | 6 ++++-- src/spork.cpp | 11 ++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/governance/governance-vote.cpp b/src/governance/governance-vote.cpp index f07037d70218..2ceaa1acf2c5 100644 --- a/src/governance/governance-vote.cpp +++ b/src/governance/governance-vote.cpp @@ -164,7 +164,8 @@ bool CGovernanceVote::Sign(const CKey& key, const CKeyID& keyID) { std::string strError; - if (sporkManager.IsSporkActive(SPORK_6_NEW_SIGS)) { + // Harden Spork6 so that it is active on testnet and no other networks + if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { uint256 hash = GetSignatureHash(); if (!CHashSigner::SignHash(hash, key, vchSig)) { @@ -198,7 +199,8 @@ bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const { std::string strError; - if (sporkManager.IsSporkActive(SPORK_6_NEW_SIGS)) { + // Harden Spork6 so that it is active on testnet and no other networks + if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { uint256 hash = GetSignatureHash(); if (!CHashSigner::VerifyHash(hash, keyID, vchSig, strError)) { diff --git a/src/spork.cpp b/src/spork.cpp index 16c41d6d0429..e5c9d9140cdb 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -141,7 +141,9 @@ void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CD } CKeyID keyIDSigner; - bool fSpork6IsActive = IsSporkActive(SPORK_6_NEW_SIGS); + + // Harden Spork6 so that it is active on testnet and no other networks + bool fSpork6IsActive = Params().NetworkIDString() == CBaseChainParams::TESTNET; if (!spork.GetSignerKeyID(keyIDSigner, fSpork6IsActive) || !setSporkPubKeyIDs.count(keyIDSigner)) { // Note: unlike for other messages we have to check for new format even with SPORK_6_NEW_SIGS // inactive because SPORK_6_NEW_SIGS default is OFF and it is not the first spork to sync @@ -197,7 +199,8 @@ bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& conn LOCK(cs); - bool fSpork6IsActive = IsSporkActive(SPORK_6_NEW_SIGS); + // Harden Spork6 so that it is active on testnet and no other networks + bool fSpork6IsActive = Params().NetworkIDString() == CBaseChainParams::TESTNET; if (!spork.Sign(sporkPrivKey, fSpork6IsActive)) { LogPrintf("CSporkManager::%s -- ERROR: signing failed for spork %d\n", __func__, nSporkID); return false; @@ -314,7 +317,8 @@ bool CSporkManager::SetPrivKey(const std::string& strPrivKey) } CSporkMessage spork; - if (!spork.Sign(key, IsSporkActive(SPORK_6_NEW_SIGS))) { + // Harden Spork6 so that it is active on testnet and no other networks + if (!spork.Sign(key, Params().NetworkIDString() == CBaseChainParams::TESTNET)) { LogPrintf("CSporkManager::SetPrivKey -- Test signing failed\n"); return false; } @@ -389,6 +393,7 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId, bool fSporkSixActive) { std::string strError = ""; + // fSporkSixActive will only be true on testnet, it will be false on all other networks if (fSporkSixActive) { uint256 hash = GetSignatureHash(); From aee38d06762f649050606c5ad865cd9f892b63c7 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 18 Aug 2020 23:26:18 -0500 Subject: [PATCH 2/5] remove now unused SPORK_6_NEW_SIGS Signed-off-by: pasta --- src/spork.cpp | 1 - src/spork.h | 1 - 2 files changed, 2 deletions(-) diff --git a/src/spork.cpp b/src/spork.cpp index e5c9d9140cdb..55e7998cd7d9 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -19,7 +19,6 @@ const std::string CSporkManager::SERIALIZATION_VERSION_STRING = "CSporkManager-V std::vector sporkDefs = { MAKE_SPORK_DEF(SPORK_2_INSTANTSEND_ENABLED, 4070908800ULL), // OFF MAKE_SPORK_DEF(SPORK_3_INSTANTSEND_BLOCK_FILTERING, 4070908800ULL), // OFF - MAKE_SPORK_DEF(SPORK_6_NEW_SIGS, 4070908800ULL), // OFF MAKE_SPORK_DEF(SPORK_9_SUPERBLOCKS_ENABLED, 4070908800ULL), // OFF MAKE_SPORK_DEF(SPORK_17_QUORUM_DKG_ENABLED, 4070908800ULL), // OFF MAKE_SPORK_DEF(SPORK_19_CHAINLOCKS_ENABLED, 4070908800ULL), // OFF diff --git a/src/spork.h b/src/spork.h index 46f2eb5a4a1b..6c9d9644b5c7 100644 --- a/src/spork.h +++ b/src/spork.h @@ -23,7 +23,6 @@ class CSporkManager; enum SporkId : int32_t { SPORK_2_INSTANTSEND_ENABLED = 10001, SPORK_3_INSTANTSEND_BLOCK_FILTERING = 10002, - SPORK_6_NEW_SIGS = 10005, SPORK_9_SUPERBLOCKS_ENABLED = 10008, SPORK_17_QUORUM_DKG_ENABLED = 10016, SPORK_19_CHAINLOCKS_ENABLED = 10018, From 7b8f41ebb7ebc829f7253dac1005810f24dc7f08 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 21 Aug 2020 08:36:10 -0500 Subject: [PATCH 3/5] force fSporkSixActive to be correct, otherwise return Signed-off-by: pasta --- src/spork.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/spork.cpp b/src/spork.cpp index 55e7998cd7d9..2436f5631179 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -393,6 +393,7 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId, bool fSporkSixActive) std::string strError = ""; // fSporkSixActive will only be true on testnet, it will be false on all other networks + if (fSporkSixActive != (Params().NetworkIDString() == CBaseChainParams::TESTNET) return false; if (fSporkSixActive) { uint256 hash = GetSignatureHash(); From 43c69ae70a5baf80583520a88e71372ae598dd7a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 22 Aug 2020 05:28:18 +0300 Subject: [PATCH 4/5] Harden spork6 even more --- src/governance/governance-vote.cpp | 13 +----- src/spork.cpp | 68 +++++++++++------------------- src/spork.h | 6 +-- 3 files changed, 29 insertions(+), 58 deletions(-) diff --git a/src/governance/governance-vote.cpp b/src/governance/governance-vote.cpp index 2ceaa1acf2c5..ea17200539b0 100644 --- a/src/governance/governance-vote.cpp +++ b/src/governance/governance-vote.cpp @@ -204,17 +204,8 @@ bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const uint256 hash = GetSignatureHash(); if (!CHashSigner::VerifyHash(hash, keyID, vchSig, strError)) { - // could be a signature in old format - std::string strMessage = masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" + - std::to_string(nVoteSignal) + "|" + - std::to_string(nVoteOutcome) + "|" + - std::to_string(nTime); - - if (!CMessageSigner::VerifyMessage(keyID, vchSig, strMessage, strError)) { - // nope, not in old format either - LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyMessage() failed, error: %s\n", strError); - return false; - } + LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyHash() failed, error: %s\n", strError); + return false; } } else { std::string strMessage = masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" + diff --git a/src/spork.cpp b/src/spork.cpp index 2436f5631179..0cd515ebba2a 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -80,12 +80,10 @@ void CSporkManager::CheckAndRemove() mapSporksByHash.erase(itSignerPair->second.GetHash()); continue; } - if (!itSignerPair->second.CheckSignature(itSignerPair->first, false)) { - if (!itSignerPair->second.CheckSignature(itSignerPair->first, true)) { - mapSporksByHash.erase(itSignerPair->second.GetHash()); - itActive->second.erase(itSignerPair++); - continue; - } + if (!itSignerPair->second.CheckSignature(itSignerPair->first)) { + mapSporksByHash.erase(itSignerPair->second.GetHash()); + itActive->second.erase(itSignerPair++); + continue; } ++itSignerPair; } @@ -100,8 +98,7 @@ void CSporkManager::CheckAndRemove() while (itByHash != mapSporksByHash.end()) { bool found = false; for (const auto& signer: setSporkPubKeyIDs) { - if (itByHash->second.CheckSignature(signer, false) || - itByHash->second.CheckSignature(signer, true)) { + if (itByHash->second.CheckSignature(signer)) { found = true; break; } @@ -141,18 +138,11 @@ void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CD CKeyID keyIDSigner; - // Harden Spork6 so that it is active on testnet and no other networks - bool fSpork6IsActive = Params().NetworkIDString() == CBaseChainParams::TESTNET; - if (!spork.GetSignerKeyID(keyIDSigner, fSpork6IsActive) || !setSporkPubKeyIDs.count(keyIDSigner)) { - // Note: unlike for other messages we have to check for new format even with SPORK_6_NEW_SIGS - // inactive because SPORK_6_NEW_SIGS default is OFF and it is not the first spork to sync - // (and even if it would, spork order can't be guaranteed anyway). - if (!spork.GetSignerKeyID(keyIDSigner, !fSpork6IsActive) || !setSporkPubKeyIDs.count(keyIDSigner)) { - LOCK(cs_main); - LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); - Misbehaving(pfrom->GetId(), 100); - return; - } + if (!spork.GetSignerKeyID(keyIDSigner) || !setSporkPubKeyIDs.count(keyIDSigner)) { + LOCK(cs_main); + LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); + Misbehaving(pfrom->GetId(), 100); + return; } { @@ -198,15 +188,13 @@ bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& conn LOCK(cs); - // Harden Spork6 so that it is active on testnet and no other networks - bool fSpork6IsActive = Params().NetworkIDString() == CBaseChainParams::TESTNET; - if (!spork.Sign(sporkPrivKey, fSpork6IsActive)) { + if (!spork.Sign(sporkPrivKey)) { LogPrintf("CSporkManager::%s -- ERROR: signing failed for spork %d\n", __func__, nSporkID); return false; } CKeyID keyIDSigner; - if (!spork.GetSignerKeyID(keyIDSigner, fSpork6IsActive) || !setSporkPubKeyIDs.count(keyIDSigner)) { + if (!spork.GetSignerKeyID(keyIDSigner) || !setSporkPubKeyIDs.count(keyIDSigner)) { LogPrintf("CSporkManager::UpdateSpork: failed to find keyid for private key\n"); return false; } @@ -316,8 +304,7 @@ bool CSporkManager::SetPrivKey(const std::string& strPrivKey) } CSporkMessage spork; - // Harden Spork6 so that it is active on testnet and no other networks - if (!spork.Sign(key, Params().NetworkIDString() == CBaseChainParams::TESTNET)) { + if (!spork.Sign(key)) { LogPrintf("CSporkManager::SetPrivKey -- Test signing failed\n"); return false; } @@ -349,7 +336,7 @@ uint256 CSporkMessage::GetSignatureHash() const return s.GetHash(); } -bool CSporkMessage::Sign(const CKey& key, bool fSporkSixActive) +bool CSporkMessage::Sign(const CKey& key) { if (!key.IsValid()) { LogPrintf("CSporkMessage::Sign -- signing key is not valid\n"); @@ -359,7 +346,8 @@ bool CSporkMessage::Sign(const CKey& key, bool fSporkSixActive) CKeyID pubKeyId = key.GetPubKey().GetID(); std::string strError = ""; - if (fSporkSixActive) { + // Harden Spork6 so that it is active on testnet and no other networks + if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { uint256 hash = GetSignatureHash(); if(!CHashSigner::SignHash(hash, key, vchSig)) { @@ -388,18 +376,15 @@ bool CSporkMessage::Sign(const CKey& key, bool fSporkSixActive) return true; } -bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId, bool fSporkSixActive) const +bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const { std::string strError = ""; - // fSporkSixActive will only be true on testnet, it will be false on all other networks - if (fSporkSixActive != (Params().NetworkIDString() == CBaseChainParams::TESTNET) return false; - if (fSporkSixActive) { + // Harden Spork6 so that it is active on testnet and no other networks + if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { uint256 hash = GetSignatureHash(); if (!CHashSigner::VerifyHash(hash, pubKeyId, vchSig, strError)) { - // Note: unlike for many other messages when SPORK_6_NEW_SIGS is ON sporks with sigs in old format - // and newer timestamps should not be accepted, so if we failed here - that's it LogPrint(BCLog::SPORK, "CSporkMessage::CheckSignature -- VerifyHash() failed, error: %s\n", strError); return false; } @@ -407,24 +392,19 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId, bool fSporkSixActive) std::string strMessage = std::to_string(nSporkID) + std::to_string(nValue) + std::to_string(nTimeSigned); if (!CMessageSigner::VerifyMessage(pubKeyId, vchSig, strMessage, strError)){ - // Note: unlike for other messages we have to check for new format even with SPORK_6_NEW_SIGS - // inactive because SPORK_6_NEW_SIGS default is OFF and it is not the first spork to sync - // (and even if it would, spork order can't be guaranteed anyway). - uint256 hash = GetSignatureHash(); - if (!CHashSigner::VerifyHash(hash, pubKeyId, vchSig, strError)) { - LogPrint(BCLog::SPORK, "CSporkMessage::CheckSignature -- VerifyHash() failed, error: %s\n", strError); - return false; - } + LogPrint(BCLog::SPORK, "CSporkMessage::CheckSignature -- VerifyMessage() failed, error: %s\n", strError); + return false; } } return true; } -bool CSporkMessage::GetSignerKeyID(CKeyID &retKeyidSporkSigner, bool fSporkSixActive) +bool CSporkMessage::GetSignerKeyID(CKeyID &retKeyidSporkSigner) { CPubKey pubkeyFromSig; - if (fSporkSixActive) { + // Harden Spork6 so that it is active on testnet and no other networks + if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { if (!pubkeyFromSig.RecoverCompact(GetSignatureHash(), vchSig)) { return false; } diff --git a/src/spork.h b/src/spork.h index 6c9d9644b5c7..ea490688c1cd 100644 --- a/src/spork.h +++ b/src/spork.h @@ -120,13 +120,13 @@ class CSporkMessage /** * Sign will sign the spork message with the given key. */ - bool Sign(const CKey& key, bool fSporkSixActive); + bool Sign(const CKey& key); /** * CheckSignature will ensure the spork signature matches the provided public * key hash. */ - bool CheckSignature(const CKeyID& pubKeyId, bool fSporkSixActive) const; + bool CheckSignature(const CKeyID& pubKeyId) const; /** * GetSignerKeyID is used to recover the spork address of the key used to @@ -135,7 +135,7 @@ class CSporkMessage * This method was introduced along with the multi-signer sporks feature, * in order to identify which spork key signed this message. */ - bool GetSignerKeyID(CKeyID& retKeyidSporkSigner, bool fSporkSixActive); + bool GetSignerKeyID(CKeyID& retKeyidSporkSigner); /** * Relay is used to send this spork message to other peers. From e253393e5102a10fd59abf4577550840f819fb50 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 22 Aug 2020 05:36:54 +0300 Subject: [PATCH 5/5] Add TODO in chainparams as a reminder to drop all spork6 related code on next testnet reset --- src/chainparams.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 7c423583faa7..01b12aa5a4bb 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -489,7 +489,7 @@ class CTestNetParams : public CChainParams { consensus.fPowAllowMinDifficultyBlocks = true; consensus.fPowNoRetargeting = false; consensus.nPowKGWHeight = 4002; // nPowKGWHeight >= nPowDGWHeight means "no KGW" - consensus.nPowDGWHeight = 4002; + consensus.nPowDGWHeight = 4002; // TODO: make sure to drop all spork6 related code on next testnet reset consensus.nRuleChangeActivationThreshold = 1512; // 75% for testchains consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;