Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Jun 10, 2024

Issue being fixed or feature implemented

We have composite commands such as 'bls generate' that do not exist in bitcoin's implementation of rpc.
It doesn't let to backport yet bitcoin#18531 which enforced extra checks for arguments name (name of rpc and list arguments in rpc help and actual implementation must match).

What was done?

This PR improves support of composite commands in Dash Core. New style of composite commands are applied for bls composite commands: bls generate and bls fromsecret as proof of concept.
Once this PR is merged, I will provide similar fixes for other "compose" rpc commands: protx, masternode (and everything else if any).

Beside better validation of arguments and command names, it improves suggest menu in Qt app (see a screenshot) for composite commands.

image

How Has This Been Tested?

Run unit and functional tests. Also extra tests to conduct in qt app:

  • check suggest for 'bls ....' and 'help bls ...'
  • check output of next commands:
help bls
help bls generate
help bls from secret
bls
bls generate
bls generate 1
  • also let's see that old-fashion composite commands are not broken:
help protx
help protx diff
protx diff 00000021c7604b9992254f9f1ed91de5d65eaade33c773abea63a7b0e93293ee 000000e44f9894838ebf768b464177cfce8859dcf92b0509f5c2fba774315996

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 requested review from PastaPastaPasta and UdjinM6 June 10, 2024 08:26
This linter is subject to remove in 20012 anyway, which is todo after 18531 is done
@knst knst force-pushed the refactor-rpc-18531 branch from 0b2b692 to 9413ecd Compare June 10, 2024 10:29
@knst knst added this to the 21 milestone Jun 10, 2024
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.

LGTM, light ACK

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 9413ecd

@PastaPastaPasta PastaPastaPasta merged commit 21af5af into dashpay:develop Jun 10, 2024
@knst knst deleted the refactor-rpc-18531 branch June 11, 2024 07:14
PastaPastaPasta added a commit that referenced this pull request Jun 13, 2024
…ode NNN

