Skip to content

feat: support descriptor wallets for RPC governance votemany, votealias - #6094

Merged
PastaPastaPasta merged 2 commits into
dashpay:developfrom
knst:bp-descriptors-8-governance
Jul 16, 2024
Merged

feat: support descriptor wallets for RPC governance votemany, votealias#6094
PastaPastaPasta merged 2 commits into
dashpay:developfrom
knst:bp-descriptors-8-governance

Conversation

@knst

@knst knst commented Jul 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

RPCs governance votemany and governance votealias use forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as

test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)

See https://github.com/dashpay/dash-issues/issues/59 to track progress

What was done?

Use CWallet's interfaces instead LegacyScriptPubKeyMan

How Has This Been Tested?

Functional tests feature_governance.py and feature_governance_cl.py to run by both ways - legacy and descriptor wallets.

Run unit and functional tests.

Extra test done locally:

--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
         if self.options.descriptors is None:
             # Prefer BDB unless it isn't available
-            if self.is_bdb_compiled():
-                self.options.descriptors = False
-            elif self.is_sqlite_compiled():
+            if self.is_sqlite_compiled():
                 self.options.descriptors = True
+            elif self.is_bdb_compiled():
+                self.options.descriptors = False

to flip flag descriptor wallets/legacy wallets for all functional tests.

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst added this to the 21 milestone Jul 4, 2024
@knst
knst force-pushed the bp-descriptors-8-governance branch from 101da69 to c72ec70 Compare July 5, 2024 08:37
@PastaPastaPasta

PastaPastaPasta commented Jul 6, 2024

Copy link
Copy Markdown
Member

Any reason this should be in v21 instead of 21.1?

UdjinM6
UdjinM6 approved these changes Jul 6, 2024
View reviewed changes

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c72ec70

@PastaPastaPasta PastaPastaPasta modified the milestones: 21, 21.1 Jul 7, 2024
PastaPastaPasta
PastaPastaPasta approved these changes Jul 7, 2024
View reviewed changes

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c72ec70

@PastaPastaPasta
PastaPastaPasta merged commit f16025f into dashpay:develop Jul 16, 2024
@UdjinM6 UdjinM6 modified the milestones: 21.1, 21.2 Aug 8, 2024
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
@PastaPastaPasta PastaPastaPasta mentioned this pull request Jul 30, 2026
Merged
5 tasks
PastaPastaPasta added a commit that referenced this pull request Aug 1, 2026
@PastaPastaPasta
Merge #7497: test: segregate wallet-mode coverage and deduplicate fun…
627f82f 
…ctional test runs

