Repository navigation
backport: v24.0.x backports for v24.0.0-rc.3 - #7808
Merged
PastaPastaPasta merged 17 commits intoOct 7, 2026
Merged
Conversation
…ure_mnehf, feature_notifications, p2p_instantsend e308b31 test: use a single-member quorum in p2p_instantsend (Konstantin Akimov) 233a89b test: use a single-member quorum in feature_notifications (Konstantin Akimov) 49e09b9 test: use a single-member quorum in feature_mnehf (Konstantin Akimov) 2e6e5bb test: make mine_quorum_single_member usable right after setup (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented The ChainLock and InstantSend Notifications, P2P InstantSend and EHF feature tests only need the locks & ehf to happen, not a multi-member quorum. ## What was done? - fix: make mine_quorum_single_member usable right after setup - fix: stall of single-member quorum due to mempool's sync - use a single-member quorum in feature_mnehf - use a single-member quorum in feature_notifications - use a single-member quorum in p2p_instantsend - moved testing of node's connection after restart to feature_llmq_connections ## How Has This Been Tested? CI run with tsan: https://github.com/dashpay/dash/actions/runs/36907697165/job/110532135383?pr=7778 https://github.com/dashpay/dash/actions/runs/36752482052/job/110043834012 | Test | develop (s) | PR (s) | |-----------------------------|-------------|--------| | feature_llmq_connections.py | 142 | 157 | | feature_mnehf.py | 102 | 74 | | feature_notifications.py | 44 | 31 | | p2p_instantsend.py | 100 | 56 | Local run: | Test | develop (s) | PR (s) | |-----------------------------|-------------|--------| | feature_llmq_connections.py | 80 | 88 | | feature_mnehf.py | 49 | 41 | | feature_notifications.py | 29 | 21 | | p2p_instantsend.py | 81 | 42 | | **ALL (accumulated)** | **239** | **192** | ## 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 Top commit has no ACKs. Tree-SHA512: 03de5bb1bbeb525848a5d0079f31e89f2906e1fde0606f5a8af9d30bb959d737bdb98ed0fe74261ceab909488b6b96cc512557aeb150105e1dce909c65f7f9a0 (cherry picked from commit 8819507)
…Max peers aaa5301 fix(governance): request object votes from nPeersPerHashMax peers (pasta) Pull request description: ## Issue being fixed or feature implemented `SyncManager::RequestGovernanceObjectVotes` is meant to ask up to `nPeersPerHashMax` distinct peers (3 on real networks, unlimited on regtest) for the votes of each governance object, at most once per peer per hour. When the per-object filter was moved into `CGovernanceManager::FetchGovernanceObjectVotes` (77faacc, first released in v23.1.0), the caller started passing `nMaxObjRequestsPerNode` as the helper's `nPeersPerHashMax` argument. The two values are unrelated: one is how many objects to request from a single peer per call, the other is how many peers to ask about a single object. On mainnet `nMaxObjRequestsPerNode` is hard-coded to 1. So after one peer has been asked for an object's votes, the object is dropped from the candidate list for every other peer until the 1 hour `mapAskedRecently` entry expires. During initial governance sync each object's votes are therefore requested from exactly one peer instead of up to three. On testnet and devnets the bound becomes `2000 / enabled masternodes`, which can be larger than 3. Objects that already have 3 asked peers are then re-selected, and that only costs some extra requests. ### Why this is a real problem Code path at c14104b: - [`src/node/sync_manager.cpp#L55`](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/sync_manager.cpp#L55) and [`#L66`](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/sync_manager.cpp#L66): `nPeersPerHashMax` is 3 on real networks. - [`src/node/sync_manager.cpp#L74`](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/sync_manager.cpp#L74): `nMaxObjRequestsPerNode` is 1 on mainnet. - [`src/node/sync_manager.cpp#L86`](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/sync_manager.cpp#L86): `FetchGovernanceObjectVotes(nMaxObjRequestsPerNode, nNow, mapAskedRecently)`. - [`src/governance/governance.cpp#L942`](https://github.com/dashpay/dash/blob/c14104b172a2/src/governance/governance.cpp#L942) and [`#L954`](https://github.com/dashpay/dash/blob/c14104b172a2/src/governance/governance.cpp#L954): the parameter is named `nPeersPerHashMax`, and an object is skipped when `asked_peers.size() >= nPeersPerHashMax`. - [`src/node/sync_manager.cpp#L122`](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/sync_manager.cpp#L122): the inner loop still stops at `nPeersPerHashMax`, but it never gets the chance on mainnet, because the outer filter has already removed the object. - [`src/node/sync_manager.cpp#L297-L298`](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/sync_manager.cpp#L297-L298): initial sync calls `RequestGovernanceObjectVotes` with one peer at a time, so the per-object filter is what decides whether a second and third peer are asked. Before 77faacc the inline loop compared against `nPeersPerHashMax`. The refactor passed the other variable. To show the behaviour directly, I injected a temporary, uncommitted test case into `src/test/governance_inv_tests.cpp`. It uses the existing mainnet `GovernanceInvSetup` fixture, adds one governance object and four connected peers, and calls the real `SyncManager::RequestGovernanceObjectVotes` once per peer, the same way the initial-sync loop does. It then counts the `MNGOVERNANCESYNC` per-object vote requests queued to each peer. The expected result is one request each to peers 1 to 3 and none to peer 4. <details><summary>Unfixed (c14104b): only the first peer is ever asked</summary> ``` $ src/test/test_dash --run_test=governance_inv_tests/object_votes_requested_from_up_to_three_peers --log_level=message Running 1 test case... peer=1 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1 peer=2 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0 test/governance_inv_tests.cpp:599: error: in "governance_inv_tests/object_votes_requested_from_up_to_three_peers": peer=2 was asked for the object's votes 0 time(s), expected 1 peer=3 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0 test/governance_inv_tests.cpp:599: error: in "governance_inv_tests/object_votes_requested_from_up_to_three_peers": peer=3 was asked for the object's votes 0 time(s), expected 1 peer=4 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0 *** 2 failures are detected in the test module "Dash Core Test Suite" exit=201 ``` </details> <details><summary>With this PR: three peers are asked, the fourth is not</summary> ``` $ src/test/test_dash --run_test=governance_inv_tests/object_votes_requested_from_up_to_three_peers --log_level=message Running 1 test case... peer=1 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1 peer=2 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1 peer=3 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1 peer=4 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0 *** No errors detected exit=0 ``` </details> ### Why it matters On mainnet the vote sync for each governance object has no redundancy. If the single peer picked for an object is slow, lagging or does not answer, the node's vote tally for that object stays incomplete. It stays that way until the hourly re-request after sync, or until the missing votes arrive through normal inventory relay. This affects nodes doing initial governance sync, such as fresh nodes and nodes restarting after a long sleep. There is no consensus or funds impact. The effect is slower and less reliable convergence of governance vote tallies. The regression has shipped since v23.1.0. ## What was done? Pass `nPeersPerHashMax` instead of `nMaxObjRequestsPerNode` as the first argument of `FetchGovernanceObjectVotes` in `SyncManager::RequestGovernanceObjectVotes`. ### Why this is the correct minimal fix The callee's parameter is the per-object peer cap, and the caller already computes the correct value (`nPeersPerHashMax`) and uses it for the matching inner-loop break. Passing it restores the pre-77faacce semantics exactly: an object stays a candidate until 3 distinct peers (unlimited on regtest) have been asked within the last hour. `nMaxObjRequestsPerNode` keeps its own role as the bound of the per-call object loop, so the number of objects requested from each peer per call does not change. No other caller of `FetchGovernanceObjectVotes` exists. A strong type for the argument, or a rename, would stop this kind of mix-up in the future, but both are refactors beyond restoring the intended behaviour. ## How Has This Been Tested? - Reproduction above: a temporary test case in `src/test/governance_inv_tests.cpp` that calls `SyncManager::RequestGovernanceObjectVotes` per peer on the mainnet fixture. It fails on the unfixed tree (peers 2 and 3 never asked) and passes with this change (peers 1 to 3 asked once, peer 4 not asked). It is not part of this PR. `RequestGovernanceObjectVotes` is a private member, and the harness reached it with an explicit-instantiation access trick, which is not acceptable in a committed test. The regtest functional tests cannot observe the bug, because on mockable chains the cap is effectively unlimited both before and after the fix (2000 / MN count vs `SIZE_MAX`). - Unit tests, all passing: `src/test/test_dash --run_test=<suite>` for `governance_inv_tests`, `governance_vote_sync_tests`, `governance_vote_processing_tests`, `governance_vote_wire_tests`, `governance_superblock_tests`, `governance_validators_tests`. - Functional tests, all passing: `test/functional/test_runner.py feature_governance.py feature_governance_cl.py feature_governance_objects.py p2p_govsync_bloom.py`. In the first parallel run, `feature_governance.py` hit an RPC timeout while the host load average was about 100. It passed when rerun on its own. - `clang-format` on the changed lines produces no changes; `test/lint/lint-whitespace.py` passes. Environment: macOS arm64, debug build (`--enable-debug --enable-werror`). ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: eb194251f30e68eb8bcd0ed647d48b086337c32e02fae968bbb057083cd5e8d4ac8012b6d11de441a7511d78b91f34dc2f299a7ebee35bfc4696770fe4744013 (cherry picked from commit a8b44a1)
…kgstatus 9581df5 fix(rpc): use a consistent tip height in quorum dkgstatus (pasta) Pull request description: ## Issue being fixed or feature implemented `quorum dkgstatus` can abort `dashd` if the chain tip is disconnected while the RPC is running. The RPC reads the tip once under `cs_main` and drops the lock ([quorums.cpp#L371](https://github.com/dashpay/dash/blob/c14104b172a2/src/rpc/quorums.cpp#L371), [#L373](https://github.com/dashpay/dash/blob/c14104b172a2/src/rpc/quorums.cpp#L373)). For each quorum type it then takes `cs_main` again and calls `GetMineableCommitments(llmq_params, tipHeight + 1)` with that earlier height ([#L429-L430](https://github.com/dashpay/dash/blob/c14104b172a2/src/rpc/quorums.cpp#L429-L430)). `GetMineableCommitments` → `GetNumCommitmentsRequired` → `IsMiningPhase` starts with `assert(nHeight <= active_chain.Height() + 1)` ([blockprocessor.cpp#L889](https://github.com/dashpay/dash/blob/c14104b172a2/src/llmq/blockprocessor.cpp#L889), [#L610](https://github.com/dashpay/dash/blob/c14104b172a2/src/llmq/blockprocessor.cpp#L610), [#L317](https://github.com/dashpay/dash/blob/c14104b172a2/src/llmq/blockprocessor.cpp#L317)). That assert runs on every call, not only inside the mining window. If even one block is disconnected between the two lock scopes, `tipHeight + 1` is greater than `Height() + 1` and the process aborts. The chain can shrink while `cs_main` is released: `invalidateblock`, a reorg that disconnects before it connects, or ChainLock enforcement, which disconnects the conflicting blocks in `EnforceBlock` and reconnects later in a separate `ActivateBestChain` call. Before dashpay#7770 the RPC passed `tipHeight`, so it took a two-block rollback to hit the assert. dashpay#7770 changed it to `tipHeight + 1`, and now a one-block rollback is enough. A related problem sits in the same function, on the masternode-only path: `WITH_LOCK(cs_main, return chainman.ActiveChain()[quorumHeight])` re-reads the active chain ([#L390](https://github.com/dashpay/dash/blob/c14104b172a2/src/rpc/quorums.cpp#L390)). That returns `nullptr` if the chain has dropped below `quorumHeight`, and the result is dereferenced on the next line. ### Why this is a real problem I reproduced the abort on an unmodified `c14104b172a2` debug build with a functional-test-style script. It starts one regtest node with 200 blocks. Six RPC threads call `quorum dkgstatus` in a loop while one thread runs `invalidateblock <tip>` / `reconsiderblock <tip>`. No masternode is needed: the `GetMineableCommitments` call runs on every node. The node aborts within 0.2 s every time (4 of 4 runs). With this PR, the same script runs for 120 s (816,452 `dkgstatus` calls against 44,361 rollbacks) and the node stays up. <details><summary>Reproduction script and output</summary> ```python #!/usr/bin/env python3 import os, sys, threading, time sys.path.insert(0, os.path.join(os.environ["DASH_WORKTREE"], "test", "functional")) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import get_rpc_proxy DURATION = int(os.environ.get("REPRO_SECONDS", "120")) class DkgStatusRollbackRace(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True self.extra_args = [["-rpcthreads=16", "-rpcworkqueue=64"]] def run_test(self): node = self.nodes[0] self.generate(node, 200, sync_fun=self.no_op) stop = threading.Event() counts = {"dkgstatus": 0, "rollbacks": 0} errors = [] def dkg_worker(): rpc = get_rpc_proxy(node.url, 0, timeout=30) while not stop.is_set(): try: rpc.quorum("dkgstatus"); counts["dkgstatus"] += 1 except Exception as e: errors.append(repr(e)); stop.set() def rollback_worker(): rpc = get_rpc_proxy(node.url, 0, timeout=30) while not stop.is_set(): try: tip = rpc.getbestblockhash() rpc.invalidateblock(tip); rpc.reconsiderblock(tip) counts["rollbacks"] += 1 except Exception as e: errors.append(repr(e)); stop.set() threads = [threading.Thread(target=dkg_worker) for _ in range(6)] + [threading.Thread(target=rollback_worker)] start = time.time() for t in threads: t.start() while not stop.is_set() and time.time() - start < DURATION and node.process.poll() is None: time.sleep(0.2) stop.set() for t in threads: t.join() self.log.info("ran %.1fs: %d dkgstatus calls, %d invalidate/reconsider rounds", time.time() - start, counts["dkgstatus"], counts["rollbacks"]) # (the full script also reports dashd's exit code and stderr, then fails if dashd exited) ... if __name__ == "__main__": DkgStatusRollbackRace().main() ``` Unfixed (`c14104b172a2`): ``` $ DASH_WORKTREE=$PWD REPRO_SECONDS=120 python3 repro_dkgstatus_rollback.py --configfile=test/config.ini --portseed=3303 TestFramework (INFO): tip height 200, enabled dkgstatus types: 4 TestFramework (INFO): ran 0.2s: 177 dkgstatus calls, 8 invalidate/reconsider rounds TestFramework (ERROR): RESULT: dashd EXITED with code -6 after 0.2s TestFramework (ERROR): dashd stderr: Assertion failure: TestFramework (ERROR): dashd stderr: assertion: nHeight <= active_chain.Height() + 1 TestFramework (ERROR): dashd stderr: Assertion failed: (nHeight <= active_chain.Height() + 1), function IsMiningPhase, file blockprocessor.cpp, line 317. TestFramework (ERROR): first RPC error: RemoteDisconnected('Remote end closed connection without response') ``` Three more unfixed runs: ``` ran 0.2s: 299 dkgstatus calls, 16 invalidate/reconsider rounds -> dashd EXITED with code -6 (nHeight <= active_chain.Height() + 1) ran 0.2s: 45 dkgstatus calls, 2 invalidate/reconsider rounds -> dashd EXITED with code -6 (nHeight <= active_chain.Height() + 1) ran 0.2s: 150 dkgstatus calls, 8 invalidate/reconsider rounds -> dashd EXITED with code -6 (nHeight <= active_chain.Height() + 1) ``` With this PR: ``` TestFramework (INFO): tip height 200, enabled dkgstatus types: 4 TestFramework (INFO): ran 120.2s: 816452 dkgstatus calls, 44361 invalidate/reconsider rounds TestFramework (INFO): RESULT: dashd still running, dkgstatus answered every call TestFramework (INFO): Tests successful ``` </details> ### Why it matters `dkgstatus` is what operators and monitoring tools poll on masternodes. If a poll happens to overlap a tip disconnect, the node aborts. A crashed masternode can then collect PoSe penalties. The trigger needs RPC access plus a rollback at the wrong moment, so this is a local reliability bug, not something a remote peer can trigger on demand. Funds and consensus are not affected. ## What was done? Two changes in `quorum_dkgstatus` in `src/rpc/quorums.cpp`: - The height passed to `GetMineableCommitments` is now `chainman.ActiveChain().Height() + 1`, read inside the same `LOCK(cs_main)` as the call. This keeps the "next block after the tip" meaning from dashpay#7770 and guarantees `nHeight <= Height() + 1` when `IsMiningPhase` checks it. - The quorum base block is now found with `pindexTip->GetAncestor(quorumHeight)` instead of `ActiveChain()[quorumHeight]`. Since `quorumHeight <= tipHeight`, this never returns `nullptr`. It also matches the tip the rest of the loop already uses for `GetEnabledQuorumTypes` / `IsQuorumRotationEnabled`, and it removes one `cs_main` acquisition. ### Why this is the correct minimal fix The root cause is that the RPC mixes a height read under one `cs_main` scope with a chain read under another. Both changes make each lookup consistent with the chain state it is checked against, and nothing else changes. Holding `cs_main` across the whole RPC would also work, but `GetQuorumConnections` and `connman.ForEachNode` run in that loop, and holding the lock across them is unnecessary. Clamping or skipping in the RPC would leave the stale height in place. The asserts in `IsMiningPhase` / `GetNumCommitmentsRequired` / `GetMineableCommitments` are deliberately left alone: they protect block creation and validation, where the height always comes from the chain under the same lock. Output is unchanged when the chain does not move during the call. If the chain grows during the call, `minableCommitments` now describes the block after the current tip instead of the one after the earlier sample. That is what the field is documented to mean. This fixes a regression from dashpay#7770, which shipped in v24.0.0-rc.2, so it should also go to the v24.0.x branch before the final release. ## How Has This Been Tested? macOS arm64, debug build (`--enable-debug`, assertions on). - Ran the reproduction script above against unfixed `c14104b172a2`: dashd aborted on the `IsMiningPhase` assert within 0.2 s in 4 of 4 runs. - Ran the same script with this PR: 120 s, 816,452 calls, 44,361 rollbacks, no abort. A second 30 s run after clang-format: 208,389 calls, 11,386 rollbacks, no abort. - `test/functional/test_runner.py feature_llmq_simplepose.py feature_llmq_rotation.py feature_llmq_connections.py`: all passed, including `feature_llmq_simplepose.py --disable-spork23`. These tests call `quorum dkgstatus` on masternodes through the framework's quorum-connection and probe helpers, so they exercise the `GetAncestor` path and the `minableCommitments` path. - `clang-format-diff.py` on the change is clean. No regression test is added. The bug is a race between two RPC threads, and catching it needs a multi-threaded stress loop. Such a test can only show the bug on a lucky interleaving, and it would add run time to the suite for a two-line change. The script above is how the fix was verified. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: f11c5ebd5887ade5e051ec5b95e2c3336f2fa5d891b58d38f1233fe9eba7393cd28184b91d40391c40c0d9b19c20a9d8e1c9208983e0dc6096ebb994262fd9f8 (cherry picked from commit 8ad2c07)
…ead of aborting at startup cdeadac fix(qt): report malformed lang/font settings instead of aborting at startup (pasta) Pull request description: ## Issue being fixed or feature implemented If `settings.json` contains a `"lang"`, `"font-family"`, `"font-scale"`, `"font-weight-normal"` or `"font-weight-bold"` value of an unexpected JSON type (an object or an array, e.g. after a hand edit or a bad merge of the file), dash-qt is terminated at startup by an uncaught exception (`std::terminate`, exit 134). It should instead show the existing "Could not read setting ... Settings file ... might be corrupt or invalid." error, which is what already happens for the same mistake in `prune`, `proxy`, `onion`, `dbcache` and the other settings handled by the `getOption()` loop in `OptionsModel::Init`. The conversion helpers (`SettingToString` / `SettingToInt`) fall back to `UniValue::get_str()` for anything that is not null, a bool or a number, so an object or array throws `UniValue::type_error`. `OptionsModel::Init` already catches that for the settings read in its `getOption()` loop (added with bitcoin-core/gui#602), but a few reads happen outside that loop: - `"lang"`, read at the top of `OptionsModel::Init` (same as upstream), and also read earlier through `gArgs.GetArg("-lang")` in `GetLangTerritory()`, which is called again right after `common::InitConfig` has loaded `settings.json`. - The Dash font settings (`font-family`, `font-scale`, `font-weight-normal`, `font-weight-bold`), read before the loop once fonts are loaded. - When `-font-family` is given on the command line and the weight is not, `Init` does not read the stored `font-weight-normal` / `font-weight-bold` at all. A malformed weight then gets through `Init`, and `GuiMain` throws on it a moment later from `gArgs.IsArgSet("-font-weight-normal")` / `gArgs.GetIntArg(...)`, which also see `settings.json`. None of these call sites is inside a `try` block. The only `catch` in `GuiMain` starts after the options model has been created, so the exception ends in `std::terminate`. ### Why this is a real problem Code path at c14104b: - `SettingToString` / `SettingToInt` fall back to `get_str()`: [src/util/system.cpp#L529-L536](https://github.com/dashpay/dash/blob/c14104b172a2/src/util/system.cpp#L529-L536), [#L554-L561](https://github.com/dashpay/dash/blob/c14104b172a2/src/util/system.cpp#L554-L561) - Unprotected `"lang"` read: [src/qt/optionsmodel.cpp#L208](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/optionsmodel.cpp#L208) - Unprotected font reads: [src/qt/optionsmodel.cpp#L258-L302](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/optionsmodel.cpp#L258-L302); the stored weights are skipped when only `-font-family` is overridden: [#L281-L282](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/optionsmodel.cpp#L281-L282), [#L297-L298](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/optionsmodel.cpp#L297-L298) - `GuiMain` reading the weights through `gArgs` after `Init`: [src/qt/bitcoin.cpp#L700-L701](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L700-L701), [#L709-L710](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L709-L710) - The loop that does handle type errors: [src/qt/optionsmodel.cpp#L355-L370](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/optionsmodel.cpp#L355-L370) - `GetLangTerritory()` reading `-lang` through `gArgs`: [src/qt/bitcoin.cpp#L122](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L122), called again after `InitConfig` at [#L621](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L621) / [#L644](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L644) - `createOptionsModel` only handles a `false` return, not an exception: [src/qt/bitcoin.cpp#L248-L265](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L248-L265), called at [#L687](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L687), before the `try` at [#L777](https://github.com/dashpay/dash/blob/c14104b172a2/src/qt/bitcoin.cpp#L777) Reproduction with dash-qt built from unmodified c14104b (macOS, `--with-gui`, debug build): <details> <summary>dash-qt with <code>{"lang": {}}</code> in settings.json (unfixed)</summary> ``` $ cat $DATADIR/regtest/settings.json {"lang": {}} $ timeout 60 src/qt/dash-qt -regtest -datadir=$DATADIR -nosplash std::terminate() called due to unhandled exception Exception: type=UniValue::type_error, what="JSON value of type object is not of expected type string" exit status: 134 ``` No `debug.log` is created: the abort happens in the second `initTranslations()` call, before the node is created. </details> <details> <summary>dash-qt with <code>{"font-weight-normal": []}</code> in settings.json and <code>-font-family</code> on the command line (unfixed)</summary> ``` $ cat $DATADIR/regtest/settings.json {"font-weight-normal": []} $ timeout 20 src/qt/dash-qt -regtest -datadir=$DATADIR -nosplash -font-family=Montserrat std::terminate() called due to unhandled exception Exception: type=UniValue::type_error, what="JSON value of type array is not of expected type string" exit status: 134 ``` </details> The new `OptionTests` cases, run against the unfixed `OptionsModel::Init`: <details> <summary>test_dash-qt (unfixed), default <code>minimal</code> platform</summary> ``` $ src/qt/test/test_dash-qt ********* Start testing of OptionTests ********* PASS : OptionTests::initTestCase() PASS : OptionTests::migrateSettings() PASS : OptionTests::integerGetArgBug() SKIP : OptionTests::invalidFontSetting() AppTests cannot initialize fonts with the 'minimal' platform plugin. FAIL! : OptionTests::invalidLangSetting() Caught unhandled exception Loc: [qtestcase.cpp(1966)] Totals: 3 passed, 1 failed, 1 skipped, 0 blacklisted, 9ms ********* Finished testing of OptionTests ********* exit status: 134 ``` </details> <details> <summary>test_dash-qt (unfixed), <code>QT_QPA_PLATFORM=cocoa</code> so fonts are loaded</summary> ``` $ QT_QPA_PLATFORM=cocoa src/qt/test/test_dash-qt ********* Start testing of OptionTests ********* PASS : OptionTests::initTestCase() PASS : OptionTests::migrateSettings() PASS : OptionTests::integerGetArgBug() FAIL! : OptionTests::invalidFontSetting() Caught unhandled exception Loc: [qtestcase.cpp(1966)] Totals: 3 passed, 1 failed, 0 skipped, 0 blacklisted, 9ms ********* Finished testing of OptionTests ********* libc++abi: terminating due to uncaught exception of type UniValue::type_error: JSON value of type array is not of expected type string exit status: 134 ``` </details> ### Why it matters The user gets a crash with no explanation instead of an error that names the bad setting and the file. The way out (start with `-nosettings` or fix `settings.json` by hand) is not discoverable from an abort. It only affects startup of dash-qt with a malformed local `settings.json`; dashd is not affected and nothing reaches this path from the network. ## What was done? - `OptionsModel::Init`: the `"lang"` read and the four font reads are wrapped in `try`/`catch` and report the error the same way as the existing `getOption()` loop. The error formatting is moved into a small local lambda (`setting_error`) so the loop and the new call sites share one message; the translatable string is unchanged. - `OptionsModel::Init`: the stored font weights are now always read (and so checked), and the existing condition then picks either that value or the font's default weight as before. This closes the `-font-family` path above. - `GetLangTerritory()`: if reading `-lang` throws, keep the language picked from QSettings / the system locale. Startup then continues to `OptionsModel::Init`, which reports the bad `"lang"` value through the normal error dialog. - `OptionTests`: `invalidLangSetting` and `invalidFontSetting` put an object/array into the persistent settings and check that `Init` returns `false` with the expected error. `invalidFontSetting` also covers the weights with `-font-family` set on the command line. The font case needs loaded fonts, so like `updateFontsWithPixelSizedWidget` it is skipped on the `minimal` platform. ### Why this is the correct minimal fix The root cause is that these reads sit outside the error handling that bitcoin-core/gui#602 added for exactly this kind of type error, so the fix puts them under the same handling and reuses the same message. For the font weights only the `SettingToInt` read is inside the `try`. The font settings are not simply added to the `getOption()` loop: that loop also calls `addOverriddenOption()`, which the font blocks already do, so the overridden-options list would get duplicate entries. Reading the stored weights unconditionally does not change which weight is applied; it only makes sure that a value `GuiMain` will read through `gArgs` anyway has been checked first. Fixing only `OptionsModel::Init` is not enough for `"lang"`: `GetLangTerritory()` reads the same value first and would still abort before `Init` runs. A type check there would need to duplicate which `UniValue` types `SettingToString` accepts, while catching the conversion error keeps the existing `-lang` behaviour (strings, `-nolang`, numbers) unchanged. Making the conversion helpers non-throwing or widening `GuiMain`'s `try` would be larger changes; the latter would also only swap the abort for a generic "runaway exception" message. Behaviour for valid settings is unchanged. If the bold weight is malformed after the normal weight has been reset via `forceSetting`, that partial state is not undone; `Init` returns `false` and startup stops anyway. The `"lang"` handling exists the same way in Bitcoin Core and could also be sent upstream. ## How Has This Been Tested? macOS arm64, `--with-gui --enable-debug --enable-werror`, depends Qt 5. - Before the fix: the two dash-qt reproductions and the two `test_dash-qt` runs above (abort, exit 134). - With the `try`/`catch` changes but without always reading the stored weights, the `-font-family` cases of `invalidFontSetting` fail (cocoa): ``` FAIL! : OptionTests::invalidFontSetting() '!OptionsModel{m_node}.Init(error)' returned FALSE. () Loc: [qt/test/optiontests.cpp(135)] ``` - After the fix, `src/qt/test/test_dash-qt` (default `minimal` platform): all suites pass; in `OptionTests`, `invalidLangSetting` passes and `invalidFontSetting` is skipped as expected. ``` SKIP : OptionTests::invalidFontSetting() AppTests cannot initialize fonts with the 'minimal' platform plugin. PASS : OptionTests::invalidLangSetting() ... Totals: 8 passed, 0 failed, 2 skipped, 0 blacklisted, 11ms ``` - After the fix, `QT_QPA_PLATFORM=cocoa src/qt/test/test_dash-qt`: all `OptionTests` pass, including both new cases. ``` PASS : OptionTests::invalidFontSetting() PASS : OptionTests::invalidLangSetting() ... Totals: 10 passed, 0 failed, 0 skipped, 0 blacklisted, 15ms ``` - After the fix, dash-qt with `{"lang": {}}`: no abort; it reports the error and shows the dialog (the process was then killed by `timeout` while the modal dialog was open): ``` $ timeout 20 src/qt/dash-qt -regtest -datadir=$DATADIR -nosplash Error: Could not read setting "lang", JSON value of type object is not of expected type string. Settings file "<datadir>/regtest/settings.json" might be corrupt or invalid. exit status: 124 ``` - After the fix, dash-qt with `{"font-weight-normal": []}` and `-font-family=Montserrat`: same, no abort: ``` $ timeout 20 src/qt/dash-qt -regtest -datadir=$DATADIR -nosplash -font-family=Montserrat Error: Could not read setting "font-weight-normal", JSON value of type array is not of expected type string. Settings file "<datadir>/regtest/settings.json" might be corrupt or invalid. exit status: 124 ``` - `contrib/devtools/clang-format-diff.py` only suggests reflowing the moved `tr()` line and the test lambdas/strings, which follow the style of the surrounding code; `test/lint/lint-whitespace.py` is clean. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 69ce7789a69d196e7934f802450df2f8d40dc02fe7c7490c46eb5a86cf8674d6272472476a71d5c9984dfad146c78f6dcbee9731ad55f24b25d3830aeba933b3 (cherry picked from commit c932b8c)
…en listdiff changes its address 18c08a6 test: add missing negative test for protx diff (Konstantin Akimov) 26a0440 docs: v24 had not been released yet, no need to explain detailed difference in release notes (Konstantin Akimov) 1eae1b6 test: use existing test suite to check new address invariants rather than filling them from scratch (Konstantin Akimov) bec74ca refactor: move extra logic with addresses to rpc/ where it's used (Konstantin Akimov) 1aab06b doc: add release notes for protx listdiff legacy evonode Platform addresses (Ivan Shumkov) ac2d836 fix(rpc): report legacy evonode Platform addresses when listdiff changes its address (Ivan Shumkov) Pull request description: ## Issue being fixed or feature implemented `addresses` is the non-deprecated way to read a masternode's endpoints; `service`, `platformP2PPort` and `platformHTTPPort` are only printed with `-deprecatedrpc=service`. For an evonode on legacy (pre-extended) addresses, the full masternode state (`protx info`, `protx list`, `addedMNs` in `protx listdiff`, ...) reports `addresses.platform_p2p` / `addresses.platform_https` as the core P2P address paired with the legacy `platformP2PPort` / `platformHTTPPort` (`GetNetInfoWithLegacyFields()`). The state diffs in `protx listdiff`'s `updatedMNs` only reported them when the ports themselves changed. A diff that changed only the core P2P address carried `core_p2p` alone. So a client that follows `protx listdiff` and reads only `addresses` never learns the Platform ports of a legacy evonode that it first saw without an address, if the evonode later gets an address with unchanged ports. An evonode has no address when it is registered without one (allowed; it starts PoSe-banned) or after its operator fields are reset (revocation or operator key change), and a ProUpServTx later revives it, typically with the same ports. This happened on testnet: evonode `b2626fdf98bc671ccf7711bb2dacaed615b98c4092f527a0bacd1519a408c2ae` was registered at 1394574, had its operator key changed at 1394576 and was revived at 1394577 with ports 36656/1443 unchanged. Its ports were then only visible through the deprecated fields. Extended-address evonodes don't have this problem, since their diffs carry the whole netInfo object, Platform entries included. ### Why Dash Platform needs this Dash Platform's Drive follows `protx listdiff` to maintain its masternode list and build its validator sets, and validator-set membership is consensus-relevant for Platform. Drive currently reads the flat `platformP2PPort` / `platformHTTPPort`, so every Platform node has to run Core with `-deprecatedrpc=service`. To read only `addresses` and drop that flag, Drive must be able to learn every evonode's Platform ports from `addresses`, which it can't in the case above. `protx listdiff` is computed from stored states when it is called, so with this change historical windows are covered as well. ## What was done? The change is confined to `protx listdiff` in `src/rpc/evo.cpp`: - Each updated masternode is looked up in the target list instead of the base list. `updatedMNs` is keyed by the target entry's internal id, and `proTxHash` and type are the same in both lists; the target entry gives the masternode's state at the diff's target block. - When a diff changes the masternode's address and the new address is not empty, `addresses` in the diff output is replaced with `GetNetInfoWithLegacyFields()` of the target state. This is the helper the full-state outputs use, so the diff reports exactly what the full state does. For regular masternodes and extended-address evonodes the helper returns what the diff already reported, so only legacy evonodes see a difference. `CDeterministicMNStateDiff::ToJson()` and `evo/dmnstate.*` are not changed. The target state is only known to the RPC, and `GetNetInfoWithLegacyFields()` lives outside `libdashkernel`, so the completion is done in the RPC layer. Unchanged, on purpose: - A diff that changes only the Platform ports still pairs them with the `255.255.255.255` placeholder, as documented in the 23.0 release notes. - A diff that clears a masternode's address still omits `addresses`. If the ports change in the same diff window, they are still reported as `[::]:<port>`. - `protx diff`, the full-state outputs and what `-deprecatedrpc=service` gates are unchanged. There are no consensus, serialization, evodb or P2P changes. ## How Has This Been Tested? `rpc_netinfo.py`, in the existing `test_deprecation()` step (legacy evonode, queried on a node without `-deprecatedrpc=service`): - Address-only diff: the first and the third ProUpServTx of that step set the same Platform ports, so `protx listdiff` between their heights changes only the core P2P address. The test checks that its `addresses` holds `core_p2p`, `platform_p2p` and `platform_https` on the new address with the unchanged ports. This is the case being fixed. - Cleared address: the operator is revoked, which clears the address and keeps the legacy ports. The test checks that the full state's `addresses` is empty and that the diff has no `addresses` key. Already covered by existing assertions: - `test_deprecation()`: a legacy evonode whose address and ports change together, and the placeholder for a port-only change. - `test_shims()`: the exact `addresses` of an extended-address evonode's diff. ## Breaking Changes `protx listdiff`: for an evonode on legacy addresses, an `updatedMNs` entry whose diff changes the core P2P address now includes `platform_p2p` and `platform_https` in `addresses`, where it previously carried `core_p2p` alone. Both keys were already possible there, with the same format. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: self-ACK 18c08a6 kwvg: utACK 18c08a6 Tree-SHA512: 9e9007b75aed678112026d8a0b1257c136294fff7973816cc9f5ce58b4e72f21d580e629ab86da16acd061ae98a346740325c31d25b8bfa326ca8b33dffa9ada (cherry picked from commit 0021b20)
…ass PSBT by const reference in PSBTInputSignedAndVerified) 4b94367 partial Merge bitcoin#31650: refactor: Avoid copies by using const references or by move-construction (pasta) 2cb6f5a Merge bitcoin#34272: psbt: Fix `PSBTInputSignedAndVerified` bounds `assert` (pasta) Pull request description: ## Issue being fixed or feature implemented `PSBTInputSignedAndVerified()` takes its `PartiallySignedTransaction` by value (`const PartiallySignedTransaction psbt`, missing the `&`). Each call therefore copies the whole PSBT: the unsigned transaction with all its inputs and outputs, and every `PSBTInput`/`PSBTOutput` with its scripts, keypath maps and unknown fields. The function is called once per input. `SignPSBTInput()` calls it first thing, `FinalizePSBT()` calls `SignPSBTInput()` for every input, and `AnalyzePSBT()` calls it directly and then again through `SignPSBTInput()`. So `finalizepsbt`, `analyzepsbt`, `walletprocesspsbt` and the other PSBT paths do work proportional to (number of inputs) × (PSBT size), which is quadratic in the input count. The copy happens before any check, so inputs with no UTXO data pay for it too. This came in with the partial backport of bitcoin#25595 (2aa0e7b), so it affects the v24.0.0 release candidates. v23.2 used `PSBTInputSigned(const PSBTInput&)` on these paths and was linear. Upstream fixed the signature in bitcoin#31650. The same function also has an off-by-one debug assertion, `assert(psbt.inputs.size() >= input_index)`, which accepts `input_index == psbt.inputs.size()` even though the next line indexes `psbt.inputs[input_index]`. Upstream fixed this in bitcoin#34272. No current caller passes an out-of-range index: every caller loops over `i < tx->vin.size()`, and decoding requires `inputs.size() == vin.size()`. So this part only restores the intended guard. ### Why this is a real problem Code path at c14104b: - By-value declaration and definition: [src/psbt.h#L872](https://github.com/dashpay/dash/blob/c14104b172a2/src/psbt.h#L872), [src/psbt.cpp#L184-L187](https://github.com/dashpay/dash/blob/c14104b172a2/src/psbt.cpp#L184-L187) - Called unconditionally from `SignPSBTInput()`: [src/psbt.cpp#L269](https://github.com/dashpay/dash/blob/c14104b172a2/src/psbt.cpp#L269) - `FinalizePSBT()` calls `SignPSBTInput()` once per input: [src/psbt.cpp#L333](https://github.com/dashpay/dash/blob/c14104b172a2/src/psbt.cpp#L333) - `AnalyzePSBT()` makes two calls per non-final input: [src/node/psbt.cpp#L61](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/psbt.cpp#L61), [src/node/psbt.cpp#L66](https://github.com/dashpay/dash/blob/c14104b172a2/src/node/psbt.cpp#L66) Measured on regtest with a functional-test-style script. It builds a PSBT with N inputs using `createpsbt` (inputs without UTXO data, one `data` output), then times one `finalizepsbt` and one `analyzepsbt` call. Both builds use the same configuration (`--enable-debug`, so absolute times are higher than in a release build; the growth ratio is the point). <details><summary>Script</summary> ```python class PSBTCopyScaling(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True self.rpc_timeout = 600 def run_test(self): node = self.nodes[0] for n in (2000, 4000, 8000, 16000): inputs = [{"txid": "%064x" % (i + 1), "vout": 0} for i in range(n)] psbt = node.createpsbt(inputs, [{"data": "00"}]) t0 = time.perf_counter(); res = node.finalizepsbt(psbt); t_fin = time.perf_counter() - t0 assert not res["complete"] t0 = time.perf_counter(); res = node.analyzepsbt(psbt); t_ana = time.perf_counter() - t0 assert len(res["inputs"]) == n self.log.info(...) ``` </details> <details><summary>Unfixed (c14104b): each doubling of the input count costs 4x</summary> ``` TestFramework (INFO): inputs= 2000 psbt_bytes= 84039 finalizepsbt= 0.092s analyzepsbt= 0.183s TestFramework (INFO): inputs= 4000 psbt_bytes= 168039 finalizepsbt= 0.361s analyzepsbt= 0.730s (x3.9 / x4.0 vs previous) TestFramework (INFO): inputs= 8000 psbt_bytes= 336039 finalizepsbt= 1.436s analyzepsbt= 2.862s (x4.0 / x3.9 vs previous) TestFramework (INFO): inputs= 16000 psbt_bytes= 672039 finalizepsbt= 5.735s analyzepsbt= 11.462s (x4.0 / x4.0 vs previous) ``` </details> <details><summary>With this PR: linear, over 250x faster at 16000 inputs</summary> ``` TestFramework (INFO): inputs= 2000 psbt_bytes= 84039 finalizepsbt= 0.016s analyzepsbt= 0.008s TestFramework (INFO): inputs= 4000 psbt_bytes= 168039 finalizepsbt= 0.016s analyzepsbt= 0.012s (x1.0 / x1.5 vs previous) TestFramework (INFO): inputs= 8000 psbt_bytes= 336039 finalizepsbt= 0.014s analyzepsbt= 0.021s (x0.9 / x1.8 vs previous) TestFramework (INFO): inputs= 16000 psbt_bytes= 672039 finalizepsbt= 0.021s analyzepsbt= 0.040s (x1.6 / x1.9 vs previous) ``` </details> ### Why it matters A PSBT with several thousand inputs (for example, a large consolidation) makes `analyzepsbt`/`finalizepsbt` take seconds instead of milliseconds. A 670 kB PSBT already ties up an RPC worker thread for over 11 seconds, and the cost keeps growing quadratically with larger PSBTs. Only the RPC interface and local tooling reach these paths, so the impact is wasted CPU and allocation work plus slow RPC responses. There is no network-facing or consensus effect. ## What was done? Two backport commits: - bitcoin#34272 (full, 2f5b1c5): changes the bounds assertion in `PSBTInputSignedAndVerified()` to `assert(input_index < psbt.inputs.size())`. - bitcoin#31650 (partial): only the `src/psbt.{h,cpp}` hunk of faf0c2d, which changes the parameter to `const PartiallySignedTransaction& psbt`. Omitted from bitcoin#31650: - The rest of faf0c2d: `src/external_signer.{h,cpp}` (move-construct the `ExternalSigner` strings, `const std::string&` chain in `Enumerate`), `src/netbase.h` (move-construct the `Proxy` unix socket path), `src/test/argsman_tests.cpp`, `src/wallet/rpc/util.{h,cpp}` (`HandleWalletError` takes `const std::shared_ptr<CWallet>&`) and `src/wallet/wallet.{h,cpp}` (`GetNewDestination` label by `const std::string&`). These are small copy cleanups unrelated to PSBT handling. They can come with a full backport of the PR. - All of fa64d84 (enable clang-tidy `readability-avoid-const-params-in-decls`). It needs a tree-wide sweep of Dash-specific declarations first. ### Why this is the correct minimal fix The function only reads the PSBT, and no caller relies on getting a private copy. `SignPSBTInput()` mutates its own `psbt` only after the helper returns. Passing by const reference removes the per-call copy and so removes the quadratic term, without changing any result. This is exactly the upstream change. Smaller alternatives do not exist, and restructuring the callers is unnecessary. The assertion change follows upstream bitcoin#34272, which was raised in review of bitcoin#31650. It touches the same function and makes the existing guard match the index it protects. Behavior for all valid inputs is unchanged. This should also go to the v24.0.x release branch: the by-value copy is new in the v24.0.0 release candidates (v23.2 was linear on these paths). ## How Has This Been Tested? - Built `dashd`, `dash-cli` and `test_dash` (macOS arm64, depends prefix, `--enable-debug --enable-werror --without-gui`) before and after the change. - Ran the scaling script above against both builds (output shown above): quadratic before, linear after. - `src/test/test_dash --run_test=psbt_wallet_tests`: no errors. - `test/functional/test_runner.py rpc_psbt.py`: `--descriptors` and `--legacy-wallet` both passed. - `test/lint/lint-whitespace.py`: clean. No regression test was added. The change is a missing `&` and an assertion bound that no caller can reach, and the effect is purely on performance, which cannot be asserted deterministically in the test suite. The existing PSBT unit and functional tests cover the unchanged behavior. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: d3974e9e15d6e8a454a3a6ba2c0d922f519ce1227874508c3ad7ac82ad0e33a21d4e1034f240eeca9cc65090f112b21c5db6e215d2a9f8f5ff0db7b085826c70 (cherry picked from commit 4647220)
…ed dash-cli command 5339d4f refactor(contrib): match dash-cli connection options by prefix in fish completion (pasta) ff98899 fix(contrib): stop fish completion from re-running the typed dash-cli command (pasta) Pull request description: ## Issue being fixed or feature implemented The fish completion for `dash-cli` (`contrib/completions/fish/dash-cli.fish`, imported from bitcoin#24611) builds its suggestions by running the command line the user has typed so far, with `help` or `-help` appended. Once an RPC command has been typed, everything after it is passed to the node as RPC parameters, so the appended `help` does not ask for help: it becomes one more parameter of the typed command, and the command runs. For anyone who has installed this completion, pressing Tab after a complete line such as `dash-cli sendtoaddress <address> 1 ` sends the coins before Enter is pressed, with `help` as the wallet comment. Each Tab runs the line several times, because every `complete` rule's `--condition` calls the helper again. Pressing Enter afterwards sends once more. The same applies to any RPC that accepts a trailing string parameter. A leading `-generate` reserves a new wallet address on each Tab. ### Why this is a real problem Code path at c14104b: - [`dash-cli.fish#L5`](https://github.com/dashpay/dash/blob/c14104b172a2/contrib/completions/fish/dash-cli.fish#L5) takes every completed token with `commandline -oc`. [L13-L16](https://github.com/dashpay/dash/blob/c14104b172a2/contrib/completions/fish/dash-cli.fish#L13-L16) strip only a bare `help` and `-stdin*`, and [L19](https://github.com/dashpay/dash/blob/c14104b172a2/contrib/completions/fish/dash-cli.fish#L19) runs `$cmd help`. [L63](https://github.com/dashpay/dash/blob/c14104b172a2/contrib/completions/fish/dash-cli.fish#L63) and [L65](https://github.com/dashpay/dash/blob/c14104b172a2/contrib/completions/fish/dash-cli.fish#L65) run `$cmd -help` the same way. - All three rules ([L75-L99](https://github.com/dashpay/dash/blob/c14104b172a2/contrib/completions/fish/dash-cli.fish#L75-L99)) call the helper from `--condition`, which fish evaluates on every Tab. - [`src/util/system.cpp#L262-L276`](https://github.com/dashpay/dash/blob/c14104b172a2/src/util/system.cpp#L262-L276): the first non-dash argument is the command. All later argv entries, including `help` and `-help`, are copied into the command arguments without being parsed. [`src/bitcoin-cli.cpp#L1207-L1212`](https://github.com/dashpay/dash/blob/c14104b172a2/src/bitcoin-cli.cpp#L1207-L1212) sends `args[0]` as the method. - [`src/wallet/rpc/spend.cpp#L238`](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/rpc/spend.cpp#L238): the third `sendtoaddress` parameter is an optional string `comment`, so `help` is accepted and the send goes ahead. Reproduction against the unfixed tree. A fresh regtest `dashd` is built from this branch, wallet `w1` is funded, and `w2` provides the destination address. One Tab is simulated with fish 4.9.3 `complete -C '<line>'`, which evaluates the completion rules exactly as an interactive Tab does. Enter is never pressed. Script: `regtest-repro.sh` (included below). <details><summary>Unfixed: one Tab after a typed <code>sendtoaddress</code> line sends 5 DASH</summary> ``` $ git rev-parse --short HEAD c14104b $ regtest-repro.sh $PWD == completion output == Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 getblockc': getblockchaininfo getblockcount Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 sendtoa': sendtoaddress "address" amount ( "comment" "comment_to" subt Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 -rpcw': -rpcwait -rpcwaittimeout= -rpcwallet= Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 help getblockc': getblockchaininfo getblockcount == sendtoaddress == w1 txs before Tab: 0 sends w2 balance before Tab (minconf 0): 0.00000000 $ fish -c "source dash-cli.fish; complete -C 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 sendtoaddress yYY9kJyUeiJJRU9nAEsx5Nu2wXZEjK4CnD 1 '" # one Tab press, Enter never pressed w1 txs after Tab: 5 sends w2 balance after Tab (minconf 0): 5.00000000 mempool size: "size": 5, comments on the w1 sends: 2 "comment": "-help", 3 "comment": "help", == -generate == w1 before Tab: "keypoolsize": 1000,, blocks: 110 $ fish -c "source dash-cli.fish; complete -C 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 -generate '" # one Tab press, Enter never pressed w1 after Tab: "keypoolsize": 999,, blocks: 110 ``` </details> To show exactly what the completion executes, the same line was also completed with a stand-in `dash-cli` on `PATH` that only logs its arguments: <details><summary>Unfixed: commands run by the completion for one Tab</summary> ``` $ git rev-parse --short HEAD c14104b $ stub-repro.sh $PWD ### Tab after: 'dash-cli -regtest ' ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest -help ran: dash-cli -regtest -help ### Tab after: 'dash-cli -regtest -rpcwallet=w1 sendtoaddress yNmxLr8sgqWjj8qEcvH7bD3b4G4Bu4PZpq 1 ' ran: dash-cli -regtest -rpcwallet=w1 sendtoaddress yNmxLr8sgqWjj8qEcvH7bD3b4G4Bu4PZpq 1 help ran: dash-cli -regtest -rpcwallet=w1 sendtoaddress yNmxLr8sgqWjj8qEcvH7bD3b4G4Bu4PZpq 1 help ### Tab after: 'dash-cli -regtest -generate ' ran: dash-cli -regtest -generate help ran: dash-cli -regtest -generate help ran: dash-cli -regtest -generate help ran: dash-cli -regtest -generate -help ran: dash-cli -regtest -generate -help ### Tab after: 'dash-cli -regtest -stdin ' ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest -stdin -help ran: dash-cli -regtest -stdin -help ``` </details> <details><summary>regtest-repro.sh</summary> ```sh #!/bin/sh # Usage: regtest-repro.sh <worktree> # Starts a fresh regtest dashd from <worktree>/src, funds wallet w1, then asks # fish for completions (exactly what one Tab press does) of a fully typed but # not yet submitted `sendtoaddress` line and of `-generate `, and reports what # changed on the node. Enter is never pressed. WT=$1 P=$(cd "$(dirname "$0")" && pwd) FISH=fish D=$P/regtest-datadir rm -rf "$D"; mkdir -p "$D" export PATH=$WT/src:$PATH CLI="dash-cli -regtest -datadir=$D -rpcport=19876" "$WT/src/dashd" -regtest -datadir="$D" -rpcport=19876 -port=19877 -listen=0 -fallbackfee=0.0001 -daemon >/dev/null $CLI -rpcwait getblockcount >/dev/null $CLI createwallet w1 >/dev/null $CLI createwallet w2 >/dev/null $CLI -rpcwallet=w1 generatetoaddress 110 "$($CLI -rpcwallet=w1 getnewaddress)" >/dev/null ADDR=$($CLI -rpcwallet=w2 getnewaddress) tab() { echo "\$ fish -c \"source dash-cli.fish; complete -C '$1'\" # one Tab press, Enter never pressed" "$FISH" --no-config -c "source $WT/contrib/completions/fish/dash-cli.fish; complete -C '$1' >/dev/null" } echo "== completion output ==" for l in "$CLI getblockc" "$CLI -rpcwallet=w1 sendtoa" "$CLI -rpcwallet=w1 -rpcw" "$CLI help getblockc"; do echo "Tab after '$(echo "$l" | sed "s|$D|DATADIR|")':" "$FISH" --no-config -c "source $WT/contrib/completions/fish/dash-cli.fish; complete -C '$l'" | cut -c1-60 | sed 's/^/ /' done echo "== sendtoaddress ==" echo "w1 txs before Tab: $($CLI -rpcwallet=w1 listtransactions '*' 1000 | grep -c '"category": "send"') sends" echo "w2 balance before Tab (minconf 0): $($CLI -rpcwallet=w2 getreceivedbyaddress $ADDR 0)" tab "$CLI -rpcwallet=w1 sendtoaddress $ADDR 1 " echo "w1 txs after Tab: $($CLI -rpcwallet=w1 listtransactions '*' 1000 | grep -c '"category": "send"') sends" echo "w2 balance after Tab (minconf 0): $($CLI -rpcwallet=w2 getreceivedbyaddress $ADDR 0)" echo "mempool size: $($CLI getmempoolinfo | grep '"size"')" echo "comments on the w1 sends:" $CLI -rpcwallet=w1 listtransactions '*' 1000 | grep '"comment"' | sort | uniq -c echo "== -generate ==" echo "w1 before Tab: $($CLI -rpcwallet=w1 getwalletinfo | grep '"keypoolsize"'), blocks: $($CLI getblockcount)" tab "$CLI -rpcwallet=w1 -generate " echo "w1 after Tab: $($CLI -rpcwallet=w1 getwalletinfo | grep '"keypoolsize"'), blocks: $($CLI getblockcount)" $CLI stop >/dev/null sleep 3 ``` </details> ### Why it matters Pressing Tab is expected to be harmless. With this completion installed, Tab can broadcast a payment the user has not confirmed. It does so several times per keypress, so a later Enter produces a duplicate payment. Any other state-changing RPC that tolerates the extra parameter runs early in the same way. The scope is limited: the completion is opt-in and not installed by the build, it needs working RPC credentials and an unlocked wallet, and the user must already have typed the full command. Nothing is reachable remotely. The effect is still an unintended spend of the user's own funds. ## What was done? Only `contrib/completions/fish/dash-cli.fish` changes. The first commit is the fix; the second replaces the explicit option list with prefix matching. - `__fish_dash_cli_get_commands_helper` now runs `help` with only the executable and the connection options typed before the RPC command, matched by prefix: `-conf`, `-datadir`, `-testnet`, `-regtest`, `-devnet`, `-chain`, and every `-rpc*` option except `-rpcwait*` (which would make Tab hang while the node is down). This is the same prefix approach as the dash-cli bash completion, so new `-rpc*` connection options need no edits here. It stops at the first non-dash token, so the RPC command and its arguments are never passed on. Other leading options are dropped, including `-generate`, `-getinfo`, `-netinfo`, `-addrinfo`, `-named`, `-rpcwait*` and `-stdin*`. This replaces the old ad hoc stripping of `help` and `-stdin*`, which the allowlist covers. - `__fish_dash_cli_get_options` runs `-help` on the bare executable (`$cmd[1]`). The option list does not depend on the rest of the line, and the other Dash completion scripts (`dash-tx.fish`, `dash-wallet.fish`, `dash-util.fish`, `dashd.fish`) already do this. ### Why this is the correct minimal fix The root cause is that the completion re-executes user input. The fix stops passing that input on, and keeps only what is needed to reach the same node and wallet (datadir, conf, network, RPC endpoint and credentials, wallet). An allowlist is used because a denylist would have to keep up with every dash-cli option that changes what gets executed. Some leading options are themselves commands: `-generate` calls `getnewaddress` before it validates its arguments. Stripping only `help` would not help, because any typed RPC still runs. Excluding specific RPCs would always miss some. Left unchanged on purpose: the completion rules, the output parsing, and the `-help`/`-?` early returns. The helper is still called once per rule on each Tab, which now only issues the read-only `help` RPC. Caching it, or adding a default client timeout, would be separate improvements. Bitcoin Core's `bitcoin-cli.fish` has the same pattern. ## How Has This Been Tested? No automated test was added. The repository has no harness for shell completion scripts, and the behaviour was checked end to end against a real node instead. Environment: macOS arm64, `dashd`/`dash-cli` built from this branch, fish 4.9.3. - `regtest-repro.sh` before the fix: one Tab after `sendtoaddress <w2 address> 1 ` produced 5 sends from `w1` (5 DASH received by `w2`, 5 transactions in the mempool, comments `help` ×3 and `-help` ×2). The `-generate ` Tab reduced the keypool from 1000 to 999. Output is above. - `regtest-repro.sh` after the fix: no sends, `w2` balance 0, empty mempool, keypool unchanged. Command, `help <command>` and option completion return the same suggestions as before. <details><summary>Fixed: same script</summary> ``` $ git diff --stat contrib/completions/fish/dash-cli.fish | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) $ regtest-repro.sh $PWD == completion output == Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 getblockc': getblockchaininfo getblockcount Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 sendtoa': sendtoaddress "address" amount ( "comment" "comment_to" subt Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 -rpcw': -rpcwait -rpcwaittimeout= -rpcwallet= Tab after 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 help getblockc': getblockchaininfo getblockcount == sendtoaddress == w1 txs before Tab: 0 sends w2 balance before Tab (minconf 0): 0.00000000 $ fish -c "source dash-cli.fish; complete -C 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 sendtoaddress ya4Gcphm7nFvTRuHjLqnmqh6W6tkhjmvfL 1 '" # one Tab press, Enter never pressed w1 txs after Tab: 0 sends w2 balance after Tab (minconf 0): 0.00000000 mempool size: "size": 0, comments on the w1 sends: == -generate == w1 before Tab: "keypoolsize": 1000,, blocks: 110 $ fish -c "source dash-cli.fish; complete -C 'dash-cli -regtest -datadir=DATADIR -rpcport=19876 -rpcwallet=w1 -generate '" # one Tab press, Enter never pressed w1 after Tab: "keypoolsize": 1000,, blocks: 110 ``` </details> <details><summary>Fixed: commands run by the completion for one Tab (logging stand-in)</summary> ``` $ git diff --stat contrib/completions/fish/dash-cli.fish | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) $ stub-repro.sh $PWD ### Tab after: 'dash-cli -regtest ' ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -help ran: dash-cli -help ### Tab after: 'dash-cli -regtest -rpcwallet=w1 sendtoaddress yNmxLr8sgqWjj8qEcvH7bD3b4G4Bu4PZpq 1 ' ran: dash-cli -regtest -rpcwallet=w1 help ran: dash-cli -regtest -rpcwallet=w1 help ### Tab after: 'dash-cli -regtest -generate ' ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -help ran: dash-cli -help ### Tab after: 'dash-cli -regtest -stdin ' ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -regtest help ran: dash-cli -help ran: dash-cli -help ``` </details> - After switching to prefix matching (second commit), the regtest check was repeated: command, `help <command>`, option and `-rpcwait` line completions return the expected suggestions; Tab after the `sendtoaddress` line and after `-generate ` leaves w1 with 0 sends, keypool 1000 and an empty mempool; with the node stopped, Tab after `-rpcwait getblockc` returns immediately. A logging stand-in confirmed that every connection option (`-conf`, `-datadir`, `-testnet`, `-regtest`, `-devnet`, `-chain`, `-rpcconnect`, `-rpcport`, `-rpcuser`, `-rpcpassword`, `-rpccookiefile`, `-rpcwallet`, `-rpcclienttimeout`) is passed on and `-rpcwait`, `-rpcwaittimeout`, `-stdin*`, `-named`, `-generate`, `-getinfo`, `-netinfo` are not. - `fish --no-execute contrib/completions/fish/dash-cli.fish` succeeds (syntax check). `fish_indent` reports no changes in the edited hunks. - `test/lint/lint-whitespace.py` passes. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 233c7ab06389315b9ca8aed82b36438b55f482e6fc167c4cecaaba135ff0363d84c1b593904bfdb0ac529c2f2b1722f29eeb12c6616d9da75bb1c1934adce9e9 (cherry picked from commit 3789ec0)
… so rebalancing cannot oscillate 076b093 fix(coinjoin): floor the denomination gap threshold so rebalancing cannot oscillate (pasta) Pull request description: ## Issue being fixed or feature implemented CoinJoin promotion and demotion use a gap threshold so that converting coins between two adjacent denominations does not flip back and forth. Since the threshold was scaled with the goal (`GetGapThreshold(nGoal) = max(nGoal / 5, 1)`), any `-coinjoindenomsgoal` below 25 gets a gap of 4 or less. That is too small to absorb the 10:1 asymmetry of a conversion: a promotion moves the smaller denomination's deficit by up to +10 and the larger one's by -1, so with a gap of `g` an immediate reversal is possible whenever `2g + 1 <= PROMOTION_RATIO`, i.e. `g <= 4`. Example at the minimum goal of 10 (gap 2), all coins fully mixed: - 10 smaller / 7 larger: deficits 0 vs 3, `3 > 0 + 2`, so the wallet promotes. - Result 0 smaller / 8 larger: deficits 10 vs 2, `10 > 2 + 2`, so the wallet demotes back to 10 / 7. - Once the 10 demoted outputs are mixed again, the promotion fires again, and so on. Only wallets that set the goal below 25 are affected; the default of 50 (gap 10) is not. ### Why this is a real problem The decision functions fire on a one-sided deficit comparison with no other hysteresis: - [`GetGapThreshold`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/common.h#L131) - [`ShouldPromoteDenoms`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/common.h#L224-L233) and [`ShouldDemoteDenoms`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/common.h#L242-L251) - The client passes wallet totals and `GetDenomsGoal()` straight through ([client.cpp](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/client.cpp#L2494-L2520)), and the goal can be set as low as [`MIN_COINJOIN_DENOMS_GOAL = 10`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/options.h#L19) ([clamp](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/options.cpp#L72)). The new regression test `rebalance_never_immediately_reverses` checks every goal from 10 to 200 and every pair of counts from 0 to twice the goal: after any promotion the matching demotion must not fire, and the reverse. Run against unfixed develop (c14104b) with only the test added, it fails 710 times, at exactly goals 10 through 24, and the 10/7 example above is among the failures: <details><summary>Unfixed run (abridged)</summary> ``` $ src/test/test_dash --color_output=false --run_test=coinjoin_inouts_tests/rebalance_never_immediately_reverses Running 1 test case... test/coinjoin_inouts_tests.cpp:1386: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 10: demoting 5/0 is undone by a promotion test/coinjoin_inouts_tests.cpp:1386: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 10: demoting 6/0 is undone by a promotion ... test/coinjoin_inouts_tests.cpp:1383: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 10: promoting 10/7 is undone by a demotion ... test/coinjoin_inouts_tests.cpp:1383: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 24: promoting 25/19 is undone by a demotion *** 710 failures are detected in the test module "Dash Core Test Suite" exit=201 ``` Goals with failures: 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24. </details> ### Why it matters A wallet with a low goal can keep promoting and demoting the same value indefinitely. Each round pays mixing fees and puts another 10-input promotion and 1:10 demotion on-chain, which works against the privacy that rebalancing is meant to protect. The cycle is slow, because demoted outputs have to be remixed before they can be promoted again, and it only affects the wallet owner: no funds are lost and nothing outside the wallet changes. ## What was done? - Added `MIN_GAP_THRESHOLD = (PROMOTION_RATIO + 1) / 2` (5) in `src/coinjoin/common.h`, with the `2 * gap + 1 > PROMOTION_RATIO` derivation in its comment. - `GetGapThreshold` now returns `std::max(nGoal / GAP_DIVISOR, MIN_GAP_THRESHOLD)`. - Added the `rebalance_never_immediately_reverses` test to `src/test/coinjoin_inouts_tests.cpp`, next to the existing gap-threshold tests. ### Why this is the correct minimal fix A promotion needs `Dl > Ds + g`. Afterwards `Dl` drops by 1 and `Ds` rises by at most `PROMOTION_RATIO`. Capping deficits at the goal only shrinks these changes. A demotion right after would need `Ds' > Dl' + g`, so `Ds' >= Ds + 2g + 1`, which is impossible once `2g + 1 > PROMOTION_RATIO`. Demote-then-promote is symmetric. Five is the smallest gap that satisfies this, and it only changes goals 10 to 24. Goals of 25 and above, including the default of 50, keep their current threshold. The floor stays below the goal, so conversion is still reachable at the minimum goal: at goal 10 the extreme 10-vs-0 imbalance (gap 10 > 5) still promotes and demotes. The existing `gap_threshold_reachable_at_every_goal` test, which guards against the threshold disabling the feature, still passes unchanged. Adding a cooldown or remembering past conversions would also stop the loop, but it would need new wallet state when the threshold alone is the root cause. The deficit logic and the fully-mixed gates are unchanged. ## How Has This Been Tested? Built `src/test/test_dash` on macOS arm64 (`--enable-debug --enable-werror`). - Before the fix (develop c14104b + new test only): `src/test/test_dash --run_test=coinjoin_inouts_tests/rebalance_never_immediately_reverses` fails with 710 failures, all at goals 10 to 24 (see above). - After the fix: - `src/test/test_dash --run_test=coinjoin_inouts_tests/rebalance_never_immediately_reverses`: no errors. - `src/test/test_dash --run_test=coinjoin_inouts_tests`: 52 test cases, no errors. This includes `gap_threshold_reachable_at_every_goal` and the other promote/demote decision tests. - `coinjoin_tests` (14 cases), `coinjoin_queue_tests` (11), `coinjoin_dstxmanager_tests` (5): no errors. - `contrib/devtools/clang-format-diff.py` on the diff reports no changes. `test/lint/lint-whitespace.py`, `lint-includes.py`, `lint-format-strings.py`, `lint-tests.py` and `lint-circular-dependencies.py` pass. ## Breaking Changes None. With `-coinjoindenomsgoal` between 10 and 24, the deficit gap a conversion needs rises from 2–4 to 5. No other behavior changes. This fixes a regression introduced in the v24.0.0 release candidates and should also go to the v24.0.x branch. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 93b9fb1489afff38b62d9193b8505e322bb91deda426b87dfd8a61a25a587466ee4006af86ee8276a9aa6d79ae031a3f64f1aef5cc5415153270b14d1a805211 (cherry picked from commit 7964c37)
…nce validation is enabled d77f357 fix(init): restore the txindex requirement when governance validation is enabled (pasta) Pull request description: ## Issue being fixed or feature implemented Governance validation looks up each proposal's collateral transaction through the transaction index. Without `-txindex`, every proposal fails validation with "Invalid proposal collateral". Until v23.1.0 `dashd` refused to start with governance enabled and `-txindex=0` outside regtest: > Transaction index can't be disabled with governance validation enabled. Either start with -disablegovernance command line switch or enable transaction index. Commit 91129d4 ("refactor: drop dead code with ERROR_TXINDEX_DISABLED_WHEN_GOV_ENABLED", merged in dashpay#7008) removed this check from `LoadChainstate`. Its commit message says governance is now force-disabled in this case and the "governance validation disabled" warning is shown instead. That is not the case: only `-prune` soft-sets `-disablegovernance`. An explicit `-txindex=0` on a non-pruned mainnet, testnet or devnet node that is not a masternode now starts quietly with governance enabled and no transaction index. That node then rejects every proposal it is asked to submit or receives from peers, and adds misbehavior score to each peer that relays one. ### Why this is a real problem Code path at c14104b: - The only parameter interaction that turns governance off is the `-prune` one: [src/init.cpp#L1083-L1091](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L1083-L1091). The txindex requirement applies only to masternodes ([src/init.cpp#L1398-L1400](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L1398-L1400)), the warning fires only when `-disablegovernance` is set ([src/init.cpp#L1442-L1447](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L1442-L1447)), and governance is enabled based solely on `-disablegovernance` ([src/init.cpp#L1732](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L1732)). - `CGovernanceObject::IsCollateralValid` finds the collateral only through `g_txindex` and fails with "Can't find collateral tx" when it is null: [src/governance/object.cpp#L796-L806](https://github.com/dashpay/dash/blob/c14104b172a2/src/governance/object.cpp#L796-L806). Proposals then fail `IsValidLocally` ([src/governance/object.cpp#L738-L741](https://github.com/dashpay/dash/blob/c14104b172a2/src/governance/object.cpp#L738-L741)), and `ProcessObject` returns false ([src/governance/governance.cpp#L266-L269](https://github.com/dashpay/dash/blob/c14104b172a2/src/governance/governance.cpp#L266-L269)). - For a proposal received from a peer, a false return from `ProcessObject` adds 20 misbehavior points to that peer: [src/governance/net_governance.cpp#L193-L198](https://github.com/dashpay/dash/blob/c14104b172a2/src/governance/net_governance.cpp#L193-L198). After five proposals the peer reaches the discouragement threshold and, unless it is a manual or `noban` connection, is disconnected and its address discouraged: [src/net_processing.cpp#L5833-L5869](https://github.com/dashpay/dash/blob/c14104b172a2/src/net_processing.cpp#L5833-L5869). The reproductions below ran against an unfixed `dashd`: Parts A and B against c14104b, Part C against the branch rebased on 8819507 with the `src/init.cpp` hunk reversed (nothing between those two commits touches C++). They are small `BitcoinTestFramework` scripts. Part A uses devnet because regtest is exempt from the check. The framework cannot mine the devnet genesis block, so Parts B and C show the effects on regtest, where `-txindex=0` with governance is allowed on every revision. The collateral lookup and peer scoring are the same on every network. <details> <summary>Part A (devnet): a node with <code>-txindex=0</code> starts with governance enabled and no warning</summary> ``` $ DASH_WORKTREE=<worktree> python3 repro_a_devnet_startup.py --configfile=<worktree>/test/config.ini TestFramework (INFO): Start a devnet node with -txindex=0 (governance not disabled) TestFramework (INFO): RESULT: dashd started; stderr: '' TestFramework (INFO): getindexinfo: {} TestFramework (INFO): debug.log: ... Command-line arg: txindex="0" TestFramework (INFO): debug.log: ... [flat-database.h:194] [Store] Verifying governance.dat format... TestFramework (INFO): debug.log: ... [flat-database.h:200] [Store] Writing info to governance.dat... TestFramework (INFO): debug.log: ... [flat-database.h:71] [CoreWrite] Governance Objects: 0 (Proposals: 0, Triggers: 0, Other: 0; Erased: 0) TestFramework (INFO): debug.log: ... [governance/governance.cpp:1105] [UpdatedBlockTip] [gobject] CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: 0 TestFramework (INFO): Start a devnet node with -txindex=0 -disablegovernance TestFramework (INFO): RESULT: dashd started TestFramework (INFO): RESULT: stderr was the governance-disabled warning TestFramework (INFO): Tests successful ``` </details> <details> <summary>Part B (regtest): with governance enabled and <code>-txindex=0</code>, a valid proposal is rejected; the same node accepts it with <code>-txindex=1</code></summary> ``` $ DASH_WORKTREE=<worktree> python3 repro_b_regtest_proposal.py --configfile=<worktree>/test/config.ini TestFramework (INFO): Node started with -txindex=0; getindexinfo: {} TestFramework (INFO): Collateral 246c979b734f002c8f7d1f472fe159f2a1541472e00fd44b6682134b36327da6 confirmations: 6 TestFramework (INFO): RESULT (-txindex=0): gobject submit rejected: Governance object is not valid - 6926f935aa79335ddac0cf52ca7665a15dc58cda80c89af9dcd22501a819fe63 - Invalid proposal collateral (-32603) TestFramework (INFO): debug.log: ... [governance/object.cpp:804] [IsCollateralValid] CGovernanceObject::IsCollateralValid -- Can't find collateral tx 246c979b734f002c8f7d1f472fe159f2a1541472e00fd44b6682134b36327da6 TestFramework (INFO): gobject count: {'objects_total': 0, 'proposals': 0, 'triggers': 0, 'other': 0, 'erased': 0, 'votes': 0} TestFramework (INFO): Restart the same node with -txindex=1 and submit the same proposal TestFramework (INFO): RESULT (-txindex=1): gobject submit accepted: 6926f935aa79335ddac0cf52ca7665a15dc58cda80c89af9dcd22501a819fe63 TestFramework (INFO): gobject count: {'objects_total': 1, 'proposals': 1, 'triggers': 0, 'other': 0, 'erased': 0, 'votes': 0} TestFramework (INFO): Tests successful ``` </details> <details> <summary>Part C (regtest): the <code>-txindex=0</code> node scores 20 misbehavior points per valid proposal relayed by a peer and disconnects it after five</summary> node0 (`-txindex=1`) connects to node1 (`-txindex=0`, governance enabled), so node1 sees node0 as an ordinary inbound peer. node0 then submits five valid proposals. Because node0 is on a local address, node1 disconnects it without adding the address to the discouragement list; a non-local peer is disconnected and discouraged. ``` $ DASH_WORKTREE=<worktree> python3 repro_c_peer_misbehaving.py --configfile=<worktree>/test/config.ini TestFramework (INFO): node0 getindexinfo: {'txindex': {'synced': True, 'best_block_height': 0}} TestFramework (INFO): node1 getindexinfo: {} TestFramework (INFO): node1 sees node0 as peer=0 inbound=True connection_type=inbound permissions=[] TestFramework (INFO): node0 accepted proposal b23bc414c1e0b465070af792c669ab4eabc529f2deee560ef4593458f01a499e TestFramework (INFO): node0 accepted proposal c81ae2dd9fe14ac2c13eea80ad52a87df586e7be6e317a6540b240a940678ab4 TestFramework (INFO): node0 accepted proposal e6a890d8da141a791fb5cb5b4dfbde92da3e423f7258b383cc3f8e9fa1620c7b TestFramework (INFO): node0 accepted proposal 9e7706395cfacfdbb9aad6e3762abece051a60e5db0ebbec219a9e1201284c54 TestFramework (INFO): node0 accepted proposal 499abad8125b978ba13ec4b6e19bb2bab9d9d63a5475f6e8c37bbc739e8780e3 TestFramework (INFO): node0 gobject count: {'objects_total': 5, 'proposals': 5, 'triggers': 0, 'other': 0, 'erased': 0, 'votes': 0} TestFramework (INFO): node1 gobject count: {'objects_total': 0, 'proposals': 0, 'triggers': 0, 'other': 0, 'erased': 0, 'votes': 0} TestFramework (INFO): node1 peers after relay: 0 TestFramework (INFO): node1 debug.log: ... [governance/governance.cpp:268] [ProcessObject] [gobject] MNGOVERNANCEOBJECT -- Governance object is invalid - Invalid proposal collateral TestFramework (INFO): node1 debug.log: ... [net_processing.cpp:1954] [Misbehaving] [net] Misbehaving: peer=0 (0 -> 20) TestFramework (INFO): node1 debug.log: ... [governance/governance.cpp:268] [ProcessObject] [gobject] MNGOVERNANCEOBJECT -- Governance object is invalid - Invalid proposal collateral TestFramework (INFO): node1 debug.log: ... [net_processing.cpp:1954] [Misbehaving] [net] Misbehaving: peer=0 (20 -> 40) TestFramework (INFO): node1 debug.log: ... [governance/governance.cpp:268] [ProcessObject] [gobject] MNGOVERNANCEOBJECT -- Governance object is invalid - Invalid proposal collateral TestFramework (INFO): node1 debug.log: ... [net_processing.cpp:1954] [Misbehaving] [net] Misbehaving: peer=0 (40 -> 60) TestFramework (INFO): node1 debug.log: ... [governance/governance.cpp:268] [ProcessObject] [gobject] MNGOVERNANCEOBJECT -- Governance object is invalid - Invalid proposal collateral TestFramework (INFO): node1 debug.log: ... [net_processing.cpp:1954] [Misbehaving] [net] Misbehaving: peer=0 (60 -> 80) TestFramework (INFO): node1 debug.log: ... [governance/governance.cpp:268] [ProcessObject] [gobject] MNGOVERNANCEOBJECT -- Governance object is invalid - Invalid proposal collateral TestFramework (INFO): node1 debug.log: ... [net_processing.cpp:1954] [Misbehaving] [net] Misbehaving: peer=0 (80 -> 100) DISCOURAGE THRESHOLD EXCEEDED TestFramework (INFO): node1 debug.log: ... [net_processing.cpp:5860] [MaybeDiscourageAndDisconnect] [net] Warning: disconnecting but not discouraging local peer 0! TestFramework (INFO): Tests successful ``` </details> ### Why it matters This only affects operators who explicitly set `-txindex=0` (the default is `1`) on a non-pruned node that is not a masternode. Nothing at startup tells them anything is wrong, and: - `gobject list`/`count`, the governance view in the GUI and proposal vote tracking stay empty or incomplete, and `gobject submit` fails with an unhelpful collateral error. - The node does not relay proposals. - Every honest peer that relays five or more proposals to it, which on mainnet includes any peer it syncs governance objects from, reaches the discouragement threshold and is disconnected; non-local peers are also discouraged. This applies to its outbound peers as well as inbound ones, so the misconfigured node keeps losing its own connections. Trigger and superblock validation do not use the transaction index, so block validation is not affected. The regression was introduced in v23.1.0. It is not a v24.0.0 release-candidate regression. ## What was done? Restored the check in `AppInitParameterInteraction`, next to the other governance and masternode parameter checks. Outside regtest, if governance validation is enabled and `-txindex` is disabled, `dashd` now fails to start with the original error message. Added `feature_governance_txindex_devnet.py`. It runs on devnet, because regtest is exempt, and checks two things: `-txindex=0` alone is refused with that error, and `-txindex=0 -disablegovernance` still starts with the governance-disabled warning. ### Why this is the correct minimal fix - It brings back the behavior from before 91129d4: the same condition, the same translatable message and the same regtest exemption. It is a parameter check, so it lives with the other parameter checks instead of going back into `LoadChainstate`. That needs no changes to the `LoadChainstate` signature or to `ChainstateLoadingError`, and it fails before the block index is loaded. - The regtest exemption is kept on purpose. Many regtest functional tests run with `-txindex=0` and governance enabled (`feature_reindex.py`, `rpc_blockchain.py`, `rpc_misc.py`, `feature_unsupported_utxo_db.py`), as they did before the check was removed. - The alternative is to do what the 91129d4 commit message describes: soft-set `-disablegovernance` when `-txindex=0`. That would change behavior instead of restoring it, and it would quietly turn governance off for an operator who may simply have forgotten the index. A clear startup error matches how the neighbouring incompatible combinations are handled, such as `-prune` with `-disablegovernance=false`. If reviewers prefer the soft-set, it is an equally small change. - Unchanged: `-prune` still soft-sets `-disablegovernance`/`-txindex=0` and keeps its own error for `-disablegovernance=false`. A masternode with `-txindex=0` still gets the existing "Masternode must have transaction index enabled" error first, because the masternode block runs before the new check. The peer scoring in `net_governance.cpp` is left as is; with the check restored, a node that would score honest peers for valid proposals can no longer start outside regtest. ## How Has This Been Tested? Built on macOS arm64 (`--enable-debug --without-gui --enable-werror`). - New regression test against the unfixed `dashd`: `test/functional/test_runner.py feature_governance_txindex_devnet.py` fails. The node does not exit and keeps running with `-txindex=0`: `dashd should have exited within 60s with expected error Error: Transaction index can't be disabled with governance validation enabled. ...`. - The same test with the fix: passes (2-3 s), before and after rebasing onto 8819507. - Reproduction Part A with the fix: `dashd refused to start (exit 1); stderr: Error: Transaction index can't be disabled with governance validation enabled. Either start with -disablegovernance command line switch or enable transaction index.` `-txindex=0 -disablegovernance` still starts with the warning. Parts B and C are regtest-only and are unaffected by the fix, as intended. - Related existing functional tests with the fix, all passed: `feature_config_args.py`, `feature_governance_objects.py`, `feature_masternode_params_devnet.py`, `p2p_connect_to_devnet.py`, `feature_reindex.py`, `rpc_blockchain.py` (v1 and v2 transport), `rpc_misc.py`, `wallet_import_rescan.py --legacy-wallet`. `feature_unsupported_utxo_db.py` was skipped locally (it needs previous-release binaries and Linux). - Lint: `lint-whitespace.py`, `lint-python.py`, `lint-files.py`, `lint-tests.py`, `lint-format-strings.py`, `lint-python-dead-code.py`, `lint-qt-translation.py` are clean. The C++ hunk is formatted with `contrib/devtools/clang-format-diff.py`. ## Breaking Changes Outside regtest, a node started with `-txindex=0` and without `-disablegovernance` now refuses to start, as it did before v23.1.0. Such a node could not validate proposals anyway. To keep running without the index, operators add `-disablegovernance`. The same applies to `-reindex-chainstate` runs, which already require `-txindex=0`. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: utACK d77f357 Tree-SHA512: 55ca460164adbe6e77ec7ba926e9d07516c20ee0162a86c2d98551ca47712090e03b08948929f40c7221ab9741b4ce8a6369b80a6800e4526f0efeaad147443c (cherry picked from commit 248a9d9)
…s start/timeout window dcff069 fix: only sign EHF signals inside the deployment's start/timeout window (pasta) Pull request description: ## Issue being fixed or feature implemented Masternodes create and mine MnEHF (DIP-0023) signals for deployments whose start/timeout window is not open, including deployments that are `NEVER_ACTIVE`. `CEHFSignalsHandler::UpdatedBlockTip()` asks the mnhf quorum to sign a signal for every `useEHF` deployment whose bit has not been mined yet, as long as `CChainParams::IsValidMNActivation()` returns true. That function is the validation check, and it is deliberately permissive: - A `NEVER_ACTIVE` start time is `-2`, so `timePast < nStartTime` is never true and the deployment counts as in range. - A deployment outside of its window is skipped with `continue`, and then the "unknown MnEHF fork bit" fallback returns true. Validation needs that fallback so that old signals on a reused bit stay valid on reindex (`mnhf_bit_reuse_reindex_tests`). So the producer signs regardless of the window. On mainnet today V24 is on bit 13 with `nStartTime = NEVER_ACTIVE`, so as soon as enough mainnet masternodes run this code they will sign and mine a bit-13 signal. Setting a future mainnet start time would not help either: that case goes through the fallback and is signed too. This has happened before. 1be3ac2 ("fix: move DEPLOYMENT_V24 to a fresh version bit") moved V24 off bit 12 because bit 12 "already has mined MNHF signals on mainnet and testnet from before V24 had a start time". That commit moved the bit but left the producer unchanged, so bit 13 would go the same way. ### Why this is a real problem Code path at c14104b (unchanged on current develop): - The producer only checks that the signal is missing and `IsValidMNActivation()`: [src/llmq/ehf_signals.cpp#L45](https://github.com/dashpay/dash/blob/c14104b172a2/src/llmq/ehf_signals.cpp#L45) - `IsValidMNActivation()` skips out-of-window deployments and then accepts unknown bits: [src/chainparams.cpp#L109-L131](https://github.com/dashpay/dash/blob/c14104b172a2/src/chainparams.cpp#L109-L131) - Mainnet V24 is bit 13, `NEVER_ACTIVE`: [src/chainparams.cpp#L212-L213](https://github.com/dashpay/dash/blob/c14104b172a2/src/chainparams.cpp#L212-L213) - Validation uses the same check, so the signal is accepted into the mempool and blocks: [src/evo/mnhftx.cpp#L124-L126](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/mnhftx.cpp#L124-L126), [src/evo/mnhftx.cpp#L237-L240](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/mnhftx.cpp#L237-L240) - A mined signal only expires if its MTP is below a positive `nStartTime`, so nodes with `NEVER_ACTIVE` keep it forever and reject any later signal for that bit as `bad-mnhf-duplicate`: [src/evo/mnhftx.cpp#L80](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/mnhftx.cpp#L80), [src/evo/mnhftx.cpp#L232-L235](https://github.com/dashpay/dash/blob/c14104b172a2/src/evo/mnhftx.cpp#L232-L235) Reproduction: the updated `feature_mnehf.py` from this PR, run against a `dashd` without the `src/llmq/ehf_signals.cpp` change. The test starts the `testdummy` deployment (bit 28) as `NEVER_ACTIVE` (`-vbparams=testdummy:-2:...`), mines two blocks and checks the masternode's log. The unpatched masternode tries to sign it on every new tip: <details> <summary>feature_mnehf.py against unpatched dashd</summary> ``` TestFramework (INFO): Check that masternodes do not sign testdummy while it is NEVER_ACTIVE AssertionError: [node 1] Unexpected message "bit=28 at height" partially matches log: - ... [trySignEHFSignal] [ehf] CEHFSignalsHandler::trySignEHFSignal: already signed bit=28 at height=175 id=a0eee872d7d3170dd20d5c5e8380c92b3aa887da5f63d8033289fafa35a90691 ``` </details> ### Why it matters If v24.0.0 ships like this, upgraded mainnet masternodes will mine a bit-13 signal long before V24 has a start time. Nodes whose V24 start time is still `NEVER_ACTIVE` (v24.0.0 itself) never expire that signal, so when the real signal is mined after a later release sets the start time, those nodes reject that block as `bad-mnhf-duplicate`. If the start time were instead set at or before the early signal, the early signal would count toward activation. In both cases the bit cannot be used cleanly any more, which is what already happened to bit 12. Nothing outside the masternode set can trigger this. It happens on its own once the quorum runs the code. This should also go to v24.0.x: v24.0.0-rc.1 and rc.2 carry V24 on bit 13 with mainnet `NEVER_ACTIVE` and the same producer code. ## What was done? In `CEHFSignalsHandler::UpdatedBlockTip()`, a deployment is now skipped unless its own signalling window is open at the tip MTP: `nStartTime != NEVER_ACTIVE && nStartTime <= MTP && MTP <= nTimeout`. The existing "not mined yet" and `IsValidMNActivation()` conditions are kept as they were. `feature_mnehf.py` gets a new first step, `test_no_signing_outside_window()`, which runs right after the first quorum is mined and before the existing testdummy activation flow: - `testdummy` starts as `NEVER_ACTIVE`, then the nodes are restarted with a window that has already timed out (`0:1`) and one that starts 10 minutes in the future. In each case two blocks are mined and the test checks that the masternode logged no signing attempt for bit 28 and that its mempool has no bit-28 signal. - Mocktime is then moved past the start time and the test waits until the masternode signs `testdummy`, so the negative checks are not passing just because the masternode cannot sign. - The rest of the test is unchanged. It uses `testdummy` rather than `v24`, so it keeps working after v24 is buried. ### Why this is the correct minimal fix The wrong decision is made in the producer, so the fix goes there. `IsValidMNActivation()` cannot be tightened instead. It is the validation check, and its fallback for out-of-window and unknown bits is what keeps old signals valid on reindex after a bit is reused (`mnhf_bit_reuse_reindex_tests`). Changing it would make nodes disagree about blocks they already accepted. `CheckMNHFTx()` and `CMNHFManager::ProcessBlock()` are not touched, so consensus is unchanged: a signal mined by an old node is still accepted exactly as before. The `NEVER_ACTIVE` check has to be explicit. It is the mainnet case, and the plain range check treats `-2` as an already-passed start time. Deployments with `ALWAYS_ACTIVE` (`-1`) or a past start and `NO_TIMEOUT` still pass, so regtest/devnet deployments (start 0) and testnet V24 (its window opened 2026-09-24) are signed as before. Checking the tip MTP is enough. The signal is judged later by the MTP of the block that includes it, and that MTP is greater than or equal to the tip's. I did not add a re-check in `HandleNewRecoveredSig()`. It only submits signals for request ids that `trySignEHFSignal()` registered, and those are only registered while the window is open. ## How Has This Been Tested? macOS arm64, `--enable-debug` build. - `test/functional/feature_mnehf.py` against a `dashd` without the `src/llmq/ehf_signals.cpp` change: fails at the NEVER_ACTIVE check (output above). - `test/functional/feature_mnehf.py` with this PR: passes. - Each condition of the new check was removed in turn, and every variant fails in the step meant to catch it: - Without the `NEVER_ACTIVE` check, the "while it is NEVER_ACTIVE" step fails. - Without `MTP <= nTimeout`, the "after its timeout" step fails. - Without `nStartTime <= MTP`, the "before its start time" step fails. - `test/functional/feature_asset_locks.py` with this PR: passes. It activates regtest v24 (start 0) through masternode-signed EHF. - `src/test/test_dash --run_test=evo_mnhf_tests` (includes `mnhf_bit_reuse_reindex_tests`): passes. - `test/lint/lint-python.py`, `test/lint/lint-whitespace.py` and `contrib/devtools/clang-format-diff.py` on the diff: clean. ## Breaking Changes None. Consensus validation is unchanged. Masternodes no longer create signals for deployments whose window is not open. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: utACK dcff069 Tree-SHA512: 627f886a0b45e5ce807e7f306fa39606dd30bd24eb119479f9810929f3dad1c06005eeb237f5c0096d7d8835d35e0017f79fa831f800c22f5f63d8ba1b19d508 (cherry picked from commit 3a22d16)
…e object lock 96767a4 fix: read deletion flags before deletion time in CheckAndRemove (pasta) ebb27a3 fix: guard cached governance object flags with the object lock (pasta) 94df4f1 refactor: give CGovernanceObjectVoteFile a copy assignment operator (pasta) 059bf00 refactor: require cs in CGovernanceObject::CountMatchingVotes (pasta) Pull request description: ## Issue being fixed or feature implemented `gobject get` can report wrong values for `fCachedFunding`, `fCachedValid` and `fCachedEndorsed` while the node is recomputing them. For example, a proposal that has reached the funding threshold can come back as `fCachedFunding: false`, and a proposal the network voted invalid can come back as `fCachedValid: true`. The reads are also a data race on plain `bool` members. That is undefined behavior, and ThreadSanitizer reports it (see below). There are two causes: 1. The cached flags (`fCachedFunding`, `fCachedValid`, `fCachedDelete`, `fCachedEndorsed`, `fDirtyCache`) have no lock. Readers take no lock: - `CGovernanceObject::GetStateJson()`, used by `gobject get`. - The copy constructor, used by `gobject list`, `gobject diff` and the Qt proposal list through `GetAllNewerThan()`. No single outer lock covers the writers either: - `CGovernanceManager::CheckAndRemove()` holds `cs_main` + `cs_store`. - `SuperblockManager::IsSuperblockTriggered()` holds only `cs_sb`. It is called from block-template creation and block validation, and also from the `masternode winners` RPC without `cs_main`. - The vote path writes `fDirtyCache` while holding the object's own `cs`. 2. `CGovernanceObject::UpdateSentinelVariables()` first resets `fCachedFunding`/`fCachedEndorsed` to `false` and `fCachedValid` to `true`. It then counts votes, and only after that writes the real values. A reader that runs during this window sees the defaults instead of the old or the new result. There is also a related ordering bug. `fCachedDelete` was set before the lock that sets `nDeletionTime`. `CheckAndRemove()` reads `GetDeletionTime()` and then `IsSetCachedDelete()` in separate calls. If `IsSuperblockTriggered()` marks a trigger for deletion in between, `CheckAndRemove()` sees a set delete flag with a zero deletion time, computes `now - 0`, and erases the object immediately instead of after `GOVERNANCE_DELETION_DELAY`. Block validation itself is not affected. Its `IsSuperblockTriggered()` call holds `cs_main` and `cs_sb`, which already serializes it against the other writers. The visible impact is on RPC/GUI output, plus undefined behavior under the C++ memory model. ### Reproduction A throwaway test case (not part of this PR) in `governance_vote_processing_tests` reuses its single-masternode fixture. The masternode votes YES on funding and NO on validity, so with `nGovernanceMinQuorum=1` the correct state never changes: `fCachedFunding=true`, `fCachedValid=false`. One thread calls `UpdateSentinelVariables()` in a loop, as `CheckAndRemove()` / `IsSuperblockTriggered()` do. The main thread calls `GetStateJson()` for 3 seconds, as `gobject get` does, and counts reads that disagree with the correct state. <details> <summary>Test case</summary> ```cpp BOOST_AUTO_TEST_CASE(proof_cached_flags_read_during_recompute) { auto& govman = *m_node.govman; const CGovernanceObject proposal{MakeProposal(uint256::ONE)}; const uint256 parent_hash{proposal.GetHash()}; govman.AddGovernanceObjectForTesting(proposal); auto obj = govman.FindGovernanceObject(parent_hash); BOOST_REQUIRE(obj != nullptr); CGovernanceVote funding{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)}; SignWithVotingKey(funding, mn_voting_key); CGovernanceVote invalid{MakeVote(parent_hash, VOTE_SIGNAL_VALID, VOTE_OUTCOME_NO)}; SignWithOperatorKey(invalid, mn_operator_key); CGovernanceException exception; uint256 hash_to_request; BOOST_REQUIRE(govman.ProcessVote(funding, exception, hash_to_request)); BOOST_REQUIRE(govman.ProcessVote(invalid, exception, hash_to_request)); const auto mn_list{tip_mn_list()}; obj->UpdateSentinelVariables(mn_list); BOOST_REQUIRE(obj->IsSetCachedFunding()); BOOST_REQUIRE(!obj->IsSetCachedValid()); std::atomic<bool> stop{false}; std::thread writer([&] { while (!stop) obj->UpdateSentinelVariables(mn_list); }); int reads{0}, wrong_funding{0}, wrong_valid{0}; const auto deadline{std::chrono::steady_clock::now() + 3s}; while (std::chrono::steady_clock::now() < deadline) { const UniValue state{obj->GetStateJson(*m_node.chainman, mn_list, /*local_valid_key=*/"fLocalValidity")}; ++reads; if (!state["fCachedFunding"].get_bool()) ++wrong_funding; if (state["fCachedValid"].get_bool()) ++wrong_valid; } stop = true; writer.join(); BOOST_TEST_MESSAGE(strprintf("GetStateJson reads=%d fCachedFunding=false seen=%d fCachedValid=true seen=%d", reads, wrong_funding, wrong_valid)); BOOST_CHECK_EQUAL(wrong_funding, 0); BOOST_CHECK_EQUAL(wrong_valid, 0); } ``` </details> Results against the unfixed tree (c14104b): ``` # regular debug build GetStateJson reads=198415 fCachedFunding=false seen=76428 fCachedValid=true seen=193173 # --with-sanitizers=thread GetStateJson reads=29726 fCachedFunding=false seen=0 fCachedValid=true seen=29726 WARNING: ThreadSanitizer: data race Write of size 1 ... by thread T11: #0 CGovernanceObject::UpdateSentinelVariables(CDeterministicMNList const&) object.cpp:996 Previous read of size 1 ... by main thread: #0 CGovernanceObject::IsSetCachedValid() const object.h:196 #1 CGovernanceObject::GetStateJson(...) const object.cpp:617 ``` ## What was done? The flags are now protected by the object's existing `cs` mutex, the same lock that already guards `fExpired`, `nDeletionTime` and the vote maps. Clang's thread-safety analysis enforces it. 1. **`refactor: require cs in CGovernanceObject::CountMatchingVotes`.** `CountMatchingVotes()` becomes a private helper with `EXCLUSIVE_LOCKS_REQUIRED(cs)`. The public `Get{Yes,No,Abstain}Count()` wrappers take the lock. No behavior change. This lets one critical section count several signals. 2. **`refactor: give CGovernanceObjectVoteFile a copy assignment operator`.** `CGovernanceObjectVoteFile` has a copy constructor that rebuilds `mapVoteIndex` (list iterators) against its own list, but no copy assignment. The implicit one can't be instantiated because `CGovernanceVote` is not assignable. This commit adds one with the same rebuild semantics, so the copy constructor in commit 3 can copy the vote file while holding the source's lock. A unit test checks that a copy-assigned file is still usable after its source is destroyed. 3. **`fix: guard cached governance object flags with the object lock`.** - The five flags are `GUARDED_BY(cs)`, and the `IsSetCached*()`/`IsSetDirtyCache()` getters take the lock, like `IsSetExpired()`. - `UpdateSentinelVariables()` counts the votes and writes every flag inside one `LOCK(cs)`. Each flag is assigned its computed value directly, with no reset to a default first. This means: - readers see either the previous result or the new one; - two recomputes running at the same time can't interleave flag by flag; - `fDirtyCache = false` is ordered with the vote counts under the same lock that vote writers already hold, so a vote arriving during a recompute still marks the object dirty. - `PrepareDeletion()` and `UpdateSentinelVariables()` share a private `MarkForDeletion()` helper that sets `fCachedDelete` and `nDeletionTime` together, so `fCachedDelete` implies a non-zero deletion time. - `GetStateJson()` reads all four flags under one lock, so `gobject get` returns a consistent snapshot. - The copy constructor copies all `cs`-guarded state while holding `other.cs`. - The computed values are the same as before. 4. **`fix: read deletion flags before deletion time in CheckAndRemove`.** `CheckAndRemove()` now reads `IsSetCachedDelete()`/`IsSetExpired()` before `GetDeletionTime()`. - `fCachedDelete` is never cleared, and it now changes together with `nDeletionTime`, so a set delete flag is always paired with its deletion time. - `fExpired` is only set by `SuperblockManager::Clean()`. That runs from `CheckAndRemove()` itself and stamps the deletion time before this loop. ### Locking notes - No code holds a governance object's `cs` while calling the getters, `UpdateSentinelVariables()`, `PrepareDeletion()`, `GetStateJson()`, or the copy constructor. The external `cs` holders only touch the vote file. - `UpdateSentinelVariables()` and `GetStateJson()` take no other lock while holding `cs`. - The `cs_main`/`cs_store`/`cs_sb` → `obj.cs` edges already existed through `GetYesCount()` and the old `UpdateSentinelVariables()`. No new lock-order edges are introduced. ### Intentionally left unchanged `fCachedLocalValidity`, `strLocalValidityError` and `fUnparsable` follow the same unguarded pattern. However, `GetStateJson()` recomputes local validity instead of reading the cache, and I found no concurrent reader of those fields, so they are out of scope here. ## How Has This Been Tested? macOS arm64, Apple clang. Built with depends using `--disable-hardening --enable-debug --enable-crash-hooks --enable-stacktraces --enable-suppress-external-warnings --without-gui --enable-tests --disable-bench --disable-fuzz-binary --enable-werror`. This enables `-Wthread-safety` as an error. - **Each commit builds on its own.** - **Thread-safety annotations are enforced.** As a mutation check, I removed the `LOCK(cs)` in `GetStateJson()`. The build then fails with `passing variable 'fCachedValid' by reference requires holding mutex 'cs' [-Werror,-Wthread-safety-reference]`. - **Reproduction above with this PR:** 0 wrong reads across three runs (148887, 155774 and 184427 reads). - **ThreadSanitizer build (`--with-sanitizers=thread`) with this PR.** The reproduction ran with a second reader added that also copy-constructs the object each iteration (the `gobject list` path). Result: no ThreadSanitizer warnings and 0 wrong reads. The same binary built from the unfixed tree reports the data race shown above. - **Regression test status.** The reproduction is not included as a regression test. It is a timing-dependent multithreaded test that runs for several seconds, and this race can't be checked deterministically in a unit test. The `CheckAndRemove()` ordering fix has the same limitation; I verified it by reviewing every writer of `fCachedDelete`, `fExpired` and `nDeletionTime`. - **New unit test:** `governance_vote_sync_tests/copy_assigned_vote_file_indexes_its_own_votes`. - `src/test/test_dash --run_test='governance*'`: `*** No errors detected`. - `test/functional/test_runner.py feature_governance.py feature_governance_cl.py feature_governance_objects.py`: all passed. - `git diff -U0 develop | contrib/devtools/clang-format-diff.py -p1` is clean except for the `LogPrint` in `CheckAndRemove()`. That call keeps its existing layout because `test/lint/lint-logs.py` needs the format string's `\n"` on the same line as `LogPrint(`. `lint-logs`, `lint-format-strings`, `lint-includes`, `lint-whitespace`, `lint-assertions`, `lint-include-guards`, `lint-locale-dependence` and `lint-circular-dependencies` pass locally. ## Breaking Changes None. RPC output fields and values are unchanged. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 748663c6af9c62b7f5a15083667fc5ed44907c896f0c7567aab0b08ccb9d908114b6e23c179d88773fb69e190fcb6770170a8f05469636337b67f480a1a7c2a9 (cherry picked from commit 2ca083a)
…allet if migration exited early, keep mixed watchonly txs) 17b0589 test: test migration of tx with both spendable and watchonly (pasta) 03821b7 fix(wallet): keep txs that belong to both watchonly and migrated wallets (pasta) c305aab test: make sure that migration test does not rescan on reloading (pasta) 1f9bf08 fix(wallet): reload the wallet if migration exited early (pasta) Pull request description: ## Issue being fixed or feature implemented `migratewallet` (and the GUI "Migrate wallet" action, which calls the same `MigrateLegacyToDescriptor()`) has two bugs that upstream fixed in bitcoin#28868. That PR is listed as outstanding in the "next batch" section of dashpay#7277. 1. **A failed migration leaves the wallet unloaded.** If the wallet is loaded, `MigrateLegacyToDescriptor()` unloads it first and then runs its checks. Several of those checks return early without loading it again: the wallet is already a descriptor wallet, the backup could not be written, or the passphrase is missing or wrong. After any of these errors the wallet is gone from `listwallets`, and RPC calls to it fail with `Requested wallet does not exist or is not loaded` until the user runs `loadwallet` manually. Mistyping the passphrase once on an encrypted legacy wallet is enough to trigger this. 2. **A transaction with outputs in both resulting wallets only stays in the migrated wallet.** During migration, `ApplyMigrationData()` only offers a transaction to the new `<name>_watchonly` wallet when the migrated wallet does *not* claim it. If one output pays a spendable address and another pays an imported watch-only address, the transaction stays in the migrated wallet and is never copied to `<name>_watchonly`. That wallet is created at the chain tip, so it does not rescan, and its history and balance silently omit that transaction. This is the watch-only bug mentioned in dashpay#7275. ### Why this is a real problem Code path at c14104b: - The loaded wallet is unloaded before any validation: [wallet.cpp#L5006-L5012](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/wallet.cpp#L5006-L5012) - These early returns do not reload it: "already a descriptor wallet" [#L5033-L5035](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/wallet.cpp#L5033-L5035), backup failure [#L5055-L5057](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/wallet.cpp#L5055-L5057), missing or wrong passphrase [#L5062-L5075](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/wallet.cpp#L5062-L5075) - Watch-only copying only runs when `!IsMine(tx) && !IsFromMe(tx)`: [wallet.cpp#L4759-L4781](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/wallet.cpp#L4759-L4781) I reproduced all three cases against an unfixed `dashd` built from c14104b, using a functional-test-style script on regtest: <details><summary>Reproduction on c14104b (unfixed)</summary> Steps: 1. `createwallet desc` (descriptor), then `migratewallet desc`, then `listwallets` 2. `createwallet enc descriptors=false`, then `encryptwallet pass`, then `migratewallet enc badpass`, then `listwallets` and `getwalletinfo` on `enc` 3. `createwallet imports descriptors=false`, then `importaddress <addr from default wallet>`. From the default wallet, `send` one output to an `imports` address and one to the imported address, then mine a block. Then `migratewallet` and `gettransaction <txid>` on `imports_watchonly` ``` TestFramework (INFO): Case 1: migratewallet on a loaded descriptor wallet TestFramework (INFO): listwallets before: ['default_wallet', 'desc'] TestFramework (INFO): listwallets after: ['default_wallet'] TestFramework (INFO): Case 2: migratewallet on a loaded encrypted legacy wallet with a wrong passphrase TestFramework (INFO): listwallets before: ['default_wallet', 'enc'] TestFramework (INFO): listwallets after: ['default_wallet'] TestFramework (INFO): getwalletinfo on 'enc' -> RPC error -18 (wallet not loaded) TestFramework (INFO): Case 3: tx with a spendable output and a watch-only output TestFramework (INFO): imports_watchonly.gettransaction(cfbc39814ffc474a35bae511c660de7a759653f3a362639f08e38fe2bd6382ef) -> Invalid or non-wallet transaction id (-5) TestFramework (INFO): RESULT descriptor wallet still loaded: False TestFramework (INFO): RESULT encrypted wallet still loaded: False TestFramework (INFO): RESULT mixed tx present in watchonly wallet: False ``` With this branch, the same script reports `listwallets after: ['default_wallet', 'desc']` and `['default_wallet', 'desc', 'enc']`, and all three results are `True`. </details> The upstream tests backported here also fail on the unfixed code (see "How Has This Been Tested?"). ### Why it matters Neither bug loses funds or keys. The first one is a usability trap in a v24 feature: a typo in the passphrase makes the wallet disappear from the node, the GUI and RPC clients, with no hint that a `loadwallet` is needed. The second one leaves the post-migration watch-only wallet with incomplete history and balance, and nothing reports it. Users migrating real wallets have already hit it (see dashpay#7275). ## What was done? Backport of bitcoin#28868, minus one commit: - bitcoin/bitcoin commit `78ba0e6748d2` "wallet: Reload the wallet if migration exited early": remember whether the wallet was loaded (`was_loaded`), define the `reload_wallet` helper before the early exits, and reload the wallet on the descriptor-wallet, backup-failure and passphrase-failure exits. As upstream does, the passphrase unlock moves out of the `LOCK(cs_wallet)` scope so the reload does not happen while the wallet lock is held. - bitcoin/bitcoin commit `71cb28ea8cb5` "test: Make sure that migration test does not rescan on reloading": adds the `migrate_wallet()` helper, which reloads the wallet before migrating and checks that migration does not log `Rescanning`. Because the helper calls `getwalletinfo` on the wallet first, it fails if an earlier failed `migratewallet` left the wallet unloaded. The Dash-only `test_wallet_name_with_slashes` case also goes through the helper. - bitcoin/bitcoin commit `c62a8d03a862` "wallet: Keep txs that belong to both watchonly and migrated wallets": every transaction is offered to the watch-only wallet. It is removed from the migrated wallet only if the migrated wallet does not also own it. - bitcoin/bitcoin commit `4da76ca24725` "test: Test migration of tx with both spendable and watchonly" Dash adaptations and omitted hunks: - **Omitted bitcoin/bitcoin commit `9332c7edda79`** "wallet: Write bestblock to watchonly and solvable wallets". Upstream needs it because, after bitcoin#28609, the watch-only and solvable wallets are created in an empty context and reloaded at the end of migration, and without a best block record they rescan on that reload. Dash has not backported bitcoin#28609. Here those wallets are created with `CreateWallet(context, ...)`, which already writes the chain tip as their best block on first run, and they are not reloaded during migration. The commit fixes nothing on Dash today and belongs with a bitcoin#28609 backport. - 78ba0e6: Dash's "already a descriptor wallet" check (`!GetLegacyScriptPubKeyMan()`) and Dash's backup filename logic are kept as they are. The upstream hunk that moves `reload_wallet` out of the success branch does not apply, because that helper came from bitcoin#28609. The success path keeps its existing direct reload. - c62a8d0: the watch-only copy still uses `AddToWallet()`, because bitcoin#28125 (`LoadToWallet()`/`CopyFrom()` and the shared watch-only `WalletBatch`) is not backported. Only the ownership logic changes, exactly as upstream. - 71cb28e: the upstream hunk that routes `test_addressbook` through `self.migrate_wallet()` is not carried over. That scenario was added by bitcoin#28038, which Dash has not backported, so there is no `test_addressbook` to change. Every other scenario that upstream routes through the helper does so here as well. - 4da76ca: applied to Dash's existing `test_other_watchonly`. The upstream context lines that check copied tx metadata come from bitcoin#28125 and are not part of this change. ### Why this is the correct minimal fix The wallet goes missing because the unload happens before the checks, while only the success path reloads it. Reloading on each early exit is the upstream fix, and every exit taken after the unload but before any database change is covered. Moving the checks ahead of the unload would mean running the backup and the passphrase check against the loaded instance, which is a larger restructuring than this fix needs. As upstream does, no reload is attempted when `MakeWalletDatabase()`/`CWallet::Create()` fail (the same open would fail again) or when `MigrateToSQLite()` fails (by then the original BDB file may already have been removed, and the failed-migration restore path does not run). The `.legacy.bak` file written before a wrong-passphrase exit is also kept, as upstream does. The watch-only change is the upstream one-condition fix, and the rest of bitcoin#28609/bitcoin#28125 is not needed for either bug. The `migratewallet` RPC first shipped in v24.0.0-rc.1 (dashpay#7275/dashpay#7277 are on v24.0.x), so this is a candidate for v24.0.x. ## How Has This Been Tested? macOS arm64, `--enable-debug --enable-werror`, built `dashd`/`dash-cli`/`dash-wallet` at each step. - Reproduction script above: on c14104b it fails with all three bugs present. With this branch it passes. - `test/functional/wallet_migration.py` with only the 71cb28e test change, against the unfixed c14104b binary: **fails** in `test_encrypted`. After the expected wrong-passphrase errors, `migrate_wallet()` → `getwalletinfo` raises `Requested wallet does not exist or is not loaded (-18)`. All earlier migrations pass the no-`Rescanning` check. - Same test after 78ba0e6: passes. - `wallet_migration.py` with the 4da76ca test change, against a binary that has 78ba0e6 but not c62a8d0: **fails** at `watchonly.gettransaction(watchonly_spendable_txid)` with `Invalid or non-wallet transaction id (-5)`. The new listtransactions counts before migration (6) and in the migrated wallet (2) already pass at that point. - Full branch: `test/functional/test_runner.py wallet_migration.py` passes. - `test/lint/lint-python.py` and `test/lint/lint-whitespace.py`: clean. `clang-format-diff` only suggests re-wrapping the passphrase error strings. They keep upstream's layout to stay 1:1, and `wallet.cpp` is not in `non-backported.txt`. ## Breaking Changes None. After a failed `migratewallet`, a wallet that was loaded beforehand is now loaded again, as it was before the call. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: utACK 17b0589 Tree-SHA512: c245f96c7a9a5922ae1ffad9e5c0a3a6eab3494949239cc1d94fab5cdab090239cc13fa4ea3d0a4cd017ef36657aac834d6dd0f84d2bbfd482430ed639e8b1c0 (cherry picked from commit 233aefd) Conflict in test/functional/wallet_migration.py: test_conflict_txs() comes from bitcoin#28542 (dashpay#7762), which v24.0.x does not have, so it is left out along with its migratewallet() -> migrate_wallet() change.
…ind is given and correct the bind release note 5f40d56 partial Merge bitcoin#36170: net: require a dedicated bind for automatic Tor (Konstantin Akimov) 72b3c71 docs: correct when the implicit onion bind is added in release notes (pasta) 5bb8eb3 fix(net): keep a dedicated onion listener when -bind is given (pasta) Pull request description: ## Issue being fixed or feature implemented The backport of bitcoin#22729 (dashpay#7300) changed what happens when a node is started with an explicit `-bind=<addr>` but no `-bind=...=onion`. Before, Dash Core always added the dedicated onion listener `127.0.0.1:9996` (`19996` testnet, `19896` regtest) and pointed the automatically created Tor onion service (`-listenonion`, on by default) at it. Now it binds no onion listener and points the onion service at the first `-bind` address. That listener is not in `onion_binds`, so every peer Tor forwards to it is classified by its source address: Tor's loopback address, or the node's own IP when `-bind` is a routable address. It is not classified as an onion peer. As a result, a node that runs an onion service next to a clearnet address (`-externalip`, or a discovered/bound routable address) advertises its clearnet address to peers that reached it through its `.onion` address. That links the two identities, which the privacy-network check in `GetLocal()` is meant to prevent. Those peers also lose the rest of the onion-specific handling, such as `getpeerinfo` `network` and eviction protection for onion peers. The release note added for dashpay#7300 has a related problem. It says `-listenonion=0` drops the implicit onion bind, but it never did: the bind is added before `-listenonion` is looked at. An operator whose `127.0.0.1:9996` is taken who follows the note still gets a node that refuses to start. Both changes are only in v24.0.0-rc.1 and rc.2. ### Why this is a real problem Code path at c14104b: - [`src/init.cpp#L2678-L2695`](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L2678-L2695): with `vBinds` non-empty and no `=onion` bind, `onion_service_target = connOptions.vBinds.front()` ([L2682](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L2682)). It is not pushed into `onion_binds`, and `StartTorControl(onion_service_target)` still runs ([L2694](https://github.com/dashpay/dash/blob/c14104b172a2/src/init.cpp#L2694)). - [`src/net.h#L1291`](https://github.com/dashpay/dash/blob/c14104b172a2/src/net.h#L1291) copies only `onion_binds` into `m_onion_binds`. [`src/net.cpp#L1977`](https://github.com/dashpay/dash/blob/c14104b172a2/src/net.cpp#L1977) sets `inbound_onion` only for listeners in `m_onion_binds`. - [`src/net.cpp#L584-L592`](https://github.com/dashpay/dash/blob/c14104b172a2/src/net.cpp#L584-L592): without `m_inbound_onion`, `ConnectedThroughNetwork()` and `IsConnectedThroughPrivacyNet()` fall back to the peer's source address. The privacy filter in `GetLocal()` ([L186-L193](https://github.com/dashpay/dash/blob/c14104b172a2/src/net.cpp#L186-L193)) then skips the node's own onion address for that peer and lets the clearnet one through. I reproduced this on the unfixed build with a small functional-test-style script. A minimal fake Tor control port accepts `PROTOCOLINFO`/`AUTHENTICATE`/`ADD_ONION` and records the target dashd asks Tor to forward to. A P2P peer then connects to exactly that target, as Tor does for every inbound onion connection, and sends `getaddr`. The node runs with `-bind=127.0.0.1:<port> -listenonion=1 -externalip=1.2.3.4`. Unfixed (c14104b): ``` Tor control commands received: ['PROTOCOLINFO 1', 'AUTHENTICATE', 'GETINFO net/listeners/socks', 'ADD_ONION NEW:ED25519-V3 Port=19899,127.0.0.1:17626'] onion-service target handed to Tor: 127.0.0.1:17626 getnetworkinfo localaddresses: [{'address': '1.2.3.4', 'port': 17626, 'score': 4}, {'address': 'pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion', 'port': 19899, 'score': 4}] Connecting a peer to 127.0.0.1:17626, as Tor would for an inbound onion connection getpeerinfo: addr=127.0.0.1:63442 addrbind=127.0.0.1:17626 network=not_publicly_routable addresses self-advertised to this peer: ['1.2.3.4:17626'] ``` The onion service forwards to the clearnet listener, the forwarded peer is `not_publicly_routable` instead of `onion`, and it is sent the node's clearnet address `1.2.3.4`. In the fixed run the list is empty for two reasons. The privacy filter withholds `1.2.3.4` from an onion peer, and the test `P2PInterface` never sends `sendaddrv2`, so the node cannot send it the v3 onion address either. The fixed run also prints a `wait_until() failed` line after the 10 s wait for addresses; that line is left out of the excerpt above. With this PR: ``` Tor control commands received: ['PROTOCOLINFO 1', 'AUTHENTICATE', 'GETINFO net/listeners/socks', 'ADD_ONION NEW:ED25519-V3 Port=19899,127.0.0.1:19896'] onion-service target handed to Tor: 127.0.0.1:19896 getnetworkinfo localaddresses: [{'address': '1.2.3.4', 'port': 18132, 'score': 4}, {'address': 'pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion', 'port': 19899, 'score': 4}] Connecting a peer to 127.0.0.1:19896, as Tor would for an inbound onion connection getpeerinfo: addr=127.0.0.1:50581 addrbind=127.0.0.1:19896 network=onion addresses self-advertised to this peer: [] ``` <details><summary>Reproduction script (run with <code>WORKTREE=<src root> python3 repro_tor_target.py --configfile=<src root>/test/config.ini</code>)</summary> ```python #!/usr/bin/env python3 """Reproduction: with an explicit -bind (no =onion) and -listenonion=1, which listener does dashd hand to Tor as the onion-service target, and how is a peer that Tor forwards to that target classified? A minimal fake Tor control port answers PROTOCOLINFO/AUTHENTICATE/ADD_ONION and records the target from the ADD_ONION command. The test then connects a P2P peer to exactly that target (what Tor does for every inbound onion connection), prints getpeerinfo's network field, sends getaddr and prints the addresses the node self-advertises to that peer. Run from anywhere: python3 repro_tor_target.py --configfile=<worktree>/test/config.ini """ import os import socket import sys import threading WORKTREE = os.environ["WORKTREE"] sys.path.insert(0, os.path.join(WORKTREE, "test", "functional")) from test_framework.messages import msg_getaddr # noqa: E402 from test_framework.p2p import P2PInterface # noqa: E402 from test_framework.test_framework import BitcoinTestFramework # noqa: E402 from test_framework.util import p2p_port # noqa: E402 ONION_ID = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd" EXTERNAL_IP = "1.2.3.4" class FakeTorControl: def __init__(self): self.srv = socket.socket(socket.AF_INET, socket.SOCK_STREAM) self.srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) self.srv.bind(("127.0.0.1", 0)) self.srv.listen(1) self.port = self.srv.getsockname()[1] self.commands = [] self.target = None threading.Thread(target=self._serve, daemon=True).start() def _serve(self): conn, _ = self.srv.accept() f = conn.makefile("rwb", buffering=0) while True: line = f.readline() if not line: return cmd = line.decode().strip() self.commands.append(cmd) if cmd.startswith("PROTOCOLINFO"): f.write(b'250-PROTOCOLINFO 1\r\n250-AUTH METHODS=NULL\r\n250-VERSION Tor="0.4.8.12"\r\n250 OK\r\n') elif cmd.startswith("ADD_ONION"): # ADD_ONION NEW:ED25519-V3 Port=<virtport>,<target> self.target = cmd.split("Port=")[1].split(",", 1)[1] f.write(f"250-ServiceID={ONION_ID}\r\n250-PrivateKey=ED25519-V3:AAAA\r\n250 OK\r\n".encode()) else: f.write(b"250 OK\r\n") class AddrCollector(P2PInterface): def __init__(self): super().__init__() self.addrs = [] def on_addr(self, message): self.addrs += [f"{a.ip}:{a.port}" for a in message.addrs] def on_addrv2(self, message): self.addrs += [f"{a.ip}:{a.port}" for a in message.addrs] class ReproTorTarget(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.bind_to_localhost_only = False self.num_nodes = 1 def setup_network(self): self.tor = FakeTorControl() self.extra_args = [[ f"-bind=127.0.0.1:{p2p_port(0)}", "-listenonion=1", f"-torcontrol=127.0.0.1:{self.tor.port}", f"-externalip={EXTERNAL_IP}", "-debug=tor", ]] self.setup_nodes() def run_test(self): node = self.nodes[0] self.log.info(f"dashd args: {self.extra_args[0]}") self.wait_until(lambda: self.tor.target is not None) self.log.info(f"Tor control commands received: {self.tor.commands}") self.log.info(f"onion-service target handed to Tor: {self.tor.target}") self.log.info(f"getnetworkinfo localaddresses: {node.getnetworkinfo()['localaddresses']}") self.generate(node, 1, sync_fun=self.no_op) # leave IBD so self-advertisement happens host, port = self.tor.target.rsplit(":", 1) self.log.info(f"Connecting a peer to {host}:{port}, as Tor would for an inbound onion connection") peer = node.add_p2p_connection(AddrCollector(), dstaddr=host, dstport=int(port)) info = [p for p in node.getpeerinfo() if p["addrbind"] == f"{host}:{port}"][0] self.log.info(f"getpeerinfo: addr={info['addr']} addrbind={info['addrbind']} network={info['network']}") peer.send_and_ping(msg_getaddr()) try: self.wait_until(lambda: len(peer.addrs) > 0, timeout=10) except AssertionError: pass self.log.info(f"addresses self-advertised to this peer: {peer.addrs}") if __name__ == "__main__": ReproTorTarget().main() ``` </details> Release-note claim, unfixed build, with `127.0.0.1:19896` held by another process: ``` $ src/dashd -regtest -datadir=$D -listenonion=0 -port=18555 -rpcport=18556 -printtoconsole=0 Error: Unable to bind to 127.0.0.1:19896 on this computer. Dash Core is probably already running. Error: Failed to listen on any port. Use -listen=0 if you want this. exit=1 ``` ### Why it matters Running an onion service alongside a clearnet address is a supported setup. Operators who also pass an explicit `-bind`, which is common on servers and masternodes, would silently lose the separation between the two once v24.0.0 ships: any peer that connects to the `.onion` address can learn the node's clearnet IP from self-advertisement. Exposure needs a working Tor control connection and a routable local address (`-externalip`, or discovery or a routable `-bind`), so it does not affect every node. Nodes with `-bind` that do not use Tor are unaffected either way. The release-note error is only an operational trap, but it would ship in the v24.0.0 notes. ## What was done? - `src/init.cpp`: drop the branch that used `vBinds.front()` as the onion-service target, so the target is always the first `onion_binds` entry. Without a `-bind=...=onion`, the default onion target is now added to `onion_binds` and used as the Tor target unless `-bind` is given **and** `-listenonion` is disabled. In that one case no onion bind is added and Tor control is not started. `-listenonion` is read after parameter interaction, so `-listen=0` (which soft-sets `-listenonion=0`) is handled as before. The `-bind` help text now mentions the exception. - `doc/release-notes-7300.md`: describe the actual rule. The implicit bind is added unless `-bind` is given together with `-listenonion=0`. `-bind=...=onion` moves it, `-listenonion=0` alone or `-whitebind` alone keeps it, and `-listen=0` disables all binds. - `test/functional/feature_bind_extra.py`: node 2 (explicit `-bind`, no extra port) now says `-listenonion=0` explicitly. Before, it relied on the framework's config default. A new node 3 with explicit `-bind` and `-listenonion=1` must listen on both its `-bind` port and `127.0.0.1:19896`, and a peer connecting to `19896` must show up with `network` `onion`. Node 3's `-torcontrol` points at an unused port so the test never creates an onion service through a Tor instance running on the test host. - `test/functional/feature_proxy.py`: one step starts node 1 with `-listenonion=1` on top of the framework's `bind=127.0.0.1`. With this PR that node would also bind the fixed regtest onion target `127.0.0.1:19896`, which is the port `feature_bind_extra.py` node 3 holds, and since dashpay#7300 a failed bind stops startup. Under `test_runner.py -jN` the two tests could then collide. The step now passes `-bind=127.0.0.1:<tor_port(1)>=onion`, the same per-node port the framework uses when it adds the binds itself. I kept this local to the one test rather than teaching `TestNode.start()` a new rule, because it is the only test that enables `-listenonion` together with an explicit bind. ### Why this is the correct minimal fix The root cause is that the onion service can be pointed at a listener that is not tagged as onion. The only listener that can be both the Tor target and correctly tagged is one in `onion_binds`, so the target must always come from `onion_binds`. The alternative of adding `vBinds.front()` to `onion_binds` would tag every clearnet peer on that port as onion, which leaks the onion address to clearnet peers and distorts eviction, so it is not an option. Two other designs would avoid the multi-instance cost described under Breaking Changes. Both are reasonable, and the choice is up to maintainers: - When `-bind` is given without `=onion` and `-listenonion` is not set explicitly, soft-set `-listenonion=0` during parameter interaction. This keeps rc.1's "respect `-bind`" behavior and avoids the extra bind. The cost is that nodes upgrading from v23 with `-bind` and an automatic onion service would silently lose that onion service. - Keep the default onion bind, but treat a failure to bind it as non-fatal and skip Tor control in that case. This brings back v23's tolerance for this one bind and gives up part of dashpay#7300's "fail on any bind error" rule. This PR does neither. It keeps v23's behavior for the onion service, plus the stricter bind handling from dashpay#7300. bitcoin#22729's goal was to let users avoid the extra `127.0.0.1:9996` bind. This PR keeps that, but only when the user also says they don't want an onion service (`-listenonion=0`). Without an onion service there is nothing to tag. Nothing changes for nodes without `-bind`, with `-bind=...=onion`, or with `-listen=0`. The stricter "fail startup if any bind fails" part of bitcoin#22729 is unchanged too. ### Relation to upstream Bitcoin Core has the same bug. The open upstream fix, bitcoin#36170 ("net: require a dedicated bind for automatic Tor"), has an Approach ACK. It also removes the `vBinds.front()` fallback, but handles the `-bind` + `-listenonion` case differently: instead of adding the default onion bind, it refuses to start until the operator adds `-bind=<addr>=onion` or `-listenonion=0`. That turns every existing `-bind` setup with the default `-listenonion=1` into a startup error, including many masternodes, so this PR keeps v23's default onion bind for that case. The `src/init.cpp` block uses the same layout as bitcoin#36170: the default bind is added up front under one condition, and the Tor target is always `onion_binds[0]` inside the `-listenonion` branch. Backporting bitcoin#36170 later then only changes that condition and adds its errors. bitcoin#36170 also rejects a wildcard `=onion` bind. That is left to the backport, since it is a separate new startup error. This should also go to the v24.0.x release branch, since the behavior was introduced in v24.0.0-rc.1. ## How Has This Been Tested? macOS arm64, `--enable-debug` build, functional tests run outside any sandbox. - Reproduction script above: unfixed build shows the onion target `127.0.0.1:<bind port>`, peer `network=not_publicly_routable`, and the peer is sent `1.2.3.4`. Fixed build shows target `127.0.0.1:19896`, peer `network=onion`, and no clearnet address is sent. - `feature_bind_extra.py` with the new cases. The test is Linux-only (it reads `/proc/net/tcp`), so I ran it locally through a wrapper that swaps `get_bind_addrs()` for an `lsof`-based equivalent and lifts the platform skip. The test itself is unchanged. Nodes 0–2 pass on both builds. - Unfixed (only the `src/init.cpp` hunks reverted): fails at node 3 with `AssertionError: not({('7f000001', 17013)} == {('7f000001', 17013), ('7f000001', 19896)})`. - Fixed: passes, including the `network == "onion"` check. - `feature_proxy.py` with `--nocleanup`, checking the `Bound to` lines in node 1's `debug.log`: - Fixed build without the `feature_proxy.py` change: node 1 binds `127.0.0.1:19896`. This is the collision described above. - Fixed build with the change: node 1 binds only its p2p port and its `tor_port()`, and nothing on `19896`. The test passes. - Unfixed build with the change: same binds, and the test passes. - Release-note cases on the fixed build, with `127.0.0.1:19896` held by another process: `-listenonion=0` alone still fails with `Unable to bind to 127.0.0.1:19896`, as the corrected note says. `-bind=127.0.0.1:18555 -listenonion=0` starts. - `test/functional/test_runner.py feature_proxy.py p2p_eviction.py rpc_net.py` (after the test changes) and earlier `feature_config_args.py`: all passed. `feature_config_args.py` failed once in a `-j4` run with `Unable to start HTTP server` (an RPC port collision with another run on the same host) and passed on rerun. `feature_bind_extra.py`, `feature_bind_port_*.py` and `rpc_bind.py` are skipped on macOS by the runner. CI covers them on Linux. - `test/lint/lint-whitespace.py` and flake8 with the codes enabled in `test/lint/lint-python.py` pass. The `-bind` help line keeps the single-line `AddArg` style of its neighbours rather than the clang-format suggestion. ## Breaking Changes Compared with v24.0.0-rc.1/rc.2 only: a node started with an explicit `-bind` and `-listenonion` enabled (the default) binds `127.0.0.1:9996` (`19996` testnet) again, as v23 did. With the stricter bind handling from dashpay#7300, it fails to start if that port is unavailable. This breaks one setup that works on rc.1/rc.2: several `dashd` instances on one host and one chain, each with its own explicit `-bind`, and `-listenonion` left at its default. For example, several masternodes on separate IPs on one server. On rc.1/rc.2 each instance binds only its own `-bind` address. With this PR every instance also tries to bind `127.0.0.1:9996`; the first one gets it and every later one fails to start. v23 started all of them because a failed bind was only a warning there, and avoiding this extra bind was the reason for bitcoin#22729. Operators with that setup need one of these per instance: - `-listenonion=0`, if the instance does not need an automatic onion service; or - a distinct `-bind=127.0.0.1:<port>=onion` for each instance. Maintainers should weigh this cost against the self-advertisement problem above before merging. The alternatives listed under "Why this is the correct minimal fix" avoid this cost but make other trade-offs. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: knst: self-ACK 5f40d56 Tree-SHA512: 049a3879405756f81ac5fd5a7f1322cc56a1e78d705dd7774e6e0a144fd6a0da0439d9dd106901c91fb3c9dd8395c0143bfa58c363f0b2ec80ffe43ee123312c (cherry picked from commit d721bef)
…oin#29523: weight-check the knapsack exact-match selection d279879 backport: bitcoin#26720: remaining ChooseSelectionResult changes (pasta) 12f213f backport: partial bitcoin#29523: weight-check the knapsack exact-match selection (pasta) Pull request description: ## Issue being fixed or feature implemented When the wallet's small coins add up exactly to the selection target, `KnapsackSolver` returns all of them without checking the max allowed weight. If that set is too heavy for a standard transaction, `ChooseSelectionResult` drops it and the send fails with "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs". This happens even when the wallet has a single larger coin (other than a CoinJoin denomination, see below) that could pay on its own. The knapsack's stochastic path already falls back to that larger coin when its result is too heavy. The exact-match shortcut never got the same check. This affects wallets with many small UTXOs (for example from mining payouts or many small incoming payments) when the fee-adjusted target happens to equal the sum of the coins below it. That is uncommon, but when it happens the send fails with a misleading message. The same send would succeed if the target were slightly different. A second gap comes from the order of earlier backports. The bitcoin#26720 backport (5288a4a) landed before bitcoin#25729 and bitcoin#26661. Because of that, its `ChooseSelectionResult` changes were never applied: the post-selection weight filter is still there, and the specific errors returned by the selection algorithms are discarded. For example, when the knapsack's stochastic path returns its own max-weight error, the user sees a generic "Insufficient funds" instead. ### Why this is a real problem Code path at c14104b: - The exact-match shortcut returns every applicable group without a weight check: [coinselection.cpp#L401-L406](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/coinselection.cpp#L401-L406) - The stochastic path does check the weight and falls back to `lowest_larger`: [coinselection.cpp#L453-L458](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/coinselection.cpp#L453-L458) - `ChooseSelectionResult` filters the overweight result out and returns the max-weight error: [spend.cpp#L563-L572](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/spend.cpp#L563-L572) - Specific knapsack errors are ignored, so a failed knapsack ends as a plain `util::Error()`: [spend.cpp#L544-L561](https://github.com/dashpay/dash/blob/c14104b172a2/src/wallet/spend.cpp#L544-L561) I added a case to `coinselector_tests/check_max_weight` and ran it against the unfixed code. The wallet holds 1500 coins of 0.033 (summing exactly to the 49.5 target, 1500 x 68 bytes = 102,000 bytes, over the limit) plus one 50 coin. Selection should pick the 50 coin, but it fails: <details> <summary>Unfixed code (c14104b + new test case)</summary> ``` $ src/test/test_dash --run_test=coinselector_tests/check_max_weight Running 1 test case... wallet/test/coinselector_tests.cpp:1206: fatal error: in "coinselector_tests/check_max_weight": critical check result has failed *** 1 failure is detected in the test module "Dash Core Test Suite" exit=201 ``` Printing the returned error (temporary `BOOST_REQUIRE_MESSAGE`): ``` wallet/test/coinselector_tests.cpp:1206: fatal error: in "coinselector_tests/check_max_weight": selection error: The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs ``` </details> ### Why it matters A user with enough funds, including a single coin that covers the payment, cannot send a specific amount. The error tells them to send less or consolidate, which is wrong advice in this case. Funds are never at risk. Coin control or consolidation works around it, but the user has no reason to know that. ## What was done? Two commits: 1. **partial bitcoin#29523** ([734076c](bitcoin@734076c), knapsack exact-match hunk only). If the exact-match selection is over `max_weight`, the result is cleared and selection falls through to the existing stochastic path. Since the applicable coins sum exactly to the target, that path ends at the existing weight check, which returns the closest larger coin or `ErrorMaxWeightExceeded()` when there is none. The check sits inside Dash's `tryDenom` loop, so the CoinJoin denomination ordering and the `fFullyMixedOnly` / `maxTxFee` check at the end are unchanged. Adds the regression case to `check_max_weight`. 2. **bitcoin#26720, remaining hunks** (`ChooseSelectionResult` part of [1284223](bitcoin@1284223) and [5a2bc45](bitcoin@5a2bc45)). Each algorithm's specific error is collected with `append_error`, and the first one is returned when no algorithm found a solution. The post-selection weight filter is removed, because every algorithm now enforces the max weight itself. The existing overweight case in `check_max_weight` (1515 small coins, no larger coin) now also checks that the max-weight error is reported. ### Why this is the correct minimal fix The root cause is the missing weight check on the exact-match shortcut. The upstream fix for that path is a 3-line hunk, and it is taken as is, using Dash's existing `GetSelectionWeight` helper, which the stochastic path in the same function already uses. Commit 1 on its own would turn the max-weight message into "Insufficient funds" for an overweight exact sum with no larger coin, because `ChooseSelectionResult` discards specific knapsack errors (the knapsack's stochastic path already loses its max-weight error the same way on develop). I confirmed this by running the new assertion with only commit 1 applied. Commit 2 closes that gap with the upstream bitcoin#26720 code. With the knapsack fixed, the post-selection filter can no longer trigger for multi-group results, so removing it, as upstream did, is safe. Intentionally not taken from bitcoin#29523: the `max_tx_weight` RPC funding option, the per-group weight skip, and the weight tracking inside `ApproximateBestSubset`. None of these is needed for this fix. Without the per-group skip, a single `OutputGroup` returned by the knapsack's exact-group or `lowest_larger` paths is not weight-checked during coin selection once the post-selection filter is gone. That needs `avoid_partial_spends` and close to 100 large (e.g. multisig) inputs to one address, and is still rejected later by the "Transaction too large" check in `CreateTransactionInternal`, so only the error message differs. Upstream was in the same state between bitcoin#26720 and bitcoin#29523. Behavior intentionally left unchanged: when only non-denominated coins are considered first (`tryDenom == 0`) and they sum exactly to the target but are overweight, selection falls through to the stochastic path, which at that stage only sees non-denominated coins. If none of them is larger than the target, the knapsack returns the max-weight error without trying a larger CoinJoin-denominated coin. This is how the existing `nTotalLower > target` case already behaves, and it failed before this change too, so it is left for a separate change if needed. User-visible message change: overweight knapsack failures that used to end as "Insufficient funds." or, for CoinJoin sends, "Unable to locate enough mixed funds for this transaction..." now report the max-weight message, as upstream does. Not taken from bitcoin#26720: the test-only commits [ba9431c](bitcoin@ba9431c) (BnB max weight coverage) and [25ab147](bitcoin@25ab147) (test wallet-creation refactor). They are unrelated to this fix, and Dash's `ChooseSelectionResult` passes BnB an empty pool. `AttemptSelection` still returns early on a specific error, the same as upstream. ## How Has This Been Tested? macOS arm64, `--enable-debug --enable-tests --without-gui`. - Unfixed code plus the new exact-sum case: `src/test/test_dash --run_test=coinselector_tests/check_max_weight` fails (output above). - Commit 1 only, with commit 2's new assertion: the exact-sum case passes. The max-weight message assertion on the existing 1515-coin case fails (`coinselector_tests.cpp:1192: error: ... check util::ErrorString(result).original.find("The inputs size exceeds the maximum weight") != std::string::npos has failed`), because the knapsack's error is replaced by a generic one. - Both commits: `src/test/test_dash --run_test=coinselector_tests` passes (10 test cases). `check_max_weight` passed 3 further runs out of 3. `--run_test=spend_tests,wallet_tests` also passes. - Functional: `test/functional/test_runner.py wallet_basic.py wallet_create_tx.py wallet_fundrawtransaction.py wallet_send.py wallet_sendall.py rpc_coinjoin.py` all passed (run before the test-only rework of commit 2; the non-test code is unchanged since). `wallet_sendall.py --descriptors` failed after 1 s in the first parallel run and passed on a standalone rerun. - `test/lint/lint-whitespace.py` passes. The upstream lines in `spend.cpp` are kept verbatim rather than clang-formatted. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 9bc892a4ae561619762acfdb362f791024021544c6b240d0ec2824c0ddbdde9ded297ee2cfda1045cb1d3b4f243e24cdd3905d012db34701a42ef5e87031643b (cherry picked from commit 62d541b)
…hdseed 409b033 fix: return non-zero amount of accounts after sethdseed (pasta) Pull request description: ## Issue being fixed or feature implemented `migratewallet` fails on a legacy HD wallet whose seed has not derived any key yet: ``` MigrateToDescriptor: legacy HD chain has 0 accounts; migration only supports wallets with a single BIP44 account ``` `sethdseed` with an explicit seed and the `-hdseed` startup path both write the HD chain with `AddHDChainSingle()` first and call `newHdChain.AddAccount()` afterwards, on a local copy that is never read again. The call has been dead code since 266aefc. The chain on disk therefore has an empty account map until `DeriveNewChildKey()` derives the first key and `SetAccount()` creates account 0 as a side effect. `CHDChain::GetAccount()` and `SetAccount()` already treat an empty map as "account 0 with zeroed counters", but `CountAccounts()` reports 0 for the same chain, and `MigrateToDescriptor()` rejects anything but 1. Normally the keypool top-up hides this right away. It does not happen when `sethdseed` is called with `newkeypool=false` and no key is derived before migration. The clearest case is an encrypted wallet: `migratewallet` reloads it locked, so the top-up on load derives nothing, and `MigrateToDescriptor()` then sees a chain with no stored accounts. `getwalletinfo` reports `hdaccountcount: 0` for such a wallet, although the next key will be derived from account 0. The owner of such a wallet cannot migrate it to a descriptor wallet. Migration fails closed and no keys or funds are lost, but the error suggests an unsupported account layout and the workaround (unlock and derive a key before migrating) is not obvious. ## What was done? A seeded chain is one account, whether or not a key has been derived from it yet. That is also how Bitcoin Core's `CHDChain` behaves: `SetHDSeed()` writes a complete chain with zeroed counters in one step, and `MigrateToDescriptor()` reads those counters with no account guard. Commit 1: `CHDChain::CountAccounts()` returns 0 for a null chain and otherwise at least 1. This makes it consistent with `GetAccount()`/`SetAccount()` and fixes all three callers at once: the migration guard, `getwalletinfo` and `dumpwallet`. The guard in `MigrateToDescriptor()` stays `!= 1`, so chains with more than one account are still rejected and the backported file is untouched. Wallets already on disk with an empty account map migrate without any repair step. Commit 2: `sethdseed` and the `-hdseed` path call `AddAccount()` before the chain is encrypted and written, the same order `GenerateNewHDChain()` has always used for mnemonic-based chains. The chain record written for a seed-based wallet is now identical in shape to the one written for every mnemonic-based wallet. Nothing reads the local copy after the write, so removing the old call changes no behavior; the only visible difference is that `getwalletinfo` reports `hdaccountcount: 1` immediately after `sethdseed`. Test: `wallet_hd.py` already builds exactly this state. `wallet_imported_seed` is a blank wallet that receives `sethdseed(False, seed)` and is inspected with `getwalletinfo` before any key is derived. One assertion there checks `hdaccountcount == 1`. On the unfixed tree it reports 0, which is the value the migration guard rejects; either commit alone makes it pass. ## How Has This Been Tested? Run unit & functional tests. ## Breaking Changes None. `getwalletinfo` and `dumpwallet` report one account instead of zero for a seeded legacy chain that has not derived a key yet, which is the account the next derivation uses. The on-disk chain record written by `sethdseed` and `-hdseed` now carries account 0 from the start, which is the same record every mnemonic-based legacy wallet already has, so older binaries read it unchanged. ## 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 _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: f3177e86c4ceaee0fc3cc29a4634ca041e597b60b5cb7d5a5f193f33045bb13310874ae5448393d189fb5925a0f3fb0311e5dfdcf9c0688b6acbdb1a7324736f (cherry picked from commit f3ccc66)
…r update that keeps the operator key 606efac revert: drop the pending ProRegTx eviction on collateral spend for now (UdjinM6) 612de70 test: a pending ProRegTx survives a spend of the collateral it carries (Konstantin Akimov) 9895ee7 fix(mempool): keep a pending ProRegTx when its own collateral output is spent (Konstantin Akimov) 5129689 test: the eviction tests each rebuilt a fixture that an existing test already (Konstantin Akimov) 4767667 test: a confirmed dissolution evicts pending updates of the dissolved masternode (UdjinM6) e1e9703 test: pending ProUpRevTx also leaves the mempool once the collateral is spent (UdjinM6) 9731e91 test: pending updates leave the mempool once a block spends the masternode collateral (UdjinM6) 7005c5b test: a pending ProUpServTx survives a registrar update that keeps the operator key (UdjinM6) a0b562c fix(mempool): keep a pending ProUpServTx across a registrar update that keeps the operator key (UdjinM6) 870ddeb test: a pending ProRegTx leaves the mempool once a block spends its collateral (UdjinM6) daccd7e fix(mempool): evict a pending ProRegTx once a block spends its collateral (UdjinM6) Pull request description: ## Issue being fixed or feature implemented When a ProUpRegTx is confirmed, `removeProTxKeyChangedConflicts()` evicts the pending updates of the same masternode that were valid only for the previous operator key. It decides this by comparing each pending update's `validForProTxKey` with the new key. `addUncheckedProTx()` sets that field for ProUpRegTx and ProUpRevTx but not for ProUpServTx, so it stays null and never matches. As a result, every confirmed ProUpRegTx of a masternode evicts all its pending ProUpServTxs, even when the operator key does not change, for example a payout or voting key update. A ProUpServTx is signed by the operator key and is still valid in that case. Evicting it drops a valid transaction together with its InstantSend lock, and the operator has to resubmit it. The mempool's eviction paths for pending updates of a registered masternode also had little test coverage. ## What was done? - `addUncheckedProTx()` records `validForProTxKey` for a ProUpServTx from the operator key at the tip, as it already does for ProUpRegTx and ProUpRevTx. A confirmed ProUpRegTx then evicts the ProUpServTx only if it changes the operator key. If the masternode is not in the tip list, the field stays null and any registrar update still evicts the ProUpServTx, as before. - New coverage for eviction paths that were not tested: - `test_mempool_proreg_replacement_update_conflict`: - a confirmed ProUpRegTx that keeps the operator key, together with a spend of an unrelated output, leaves pending ProUpServTx and ProUpRevTx in place; - one that changes the key evicts them; - a spend of the masternode collateral evicts pending ProUpServTx, ProUpRegTx and ProUpRevTx. - `feature_masternode_shares.py`: in the dissolution reorg scenario, a ProUpSharedRegTx and a ProUpShareTx submitted while the dissolution block is disconnected are gone from the mempool once the block is reconnected. The branch history also contains an eviction of a pending ProRegTx whose external collateral is spent, a follow-up exemption for a ProRegTx whose own collateral output is spent, and a commit reverting both. That change needs to be coordinated with a related InstantSend policy change and will be proposed again separately. The net diff of this PR does not touch `removeProTxSpentCollateralConflicts()`. Known limitation: while a reorg returns disconnected transactions to the mempool, the masternode list tip has not been moved yet. A ProUpServTx re-added at that point can record the operator key from the pre-reorg list. ProUpRegTx and ProUpRevTx entries already take their key the same way. dashpay#7728 moves the list tip before transactions are re-added, which covers all three. Related: dashpay#7728 keeps an operator key change and a ProUpServTx signed with the old key out of the mempool together. It does not change what happens when a ProUpRegTx that keeps the key is confirmed, so the two PRs complement each other. ## How Has This Been Tested? Built on macOS arm64 with `--enable-debug --enable-werror --enable-crash-hooks`. - Removing the `validForProTxKey` assignment makes `test_mempool_proreg_replacement_update_conflict` fail at the check that the ProUpServTx survives a ProUpRegTx that keeps the key. - Disabling the registered-masternode branch of `removeProTxSpentCollateralConflicts()` makes `test_mempool_proreg_replacement_update_conflict` fail at the collateral-spend check, and `feature_masternode_shares.py` fail at the dissolution check. - These mutation checks were run on an earlier head of this branch. The tests and the ProUpServTx fix have not changed since. Run at the current head: - All unit tests: `test_dash`, 1056 cases. - Functional: `feature_masternode_shares.py`, `feature_dip3_deterministicmns.py --descriptors`, `rpc_masternode.py`, `mempool_reorg.py`. - `test/lint/all-lint.py`: the only failure is an existing cppcheck warning in `src/governance/object.cpp`, which this PR does not touch. Python lint was skipped locally because flake8 is not installed; `pyflakes` is clean on the changed functional test. ## Breaking Changes None. Consensus is unchanged and this only affects mempool policy: a pending ProUpServTx is no longer evicted by a confirmed ProUpRegTx that keeps the operator key. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 7456982f43c2ea8c8e8f0cbcba3413c4e6f40b4149ee62247c4ed251fed057d25062eb4f215331f7c7d01746e077f7b946d17b99d6849234c85a93208d9b200a (cherry picked from commit 15db478)
… payouts behind a new evo_shares deployment 787ad82 refactor: share platformNodeID handling between provider transactions (pasta) 8edce1f doc: add release notes for the evo_shares deployment (pasta) d42644f test: cover evo_shares signing and EvoNode owner payouts (pasta) b3e6f5e feat(rpc): prepare shared EvoNode registrations (pasta) 9deae1f feat: restrict EvoNode shares and multiple payouts until evo_shares (pasta) 68a9144 feat: add the evo_shares deployment, not signed in v24 (pasta) e4428b7 refactor: pass special transaction deployment rules as a struct (pasta) Pull request description: ## Issue being fixed or feature implemented Platform does not support EvoNodes with shared collateral or with more than one owner payout yet, and asked that v24 not allow them. v24 introduces both (shared masternode collateral and the version 3 payout list), and today an EvoNode can already register or update with several owner payouts; only shared EvoNode registration is rejected, statelessly. This PR keeps both disabled for EvoNodes in v24 and adds a deployment, `evo_shares`, that lifts the restriction later (planned for v24.1, once Platform supports it) without a chain split between v24 and v24.1 nodes. > [!IMPORTANT] > Mainnet `v24` and `evo_shares` start times are both still `TODO`. They must be set to the same value before release. A new unit test (`evo_shares_follows_v24`) fails if `evo_shares`' parameters ever differ from `v24`'s on any network, so the mainnet start time cannot be filled in for only one of them. ### Why v24 does not sign it instead of using a later start time The new deployment's consensus parameters ship in v24 and must be identical in v24.1: `nStartTime` is consensus (it gates versionbits `STARTED`, and an MNHF signal mined before it is treated as expired), so v24.1 cannot move it, and a start time could not express "once enough of the network runs v24.1" anyway. Instead v24 masternodes simply do not sign the `evo_shares` MNHF signal. A masternode-activated deployment cannot start until its signal is mined, so `evo_shares` stays inactive until masternodes running v24.1 sign it; v24 nodes follow it from then on like any other deployment. ## What was done? 1. **refactor:** `SpecialTxRules` (filled by `GetSpecialTxRules()`) replaces the `bool is_v24_active` parameter of `CheckSpecialTx`, `ProcessSpecialTxsInBlock` and their callers, so another deployment can be passed without touching every caller again. No behaviour change. 2. **`DEPLOYMENT_EVO_SHARES`** (`evo_shares`, bit 14, masternode-activated) with `v24`'s window, thresholds and start times on every network. The MNHF signer skips it; v24.1 drops that skip. 3. **Rules**, until `evo_shares` is active: - an EvoNode ProRegTx with a share table is rejected with `bad-protx-shares-evo`. The rule moves from the stateless `CProRegTx::IsTriviallyValid()` into `CheckProRegTx()`; - an EvoNode ProRegTx or ProUpRegTx with more than one owner payout is rejected with the new `bad-protx-payouts-evo`. The ProUpRegTx check also covers operator key rotation; - neither rule needs a v24 gate: share tables and payout lists only exist in version 3 payloads, which need v24. - The shared registration consent digest also covers an EvoNode's `platformNodeID`. Only the digest of EvoNode registrations changes, and those were invalid until now; the digest of a regular shared registration is unchanged. 4. **RPC:** `protx shared_register_prepare_evo` takes the arguments of `protx shared_register_prepare` followed by `platformNodeID`, `platformP2PAddrs` and `platformHTTPSAddrs`. Preparing a shared EvoNode registration fails until `evo_shares` is active. Dash Qt still prepares regular shared registrations only; EvoNode support there is left for v24.1. 5. **Tests and release notes** (below), and a small follow-up refactor that moves `register_evo` / `update_service_evo` onto the `platformNodeID` helpers added for the new RPC. ## How Has This Been Tested? macOS arm64, debug build (`--enable-debug --enable-werror`). - `make check`-equivalent: the full `test_dash` suite passes. - Every commit builds and passes `evo_*`, `versionbits_tests` and `miner_tests` on its own. - New unit tests: - `evonode_registration_needs_evo_shares` / `evonode_payout_update_needs_evo_shares`: table-driven `CheckProRegTx` / `CheckProUpRegTx` cases (EvoNode vs regular masternode, one vs two payouts, shared, operator key rotation, `evo_shares` on/off); - `shared_reg_consent_hash`: `platformNodeID` is covered for EvoNodes and not for regular masternodes; - `evo_shares_follows_v24`. - New functional test `feature_evo_shares_activation.py`: a masternode signs another masternode-activated deployment (`testdummy`) but never `evo_shares`, which stays `defined`; an EvoNode keeps one owner payout until `evo_shares` is active (registration, registrar update, update with operator key rotation). It then lets miners activate `evo_shares` (`-vbparams` with EHF disabled) and checks EvoNodes can update and register with several owner payouts. - `feature_masternode_shares.py`: shared EvoNode preparation fails before activation; the same signed registration is rejected with `bad-protx-shares-evo` in a block below the activation height; after activation it registers, is paid split by shares, updates its Platform service, a share reward and its voting key, and dissolves with a refund to every share. - Mutation checks, run locally: disabling either consensus rule fails exactly the restricted cases of the new unit tests; letting the signer sign `evo_shares` fails `feature_evo_shares_activation.py`. - Also run: `rpc_blockchain.py`, `rpc_help.py`, `feature_mnehf.py`, `feature_versionbits_warning.py`, `mining_basic.py`, `feature_protx_version.py`, `feature_dip3_deterministicmns.py`, `rpc_netinfo.py`, `feature_asset_locks.py`, `wallet_dash_rpcs.py`; Python, whitespace, file, include, assertion and format-string lints. ## Breaking Changes Consensus change to rules that are not active on mainnet or testnet yet: - Testnet: `v24` is not active yet (the tip coinbase is still CbTx version 3), so no historical block is affected. Testnet masternodes should run a build with this change before `v24` activates there, otherwise an EvoNode registered or updated with several owner payouts after activation would split them from upgraded nodes. - Devnets where `v24` is already active (its devnet start time is July 2025): any EvoNode registered or updated there with more than one owner payout makes those blocks invalid for this build, so such devnets need to check (`protx list registered 1`) or be reset. - New RPC `protx shared_register_prepare_evo`. Follow-up outside this repo: the shared masternode DIP should describe the `platformNodeID` field in the EvoNode consent digest. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: d730be63b6871e689c524452b967948504ad1ad0249aa31e213c43571ce091a9bc164e4da91f2f0417c8a2089e2bac1e70e14352cb7047be35592fe4a29c4499 (cherry picked from commit f35a24c)
Collaborator
|
⛔ Final review complete — 1 blocking finding(s) (commit fbe6526) |
thepastaclaw
suggested changes
Oct 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the findings against fbe6526 and the rc.2 base. One wallet regression blocks approval: propagating a size-limit error from the first output-type bucket prevents selection from another bucket that can fund the payment. Two backport-description corrections remain actionable; three test-coverage requests are explicitly excluded by the picked commits. This was static verification only; the supplied CI snapshot shows successful Linux, fuzz, and no-wallet source builds, with macOS, ARM64 Linux, Windows, sanitizer, SQLite, and lint checks still pending.
🔴 1 blocking | 🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, cross-cutting diff changes consensus activation and EvoNode share/multiple-payout validation in src/evo/specialtxman.cpp, src/evo/providertx_service.cpp, and src/validation.cpp, meeting both the intricacy and critical-surface criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/spend.cpp`:
- [BLOCKING] src/wallet/spend.cpp:566-569: Keep trying other output types after a size-limit error
Returning the knapsack's detailed error here causes AttemptSelection() to return immediately at line 507, before examining later output-type buckets or trying the permitted mixed-type fallback. This affects Dash wallets: AvailableCoins() solves spendable P2SH redeem scripts, so P2SH multisig is classified as UNKNOWN, while P2PKH is classified as LEGACY and examined first. A wallet with 1515 confirmed P2PKH outputs of 0.033 DASH and a spendable 50 DASH P2SH multisig output can fund a 49.5 DASH payment using the multisig output. The LEGACY bucket instead requires an oversized selection and returns the size-limit error, preventing the usable UNKNOWN bucket from being tried under every eligibility filter. At the rc.2 base, that stochastic knapsack error was discarded, allowing selection to continue. Collect per-type errors in AttemptSelection() and return an error only after the remaining buckets and permitted mixed-type fallback fail. Add a regression covering both output types; the new test places every coin in LEGACY and does not exercise this failure.
In `src/init.cpp`:
- [SUGGESTION] src/init.cpp:2686-2695: Update the #7786 backport scope and breaking-change description
The PR description says an explicit normal -bind restores the default onion listener when -listenonion is enabled. HEAD does not implement that policy: it adds the default listener only when both onion_binds and vBinds are empty, and otherwise refuses startup without a dedicated onion bind. It also rejects wildcard onion binds. Consequently, even one node with only a normal -bind and default -listenonion refuses startup, whether or not Tor is configured. The release note and feature_bind_extra.py already describe and test this refusal. Update the Breaking Changes section to match them, and identify the partial bitcoin#36170 adoption carried by commit 5f40d56f4ddb in the backport inventory. This requests accurate metadata, not a change to the selected startup policy.
In `test/functional/feature_bind_extra.py`:
- [SUGGESTION] test/functional/feature_bind_extra.py:90-98: Partial bitcoin#36170 leaves two upstream test adaptations unaccounted for
The inspected bitcoin#36170 merge-preview diff, 17266b185623, also adds dedicated onion binds to feature_torcontrol.py and p2p_private_broadcast.py. Those tests originated in commits 569383356e and e74d54e048 and are absent from both the rc.2 base and HEAD. The selected Dash commit is explicitly a partial adoption, but its message and the release PR description do not account for these two omitted test adaptations. Record them as prerequisite-dependent deferred hunks when documenting the partial #36170 scope. The carried startup tests are present, so this is a documentation-completeness issue, not a demonstrated failure of an existing Dash test or a request to import private broadcasting into this release.
Comment on lines
566
to
+569
| if (results.empty()) { | ||
| // No solution found | ||
| return util::Error(); | ||
| } | ||
|
|
||
| std::vector<SelectionResult> eligible_results; | ||
| std::copy_if(results.begin(), results.end(), std::back_inserter(eligible_results), [coin_selection_params](const SelectionResult& result) { | ||
| const auto initWeight{coin_selection_params.tx_noinputs_size}; | ||
| return initWeight + result.GetWeight() <= static_cast<int>(MAX_STANDARD_TX_SIZE); | ||
| }); | ||
|
|
||
| if (eligible_results.empty()) { | ||
| return util::Error{_("The inputs size exceeds the maximum weight. " | ||
| "Please try sending a smaller amount or manually consolidating your wallet's UTXOs")}; | ||
| // No solution found, retrieve the first explicit error (if any). | ||
| // future: add 'severity level' to errors so the worst one can be retrieved instead of the first one. | ||
| return errors.empty() ? util::Error() : errors.front(); |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Keep trying other output types after a size-limit error
Returning the knapsack's detailed error here causes AttemptSelection() to return immediately at line 507, before examining later output-type buckets or trying the permitted mixed-type fallback. This affects Dash wallets: AvailableCoins() solves spendable P2SH redeem scripts, so P2SH multisig is classified as UNKNOWN, while P2PKH is classified as LEGACY and examined first. A wallet with 1515 confirmed P2PKH outputs of 0.033 DASH and a spendable 50 DASH P2SH multisig output can fund a 49.5 DASH payment using the multisig output. The LEGACY bucket instead requires an oversized selection and returns the size-limit error, preventing the usable UNKNOWN bucket from being tried under every eligibility filter. At the rc.2 base, that stochastic knapsack error was discarded, allowing selection to continue. Collect per-type errors in AttemptSelection() and return an error only after the remaining buckets and permitted mixed-type fallback fail. Add a regression covering both output types; the new test places every coin in LEGACY and does not exercise this failure.
source: gpt-6.1-sol (phase2-reviewer: general)
Member
Author
There was a problem hiding this comment.
The behavior change is real, but it isn't blocking this release PR, and the code stays as-is to match upstream.
- On rc.2,
ChooseSelectionResult()dropped the knapsack'sErrorMaxWeightExceeded. backport: bitcoin#26720 (remaining), partial bitcoin#29523: weight-check the knapsack exact-match selection #7796 now passes it up, soAttemptSelection()stops at the first output-type bucket that reports it. - This is the upstream shape. Bitcoin Core master still has the same
if (HasErrorMsg(result)) return result;inAttemptSelection()(src/wallet/spend.cpp:707), so the backport now matches upstream exactly. This PR only carries backport: bitcoin#26720 (remaining), partial bitcoin#29523: weight-check the knapsack exact-match selection #7796 as merged on develop. - In Dash, the scenario needs a spendable P2SH (
UNKNOWN) output plus enough small P2PKH outputs that theLEGACYbucket goes over the size limit. Even then, the send fails with an actionable "inputs size exceeds the maximum weight" error rather than building a bad transaction.
If we want per-type error collection, it should go to develop as its own change that diverges from upstream, and then be backported. It doesn't belong in a release cherry-pick.
🤖 Posted autonomously by Claude on behalf of pasta.
Collaborator
There was a problem hiding this comment.
I agree that this matches the upstream AttemptSelection() shape, but upstream alignment does not remove the regression introduced by propagating this error. There is also a reverse-order failure: a successful LEGACY selection is appended to results, then an oversized UNKNOWN selection causes the immediate return to discard that already-valid solution. In that case, the consolidation advice concerns a bucket the transaction does not need to spend. Landing the repair on develop first is reasonable, but I still consider carrying that repair necessary before releasing this newly introduced send failure.
Comment on lines
+2686
to
+2695
| if (connOptions.onion_binds.empty() && connOptions.vBinds.empty()) { | ||
| connOptions.onion_binds.push_back(DefaultOnionServiceTarget()); | ||
| } | ||
|
|
||
| if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) { | ||
| if (connOptions.onion_binds.empty()) { | ||
| return InitError(_("The automatic Tor onion service requires a dedicated onion bind. Use a specific address such as -bind=127.0.0.1:<port>=onion, or disable the service with -listenonion=0.")); | ||
| } | ||
| if (std::ranges::any_of(connOptions.onion_binds, [](auto& b) { return b.IsBindAny(); })) { | ||
| return InitError(_("The automatic Tor onion service cannot use a wildcard onion bind because the Tor daemon wouldn't be able to forward incoming connections to us. Use a specific address such as -bind=127.0.0.1:<port>=onion, or disable the service with -listenonion=0.")); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Update the #7786 backport scope and breaking-change description
The PR description says an explicit normal -bind restores the default onion listener when -listenonion is enabled. HEAD does not implement that policy: it adds the default listener only when both onion_binds and vBinds are empty, and otherwise refuses startup without a dedicated onion bind. It also rejects wildcard onion binds. Consequently, even one node with only a normal -bind and default -listenonion refuses startup, whether or not Tor is configured. The release note and feature_bind_extra.py already describe and test this refusal. Update the Breaking Changes section to match them, and identify the partial bitcoin#36170 adoption carried by commit 5f40d56 in the backport inventory. This requests accurate metadata, not a change to the selected startup policy.
source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)
Member
Author
There was a problem hiding this comment.
Agreed. The Breaking Changes text came from #7786's original description, not the merged code. I've updated the PR description to match doc/release-notes-7300.md: -bind without a specific =onion bind is refused while -listenonion is enabled, and wildcard onion binds are refused. I also listed the partial bitcoin#36170 (5f40d56).
🤖 Posted autonomously by Claude on behalf of pasta.
Collaborator
There was a problem hiding this comment.
The updated description addresses my concern: it now matches the startup checks and feature_bind_extra.py, including refusal without a dedicated onion bind and rejection of wildcard onion binds. It also identifies the partial bitcoin#36170 adoption. I am not requesting a startup-policy or code change for this thread.
The finding stands on this commit until that change is pushed; the push gets a full review.
Comment on lines
+90
to
+98
| self.log.info("Test -listenonion with a normal bind and no dedicated onion bind") | ||
| self.stop_node(2) | ||
| self.nodes[2].assert_start_raises_init_error( | ||
| self.extra_args[2] + ["-listenonion=1", "-torcontrol=127.0.0.1:1"], | ||
| "Error: The automatic Tor onion service requires a dedicated onion bind. Use a specific address such as -bind=127.0.0.1:<port>=onion, or disable the service with -listenonion=0.", | ||
| ) | ||
|
|
||
| self.log.info("Test -bind with dedicated onion bind starts when -listenonion=1") | ||
| self.restart_node(1, extra_args=self.extra_args[1] + ["-listenonion=1", "-torcontrol=127.0.0.1:1"]) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Partial bitcoin#36170 leaves two upstream test adaptations unaccounted for
The inspected bitcoin#36170 merge-preview diff, 17266b1, also adds dedicated onion binds to feature_torcontrol.py and p2p_private_broadcast.py. Those tests originated in commits 5693833 and e74d54e and are absent from both the rc.2 base and HEAD. The selected Dash commit is explicitly a partial adoption, but its message and the release PR description do not account for these two omitted test adaptations. Record them as prerequisite-dependent deferred hunks when documenting the partial bitcoin#36170 scope. The carried startup tests are present, so this is a documentation-completeness issue, not a demonstrated failure of an existing Dash test or a request to import private broadcasting into this release.
source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)
Member
Author
There was a problem hiding this comment.
Added to the PR description. Upstream's feature_torcontrol.py and p2p_private_broadcast.py adaptations are left out because Dash has neither test: they come from upstream commits 5693833 and e74d54e. The commit itself is a 1:1 cherry-pick of the develop merge and isn't amended.
🤖 Posted autonomously by Claude on behalf of pasta.
Collaborator
There was a problem hiding this comment.
The added explanation accounts for both omitted adaptations and addresses this documentation concern. I verified that those upstream commits introduce the two functional tests and that neither test exists in this checkout. Documenting the omission in the release PR is sufficient; I am not asking you to amend the develop cherry-pick or import those tests here.
The finding stands on this commit until that change is pushed; the push gets a full review.
thepastaclaw
added
the
pastaclaw:changes-requested
thepastaclaw
removed
the
pastaclaw:changes-requested
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Backports for v24.0.0-rc.3.
v24.0.xwas cut at the v24.0.0-rc.2 tag (1e239d4), and develop has since moved to 24.1.0 (#7775), so rc.3 is built from this branch instead of being tagged on develop like rc.1 and rc.2.What was done?
Cherry-picked (
git cherry-pick -m1 -x) every develop merge since rc.2 that carriesbackport-candidate-24.0.x, plus #7778, which #7807's test needs. They are in develop merge order:feature_evo_shares_activation.pydepends on itsmine_quorum_single_member()fix and fails without it.)PSBTInputSignedAndVerifiedboundsassertbitcoin/bitcoin#34272, partial refactor: Avoid copies by using const references or by move-construction bitcoin/bitcoin#31650 (pass PSBT by const reference in PSBTInputSignedAndVerified)feature_torcontrol.pyandp2p_private_broadcast.pyare not included, because those tests depend on upstream commits (5693833, e74d54e) that Dash does not have.max_tx_weightto transaction funding options (take 2) bitcoin/bitcoin#29523: weight-check the knapsack exact-match selectionOne pick needed conflict resolution, in a test only, described in its commit message: #7793,
test/functional/wallet_migration.py.test_conflict_txs()comes from bitcoin#28542 (#7762), which is not on this branch, so it is left out. The C++ part of #7793 is identical to develop.Merged since rc.2 but intentionally not included: #7775 (24.1 version bump), #7260 and #7804 (new Qt feature and its CI fix), #7584 (refactor), routine upstream backports #7257, #7759, #7762, #7768, #7788, #7801, and test/CI-only changes #7777 and #7797.
No version change is needed.
configure.acis already 24.0.0 withCLIENT_VERSION_IS_RELEASEfalse, so the build takes its version string from thev24.0.0-rc.3tag, as it did for rc.2.How Has This Been Tested?
Built on macOS arm64 against depends with
--enable-debug --enable-werror(dashd, dash-qt, tests).make check: all unit test suites pass, includingtest_dash-qt.feature_evo_shares_activation.py,feature_masternode_shares.py,feature_mnehf.py,feature_governance.py --descriptors,feature_governance_objects.py,feature_governance_txindex_devnet.py,feature_proxy.py,feature_notifications.py,feature_llmq_connections.py,feature_llmq_chainlocks_automatic.py,feature_llmq_singlenode.py,p2p_instantsend.py,rpc_blockchain.py(v1 and v2 transport),rpc_netinfo.py,rpc_quorum.py,rpc_verifychainlock.py,wallet_hd.py(legacy and descriptors),wallet_migration.py.feature_bind_extra.py(from fix(net): keep a dedicated onion listener when -bind is given and correct the bind release note #7786) only runs on Linux and was skipped locally, so CI covers it.Every touched file was diffed against develop. The remaining differences come only from develop PRs that are not part of this backport.
Breaking Changes
Carried over from the picked PRs:
evo_sharesdeployment (bit 14). Mainnet and testnet are unaffected until it activates. Devnets withv24already active and an EvoNode carrying several owner payouts need to check or reset. New RPCprotx shared_register_prepare_evo.-txindex=0and without-disablegovernancerefuses to start again, as before v23.1.0.-bindbut no specific-bind=<addr:port>=onionnow refuses to start while-listenonionis enabled. That is the default when listening, even without Tor configured. A wildcard onion bind is also refused. Such nodes need-bind=127.0.0.1:<port>=onionor-listenonion=0. Nodes without-bind, including those using only-whitebind, keep the default127.0.0.1:9996(19996testnet) onion target. Seedoc/release-notes-7300.md.protx listdiffnow includesplatform_p2pandplatform_httpsfor a legacy-address evonode whose core P2P address changes.Checklist:
🤖 Generated with Claude Code