64f3477 refactor: remove unused header spork.h from rpc/masternode (Konstantin Akimov)
43e9ab4 refactor: use new type of composite commands for masternode NNN (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  See #6051

  ## What was done?
  Commands starting from 'masternode ...' uses new a new way to make composite commands.

  ## How Has This Been Tested?
  Run unit/functional tests.
  Please notice, we support both styles `masternodelist` and `masternode list` which are still both supported.

  ## Breaking Changes
  N/A

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

ACKs for top commit:
  PastaPastaPasta:
    utACK 64f3477

Tree-SHA512: 5a658588dff7f29d3025d2a720c8efe8096f79e8d632800911573f2f9a42450c2f27da070a94c5739e2027153413cdcacde82a7b4614467e036c008719692ed9
PastaPastaPasta added a commit that referenced this pull request Jun 13, 2024
b478406 refactor: clean-up, missing const in rpc/quorums (Konstantin Akimov)
50c99e8 fix: adopt platform restriction for new composite commands (Konstantin Akimov)
e3c4d66 refactor: quorum sign methods (Konstantin Akimov)
2af9d86 refactor: use new approach for rpc quorums NNN (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  See #6051

  ## What was done?
  Commands starting from 'quorum ...' uses new a new way to make composite commands.

  ## How Has This Been Tested?
  Run unit/functional tests.
  Please notice, there's required extra changes for platform restrictions, the subcommand is no more first argument, but part of command, space separated.

  ## Breaking Changes
  N/A

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

ACKs for top commit:
  UdjinM6:
    utACK b478406
  PastaPastaPasta:
    utACK b478406

Tree-SHA512: c53856a3f45fee44a8130876ed4545f6f2c0f59f74e4d4745e7354097fee3a74ed526cc8160de532dbb4aeb81b82638ee912a0961b910692e78889baa2c976c3
@knst
Copy link
Collaborator Author

knst commented Jun 25, 2024

just note for history:
it seems as this PR breaks help for composite commands that has not been refactored yet, such as help protx list

PastaPastaPasta added a commit that referenced this pull request Jun 27, 2024
7b44af2 refactor: move common code inside protx_register_common_wrapper (Konstantin Akimov)
89d5e26 refactor: replace bunch of bool flags to the enum (Konstantin Akimov)
1351bc9 refactor: move common code to the wrapper (Konstantin Akimov)
8d4c28a refactor: remove unused protx code from old implementation (Konstantin Akimov)
2a837f8 refactor: use new composite for RPC 'protx NNN' (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  See #6051

  ## What was done?
  Commands starting from 'protx ...' uses new a new way to make composite commands.

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

  ## Breaking Changes
  N/A

  ## 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:
  PastaPastaPasta:
    utACK 7b44af2

Tree-SHA512: a5124121de93ba566bfa6c7c58a5217446301dd027839d137e056df00388282af6be0feb0b81ea0217310e12231a68e3b405fa82b66df93a3fb159d341d7a199
PastaPastaPasta added a commit that referenced this pull request Jun 27, 2024
…nce NNN

91aae7b refactor: use properly RPCHelpMan for governance RPC getsuperblockbudget (Konstantin Akimov)
e92e5ef refactor: use properly RPCHelpMan for governance RPC voteraw (Konstantin Akimov)
87b3011 refactor: duplicated code between governance RPC list and diff (Konstantin Akimov)
faa6179 docs: move comments from the old code to the new implementation of governance RPC (Konstantin Akimov)
39cd5e4 fix: format code for rpc governance (Konstantin Akimov)
0062421 refactor: use new style of composite commands for RPC gobject NNN (Konstantin Akimov)
42e0b37 refactor: use properly RPCHelpMan for governance RPC getgovernanceinfo (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  See #6051

  ## What was done?
  Commands starting from 'governance ...' uses new a new way to make composite commands.

  This PR includes also a refactoring to remove common code between `gobject list` and `gobject diff`
  This PR includes also a refactoring to properly use RPCHelpMan for remaining governance's RPC.

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

  ## Breaking Changes
  N/A

  ## 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 91aae7b
  PastaPastaPasta:
    utACK 91aae7b

Tree-SHA512: 46c00df924a7d2c5af799c605bfc479034019eed346f9d487827b0688993aa712ba3dc528d9bdd09b64f0e07b6940078337ca1baf0a1355f87eb792406c676ca
PastaPastaPasta added a commit that referenced this pull request Jul 15, 2024
…latform needs

85abbb9 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  dashpay/dash-issues#66
  dashpay/dash-issues#65

  ## What was done?
  Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see #6052 #6051 #6055 and other related PRs

  This PR makes whitelist feature to be compatible with composite commands.

  Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.

  Platform at their side can use config such as this one (created based on shumkov's example):
  ```
  rpc: {
            host: '127.0.0.1',
            port: 9998,
            users: [
              {
                user: 'dashmate',
                password: 'rpcpassword',
                whitelist: null,
                lowPriority: false,
              },
              {
                username: 'platform-dapi',
                password: 'rpcpassword',
                whitelist: [],
                lowPriority: true,
              },
              {
                username: 'platform-drive-consensus',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
                lowPriority: false,
              },
              {
                username: 'platform-drive-other',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
  ],
                lowPriority: true,
              },
            ],
            allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
          },
  ```

  ## How Has This Been Tested?
  Updated functional tests, see commits

  ## Breaking Changes
  n/a

  ## 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:
    LGTM, utACK 85abbb9
  PastaPastaPasta:
    utACK 85abbb9

Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2024
…s for platform needs

85abbb9 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  dashpay/dash-issues#66
  dashpay/dash-issues#65

  ## What was done?
  Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see dashpay#6052 dashpay#6051 dashpay#6055 and other related PRs

  This PR makes whitelist feature to be compatible with composite commands.

  Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.

  Platform at their side can use config such as this one (created based on shumkov's example):
  ```
  rpc: {
            host: '127.0.0.1',
            port: 9998,
            users: [
              {
                user: 'dashmate',
                password: 'rpcpassword',
                whitelist: null,
                lowPriority: false,
              },
              {
                username: 'platform-dapi',
                password: 'rpcpassword',
                whitelist: [],
                lowPriority: true,
              },
              {
                username: 'platform-drive-consensus',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
                lowPriority: false,
              },
              {
                username: 'platform-drive-other',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
  ],
                lowPriority: true,
              },
            ],
            allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
          },
  ```

  ## How Has This Been Tested?
  Updated functional tests, see commits

  ## Breaking Changes
  n/a

  ## 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:
    LGTM, utACK 85abbb9
  PastaPastaPasta:
    utACK 85abbb9

Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
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