From 0ebf462be40be9a63bbd03b61e983786fca42f4b Mon Sep 17 00:00:00 2001 From: "James C. Owens" Date: Sat, 7 May 2022 21:30:13 -0400 Subject: [PATCH] Move DoS into contract validators to allow variability of DoS based on context Straighten out ValidateMRC --- src/gridcoin/beacon.cpp | 8 ++- src/gridcoin/beacon.h | 3 +- src/gridcoin/contract/contract.cpp | 13 +++-- src/gridcoin/contract/contract.h | 3 +- src/gridcoin/contract/handler.h | 3 +- src/gridcoin/mrc.cpp | 4 +- src/gridcoin/mrc.h | 2 +- src/gridcoin/project.h | 3 +- src/gridcoin/voting/registry.cpp | 10 +++- src/gridcoin/voting/registry.h | 3 +- src/main.cpp | 93 +++++++++++++++++++++--------- src/main.h | 2 +- src/miner.cpp | 3 +- 13 files changed, 103 insertions(+), 47 deletions(-) diff --git a/src/gridcoin/beacon.cpp b/src/gridcoin/beacon.cpp index d654988f19..52e6e73597 100644 --- a/src/gridcoin/beacon.cpp +++ b/src/gridcoin/beacon.cpp @@ -752,7 +752,7 @@ bool BeaconRegistry::SetNeedsIsContractCorrection(bool flag) return m_beacon_db.SetNeedsIsContractCorrection(flag); } -bool BeaconRegistry::Validate(const Contract& contract, const CTransaction& tx) const +bool BeaconRegistry::Validate(const Contract& contract, const CTransaction& tx, int& DoS) const { if (contract.m_version <= 1) { return true; @@ -761,16 +761,19 @@ bool BeaconRegistry::Validate(const Contract& contract, const CTransaction& tx) const auto payload = contract.SharePayloadAs(); if (payload->m_version < 2) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: Legacy beacon contract", __func__); return false; } if (!payload->WellFormed(contract.m_action.Value())) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: Malformed beacon contract", __func__); return false; } if (!payload->VerifySignature()) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: Invalid beacon signature", __func__); return false; } @@ -785,6 +788,7 @@ bool BeaconRegistry::Validate(const Contract& contract, const CTransaction& tx) // of the original beacon: if (contract.m_action == ContractAction::REMOVE) { if (current_beacon->m_public_key != payload->m_beacon.m_public_key) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: Beacon key mismatch", __func__); return false; } @@ -800,11 +804,13 @@ bool BeaconRegistry::Validate(const Contract& contract, const CTransaction& tx) // Transition to version 2 beacons after the block version 11 threshold. // Legacy beacons are not renewable: if (current_beacon->m_timestamp <= g_v11_timestamp) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: Can't renew legacy beacon", __func__); return false; } if (!current_beacon->Renewable(tx.nTime)) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: Beacon for CPID %s is not renewable. Age: %" PRId64, __func__, diff --git a/src/gridcoin/beacon.h b/src/gridcoin/beacon.h index 5982c4e80c..dceca0f0b9 100644 --- a/src/gridcoin/beacon.h +++ b/src/gridcoin/beacon.h @@ -613,10 +613,11 @@ class BeaconRegistry : public IContractHandler //! //! \param contract Contains the beacon data to validate. //! \param tx Transaction that contains the contract. + //! \param DoS Misbehavior out. //! //! \return \c true if the contract contains a valid beacon. //! - bool Validate(const Contract& contract, const CTransaction& tx) const override; + bool Validate(const Contract& contract, const CTransaction& tx, int &DoS) const override; //! //! \brief Register a beacon from contract data. diff --git a/src/gridcoin/contract/contract.cpp b/src/gridcoin/contract/contract.cpp index 26fee5e7bb..8ab316e43d 100644 --- a/src/gridcoin/contract/contract.cpp +++ b/src/gridcoin/contract/contract.cpp @@ -158,7 +158,7 @@ class AppCacheContractHandler : public IContractHandler ClearCache(Section::SCRAPER); } - bool Validate(const Contract& contract, const CTransaction& tx) const override + bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) const override { return true; // No contextual validation needed yet } @@ -195,7 +195,7 @@ class UnknownContractHandler : public IContractHandler // Nothing to do. } - bool Validate(const Contract& contract, const CTransaction& tx) const override + bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) const override { return true; // No contextual validation needed yet } @@ -279,12 +279,13 @@ class Dispatcher //! //! \param contract Contract to validate. //! \param tx Transaction that contains the contract. + //! \param DoS Misbehavior score out. //! //! \return \c false If the contract fails validation. //! - bool Validate(const Contract& contract, const CTransaction& tx) + bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) { - return GetHandler(contract.m_type.Value()).Validate(contract, tx); + return GetHandler(contract.m_type.Value()).Validate(contract, tx, DoS); } //! @@ -595,10 +596,10 @@ void GRC::ApplyContracts( } } -bool GRC::ValidateContracts(const CTransaction& tx) +bool GRC::ValidateContracts(const CTransaction& tx, int& DoS) { for (const auto& contract : tx.GetContracts()) { - if (!g_dispatcher.Validate(contract, tx)) { + if (!g_dispatcher.Validate(contract, tx, DoS)) { return false; } } diff --git a/src/gridcoin/contract/contract.h b/src/gridcoin/contract/contract.h index bc89e4efd6..e081bc2268 100644 --- a/src/gridcoin/contract/contract.h +++ b/src/gridcoin/contract/contract.h @@ -514,10 +514,11 @@ void ApplyContracts( //! \brief Perform contextual validation for the contracts in a transaction. //! //! \param tx Transaction to validate contracts for. +//! \param DoS Mishavior score out parameter //! //! \return \c false When a contract in the transaction fails validation. //! -bool ValidateContracts(const CTransaction& tx); +bool ValidateContracts(const CTransaction& tx, int& DoS); //! //! \brief Revert previously-applied contracts from a transaction by passing diff --git a/src/gridcoin/contract/handler.h b/src/gridcoin/contract/handler.h index cd46dc7d60..01763866b4 100644 --- a/src/gridcoin/contract/handler.h +++ b/src/gridcoin/contract/handler.h @@ -77,10 +77,11 @@ struct IContractHandler //! //! \param contract Contract to validate. //! \param tx Transaction that contains the contract. + //! \param DoS Misbehavior score out. //! //! \return \c false If the contract fails validation. //! - virtual bool Validate(const Contract& contract, const CTransaction& tx) const = 0; + virtual bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) const = 0; //! //! \brief Destroy the contract handler state to prepare for historical diff --git a/src/gridcoin/mrc.cpp b/src/gridcoin/mrc.cpp index f56b95a47d..6aff5379b7 100644 --- a/src/gridcoin/mrc.cpp +++ b/src/gridcoin/mrc.cpp @@ -207,10 +207,10 @@ uint256 MRC::GetHash() const return hasher.GetHash(); } -bool GRC::MRCContractHandler::Validate(const Contract& contract, const CTransaction& tx) const +bool GRC::MRCContractHandler::Validate(const Contract& contract, const CTransaction& tx, int& DoS) const { // Fully validate the incoming MRC txn. - return ValidateMRC(contract, tx); + return ValidateMRC(contract, tx, DoS); } namespace { diff --git a/src/gridcoin/mrc.h b/src/gridcoin/mrc.h index f42f80bf59..32b34c7662 100644 --- a/src/gridcoin/mrc.h +++ b/src/gridcoin/mrc.h @@ -322,7 +322,7 @@ class MRCContractHandler : public IContractHandler // Reset is a noop for MRC's here. void Reset() override {} - bool Validate(const Contract& contract, const CTransaction& tx) const override; + bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) const override; // Add is a noop here, because this is handled at the block level by the staker (in the miner) as with the claim. void Add(const ContractContext& ctx) override {} diff --git a/src/gridcoin/project.h b/src/gridcoin/project.h index f62d4bba49..85cf7e1d8a 100644 --- a/src/gridcoin/project.h +++ b/src/gridcoin/project.h @@ -297,10 +297,11 @@ class Whitelist : public IContractHandler //! //! \param contract Contract to validate. //! \param tx Transaction that contains the contract. + //! \param DoS Misbehavior score out //! //! \return \c false If the contract fails validation. //! - bool Validate(const Contract& contract, const CTransaction& tx) const override + bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) const override { return true; // No contextual validation needed yet } diff --git a/src/gridcoin/voting/registry.cpp b/src/gridcoin/voting/registry.cpp index 6fdfc75488..765618f637 100644 --- a/src/gridcoin/voting/registry.cpp +++ b/src/gridcoin/voting/registry.cpp @@ -773,7 +773,7 @@ void PollRegistry::Reset() m_latest_poll = nullptr; } -bool PollRegistry::Validate(const Contract& contract, const CTransaction& tx) const +bool PollRegistry::Validate(const Contract& contract, const CTransaction& tx, int& DoS) const { // Vote contract claims do not affect consensus. Vote claim validation // occurs on-demand while computing the results of the poll: @@ -791,13 +791,19 @@ bool PollRegistry::Validate(const Contract& contract, const CTransaction& tx) co const auto payload = contract.SharePayloadAs(); if (payload->m_version < 2) { + DoS = 25; LogPrint(LogFlags::CONTRACT, "%s: rejected legacy poll", __func__); return false; } CTxDB txdb("r"); - return PollClaimValidator(txdb).Validate(*payload, tx); + if (!PollClaimValidator(txdb).Validate(*payload, tx)) { + DoS = 25; + return false; + } + + return true; } void PollRegistry::Add(const ContractContext& ctx) diff --git a/src/gridcoin/voting/registry.h b/src/gridcoin/voting/registry.h index 8bea4cc4f7..b4d52474e3 100644 --- a/src/gridcoin/voting/registry.h +++ b/src/gridcoin/voting/registry.h @@ -357,10 +357,11 @@ class PollRegistry : public IContractHandler //! //! \param contract Contract to validate. //! \param tx Transaction that contains the contract. + //! \param DoS Misbehavior score out. //! //! \return \c false If the contract fails validation. //! - bool Validate(const Contract& contract, const CTransaction& tx) const override; + bool Validate(const Contract& contract, const CTransaction& tx, int& DoS) const override; //! //! \brief Register a poll or vote from contract data. diff --git a/src/main.cpp b/src/main.cpp index d44ecc4bbc..745623242b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -395,10 +395,13 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CTransaction &tx, bool* pfMissingInput return error("AcceptToMemoryPool : nonstandard transaction type"); // Perform contextual validation for any contracts: - if (!tx.GetContracts().empty() && !GRC::ValidateContracts(tx)) { - return tx.DoS(25, error("%s: invalid contract in tx %s", - __func__, - tx.GetHash().ToString())); + + int DoS = 0; + if (!tx.GetContracts().empty() && !GRC::ValidateContracts(tx, DoS)) { + return tx.DoS(DoS, error("%s: invalid contract in tx %s, assigning DoS mishehavior of %i", + __func__, + tx.GetHash().ToString(), + DoS)); } // is it already in the memory pool? @@ -412,9 +415,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CTransaction &tx, bool* pfMissingInput // but I am sufficiently concerned about MRC DoS that it is necessary to stop duplicate MRC request transactions // from the same CPID in the accept to memory pool stage. // - // TODO: Consider implementing another map in the mempool indexed by CPID to reduce the expense of this. - // It isn't horrible for right now, because the outer loop only runs when there are contracts, and the inner loop - // only runs if there is an MRC contract on the incoming. + // We have implemented a bloom filter to help with the overhead. for (const auto& contract : tx.GetContracts()) { if (contract.m_type == GRC::ContractType::MRC) { GRC::MRC mrc = contract.CopyPayloadAs(); @@ -1939,10 +1940,12 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex, bool fJustCheck) return false; } - if (nVersion >= 11 && !GRC::ValidateContracts(tx)) { - return tx.DoS(25, error("%s: invalid contract in tx %s", - __func__, - tx.GetHash().ToString())); + int DoS = 0; + if (nVersion >= 11 && !GRC::ValidateContracts(tx, DoS)) { + return tx.DoS(DoS, error("%s: invalid contract in tx %s, assigning DoS mishehavior of %i", + __func__, + tx.GetHash().ToString(), + DoS)); } } @@ -4640,11 +4643,12 @@ GRC::MintSummary CBlock::GetMint() const EXCLUSIVE_LOCKS_REQUIRED(cs_main) //! \param tx The transaction that contains the contract //! \return true if successfully validated //! -bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx) +bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx, int& DoS) { // The MRC transaction should only have one contract on it, and the contract type should be MRC (which we already // know to arrive at this virtual method implementation). if (tx.GetContracts().size() != 1) { + DoS = 25; return error("%s: Validation failed: The transaction, hash %s, that contains the MRC has more than one contract.", __func__, tx.GetHash().GetHex()); @@ -4676,6 +4680,7 @@ bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx) mrc.m_version); if (burn_amount < mrc.RequiredBurnAmount()) { + DoS = 25; return error("%s: Burn amount of %s in mrc contract is less than the required %s.", __func__, FormatMoney(mrc.m_fee), @@ -4683,25 +4688,34 @@ bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx) ); } - // If the mrc last block hash is not pindexBest's block hash return false, because MRC requests can only be valid - // in the mempool if they refer to the head of the current chain. - if (pindexBest->GetBlockHash() != mrc.m_last_block_hash) { - return error("%s: Validation failed: MRC m_last_block_hash %s cannot be found in the chain.", - __func__, - mrc.m_last_block_hash.GetHex()); - } - const GRC::CpidOption cpid = mrc.m_mining_id.TryCpid(); // No Cpid, the MRC must be invalid. - if (!cpid) return error("%s: Validation failed: MRC has no CPID."); + if (!cpid) { + DoS = 25; + return error("%s: Validation failed: MRC has no CPID."); + } + + // If the mrc last block hash is not pindexBest's block hash return false, because MRC requests can only be valid + // in the mempool if they refer to the head of the current chain. Note that this can happen even if the MRC is valid + // if the receiving node is out-of-sync or there is a "crossing in the mail" situation. Therefore we assign a very low + // DoS of 1 for this, and only if the wallet is in sync. (The DoS parameter is initialized to 0 by the caller.) + // Note that a flag is set here, and the DoS assignment and return error is delayed until AFTER the payment interval + // reject code, because the payment_interval_by_tx_time_reject can still be calculated even if the last_block_hash is + // not matched, and the payment_interval_by_tx_time_reject is a more severe error. + int64_t mrc_time = 0; + bool last_block_hash_matched = true; - int64_t mrc_time = pindexBest->nTime; + if (pindexBest->GetBlockHash() != mrc.m_last_block_hash) { + last_block_hash_matched = false; + } else { + mrc_time = pindexBest->nTime; + } // We are not going to even accept MRC transactions to the memory pool that have a payment interval less than // MRCZeroPaymentInterval / 2. This is to prevent a rogue actor from trying to fill slots in a DoS to rightful // MRC recipients. - const int64_t& reject_payment_interval = Params().GetConsensus().MRCZeroPaymentInterval / 2; + const int64_t reject_payment_interval = Params().GetConsensus().MRCZeroPaymentInterval / 2; const GRC::ResearchAccount& account = GRC::Tally::GetAccount(*cpid); const int64_t last_reward_time = account.LastRewardTime(); @@ -4709,14 +4723,20 @@ bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx) const int64_t payment_interval_by_mrc = mrc_time - last_reward_time; const int64_t payment_interval_by_tx_time = tx.nTime - last_reward_time; - bool payment_interval_by_mrc_reject = (payment_interval_by_mrc < reject_payment_interval) ? true : false; - bool payment_interval_by_tx_time_reject = (payment_interval_by_tx_time < reject_payment_interval) ? true : false;; + // If last_block_hash_matched is false then payment_interval_by_mrc_reject is forced false as it cannot be evaluated. + bool payment_interval_by_mrc_reject = (last_block_hash_matched && payment_interval_by_mrc < reject_payment_interval); + bool payment_interval_by_tx_time_reject = (payment_interval_by_tx_time < reject_payment_interval); + + // For mainnet, if either the payment_interval_by_mrc_reject OR the payment_interval_by_tx_time_reject is true, + // then return false. This is stricter than testnet below. See the commentary below on why. + if (!fTestNet && (payment_interval_by_mrc_reject || payment_interval_by_tx_time_reject)) { + DoS = 25; - if (!fTestNet && payment_interval_by_mrc_reject) { return error("%s: Validation failed: MRC payment interval by mrc time, %" PRId64 " sec, is less than 1/2 of the MRC " "Zero Payment Interval of %" PRId64 " sec.", __func__, - payment_interval_by_mrc, + last_block_hash_matched ? + std::min(payment_interval_by_mrc, payment_interval_by_tx_time) : payment_interval_by_tx_time, reject_payment_interval); } @@ -4724,6 +4744,8 @@ bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx) // than 1/2 of MRCZeroPaymentInterval) for the transaction to be rejected. This difference from mainnet is to // accomodate a post testnet v12 change in this function that originally shifted from tx time to mrc time for MRC // payment interval rejection, after some mrc tests were already done post mandatory, which broke syncing from zero. + // Note this has the effect of rendering the payment interval check on testnet inoperative if the + // last_block_hash_matched is false due to the AND instead of OR condition. // // TODO: On the next mandatory align the restriction to mainnet from that point forward. if (fTestNet && payment_interval_by_mrc_reject && payment_interval_by_tx_time_reject) { @@ -4735,9 +4757,24 @@ bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx) reject_payment_interval); } + // This is the second part of the m_last_block_hash match validation. If the payment interval section above passed, + // but the last_block_hash did not match, then return false. Assign a DoS of 1 for this if in sync. + if (!last_block_hash_matched) { + if (!OutOfSyncByAge()) { + DoS = 1; + } + + return error("%s: Validation failed: MRC m_last_block_hash %s cannot be found in the chain.", + __func__, + mrc.m_last_block_hash.GetHex()); + } + // Note that the below overload of ValidateMRC repeats the check for a valid Cpid. It is low overhead and worth not // repeating a bunch of what would be common code to eliminate. - ValidateMRC(pindexBest, mrc); + if (!ValidateMRC(pindexBest, mrc)) { + DoS = 25; + return false; + } // If we get here, return true. return true; diff --git a/src/main.h b/src/main.h index 9ff658565c..510d7e4d3a 100644 --- a/src/main.h +++ b/src/main.h @@ -131,7 +131,7 @@ unsigned int GetCoinstakeOutputLimit(const int& block_version); Fraction FoundationSideStakeAllocation(); CBitcoinAddress FoundationSideStakeAddress(); unsigned int GetMRCOutputLimit(const int& block_version, bool include_foundation_sidestake); -bool ValidateMRC(const GRC::Contract &contract, const CTransaction& tx); +bool ValidateMRC(const GRC::Contract &contract, const CTransaction& tx, int& DoS); bool ValidateMRC(const CBlockIndex* mrc_last_pindex, const GRC::MRC& mrc); int GetNumBlocksOfPeers(); diff --git a/src/miner.cpp b/src/miner.cpp index e62cfc191d..03cf12afb2 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -374,7 +374,8 @@ bool CreateRestOfTheBlock(CBlock &block, CBlockIndex* pindexPrev, // that we don't include a transaction that disrupts validation of // the block: // - if (!tx.GetContracts().empty() && !GRC::ValidateContracts(tx)) { + int DoS = 0; // Unused here. + if (!tx.GetContracts().empty() && !GRC::ValidateContracts(tx, DoS)) { LogPrint(BCLog::LogFlags::MINER, "%s: contract failed contextual validation. Skipped tx %s", __func__,