-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
backport: v21.0.x rc.2 #6115
backport: v21.0.x rc.2 #6115
Conversation
… user, http-code, time of running 1a691bd feat: add logging for RPC HTTP requests: command, user, http-code, time of running (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Currently there is no way to gather stats for http rpc in dash core. This PR aim to change it. ## What was done? Implemented some basic stats for each RPC request: - rpc command - flag "is external" - http status - time to serve query (rpc time, not http time) ## How Has This Been Tested? See new logs: ```log [httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=false status=200 elapsed_time_ms=0 [httpworker.2] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=quorum is_external=false status=500 elapsed_time_ms=0 [httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command= is_external=false status=401 elapsed_time_ms=0 [httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=true status=200 elapsed_time_ms=28 [httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=operator command=getbestblockhash is_external=false status=200 elapsed_time_ms=0 ``` ## Breaking Changes N/A It doesn't change behavior of rpc server and http server. ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 ACKs for top commit: PastaPastaPasta: utACK 1a691bd Tree-SHA512: b62fceedb9a901e87c23c4aa6e6dfa7226d44da84a081ea245b40e7ff887103302147cebe0f7ff90bf9c8d4fa9ecafbaa6f25f39d2008f62c4f2beeef2877b57
…lets c944908 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov) 685cf34 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented As [noticed by kwvg](dashpay#6090 (review)), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue. dashpay/dash-issues#59 ## What was done? Removed default argument "bool mixing = false" from WalletStorage interface, Refactored a logic of CWallet::IsLocked to make it shorter, clearer and unified with bitcoin. ## How Has This Been Tested? A. Run Dash-Qt with descriptor wallet, run mixing, enter passphrase. The wallet is partially unlocked (for mixing only) - possible to see yellow lock in status. Mixing happens. B. Open "send transaction dialog", try to send a transaction: the app asks password to unlock wallet as expected. Though, I am not sure how exactly to test that **all** rpc are indeed locked when descriptor wallet is unlocked for mixing only. ## 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 ACKs for top commit: UdjinM6: LGTM, ~utACK~ light ACK c944908 kwvg: ACK c944908 PastaPastaPasta: utACK c944908 Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c
…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
…turn enum in CL verifying code 0133c98 feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov) 6004e06 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov) 130b6d1 refactor: replace static private member method to static method (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet. This PR aims to fix this issue, as requested by shumkov for needs of platform: > mailformed signature and can’t verify signature due to unknown quorum is the same error? > possible to distingush ? ## What was done? Return enum in CL verifying code `chainlock_handler.VerifyChainLock`. The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}` ## How Has This Been Tested? Functional test `feature_llmq_chainlocks.py` is updated ## Breaking Changes `submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't 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 0133c98 PastaPastaPasta: utACK 0133c98 Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
…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
…says there is a default that doesn't exist 60e36b7 fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta) Pull request description: ## Issue being fixed or feature implemented This default doesn't actually exist in code. ## What was done? Remove default from text ## How Has This Been Tested? ## Breaking Changes None ## 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: UdjinM6: utACK 60e36b7 Tree-SHA512: 658eb2cf101d0450619461b7ffe6069de5c04c1fdcca216713f9374cc1e60413ec9b96c3a2931fb69a4c2f8277dd6677100270960a58197da3b92dffb1e9e327
…msign 2db69d7 chore: add release notes for "quorum platformsign" (Konstantin Akimov) 283c5f8 feat: create new composite command "quorum platformsign" (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented It splits from dashpay#6100 With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only. ## What was done? Implemented a new composite command "quorum platformsign" This composite command let to limit quorum type for signing for case of whitelist. After that old way to limit platform commands can be deprecated - dashpay#6105 ## How Has This Been Tested? Updated a functional tests to use platform signing for Asset Unlocks feature. ## 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 2db69d7 PastaPastaPasta: utACK 2db69d7 Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM db82817
Guix Automation has began to build this PR tagged as v21.0.0-devpr6115.db828177. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, utACK db82817
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK db82817
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.0.0-devpr6115.db828177. The image should be on dockerhub soon. |
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.