Merge pull request #2505 from jamescowens/mrc

rpc, contract: Adjust ValidateMRC, CreateMRC, and createmrcrequest to correct provided fee handling

src/gridcoin/mrc.cpp

@@ -209,54 +209,8 @@ uint256 MRC::GetHash() const
 
 bool GRC::MRCContractHandler::Validate(const Contract& contract, const CTransaction& tx) const
 {
-    // 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) return false;
-
-    // Check that the burn in the contract is equal or greater than the required burn.
-    CAmount burn_amount = 0;
-
-    for (const auto& output : tx.vout) {
-        if (output.scriptPubKey == (CScript() << OP_RETURN)) {
-            burn_amount += output.nValue;
-        }
-    }
-
-    GRC::MRC mrc = contract.CopyPayloadAs<GRC::MRC>();
-
-    LogPrintf("INFO: %s: mrc m_client_version = %s, m_fee = %s, m_last_block_hash = %s, m_magnitude = %u, "
-              "m_magnitude_unit = %f, m_mining_id = %s, m_organization = %s, m_research_subsidy = %s, "
-              "m_version = %s",
-              __func__,
-              mrc.m_client_version,
-              FormatMoney(mrc.m_fee),
-              mrc.m_last_block_hash.GetHex(),
-              mrc.m_magnitude,
-              mrc.m_magnitude_unit,
-              mrc.m_mining_id.ToString(),
-              mrc.m_organization,
-              FormatMoney(mrc.m_research_subsidy),
-              mrc.m_version);
-
-    if (burn_amount < mrc.RequiredBurnAmount()) {
-        return error("%s: Burn amount of %s in mrc contract is less than the required %s.",
-                     __func__,
-                     FormatMoney(mrc.m_fee),
-                     FormatMoney(mrc.RequiredBurnAmount())
-                     );
-    }
-
-    // We cannot fully validate incoming mrc transactions to the mempool. During testing under stress, a node can send an
-    // mrc transaction right before the receipt of a block staked by another node, which then results in the just submitted
-    // transaction being invalid on nodes that receive it after the staked block, but valid on the sending node (because
-    // the order is reversed). To avoid this we will relieve the strict requirement here and do a partial validation which
-    // checks the burn fee and the MRC contract signature to ensure the MRC contract is validly signed by an active beacon
-    // holder. The block level validations handle the full MRC validation. This completes the concept of a "bad" or "stale"
-    // mrc transaction being accepted into the memory pool. They will also be bound into the block, but they will be
-    // "absorbed," meaning they will not result in a coinstake payout to the mrc requester. The deterrent to prevent someone
-    // from flooding the mempool with invalid mrc transactions that pass this level of checking is the loss of the burn fees
-    // AND the trouble to create the valid beacon context and signature for each MRC request transaction.
-    return ValidateMRC(tx, mrc);
+    // Fully validate the incoming MRC txn.
+    return ValidateMRC(contract, tx);
 }
 
 namespace {
@@ -359,13 +313,29 @@ bool GRC::CreateMRC(CBlockIndex* pindex,
     mrc.m_organization = gArgs.GetArg("-org", "").substr(0, GRC::Claim::MAX_ORGANIZATION_SIZE);
 
     mrc.m_last_block_hash = pindex->GetBlockHash();
-    mrc.m_fee = mrc.ComputeMRCFee();
-    fee = mrc.m_fee;
 
-    if (!TrySignMRC(pwallet, pindex, mrc)) {
-        error("%s: Failed to sign mrc.", __func__);
+    CAmount computed_mrc_fee = mrc.ComputeMRCFee();
 
-        return false;
+    // If no input fee provided (i.e. zero), then use computed_mrc_fee and also set parameter to this value. If an input
+    // fee is provided, it must be bound by the computed_mrc_fee and the m_research_subsidy (inclusive).
+    if (!fee) {
+        fee = mrc.m_fee = computed_mrc_fee;
+    } else if (fee >= computed_mrc_fee && fee <= mrc.m_research_subsidy) {
+        mrc.m_fee = fee;
+    } else {
+        // Set the output fee equal to computed (for help with error handling)
+        CAmount provided_fee = fee;
+        fee = computed_mrc_fee;
+        return error("%s: Invalid fee specified for mrc. The specified fee of %s is not bounded by the "
+                     "minimum calculated fee of %s and the computed research reward of %s.",
+                     __func__,
+                     FormatMoney(provided_fee),
+                     FormatMoney(computed_mrc_fee),
+                     FormatMoney(mrc.m_research_subsidy));
+    }
+
+    if (!TrySignMRC(pwallet, pindex, mrc)) {
+        return error("%s: Failed to sign mrc.", __func__);
     }
 
     LogPrintf(

src/gridcoin/mrc.h

@@ -337,7 +337,7 @@ class MRCContractHandler : public IContractHandler
 //! \param pindexPrev: The index for the last block (head of the chain) when the MRC was created.
 //! \param mrc: The MRC contract object itself (out parameter)
 //! \param nReward: The research reward (out parameter)
-//! \param fee: The MRC fees to be taken out of the research reward (out parameter)
+//! \param fee: The MRC fees to be taken out of the research reward (in/out parameter)
 //! \param pwallet: The wallet object
 //! \return
 //!

src/main.cpp

@@ -4635,40 +4635,69 @@ GRC::MintSummary CBlock::GetMint() const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 }
 
 //!
-//! \brief Used in GRC::MRCContractHandler::Validate
-//! \param tx The MRC request transaction
-//! \param mrc The MRC contract
+//! \brief Used in GRC::MRCContractHandler::Validate (essentially AcceptToMemoryPool)
+//! \param contract The contract that contains the MRC
+//! \param tx The transaction that contains the contract
 //! \return true if successfully validated
 //!
-bool ValidateMRC(const CTransaction& tx, const GRC::MRC& mrc)
+bool ValidateMRC(const GRC::Contract& contract, const CTransaction& tx)
 {
-    const int64_t& mrc_time = tx.nTime;
+    // 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) {
+        return error("%s: Validation failed: The transaction, hash %s, that contains the MRC has more than one contract.",
+                     __func__,
+                     tx.GetHash().GetHex());
+    }
 
-    const GRC::CpidOption cpid = mrc.m_mining_id.TryCpid();
+    // Check that the burn in the contract is equal or greater than the required burn.
+    CAmount burn_amount = 0;
 
-    // No Cpid, the MRC must be invalid.
-    if (!cpid) return error("%s: Validation failed: MRC has no CPID.");
-
-    // Check to ensure the beacon was active at the transaction time of the MRC and the MRC signature. This also
-    // checks the signature using the block hash in the mrc itself. This is done for the ContractHandler::Validate whereas
-    // the stronger form is done in the ConnectBlock using the overloaded version below.
-    // If this fails, no point in going further.
-    if (const GRC::BeaconOption beacon = GRC::GetBeaconRegistry().TryActive(*cpid, mrc_time)) {
-        if (!mrc.VerifySignature(
-            beacon->m_public_key,
-            mrc.m_last_block_hash)) {
-            return error("%s: Validation failed: MRC signature validation failed for MRC for CPID %s.",
-                         __func__,
-                         cpid->ToString()
-                         );
+    for (const auto& output : tx.vout) {
+        if (output.scriptPubKey == (CScript() << OP_RETURN)) {
+            burn_amount += output.nValue;
         }
-    } else {
-        return error("%s: Validation failed: The beacon for the cpid %s referred to in the MRC is not active.",
+    }
+
+    GRC::MRC mrc = contract.CopyPayloadAs<GRC::MRC>();
+
+    LogPrintf("INFO: %s: mrc m_client_version = %s, m_fee = %s, m_last_block_hash = %s, m_magnitude = %u, "
+              "m_magnitude_unit = %f, m_mining_id = %s, m_organization = %s, m_research_subsidy = %s, "
+              "m_version = %s",
+              __func__,
+              mrc.m_client_version,
+              FormatMoney(mrc.m_fee),
+              mrc.m_last_block_hash.GetHex(),
+              mrc.m_magnitude,
+              mrc.m_magnitude_unit,
+              mrc.m_mining_id.ToString(),
+              mrc.m_organization,
+              FormatMoney(mrc.m_research_subsidy),
+              mrc.m_version);
+
+    if (burn_amount < mrc.RequiredBurnAmount()) {
+        return error("%s: Burn amount of %s in mrc contract is less than the required %s.",
                      __func__,
-                     cpid->ToString()
+                     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.");
+
+    int64_t 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.
@@ -4677,11 +4706,7 @@ bool ValidateMRC(const CTransaction& tx, const GRC::MRC& mrc)
     const GRC::ResearchAccount& account = GRC::Tally::GetAccount(*cpid);
     const int64_t last_reward_time = account.LastRewardTime();
 
-    // Here we are using the nTime of the transaction here instead of the block time of the last block.
-    // Note that tx.nTime is > last_block.nTime, so this is a little looser than
-    // last_block.nTime - last_reward_time < reject_payment_interval, but that is ok, because we are using
-    // half of the MRCZeroPaymentInterval.
-    const int64_t payment_interval = tx.nTime - last_reward_time;
+    const int64_t payment_interval = mrc_time - last_reward_time;
 
     if (payment_interval < reject_payment_interval) {
         return error("%s: Validation failed: MRC payment interval by tx time, %" PRId64 " sec, is less than 1/2 of the MRC "
@@ -4691,6 +4716,10 @@ bool ValidateMRC(const CTransaction& tx, const GRC::MRC& mrc)
                      reject_payment_interval);
     }
 
+    // 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 we get here, return true.
     return true;
 }

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 CTransaction& tx, const GRC::MRC& mrc);
+bool ValidateMRC(const GRC::Contract &contract, const CTransaction& tx);
 bool ValidateMRC(const CBlockIndex* mrc_last_pindex, const GRC::MRC& mrc);
 
 int GetNumBlocksOfPeers();

src/miner.cpp

@@ -1048,11 +1048,11 @@ void SplitCoinStakeOutput(CBlock &blocknew, int64_t &nReward, bool &fEnableStake
     // [MRC 1], ..., [MRC p].
     //
     // Currently according to the output limit rules encoded in CreateMRC and here:
-    // For block version 10:
+    // For block version 10-11:
     // one empty, m <= 6, m + n <= 7, and p = 0.
     //
-    // For block version 11 and above:
-    // one empty, m <= 6, m + n <= 7, and p <= 5.
+    // For block version 12:
+    // one empty, m <= 6, m + n <= 10, and p <= 10.
 
     // The total generated GRC is the total of the reward splits - the fees (the original GridcoinReward which is the
     // research reward + CBR), plus the total of the MRC outputs 2 to p (these outputs already have the fees subtracted)

src/rpc/blockchain.cpp

@@ -2522,22 +2522,16 @@ UniValue createmrcrequest(const UniValue& params, const bool fHelp) {
         throw runtime_error("MRC requests require version 12 blocks to be active.");
     }
 
-    if (!GRC::CreateMRC(pindex, mrc, reward, fee, pwalletMain)) {
-        throw runtime_error("MRC request creation failed.");
-    }
-
+    // If the fee is provided, set the fee for the CreateMRC to this value to override the calculated fee.
     if (provided_fee != 0) {
-        if (!force) {
-            if (provided_fee < fee) {
-                throw runtime_error("Provided fee lower than required.");
-            }
-
-            if (provided_fee > mrc.m_research_subsidy) {
-                throw runtime_error("Provided fee higher than subsidy.");
-            }
-        }
+        fee = provided_fee;
+    }
 
-        mrc.m_fee = provided_fee;
+    // If the fee is not overridden by the provided fee above (i.e. zero), it will be filled in
+    // at the calculated mrc value by CreateMRC. CreateMRC also rechecks the bounds
+    // of the provided fee.
+    if (!GRC::CreateMRC(pindex, mrc, reward, fee, pwalletMain)) {
+        throw runtime_error("MRC request creation failed. Please check the log for details.");
     }
 
     if (!dry_run && !force && reward == fee) {

src/test/gridcoin/mrc_tests.cpp

@@ -193,7 +193,7 @@ BOOST_AUTO_TEST_CASE(createmrc_creates_valid_mrcs)
 {
     account.m_accrual = 72;
     GRC::MRC mrc;
-    CAmount reward, fee;
+    CAmount reward, fee{0};
     GRC::CreateMRC(pindex->pprev, mrc, reward, fee, wallet);
 
     BOOST_CHECK_EQUAL(reward, 72);
@@ -207,7 +207,7 @@ BOOST_AUTO_TEST_CASE(it_accepts_valid_fees)
 {
     account.m_accrual = 72;
     GRC::MRC mrc;
-    CAmount reward, fee;
+    CAmount reward, fee{0};
     GRC::CreateMRC(pindex->pprev, mrc, reward, fee, wallet);
 
     mrc.m_fee = 14;