consensus: Move DoS into contract validators to allow variability of DoS based on context and further fixes to ValidateMRC

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<BeaconPayload>();
 
     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__,

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.

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;
         }
     }

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 Misbehavior 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

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

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 {

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 {}

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
     }

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<PollPayload>();
 
     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)

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.

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 misbehavior 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<GRC::MRC>();
@@ -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 misbehavior 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,57 +4680,77 @@ 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),
                      FormatMoney(mrc.RequiredBurnAmount())
                      );
     }
 
-    // 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.");
+    }
 
-    int64_t mrc_time = pindexBest->nTime;
+    // 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;
+
+    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();
 
     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);
     }
 
     // For testnet, both rejection conditions must be true (i.e. the payment interval by both mrc and tx time is less
     // 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
+    // accommodate 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) {
+        DoS = 25;
+
         return error("%s: Validation failed: MRC payment interval on testnet by both mrc time, %" PRId64 " sec, "
                      "and tx time, %" PRId64 " is less than 1/2 of the MRC Zero Payment Interval of %" PRId64 " sec.",
                      __func__,
@@ -4735,9 +4759,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;