From 51b145c38231df391c485bc16d78efd4dbecbf86 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 3 Oct 2026 19:52:05 -0400 Subject: [PATCH 001/153] Merge #7778: test: use single-member in functional tests: feature_mnehf, feature_notifications, p2p_instantsend e308b31980ca93a5330471a0fa0a7619b78fd48c test: use a single-member quorum in p2p_instantsend (Konstantin Akimov) 233a89b39632814249688127d6ad660c5417957a test: use a single-member quorum in feature_notifications (Konstantin Akimov) 49e09b935af2cdfc4586eba1a11fecb802ffc970 test: use a single-member quorum in feature_mnehf (Konstantin Akimov) 2e6e5bb8cdb3f79fb06eac320bab78aedf33cdc1 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 88195077594b838b3abd5724bda9bd4ea1677a80) --- test/functional/feature_llmq_connections.py | 85 ++++++++++++++++- test/functional/feature_mnehf.py | 25 +++-- test/functional/feature_notifications.py | 21 ++--- test/functional/p2p_instantsend.py | 91 ++----------------- .../test_framework/test_framework.py | 5 +- 5 files changed, 119 insertions(+), 108 deletions(-) diff --git a/test/functional/feature_llmq_connections.py b/test/functional/feature_llmq_connections.py index 9b7955f6b2b5..b40d326fa285 100755 --- a/test/functional/feature_llmq_connections.py +++ b/test/functional/feature_llmq_connections.py @@ -16,7 +16,7 @@ DashTestFramework, MasternodeInfo, ) -from test_framework.util import assert_greater_than_or_equal +from test_framework.util import assert_greater_than_or_equal, force_finish_mnsync class LLMQConnections(DashTestFramework): def add_options(self, parser): @@ -114,6 +114,89 @@ def run_test(self): break assert added # no way we added none + self.test_instantsend_after_restart() + + def test_instantsend_after_restart(self): + self.log.info("Testing InstantSend works after full restart without new blocks") + + # node0 missed the EHF signal txs the quorum members created while it was disconnected from them + for mn_info in self.mninfo: + for txid in mn_info.get_node(self).getrawmempool(): + self.nodes[0].sendrawtransaction(mn_info.get_node(self).getrawtransaction(txid)) + + # fund sender with confirmed coins + sender = self.nodes[0] + receiver = self.nodes[0] + sender_addr = sender.getnewaddress() + fund_id = self.nodes[0].sendtoaddress(sender_addr, 1) + self.wait_for_instantlock(fund_id) + tip = self.generate(self.nodes[0], 2)[-1] + self.bump_mocktime(30) + self.wait_for_chainlocked_block_all_nodes(tip) + self.sync_blocks() + assert sender.getbalance() >= 0.5 + + receiver_addr = receiver.getnewaddress() + + # restart all nodes without mining new blocks + self.log.info("Restarting all nodes") + num_simple_nodes = self.num_nodes - self.mn_count + self.stop_nodes() + + for i in range(num_simple_nodes): + self.start_node(i) + for mn_info in self.mninfo: + self.start_masternode(mn_info) + + # reconnect: simple nodes to node 0, MNs to node 0 only. + # Quorum connections between MNs must be re-established automatically + # via InitializeCurrentBlockTip → EnsureQuorumConnections, NOT via + # manual connect_nodes between MN pairs. + for i in range(1, num_simple_nodes): + self.connect_nodes(i, 0) + for mn_info in self.mninfo: + self.connect_nodes(mn_info.nodeIdx, 0) + for i in range(num_simple_nodes): + force_finish_mnsync(self.nodes[i]) + + # bump past WAIT_FOR_ISLOCK_TIMEOUT so txFirstSeenTime loss doesn't + # block chainlock signing for TXs mined before restart + self.bump_mocktime(10 * 60 + 1) + self.sync_blocks() + + # Verify that MNs formed quorum connections to other MNs after restart. + # InitializeCurrentBlockTip → EnsureQuorumConnections must populate + # masternodeQuorumNodes so ThreadOpenMasternodeConnections establishes + # MN-to-MN links beyond the manual connections to node 0. + self.log.info("Verifying MN-to-MN quorum connections formed after restart") + for llmq_type, llmq_type_name in ((100, 'llmq_test'), (103, 'llmq_test_dip0024')): + for q in self.nodes[0].quorum('list')[llmq_type_name]: + members = self.get_quorum_masternodes(q, llmq_type) + for mn_info in members: + others = {m.proTxHash for m in members if m is not mn_info} + + def check_mn_peers(node=mn_info.get_node(self), others=others): + peers = [p['verified_proregtx_hash'] for p in node.getpeerinfo() if p.get('verified_proregtx_hash')] + # a member listed twice still has a duplicate connection on its way out, and a sig + # share queued to that one is lost + return others <= set(peers) and len(peers) == len(set(peers)) + self.wait_until(check_mn_peers, timeout=30) + + # re-grab references after restart + sender = self.nodes[0] + receiver = self.nodes[0] + + # send a TX — needs IS lock from all restarted MNs, no new blocks mined + is_id = sender.sendtoaddress(receiver_addr, 0.5) + self.wait_for_instantlock(is_id) + self.log.info("InstantSend lock succeeded after full restart") + + # clean up + receiver.sendtoaddress(self.nodes[0].getnewaddress(), 0.5, "", "", True) + self.bump_mocktime(30) + self.sync_mempools() + self.generate(self.nodes[0], 2) + def check_reconnects(self, expected_connection_count): self.log.info("disable and re-enable networking on all masternodes") for mn in self.mninfo: # type: MasternodeInfo diff --git a/test/functional/feature_mnehf.py b/test/functional/feature_mnehf.py index 47e95df78f42..cd4bcaecef1b 100755 --- a/test/functional/feature_mnehf.py +++ b/test/functional/feature_mnehf.py @@ -30,8 +30,10 @@ def add_options(self, parser): self.add_wallet_options(parser) def set_test_params(self): - extra_args = [["-vbparams=testdummy:0:999999999999:0:4:4:4:5:1", "-persistmempool=0"]] * 4 - self.set_dash_test_params(4, 3, extra_args=extra_args) + extra_args = [["-vbparams=testdummy:0:999999999999:0:4:4:4:5:1", "-persistmempool=0"]] * 2 + # One masternode is enough: the EHF signal only needs a recovered signature of any quorum + self.set_dash_test_params(2, 1, extra_args=extra_args) + self.set_dash_llmq_test_params(1, 1) def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -52,12 +54,13 @@ def restart_all_nodes(self, params=None): self.connect_nodes(i, 0) - def create_mnehf(self, versionBit, pubkey=None): + def create_mnehf(self, versionBit, pubkey=None, quorumHash=None): # request ID = sha256("mnhf", versionBit) request_id_buf = ser_string(b"mnhf") + struct.pack(" {os.path.join(self.alertnotify_dir, '%s')}", f"-blocknotify=echo > {os.path.join(self.blocknotify_dir, '%s')}", f"-shutdownnotify=echo > {self.shutdownnotify_file}", f"-chainlocknotify=echo > {os.path.join(self.chainlocknotify_dir, '%s')}", - ], [ + ] + self.extra_args[1] += [ f"-walletnotify=echo %h_%b > {os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s'))}", f"-instantsendnotify=echo > {os.path.join(self.instantsendnotify_dir, notify_outputname('%w', '%s'))}", - ], - [], [], [], []] + ] self.wallet_names = [self.default_wallet_name, self.wallet] super().setup_network() @@ -107,13 +109,8 @@ def run_test(self): self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0) self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", 4070908800) self.wait_for_sporks_same() - self.log.info("Mine quorum for InstantSend") - (quorum_info_i_0, quorum_info_i_1) = self.mine_cycle_quorum() - self.log.info("Mine quorum for ChainLocks") - if len(self.nodes[0].quorum('list')['llmq_test']) == 0: - self.mine_quorum(llmq_type_name='llmq_test', llmq_type=104) - else: - self.log.info("Quorum `llmq_test` already exist") + self.log.info("Mine quorums for InstantSend and ChainLocks") + self.mine_quorum_single_member() self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", 0) self.wait_for_sporks_same() diff --git a/test/functional/p2p_instantsend.py b/test/functional/p2p_instantsend.py index f85e2a87501d..d6c4d0325cd8 100755 --- a/test/functional/p2p_instantsend.py +++ b/test/functional/p2p_instantsend.py @@ -6,7 +6,7 @@ from test_framework.messages import msg_qsendrecsigs from test_framework.p2p import P2PInterface from test_framework.test_framework import DashTestFramework -from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync +from test_framework.util import assert_equal, assert_raises_rpc_error ''' p2p_instantsend.py @@ -37,7 +37,10 @@ def add_options(self, parser): self.add_wallet_options(parser) def set_test_params(self): - self.set_dash_test_params(8, 4) + # One masternode with single-member quorums: this test is about InstantSend on the non-masternode + # nodes, not about the quorums themselves + self.set_dash_test_params(5, 1, [["-llmqtestinstantsenddip0024=llmq_test_instantsend"]] * 5) + self.set_dash_llmq_test_params(1, 1) # set sender, receiver, isolated nodes self.isolated_idx = 1 self.receiver_idx = 2 @@ -46,18 +49,12 @@ def set_test_params(self): def run_test(self): self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0) self.wait_for_sporks_same() - self.log.info("Mine quorum for InstantSend") - (quorum_info_i_0, quorum_info_i_1) = self.mine_cycle_quorum() - self.log.info("Mine quorum for ChainLocks") - if len(self.nodes[0].quorum('list')['llmq_test']) == 0: - self.mine_quorum(llmq_type_name='llmq_test', llmq_type=104) - else: - self.log.info("Quorum `llmq_test` already exist") + self.log.info("Mine quorums for InstantSend and ChainLocks") + self.mine_quorum_single_member() self.test_mempool_doublespend() self.test_block_doublespend() self.test_isdlock_relayed_to_recsigs_observer() - self.test_instantsend_after_restart() def test_block_doublespend(self): sender = self.nodes[self.sender_idx] @@ -171,79 +168,5 @@ def test_isdlock_relayed_to_recsigs_observer(self): for node, _ in observers: node.disconnect_p2ps() - def test_instantsend_after_restart(self): - self.log.info("Testing InstantSend works after full restart without new blocks") - - # fund sender with confirmed coins - sender = self.nodes[self.sender_idx] - receiver = self.nodes[self.receiver_idx] - sender_addr = sender.getnewaddress() - fund_id = self.nodes[0].sendtoaddress(sender_addr, 1) - self.wait_for_instantlock(fund_id) - tip = self.generate(self.nodes[0], 2)[-1] - self.bump_mocktime(30) - self.wait_for_chainlocked_block_all_nodes(tip) - self.sync_blocks() - assert sender.getbalance() >= 0.5 - - receiver_addr = receiver.getnewaddress() - - # restart all nodes without mining new blocks - self.log.info("Restarting all nodes") - num_simple_nodes = self.num_nodes - self.mn_count - self.stop_nodes() - - for i in range(num_simple_nodes): - self.start_node(i) - for mn_info in self.mninfo: - self.start_masternode(mn_info) - - # reconnect: simple nodes to node 0, MNs to node 0 only. - # Quorum connections between MNs must be re-established automatically - # via InitializeCurrentBlockTip → EnsureQuorumConnections, NOT via - # manual connect_nodes between MN pairs. - for i in range(1, num_simple_nodes): - self.connect_nodes(i, 0) - for mn_info in self.mninfo: - self.connect_nodes(mn_info.nodeIdx, 0) - for i in range(num_simple_nodes): - force_finish_mnsync(self.nodes[i]) - - # bump past WAIT_FOR_ISLOCK_TIMEOUT so txFirstSeenTime loss doesn't - # block chainlock signing for TXs mined before restart - self.bump_mocktime(10 * 60 + 1) - self.sync_blocks() - - # Verify that MNs formed quorum connections to other MNs after restart. - # InitializeCurrentBlockTip → EnsureQuorumConnections must populate - # masternodeQuorumNodes so ThreadOpenMasternodeConnections establishes - # MN-to-MN links beyond the manual connections to node 0. - self.log.info("Verifying MN-to-MN quorum connections formed after restart") - for mn_info in self.mninfo: - mn_node = self.nodes[mn_info.nodeIdx] - - def check_mn_peers(node=mn_node, my_hash=mn_info.proTxHash): - peers = node.getpeerinfo() - mn_peers = set(p['verified_proregtx_hash'] for p in peers - if p.get('verified_proregtx_hash', '') != '') - other_mn_peers = mn_peers - {my_hash} - return len(other_mn_peers) > 0 - self.wait_until(check_mn_peers, timeout=30) - - # re-grab references after restart - sender = self.nodes[self.sender_idx] - receiver = self.nodes[self.receiver_idx] - - # send a TX — needs IS lock from all restarted MNs, no new blocks mined - is_id = sender.sendtoaddress(receiver_addr, 0.5) - self.wait_for_instantlock(is_id) - self.log.info("InstantSend lock succeeded after full restart") - - # clean up - receiver.sendtoaddress(self.nodes[0].getnewaddress(), 0.5, "", "", True) - self.bump_mocktime(30) - self.sync_mempools() - self.generate(self.nodes[0], 2) - if __name__ == '__main__': InstantSendTest().main() diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 53bd9798cf43..e96dbc09f983 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -2381,12 +2381,13 @@ def mine_quorum_single_member(self): node = self.nodes[0] quorums = node.quorum('list')['llmq_test'] + self.mine_until_mns_confirmed_for_next_dkg() skip_count = 24 - (node.getblockcount() % 24) if skip_count != 0: self.bump_mocktime(1) - self.generate(node, skip_count) + self.generate(node, skip_count, sync_fun=lambda: self.sync_blocks()) time.sleep(1) - self.generate(node, 30) + self.generate(node, 30, sync_fun=lambda: self.sync_blocks()) new_quorums_list = node.quorum('list')['llmq_test'] self.log.info(f"Test Quorums at height={node.getblockcount()} : {new_quorums_list}") From 7d1a90dc6964ca61dc6d48dafcb3586f907e339d Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 11:46:24 -0400 Subject: [PATCH 002/153] Merge #7791: fix(net): request object votes from nPeersPerHashMax peers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit aaa53012ca7a774bbf545d617fe5228eef0bc9b8 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` (77faacce82821f2f0493f7106a100cc0cc1ff7e8, 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 c14104b172a2: - [`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 77faacce 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.
Unfixed (c14104b172a2): only the first peer is ever asked ``` $ 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 ```
With this PR: three peers are asked, the fourth is not ``` $ 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 ```
### 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=` 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 a8b44a1e83aaac55873babf00dc5ea049955ac7d) --- src/node/sync_manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/sync_manager.cpp b/src/node/sync_manager.cpp index 4eb1598e284f..2f11b9f3cb31 100644 --- a/src/node/sync_manager.cpp +++ b/src/node/sync_manager.cpp @@ -83,7 +83,7 @@ int SyncManager::RequestGovernanceObjectVotes(const std::vector& vNodesC static std::map> mapAskedRecently GUARDED_BY(cs_recently); LOCK(cs_recently); - auto [vTriggerObjHashes, vOtherObjHashes] = m_gov_manager.FetchGovernanceObjectVotes(nMaxObjRequestsPerNode, nNow, + auto [vTriggerObjHashes, vOtherObjHashes] = m_gov_manager.FetchGovernanceObjectVotes(nPeersPerHashMax, nNow, mapAskedRecently); if (vTriggerObjHashes.empty() && vOtherObjHashes.empty()) return -2; From 40de493fdc76c662044abe7603ed2f9b2b8b218b Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 11:55:37 -0400 Subject: [PATCH 003/153] Merge #7781: fix(rpc): use a consistent tip height in quorum dkgstatus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9581df5987d7be78f0c1745b02d2d5ff32859e94 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 #7770 the RPC passed `tipHeight`, so it took a two-block rollback to hit the assert. #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 ` / `reconsiderblock `. 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.
Reproduction script and output ```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 ```
### 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 #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 #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 8ad2c07b35cf175d8ecf8841835b84fedc5787f5) --- src/rpc/quorums.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index 67e9da8de518..2100cbca2668 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -387,7 +387,7 @@ static RPCHelpMan quorum_dkgstatus() if (node.active_ctx) { int quorumHeight = tipHeight - (tipHeight % llmq_params.dkgInterval) + quorumIndex; if (quorumHeight <= tipHeight) { - const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(cs_main, return chainman.ActiveChain()[quorumHeight]); + const CBlockIndex* pQuorumBaseBlockIndex = pindexTip->GetAncestor(quorumHeight); obj.pushKV("pQuorumBaseBlockIndex", pQuorumBaseBlockIndex->nHeight); obj.pushKV("quorumHash", pQuorumBaseBlockIndex->GetBlockHash().ToString()); obj.pushKV("pindexTip", pindexTip->nHeight); @@ -427,7 +427,8 @@ static RPCHelpMan quorum_dkgstatus() } LOCK(cs_main); - std::optional> vfqc = llmq_ctx.quorum_block_processor->GetMineableCommitments(llmq_params, tipHeight + 1); + std::optional> vfqc = + llmq_ctx.quorum_block_processor->GetMineableCommitments(llmq_params, chainman.ActiveChain().Height() + 1); if (vfqc.has_value()) { for (const auto& fqc : vfqc.value()) { minableCommitments.push_back(fqc.ToJson()); From 73f6ea25c711f3705f53b48be78f9ab9e47a3eb4 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 12:09:09 -0400 Subject: [PATCH 004/153] Merge #7789: fix(qt): report malformed lang/font settings instead of aborting at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cdeadac4f83bd004130c9a935fbbcdd643d5ac07 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 c14104b172a2: - `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 c14104b172a2 (macOS, `--with-gui`, debug build):
dash-qt with {"lang": {}} in settings.json (unfixed) ``` $ 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.
dash-qt with {"font-weight-normal": []} in settings.json and -font-family on the command line (unfixed) ``` $ 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 ```
The new `OptionTests` cases, run against the unfixed `OptionsModel::Init`:
test_dash-qt (unfixed), default minimal platform ``` $ 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 ```
test_dash-qt (unfixed), QT_QPA_PLATFORM=cocoa so fonts are loaded ``` $ 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 ```
### 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 "/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 "/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 c932b8cd91759838ca9749c43fbaa305d78ae78a) --- src/qt/bitcoin.cpp | 7 +++- src/qt/optionsmodel.cpp | 64 ++++++++++++++++++++++++++----------- src/qt/test/optiontests.cpp | 55 +++++++++++++++++++++++++++++++ src/qt/test/optiontests.h | 2 ++ 4 files changed, 109 insertions(+), 19 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 734a45a6c9d8..61c4e825ea43 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -119,7 +119,12 @@ static QString GetLangTerritory() if(!lang_territory_qsettings.isEmpty()) lang_territory = lang_territory_qsettings; // 3) -lang command line argument - lang_territory = QString::fromStdString(gArgs.GetArg("-lang", lang_territory.toStdString())); + try { + lang_territory = QString::fromStdString(gArgs.GetArg("-lang", lang_territory.toStdString())); + } catch (const std::exception&) { + // Keep the language selected above if "lang" in settings.json has an + // unexpected type. OptionsModel::Init reports it as an error. + } return lang_territory; } diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index e0f5adc3a690..12e077381cb7 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -204,8 +204,20 @@ void OptionsModel::addOverriddenOption(const std::string &option) // Writes all missing QSettings with their default values bool OptionsModel::Init(bilingual_str& error) { + // Handles exceptions thrown by univalue that can happen if settings in + // settings.json don't have the expected types. + const auto setting_error = [&](const std::string& setting, const std::exception& e) { + error.original = strprintf("Could not read setting \"%s\", %s.", setting, e.what()); + error.translated = tr("Could not read setting \"%1\", %2.").arg(QString::fromStdString(setting), e.what()).toStdString(); + return false; + }; + // Initialize display settings from stored settings. - language = QString::fromStdString(SettingToString(node().getPersistentSetting("lang"), "")); + try { + language = QString::fromStdString(SettingToString(node().getPersistentSetting("lang"), "")); + } catch (const std::exception& e) { + return setting_error("lang", e); + } checkAndMigrate(); @@ -256,9 +268,13 @@ bool OptionsModel::Init(bilingual_str& error) addOverriddenOption("-font-family"); } if (GUIUtil::fontsLoaded()) { - const QString font_name = QString::fromStdString( - SettingToString(node().getPersistentSetting("font-family"), "")); - GUIUtil::setActiveFont(font_name); + try { + const QString font_name = QString::fromStdString( + SettingToString(node().getPersistentSetting("font-family"), "")); + GUIUtil::setActiveFont(font_name); + } catch (const std::exception& e) { + return setting_error("font-family", e); + } } // Font Scale @@ -266,7 +282,11 @@ bool OptionsModel::Init(bilingual_str& error) addOverriddenOption("-font-scale"); } if (GUIUtil::fontsLoaded()) { - GUIUtil::setFontScale(SettingToInt(node().getPersistentSetting("font-scale"), GUIUtil::defaultFontScale())); + try { + GUIUtil::setFontScale(SettingToInt(node().getPersistentSetting("font-scale"), GUIUtil::defaultFontScale())); + } catch (const std::exception& e) { + return setting_error("font-scale", e); + } } // Font Weight (Normal) @@ -276,11 +296,17 @@ bool OptionsModel::Init(bilingual_str& error) const bool override_family{isOptionOverridden("-font-family")}; if (GUIUtil::fontsLoaded()) { - // If font was overridden by CLI but weight wasn't, use the font's default weight const int default_arg = GUIUtil::defaultWeightArg(GUIUtil::FontWeight::Normal); - const int arg = (!override_family || isOptionOverridden("-font-weight-normal")) - ? SettingToInt(node().getPersistentSetting("font-weight-normal"), default_arg) - : default_arg; + // Read the stored value even when it is not used below, so that a + // malformed one is reported here rather than thrown later via gArgs. + int setting_arg; + try { + setting_arg = SettingToInt(node().getPersistentSetting("font-weight-normal"), default_arg); + } catch (const std::exception& e) { + return setting_error("font-weight-normal", e); + } + // If font was overridden by CLI but weight wasn't, use the font's default weight + const int arg = (!override_family || isOptionOverridden("-font-weight-normal")) ? setting_arg : default_arg; if (!GUIUtil::setWeightFromArg(GUIUtil::FontWeight::Normal, arg)) { node().forceSetting("font-weight-normal", default_arg); GUIUtil::setWeightFromArg(GUIUtil::FontWeight::Normal, default_arg); @@ -292,11 +318,17 @@ bool OptionsModel::Init(bilingual_str& error) addOverriddenOption("-font-weight-bold"); } if (GUIUtil::fontsLoaded()) { - // If font was overridden by CLI but weight wasn't, use the font's default weight const int default_arg = GUIUtil::defaultWeightArg(GUIUtil::FontWeight::Bold); - const int arg = (!override_family || isOptionOverridden("-font-weight-bold")) - ? SettingToInt(node().getPersistentSetting("font-weight-bold"), default_arg) - : default_arg; + // Read the stored value even when it is not used below, so that a + // malformed one is reported here rather than thrown later via gArgs. + int setting_arg; + try { + setting_arg = SettingToInt(node().getPersistentSetting("font-weight-bold"), default_arg); + } catch (const std::exception& e) { + return setting_error("font-weight-bold", e); + } + // If font was overridden by CLI but weight wasn't, use the font's default weight + const int arg = (!override_family || isOptionOverridden("-font-weight-bold")) ? setting_arg : default_arg; if (!GUIUtil::setWeightFromArg(GUIUtil::FontWeight::Bold, arg)) { node().forceSetting("font-weight-bold", default_arg); GUIUtil::setWeightFromArg(GUIUtil::FontWeight::Bold, default_arg); @@ -361,11 +393,7 @@ bool OptionsModel::Init(bilingual_str& error) try { getOption(option); } catch (const std::exception& e) { - // This handles exceptions thrown by univalue that can happen if - // settings in settings.json don't have the expected types. - error.original = strprintf("Could not read setting \"%s\", %s.", setting, e.what()); - error.translated = tr("Could not read setting \"%1\", %2.").arg(QString::fromStdString(setting), e.what()).toStdString(); - return false; + return setting_error(setting, e); } } diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index fba2d2f118f6..626b5d1079d1 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -101,6 +101,61 @@ void OptionTests::integerGetArgBug() gArgs.WriteSettingsFile(); } +void OptionTests::invalidFontSetting() +{ + if (QApplication::platformName() == "minimal") { + QSKIP("AppTests cannot initialize fonts with the 'minimal' platform plugin."); + } + + // Font settings are only read once loadFonts() has run, which AppTests does. + QVERIFY2(GUIUtil::fontsLoaded(), + "GUIUtil::loadFonts() must succeed in AppTests::appTests() before OptionTests run."); + + for (const std::string setting : {"font-family", "font-scale", "font-weight-normal", "font-weight-bold"}) { + gArgs.LockSettings([&](util::Settings& settings) { + settings.rw_settings[setting] = UniValue{UniValue::VARR}; + }); + bilingual_str error; + QVERIFY(!OptionsModel{m_node}.Init(error)); + QCOMPARE(QString::fromStdString(error.original), + QString::fromStdString(strprintf("Could not read setting \"%s\", JSON value of type array is not of expected type string.", setting))); + gArgs.LockSettings([&](util::Settings& settings) { + settings.rw_settings.erase(setting); + }); + } + + // With -font-family on the command line the stored weights are not used, but + // dash-qt still reads them through gArgs later, so they must be reported too. + for (const std::string setting : {"font-weight-normal", "font-weight-bold"}) { + gArgs.LockSettings([&](util::Settings& settings) { + settings.command_line_options["font-family"] = {UniValue{GUIUtil::defaultFontFamily().toStdString()}}; + settings.rw_settings[setting] = UniValue{UniValue::VARR}; + }); + bilingual_str error; + QVERIFY(!OptionsModel{m_node}.Init(error)); + QCOMPARE(QString::fromStdString(error.original), + QString::fromStdString(strprintf("Could not read setting \"%s\", JSON value of type array is not of expected type string.", setting))); + gArgs.LockSettings([&](util::Settings& settings) { + settings.command_line_options.erase("font-family"); + settings.rw_settings.erase(setting); + }); + } +} + +void OptionTests::invalidLangSetting() +{ + // A "lang" value with an unexpected type in settings.json must be reported + // through the error string like other settings instead of throwing out of + // OptionsModel::Init. + gArgs.LockSettings([&](util::Settings& settings) { + settings.rw_settings["lang"] = UniValue{UniValue::VOBJ}; + }); + bilingual_str error; + QVERIFY(!OptionsModel{m_node}.Init(error)); + QCOMPARE(QString::fromStdString(error.original), + QString{"Could not read setting \"lang\", JSON value of type object is not of expected type string."}); +} + void OptionTests::parametersInteraction() { // Test that the bug https://github.com/bitcoin-core/gui/issues/567 does not resurface. diff --git a/src/qt/test/optiontests.h b/src/qt/test/optiontests.h index 89976d97d0b3..2cee85ab5903 100644 --- a/src/qt/test/optiontests.h +++ b/src/qt/test/optiontests.h @@ -21,6 +21,8 @@ private Q_SLOTS: void init(); // called before each test function execution. void migrateSettings(); void integerGetArgBug(); + void invalidFontSetting(); + void invalidLangSetting(); void parametersInteraction(); void extractFilter(); void effectivePointSize(); From a1d7411b734c0d97559b0baa12ff122e0b697bf6 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 12:15:54 -0400 Subject: [PATCH 005/153] Merge #7779: fix!: report legacy evonode Platform addresses when listdiff changes its address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 18c08a6b7199cbb85b2a1aee2097cc22cdf863be test: add missing negative test for protx diff (Konstantin Akimov) 26a04403fcc42fdc70d3ccdcf8b65ebeec4b34da docs: v24 had not been released yet, no need to explain detailed difference in release notes (Konstantin Akimov) 1eae1b690c8455ebc1ade8dcba7778faff95998e test: use existing test suite to check new address invariants rather than filling them from scratch (Konstantin Akimov) bec74ca82c9ca88330b11c27b133c72d3cc9d23a refactor: move extra logic with addresses to rpc/ where it's used (Konstantin Akimov) 1aab06b2ee83d478aca1eab49a9f05cd7cfe2d97 doc: add release notes for protx listdiff legacy evonode Platform addresses (Ivan Shumkov) ac2d836b07d463df307e55d4917f4a5f1dbd68d6 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 `[::]:`. - `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 18c08a6b7199cbb85b2a1aee2097cc22cdf863be kwvg: utACK 18c08a6b7199cbb85b2a1aee2097cc22cdf863be Tree-SHA512: 9e9007b75aed678112026d8a0b1257c136294fff7973816cc9f5ce58b4e72f21d580e629ab86da16acd061ae98a346740325c31d25b8bfa326ca8b33dffa9ada (cherry picked from commit 0021b207b228bdc539ecaa788589a14dcfc533df) --- doc/release-notes-7779.md | 6 ++++++ src/rpc/evo.cpp | 10 ++++++++-- test/functional/rpc_netinfo.py | 18 ++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 doc/release-notes-7779.md diff --git a/doc/release-notes-7779.md b/doc/release-notes-7779.md new file mode 100644 index 000000000000..6358edcfd0d5 --- /dev/null +++ b/doc/release-notes-7779.md @@ -0,0 +1,6 @@ +Updated RPCs +------------ + +- `protx listdiff` reports the complete `addresses` object, `platform_p2p` and `platform_https` included, + when the core P2P address of an EvoNode on legacy (pre-extended) addresses changes, so its Platform + ports can be followed without `-deprecatedrpc=service`. (#7779) diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index ffb820e119ce..ce043a8d9488 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1959,12 +1959,18 @@ static RPCHelpMan protx_listdiff() UniValue jupdatedMNs(UniValue::VARR); for(const auto& [internal_id, stateDiff] : mnDiff.updatedMNs) { - auto dmn = baseBlockMNList.GetMNByInternalId(internal_id); + auto dmn = blockMNList.GetMNByInternalId(internal_id); // BuildDiff will construct itself with MNs that we already have knowledge // of, meaning that fetch operations should never fail. CHECK_NONFATAL(dmn); + UniValue diff_json{stateDiff.ToJson(dmn->nType)}; + if ((stateDiff.fields & CDeterministicMNStateDiff::Field_netInfo) && !dmn->pdmnState->netInfo->IsEmpty()) { + // A legacy EvoNode's Platform addresses pair its core P2P address with the Platform ports, + // which the diff doesn't carry unless they changed too + diff_json.pushKV("addresses", GetNetInfoWithLegacyFields(*dmn->pdmnState, dmn->nType)); + } UniValue obj(UniValue::VOBJ); - obj.pushKV(dmn->proTxHash.ToString(), stateDiff.ToJson(dmn->nType)); + obj.pushKV(dmn->proTxHash.ToString(), diff_json); jupdatedMNs.push_back(obj); } ret.pushKV("updatedMNs", jupdatedMNs); diff --git a/test/functional/rpc_netinfo.py b/test/functional/rpc_netinfo.py index 60483b727a43..37cbb188318f 100755 --- a/test/functional/rpc_netinfo.py +++ b/test/functional/rpc_netinfo.py @@ -432,6 +432,7 @@ def test_deprecation(self): # updating Platform fields to trigger the conditions needed to report the dummy address proupservtx_hash = self.node_evo.update_mn(self, True, f"127.0.0.1:{self.node_evo.mn.nodePort + 10}", DEFAULT_PORT_PLATFORM_P2P + 10, DEFAULT_PORT_PLATFORM_HTTP + 10) proupservtx_rpc = self.node_evo.node.getrawtransaction(proupservtx_hash, True) + proupservtx_height_first = proupservtx_rpc['height'] # Restore back to defaults proupservtx_hash = self.node_evo.update_mn(self, True, f"127.0.0.1:{self.node_evo.mn.nodePort}", DEFAULT_PORT_PLATFORM_P2P, DEFAULT_PORT_PLATFORM_HTTP) @@ -441,6 +442,11 @@ def test_deprecation(self): proupservtx_hash_pl = self.node_evo.update_mn(self, True, f"127.0.0.1:{self.node_evo.mn.nodePort}", DEFAULT_PORT_PLATFORM_P2P + 10, DEFAULT_PORT_PLATFORM_HTTP + 10) proupservtx_rpc_pl = self.node_evo.node.getrawtransaction(proupservtx_hash_pl, True) + # Revoke the operator key, which clears the core P2P address but keeps the Platform fields + prouprevtx_hash = self.node_evo.mn.revoke(self.node_evo.node, submit=True, reason=0, fundsAddr=self.node_evo.mn.fundsAddr) + self.node_evo.bury_tx(self, self.node_evo.node, prouprevtx_hash) + prouprevtx_height = self.node_evo.node.getrawtransaction(prouprevtx_hash, True)['height'] + # CSimplifiedMNListEntry::ToJson() <- CSimplifiedMNListDiff::mnList <- CSimplifiedMNListDiff::ToJson() <- protx_diff masternode_active_height: int = masternode_status['dmnState']['registeredHeight'] protx_diff_rpc = self.node_evo.node.protx('diff', masternode_active_height - 1, masternode_active_height) @@ -455,6 +461,13 @@ def test_deprecation(self): protx_listdiff_rpc_pl = self.node_evo.node.protx('listdiff', proupservtx_height_pl - 1, proupservtx_height_pl) protx_listdiff_rpc_pl = self.extract_from_listdiff(protx_listdiff_rpc_pl, proregtx_hash) + # The first and the third update set the same platform fields, so between them only the core P2P address differs + protx_listdiff_rpc_addr = self.node_evo.node.protx('listdiff', proupservtx_height_first, proupservtx_height_pl) + protx_listdiff_rpc_addr = self.extract_from_listdiff(protx_listdiff_rpc_addr, proregtx_hash) + + protx_listdiff_rpc_rev = self.node_evo.node.protx('listdiff', prouprevtx_height - 1, prouprevtx_height) + protx_listdiff_rpc_rev = self.extract_from_listdiff(protx_listdiff_rpc_rev, proregtx_hash) + self.log.info("Test RPCs return an 'addresses' field") assert "addresses" in proregtx_rpc['proRegTx'].keys() assert "addresses" in masternode_status['dmnState'].keys() @@ -474,6 +487,11 @@ def test_deprecation(self): assert "core_p2p" not in protx_listdiff_rpc_pl['addresses'].keys() assert_equal(protx_listdiff_rpc_pl['addresses']['platform_https'][0], f"{DMNSTATE_DIFF_DUMMY_ADDR}:{DEFAULT_PORT_PLATFORM_HTTP + 10}") assert_equal(protx_listdiff_rpc_pl['addresses']['platform_p2p'][0], f"{DMNSTATE_DIFF_DUMMY_ADDR}:{DEFAULT_PORT_PLATFORM_P2P + 10}") + # If only the core P2P address was updated, the platform fields are still reported, paired with the new address + self.check_netinfo_fields(protx_listdiff_rpc_addr['addresses'], self.node_evo.mn.nodePort, DEFAULT_PORT_PLATFORM_HTTP + 10, DEFAULT_PORT_PLATFORM_P2P + 10) + # If the core P2P address was cleared, there is nothing to pair the platform fields with and 'addresses' is omitted + assert_equal(self.node_evo.node.protx('info', proregtx_hash)['state']['addresses'], {}) + assert "addresses" not in protx_listdiff_rpc_rev.keys() self.log.info("Test RPCs by default no longer return a 'service' field") assert "service" not in proregtx_rpc['proRegTx'].keys() From 89d44c8723cafa1f484eb172d3a718209f1fe32b Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 12:20:59 -0400 Subject: [PATCH 006/153] Merge #7795: backport: bitcoin#34272, partial bitcoin#31650 (pass PSBT by const reference in PSBTInputSignedAndVerified) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4b9436771ee5aeef679613fe46ec98debeb79827 partial Merge bitcoin/bitcoin#31650: refactor: Avoid copies by using const references or by move-construction (pasta) 2cb6f5aef47ca719cc172f45b354595cb122c5b7 Merge bitcoin/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 (2aa0e7b3cb4d), 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 c14104b172a2: - 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).
Script ```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(...) ```
Unfixed (c14104b172a2): each doubling of the input count costs 4x ``` 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) ```
With this PR: linear, over 250x faster at 16000 inputs ``` 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) ```
### 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, 2f5b1c5f8059): changes the bounds assertion in `PSBTInputSignedAndVerified()` to `assert(input_index < psbt.inputs.size())`. - bitcoin#31650 (partial): only the `src/psbt.{h,cpp}` hunk of faf0c2d942c8, which changes the parameter to `const PartiallySignedTransaction& psbt`. Omitted from bitcoin#31650: - The rest of faf0c2d942c8: `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&`) 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 fa64d8424b8d (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 464722042c487ea87b91e35962856eba5a648138) --- src/psbt.cpp | 4 ++-- src/psbt.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/psbt.cpp b/src/psbt.cpp index 2ba214b74d19..fdc2c4f149b7 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -181,10 +181,10 @@ void PSBTOutput::Merge(const PSBTOutput& output) if (redeem_script.empty() && !output.redeem_script.empty()) redeem_script = output.redeem_script; } -bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned int input_index, const PrecomputedTransactionData* txdata) +bool PSBTInputSignedAndVerified(const PartiallySignedTransaction& psbt, unsigned int input_index, const PrecomputedTransactionData* txdata) { CTxOut utxo; - assert(psbt.inputs.size() >= input_index); + assert(input_index < psbt.inputs.size()); const PSBTInput& input = psbt.inputs[input_index]; if (input.non_witness_utxo) { diff --git a/src/psbt.h b/src/psbt.h index 4777289c9114..a79a8f3a1456 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -869,7 +869,7 @@ PrecomputedTransactionData PrecomputePSBTData(const PartiallySignedTransaction& bool PSBTInputSigned(const PSBTInput& input); /** Checks whether a PSBTInput is already signed by doing script verification using final fields. */ -bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned int input_index, const PrecomputedTransactionData* txdata); +bool PSBTInputSignedAndVerified(const PartiallySignedTransaction& psbt, unsigned int input_index, const PrecomputedTransactionData* txdata); /** Signs a PSBTInput, verifying that all provided data matches what is being signed. * From d9f659283d066cacae25e07170caf94d10810c2e Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 13:08:08 -0400 Subject: [PATCH 007/153] Merge #7783: fix: stop fish completion from re-running the typed dash-cli command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5339d4ff6c2a30d56299bba1b3c81b923595a231 refactor(contrib): match dash-cli connection options by prefix in fish completion (pasta) ff98899da896f82c19a136e2b1551e3fa4384a9a 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
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 c14104b172a2: - [`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 ''`, which evaluates the completion rules exactly as an interactive Tab does. Enter is never pressed. Script: `regtest-repro.sh` (included below).
Unfixed: one Tab after a typed sendtoaddress line sends 5 DASH ``` $ git rev-parse --short HEAD c14104b172a2 $ 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 ```
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:
Unfixed: commands run by the completion for one Tab ``` $ git rev-parse --short HEAD c14104b172a2 $ 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 ```
regtest-repro.sh ```sh #!/bin/sh # Usage: regtest-repro.sh # Starts a fresh regtest dashd from /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 ```
### 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 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 ` and option completion return the same suggestions as before.
Fixed: same script ``` $ 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 ```
Fixed: commands run by the completion for one Tab (logging stand-in) ``` $ 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 ```
- After switching to prefix matching (second commit), the regtest check was repeated: command, `help `, 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 3789ec0c719e25a595d108c3caf77ed6681f4097) --- contrib/completions/fish/dash-cli.fish | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/contrib/completions/fish/dash-cli.fish b/contrib/completions/fish/dash-cli.fish index 9a059d5f4ed4..8353506726a9 100644 --- a/contrib/completions/fish/dash-cli.fish +++ b/contrib/completions/fish/dash-cli.fish @@ -9,14 +9,20 @@ function __fish_dash_cli_get_commands_helper return end - # Strip help cmd from token to avoid duplication errors - set --local cmd (string match --invert --regex -- '^help$' $cmd) - # Strip -stdin* options to avoid waiting for input while we fetch completions - # TODO: this appears to be broken when run as tab completion (requires ctrl+c to exit) - set --local cmd (string match --invert --regex -- '^-stdin.*$' $cmd) + # Only keep the connection options given before the RPC command. Never pass + # on the typed command or its arguments: they would be executed on every Tab + # (e.g. `sendtoaddress
` would send funds). + # -rpcwait* is dropped so Tab doesn't hang while the node is down. + set --local base $cmd[1] + for arg in $cmd[2..-1] + string match --quiet --regex -- '^-' $arg; or break + if string match --quiet --regex -- '^--?(conf|datadir|testnet|regtest|devnet|chain|rpc(?!wait))' $arg + set --append base $arg + end + end # Match, format and return commands - for command in ($cmd help 2>&1 | string match --invert -r '^\=\=.*' | string match --invert -r '^\\s*$') + for command in ($base help 2>&1 | string match --invert -r '^\=\=.*' | string match --invert -r '^\\s*$') echo $command end end @@ -60,9 +66,9 @@ function __fish_dash_cli_get_options set --local options if set -q _flag_nofiles - set --append options ($cmd -help 2>&1 | string match -r '^ -.*' | string replace -r ' -' '-' | string replace -r '=.*' '=' | string match --invert -r '^.*=$') + set --append options ($cmd[1] -help 2>&1 | string match -r '^ -.*' | string replace -r ' -' '-' | string replace -r '=.*' '=' | string match --invert -r '^.*=$') else - set --append options ($cmd -help 2>&1 | string match -r '^ -.*' | string replace -r ' -' '-' | string replace -r '=.*' '=' | string match -r '^.*=$') + set --append options ($cmd[1] -help 2>&1 | string match -r '^ -.*' | string replace -r ' -' '-' | string replace -r '=.*' '=' | string match -r '^.*=$') end for option in $options From 2d1735dd7ed90128222aa295c2e78a2a2fc929e4 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 13:12:09 -0400 Subject: [PATCH 008/153] Merge #7794: fix(wallet): floor the denomination gap threshold so rebalancing cannot oscillate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 076b093a7c32f669af85ff19243aeaf13cb97714 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 (c14104b172a2) 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:
Unfixed run (abridged) ``` $ 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.
### 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 c14104b172a2 + 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 7964c37ffd4a970a7aa0510a6a77f4cf4fa65629) --- src/coinjoin/common.h | 12 ++++++++++-- src/test/coinjoin_inouts_tests.cpp | 23 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/coinjoin/common.h b/src/coinjoin/common.h index 3a3b3177bc90..8765c4633f84 100644 --- a/src/coinjoin/common.h +++ b/src/coinjoin/common.h @@ -120,15 +120,23 @@ constexpr CAmount GetMaxCollateralAmount() { return GetCollateralAmount() * 4; } // Promotion/demotion constants (post-V24 feature) constexpr int PROMOTION_RATIO = 10; // 10 smaller denomination coins = 1 larger denomination coin constexpr int GAP_DIVISOR = 5; // Deficit gap required to trigger promotion/demotion, as 1/N of the goal +/** + * Smallest gap that stops a conversion from making the opposite conversion worthwhile. A + * conversion needs the receiving deficit to exceed the other by more than the gap, and it moves + * the two deficits by at most PROMOTION_RATIO and 1, so reversing it right away would need + * 2 * gap + 1 <= PROMOTION_RATIO. Any gap with 2 * gap + 1 > PROMOTION_RATIO rules that out. + */ +constexpr int MIN_GAP_THRESHOLD = (PROMOTION_RATIO + 1) / 2; /** * How far behind the other denomination a denomination has to be before converting is worth it; * the gap is what keeps promotion and demotion from oscillating. It is a fraction of the goal * rather than a constant because the largest gap two denominations can show is the goal itself: * a constant of 10 was unsatisfiable at MIN_COINJOIN_DENOMS_GOAL (also 10) and silently - * disabled the feature there. At the default goal of 50 this still yields 10. + * disabled the feature there. At the default goal of 50 this still yields 10; below a goal of 25 + * it is floored at MIN_GAP_THRESHOLD. */ -constexpr int GetGapThreshold(int nGoal) { return std::max(nGoal / GAP_DIVISOR, 1); } +constexpr int GetGapThreshold(int nGoal) { return std::max(nGoal / GAP_DIVISOR, MIN_GAP_THRESHOLD); } /** * Which side(s) of the session denomination a participant occupies. A standard entry mixes at diff --git a/src/test/coinjoin_inouts_tests.cpp b/src/test/coinjoin_inouts_tests.cpp index ee7b268831f6..9bf2f803869b 100644 --- a/src/test/coinjoin_inouts_tests.cpp +++ b/src/test/coinjoin_inouts_tests.cpp @@ -19,6 +19,7 @@ #include