9305862f71189d47c873d366bf976622447e18af wallet: load flags before everything else (Sjors Provoost)
Pull request description:
Load and set wallet flags before processing other records. That way we can take them into account while processing those other records.
Suggested here:
https://github.com/bitcoin/bitcoin/pull/16546#discussion_r572334983
ACKs for top commit:
laanwj:
Code review ACK 9305862f71189d47c873d366bf976622447e18af
gruve-p:
ACK 9305862f71
achow101:
ACK 9305862f71189d47c873d366bf976622447e18af
Tree-SHA512: 7104523e369ce3c670571fe5e8b52c67b9ca92b8e36a2da5eb6f9f8bf8ed0544897007257204b68f6f371d682b3ef0d0635d36e6e8416ac74af1999d9fbc869c
3eb6f8b2e61c24a22ea9396d86672307845f35eb wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb56bf5d455154b0498b1f58f77d20ed wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e357159c7154f69f28cb5587c5ca20d6594 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce88696a3216fc38b7d393906b733e8b1 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788cb0862aafbb116fd37936cbed6a431 wallet: refactor GetClosestWalletFeature() (Jon Atack)
Pull request description:
This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.
This PR fixes 4 upgradewallet issues:
- this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
- it returns nothing in the absence of an RPC error, which isn't reassuring for users
- it returns the same thing both in the case of a successful upgrade and when no upgrade took place
- the error message object is currently dead code
This PR fixes the above and provides:
...user feedback to not silently return without upgrading
```
{
"wallet_name": "disable private keys",
"previous_version": 169900,
"current_version": 169900,
"result": "Already at latest version. Wallet version unchanged."
}
```
...better feedback after successfully upgrading
```
{
"wallet_name": "watch-only",
"previous_version": 159900,
"current_version": 169900,
"result": "Wallet upgraded successfully from version 159900 to version 169900."
}
```
...helpful error responses
```
{
"wallet_name": "blank",
"previous_version": 169900,
"current_version": 169900,
"error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
}
{
"wallet_name": "blank",
"previous_version": 130000,
"current_version": 130000,
"error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
}
```
updated help:
```
upgradewallet ( version )
Upgrade the wallet. Upgrades to the latest version if no version number is specified.
New keys may be generated and a new wallet backup will need to be made.
Arguments:
1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"previous_version" : n, (numeric) Version of wallet before this operation
"current_version" : n, (numeric) Version of wallet after this operation
"result" : "str", (string, optional) Description of result, if no error
"error" : "str" (string, optional) Error message (if there is one)
}
```
ACKs for top commit:
achow101:
ACK 3eb6f8b
MarcoFalke:
review ACK 3eb6f8b2e61c24a22ea9396d86672307845f35eb 🛡
Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
5f9c0b6360215636cfa62a70d3a70f1feb3977ab wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08215feba53ead27096ac7fda34acb3c test: Remove unused wallet.dat (MarcoFalke)
bf7635963c03203e7189ddaa56c6b086a0108cbf tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9decc3e855ee4b0bbf9e61121c8e9904e5 test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc434854f881330771a93a1280ac67b1d3549 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19be65b0dd23df1df571c71428c2bc32 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c995e832e643f605d35a7aa112837e6 wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc6258c258e9f4411c50630ec4a552341b wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f34dedf75b063b962845fa8eca604514 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842df489f1b8d68e67a234788966218184 wallet: Add utility method for CanSupportFeature (Andrew Chow)
Pull request description:
This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.
The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.
`CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.
`nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.
Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.
Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too.
Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict.
ACKs for top commit:
MarcoFalke:
approach ACK 5f9c0b6360
laanwj:
Code review ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab
jonatack:
ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`
Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
facd7dd3d1f9d51e1133974ff69eeb48f5ae282b wallet: Fix typo in comments; Simplify assert (MarcoFalke)
Pull request description:
Follow up to https://github.com/bitcoin/bitcoin/pull/19046#discussion_r443783101 and https://github.com/bitcoin/bitcoin/pull/19046#discussion_r443793690
ACKs for top commit:
practicalswift:
ACK facd7dd3d1f9d51e1133974ff69eeb48f5ae282b
jonatack:
ACK facd7dd3d1f9d51e1133974ff69eeb48f5ae282b
hebasto:
ACK facd7dd3d1f9d51e1133974ff69eeb48f5ae282b, spelling verified with `test/lint/lint-spelling.sh`: all remaining warnings are false positive.
Tree-SHA512: 2b185d138058840db56726bb6bcc42e5288a954e2a410c49e04806a047fbbdaf0bb2decc70ecf7613c69caa766655705ca44151613e7ea5015b386d1e726d870
3a9aba21a49a6d80bd187940d5e26893937b6832 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow)
d9cd095b5965fc20c09f401370e7ba99446663e3 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow)
0122fbab4c340b23ae56173de6c5ab866ba25ab8 Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow)
Pull request description:
`SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet.
`AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet.
`LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk.
ACKs for top commit:
jnewbery:
Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832
ryanofsky:
Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832. Only changes since last review tweaks making m_wallet_flags updates more safe
meshcollider:
utACK 3a9aba21a49a6d80bd187940d5e26893937b6832
Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
This commit helps to unify our implementation with bitcoin which has
the similar refactoring in bitcoin#17681 but we DNM it due to difference in
sethdseed behavior: we do not replace seed ever
The key difference between bitcoin's and dash's implementation that sethdseed
does not update existing seed for wallet. Seed can be set only once.
It behave similarly to `upgradetohd` rpc, but since v20.1 all wallets are HD and
the name `upgradetohd` is not relevant more.
94a8e1a713 Merge bitcoin/bitcoin#28097: depends: xcb-proto 1.15.2 (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Resolves build failures with Python 3.12, i.e building on rawhide:
```bash
make -C depends -j9
...
make[3]: Nothing to be done for 'install-exec-am'.
/usr/bin/mkdir -p '/bitcoin/depends/work/staging/aarch64-unknown-linux-gnu/xcb_proto/1.14.1-4a91ac9dc41/bitcoin/depends/aarch64-unknown-linux-gnu/lib/python3.12/site-packages/xcbgen'
/usr/bin/install -c -m 644 __init__.py error.py expr.py align.py matcher.py state.py xtypes.py '/bitcoin/depends/work/staging/aarch64-unknown-linux-gnu/xcb_proto/1.14.1-4a91ac9dc41/bitcoin/depends/aarch64-unknown-linux-gnu/lib/python3.12/site-packages/xcbgen'
Traceback (most recent call last):
File "<string>", line 2, in <module>
ModuleNotFoundError: No module named 'imp'
make[3]: *** [Makefile:271: install-pkgpythonPYTHON] Error 1
```
`imp` was removed in 3.12: https://docs.python.org/3/library/imp.html.
## What was done?
Bitcoin backport bitcoin#28097
## How Has This Been Tested?
Run build `depends` with clean - it doesn't fail
## 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: 61415fcc0c86f57d6b124907505102ae5b319c95d6ee0b531ce75fa2370a82bafd7a40b21e24eb7d1ac38ba93d59d9e7b89829667c61f4f4caf46029e6dd1cf7
b2910fba02 fix: resolve potential deadlocks in CJ (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'cs_wallet' in wallet/wallet.cpp:3826 (in thread 'qt-init')
(2) 'pwallet->cs_wallet' in wallet/walletdb.cpp:705 (in thread 'qt-init')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:971 (in thread 'qt-init')
Current lock order is:
'cs_deqsessions' in coinjoin/client.cpp:261 (in thread 'main')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:1522 (in thread 'main')
(2) 'cs_wallet' in wallet/wallet.cpp:1629 (in thread 'main')
```
This one is for `ResetPool()`.
## What was done?
Lock `cs_wallet` when calling `keyHolderStorage.ReturnAll()` in some places* to ensure the right order of locks.
(*In other places `cs_wallet` is held already, no need to double lock).
## How Has This Been Tested?
Mixing
## 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 b2910fba02
Tree-SHA512: 8be98df021f7683cd496ebe095dd7b32ebea76c7f9c7c085af3bc0a6901d9cfd4d4624e20a2eee1f3b0d69fd711f8fceb9a91c386b9bf02475632a23b3a0f09a
04ba164064 refactor: immediately update lastCleanupTime (pasta)
d0d2641154 refactor: minimize locking of cs_main in chainlocks.cpp (pasta)
Pull request description:
## Issue being fixed or feature implemented
Simple changes, just look at the two commits: first we minimize scope of cs_main to what actually needs it. Then we change where we update the `lastCleanupTime` to right after we check it to minimize any chance of two threads entering at the same time.
## What was done?
## How Has This Been Tested?
Built, ran for a bit
## 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)_
Top commit has no ACKs.
Tree-SHA512: 63432ccc16a67e6478e578c96fe809cf33d08e5068285c21e65ccc175c0c9e82c23519d57f0c4ebac162a094bfc20eb44df30cbe85b26815d02eaac06ceb66ad
0bba55f6af spork: replace `LOCKS_EXCLUDED` with negative `EXCLUSIVE_LOCKS_REQUIRED` (Kittywhiskers Van Gogh)
d657951f90 masternode: replace `LOCKS_EXCLUDED` with negative `EXCLUSIVE_LOCKS_REQUIRED` (Kittywhiskers Van Gogh)
8b1d3b55ab llmq: replace `LOCKS_EXCLUDED` with negative `EXCLUSIVE_LOCKS_REQUIRED` (Kittywhiskers Van Gogh)
cceff152ef coinjoin: replace `LOCKS_EXCLUDED` with negative `EXCLUSIVE_LOCKS_REQUIRED` (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
With the exception some usages of `cs_main` and a few (`Recursive`)`Mutex`es, Bitcoin has replaced their usage of `LOCKS_EXCLUDED(cs)` with `EXCLUSIVE_LOCKS_REQUIRED(!cs)` due to the stricter enforcement that negative locking brings with Clang (and it having a trickle-up effect caused by needing lock annotations on calling functions as well).
Dash intensively uses `LOCKS_EXCLUDED` for Dash-specific logic and moving it over also required updating (or adding) lock annotations for calling functions.
This pull request is being opened due to an upcoming pull request that includes https://github.com/bitcoin/bitcoin/pull/25109, which requires all `AssertLockNotHeld` usage to accompany a negative lock annotation.
## Breaking Changes
None expected. (Negative) lock enforcement has been made stricter but no new locks should be introduced by 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 **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 0bba55f6af
Tree-SHA512: 54c6b80f6ad8dc821cbe9250f8e8f111bc49fb7b38ef0de10423110c066c3e7ea4b93dc61580f319837ac4690ed21912f3ec43aaa1d3cd954043798a675226ee
10869fff3b 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: https://github.com/dashpay/dash/pull/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
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'cs_wallet' in wallet/wallet.cpp:3826 (in thread 'qt-init')
(2) 'pwallet->cs_wallet' in wallet/walletdb.cpp:705 (in thread 'qt-init')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:971 (in thread 'qt-init')
Current lock order is:
'cs_deqsessions' in coinjoin/client.cpp:261 (in thread 'main')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:1522 (in thread 'main')
(2) 'cs_wallet' in wallet/wallet.cpp:1629 (in thread 'main')
```
ab7ac1b85b partial bitcoin#25176: Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes (Kittywhiskers Van Gogh)
c89799d46c merge bitcoin#24692: Follow-ups to #21160 (Kittywhiskers Van Gogh)
33098aefff merge bitcoin#21160: Move tx inventory into net_processing (Kittywhiskers Van Gogh)
24205d94fe partial bitcoin#20196: fix GetListenPort() to derive the proper port (Kittywhiskers Van Gogh)
7f7200986b merge bitcoin-core/gui#526: Add address relay/processed/rate-limited fields to peer details (Kittywhiskers Van Gogh)
a9114f1ce8 merge bitcoin#23695: Always serialize local timestamp for version msg (Kittywhiskers Van Gogh)
d936c28b4e merge bitcoin#23575: Rework FillNode (Kittywhiskers Van Gogh)
6f8c730f35 merge bitcoin#19499: Make timeout mockable and type safe, speed up test (Kittywhiskers Van Gogh)
43a82bdd29 merge bitcoin#20769: fixes "advertised address where nobody is listening" (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/5978
* Dependent on https://github.com/dashpay/dash/pull/5977
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK ab7ac1b85b
Tree-SHA512: 87bf5108bb80576c5bff8cd577add7800044da252fd18590e06a727f0bf70de94e2e9294b4412cdd9f1f6676472b0359902af361aaffc4c9ee299ad07d6af009
c8742057ac fix(qt): tab switching via shortcuts doesn't work after 5986 (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Looks like 3265b54a2f (#5986) broke it
## What was done?
## How Has This Been Tested?
run dash-qt, try using shortcuts `cmd+1`, `cmd+2` etc. (on macos)
## 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 _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
ACK c8742057ac
kwvg:
ACK c8742057ac
Tree-SHA512: 62a593ec804d75ff8aada0ef9ea90106adbf8cd11b202a6296086f55c2a4d2181e56dc8e56193a0ed49d94e55ee3236ab441ab477c8ca6d7b0c649dff987dbbc
b4477e409c trivial: don't print `fDisableGovernance` value anymore (Kittywhiskers Van Gogh)
a42370df93 refactor: remove fDisableGovernance global, define default in variable (Kittywhiskers Van Gogh)
b1527599e4 refactor: remove fMasternodeMode and fDisableGovernance from Qt code (Kittywhiskers Van Gogh)
9402ce7171 refactor: limit usage of fDisableGovernance, use `IsValid()` instead (Kittywhiskers Van Gogh)
106f6bdd4e refactor: reduce fMasternodeMode usage in governance and mnauth (Kittywhiskers Van Gogh)
3ba293fbcc refactor: remove fMasternodeMode checks in CActiveMasternodeManager (Kittywhiskers Van Gogh)
b0216ac8a6 refactor: remove fMasternodeMode usage in rpc logic (Kittywhiskers Van Gogh)
4d629a04fb refactor: limit fMasternodeMode usage in blockstorage, init, net_processing (Kittywhiskers Van Gogh)
a9cbdfcebc refactor: remove fMasternodeMode usage from llmq logic (Kittywhiskers Van Gogh)
c62a3d5778 refactor: remove fMasternodeMode usage from coinjoin logic (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
Since https://github.com/dashpay/dash/pull/5940, `CActiveMasternodeManager` ceased to be a global variable and became a conditional smart pointer initialized based on the value of `fMasternodeMode`.
Likewise, since https://github.com/dashpay/dash/pull/5555, we can tell if any `CFlatDB`-based manager has successfully loaded its database. `CGovernanceManager` is one of them and conditionally loads its database based on the value of `fGovernanceDisabled`.
`fMasternodeMode` and `fGovernanceDisabled` were (and the former to a certain degree still is) unavoidable globals due to the way the functionality they influenced was structured (i.e. decided in initialization code with no way to query from the manager itself). As we can directly ask the managers now, we can start reducing the usage of these globals and at least in this PR, get rid of one of them.
This PR was the idea of PastaPastaPasta, special thanks for the suggestion!
## Additional Information
* There are two conventions being used for checking `nullptr`-ity of a pointer, `if (mn_activeman)` and `if (mn_activeman != nullptr)`. The former is used in initialization and RPC code due to existing conventions there ([source](2dacfb08bd/src/init.cpp (L1659-L1677)), [source](2dacfb08bd/src/rpc/net.cpp (L942-L945)), [source](2dacfb08bd/src/rpc/misc.cpp (L215-L218))). The latter is used whenever the value has to be passed as a `bool` (you cannot pass the implicit conversion to a `bool` argument without explicitly casting it) and in Dash-specific code where it is the prevalent convention ([source](2dacfb08bd/src/governance/governance.cpp (L125)), [source](2dacfb08bd/src/coinjoin/client.cpp (L1064))).
Unfortunately, that means this PR expresses the same thing sometimes in two different ways but this approach was taken so that reading is consistent within the same file. Codebase-wide harmonization is outside the scope of this PR.
* Where `mn_activeman` isn't directly available, the result of the check is passed as an argument named `is_masternode` and/or set for the manager during its construction as `m_is_masternode` (`const bool`) as it is expected for the `CActiveMasternodeManager`'s presence or absence to remain as-is for the duration of the manager's lifetime.
This does mean that some parts of the codebase check for `mn_activeman` while others check for {`m_`}`is_masternode`, which does reduce clarity while reading. Suggestions on improving this are welcomed.
* One of the reasons this PR was made was to avoid having to deal the _possibility_ of `fMasternodeMode` or `fDisableGovernance` from desynchronizing from the behaviour of the managers it's suppose to influence. It's why additional assertions were placed in to make sure that `fMasternodeMode` and the existence of `mn_activeman` were always in sync ([source](2dacfb08bd/src/evo/mnauth.cpp (L137-L139)), [source](2dacfb08bd/src/rpc/governance.cpp (L319-L320))).
But removing the tracking global and relying on a manager's state itself prevents a potential desync, which is what this PR is aiming to do.
## Breaking Changes
None expected.
## 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 **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 7861afd17c83b92af4c95b2841e9b0f676116eb3f234c4d0b1dcd3c20395452893e8ca3a17c7225389c8411dac80aeb5050f06a2ae35df5ec48998a571ef120c
e5129e6c41 fix: if hdseed is wrong - do not setup random seed, user can lost his fund (Konstantin Akimov)
e52498b7d5 chore: add todoes to UpgradeToHD function (Konstantin Akimov)
0d12ea9fa6 fix: wrong if/else branches for key refill in wallet creation (Konstantin Akimov)
d2c3dcb839 refactor: move list of function in rpcwallet to the end (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Reviewed our implementation of HD wallets and compare it to bitcoin's for aiming to backport or re-implement `sethdseed` rpc.
Noticed some strange things in our implementation, which this PR is aim to fix.
## What was done?
See each commit for detailed changes.
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
`-hdseed` doesn't assign a random seed anymore if an user provided an invalid hex string.
## 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 e5129e6c41
Tree-SHA512: b404dbc0762777abf421a847f58cd243e0aa00151ba3f036835c9ff54c1109b6159921ec24e29455e975797f49d54832c55f7876188da90f37dd1e4a811a21e0
CGovernanceManager::IsValid() returns true only if its db is successfully
initialized. If we attempt to initialize it and fail, init logic will
report to us that it failed. If we don't attempt to initialize it at all,
it will remain false.
Since fDisableGovernance is the same as not initializing it at all and
the other case where IsValid() is false is dealt with in init, we can
use IsValid() to infer if governance is enabled.
CActiveMasternodeManager no longer exists as a global variable, it is a
conditionally initialized smart pointer. If it exists, then it's in masternode
mode, no need to check if we're in masternode mode anymore.
7cc77f3a30 Merge #21373: test: generate fewer blocks in feature_nulldummy to fix timeouts, speed up (MarcoFalke)
a933a60b1a feat: new command line argument -bip147height for bitcoin#21373 (Konstantin Akimov)
51911388f2 Merge #21377: Speedy trial support for versionbits (fanquake)
ecade9bc39 Merge bitcoin/bitcoin#21749: test: Bump shellcheck version (W. J. van der Laan)
eeec2f2799 Merge bitcoin/bitcoin#21884: fuzz: Remove unused --enable-danger-fuzz-link-all option (fanquake)
51633d70ea Merge bitcoin/bitcoin#21874: fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT (MarcoFalke)
a02a2c0322 Merge bitcoin/bitcoin#21681: validation: fix ActivateSnapshot to use hardcoded nChainTx (MarcoFalke)
71f23d6e33 Merge bitcoin/bitcoin#21814: test: Fix feature_config_args.py intermittent issue (MarcoFalke)
de4d2a839d Merge bitcoin/bitcoin#21714: refactor: Drop CCoinControl::SetNull (MarcoFalke)
a63f9c31cc Merge bitcoin-core/gui#284: refactor: Simplify SendCoinsDialog::updateCoinControlState (Hennadii Stepanov)
b2d889380c Merge bitcoin/bitcoin#21792: test: Fix intermittent issue in p2p_segwit.py (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular batch of backports from bitcoin v22
## What was done?
Implemented new commandline argument `-bip147height` for RegTest.
Backports:
- bitcoin/bitcoin#21377
- bitcoin/bitcoin#21792
- bitcoin-core/gui#284
- bitcoin/bitcoin#21714
- bitcoin/bitcoin#21373
- bitcoin/bitcoin#21814
- bitcoin/bitcoin#21681
- bitcoin/bitcoin#21874
- bitcoin/bitcoin#21884
- bitcoin/bitcoin#21749
## 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:
PastaPastaPasta:
utACK 7cc77f3a30
Tree-SHA512: d46218667430af19c445d67d6411b1e7f19920e85f86e6e74ae7b9062aeb3763637a8613587c203ba8d285ccc3ee755f936141010944cfae8627397e8b8584d3
f2fe39fadc trivial: add `Asserts` for `m_peerman` pointer container uses (Kittywhiskers Van Gogh)
35be4e2ebe llmq: pass PeerManager to llmq::CInstantSendManager constructor (Kittywhiskers Van Gogh)
bfd33cd2b4 net: move CConnman::RelayInv{Filtered} into PeerManager (Kittywhiskers Van Gogh)
313a7e9a50 trivial: cleanup unnecessary headers in context files (Kittywhiskers Van Gogh)
c3f1ac2291 net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction (Kittywhiskers Van Gogh)
0323c6ca17 net: move Relay{Inv, InvFiltered, Transaction} out of CConnman (Kittywhiskers Van Gogh)
d54ba447a7 net: use ForEachNode instead of manually iterating through vNodes (Kittywhiskers Van Gogh)
Pull request description:
## Additional information
* Dependency for https://github.com/dashpay/dash/pull/5982
* Unfortunately, we are stuck with using the `unique_ptr` const-ref of `PeerManager` for initializing {`CJ`, `LLMQ`}`Context` due to some unit tests depending on Dash-specific entities but based on a testing setup that doesn't initialize `PeerManager` (specifically `validation_chainstatemanager_tests`, which uses `ChainTestingSetup`, as opposed to `TestingSetup`, which initializes `PeerManager`).
Attempting to dereference it earlier will result in crashing (see CI pipeline [here](https://gitlab.com/dashpay/dash/-/pipelines/1245924304))
## Breaking Changes
None observed.
## 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 **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
re-utACK [f2fe39f](f2fe39fadc)
knst:
utACK [f2fe39f](f2fe39fadc)
Tree-SHA512: 5e9ca47ba9f7f0c5fdc93bf6cfd99d91be6d134610d2bd3dccde17443a956c7d8848b3406082ee8b6ee3fd25f586ce7bac45bc89b568424479b8fc149172ae51
