Skip to content

feat: support rpc protx-register for descriptor wallets - part VI - #6003

Merged
PastaPastaPasta merged 4 commits into
dashpay:developfrom
knst:bp-descriptors-6-protx
May 14, 2024
Merged

feat: support rpc protx-register for descriptor wallets - part VI#6003
PastaPastaPasta merged 4 commits into
dashpay:developfrom
knst:bp-descriptors-6-protx

Conversation

@knst

@knst knst commented Apr 30, 2024

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Many rpc such as protx register uses 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)

for all functional tests that uses Masternodes/evo nodes.

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

What was done?

Some direct usages of LegacyScriptPubKeyMan refactored to use CWallet's functionality.
There are still 4 functional tests that doesn't work for descriptor wallets:

  • feature_dip3_deterministicmns.py (no rpc protx updateregistar)
  • feature_governance.py: no rpc for governance votemany and governance votealias
  • interface_zmq_dash.py (see governance)

That's part I of changes, other changes are not PR-ready yet, WIP.

How Has This Been Tested?

Firstly, the flag --legacy-wallets are removed for many functional tests.
Secondly, the flag --descriptors is inverted in default value:

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 585a6a74d6..9ad5fd1daa 100755
--- 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
             else:
                 # If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter
                 # It still needs to exist and be None in order for tests to work however.

Breaking Changes

N/A, descriptor wallets have not been publicly released yet

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 Apr 30, 2024
@knst knst mentioned this pull request May 3, 2024
5 tasks
PastaPastaPasta added a commit that referenced this pull request May 3, 2024
10869ff fix: order of locks cs_wallet and cs_main in rpc/evo (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Changing order of locking `cs_main` and `cs_wallet` to prevent a deadlock.

  There's call `protx_list` -> `BuildDMNListEntry` -> `CheckWalletOwnsScript`:
  ```
  return WITH_LOCK(pwallet->cs_wallet, return pwallet->IsMine(script)) == isminetype::ISMINE_SPENDABLE;
  ```

  It can cause a deadlock due to wrong order of locks (cs_wallet supposed to be blocked in prior of cs_main)

  ## What was done?
  This PR adds and extra lock of cs_wallet and reduce scope of cs_main for a bit

  ## How Has This Been Tested?
  Deadlock warning is reproduced with this PR: #6003

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

Top commit has no ACKs.

Tree-SHA512: 6d166fe7c47067c1c6d889d87e015ff3bc43aa9f66234341840cc8465ce00a79e7140bc09cbdb2fd08feaae5463b320e0b66bbe410422783f86cbc9d616af6b3
@github-actions

github-actions Bot commented May 3, 2024

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@knst
knst force-pushed the bp-descriptors-6-protx branch from 46ac531 to a25b163 Compare May 6, 2024 16:56
@knst knst changed the title feat: support protx rpc for descriptor wallets feat: support rpc protx-register for descriptor wallets May 6, 2024
@knst
knst force-pushed the bp-descriptors-6-protx branch from a25b163 to b5897c7 Compare May 6, 2024 17:10
knst added 4 commits May 7, 2024 00:17
Enables for rpc_quorum.py, feature_notifications.py

see dashpay#5981, it partial revert of b20f812
That are:
 - feature_dip3_deterministicmns.py
 - interface_zmq_dash.py
 - feature_governance.py
 - wallet_upgradetohd.py (as expected to be implemented for legacy-only wallets)
 - p2p_timeouts.py (why? can not understand it)

This partially reverts commit b20f812.
@knst
knst force-pushed the bp-descriptors-6-protx branch from b5897c7 to a33dcb3 Compare May 6, 2024 17:17
@knst
knst marked this pull request as ready for review May 6, 2024 19:33
@knst
knst requested review from PastaPastaPasta and UdjinM6 and removed request for PastaPastaPasta May 6, 2024 19:33

@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

@knst knst changed the title feat: support rpc protx-register for descriptor wallets feat: support rpc protx-register for descriptor wallets - part VI May 8, 2024

@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 a33dcb3

@PastaPastaPasta
PastaPastaPasta merged commit 146be9f into dashpay:develop May 14, 2024
@knst
knst deleted the bp-descriptors-6-protx branch June 12, 2024 19:13
ab4cus pushed a commit to ab4cus/e4Coin-core that referenced this pull request Jul 8, 2026
4a460ee fix: order of locks cs_wallet and cs_main in rpc/evo (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Changing order of locking `cs_main` and `cs_wallet` to prevent a deadlock.

  There's call `protx_list` -> `BuildDMNListEntry` -> `CheckWalletOwnsScript`:
  ```
  return WITH_LOCK(pwallet->cs_wallet, return pwallet->IsMine(script)) == isminetype::ISMINE_SPENDABLE;
  ```

  It can cause a deadlock due to wrong order of locks (cs_wallet supposed to be blocked in prior of cs_main)

  ## What was done?
  This PR adds and extra lock of cs_wallet and reduce scope of cs_main for a bit

  ## How Has This Been Tested?
  Deadlock warning is reproduced with this PR: dashpay/dash#6003

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

Top commit has no ACKs.

Tree-SHA512: 6d166fe7c47067c1c6d889d87e015ff3bc43aa9f66234341840cc8465ce00a79e7140bc09cbdb2fd08feaae5463b320e0b66bbe410422783f86cbc9d616af6b3
PastaPastaPasta added a commit that referenced this pull request Aug 1, 2026
…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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants