From 65603d57afb345ec44e5fb22e814d589e7b0b04a Mon Sep 17 00:00:00 2001 From: merge-script Date: Tue, 10 Nov 2026 21:32:39 +0700 Subject: [PATCH 01/10] Merge dashpay/dash#8088: fix: fetch miniupnpc sources from miniupnp.free.fr cba106e5026c7bafefebaf1e1f9d1f3c0e27561b fix(build): fetch miniupnpc sources from miniupnp.free.fr (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented curl: (28) Failed to connect to miniupnp.tuxfamily.org port 443 after 30002 ms: Timeout was reached Fetching miniupnpc-2.2.7.tar.gz from http://dash-depends-sources.s3-website-us-west-2.amazonaws.com/ % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 0 570 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 curl: (22) The requested URL returned error: 404 make[1]: *** [funcs.mk:301: /home/runner/work/dash/dash/depends/sources/download-stamps/.stamp_fetched-miniupnpc-2.2.7-b0c3a27056840fd0ec9328a5a9bac3dc5e0ec6d2e8733349cf577b0aa1e70ac1.hash] Error 22 make[1]: Leaving directory '/home/runner/work/dash/dash/depends' So apparently build with depends without cache can't succeed anymore. ## What was done? Point the package at the author's other mirror, http://miniupnp.free.fr, which serves the tarball with the expected hash. This is what Bitcoin Core did in bitcoin#30151 for the same outage but it had been reverted later by bitcoin core by mistake and never refixed, instead bitcoin core dropped miniupnpc altogether (bitcoin#31130), so there is no later upstream fix to backport. ## How Has This Been Tested? Can be tested by CI on https://github.com/dashpay/dash/pull/7939 when rebased on top of merged this PR ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(if you have permission to set one)_ Top commit has no ACKs. Tree-SHA512: c53eb4eaed3bdeb7d024bba61acf21e79054dbb2356d408397b1c7e4232e0bee3f511201d26f1e87b9b688e996501578253377d355d64f178d1de5ef1bf53426 (cherry picked from commit 3d85a15bafc1bc96515e372770db51a09fb5ecca) --- depends/packages/miniupnpc.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 033dae39439c79d6abee3b93b2009e16ac26adb4 Mon Sep 17 00:00:00 2001 From: merge-script Date: Tue, 10 Nov 2026 22:57:09 -0600 Subject: [PATCH 02/10] Merge dashpay/dash#8052: fix(qt): reserve the same shared masternode contribution again after a release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 01cda37af25029961383752f44e91aeafe1e19f6 fix(qt): reserve the same shared masternode contribution again after a release (pasta) Pull request description: ## Issue being fixed or feature implemented In the Qt shared masternode wizard, if the coordinator releases and re-reserves its coins after a participant has replied, that participant can no longer accept the locked terms. The coordinator gets no warning. GUI QA found this on the `v24.0.0-rc.7` tag (`111a2c533e19`), on regtest: 1. alice coordinates. She reserves her coins on "Your contribution" and saves the invitation, which carries her contribution, change address included. 2. bob opens the invitation and sends his Details. 3. alice goes back to "Your contribution", presses **Release**, pastes bob's Details and presses **Next**. Next reserves her coins again. 4. alice locks the terms and sends the locked terms to bob. **Result:** bob gets "The locked terms do not contain the details you sent." - alice's board shows every reply in, with no "must reply again" warning. - In the same window, bob can't load alice's current invitation either: the copy looks like nothing new to him. - His only way forward is to open the locked terms in a fresh dialog. No funds move and nothing is signed or broadcast, but the participant is stuck with an error that doesn't say what to do. **Why it happens:** the re-reserve in step 3 selects coins afresh, so alice's contribution gets a **new change address**. bob's copy still lists the contribution from the invitation, and `MnShareSession::adoptLockedTerms` compares every contribution it knows. **Why rc.6 completes the same steps:** absorbing bob's reply used to put alice's released contribution back and relock it. That was a separate bug, fixed by #8035, which keeps released contributions out of absorbed replies. With that fixed, the new contribution from the re-reserve is what gets locked. ## What was done? `SharedMnCreateDialog::reserveCoins()` now puts back the most recently released contribution for this share, unchanged, when all of these hold: - Reserve built it in this dialog from this wallet's own coin selection. That's tracked in `m_built_contributions`, compared with `MnShareSession::SameContribution`: inputs in order with their sequences, and the same change; - every input is still one Reserve could select, from the same `fundingCandidates()` list `selectCoins()` now uses: unspent, unlocked and spendable, with no unconfirmed coins the wallet doesn't trust; - the inputs minus the change still equal what the share needs: the share amount, plus the Network fee for the coordinator. A contribution with no change may also exceed it by a dust amount, which `selectCoins()` leaves to the fee in the same way (the shared `isDust()`). Otherwise it selects coins as before. In that case the contribution really did change, for example the fee or the share amount changed, or a coin was spent. Earlier replies are then out of date, as they are today. The "built here" rule matters because of what a reply can contain. A reply can list a contribution under this wallet's share name that spends another of this wallet's coins, with any change, sequences or repeated inputs the sender chose. The wallet already refuses to sign such a contribution, but a release would put it into the released list. A contribution absorbed from a reply is therefore never put back. Reserve builds the wallet's own contribution instead. `fundingCandidates()` is the coin filter `selectCoins()` already used, moved into its own function so that reuse and fresh selection accept the same coins. `reservableAgain()` makes the reuse decision once for both Reserve and the coins card on "Your contribution". While the coins are released, the card says it uses the coins "as reserved before" and that the change "returns to the same address of yours", instead of describing a fresh selection. **Known limits, left as is:** reuse doesn't survive reopening a saved session, because neither the released nor the built list is saved. Also, two changes can cancel out. For example, the coordinator releases, raises the fee and reserves, then releases again and restores the fee. The original contribution was replaced in between, so the second re-reserve can't bring it back and builds a new one. This is rare, and it's the general "the coordinator changed their own funding after replies" case, which needs a reply-again signal on the board rather than reuse. ## How Has This Been Tested? **New scenario:** `SharedMnWizardTests::reservingAgainKeepsEarlierReplies`, with a confirmed coin in a real wallet. - **Main case:** reserve, then take the invitation, then bob's and carol's replies to it. Release, absorb bob's reply and reserve again. - The contribution must match the one bob holds. - After a real `PrepareRegistration` + `freeze`, bob's `adoptLockedTerms` on the locked terms must succeed. - **A reply plants a contribution under alice's name:** the variants are change paid elsewhere, one coin listed twice, 100 duffs of change to this wallet, and an exactly funded input at sequence 1. After Release and Reserve, the wallet must go back to its own contribution each time, and the planted coin must stay unlocked. - **No change, dust remainder:** Reserve itself builds a no-change contribution from a coin 100 duffs over the share plus fee, with the other coins locked. After a release it is reserved again unchanged. - **Coins card:** a larger coin arrives after the first reservation. After Release, the card must describe the contribution Reserve will put back, not the larger coin. - **Fee changed:** after a release and a fee change, reserving again must select afresh. **Fails without the fix:** - With the reuse removed, the scenario fails at the check that the re-reserved contribution is the one sent (`sharedmnwizardtests.cpp:471`). - With reuse allowed for any released contribution, it fails at the first planted case (`:547`). With the card still showing a fresh selection, it fails at `:504` with "Uses 1 coin (600 tDASH); change 199.999 tDASH returns to a new address of yours". **Full `test_dash-qt` with the fix**, run headless (`QT_QPA_PLATFORM=minimal`) on Linux: 164 passed, 0 failed, 2 skipped. The skips are the OptionTests font tests, which can't run on the headless `minimal` platform. SharedMnWizardTests: 22 passed. **GUI on regtest**, run as a 2-party session with the steps above. Each binary was built from its exact commit: - **PR base** `9e5e3d59a1e3` and the **`v24.0.0-rc.7`** tag `111a2c533e19`: the re-reserve picks a new change address, and bob gets "The locked terms do not contain the details you sent." - **PR head at the time,** `eb4929384856`: the change address stays the same, bob gets Approve terms, and the registration confirms. Its change pays the address bob's Details listed. - **Head `496130ce0637`** (since superseded): the same flow was rerun with the same result: unchanged change address, Approve terms, ProRegTx `2db9621d…7834` confirmed. See [that evidence comment](https://github.com/dashpay/dash/pull/8052#issuecomment-6638363938). - **Head `55672100f0b0`:** it adds the coins-card change above. The same flow was rerun on it: unchanged change address, Approve terms, ProRegTx `59ba8be9…5c95` confirmed. After Release, the card reads "as reserved before … the same address of yours", where the base reads "a new address of yours". See [the current-head evidence comment](https://github.com/dashpay/dash/pull/8052#issuecomment-6639545618). - **Current head `01cda37af25`:** after `55672100` it adds only the reuse restriction above, through the untested intermediate `2a1bcc63`. The same flow was rerun on binaries built from exact `01cda37a`: unchanged change address, the "as reserved before … same address" coins card, Approve terms, and the ProRegTx `dcea870f…105d` confirmed. See [the current-head evidence comment](https://github.com/dashpay/dash/pull/8052#issuecomment-6641074891). Screenshots and provenance are in [the UI evidence comment](https://github.com/dashpay/dash/pull/8052#issuecomment-6637437445) and at [dash-ui-artifacts@`034c3d7` / dash/pr-8052](https://github.com/PastaPastaPasta/dash-ui-artifacts/blob/034c3d7dc83b896aca0182126aa748a72b4b1be9/dash/pr-8052/README.md). On the earlier pre-review head `46de5fe1dea`, the same flow also passed. So did a participant releasing and reserving again on the contribution page. A fee change after a reply gives a new contribution, as expected. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(if you have permission to set one)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 0bb7afb3948fd947a73dd5b16f7e1b75ba3e10e650383609509d1220309b6baddb2244f110623c86badf51d7c8adf84dfca36d06482d92988ec7616903661825 (cherry picked from commit b2172a9cf8a35265c517253016a836879fee4ac4) --- src/qt/mnsharesession.cpp | 26 ++-- src/qt/mnsharesession.h | 3 + src/qt/sharedmncreatedialog.cpp | 126 ++++++++++++---- src/qt/sharedmncreatedialog.h | 14 ++ src/qt/test/sharedmnwizardtests.cpp | 223 ++++++++++++++++++++++++++++ src/qt/test/sharedmnwizardtests.h | 1 + 6 files changed, 349 insertions(+), 44 deletions(-) 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/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(); From ec9e944a26b6f1b44a213375bff3dfb907795e66 Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 11 Nov 2026 10:56:59 -0600 Subject: [PATCH 03/10] Merge dashpay/dash#8094: fix(net): stop busy-looping while a peer's send buffer is full MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 19da39cf27d53f3a85935ece4ad9df0b73a4d49a fix(net): wait for socket writability after failed sends (pasta) Pull request description: ## Issue being fixed or feature implemented When a nonblocking `send()` to a peer fails with a would-block error (the peer has stopped reading and its TCP window is full), `CConnman::SocketSendData` leaves `fCanSendData` set. The peer therefore stays in the sendable set, the socket handler keeps polling with a zero timeout, and the net thread busy-loops until the peer starts reading again or times out. Before the transport refactor (bitcoin#28165, merged here in 63962ec47564) a failed send cleared the flag; that reset was lost in the merge. ## What was done? Clear `fCanSendData` whenever the send returns an error, so the next iteration waits for the socket to become writable again. The empty-send path and partial successful sends are unchanged. ## How Has This Been Tested? Reproduced with a functional script that connects a peer which never reads and streams pings at the node: before the change the node's net thread used ~100% of a CPU core (9.86 s of CPU vs 9.88 s for a reference busy loop); with the change it used 0.02-0.03 s over three runs. That script measures CPU time, so it is not added as a CI test. `test_dash --run_test=net_tests,sock_tests`, `p2p_ping.py` and `p2p_v2_transport.py` pass. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(if you have permission to set one)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 0af7f3b48164fae294d81f828cf1d5c25f6cb86a41257dd4c981116a30a8fc5c976be0345f389f03033ec390ecc1c42853bf161350be9f8f14e1f011f63d923d (cherry picked from commit 34f990e8701e4fd26cb702f922f70db03ce47fed) --- src/net.cpp | 1 + 1 file changed, 1 insertion(+) 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; } From b051d3a0b848de7156aeed17345de03ed7bcf4e2 Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 11 Nov 2026 11:08:23 -0600 Subject: [PATCH 04/10] Merge dashpay/dash#8080: test(qt): stop address book tests from answering the shared masternode wizard's prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a5618a0e83abfc809f5feac7c81d61ffda8fe254 test(qt): don't leave address book message answers armed after the edit (pasta) Pull request description: ## Issue being fixed or feature implemented Fixes #8075. With a distribution Qt, `test_dash-qt` fails in `SharedMnWizardTests::signedSessionReservation()`. It either hangs until QtTest's 300 s watchdog fires, or aborts with `free(): invalid size`. Builds against depends' Qt pass. The shared masternode wizard is not at fault. `AddressBookTests` leaves two answers to a message box behind. `EditAddressAndSubmit()` calls `ConfirmMessage(&warning_text, 5ms)` before every edit, but a successful edit opens no box. Its timer then outlives the function, holding a pointer to the function's stack `QString`. The two successful edits run last, so both timers are still pending when the suite ends. No later suite runs an event loop until the wizard's "Keep this session for later?" prompt (added in #8035, which is why the bisect lands there). Whether that prompt survives depends on Qt's event dispatcher: - **Unix dispatcher** (depends builds Qt with `-no-glib`): the test's own "Cancel" ends the modal loop before timers are checked. The stale timers fire later, when no box is left, and do nothing. - **GLib dispatcher** (what Fedora's and other distribution Qt packages use): the timers fire in the same iteration as the test's answer. They write the box's text through the dangling pointer and click its default button, "Save and Close", over the test's "Cancel". The dialog then opens a save-file dialog that nothing answers, which is the hang. When the stray write hits a live frame instead, the result is `free(): invalid size`. The hardening flags in the report are incidental: the same Fedora build passes with `QT_NO_GLIB=1`. Bitcoin Core has the same `EditAddressAndSubmit()`, but no modal test runs after it there. Since #8077, `AddressBookTests` also runs on macOS, so the stale timers exist there too. ## What was done? `ConfirmMessage()` takes an optional context object, and `EditAddressAndSubmit()` scopes it to the edit, so a box that never opens leaves nothing armed. The existing address book scenario now also opens a message box after the successful edits and checks that its own answer wins. No production code changes. The dialog did what the stale click told it to do. ## How Has This Been Tested? Reproduced in a Fedora 44 container (GCC 16.2.1, system Qt 5.15.18) with the exact `CFLAGS`/`CXXFLAGS`/`LDFLAGS`/`configure` line from the issue. Results are for the full `test_dash-qt`. The "develop" column is develop at 4c5c0153500, and the "This PR" column is this PR on 8dbdcb4a5b9. | Build | Dispatcher | develop | This PR | |---|---|---|---| | Fedora RPM flags | GLib (default) | hangs in `signedSessionReservation` | 163 passed, 0 failed | | Fedora RPM flags | Unix (`QT_NO_GLIB=1`) | 163 passed | 163 passed, 0 failed | | ASan+UBSan, `detect_stack_use_after_return=1` | GLib | **stack-use-after-return** in the `ConfirmMessage` lambda (`qt/test/util.cpp:22`), inside the wizard prompt's `exec()` | 163 passed, no ASan report | | ASan+UBSan | Unix | passes | 163 passed, no ASan report | | Default depends build, `--enable-werror` | Unix | passes | 163 passed, 0 failed | | Fedora RPM flags under valgrind memcheck, `QT_ENABLE_REGEXP_JIT=0` | GLib | n/a (hangs) | 163 passed, 0 errors | - A gdb breakpoint on `QMessageBox::buttonClicked` during the hang showed the test's "Cancel" click, then two clicks from `ConfirmMessage`'s lambda in the same `exec()`. - With `scope` replaced by `nullptr`, the new address book check fails with either dispatcher: "Compared pointers are not the same", or a crash in the stale callback at `util.cpp:22`. - UBSan reports the existing misaligned store in the vendored X11 code (`crypto/x11/sph_types.h:613`), with and without this PR. On this PR's base it also reports one invalid `bool` load at `qt/transactionrecord.h:70`, in code this PR does not touch: `decomposeTransaction()` copies a record into its list before setting its uninitialised `involvesWatchAddress`. With Qt's regexp JIT left on, valgrind's only reports are uninitialised-value warnings in JIT-generated code. With the JIT off there are none. - clang-format-diff (18.1.8) and `lint-whitespace.py` are clean. ## Breaking Changes None. Test code only. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: d68bacc7990c68c1966cf0d020795d7638b29b9f49d5bc42620189fffa3afb36585bcbab9653f95bccfbda1f9dcf8d0fd05ea7e2cd3c3955f762230c186e4b11 (cherry picked from commit 6cedd349cb7855bbcb059f0c0ba92172964e4769) --- src/qt/test/addressbooktests.cpp | 13 ++++++++++++- src/qt/test/util.cpp | 4 ++-- src/qt/test/util.h | 5 ++++- 3 files changed, 18 insertions(+), 4 deletions(-) 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/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 From d00c4e669a147f12ad8f0f139242ca0d0cc6ebb0 Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 11 Nov 2026 13:18:02 -0600 Subject: [PATCH 05/10] Merge dashpay/dash#8104: fix: keep final commitments mineable when their block is rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 393695393c4f6ca1ad2b82272f6118ba5bd4a936 fix: keep final commitments mineable when their block is rejected (pasta) Pull request description: ## Issue being fixed or feature implemented `CQuorumBlockProcessor::ProcessCommitment()` removed a block's final commitments from the mineable pool while the block was still being validated. `ConnectBlock()` processes quorum commitments early, before inputs, scripts, the coinbase amount/payee checks and several special-transaction checks. A block carrying the commitment of a finished DKG that then failed one of those later checks was never connected, yet every node that validated it (compact blocks are relayed before validation) had dropped the commitment from its pool. Nothing re-adds it, so that quorum was not mined in that DKG cycle. ## What was done? - `ProcessCommitment()` no longer touches the mineable pool, apart from dropping its caches. - New `CQuorumBlockProcessor::RemoveMineableCommitmentsForBlock()` removes the same entries as before, called once `ConnectBlock()` has accepted the block (next to where the BLS scheme switch is committed) and in `RollforwardBlock()`. - Check-only validation (`fJustCheck` / `TestBlockValidity`) returns before that point and never touches the pool. `UndoBlock()` still re-adds the commitments of a disconnected block, so reorgs behave as before. Re-adding on the failure path instead was rejected: a block can fail partway through its commitments, which could re-pool a commitment that was never verified. This doesn't overlap with #8048, which changes `AddMineableCommitment` only. ## How Has This Been Tested? `feature_llmq_dkgerrors.py`: after the existing `invalidateblock` check, the test submits the same commitment-carrying block with the coinbase paying 1 duff too much, expects `bad-cb-amount`, and asserts the commitment is still listed as mineable in `quorum dkgstatus`. It fails on `develop` and passed three times with this change. A five-node reproduction also fails on `develop`; with the fix every node keeps the commitment and the quorum is mined within its window. `llmq_*`, `evo_*` and `validation_chainstatemanager_tests` unit tests pass, as do `feature_llmq_rotation.py`, `feature_llmq_is_cl_conflicts.py`, `feature_llmq_evo.py`, `feature_llmq_signing.py`, `feature_llmq_simplepose.py`, `feature_llmq_singlenode.py`, `feature_llmq_dkg_intake.py`, `feature_dip4_coinbasemerkleroots.py`, `feature_quorum_proof_chain.py`, `rpc_quorum.py`, `feature_new_quorum_type_activation.py` and `feature_abortnode.py`. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(if you have permission to set one)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: a306a6fbf6613ca25dfce8665815c94833ebeb012642f492db882ea7154ac1a987bfe5c0afbebcc7d09762fcf8373e664ea3bb2a82e0c5921d87723426e2f4b6 v24.0.x adaptations: Chainstate has no m_special_tx member here, so ConnectBlock() and RollforwardBlock() call the processor through m_chain_helper->special_tx like the neighbouring special-tx calls. CQuorumBlockProcessor keeps the v24.0.x GetCommitmentsFromBlock(), and RemoveMineableCommitmentsForBlock()'s lock annotation drops !m_commitment_payload_mutex, which only exists with #7936. (cherry picked from commit ce9ef663c9e7fbd3d52e4f8a3d7615cac5759f42) --- src/evo/specialtxman.cpp | 5 ++++ src/evo/specialtxman.h | 2 ++ src/llmq/blockprocessor.cpp | 28 ++++++++++++++++------- src/llmq/blockprocessor.h | 3 +++ src/validation.cpp | 2 ++ test/functional/feature_llmq_dkgerrors.py | 11 +++++++++ 6 files changed, 43 insertions(+), 8 deletions(-) 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/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/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)) From a79d139bd14eab96b3e5b8ad2bc4c608f0f3362f Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 11 Nov 2026 02:22:03 +0000 Subject: [PATCH 06/10] fix: match governance voting keys by registration hash across reorgs Resolve updated masternodes in the destination list and match their previous entries by registration hash. The lookups cannot fail for a diff built from these two lists, so guard them with Assume and skip the entry rather than aborting. Co-Authored-By: GPT 6.1 Sol Co-Authored-By: Claude Opus 5.5 (cherry picked from commit a1a84a0bd55a3ca9737048ab3113c297b7ac0dcb) --- src/governance/governance.cpp | 18 +++-- src/test/governance_vote_processing_tests.cpp | 73 ++++++++++++++++++- 2 files changed, 82 insertions(+), 9 deletions(-) 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/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) From 9f3981e7c5c2e08277185677c44e7823c91ba510 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 11 Nov 2026 02:26:05 +0000 Subject: [PATCH 07/10] fix: skip undecoded provider payloads in transaction JSON Unsupported provider transaction versions can deserialize without network information. Omit their optional decoded payload objects instead of letting JSON rendering throw through the REST block handler, and retain the raw payload bytes. Co-Authored-By: GPT 6.1 Sol (cherry picked from commit 9560439a567d12183d95d1b0a94ecf0f47053d2a) --- src/core_write.cpp | 4 +-- test/functional/interface_rest.py | 45 +++++++++++++++++++++++++++++++ test/functional/rpc_deprecated.py | 10 ++++--- 3 files changed, 53 insertions(+), 6 deletions(-) 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/test/functional/interface_rest.py b/test/functional/interface_rest.py index 0e381fdd4dab..f5fe8d2c1ea9 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -12,10 +12,16 @@ import urllib.parse +from test_framework.blocktools import create_block, create_coinbase from test_framework.messages import ( BLOCK_HEADER_SIZE, COIN, + COutPoint, + CTransaction, + CTxIn, + CTxOut, ) +from test_framework.script import CScript, OP_TRUE from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -449,5 +455,44 @@ def run_test(self): self.sync_all() assert_equal(self.test_rest_request(f"/deploymentinfo/{header_only_hash}"), self.nodes[0].getdeploymentinfo(header_only_hash)) + self.log.info("Test rendering stored ProTx payloads with unsupported versions") + node = self.nodes[0] + tip = node.getbestblockhash() + protxes = [] + for tx_type in (1, 2): + for version in (0, 65535): + tx = CTransaction() + tx.nVersion = 3 + tx.nType = tx_type + tx.vin = [CTxIn(COutPoint(0x1234, len(protxes)))] + tx.vout = [CTxOut(1000, CScript([OP_TRUE]))] + tx.vExtraPayload = version.to_bytes(2, "little") + tx.calc_sha256() + protxes.append(tx) + block = create_block( + tmpl=node.getblocktemplate(), + coinbase=create_coinbase(node.getblockcount() + 1, dip4_activated=True, v20_activated=True), + txlist=protxes, + ) + block.solve() + assert_equal(node.submitblock(block.serialize().hex()), "bad-protx-version") + assert_equal(node.getblock(block.hash, 0), block.serialize().hex()) + + for deprecated_service in (False, True): + if deprecated_service: + self.restart_node(0, extra_args=self.extra_args[0] + ["-deprecatedrpc=service"]) + self.test_rest_request(f"/block/notxdetails/{block.hash}") + block_json = self.test_rest_request(f"/block/{block.hash}") + assert_equal(block_json, node.getblock(block.hash, 2)) + assert_equal(len(block_json["tx"]), len(block.vtx)) + for tx, tx_json in zip(protxes, block_json["tx"][1:]): + field = "proRegTx" if tx.nType == 1 else "proUpServTx" + for decoded in (tx_json, node.decoderawtransaction(tx.serialize().hex()), + node.getrawtransaction(tx.hash, True, block.hash)): + assert field not in decoded + assert_equal(decoded["extraPayload"], tx.vExtraPayload.hex()) + assert_equal(node.getbestblockhash(), tip) + assert_equal(self.test_rest_request("/chaininfo")["bestblockhash"], tip) + if __name__ == '__main__': RESTTest().main() diff --git a/test/functional/rpc_deprecated.py b/test/functional/rpc_deprecated.py index 7b5226591c43..e702f543109b 100755 --- a/test/functional/rpc_deprecated.py +++ b/test/functional/rpc_deprecated.py @@ -63,11 +63,13 @@ def test_malformed_deprecated_service_rawtransactions(self): self.log.info("Test malformed provider payloads do not crash -deprecatedrpc=service") node = self.nodes[2] block_count = node.getblockcount() - for tx_hex in [ - "03000100000000000000020000", # ProRegTx with only nVersion=0 payload - "03000200000000000000020000", # ProUpServTx with only nVersion=0 payload + for tx_hex, payload_field in [ + ("03000100000000000000020000", "proRegTx"), # ProRegTx with only nVersion=0 payload + ("03000200000000000000020000", "proUpServTx"), # ProUpServTx with only nVersion=0 payload ]: - assert_raises_rpc_error(-1, "Internal bug detected: obj.netInfo", node.decoderawtransaction, tx_hex) + decoded = node.decoderawtransaction(tx_hex) + assert payload_field not in decoded + assert_equal(decoded["extraPayload"], "0000") assert_equal(node.getblockcount(), block_count) if __name__ == '__main__': From 56e2196bffb500f2e424e2b814a25a764f41c61d Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 11 Nov 2026 18:26:57 -0600 Subject: [PATCH 08/10] fix(llmq): retry pending EHF signals on new tips Allow members to re-sign pending EHF signals after incomplete signing sessions expire. Exercise recovery with the same quorum after isolation and timeout in both connection modes. (cherry picked from commit b619a4f9c7762b037be3b331cb7aad59a28b30ca) --- src/llmq/ehf_signals.cpp | 2 +- test/functional/feature_mnehf.py | 61 +++++++++++++++++++++++++------- test/functional/test_runner.py | 1 + 3 files changed, 51 insertions(+), 13 deletions(-) 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/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(" Date: Thu, 12 Nov 2026 15:08:19 +0700 Subject: [PATCH 09/10] perf(llmq): keep verified DKG contributions cached for the session At commit time GetVerifiedContributions re-read and decoded every member's verification vector from LevelDB, although the session still held them in memory. Fill the cache when a verified contribution is written and keep an entry while its vector is still referenced. The commit step for 400 members and threshold 340 goes from 5.0 s to 0.2 s. (cherry picked from commit 898351d69be6ea13e4b8b34e3e9aa7b05083524c) --- src/llmq/dkgsessionmgr.cpp | 22 +++++++++++++++------- src/llmq/dkgsessionmgr.h | 6 ++++-- 2 files changed, 19 insertions(+), 9 deletions(-) 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, From 8d2bc98eea780f8c7fe9c77638c996ddaab73606 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 11 Nov 2026 21:30:44 -0600 Subject: [PATCH 10/10] fix(llmq): process our own DKG messages ahead of queued peer messages A node's own contribution, complaint, justification and premature commitment went to the back of the pending queue, so they were only verified and relayed after every peer message already waiting. Verifying a premature commitment of a 400-member quorum takes a noticeable fraction of a second, so on a busy or slow node a backlog could hold its own commitment back for much of a short commit phase. Put messages this node produced at the front of the queue. Co-Authored-By: Claude Fable 5.2 (cherry picked from commit 0c3ba643246299b77d3b2e98b5511e9f63d1663a) --- src/llmq/dkgsessionhandler.cpp | 7 ++++++- src/llmq/dkgsessionhandler.h | 3 ++- src/test/llmq_dkg_tests.cpp | 17 +++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) 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/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;