ded1b5a3df fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
**It fixes rpc failure: "Work queue depth exceeded"**
As I checked on running `dashd` in deadlock condition:
Thread 78 is a thread that keep `cs_main`:
```
#14 0x0000aaaad1f8d604 in BuildSimplifiedMNListDiff () at evo/simplifiedmns.cpp:364
```
but it is locked by `contributionsCacheCs`
```
#8 llmq::CDKGSessionManager::GetVerifiedContributions () at llmq/dkgsessionmgr.cpp:392
```
On other hand, `contributionsCacheCs` is blocked by Thread 59
```
#17 0x0000aaaad1ba1940 in llmq::CDKGSessionManager::GetVerifiedContributions () at llmq/dkgsessionmgr.cpp:393
```
and it makes circuit lock by waiting `cs_main` in
```
#9 ReadBlockFromDisk () at node/blockstorage.cpp:75
```
See https://github.com/dashpay/dash-issues/issues/69 for more details
Seems introduced there: dashpay/dash#3911
## What was done?
Deadlock is removed by reducing scope of mutex
## How Has This Been Tested?
I reviewed 2 different servers which have status `work queue exceeded`, both have same deadlock, so, this patch should fix this issue. Once this fix is merged, we can test it on testnet.
## 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
- [ ] 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: 4fe5c03c464ee6934fb927b897f007b65a8995723196edaffdae067edee7067da151130d4c4bac47d3418fdad5c8e130682f42d7ef9c044380a8c8fff78ee008
8b6c96d208 refactor: a new constant with Tx Version (Alessandro Rezzi)
9d429f4005 refactor: drop functions from struct which supposed to be simple as possible (Alessandro Rezzi)
b2bb097197 refactor: simplify vExtra using in wallet and add const (Alessandro Rezzi)
d9f0e93498 refactor: use CTransaction for immutable tx in evo_assetlocks_tests (Alessandro Rezzi)
ca0fe8c208 refactor: introduce HasExtraPayloadField() (Alessandro Rezzi)
72d2008901 refactor: introduce IsSpecialTxVersion() (Alessandro Rezzi)
Pull request description:
## Issue being fixed or feature implemented
Refactor extracted from #5860. Even if that PR should not need anymore this commit (since it has been found a better way to activate dip143), I think It's still a worthy refactor
## What was done?
Introduce `tx.IsSpecialTxVersion()` in place of `tx.nVersion == 3`, `tx.nVersion >= 3`
and `tx.HasExtraPayloadField()` in place of `tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL`
## How Has This Been Tested?
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 8b6c96d208
Tree-SHA512: aa16f9ee570e0fa86561cc440ffba40f6d554caa3e08b630b481732b899cc613eef596c672b5a20dbf3582ad109ffb687f4ad815f712dc16f636f8857d98480a
ae74ad09fb Merge #20817: lint: update list of spelling linter false positives, bump to codespell 2.0.0 (fanquake)
ab430bdb81 partial Merge #20451: lint: run mypy over contrib/devtools (Wladimir J. van der Laan)
3231ad2255 Merge #19983: Drop some TSan suppressions (MarcoFalke)
802cb9521f Merge #20697: ci: Fix COMMIT_RANGE variable value for PRs (MarcoFalke)
53ca879837 Merge #20682: ci: Install missing lint packages (MarcoFalke)
20219690a8 chore: drop travis mentioning in docs and comments (Konstantin Akimov)
8daef64f04 Merge #20691: ci, doc: Travis CI features and mentions cleanup (MarcoFalke)
8694479024 Merge #20680: ci: Only use credits for pull requests to the main repo (MarcoFalke)
9d824de302 Merge #20658: ci: Move linter task to cirrus (MarcoFalke)
955fc41375 Merge #20615: cirrus: Schedule one task with paid credits for faster CI feedback (Wladimir J. van der Laan)
5d66d57032 Merge #20572: ci: Adjust Cirrus CI task names (follow up) (MarcoFalke)
d11e379ed4 Merge #20545: ci: Adjust cirrus ci task names (MarcoFalke)
2fa526bb81 Merge #20543: ci: no-longer exclude feature_block in TSAN job (MarcoFalke)
fcb4c20802 Merge #19179: ci: Run ci configs on cirrus (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin v22, mostly CI related, other to improve lints
CI related changes are not really important because we use gitlab runner instead cirrus, but keep them up to date for sake of codebase unification.
## What was done?
- bitcoin/bitcoin#19179
- bitcoin/bitcoin#20543
- bitcoin/bitcoin#20545
- bitcoin/bitcoin#20572
- bitcoin/bitcoin#20615
- bitcoin/bitcoin#20658
- bitcoin/bitcoin#20680
- bitcoin/bitcoin#20691
- bitcoin/bitcoin#20682
- bitcoin/bitcoin#20697
- bitcoin/bitcoin#19983
- partial bitcoin/bitcoin#20451
- bitcoin/bitcoin#20817
## 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
Top commit has no ACKs.
Tree-SHA512: 5faffe15a75b78a9ea32b49f2371d6ff70c319a983c7a2b4ca6792ba3ea03f2170bacf5c255151da650948ad279c456475151d0db7dcd708eae540b30d88a05e
f3ba916e8b5b5ee2a381cef38882671eadb231df lint: ignore gitian keys file for spelling linter (Sebastian Falbesoner)
da289a6c4a0a5e110e301f34f1db57b6d31bcdcc lint: update list of spelling linter false positives (Sebastian Falbesoner)
a0022f1cfbb3d8f1f8f3ff135f854be0cb89643f test: bump codespell linter version to 2.0.0 (Sebastian Falbesoner)
Pull request description:
This small patch updates the ignore list for the spelling linter script (which uses `codespell`), both removing false-positives that are not relevant anymore and adding new ones. As [suggested by jonatack](https://github.com/bitcoin/bitcoin/pull/20762#issuecomment-750889701)~~, whose last name is now also part of the list :)~~. Also changed the linter script to not check the gitian keys file, as [suggested by hebasto](https://github.com/bitcoin/bitcoin/pull/20817#discussion_r550763409). The codespell version used is bumped to most recent version 2.0.0, which is more aware of some terms that were previously needed in the ignorelist for v1.17.1, see https://github.com/bitcoin/bitcoin/pull/20817#issuecomment-753428669.
Running spelling linter on master branch (repeated findings in the same file are removed to keep the output short):
```
$ ./test/lint/lint-spelling.sh
contrib/gitian-keys/keys.txt:16: Atack ==> Attack
doc/developer-notes.md:1284: inout ==> input, in out
doc/psbt.md:122: Asend ==> Ascend, as end
src/bench/verify_script.cpp:27: Keypair ==> Key pair
src/blockencodings.h:30: Unser ==> Under, unset, unsure, user
src/compressor.h:65: Unser ==> Under, unset, unsure, user
src/core_read.cpp:131: presense ==> presence
src/index/disktxpos.h:21: blockIn ==> blocking
src/net_processing.h:67: anounce ==> announce
src/netaddress.h:486: compatiblity ==> compatibility
src/primitives/transaction.h:35: nIn ==> inn, min, bin, nine
src/qt/bitcoinunits.cpp:101: nIn ==> inn, min, bin, nine
src/rpc/blockchain.cpp:2150: nIn ==> inn, min, bin, nine
src/rpc/misc.cpp:198: nIn ==> inn, min, bin, nine
src/script/bitcoinconsensus.cpp:81: nIn ==> inn, min, bin, nine
src/script/bitcoinconsensus.h:63: nIn ==> inn, min, bin, nine
src/script/interpreter.cpp:1279: nIn ==> inn, min, bin, nine
src/script/interpreter.h:222: nIn ==> inn, min, bin, nine
src/script/sign.cpp:17: nIn ==> inn, min, bin, nine
src/script/sign.h:39: nIn ==> inn, min, bin, nine
src/serialize.h:181: Unser ==> Under, unset, unsure, user
src/signet.cpp:142: nIn ==> inn, min, bin, nine
src/test/base32_tests.cpp:17: fo ==> of, for
src/test/base64_tests.cpp:17: fo ==> of, for
src/test/script_tests.cpp:1509: nIn ==> inn, min, bin, nine
src/test/sighash_tests.cpp:27: nIn ==> inn, min, bin, nine
src/test/validation_tests.cpp:78: excercise ==> exercise
src/undo.h:36: Unser ==> Under, unset, unsure, user
src/validation.cpp:1403: nIn ==> inn, min, bin, nine
src/validation.h:255: nIn ==> inn, min, bin, nine
src/wallet/wallet.cpp:1532: nIn ==> inn, min, bin, nine
src/wallet/walletdb.cpp:429: Crypted ==> Encrypted
test/functional/feature_nulldummy.py:63: unnecssary ==> unnecessary
test/functional/wallet_encryption.py:81: crypted ==> encrypted
test/functional/wallet_upgradewallet.py:36: fpr ==> for, far, fps
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
Running spelling linter on PR branch:
```
$ ./test/lint/lint-spelling.sh
src/core_read.cpp:131: presense ==> presence
src/net_processing.h:67: anounce ==> announce
src/netaddress.h:486: compatiblity ==> compatibility
src/test/validation_tests.cpp:78: excercise ==> exercise
src/wallet/walletdb.cpp:429: Crypted ==> Encrypted
test/functional/feature_nulldummy.py:63: unnecssary ==> unnecessary
test/functional/wallet_encryption.py:81: crypted ==> encrypted
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
This list of remaining findings doesn't contain false positives anymore -- the typos are fixed in PR https://github.com/bitcoin/bitcoin/pull/20762.
Happy new year! 🍾
ACKs for top commit:
hebasto:
re-ACK f3ba916e8b5b5ee2a381cef38882671eadb231df, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/20817#pullrequestreview-560632881) review.
jonatack:
ACK f3ba916e8b5b5ee2a381cef38882671eadb231df I don't know if there are any particular issues with bumping codespell to v2.0.0, but locally running the spelling linter and the cirrus job at https://cirrus-ci.com/task/5004066998714368 both LGTM. Thanks for also verifying and removing the unused words from the ignore list.
Tree-SHA512: e92ae6f16c01d4ff3d54f8c3a0ee95e12741f7bfe031d307a785f5cfd8a80525b16b34275f413b914c4a318f5166f9887399c21f2dad9cc7e9be41647042ef37
1ef2138c0db3bd4f9332c777fa3fb2770dc1b08c lint: run mypy over contrib/devtools (fanquake)
Pull request description:
wumpus mentioned on IRC that we don't currently run `mypy` over the `contrib/devtools` directory, and that it would likely be worthwhile given #20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations.
master (patched to check contrib devtools):
```bash
test/lint/lint-python.sh
contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str")
contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...")
contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...")
Found 4 errors in 3 files (checked 187 source files)
```
I haven't quite gone as far as to add annotations like
```python
CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {...
```
to `symbol-check.py`.
ACKs for top commit:
laanwj:
ACK 1ef2138c0db3bd4f9332c777fa3fb2770dc1b08c
Tree-SHA512: a58c2ece588c640289dc1d35dad5b1b8732788272daa0965d6bf44ee8a7f7c8e8585f94d233ac41c84b9ffcfc97841a00fe2c9acba41f58fd164f01de4b6512b
3e1571285f4a0edf59d51bbdeee028be3038b6dc Update TSan suppressions (Hennadii Stepanov)
Pull request description:
It seems possible now to drop some TSan suppressions.
Top commit has no ACKs.
Tree-SHA512: 94518fd2f3a7168b2989424de0696e42c8f509b833aafbc7e75f4c1180a0b8d9a47f43c50d06b03b26a924643afe86274b2062c9d456c17a68576d19566ed66f
3c2478c38522c176e81befd4d991a259b09be063 ci: Print COMMIT_RANGE to the log as it was in Travis CI (Hennadii Stepanov)
c123892c2e47e3706f06820aba2454d494a39564 ci: Drop Travis-specific workaround for shellcheck (Hennadii Stepanov)
10af252d97532843b26505d215f6e975f4b21672 ci: Drop Travis-specific way to set COMMIT_RANGE variable (Hennadii Stepanov)
93504da3a932f33126545ebc9383f695a6efe51e ci: Fix COMMIT_RANGE variable value for PRs (Hennadii Stepanov)
Pull request description:
This PR:
- is a #20658 and #20682 followup
- set the `COMMIT_RANGE` variable correctly for PRs
- cleans up Travis-specific code
- prints COMMIT_RANGE value to the log for convenience as it was in Travis CI
ACKs for top commit:
MarcoFalke:
ACK 3c2478c38522c176e81befd4d991a259b09be063
Tree-SHA512: beb933352b10fd5eb3e66373ddb62439e4f3a03b50fb037ee89fa92c0706cec41d05f2d307f15bb18d1e634e6464f4e123b7e2f88703c8edfd145d8d6eff0b1a
faeb40bee3bca9477785659d89af53cafa2333b5 ci: Install missing lint packages (MarcoFalke)
Pull request description:
The cirrus container is vanilla ubuntu, so we need to install the needed packages
ACKs for top commit:
hebasto:
ACK faeb40bee3bca9477785659d89af53cafa2333b5, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: e56198108e26ea0ba2a344b1b74bc294652f34e9866cca053a25fb1b83bbd87ea40254c340e5e169fdfcbd4dcb39fdc2078b5157ca729a22a9a1792ec514a33e
95487b055328b590ba83f258de9637ab0f9a2f17 doc: Drop mentions of Travis CI as it is no longer used (Hennadii Stepanov)
09d105ef0f8b4b06bf248721a1209c9e16e9db75 ci: Drop travis_fold feature as Travis CI is no longer used (Hennadii Stepanov)
Pull request description:
As Travis CI is no longer used, this PR:
- drops `travis_fold` feature
- drops mentions of Travis CI in docs
ACKs for top commit:
MarcoFalke:
ACK 95487b055328b590ba83f258de9637ab0f9a2f17
Tree-SHA512: 2e259bb8b1e37bcefc1251737bb2716f06ddb57c490010b373825c4e70f42ca38efae69a2f63f21f577d7cee3725b94097bdddbd313f8ebf499281cf97c53cef
facf5e37f619be6bd7f8768058842679cd9ad933 ci: Only use credits for pull requests to the main repo (MarcoFalke)
Pull request description:
No need to spend credits for the `master` branch, because the build shouldn't fail there anyway and it is not time-critical to get a fast feedback.
Some other changes:
* Disable `stateful` for faster scheduling
* Reduce lint memory from 8G to 1G for faster scheduling
ACKs for top commit:
hebasto:
ACK facf5e37f619be6bd7f8768058842679cd9ad933, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4d052e68217086574b9ea3d603cde1b585833c289d47dfed5308ff001d02964dc75ec3b3ebf5b233ccd09c47ad4ff5ba0bef639bf6362d984e7c49fca8fec24b
4045a6722c884be779e86016313061ac6ff80808 ci: Use cpu=1 for linter (Dhruv Mehta)
739d39022d2959c1114c14a0065daebf4fe812c1 ci: Move linter task to cirrus (Dhruv Mehta)
Pull request description:
Solves #20467: Move linter to Cirrus-CI as Travis-CI.org is shutting down
ACKs for top commit:
MarcoFalke:
ACK 4045a6722c884be779e86016313061ac6ff80808
Tree-SHA512: 9aa7487ac86c91fc68bb584d29134e304dbd46702514a5d47d1ef0de6b877d96d42b7589870fc67ad9a31f5d3a789728446da4418688f336111a9ba0f8de5feb
faf2c6e32e5b27cf936c9b26e6059027dc020374 cirrus: Schedule one task with paid credits for faster CI feedback (MarcoFalke)
Pull request description:
During times of high activity in the repo, the scheduling of Cirrus CI tasks might put them a few hours in the future. This is fine when all the tasks eventually pass. Though for failing tasks, a failure should ideally be shown to the author and reviewer as soon as possible.
Compute credits can be used to schedule immediately: https://cirrus-ci.org/pricing/#compute-credits. Running all tasks with compute credits will probably be more expensive than our previous CI invoice. However, they are also more flexible.
As a start we could enable only a single task and revisit/re-evaluate the next steps in a month.
ACKs for top commit:
laanwj:
Concept ACK faf2c6e32e5b27cf936c9b26e6059027dc020374
fanquake:
ACK faf2c6e32e5b27cf936c9b26e6059027dc020374
practicalswift:
cr ACK faf2c6e32e5b27cf936c9b26e6059027dc020374: patch looks correct
Tree-SHA512: df599e1c4cc0394f7f03413ad29954ddc87b163b02640d8bfc0497a5dc8d237b8c963c1dd9d01ac83c4a044f575300763097dac2ea6d1a4a163a1cece342b743
fa5c4f12f5d017d8dbe7700ca9688189eacbf32b ci: Adjust cirrus ci task names (MarcoFalke)
Pull request description:
The task names are too long for GitHub to display them properly without truncation in the "checks-view". Fix that by using a new naming scheme:
* Native builds don't mention "x86_64 Linux", as it is redundant, they do mention the OS (bionic or focal) in the name suffix
* Cross builds mention the target in the prefix and the OS (always bionic) in the suffix
* the macos native build simply says "macos native"
ACKs for top commit:
practicalswift:
ACK fa5c4f12f5d017d8dbe7700ca9688189eacbf32b: patch looks correct!
hebasto:
ACK fa5c4f12f5d017d8dbe7700ca9688189eacbf32b, I have reviewed the code and it looks OK, I agree it can be merged.
RiccardoMasutti:
ACK fa5c4f1
Tree-SHA512: 856deb0577c97c70069ef1d369991addc49522135c0ad9e382218fd79ba3d55a95d6c601288dcef0510764b92fbd30a9d7de32b08dc5be55482deab14049b892
2b356117e94f9ef27b67a8e98663f5d676f58c11 ci: no-longer exclude feature_block in TSAN job (fanquake)
Pull request description:
The TSAN job is now running on Cirrus.
Increase the allocated memory to the [maximum allowed](https://cirrus-ci.org/guide/linux/#linux-containers).
ACKs for top commit:
jonasschnelli:
utACK 2b356117e94f9ef27b67a8e98663f5d676f58c11 - checked the CI run and confirmed that the feature_block runs: https://cirrus-ci.com/task/6008403543719936?command=ci#L3249
MarcoFalke:
review ACK 2b356117e94f9ef27b67a8e98663f5d676f58c11
Tree-SHA512: b774995600361c74bc3267b566e12add66a4604bdf88f6e3f69669edbb8d7aff6f20fdbf0ef98187be4730ce4e18b1939bbcecd993a5c5c1ff40b237c7921b71
fa7a4385d08797876de9a05ae8a6fde18e36bd67 ci: Fix doc typos in .cirrus.yml (MarcoFalke)
fa73674738b0b5a271ae387e2ebd76d747e02d91 ci: Run i686 centos ci config on cirrus (MarcoFalke)
fa1f949a4dc2568456f3ba30bbd8ecbc78a423ca ci: Run nowallet ci config on cirrus (MarcoFalke)
Pull request description:
Travis CI Org is shutting down, so move the configs to cirrus ci
ACKs for top commit:
practicalswift:
ACK fa7a4385d08797876de9a05ae8a6fde18e36bd67: patch looks correct
Tree-SHA512: 1b7125c7f0d2288931fb8c5e90345891d5f7c1f00c4af136afbeb36bd2836f72920a1877dacc7be787e2c8d84de2a733c1ca71931e5c101b222c1fea588619b3
815e4f8026 masternode: protect m_{error,state} with cs (pasta)
136e445abc refactor: pass CActiveMasternodeManager as pointer arg to LLMQContext (Kittywhiskers Van Gogh)
5e0f77747a refactor: pass CActiveMasternodeManager as pointer arg to CJContext (Kittywhiskers Van Gogh)
f171c24a29 refactor: add CActiveMasternodeManager NodeContext alias, use in RPC (Kittywhiskers Van Gogh)
44beb941cb refactor: prefix member variable names with m_ (Kittywhiskers Van Gogh)
73cef4f5f9 refactor: make bls{Pub}KeyOperator member variables instead of pointers (Kittywhiskers Van Gogh)
fbc783635a refactor: make m_info private, get const refs (or copies) from Get*() functions (Kittywhiskers Van Gogh)
1b516ce4ed refactor: use signing helper function instead of passing blsKeyOperator (Kittywhiskers Van Gogh)
33702aca39 refactor: add helper function to decrypt messages with blsKeyOperator (Kittywhiskers Van Gogh)
3eb931b596 refactor: add helper function to sign messages with blsKeyOperator (Kittywhiskers Van Gogh)
3827355cce refactor: move key initialization to InitKeys, define destructor (Kittywhiskers Van Gogh)
e5295dec1f refactor: move activeMasternodeInfo{Cs} into CActiveMasternodeManager (Kittywhiskers Van Gogh)
b8c1f010e7 refactor: avoid accessing active masternode info if not in masternode mode (Kittywhiskers Van Gogh)
9a3c5a3c48 trivial: access activeMasternodeInfo when lock is in scope (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* `CActiveMasternodeManager`, unlike other managers, is _conditionally_ initialized (specifically, when the node is hosting a masternode). This means that checks need to be made to ensure that the conditions needed to initialize the manager are true or that the pointer leads to a valid manager instance.
As the codebase currently checks (and fast-fails) based on the node being in "masternode mode" (`fMasternodeMode`) or not, we will continue with this approach, but with additional assertions _after_ the masternode mode check if the manager exists.
* Though, since `activeMasternodeInfo`(`Cs`) are global variables, they can be accessed _regardless_ of whether the corresponding manager exists. This means some parts of the codebase attempt to fetch information about the (nonexistent) active masternode _before_ determining if it should use the masternode mode path or not (looking at you, `CMNAuth::ProcessMessage`)
Moving them into `CActiveMasternodeManager` meant adding checks _before_ attempting to access information about the masternode, as they would no longer be accessible with dummy values ([here](2110c0c309/src/init.cpp (L1633-L1635))) on account of being part of the conditionally initialized manager.
* In an attempt to opportunistically dereference the manager, `CDKGSessionManager` (accepting a pointer) was dereferencing the manager before passing it to `CDKGSessionHandler`. This was done under the assumption that `CDKGSessionManager` would only ever be initialized in masternode mode.
This is not true. I can confirm that because I spent a few days trying to debug test failures. `CDKGSessionHandler` is initialized in two scenarios:
* In masternode mode
* If the `-watchquorums` flag is enabled
The latter scenario doesn't initialize `CActiveMasternodeManager`.
Furthermore, the DKG round thread is started unconditionally ([here](2110c0c309/src/llmq/context.cpp (L79))) and the `CDKGSessionHandler::StartThreads` > `CDKGSessionHandler::StartThread` > `CDKGSessionHandler::PhaseHandlerThread` > `CDKGSessionHandler::HandleDKGRound` > `CDKGSessionHandler::InitNewQuorum` > `CActiveMasternodeManager::GetProTxHash` call chain reveals an attempt to fetch active masternode information without any masternode mode checks.
This behaviour has now been changed and the thread will only be spun up if in masternode mode.
* Dereferencing so far has been limited to objects that primarily hold data (like `CCoinJoinBroadcastTx` or `CGovernanceObject`) as they should not have knowledge of node's state (that responsibility lies with whatever manager manipulates those objects), perform one-off operations and static functions.
* `activeMasternodeInfo` allowed its members to be read-write accessible to anybody who asked. Additionally, signing and decrypting involved borrowing the operator secret key from the active masternode state to perform those operations.
This behaviour has now been changed. The internal state is now private and accessible read-only as a const ref (or copy) and `Decrypt`/`Sign` functions have been implemented to allow those operations to happen without having another manager access the operator private key in order to do so.
* You cannot combine a `WITH_LOCK` and an `Assert` (in either mutex or accessed value), doing so will cause errors if `-Werror=thread-safety` is enabled. This is why `assert`s are added even when it would intuitively seem that `Assert` would've been more appropriate to use.
## Future Considerations
Currently there are no unit tests that test the functionality of `CActiveMasternodeManager` as it's never initialized in test contexts, breakage had to be found using functional tests. Perhaps some (rudimentary) tests for `CActiveMasternodeManager` may prove to be valuable.
## Breaking Changes
Not _really_. Some behaviour has been modified but nothing that should necessitate updates or upgrades.
## 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 815e4f8026
Tree-SHA512: cbe49ea9e1c35df514e1b40869ee271baef1c348c9d09e4b356e5fc8fe5449cbbe66569258f2d664029faa9a46f711df9bf9e41eb8734c3aefc6cd8e94378948
d5d1a714fb Merge bitcoin/bitcoin#24390: test: Remove suppression no longer needed with headers-only Boost.Test (fanquake)
51630d2e5e Merge bitcoin/bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId (MarcoFalke)
a9b1575fe8 Merge bitcoin/bitcoin#22781: wallet: fix the behavior of IsHDEnabled, return false in case of a blank hd wallet. (Samuel Dobson)
0505229c89 Merge bitcoin/bitcoin#22327: cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
1dc97c7679 Merge bitcoin/bitcoin#22149: test: Add temporary logging to debug #20975 (W. J. van der Laan)
44f91cbc9a Merge #21597: test: Document race:validation_chainstatemanager_tests suppression (fanquake)
c326830f48 Merge bitcoin-core/gui#243: fix issue when disabling the auto-enabled blank wallet checkbox (MarcoFalke)
267f42fd6a Merge #21382: build: Clean remnants of QTBUG-34748 fix (fanquake)
1fcc5f1101 Merge #20540: test: Fix wallet_multiwallet issue on windows (MarcoFalke)
4afbaf2ea1 Merge #20322: test: Fix intermittent issue in wallet_listsinceblock (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Batch of backports
## What was done?
Trivial batch of backports
## How Has This Been Tested?
CI looks good
## Breaking Changes
None
## 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)_
Top commit has no ACKs.
Tree-SHA512: 8eeac54f011eb1111888c745dd56184ac9601de290f2b0f7b7ad02240e8dc1cab5a47fed26bfed2bd6f1066e0710827a3e5b2426f0bf66821cf1cd09099d5160
0d9046586b merge bitcoin#21254: Avoid connecting to real network when running tests (Kittywhiskers Van Gogh)
2f672bdb3a merge bitcoin#20721: Move ping data to net_processing (Kittywhiskers Van Gogh)
0d46acbfef merge bitcoin#21165: Use mocktime in test_seed_peers (Kittywhiskers Van Gogh)
8f40769385 merge bitcoin#19884: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty (Kittywhiskers Van Gogh)
446076d094 test: add missing `dnsseed=0` in configuration (Kittywhiskers Van Gogh)
081d8db4d5 mempool: remove stray boost::optional usage (Kittywhiskers Van Gogh)
bcd383c2d6 merge bitcoin#20724: Cleanup of -debug=net log messages (Kittywhiskers Van Gogh)
8c63868d9b merge bitcoin#19972: Add fuzzing harness for node eviction logic (Kittywhiskers Van Gogh)
18f2dc0865 partial bitcoin#19829: Move block inventory state to net_processing (Kittywhiskers Van Gogh)
ec77bd3ab4 net: move nLast{Block,TX}Time to match upstream location (Kittywhiskers Van Gogh)
f635e4a95f merge bitcoin#20624: Remove nStartingHeight check from block relay (Kittywhiskers Van Gogh)
9f1a3e5f18 merge bitcoin#20477: Add unit testing of node eviction logic (Kittywhiskers Van Gogh)
2d838a627c merge bitcoin#20146: Send post-verack handshake messages at most once (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* [bitcoin#19884](https://github.com/bitcoin/bitcoin/pull/19884) doesn't seem to play nice on its own and necessitated two more backports (namely [bitcoin#21165](https://github.com/bitcoin/bitcoin/pull/21165) and [bitcoin#21254](https://github.com/bitcoin/bitcoin/pull/21254)) before it did.
* Trying to find why that is has been time consuming but there doesn't seem to be a concrete answer. If running the daemon normally (`dashd --regtest --dnsseed=1`), the functionality behaves as expected but the test still fails.
* The closest explanation is that our OOO backports with relation to mocking time could explain why it isn't working as expected due to debug statements I added that always shown the time delta between each "enough time has passed" check was 0 seconds even when the log was advancing forward in time.
* The usage of `dnsseed=0` stems from [bitcoin#16551](https://github.com/bitcoin/bitcoin/pull/16551) (link to diff in commit comment), a backport that was skipped due to complexity. Though, some aspects of the PR have made it with [dash#3946](https://github.com/dashpay/dash/pull/3946).
* [bitcoin#19829](https://github.com/bitcoin/bitcoin/pull/19829) does away with `CConnman::ForEachNode` usage in `PeerManagerImpl::UpdatedBlockTip` ([source](https://github.com/bitcoin/bitcoin/pull/19829/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1318-R1328)). Dash cannot do the same and continues to use `ForEachNode` as we use the `CNode::CanRelay` check, which is not accessible through the `Peer` struct.
* It would be valuable to find values that are Dash-specific (like `m_masternode_connection`) and migrate them to `Peer` to avoid Dash-specific patches and closer alignment with upstream.
## Breaking Changes
Potential change in behaviour in the GUI and RPC.
In RPC, `getpeerinfo` will now display `pingwait` and `startingheight` if `fStateStats` is true (earlier behaviour was unconditional). `startingheight` has been placed below `banscore` (earlier behaviour placed it above). In the GUI (Qt), `peerHeight` and `peerPingWait` are subject to similar conditionality as mentioned earlier.
No changes in protocol or consensus. Changes are primarily related to refactoring, cleaning up, improving networking code and adding a new flag (`-fixedseeds`).
## 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 0d9046586b
Tree-SHA512: 79eedf47387a8715fc9f20c6bc051d4eae832266454445043e1478dc36daafc1679e002623917af43cf923735217622e3985f664123a1de23fadfdfece7e9b6b
81738d2881253f28b69666ada2a01ebb353f503a test: Remove suppression no longer needed with headers-only Boost.Test (Hennadii Stepanov)
Pull request description:
It appears, that moving to [headers-only](https://github.com/bitcoin/bitcoin/pull/24301) Boost.Test makes the removed suppression unneeded even without [bumping](https://github.com/bitcoin/bitcoin/pull/24383) boost version.
ACKs for top commit:
MarcoFalke:
cr ACK 81738d2881253f28b69666ada2a01ebb353f503a
Tree-SHA512: e60443f79a2e38cc78fceeff5c2956d622e8a10730129f9c27c14aef59bc6fa0894b8011e6191530443bf3165f78da978bc08ad04248ddb65e2da373264afa6a
0bd882b7405414b5355e69a9fdcd7a533e504b6b refactor: remove RecursiveMutex cs_nBlockSequenceId (Sebastian Falbesoner)
Pull request description:
The RecursiveMutex `cs_nBlockSequenceId` is only used at one place in `CChainState::ReceivedBlockTransactions()` to atomically read-and-increment the nBlockSequenceId member:
83daf47898/src/validation.cpp (L2973-L2976)
~~For this simple use-case, we can make the member `std::atomic` instead to achieve the same result (see https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith).~~
~~This is related to #19303. As suggested in the issue, I first planned to change the `RecursiveMutex` to `Mutex` (still possible if the change doesn't get Concept ACKs), but using a Mutex for this simple operation seems to be overkill. Note that at the time when this mutex was introduced (PR #3370, commit 75f51f2a63) `std::atomic` were not used in the codebase yet -- according to `git log -S std::atomic` they have first appeared in 2016 (commit 7e908c7b82), probably also because the compilers didn't support them properly earlier.~~
At this point, the cs_main lock is set, hence we can use a plain int for the member and mark it as guarded by cs_main.
ACKs for top commit:
Zero-1729:
ACK 0bd882b
promag:
Code review ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b.
hebasto:
ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b
Tree-SHA512: 435271ac8f877074099ddb31436665b500e555f7cab899e5c8414af299b154d1249996be500e8fdeff64e4639bcaf7386e12510b738ec6f20e415e7e35afaea9
8733a8e84c4b2e484f6ed6159fcf5f29a360d42e the result of CWallet::IsHDEnabled() was initialized with true. (Saibato)
Pull request description:
the result of CWallet::IsHDEnabled() was initialized with true.
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in GUi when reloading a wallet after closing.
ACKs for top commit:
achow101:
ACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
hebasto:
ACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
theStack:
utACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
meshcollider:
utACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
Tree-SHA512: 79b976594f7174d05c29fe3819037ead59aaef27498d95415ceba74d633a8e035f6b03b521000ac3370684a8cb09319d8be1a443ce2d29b3ff4089e399f6b719
fa34cb80248cc39a73fc393f65c3cfc62e849556 cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
Pull request description:
`seconds` is not enough precision to "exactly" store a timestamp n seconds into the future. Improve the precision by using `microseconds`. Fixes#22325
Also, use chrono literals.
ACKs for top commit:
jonatack:
ACK fa34cb80248cc39a73fc393f65c3cfc62e849556 review, debug-built, tested
theStack:
Tested ACK fa34cb80248cc39a73fc393f65c3cfc62e849556
Tree-SHA512: 7158da8545f9998a82bcc8636e04564efdb1e1be43b4288298c151b4df29ad47a2760259eefadd4a01db92ea18a1e017f3febc1cd8c69a4b28c86180229d8c90
faa94961d6e38392ba068381726ed4e033367b03 test: Add temporary logging to debug #20975 (MarcoFalke)
Pull request description:
to be reverted after a fix
ACKs for top commit:
laanwj:
Code review ACK faa94961d6
Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
a09e260c15 refacotor: simply RecursiveMutex -> Mutex (pasta)
Pull request description:
## What was done?
This PR is a simpler version of https://github.com/dashpay/dash/pull/5954; it is simply a recursiveMutex -> Mutex PR; as I think some of the scope minimization in the other PR introduced test failures (appears there are undocumented mutex dependancies)
## How Has This Been Tested?
CI TBD
## Breaking Changes
None
## 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:
knst:
utACK a09e260c15
Tree-SHA512: 4ef5aa94ecb1a55f30b642d5e0328e2d2a0adcf2ccc2ab6489a576a420c915339be023828785c3243f019bb73ac2cd77be7dd4397e14ec874cba68999a9e54dc
