Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: createwallet to require 'load_on_startup' for descriptor wallets #6095

Merged
merged 1 commit into from
Jul 8, 2024
Merged

fix: createwallet to require 'load_on_startup' for descriptor wallets #6095

merged 1 commit into from
Jul 8, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jul 5, 2024 •
edited
Loading

Issue being fixed or feature implemented

RPC createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup ) since #5965
load_on_startup used to be an argument 5 but now it has a number 6. Both arguments 5 and 6 are boolean and it can confuse an user: they may even not notice that something wrong when it meant to be "load on startup" but got "descriptor wallet" which is not expected.

See also previous attempt to resolve issue: #6029

What was done?

To prevent confusion if user is not aware about this breaking changes, the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup. This requirement can be removed when major amount of users updated to v21.

How Has This Been Tested?

Run unit/functional tests

Tested with CLI:

$ createwallet "tmp-create" true true "" false true
RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC (code -8)
$ createwallet "tmp-create" true true "" false true true
{
  "name": "tmp-create",
  "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet is an experimental descriptor wallet"
}

Breaking Changes

You can't more pass 'descriptor=NN' without #5965 which has not been 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 Jul 5, 2024
@knst knst force-pushed the fix-descriptor-createwallet-2 branch from a02bcb3 to b914da3 Compare July 5, 2024 08:32
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Jul 6, 2024
thephez
thephez reviewed Jul 6, 2024
View reviewed changes
src/wallet/rpcwallet.cpp Outdated
Comment on lines 3035 to 3037
if (request.params[6].isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC");
}
Copy link
Collaborator

@thephez thephez Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How's this sound?

Suggested change
if (request.params[6].isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC");
}
if (request.params[6].isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when creating descriptor wallets. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC.");
}

PastaPastaPasta
PastaPastaPasta reviewed Jul 6, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

I think we should have an adjusted release note for this?

@knst
fix: createwallet to require 'load_on_startup' for descriptor wallets
a42e9df 
createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )
load_on_startup used to be an argument 5 but now has a number 6.
Both arguments 5 and 6 are boolean and it can confuse an user.

To prevent confusion if user is not aware about this breaking changes,
the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup.
This requirement can be removed when major amount of users updated to v21
@knst knst force-pushed the fix-descriptor-createwallet-2 branch from b914da3 to a42e9df Compare July 7, 2024 14:56
UdjinM6
UdjinM6 approved these changes Jul 7, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK a42e9df

PastaPastaPasta
PastaPastaPasta approved these changes Jul 7, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK a42e9df

thephez
thephez approved these changes Jul 8, 2024
View reviewed changes
Copy link
Collaborator

@thephez thephez left a comment

Choose a reason for hiding this comment

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

utACK a42e9df

@PastaPastaPasta PastaPastaPasta merged commit 50a3b7c into dashpay:develop Jul 8, 2024
9 checks passed
@knst knst deleted the fix-descriptor-createwallet-2 branch July 8, 2024 17:56
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2024
@PastaPastaPasta
Merge dashpay#6095: fix: createwallet to require 'load_on_startup' fo…
cdf7a25 
…r descriptor wallets

a42e9df fix: createwallet to require 'load_on_startup' for descriptor wallets (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPC `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )` since dashpay#5965
  `load_on_startup` used to be an argument 5 but now it has a number 6. Both arguments 5 and 6 are boolean and it can confuse an user: they may even not notice that something wrong when it meant to be "load on startup" but got "descriptor wallet" which is not expected.

  See also previous attempt to resolve issue: dashpay#6029

  ## What was done?
  To prevent confusion if user is not aware about this breaking changes, the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup. This requirement can be removed when major amount of users updated to v21.

  ## How Has This Been Tested?
  Run unit/functional tests

  Tested with CLI:
  ```
  $ createwallet "tmp-create" true true "" false true
  RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC (code -8)
  $ createwallet "tmp-create" true true "" false true true
  {
    "name": "tmp-create",
    "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet is an experimental descriptor wallet"
  }
  ```

  ## Breaking Changes
  You can't more pass 'descriptor=NN' without  dashpay#5965 which has not been released yet.

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

ACKs for top commit:
  UdjinM6:
    utACK a42e9df
  PastaPastaPasta:
    utACK a42e9df
  thephez:
    utACK a42e9df

Tree-SHA512: bf57fed40d04a32e756e4f8bfabbe39c0cbf87275546c92f4efc19523bc3c5dd3ddc5a884d67285971dc301a6c5808bef979db52c35645ca2db0810046fe1bdc
PastaPastaPasta added a commit that referenced this pull request Jul 15, 2024
@PastaPastaPasta
Merge #6115: backport: v21.0.x rc.2
b0b6ba1 
db82817 Merge #6106: feat: create new composite quorum-command platformsign (pasta)
a45e6df Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta)
7330982 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta)
9998ffd Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta)
cdf7a25 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta)
c1c2c55 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta)
1175486 Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Backports a set of 6 PRs needed in rc.2

  ## What was done?
  Backported PRs with labels

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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 _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    LGTM, utACK db82817
  UdjinM6:
    utACK db82817

Tree-SHA512: 1b242c5db04bd5873ef622543bc2a25e29567f15962c677ea51ff05cb784291968d18f419bf611c206b912e8f15d687208ae75af33aab89038b6f0167d99c4bf
@PastaPastaPasta
Copy link
Member

back ported in #6115

PastaPastaPasta added a commit that referenced this pull request Jul 29, 2024
@PastaPastaPasta
Merge #6161: chore: merge master 21.0.0 back into develop
7ebd778 
98a3393 chore: set release to true (pasta)
cd0a3a6 Merge #6154: chore: remove trailing whitespaces in release notes (pasta)
6bc60a7 Merge #6151: chore: update seeds for v21 release (pasta)
88e949a Merge #6146: chore: bump assumevalid, minchainwork, checkpoints, chaintxdata (pasta)
cc14427 Merge #6144: docs: release notes for v21.0.0 (pasta)
0a8ece1 Merge #6122: chore: translations 2024-07 (pasta)
146d244 Merge #6140: feat: harden all sporks on mainnet to current values (pasta)
024d272 Merge #6126: feat: enable EHF activation of MN_RR on mainnet (pasta)
e780b3d Merge #6125: docs: update manpages for 21.0 (pasta)
5ede23c Merge #6118: docs: add release notes notifying change of default branch to `develop` (pasta)
1b6fe9c Merge #6117: docs: update supported versions in SECURITY.md (pasta)
27d20be Merge #6116: fix: mitigate crashes associated with some upgradetohd edge cases (pasta)
db82817 Merge #6106: feat: create new composite quorum-command platformsign (pasta)
a45e6df Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta)
7330982 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta)
9998ffd Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta)
cdf7a25 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta)
c1c2c55 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta)
1175486 Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Suppressed changes from be83865 so the diff is empty.

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## 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 _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    ACK 158cf86; no diff
  knst:
    ACK 158cf86

Tree-SHA512: 3310a39fbcb45bdf09f885fe77ba769c0a715869a3bb287eaf0f2cf54b35a7e1f832c88df3bd31097eabf2d375515c1b87ff05e0c3282cef642833a154c42bbe
@knst knst mentioned this pull request Sep 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thephez thephez thephez approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Milestone
21
Development

Successfully merging this pull request may close these issues.
4 participants
@knst @PastaPastaPasta @UdjinM6 @thephez