850aeba test: address review feedback on wallet-mode segregation (UdjinM6)
95996ba test: skip non-spork23-gated banning section in simplepose spork23 variant (pasta)
436bcfa test: merge spork21 variant of feature_llmq_signing into a single run (pasta)
6980795 test: run feature_dip3_deterministicmns in a single wallet mode (pasta)
60f0c38 test: run governance functional tests in a single wallet mode (pasta)
b052e85 test: add wallet_dash_rpcs.py covering wallet-mode-divergent Dash RPC paths (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  Functional tests account for roughly 79 minutes of serial runtime per CI job, and several whole tests are run twice with configurations whose divergent surface is only a small fraction of the test. The duplicated runs cost roughly 450 seconds of serial time per job:

  - `feature_governance.py`, `feature_governance_cl.py` and `feature_dip3_deterministicmns.py` run in both `--legacy-wallet` and `--descriptors` modes. This dates back to the descriptor-wallet support series (#6003, #6094): these tests were the regression tests for descriptor vote signing and protx funding, so both modes were wired up (c72ec70 added `feature_governance.py --descriptors` in the same commit that implemented descriptor vote signing). The tests themselves are overwhelmingly consensus-side (superblock budgets, trigger creation, DMN list updates, reorgs, payment enforcement) and wallet-mode-agnostic; only the RPC entry points that fund, look up keys and sign through the wallet differ per wallet mode.
  - `feature_llmq_signing.py` runs twice (default and `--spork21`), duplicating the entire signing-session flow which does not depend on the spork.
  - `feature_llmq_simplepose.py --disable-spork23` re-runs the contribution-miss banning section even though that ban path is not gated on spork23.

  ## What was done?

  Wallet coverage is segregated from consensus coverage, and per-test duplication is removed where the second run added no coverage:

  1. **New `wallet_dash_rpcs.py`**, run in both `--legacy-wallet` and `--descriptors` modes (~11-15s per mode). It covers exactly the wallet-mode-divergent Dash RPC surface: `gobject prepare` (wallet collateral funding) -> `list-prepared` -> `submit`; `gobject vote-many`/`vote-alias` (`CheckWalletOwnsKey`/`IsMine` lookup and `CWallet::SignGovernanceVote` -> `SignMessage` SPKM dispatch); `protx register_fund`; the external-collateral path `protx register_prepare` + `signmessage` + `register_submit`; `protx update_service`; `protx update_registrar`. Small 2-masternode topology, no quorums, no superblock cycles.
  2. **`feature_governance.py` and `feature_governance_cl.py` run in `--descriptors` mode only.** The legacy-wallet coverage of the governance wallet surface is carried by `wallet_dash_rpcs.py`.
  3. **`feature_dip3_deterministicmns.py` runs in `--descriptors` mode only**, same justification for the protx wallet surface.
  4. **`feature_llmq_signing.py` merged into a single run.** The shared flow runs with spork21 off; SPORK_21_QUORUM_ALL_CONNECTED is then enabled mid-test, one more quorum is mined, and the spork21-only sections run against it: all-connected topology plus symmetric QSENDRECSIGS checks, the `submit=false` RPC parameter with QSIGSHARE P2P share submission to the recovery member, and the recovery-member isolation scenario. `mine_quorum()` reads spork state at call time, so expected connection counts adjust automatically.
  5. **`feature_llmq_simplepose.py --disable-spork23` skips the contribution-miss banning section** (`MarkBadMember` -> `PoSePunish` is not spork23-gated; spork23 only gates `VerifyConnectionAndMinProtoVersions` and probes, so that section was identical in both variants). A single quorum is mined in normal conditions instead so the spork23-specific sections start from the same state as in the spork23-enabled run.

  Deliberately lost permutations, called out explicitly:

  - *spork21 active from the very first DKG* (previously `feature_llmq_signing.py --spork21`): covered by `feature_llmq_data_recovery.py`, which enables SPORK_21_QUORUM_ALL_CONNECTED at the top of `run_test` on a fresh chain, before the first DKG, and then forms both `llmq_test` and `llmq_test_v17` quorums every cycle under it. (`feature_llmq_connections.py` additionally covers the chain's first *rotation* (dip0024) DKGs under spork21, though it mines three non-rotation quorums before enabling the spork.) The one delta is quorum size: the removed variant used 5-member quorums while `feature_llmq_data_recovery.py` uses 4- and 3-member ones; the spork21 connection path has no first-DKG-specific branching that depends on member count.
  - *legacy-wallet x governance/DIP3 consensus scenarios*: the consensus logic in those tests does not depend on the wallet type; the wallet surface they exercised is now covered in both wallet modes by `wallet_dash_rpcs.py`.

  Changed entries were re-slotted in `BASE_SCRIPTS` according to their new runtimes.

  ## How Has This Been Tested?

  Local machine (Apple Silicon, 14 cores), all timings from `test/functional/test_runner.py -j3` runs before and after the changes:

  | Runner entry | Before | After |
  |---|---|---|
  | `feature_governance.py --legacy-wallet` | 77 s | removed |
  | `feature_governance.py --descriptors` | 73 s | 69 s |
  | `feature_governance_cl.py --legacy-wallet` | 36 s | removed |
  | `feature_governance_cl.py --descriptors` | 22 s | 25 s |
  | `feature_dip3_deterministicmns.py --legacy-wallet` | 99 s | removed |
  | `feature_dip3_deterministicmns.py --descriptors` | 92 s | 84 s |
  | `feature_llmq_signing.py` | 58 s | 76 s (merged) |
  | `feature_llmq_signing.py --spork21` | 66 s | removed |
  | `feature_llmq_simplepose.py --disable-spork23` | 102 s | 77 s |
  | `wallet_dash_rpcs.py --legacy-wallet` | - | 14 s |
  | `wallet_dash_rpcs.py --descriptors` | - | 11 s |
  | **Total (affected entries, serial)** | **625 s** | **356 s** |

  That is a ~270 s serial saving per CI job on this machine; CI runners are slower, so the absolute saving there is expected to be larger (~380 s estimated from recent CI run timings).

  Additional verification:

  - `wallet_dash_rpcs.py` passes in both wallet modes.
  - The merged `feature_llmq_signing.py` passes standalone and under parallel load.
  - Both `feature_llmq_simplepose.py` variants pass; the `--disable-spork23` variant was additionally stress-tested with multiple concurrent instances.
  - `test/lint/all-lint.py` passes for the touched files (the only failure is pre-existing `lint-cppcheck-dash` warnings in unrelated C++ files untouched by this PR).

  ## Breaking Changes

  None. Test-only changes.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation _(not applicable)_
  - [ ] 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: 1d2530e22add8f738c56ece76ea0e6f3020f9321a5373ee6c7e22365b72ecaf96e19f4599589e91eee37395f0c5ef26eb351bf58580237979735ef71f4aaabd6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes
@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

22

Development

Successfully merging this pull request may close these issues.

3 participants

@knst @PastaPastaPasta @UdjinM6