diff --git a/depends/packages/miniupnpc.mk b/depends/packages/miniupnpc.mk index e63752406107..6a0cfa8cc166 100644 --- a/depends/packages/miniupnpc.mk +++ b/depends/packages/miniupnpc.mk @@ -1,6 +1,6 @@ package=miniupnpc $(package)_version=2.2.7 -$(package)_download_path=https://miniupnp.tuxfamily.org/files/ +$(package)_download_path=http://miniupnp.free.fr/files/ $(package)_file_name=$(package)-$($(package)_version).tar.gz $(package)_sha256_hash=b0c3a27056840fd0ec9328a5a9bac3dc5e0ec6d2e8733349cf577b0aa1e70ac1 $(package)_patches=dont_leak_info.patch cmake_get_src_addr.patch fix_windows_snprintf.patch diff --git a/src/core_write.cpp b/src/core_write.cpp index a3ec9b5643cb..e7627095d5dc 100644 --- a/src/core_write.cpp +++ b/src/core_write.cpp @@ -289,11 +289,11 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry } if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { - if (const auto opt_proTx = GetTxPayload(tx)) { + if (const auto opt_proTx = GetTxPayload(tx); opt_proTx && opt_proTx->netInfo) { entry.pushKV("proRegTx", opt_proTx->ToJson()); } } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { - if (const auto opt_proTx = GetTxPayload(tx)) { + if (const auto opt_proTx = GetTxPayload(tx); opt_proTx && opt_proTx->netInfo) { entry.pushKV("proUpServTx", opt_proTx->ToJson()); } } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index 8cbceb5d6180..7b7a29ba4c4c 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -1093,6 +1093,11 @@ bool CSpecialTxProcessor::UndoSpecialTxsInBlock(const Chainstate& chainstate, co return true; } +void CSpecialTxProcessor::RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null pindex) +{ + m_qblockman.RemoveMineableCommitmentsForBlock(block, pindex); +} + bool CSpecialTxProcessor::CheckCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindex, const CCbTx& cbTx, CAmount blockSubsidy, BlockValidationState& state) { diff --git a/src/evo/specialtxman.h b/src/evo/specialtxman.h index 95a64bc23157..a6b5f0e9863c 100644 --- a/src/evo/specialtxman.h +++ b/src/evo/specialtxman.h @@ -97,6 +97,8 @@ class CSpecialTxProcessor EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool UndoSpecialTxsInBlock(const Chainstate& chainstate, const CBlock& block, const CBlockIndex* pindex, MNListUpdates& updatesRet) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + void RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null pindex) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main); // the returned list will not contain the correct block hash (we can't know it yet as the coinbase TX is not updated yet) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 138cd5ac8f7f..d7721365ca16 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -1190,10 +1190,14 @@ void CGovernanceManager::RemoveInvalidVotes() std::vector changedKeyMNs; for (const auto& [internalId, pdmnState] : diff.updatedMNs) { - auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(internalId); - // BuildDiff will construct itself with MNs that we already have knowledge - // of, meaning that fetch operations should never fail. - assert(oldDmn); + auto newDmn = tip_mn_list.GetMNByInternalId(internalId); + if (!Assume(newDmn)) { + continue; + } + auto oldDmn = lastMNListForVotingKeys->GetMN(newDmn->proTxHash); + if (!Assume(oldDmn)) { + continue; + } if ((pdmnState.fields & CDeterministicMNStateDiff::Field_keyIDVoting) && pdmnState.state.keyIDVoting != oldDmn->pdmnState->keyIDVoting) { changedKeyMNs.emplace_back(oldDmn->collateralOutpoint); } else if ((pdmnState.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && pdmnState.state.pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) { @@ -1202,9 +1206,9 @@ void CGovernanceManager::RemoveInvalidVotes() } for (const auto& id : diff.removedMns) { auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(id); - // BuildDiff will construct itself with MNs that we already have knowledge - // of, meaning that fetch operations should never fail. - assert(oldDmn); + if (!Assume(oldDmn)) { + continue; + } changedKeyMNs.emplace_back(oldDmn->collateralOutpoint); } diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 82b0a87a81a1..24385a7bd728 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -254,8 +255,7 @@ bool CQuorumBlockProcessor::ProcessBlock(Chainstate& chainstate, const CBlock& b // ProcessCommitment accepts a commitment identical to the stored one, so // rotation's multiple commitments per type need an explicit per-block check. - // It runs before any commitment is processed, because ProcessCommitment - // updates in-memory state that a rejected block would not restore. + // It runs before the signature checks, so a duplicate is rejected cheaply. // Null commitments stay exempt: they were never subject to bad-qc-dup. std::set> seen_quorums; for (const auto& [_, qc] : qcs) { @@ -445,12 +445,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh // invalidation should not depend on that. DropQcHashesCache(); - { - LOCK(minableCommitmentsCs); - mapMinedCommitmentBlockCache.erase(qc.llmqType, qc.quorumHash); - minableCommitmentsByQuorum.erase(cacheKey); - minableCommitments.erase(::SerializeHash(qc)); - } + WITH_LOCK(minableCommitmentsCs, mapMinedCommitmentBlockCache.erase(qc.llmqType, qc.quorumHash)); LogPrint(BCLog::LLMQ, "%s -- processed commitment from block. type=%d, quorumIndex=%d, quorumHash=%s\n", __func__, std23::to_underlying(qc.llmqType), qc.quorumIndex, quorumHash.ToString()); @@ -561,6 +556,23 @@ bool CQuorumBlockProcessor::UndoBlock(const Chainstate& chainstate, const CBlock return true; } +void CQuorumBlockProcessor::RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null pindex) +{ + AssertLockHeld(::cs_main); + + std::multimap qcs; + if (BlockValidationState dummy; !Assume(GetCommitmentsFromBlock(block, pindex, qcs, dummy))) { + return; + } + + LOCK(minableCommitmentsCs); + for (const auto& [_, qc] : qcs) { + if (qc.IsNull()) continue; + minableCommitmentsByQuorum.erase(std::make_pair(qc.llmqType, qc.quorumHash)); + minableCommitments.erase(::SerializeHash(qc)); + } +} + bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, gsl::not_null pindex, std::multimap& ret, BlockValidationState& state) { AssertLockHeld(::cs_main); diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index c248a13fd29f..396ab1245310 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -108,6 +108,9 @@ class CQuorumBlockProcessor bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex); bool UndoBlock(const Chainstate& chainstate, const CBlock& block, gsl::not_null pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs, !m_qc_hashes_cache_mutex); + //! Drop the commitments mined by a block that has been fully connected from the mineable pool. + void RemoveMineableCommitmentsForBlock(const CBlock& block, gsl::not_null pindex) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); //! it returns hash of commitment if it should be relay, otherwise nullopt std::optional AddMineableCommitment(const CFinalCommitment& fqc) EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp index 267a5bc372a4..04ad6bf8816d 100644 --- a/src/llmq/dkgsessionhandler.cpp +++ b/src/llmq/dkgsessionhandler.cpp @@ -52,7 +52,12 @@ void CDKGPendingMessages::PushPendingMessage(NodeId from, const uint256& sender_ count++; seenMessages.emplace(hash); - pendingMessages.emplace_back(from, std::move(pm)); + if (from == -1) { + // Our own message must not wait behind a backlog of peer messages before it is relayed + pendingMessages.emplace_front(from, std::move(pm)); + } else { + pendingMessages.emplace_back(from, std::move(pm)); + } } std::list CDKGPendingMessages::PopPendingMessages(size_t maxCount) diff --git a/src/llmq/dkgsessionhandler.h b/src/llmq/dkgsessionhandler.h index f52bbc81c416..eb3d560343fe 100644 --- a/src/llmq/dkgsessionhandler.h +++ b/src/llmq/dkgsessionhandler.h @@ -71,7 +71,8 @@ class CDKGPendingMessages * Enqueue a serialized DKG message under @p from with content hash @p hash. * @p sender_protx keys the per-proTx quota: the sender's MNAuth-verified * proTxHash for remote messages, or this node's own for messages it - * produced itself (@p from == -1). Drops the message silently on quota + * produced itself (@p from == -1), which go to the front of the queue. + * Drops the message silently on quota * overflow or duplicate hash; quota-dropped messages are not marked seen, * so another peer with budget can re-deliver them. Caller is responsible * for hashing the payload and (for real peers) routing the erase-request diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 5b68ffb0059d..02a2a81b8456 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -130,11 +130,22 @@ void CDKGSessionManager::WriteVerifiedVvecContribution(Consensus::LLMQType llmqT s << CBLSPublicKeyVersionWrapper(pubkey, false); } db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), Span{s}); + + // Reading the vvec back at commit time costs a point decompression and subgroup check per coefficient + LOCK(contributionsCacheCs); + auto& entry = contributionsCache[{llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash}]; + entry.entryTime = SteadyClock::now(); + entry.vvec = vvec; } void CDKGSessionManager::WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution) { db->Write(std::make_tuple(DB_SKCONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), skContribution); + + LOCK(contributionsCacheCs); + if (auto it = contributionsCache.find({llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash}); it != contributionsCache.end()) { + it->second.skContribution = skContribution; + } } void CDKGSessionManager::WriteEncryptedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSIESMultiRecipientObjects& contributions) @@ -228,13 +239,10 @@ void CDKGSessionManager::CleanupCache() const { LOCK(contributionsCacheCs); const auto curTime = SteadyClock::now(); - for (auto it = contributionsCache.begin(); it != contributionsCache.end(); ) { - if (curTime - it->second.entryTime > MAX_CONTRIBUTION_CACHE_TIME) { - it = contributionsCache.erase(it); - } else { - ++it; - } - } + // A vvec still referenced elsewhere (by its DKG session) costs nothing to keep and is expensive to reload + std::erase_if(contributionsCache, [&](const auto& entry) { + return entry.second.vvec.use_count() == 1 && curTime - entry.second.entryTime > MAX_CONTRIBUTION_CACHE_TIME; + }); } void CDKGSessionManager::CleanupOldContributions() const diff --git a/src/llmq/dkgsessionmgr.h b/src/llmq/dkgsessionmgr.h index cf1cdfa52d1e..8e19cd1f1aad 100644 --- a/src/llmq/dkgsessionmgr.h +++ b/src/llmq/dkgsessionmgr.h @@ -140,8 +140,10 @@ class CDKGSessionManager bool GetPrematureCommitment(const uint256& hash, CDKGPrematureCommitment& ret) const; // Contributions are written while in the DKG - void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec); - void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution); + void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec) + EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs); + void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution) + EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs); bool GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers, std::vector& memberIndexesRet, std::vector& vvecsRet, diff --git a/src/llmq/ehf_signals.cpp b/src/llmq/ehf_signals.cpp index fc8563fdc197..a4cf7c91b6ef 100644 --- a/src/llmq/ehf_signals.cpp +++ b/src/llmq/ehf_signals.cpp @@ -94,7 +94,7 @@ void CEHFSignalsHandler::trySignEHFSignal(int bit, const CBlockIndex* const pind const uint256 msgHash = mnhfPayload.PrepareTx().GetHash(); WITH_LOCK(cs, ids.insert(requestId)); - shareman.AsyncSignIfMember(llmqType, requestId, msgHash, quorum->qc->quorumHash, false, true); + shareman.AsyncSignIfMember(llmqType, requestId, msgHash, quorum->qc->quorumHash, true, true); } RecoveredSigResult CEHFSignalsHandler::HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) diff --git a/src/net.cpp b/src/net.cpp index c248af0f4ff9..9e484785b456 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1771,6 +1771,7 @@ std::pair CConnman::SocketSendData(CNode& node) const LogPrint(BCLog::NET, "socket send error for peer=%d: %s\n", node.GetId(), NetworkErrorString(nErr)); node.fDisconnect = true; } + node.fCanSendData = false; } break; } diff --git a/src/qt/mnsharesession.cpp b/src/qt/mnsharesession.cpp index cf96952a193d..5f6bcad18b66 100644 --- a/src/qt/mnsharesession.cpp +++ b/src/qt/mnsharesession.cpp @@ -104,19 +104,6 @@ bool SameShare(const MnShareSession::Share& a, const MnShareSession::Share& b) a.rewardAddress == b.rewardAddress; } -bool SameContribution(const MnShareSession::Contribution& a, const MnShareSession::Contribution& b) -{ - if (a.inputs.size() != b.inputs.size() || a.hasChange != b.hasChange) return false; - if (a.hasChange && (a.changeAddress != b.changeAddress || a.changeAmount != b.changeAmount)) return false; - for (size_t i = 0; i < a.inputs.size(); ++i) { - if (a.inputs[i].txid.compare(b.inputs[i].txid, Qt::CaseInsensitive) != 0 || - a.inputs[i].vout != b.inputs[i].vout || a.inputs[i].sequence != b.inputs[i].sequence) { - return false; - } - } - return true; -} - bool IsTxidHex(const QString& txid) { return txid.size() == 64 && IsHex(txid.toStdString()); @@ -191,6 +178,19 @@ bool DecodeSharedProTx(const QString& protx_hex, CMutableTransaction& tx, CProRe } // anonymous namespace +bool MnShareSession::SameContribution(const Contribution& a, const Contribution& b) +{ + if (a.inputs.size() != b.inputs.size() || a.hasChange != b.hasChange) return false; + if (a.hasChange && (a.changeAddress != b.changeAddress || a.changeAmount != b.changeAmount)) return false; + for (size_t i = 0; i < a.inputs.size(); ++i) { + if (a.inputs[i].txid.compare(b.inputs[i].txid, Qt::CaseInsensitive) != 0 || + a.inputs[i].vout != b.inputs[i].vout || a.inputs[i].sequence != b.inputs[i].sequence) { + return false; + } + } + return true; +} + namespace { //! U+202F NARROW NO-BREAK SPACE. BitcoinUnits groups thousands with a plain //! thin space, which a layout may break a number across. diff --git a/src/qt/mnsharesession.h b/src/qt/mnsharesession.h index 0dc8f0176ce0..fae57afc8662 100644 --- a/src/qt/mnsharesession.h +++ b/src/qt/mnsharesession.h @@ -313,6 +313,9 @@ class MnShareSession //! Whether a row with `share`'s owner address has the same amount and addresses here bool hasShare(const Share& share) const; + //! Same inputs, in order and with their sequences, and the same change. + //! Labels and changeIndex are ignored. + static bool SameContribution(const Contribution& a, const Contribution& b); //! Whether a contribution with `contribution`'s label spends the same coins and pays the same change here bool hasContribution(const Contribution& contribution) const; diff --git a/src/qt/sharedmncreatedialog.cpp b/src/qt/sharedmncreatedialog.cpp index 5588e7236022..e6c85718582c 100644 --- a/src/qt/sharedmncreatedialog.cpp +++ b/src/qt/sharedmncreatedialog.cpp @@ -1986,8 +1986,18 @@ void SharedMnCreateDialog::refreshContributionPage() const CAmount target{contributionTarget()}; CoinSelection selection; QString error; + CAmount again_total{0}; if (target <= 0) { m_coins_label->setText(tr("Choose which participant you are.")); + } else if (const auto* again{reservableAgain(&again_total)}) { + const QString coins{CoinCount(again->inputs.size())}; + m_coins_label->setText( + again->hasChange + ? tr("Uses %1 (%2) as reserved before; change %3 returns to the same address of yours.") + .arg(coins, FormatAmount(m_wallet_model, again_total), + FormatAmount(m_wallet_model, again->changeAmount)) + : tr("Uses %1 (%2) as reserved before, with no change left over.") + .arg(coins, FormatAmount(m_wallet_model, again_total))); } else if (selectCoins(target, selection, error)) { const QString coins{CoinCount(selection.inputs.size())}; m_coins_label->setText( @@ -2358,6 +2368,7 @@ void SharedMnCreateDialog::startSession() unlockAllContributedCoins(); m_coins_released = false; m_released_contributions.clear(); + m_built_contributions.clear(); m_session = MnShareSession(); m_role = Role::Coordinator; m_my_share = 0; @@ -3372,23 +3383,19 @@ CAmount SharedMnCreateDialog::contributionTarget() const return m_role == Role::Coordinator ? share + m_fee_field->value() : share; } -bool SharedMnCreateDialog::selectCoins(CAmount target, CoinSelection& selection, QString& error) const +bool SharedMnCreateDialog::isDust(CAmount value) const { - selection = CoinSelection{}; - error.clear(); - if (m_wallet_model == nullptr) { - error = tr("No wallet is available."); - return false; - } - if (target <= 0) { - error = tr("Select the participant that is you."); - return false; - } + const CTxOut probe{value, GetScriptForDestination(PKHash{CKeyID{}})}; + return IsDust(probe, m_node.getDustRelayFee()); +} +std::vector> SharedMnCreateDialog::fundingCandidates() const +{ + std::vector> candidates; + if (m_wallet_model == nullptr) return candidates; // listCoins() already holds back unconfirmed coins the wallet does not // trust, so an InstantSend-locked coin or this wallet's own unconfirmed // change can fund a share, as it can any other payment - std::vector> candidates; for (const auto& [dest, coins] : m_wallet_model->wallet().listCoins()) { for (const auto& [outpoint, txout] : coins) { if (txout.is_spent) continue; @@ -3397,6 +3404,58 @@ bool SharedMnCreateDialog::selectCoins(CAmount target, CoinSelection& selection, candidates.emplace_back(txout.txout.nValue, outpoint); } } + return candidates; +} + +bool SharedMnCreateDialog::fundsShareAgain(const MnShareSession::Contribution& released, CAmount* total_out) const +{ + if (m_wallet_model == nullptr || released.inputs.empty()) return false; + const auto candidates{fundingCandidates()}; + CAmount total{0}; + for (const auto& input : released.inputs) { + const COutPoint outpoint{uint256S(input.txid.toStdString()), input.vout}; + const auto coin{std::find_if(candidates.begin(), candidates.end(), + [&](const auto& candidate) { return candidate.second == outpoint; })}; + if (coin == candidates.end()) return false; + total += coin->first; + } + if (total_out != nullptr) *total_out = total; + const CAmount target{contributionTarget()}; + if (released.hasChange) return total - released.changeAmount == target; + // Without change, a dust remainder goes to the fee, as selectCoins() leaves it + return total == target || (total > target && isDust(total - target)); +} + +const MnShareSession::Contribution* SharedMnCreateDialog::reservableAgain(CAmount* total_out) const +{ + if (m_my_share < 0 || static_cast(m_my_share) >= m_session.shares().size()) return nullptr; + const QString& label{m_session.shares()[m_my_share].label}; + // Only a contribution this wallet built itself goes back: one absorbed from + // a reply under this share's name can carry anything the sender chose + for (auto released{m_released_contributions.rbegin()}; released != m_released_contributions.rend(); ++released) { + if (released->label != label) continue; + const auto built{std::find_if(m_built_contributions.begin(), m_built_contributions.end(), [&](const auto& c) { + return c.label == label && MnShareSession::SameContribution(c, *released); + })}; + if (built != m_built_contributions.end() && fundsShareAgain(*built, total_out)) return &*built; + } + return nullptr; +} + +bool SharedMnCreateDialog::selectCoins(CAmount target, CoinSelection& selection, QString& error) const +{ + selection = CoinSelection{}; + error.clear(); + if (m_wallet_model == nullptr) { + error = tr("No wallet is available."); + return false; + } + if (target <= 0) { + error = tr("Select the participant that is you."); + return false; + } + + std::vector> candidates{fundingCandidates()}; // Largest first keeps the input count, and with it everyone's signing work // and the transaction fee, as small as possible. std::sort(candidates.begin(), candidates.end(), @@ -3405,13 +3464,9 @@ bool SharedMnCreateDialog::selectCoins(CAmount target, CoinSelection& selection, // A change output below the dust threshold cannot be relayed, so keep // adding coins until the change clears it; if nothing does, drop the // change and let the remainder go to the fee. - const auto is_dust = [this](CAmount value) { - const CTxOut probe{value, GetScriptForDestination(PKHash{CKeyID{}})}; - return IsDust(probe, m_node.getDustRelayFee()); - }; size_t used{0}; for (; used < candidates.size(); ++used) { - if (selection.total >= target && (selection.total == target || !is_dust(selection.total - target))) break; + if (selection.total >= target && (selection.total == target || !isDust(selection.total - target))) break; selection.total += candidates[used].first; MnShareSession::Input input; input.txid = QString::fromStdString(candidates[used].second.hash.ToString()); @@ -3428,7 +3483,7 @@ bool SharedMnCreateDialog::selectCoins(CAmount target, CoinSelection& selection, return false; } const CAmount change{selection.total - target}; - selection.change = (change > 0 && !is_dust(change)) ? change : 0; + selection.change = (change > 0 && !isDust(change)) ? change : 0; return true; } @@ -3475,28 +3530,37 @@ void SharedMnCreateDialog::reserveCoins() return; } - CoinSelection selection; - if (!selectCoins(contributionTarget(), selection, error)) { - showError(error); - return; - } + // Whoever replied before a release holds the contribution as it was then, + // and refuses locked terms with any other. While its coins are still free + // and fund the same amount, reserve exactly that contribution again. MnShareSession::Contribution contribution; - contribution.label = share.label; - contribution.inputs = selection.inputs; - if (selection.change > 0) { - const QString change_address{MasternodeWidgetUtil::freshAddress(m_wallet_model, error)}; - if (change_address.isEmpty()) { + const auto* const again{reservableAgain()}; + if (again != nullptr) { + contribution = *again; + } else { + CoinSelection selection; + if (!selectCoins(contributionTarget(), selection, error)) { showError(error); return; } - contribution.hasChange = true; - contribution.changeAddress = change_address; - contribution.changeAmount = selection.change; + contribution.label = share.label; + contribution.inputs = selection.inputs; + if (selection.change > 0) { + const QString change_address{MasternodeWidgetUtil::freshAddress(m_wallet_model, error)}; + if (change_address.isEmpty()) { + showError(error); + return; + } + contribution.hasChange = true; + contribution.changeAddress = change_address; + contribution.changeAmount = selection.change; + } } if (!m_session.addContribution(contribution, error)) { showError(error); return; } + if (again == nullptr) m_built_contributions.push_back(contribution); setContributionLocked(contribution, /*lock=*/true); m_coins_released = false; // Coins reserved again are no longer released diff --git a/src/qt/sharedmncreatedialog.h b/src/qt/sharedmncreatedialog.h index 9a33083524a1..808ad1088c84 100644 --- a/src/qt/sharedmncreatedialog.h +++ b/src/qt/sharedmncreatedialog.h @@ -21,6 +21,7 @@ #include #include #include +#include #include struct SharedMnOwnFunding; @@ -274,6 +275,16 @@ private Q_SLOTS: void followNewerInvitation(const MnShareSession& imported); // Funding + //! Whether a P2PKH output of `value` would be dust + bool isDust(CAmount value) const; + //! This wallet's unspent, unlocked, spendable coins that may fund a share + std::vector> fundingCandidates() const; + //! Whether `released`'s coins are all still funding candidates, and fund + //! exactly what the share needs now + bool fundsShareAgain(const MnShareSession::Contribution& released, CAmount* total_out = nullptr) const; + //! The most recently released contribution of this wallet's share that + //! Reserve would put back as it was, with its input total, or nullptr + const MnShareSession::Contribution* reservableAgain(CAmount* total_out = nullptr) const; //! Confirmed, unlocked, spendable wallet coins covering `target`, largest //! first, with the change kept above the dust threshold when possible bool selectCoins(CAmount target, CoinSelection& selection, QString& error) const; @@ -409,6 +420,9 @@ private Q_SLOTS: //! less any coins reserved again since. A reply to an earlier invitation //! still carries them, and only Reserve may put them back. std::vector m_released_contributions; + //! Contributions Reserve has reserved, built from this wallet's own coin + //! selection or put back: the only ones it may put back after a release + std::vector m_built_contributions; Role m_role{Role::Undecided}; int m_my_share{-1}; std::optional m_validation_page; diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index df8888215cad..9b9266630f4f 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -22,6 +22,7 @@ #include +#include #include #include #include @@ -47,11 +48,14 @@ void EditAddressAndSubmit( const QString& label, const QString& address, QString expected_msg) { QString warning_text; + // A successful edit opens no message box, and the answer must not wait + // for the next one, which belongs to whoever opens it + QObject scope; dialog->findChild("labelEdit")->setText(label); dialog->findChild("addressEdit")->setText(address); - ConfirmMessage(&warning_text, 5ms); + ConfirmMessage(&warning_text, 5ms, &scope); dialog->accept(); QCOMPARE(warning_text, expected_msg); } @@ -167,6 +171,13 @@ void TestAddAddressesToSendBook(interfaces::Node& node) check_addbook_size(4); QCOMPARE(table_view->model()->rowCount(), 3); + // The next message box is answered by whoever opens it + QMessageBox later(QMessageBox::Question, QString(), QString(), QMessageBox::Yes | QMessageBox::No); + later.setDefaultButton(QMessageBox::Yes); + QTimer::singleShot(100ms, &later, [&later] { later.button(QMessageBox::No)->click(); }); + later.exec(); + QCOMPARE(later.clickedButton(), later.button(QMessageBox::No)); + auto search_line = address_book.findChild("searchLineEdit"); search_line->setText(r_label); diff --git a/src/qt/test/sharedmnwizardtests.cpp b/src/qt/test/sharedmnwizardtests.cpp index 50ae27f77f61..f4967448649c 100644 --- a/src/qt/test/sharedmnwizardtests.cpp +++ b/src/qt/test/sharedmnwizardtests.cpp @@ -75,6 +75,7 @@ using MasternodeTestUtil::FreshOperatorPubKey; using MasternodeTestUtil::FreshP2PKHAddress; using MasternodeTestUtil::DraftReply; using MasternodeTestUtil::MakeSpendingWallet; +using MasternodeTestUtil::MakeTestWallet; using MasternodeTestUtil::PrepareRegistration; using MasternodeTestUtil::PreparedRegistration; using MasternodeTestUtil::WalletGuard; @@ -400,6 +401,228 @@ void SharedMnWizardTests::coordinatorResumesSavedSession() QCOMPARE(int(participant.currentPage()), int(SharedMnCreateDialog::PageContribution)); } +void SharedMnWizardTests::reservingAgainKeepsEarlierReplies() +{ + TestChain100Setup test; + m_node.setContext(&test.m_node); + WalletContext& context{*m_node.walletLoader().context()}; + const auto wallet{MakeTestWallet(m_node, context, "coord")}; + QVERIFY(wallet != nullptr); + WalletGuard guard{context, wallet}; + CTxDestination owner; + COutPoint coin; + COutPoint other_coin; + COutPoint twice_coin; + COutPoint dust_coin; + COutPoint dust_change_coin; + COutPoint sequence_coin; + COutPoint larger_coin; + { + LOCK(wallet->cs_wallet); + const auto destination{wallet->GetNewDestination("")}; + QVERIFY(destination); + owner = *destination; + const CBlockIndex* tip{ + WITH_LOCK(test.m_node.chainman->GetMutex(), return test.m_node.chainman->ActiveChain().Tip())}; + wallet->SetLastBlockProcessed(tip->nHeight, tip->GetBlockHash()); + CMutableTransaction funding; + funding.vin.emplace_back(GetRandHash(), 0); + funding.vout.emplace_back(450 * COIN, GetScriptForDestination(owner)); + QVERIFY(wallet->AddToWallet(MakeTransactionRef(funding), + wallet::TxStateConfirmed{tip->GetBlockHash(), tip->nHeight, /*index=*/0}) != nullptr); + coin = COutPoint{funding.GetHash(), 0}; + CMutableTransaction other_funding; + other_funding.vin.emplace_back(GetRandHash(), 0); + other_funding.vout.emplace_back(420 * COIN, GetScriptForDestination(owner)); + QVERIFY(wallet->AddToWallet(MakeTransactionRef(other_funding), + wallet::TxStateConfirmed{tip->GetBlockHash(), tip->nHeight, /*index=*/1}) != nullptr); + other_coin = COutPoint{other_funding.GetHash(), 0}; + // 410 for a contribution listing its coin twice; one with a dust + // remainder over a share of 400 and the default 0.001 fee + CMutableTransaction more_funding; + more_funding.vin.emplace_back(GetRandHash(), 0); + more_funding.vout.emplace_back(410 * COIN, GetScriptForDestination(owner)); + more_funding.vout.emplace_back(400 * COIN + COIN / 1000 + 100, GetScriptForDestination(owner)); + more_funding.vout.emplace_back(400 * COIN + COIN / 1000 + 100, GetScriptForDestination(owner)); + more_funding.vout.emplace_back(405 * COIN, GetScriptForDestination(owner)); + QVERIFY(wallet->AddToWallet(MakeTransactionRef(more_funding), + wallet::TxStateConfirmed{tip->GetBlockHash(), tip->nHeight, /*index=*/2}) != nullptr); + twice_coin = COutPoint{more_funding.GetHash(), 0}; + dust_coin = COutPoint{more_funding.GetHash(), 1}; + dust_change_coin = COutPoint{more_funding.GetHash(), 2}; + sequence_coin = COutPoint{more_funding.GetHash(), 3}; + // A larger coin a fresh selection would prefer, held back until needed + CMutableTransaction larger_funding; + larger_funding.vin.emplace_back(GetRandHash(), 0); + larger_funding.vout.emplace_back(600 * COIN, GetScriptForDestination(owner)); + QVERIFY(wallet->AddToWallet(MakeTransactionRef(larger_funding), + wallet::TxStateConfirmed{tip->GetBlockHash(), tip->nHeight, /*index=*/3}) != nullptr); + larger_coin = COutPoint{larger_funding.GetHash(), 0}; + } + MasternodeTestUtil::GuiModels models{m_node}; + QVERIFY2(models.ok, qPrintable(QString::fromStdString(models.error.translated))); + WalletModel wallet_model(interfaces::MakeWallet(context, wallet), models.client); + wallet_model.wallet().lockCoin(larger_coin, /*write_to_db=*/false); + + // This wallet coordinates and has answered for alice's row, but has not + // reserved its coins yet + CBLSSecretKey operator_secret; + operator_secret.MakeNewKey(); + MnShareSession draft{InvitationSession()}; + draft.setPrepareWallet(QStringLiteral("coord")); + draft.terms().operatorPubKey = + QString::fromStdString(operator_secret.GetPublicKey().ToString(/*specificLegacyScheme=*/false)); + draft.shares()[0].ownerAddress = QString::fromStdString(EncodeDestination(owner)); + draft.shares()[0].refundAddress = FreshP2PKHAddress(); + + SharedMnCreateDialog dialog(m_node, &wallet_model, /*parent=*/nullptr); + dialog.handleImportedText(draft.toJsonString()); + QCOMPARE(int(dialog.m_role), int(SharedMnCreateDialog::Role::Coordinator)); + QCOMPARE(dialog.myShareIndex(), 0); + dialog.goToPage(SharedMnCreateDialog::PageContribution); + dialog.reserveCoins(); + QVERIFY2(dialog.myContribution() != nullptr, qPrintable(dialog.m_error_label->text())); + const MnShareSession::Contribution sent{*dialog.myContribution()}; + QVERIFY(sent.hasChange); + QVERIFY(wallet_model.wallet().isLockedCoin(coin)); + + // bob and carol answer the invitation that carried this contribution + const MnShareSession invitation{dialog.m_session}; + const MnShareSession bob{DraftReply(invitation, 1, FakeTxid('2'))}; + const MnShareSession carol{DraftReply(invitation, 2, FakeTxid('3'))}; + QVERIFY(bob.hasContribution(sent)); + + // The coordinator releases its coins, takes in bob's reply, and reserves + // again: the same coins and change, which bob's copy already lists + dialog.releaseCoins(); + QVERIFY(!wallet_model.wallet().isLockedCoin(coin)); + dialog.handleImportedText(bob.toJsonString()); + QVERIFY2(dialog.m_error_label->text().isEmpty(), qPrintable(dialog.m_error_label->text())); + QVERIFY(dialog.myContribution() == nullptr); + QVERIFY(!wallet_model.wallet().isLockedCoin(coin)); + + // The coins card describes those coins, not the larger one that has + // become available since and a fresh selection would take + const auto amount = [&](CAmount value) { return SharedMnFormatAmount(SharedMnDisplayUnit(&wallet_model), value); }; + wallet_model.wallet().unlockCoin(larger_coin); + dialog.refreshContributionPage(); + const QString preview{dialog.m_coins_label->text()}; + QVERIFY2(preview.contains(amount(sent.changeAmount)) && !preview.contains(amount(600 * COIN)), qPrintable(preview)); + wallet_model.wallet().lockCoin(larger_coin, /*write_to_db=*/false); + dialog.reserveCoins(); + QVERIFY2(dialog.myContribution() != nullptr, qPrintable(dialog.m_error_label->text())); + QVERIFY(dialog.m_session.hasContribution(sent)); + QVERIFY(wallet_model.wallet().isLockedCoin(coin)); + + // So the terms locked from it are the ones bob agreed to answer + dialog.handleImportedText(carol.toJsonString()); + QVERIFY2(dialog.m_error_label->text().isEmpty(), qPrintable(dialog.m_error_label->text())); + QVERIFY(dialog.allDetailsCollected()); + MnShareSession locked{dialog.m_session}; + const PreparedRegistration prepared{PrepareRegistration(locked, operator_secret)}; + QString error; + QVERIFY2(locked.freeze(prepared.txHex, prepared.consentHashHex, prepared.collateralIndex, error), + qPrintable(error)); + MnShareSession bob_copy{bob}; + QVERIFY2(bob_copy.adoptLockedTerms(locked, error), qPrintable(error)); + + // A contribution a reply lists under alice's name is never reserved again + // after a release, however well it funds the share: this wallet goes back + // to the one it built. First, one spending another coin of this wallet + // with its change paid elsewhere. + dialog.releaseCoins(); + MnShareSession planting{dialog.m_session}; + MnShareSession::Contribution planted; + planted.label = sent.label; + planted.inputs = {{QString::fromStdString(other_coin.hash.ToString()), other_coin.n}}; + planted.hasChange = true; + planted.changeAddress = FreshP2PKHAddress(); + planted.changeAmount = 420 * COIN - (400 * COIN + dialog.m_fee_field->value()); + QVERIFY2(planting.addContribution(planted, error), qPrintable(error)); + dialog.handleImportedText(planting.toJsonString()); + QVERIFY(dialog.m_session.hasContribution(planted)); + dialog.releaseCoins(); + dialog.reserveCoins(); + QVERIFY2(dialog.myContribution() != nullptr, qPrintable(dialog.m_error_label->text())); + QVERIFY(!dialog.m_session.hasContribution(planted)); + QVERIFY(dialog.m_session.hasContribution(sent)); + QVERIFY(wallet_model.wallet().isLockedCoin(coin)); + QVERIFY(!wallet_model.wallet().isLockedCoin(other_coin)); + + // One listing a coin twice, with this wallet's own change address + const auto plant_and_reserve_again = [&](const MnShareSession::Contribution& contribution) { + dialog.releaseCoins(); + MnShareSession reply{dialog.m_session}; + QString plant_error; + QVERIFY2(reply.addContribution(contribution, plant_error), qPrintable(plant_error)); + dialog.handleImportedText(reply.toJsonString()); + QVERIFY(dialog.m_session.hasContribution(contribution)); + dialog.releaseCoins(); + dialog.reserveCoins(); + QVERIFY2(dialog.myContribution() != nullptr, qPrintable(dialog.m_error_label->text())); + }; + MnShareSession::Contribution twice{planted}; + const MnShareSession::Input twice_input{QString::fromStdString(twice_coin.hash.ToString()), twice_coin.n}; + twice.inputs = {twice_input, twice_input}; + twice.changeAddress = sent.changeAddress; + twice.changeAmount = 2 * 410 * COIN - (400 * COIN + dialog.m_fee_field->value()); + plant_and_reserve_again(twice); + QVERIFY(!dialog.m_session.hasContribution(twice)); + QVERIFY(dialog.m_session.hasContribution(sent)); + + // One with 100 duffs of change to this wallet, which could never be relayed + MnShareSession::Contribution dust_change{twice}; + dust_change.inputs = {{QString::fromStdString(dust_change_coin.hash.ToString()), dust_change_coin.n}}; + dust_change.changeAmount = 100; + plant_and_reserve_again(dust_change); + QVERIFY(!dialog.m_session.hasContribution(dust_change)); + QVERIFY(dialog.m_session.hasContribution(sent)); + QVERIFY(!wallet_model.wallet().isLockedCoin(dust_change_coin)); + + // One that funds the share exactly, with change to this wallet, but whose + // input has a relative-lock sequence this wallet refuses to sign + MnShareSession::Contribution relative{twice}; + relative.inputs = {{QString::fromStdString(sequence_coin.hash.ToString()), sequence_coin.n, /*sequence=*/1}}; + relative.changeAmount = 405 * COIN - (400 * COIN + dialog.m_fee_field->value()); + plant_and_reserve_again(relative); + QVERIFY(!dialog.m_session.hasContribution(relative)); + QVERIFY(dialog.m_session.hasContribution(sent)); + QVERIFY(!wallet_model.wallet().isLockedCoin(sequence_coin)); + + // Without change, a dust remainder over the share is left to the fee. A + // contribution Reserve built that way is reserved again as it was. + dialog.releaseCoins(); + const std::vector held{coin, other_coin, twice_coin, dust_change_coin, sequence_coin}; + for (const auto& outpoint : held) { + wallet_model.wallet().lockCoin(outpoint, /*write_to_db=*/false); + } + dialog.reserveCoins(); + QVERIFY2(dialog.myContribution() != nullptr, qPrintable(dialog.m_error_label->text())); + const MnShareSession::Contribution no_change{*dialog.myContribution()}; + QVERIFY(!no_change.hasChange); + QCOMPARE(no_change.inputs.size(), size_t{1}); + QCOMPARE(no_change.inputs[0].txid, QString::fromStdString(dust_coin.hash.ToString())); + for (const auto& outpoint : held) { + wallet_model.wallet().unlockCoin(outpoint); + } + dialog.releaseCoins(); + dialog.reserveCoins(); + QVERIFY(dialog.m_session.hasContribution(no_change)); + QVERIFY(wallet_model.wallet().isLockedCoin(dust_coin)); + QVERIFY(!wallet_model.wallet().isLockedCoin(coin)); + + // With a different fee the released coins no longer fund the share as they + // did, so reserving again selects anew and earlier replies are out of date + dialog.releaseCoins(); + dialog.m_fee_field->setValue(dialog.m_fee_field->value() + COIN / 1000); + dialog.reserveCoins(); + QVERIFY2(dialog.myContribution() != nullptr, qPrintable(dialog.m_error_label->text())); + QVERIFY(!dialog.m_session.hasContribution(sent)); + QCOMPARE(dialog.myContribution()->changeAmount, sent.changeAmount - COIN / 1000); + QVERIFY(wallet_model.wallet().isLockedCoin(coin)); + wallet_model.wallet().unlockCoin(coin); +} + void SharedMnWizardTests::coordinatorResumesFullySignedSession() { TestingSetup test{CBaseChainParams::REGTEST}; diff --git a/src/qt/test/sharedmnwizardtests.h b/src/qt/test/sharedmnwizardtests.h index 7c96caad531f..24e961845949 100644 --- a/src/qt/test/sharedmnwizardtests.h +++ b/src/qt/test/sharedmnwizardtests.h @@ -29,6 +29,7 @@ private Q_SLOTS: void participantPagesAndRoleInference(); void participantCannotChooseATakenShare(); void coordinatorResumesSavedSession(); + void reservingAgainKeepsEarlierReplies(); void coordinatorResumesFullySignedSession(); void signedSessionReservation(); void unauthorisedSignedInputsAreDetected(); diff --git a/src/qt/test/util.cpp b/src/qt/test/util.cpp index d91b90fbba29..2646c7be4df2 100644 --- a/src/qt/test/util.cpp +++ b/src/qt/test/util.cpp @@ -13,9 +13,9 @@ #include #include -void ConfirmMessage(QString* text, std::chrono::milliseconds msec) +void ConfirmMessage(QString* text, std::chrono::milliseconds msec, QObject* context) { - QTimer::singleShot(msec, [text]() { + QTimer::singleShot(msec, context, [text]() { for (QWidget* widget : QApplication::topLevelWidgets()) { if (widget->inherits("QMessageBox")) { QMessageBox* messageBox = qobject_cast(widget); diff --git a/src/qt/test/util.h b/src/qt/test/util.h index a1788ca74e61..aa9253c25d6c 100644 --- a/src/qt/test/util.h +++ b/src/qt/test/util.h @@ -10,6 +10,7 @@ #include QT_BEGIN_NAMESPACE +class QObject; class QString; QT_END_NAMESPACE @@ -18,7 +19,9 @@ QT_END_NAMESPACE * * @param text - Optionally store dialog text. * @param msec - Number of milliseconds to pause before triggering the callback. + * @param context - Optionally cancel the callback when this object is destroyed, + * so it cannot answer a later message box or outlive `text`. */ -void ConfirmMessage(QString* text, std::chrono::milliseconds msec); +void ConfirmMessage(QString* text, std::chrono::milliseconds msec, QObject* context = nullptr); #endif // BITCOIN_QT_TEST_UTIL_H diff --git a/src/test/governance_vote_processing_tests.cpp b/src/test/governance_vote_processing_tests.cpp index e2f28c79128e..1b87452778ff 100644 --- a/src/test/governance_vote_processing_tests.cpp +++ b/src/test/governance_vote_processing_tests.cpp @@ -6,6 +6,8 @@ #include #include #include +#include +#include #include #include #include @@ -88,8 +90,8 @@ struct GovernanceVoteSetup : public TestChainSetup { int64_t proposal_time{0}; std::string proposal_payment_address; - GovernanceVoteSetup() : - TestChainSetup(DIP3_ACTIVATION_HEIGHT - 2, CBaseChainParams::REGTEST, {"-dip3params=109:500"}) + explicit GovernanceVoteSetup(const char* dip3_params = "-dip3params=109:500") : + TestChainSetup(DIP3_ACTIVATION_HEIGHT - 2, CBaseChainParams::REGTEST, {dip3_params}) { // A failed BOOST_REQUIRE below throws, and a throwing constructor means no destructor runs. // Tear the globals down by hand so one broken invariant here doesn't leave a live tx index @@ -232,6 +234,73 @@ struct TestGovernanceStore : GovernanceStore { BOOST_FIXTURE_TEST_SUITE(governance_vote_processing_tests, GovernanceVoteSetup) +struct GovernanceReorgSetup : GovernanceVoteSetup { + GovernanceReorgSetup() : + GovernanceVoteSetup("-dip3params=109:109") + { + } +}; + +BOOST_FIXTURE_TEST_CASE(votes_follow_masternode_identity_across_reorg, GovernanceReorgSetup) +{ + auto& chainman = *m_node.chainman; + auto& govman = *m_node.govman; + CKey owner_key, other_owner_key, voting_key; + CBLSSecretKey operator_key, other_operator_key; + voting_key.MakeNewKey(true); + const auto registration = CreateProRegTx(chainman, utxos, 2, payout_script(), coinbaseKey, owner_key, operator_key); + const auto other_registration = CreateProRegTx(chainman, utxos, 3, payout_script(), coinbaseKey, other_owner_key, + other_operator_key); + MineBlock({registration}); + CBlockIndex* fork_block{WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip())}; + const auto old_id = tip_mn_list().GetMN(registration.GetHash())->GetInternalId(); + + CProUpRegTx update; + update.nVersion = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false); + update.proTxHash = registration.GetHash(); + update.pubKeyOperator.Set(operator_key.GetPublicKey(), /*specificLegacyScheme=*/bls::bls_legacy_scheme); + update.keyIDVoting = voting_key.GetPubKey().GetID(); + update.scriptPayout = payout_script(); + CMutableTransaction tx; + tx.nVersion = 3; + tx.nType = TRANSACTION_PROVIDER_UPDATE_REGISTRAR; + const auto spent = FundTransaction(chainman, tx, utxos, payout_script(), COIN); + update.inputsHash = CalcTxInputsHash(CTransaction(tx)); + BOOST_REQUIRE(CHashSigner::SignHash(::SerializeHash(update), owner_key, update.vchSig)); + SetTxPayload(tx, update); + SignTransaction(tx, spent, coinbaseKey); + MineBlock({tx}); + govman.UpdatedBlockTip(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip())); + + const auto proposal = MakeProposal(uint256::ONE); + const auto parent_hash = proposal.GetHash(); + govman.AddGovernanceObjectForTesting(proposal); + auto unchanged_vote = MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES); + SignWithVotingKey(unchanged_vote, mn_voting_key); + CGovernanceVote changed_vote{COutPoint{registration.GetHash(), 0}, parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES}; + SignWithVotingKey(changed_vote, voting_key); + CGovernanceException exception; + uint256 hash_to_request; + BOOST_REQUIRE(govman.ProcessVote(unchanged_vote, exception, hash_to_request)); + BOOST_REQUIRE(govman.ProcessVote(changed_vote, exception, hash_to_request)); + BOOST_REQUIRE(govman.HaveVoteForHash(changed_vote.GetHash())); + + BlockValidationState state; + BOOST_REQUIRE(chainman.ActiveChainstate().InvalidateBlock(state, fork_block)); + MineBlock({other_registration}); + MineBlock({registration}); + BOOST_REQUIRE(old_id != tip_mn_list().GetMN(registration.GetHash())->GetInternalId()); + govman.UpdatedBlockTip(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip())); + + BOOST_CHECK(!govman.HaveVoteForHash(changed_vote.GetHash())); + BOOST_CHECK(govman.GetCurrentVotes(parent_hash, changed_vote.GetMasternodeOutpoint()).empty()); + BOOST_CHECK(govman.HaveVoteForHash(unchanged_vote.GetHash())); + BOOST_CHECK_EQUAL(govman.GetCurrentVotes(parent_hash, mn_collateral).size(), 1U); + const auto stored = govman.FindConstGovernanceObject(parent_hash); + BOOST_REQUIRE(stored != nullptr); + BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 1); +} + // A vote that arrives before its parent object must be kept and replayed once the object shows up, // otherwise every vote that races ahead of its proposal is lost for good. BOOST_AUTO_TEST_CASE(orphan_vote_is_cached_and_applied_when_parent_arrives) diff --git a/src/test/llmq_dkg_tests.cpp b/src/test/llmq_dkg_tests.cpp index 6595cf457a02..d2de8564211d 100644 --- a/src/test/llmq_dkg_tests.cpp +++ b/src/test/llmq_dkg_tests.cpp @@ -60,6 +60,23 @@ BOOST_AUTO_TEST_CASE(pending_messages_own_messages_share_quota_path) BOOST_CHECK_EQUAL(pending.PopPendingMessages(3).size(), 2U); } +BOOST_AUTO_TEST_CASE(pending_messages_own_message_is_processed_first) +{ + using namespace llmq; + + CDKGPendingMessages pending{/*_maxMessagesPerProTx=*/2}; + pending.PushPendingMessage(/*from=*/1, MakeTestHash(0xa1), MakeDKGMessage(), MakeTestHash(1)); + pending.PushPendingMessage(/*from=*/2, MakeTestHash(0xa2), MakeDKGMessage(), MakeTestHash(2)); + pending.PushPendingMessage(/*from=*/-1, MakeTestHash(0xee), MakeDKGMessage(), MakeTestHash(3)); + + const auto msgs = pending.PopPendingMessages(3); + BOOST_REQUIRE_EQUAL(msgs.size(), 3U); + auto it = msgs.begin(); + BOOST_CHECK_EQUAL((it++)->first, -1); + BOOST_CHECK_EQUAL((it++)->first, 1); + BOOST_CHECK_EQUAL((it++)->first, 2); +} + BOOST_AUTO_TEST_CASE(pending_messages_quota_survives_reconnect) { using namespace llmq; diff --git a/src/validation.cpp b/src/validation.cpp index 71a55d70615b..7813be6921b9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2925,6 +2925,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // Block is committed: keep the scheme it switched to (fJustCheck dry runs returned above). bls_scheme_guard.Commit(); + m_chain_helper->special_tx->RemoveMineableCommitmentsForBlock(block, pindex); if (this == &m_chainman.ActiveChainstate() && mnlist_updates.diff.HasChanges()) { GetMainSignals().NotifyMasternodeListChanged(false, mnlist_updates.old_list, mnlist_updates.diff); @@ -5218,6 +5219,7 @@ bool Chainstate::RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& in return false; } RecordBackgroundMNListHash(pindex, mnlist_updates.new_list); + m_chain_helper->special_tx->RemoveMineableCommitmentsForBlock(block, pindex); for (size_t i = 0; i < block.vtx.size(); i++) { const CTransactionRef& tx = block.vtx[i]; diff --git a/test/functional/feature_llmq_dkgerrors.py b/test/functional/feature_llmq_dkgerrors.py index a150a4dfc5b7..4c466279a8ce 100755 --- a/test/functional/feature_llmq_dkgerrors.py +++ b/test/functional/feature_llmq_dkgerrors.py @@ -116,6 +116,17 @@ def test_qc(self): # TODO: test signers # TODO: test validMembers + # The commitment is mineable again after invalidateblock and must stay so when a block that + # carries it is rejected by a check that runs after the commitments were processed + assert self.node_has_quorum_commitment(self.nodes[0], quorumHash, 100) + bad_block = copy.deepcopy(block) + bad_block.vtx[0].vout[0].nValue += 1 + bad_block.vtx[0].rehash() + bad_block.hashMerkleRoot = bad_block.calc_merkle_root() + bad_block.solve() + assert_equal(self.nodes[0].submitblock(bad_block.serialize().hex()), 'bad-cb-amount') + assert self.node_has_quorum_commitment(self.nodes[0], quorumHash, 100) + self.nodes[0].reconsiderblock(best) # Mine 8 (SIGN_HEIGHT_OFFSET) more blocks to make sure that the new quorum gets eligible for signing sessions self.generate(self.nodes[0], 8, sync_fun=lambda: self.sync_blocks(self.nodes)) diff --git a/test/functional/feature_mnehf.py b/test/functional/feature_mnehf.py index 98fa8259f47f..20ef81b40522 100755 --- a/test/functional/feature_mnehf.py +++ b/test/functional/feature_mnehf.py @@ -28,13 +28,14 @@ class MnehfTest(DashTestFramework): def add_options(self, parser): self.add_wallet_options(parser) + parser.add_argument("--spork21", default=False, action="store_true", + help="Test with all quorum members connected") def set_test_params(self): # testdummy starts as NEVER_ACTIVE, run_test opens its signalling window later - extra_args = [["-vbparams=testdummy:-2:999999999999:0:4:4:4:5:1", "-persistmempool=0"]] * 2 - # One masternode is enough: the EHF signal only needs a recovered signature of any quorum - self.set_dash_test_params(2, 1, extra_args=extra_args) - self.set_dash_llmq_test_params(1, 1) + extra_args = [["-vbparams=testdummy:-2:999999999999:0:4:4:4:5:1", "-persistmempool=0"]] * 4 + self.set_dash_test_params(4, 3, extra_args=extra_args) + self.set_dash_llmq_test_params(3, 3) def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -93,6 +94,8 @@ def set_sporks(self): self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", spork_enabled) self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", spork_disabled) self.nodes[0].sporkupdate("SPORK_2_INSTANTSEND_ENABLED", spork_disabled) + if self.options.spork21: + self.nodes[0].sporkupdate("SPORK_21_QUORUM_ALL_CONNECTED", spork_enabled) self.wait_for_sporks_same() def check_fork(self, expected): @@ -125,11 +128,45 @@ def test_no_signing_outside_window(self): self.check_no_ehf_signing("before its start time") self.log.info("Check that masternodes sign testdummy once its start time is reached") + request_id = hash256(ser_string(b"mnhf") + struct.pack("