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.
b9a2ce74c0 fix: remove stretching from Overview page when it's not needed (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Overview page has strange stretching on sides, which make balance moving left-right when window is scalled.
## What was done?
Removed stretches for Overview Page from the sides, it makes window a little more balanced. This PR lets to do backport of bitcoin-core/gui#176 which improves behavior further more be increasing size of "transaction list" part of window.
## How Has This Been Tested?
See screenshot. Resized window with/without patch, in "Discreet mode" off and on. Both looks not perfect but better than before.
## 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: f599f4f40986b37c3c9a8c36e2146bdf7b8e38d15a6f65f26fec4e30440ab54d07ea1f36a2b11e48a6dc3bcd64ececd3c4da6255fc6c9f58fba2a4abb2221b8d
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
That are:
- feature_dip3_deterministicmns.py
- interface_zmq_dash.py
- feature_governance.py
- wallet_upgradetohd.py (as expected to be implemented for legacy-only wallets)
- p2p_timeouts.py (why? can not understand it)
This partially reverts commit b20f812674.
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')
```
Includes:
RecursiveMutex -> Mutex,
renaming of `cs` to something more meaningful,
usage of atomics where trivially possible,
introduce a method CQuorum::SetVerificationVector to avoid needing to lock an internal mutex externally
fix: avoid cs_vvec_shShare double-lock
Co-authored-by: UdjinM6 <udjinm6@users.noreply.github.com>
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.
