1840c9441a fix: drop extra pings - follow up for #18638 (UdjinM6)
264e7f9e62 Merge #18638: net: Use mockable time for ping/pong, add tests (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Split from https://github.com/dashpay/dash/pull/6102
## What was done?
So far as bitcoin#19499 is backported, bitcoin#18638 can be finished and removed related workarounds.
## 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
- [ ] 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:
utACK 1840c9441a
PastaPastaPasta:
utACK 1840c9441a
Tree-SHA512: f34657c14514d6ee596175b3faf9ee44e58e8b0339939a0708d58ab2d119786830c183f9b236ed87c08ef8e1dbd031a38fc596b5aa4d38e10521658df4330e79
d3e842f605 refactor: remove dead code which has no use since composite commands are refactored (Konstantin Akimov)
58c5d431fe fix: follow-up to #6017 - enable one more assert in wallet_descriptor test (Konstantin Akimov)
dbed4a31af fix: update comment for wallet_keypool_hd due to bitcoin#17681 DNM (Konstantin Akimov)
4741bcc5c3 chore: remove outdated todo - removed by bitcoin#16898 (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Multiple TODO is reviewed and fixes in this PR
## What was done?
See commits
## 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
- [ ] 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:
utACK d3e842f605
PastaPastaPasta:
utACK d3e842f605
Tree-SHA512: 25080abcce8fa3dbb4e4c71d321b24cb9d23c073e6caf75366be58623023af50d28bc1cdac4098eb78a87b4fe0f3c33d39bb06257f0590b3e42e5fcad161ea2d
a4e970adb6de8425025ae3f62fb89d9e27a8ab1f build: enable -Wdocumentation if suppressing external warnings (fanquake)
3b0078f958c46e94b468c829522ba965f5549f11 doc: fixup -Wdocumentation issues (fanquake)
c6edcf1c710e4aaf1cafdbf8e86fe209b57bdeb8 build: suppress libevent warnings if supressing external warnings (fanquake)
Pull request description:
Enable `-Wdocumentation` by taking advantage of our `--enable-suppress-external-warnings` flag. Most of the CIs are using this flag now, so any regressions should be caught.
This also required modifying libevents flags when suppressing warnings, as depending on the version being built against, that could generate a large number of warnings. i.e:
```bash
In file included from httpserver.cpp:34:
In file included from ./support/events.h:12:
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:464:11: warning: parameter 'req' not found in the function declaration [-Wdocumentation]
@param req a request object
^~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:465:11: warning: parameter 'databuf' not found in the function declaration [-Wdocumentation]
@param databuf the data chunk to send as part of the reply.
^~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:467:11: warning: parameter 'call' not found in the function declaration [-Wdocumentation]
@param call back's argument.
^~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:939:4: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync]
@deprecated This function is deprecated; you probably want to use
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:946:1: note: add a deprecation attribute to the declaration to silence this warning
char *evhttp_decode_uri(const char *uri);
^
__AVAILABILITY_INTERNAL_DEPRECATED
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:979:5: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync]
@deprecated This function is deprecated as of Libevent 2.0.9. Use
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:987:1: note: add a deprecation attribute to the declaration to silence this warning
int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
^
__AVAILABILITY_INTERNAL_DEPRECATED
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: warning: parameter 'query_parse' not found in the function declaration [-Wdocumentation]
@param query_parse the query portion of the URI
^~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: note: did you mean 'uri'?
@param query_parse the query portion of the URI
^~~~~~~~~~~
uri
69 warnings generated.
```
Note that a lot of these have already been fixed upstream.
ACKs for top commit:
laanwj:
Concept and code review ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f
practicalswift:
cr ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f: automatic compiler feedback comes sooner and is more reliable than manual reviewer feedback
jonatack:
Light ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f skimmed the changes, clang 11 build is clean with the change, verified -Wdocumentation build warnings with this change when a doc fix was reverted
Tree-SHA512: 57a1e30cffcc8bcceee72d85f58ebe29eae525861c70acb237541bd480c51ede89875c033042c0af376fdbb49fb7f588ef9282a47c6e78f9d4501c41f1b21eb6
1a691bd100 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 1a691bd100
Tree-SHA512: b62fceedb9a901e87c23c4aa6e6dfa7226d44da84a081ea245b40e7ff887103302147cebe0f7ff90bf9c8d4fa9ecafbaa6f25f39d2008f62c4f2beeef2877b57
This is in line with how it's done in 880d4aaf upstream. This wasn't
noticed with CI as we build with `--enable-suppress-external-warnings`
by default.
3d8ff0cd2d fix: cppcheck warning about coinst in rpc/coinjoin (Konstantin Akimov)
63dfdd7d42 fix: workaround for buggy cppcheck in masternode/utils (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
`cppcheck 2.11` on my localhost returns 2 failures, that are not caught by our CI, probably due to difference in version:
```
src/rpc/coinjoin.cpp:73:28: warning: Variable 'chainman' can be declared as reference to const [constVariableReference]
src/masternode/utils.cpp:0:0: warning: Internal Error. MathLib::toLongNumber: input was not completely consumed: 5s [cppcheckError]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
```
## What was done?
Adds missing const, workaround for probably a crash in cppcheck 2.11.
## How Has This Been Tested?
Run `test/lint/lint-cppcheck-dash.sh`
## 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:
utACK 3d8ff0cd2d
PastaPastaPasta:
utACK 3d8ff0cd2d
Tree-SHA512: c4c7e839161a6eb8d7a25b48cd05914d94dc4e4a6d53cbd39728a74a39d626d9b533afd98a9803550d2754df5c86445e5157e0e48853af52ea5ea5cb2e15d97b
c944908399 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov)
685cf34cb9 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
As [noticed by kwvg](https://github.com/dashpay/dash/pull/6090#pullrequestreview-2153639183), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue.
https://github.com/dashpay/dash-issues/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 c944908399
kwvg:
ACK c944908399
PastaPastaPasta:
utACK c944908399
Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c
0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 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 0133c9866d
PastaPastaPasta:
utACK 0133c9866d
Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
60e36b786a 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 60e36b786a
Tree-SHA512: 658eb2cf101d0450619461b7ffe6069de5c04c1fdcca216713f9374cc1e60413ec9b96c3a2931fb69a4c2f8277dd6677100270960a58197da3b92dffb1e9e327
a42e9df06f 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 https://github.com/dashpay/dash/pull/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: https://github.com/dashpay/dash/pull/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 https://github.com/dashpay/dash/pull/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 a42e9df06f
PastaPastaPasta:
utACK a42e9df06f
thephez:
utACK a42e9df06f
Tree-SHA512: bf57fed40d04a32e756e4f8bfabbe39c0cbf87275546c92f4efc19523bc3c5dd3ddc5a884d67285971dc301a6c5808bef979db52c35645ca2db0810046fe1bdc
It helps to detect case of Tip is way too far behind: the signature can be
still valid but core can't verify it yet but later it may become a valid signature.
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2fa1153c6d76efc8113fa01b06943ece util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
It fixes this failure:
```
src/rpc/coinjoin.cpp:73:28: warning: Variable 'chainman' can be declared as reference to const [constVariableReference]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
```
It shows this error:
```
src/masternode/utils.cpp:0:0: warning: Internal Error. MathLib::toLongNumber: input was not completely consumed: 5s [cppcheckError]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
```
It doesn't seems as error in our code, but I don't like this warning
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
bebea4b9b6 fix: auto backup issue with descriptor wallets for CJ (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Qt CoinJoin session has problems (https://github.com/dashpay/dash/pull/5579#pullrequestreview-1912946808):
- Autobackup problems
- False keypool depletion reporting
https://github.com/dashpay/dash-issues/issues/59
## What was done?
Disables check for "remaining keys left" and "auto-backups" for non-legacy wallet.
## How Has This Been Tested?
Run unit/functional test
Try to run CJ mixing for descriptor wallet.
## 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:
PastaPastaPasta:
utACK bebea4b9b6
Tree-SHA512: 610551001d054c447ddca9451ac6d94f3d063ecf3ccfab437d99324efc5f99ff86e59d80a36f4ff4983d3c8107aa19292c021cb3210fcf51e79919387169c414
684503db6b feat: add a marker experimental for descriptor wallets (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Descriptor wallets pass most of functional tests now, but they are not really tested in-the-real-environment by multiple users.
## What was done?
Add a marker "experimental" for 'createwallet' rpc, for create wallet UI form.
## How Has This Been Tested?
Run and check
## 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:
PastaPastaPasta:
utACK 684503db6b
Tree-SHA512: e843e5b8080c7cd4273ea532038f3ec65e0260419b2c2a3db07ee1fadd856a7ce47f367705b4ddb19c5fc91cee5a529a54d580dbf53428c3e5e8b433c96ec0c9
375838378a feat: bump protocol version to distinguish v21 from v20.1 (pasta)
Pull request description:
## Issue being fixed or feature implemented
See commit
## What was done?
## How Has This Been Tested?
## Breaking Changes
None really; will require MNs to upgrade in order to not be banned
## 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)_
Top commit has no ACKs.
Tree-SHA512: c4a61e3c9a1ccda9b00a8982eaaff6ea7672bf59d9538342b0e45a16977554224b44b34db48d3355d28e2b26a01ab3133a5e0c38bb00a3d7a1410cfd1fcfc4ae
6c5246803d test: add tests for both current and future behaviour (UdjinM6)
4e86bda4dc feat: stricter bestCLHeightDiff checks (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Current `bestCLHeightDiff` checks are too relaxed
## What was done?
Make`bestCLHeightDiff` checks stricter
## How Has This Been Tested?
Run a node on mainnet/testet, run tests
## Breaking Changes
Old nodes aren't aware of this new logic so it's activated via `mn_rr` hardfork
## 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:
utACK 6c5246803d
Tree-SHA512: 2028d0ceb00a2270c92831ef38488d009d0bac47be4fc6a23ac93efdcf74847f1b9e99a529863fb4e14c65f120adda4e12c5b9e084d0f667d5f0fbaf80e3701d
