From c814dcaaea73c24a0b12b98a170d4314df7503f6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 4 Oct 2023 20:47:21 +0300 Subject: [PATCH] fix: Move `CreditPoolDiff` checks out of `ProcessSpecialTxsInBlock`, use correct block reward (#5594) ## Issue being fixed or feature implemented The block reward calculation logic in `SetTarget` doesn't work on superblocks. ## What was done? Move `CreditPoolDiff` checks out of `ProcessSpecialTxsInBlock` to use correct block reward. ## How Has This Been Tested? run tests ## Breaking Changes n/a, sb blocks should now be processed correctly, non-sb blocks shouldn't be affected ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ --- src/evo/creditpool.cpp | 18 +++---- src/evo/creditpool.h | 11 ++++- src/evo/specialtxman.cpp | 65 +++++++++++++++++++------- src/evo/specialtxman.h | 2 + src/miner.cpp | 2 +- src/validation.cpp | 17 +++++-- test/functional/feature_asset_locks.py | 11 ++++- 7 files changed, 92 insertions(+), 34 deletions(-) diff --git a/src/evo/creditpool.cpp b/src/evo/creditpool.cpp index f93a7e7cc1..3a13e6086c 100644 --- a/src/evo/creditpool.cpp +++ b/src/evo/creditpool.cpp @@ -228,7 +228,7 @@ void CCreditPoolDiff::AddRewardRealloced(const CAmount reward) { platformReward += reward; } -bool CCreditPoolDiff::SetTarget(const CTransaction& tx, TxValidationState& state) +bool CCreditPoolDiff::SetTarget(const CTransaction& tx, const CAmount blockReward, TxValidationState& state) { CCbTx cbTx; if (!GetTxPayload(tx, cbTx)) { @@ -239,13 +239,8 @@ bool CCreditPoolDiff::SetTarget(const CTransaction& tx, TxValidationState& state targetBalance = cbTx.creditPoolBalance; - if (!llmq::utils::IsMNRewardReallocationActive(pindex)) return true; - CAmount blockReward = 0; - for (const CTxOut& txout : tx.vout) { - blockReward += txout.nValue; - } platformReward = MasternodePayments::PlatformShare(GetMasternodePayment(cbTx.nHeight, blockReward, params.BRRHeight, true)); LogPrintf("CreditPool: set target to %lld with MN reward %lld\n", *targetBalance, platformReward); @@ -292,11 +287,18 @@ bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state) return true; } -bool CCreditPoolDiff::ProcessTransaction(const CTransaction& tx, TxValidationState& state) +bool CCreditPoolDiff::ProcessCoinbaseTransaction(const CTransaction& tx, const CAmount blockReward, TxValidationState& state) { if (tx.nVersion != 3) return true; - if (tx.nType == TRANSACTION_COINBASE) return SetTarget(tx, state); + assert(tx.nType == TRANSACTION_COINBASE); + + return SetTarget(tx, blockReward, state); +} + +bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state) +{ + if (tx.nVersion != 3) return true; if (tx.nType != TRANSACTION_ASSET_LOCK && tx.nType != TRANSACTION_ASSET_UNLOCK) return true; if (!CheckAssetLockUnlockTx(tx, pindex, pool.indexes, state)) { diff --git a/src/evo/creditpool.h b/src/evo/creditpool.h index 84efdca141..c60c5a0364 100644 --- a/src/evo/creditpool.h +++ b/src/evo/creditpool.h @@ -77,12 +77,19 @@ private: public: explicit CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindex, const Consensus::Params& consensusParams); + /** + * This function should be called for each block's coinbase transaction + * to change amount of credit pool + * coinbase transaction's Payload must be valid if nVersion of coinbase transaction equals 3 + */ + bool ProcessCoinbaseTransaction(const CTransaction& tx, const CAmount blockReward, TxValidationState& state); + /** * This function should be called for each Asset Lock/Unlock tx * to change amount of credit pool * @return true if transaction can be included in this block */ - bool ProcessTransaction(const CTransaction& tx, TxValidationState& state); + bool ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state); /** * This function should be called by miner for initialization of MasterNode reward @@ -102,7 +109,7 @@ public: } private: - bool SetTarget(const CTransaction& tx, TxValidationState& state); + bool SetTarget(const CTransaction& tx, const CAmount blockReward, TxValidationState& state); bool Lock(const CTransaction& tx, TxValidationState& state); bool Unlock(const CTransaction& tx, TxValidationState& state); }; diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index e6604ccef3..3a895f7c87 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -138,10 +138,8 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, ll int64_t nTime1 = GetTimeMicros(); const CCreditPool creditPool = creditPoolManager->GetCreditPool(pindex->pprev, consensusParams); - std::optional creditPoolDiff; - if (bool fV20Active_context = llmq::utils::IsV20Active(pindex->pprev); fV20Active_context) { + if (llmq::utils::IsV20Active(pindex->pprev)) { LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString()); - creditPoolDiff.emplace(creditPool, pindex->pprev, consensusParams); } for (const auto& ptr_tx : block.vtx) { @@ -158,21 +156,6 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, ll return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), strprintf("Process Special Transaction failed (tx hash %s) %s", ptr_tx->GetHash().ToString(), tx_state.GetDebugMessage())); } - if (creditPoolDiff != std::nullopt && !creditPoolDiff->ProcessTransaction(*ptr_tx, tx_state)) { - assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS || tx_state.GetResult() == TxValidationResult::TX_BAD_SPECIAL); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), - strprintf("Process Special Transaction failed at Credit Pool (tx hash %s) %s", ptr_tx->GetHash().ToString(), tx_state.GetDebugMessage())); - } - } - if (creditPoolDiff != std::nullopt) { - CAmount locked_proposed{0}; - if(creditPoolDiff->GetTargetBalance()) locked_proposed = *creditPoolDiff->GetTargetBalance(); - - CAmount locked_calculated = creditPoolDiff->GetTotalLocked(); - if (locked_proposed != locked_calculated) { - LogPrintf("%s: mismatched locked amount in CbTx: %lld against re-calculated: %lld\n", __func__, locked_proposed, locked_calculated); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-assetlocked-amount"); - } } int64_t nTime2 = GetTimeMicros(); @@ -265,3 +248,49 @@ bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, llmq: return true; } + +bool CheckCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams, + const CAmount blockReward, BlockValidationState& state) +{ + AssertLockHeld(cs_main); + + try { + + if (!llmq::utils::IsV20Active(pindex->pprev)) return true; + + const CCreditPool creditPool = creditPoolManager->GetCreditPool(pindex->pprev, consensusParams); + LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString()); + CCreditPoolDiff creditPoolDiff(creditPool, pindex->pprev, consensusParams); + + for (const auto& ptr_tx : block.vtx) { + TxValidationState tx_state; + if (ptr_tx->IsCoinBase()) { + if (!creditPoolDiff.ProcessCoinbaseTransaction(*ptr_tx, blockReward, tx_state)) { + assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS); + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), + strprintf("Process Coinbase Transaction failed at Credit Pool (tx hash %s) %s", ptr_tx->GetHash().ToString(), tx_state.GetDebugMessage())); + } + } else { + if (!creditPoolDiff.ProcessLockUnlockTransaction(*ptr_tx, tx_state)) { + assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS); + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), + strprintf("Process Lock/Unlock Transaction failed at Credit Pool (tx hash %s) %s", ptr_tx->GetHash().ToString(), tx_state.GetDebugMessage())); + } + } + } + CAmount locked_proposed{0}; + if (creditPoolDiff.GetTargetBalance()) locked_proposed = *creditPoolDiff.GetTargetBalance(); + + CAmount locked_calculated = creditPoolDiff.GetTotalLocked(); + if (locked_proposed != locked_calculated) { + LogPrintf("%s: mismatched locked amount in CbTx: %lld against re-calculated: %lld\n", __func__, locked_proposed, locked_calculated); + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-assetlocked-amount"); + } + + } catch (const std::exception& e) { + LogPrintf("%s -- failed: %s\n", __func__, e.what()); + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-checkcreditpooldiff"); + } + + return true; +} diff --git a/src/evo/specialtxman.h b/src/evo/specialtxman.h index bceb7f719c..ca7a380160 100644 --- a/src/evo/specialtxman.h +++ b/src/evo/specialtxman.h @@ -30,5 +30,7 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, ll const Consensus::Params& consensusParams, const CCoinsViewCache& view, bool fJustCheck, bool fCheckCbTxMerleRoots, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, llmq::CQuorumBlockProcessor& quorum_block_processor) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +bool CheckCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams, + const CAmount blockReward, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); #endif // BITCOIN_EVO_SPECIALTXMAN_H diff --git a/src/miner.cpp b/src/miner.cpp index da1efb6901..ecdbd51af9 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -463,7 +463,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda // `state` is local here because used to log info about this specific tx TxValidationState state; - if (!creditPoolDiff->ProcessTransaction(iter->GetTx(), state)) { + if (!creditPoolDiff->ProcessLockUnlockTransaction(iter->GetTx(), state)) { if (fUsingModified) { mapModifiedTx.get().erase(modit); failedTx.insert(iter); diff --git a/src/validation.cpp b/src/validation.cpp index f5e304672d..daa092653d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1985,6 +1985,7 @@ static int64_t nTimeForks = 0; static int64_t nTimeVerify = 0; static int64_t nTimeISFilter = 0; static int64_t nTimeSubsidy = 0; +static int64_t nTimeCreditPool = 0; static int64_t nTimeValueValid = 0; static int64_t nTimePayeeValid = 0; static int64_t nTimeProcessSpecial = 0; @@ -2368,14 +2369,22 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, int64_t nTime5_2 = GetTimeMicros(); nTimeSubsidy += nTime5_2 - nTime5_1; LogPrint(BCLog::BENCHMARK, " - GetBlockSubsidy: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_2 - nTime5_1), nTimeSubsidy * MICRO, nTimeSubsidy * MILLI / nBlocksTotal); + if (!CheckCreditPoolDiffForBlock(block, pindex, m_params.GetConsensus(), blockReward, state)) { + return error("ConnectBlock(DASH): CheckCreditPoolDiffForBlock for block %s failed with %s", + pindex->GetBlockHash().ToString(), state.ToString()); + } + + int64_t nTime5_3 = GetTimeMicros(); nTimeCreditPool += nTime5_3 - nTime5_2; + LogPrint(BCLog::BENCHMARK, " - CheckCreditPoolDiffForBlock: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_3 - nTime5_2), nTimeCreditPool * MICRO, nTimeCreditPool * MILLI / nBlocksTotal); + if (!MasternodePayments::IsBlockValueValid(*sporkManager, *governance, *::masternodeSync, block, pindex->nHeight, blockReward, strError)) { // NOTE: Do not punish, the node might be missing governance data LogPrintf("ERROR: ConnectBlock(DASH): %s\n", strError); return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-amount"); } - int64_t nTime5_3 = GetTimeMicros(); nTimeValueValid += nTime5_3 - nTime5_2; - LogPrint(BCLog::BENCHMARK, " - IsBlockValueValid: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_3 - nTime5_2), nTimeValueValid * MICRO, nTimeValueValid * MILLI / nBlocksTotal); + int64_t nTime5_4 = GetTimeMicros(); nTimeValueValid += nTime5_4 - nTime5_3; + LogPrint(BCLog::BENCHMARK, " - IsBlockValueValid: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_4 - nTime5_3), nTimeValueValid * MICRO, nTimeValueValid * MILLI / nBlocksTotal); if (!MasternodePayments::IsBlockPayeeValid(*sporkManager, *governance, *block.vtx[0], pindex->nHeight, blockReward)) { // NOTE: Do not punish, the node might be missing governance data @@ -2383,8 +2392,8 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-payee"); } - int64_t nTime5_4 = GetTimeMicros(); nTimePayeeValid += nTime5_4 - nTime5_3; - LogPrint(BCLog::BENCHMARK, " - IsBlockPayeeValid: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_4 - nTime5_3), nTimePayeeValid * MICRO, nTimePayeeValid * MILLI / nBlocksTotal); + int64_t nTime5_5 = GetTimeMicros(); nTimePayeeValid += nTime5_5 - nTime5_4; + LogPrint(BCLog::BENCHMARK, " - IsBlockPayeeValid: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_5 - nTime5_4), nTimePayeeValid * MICRO, nTimePayeeValid * MILLI / nBlocksTotal); int64_t nTime5 = GetTimeMicros(); nTimeDashSpecific += nTime5 - nTime4; LogPrint(BCLog::BENCHMARK, " - Dash specific: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5 - nTime4), nTimeDashSpecific * MICRO, nTimeDashSpecific * MILLI / nBlocksTotal); diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index 64e43f8545..4670e339b5 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -41,6 +41,7 @@ from test_framework.util import ( assert_equal, assert_greater_than, assert_greater_than_or_equal, + hex_str_to_bytes, ) llmq_type_test = 100 @@ -164,8 +165,15 @@ class AssetLocksTest(DashTestFramework): block_time = best_block["time"] + 1 cbb = create_coinbase(height, dip4_activated=True, v20_activated=True) - cbb.calc_sha256() + gbt = node_wallet.getblocktemplate() + cbb.vExtraPayload = hex_str_to_bytes(gbt["coinbase_payload"]) + cbb.rehash() block = create_block(tip, cbb, block_time, version=4) + # Add quorum commitments from block template + for tx_obj in gbt["transactions"]: + tx = FromHex(CTransaction(), tx_obj["data"]) + if tx.nType == 6: + block.vtx.append(tx) for tx in txes: block.vtx.append(tx) block.hashMerkleRoot = block.calc_merkle_root() @@ -281,6 +289,7 @@ class AssetLocksTest(DashTestFramework): self.sync_all() self.log.info('Mine block with incorrect credit-pool value...') + coin = coins.pop() extra_lock_tx = self.create_assetlock(coin, COIN, pubkey) self.create_and_check_block([extra_lock_tx], expected_error = 'bad-cbtx-assetlocked-amount')