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
7e698732836121912f179b7c743a72dd6fdffa72 sync: remove DEBUG_LOCKCONTENTION preprocessor directives (Jon Atack)
9b08006bc502e67956d6ab518388fad6397cac8d log, sync: improve lock contention logging and add time duration (Jon Atack)
3f4c6b87f1098436693c4990f2082515ec0ece26 log, timer: add timing macro in usec LOG_TIME_MICROS_WITH_CATEGORY (Jon Atack)
b7a17444e0746c562ae97b26eba431577947b06a log, sync: add LOCK logging category, apply it to lock contention (Jon Atack)
Pull request description:
To enable lock contention logging, `DEBUG_LOCKCONTENTION` has to be defined at compilation. Once built, the logging is not limited to a category and is high frequency, verbose and in all-caps. With these factors combined, it seems likely to be rarely used.
This patch:
- adds a `lock` logging category
- adds a timing macro in microseconds, `LOG_TIME_MICROS_WITH_CATEGORY`
- updates `BCLog::LogMsg()` to omit irrelevant decimals for microseconds and skip unneeded code and math
- improves the lock contention logging, drops the all-caps, and displays the duration in microseconds
- removes the conditional compilation directives
- allows lock contentions to be logged on startup with `-debug=lock` or at run time with `bitcoin-cli logging '["lock"]'`
```
$ bitcoind -signet -debug=lock
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 completed (4μs)
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 completed (4μs)
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 started
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 completed (20μs)
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 started
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 completed (3μs)
$ bitcoin-cli -signet logging
"lock": true,
$ bitcoin-cli -signet logging [] '["lock"]'
"lock": false,
$ bitcoin-cli -signet logging '["lock"]'
"lock": true,
```
I've tested this with Clang 13 and GCC 10.2.1, on Debian, with and without `--enable-debug`.
ACKs for top commit:
hebasto:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72, added a contention duration to the log message since my [previous](https://github.com/bitcoin/bitcoin/pull/22736#pullrequestreview-743764606) review.
theStack:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72 🔏⏲️
Tree-SHA512: c4b5eb88d3a2c051acaa842b3055ce30efde1f114f61da6e55fcaa27476c1c33a60bc419f7f5ccda532e1bdbe70815222ec2b2b6d9226f29c8e94e598aacfee7
adc0e4b382 fix: apply changes for .clang-format to make it matched with our code style (Konstantin Akimov)
0c884f9740 chore: narrow score of clang-diff-format for dash specific files only (Konstantin Akimov)
4bc0e1f697 chore: intentionally introducing wrong formatting to bip39.cpp to trigger CI (Konstantin Akimov)
2c74ad427d fix: adjust wallet/bip39 accordingly linter comments (Konstantin Akimov)
d3faa8522c refactor: use better masks for list of files; add missing bip39.{h,cpp} (Konstantin Akimov)
7788f1db0e refactor: move list of non backported files o test/util/data/non-backported.txt (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
**Note**: should be this PR either https://github.com/dashpay/dash/pull/5942 be merged, not both
CI clang-format triggers to non-dash files + clang format is differ from out current formatting.
## What was done?
See each commits
## How Has This Been Tested?
See CI result
To test locally how new style will look, just run this command:
```
diff -u <(cat {coinjoin,governance,llmq,evo,masternode}/*.{h,cpp}) <(clang-format-16 {coinjoin,governance,llmq,evo,masternode}/*.{h,cpp} )
```
## 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: d87f30ba78e04f886d7eb2b6b235c20a966bc4438e6428a83ecff5c795d72777516d4270eb9769ffebef9f06e9509acf3c535b4c87b1be6c8a5aef7e2b7efecb
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
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
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
6b71f274ae Merge bitcoin/bitcoin#29510: wallet: `getrawchangeaddress` and `getnewaddress` failures should not affect keypools for descriptor wallets (Ava Chow)
85fa37068f refactor: use Params().ExtCoinType() for descriptor wallets (Konstantin Akimov)
da8e5639ee fix: skip functional tests which requires BDB if no bdb (see 20267) (Konstantin Akimov)
4ba44fa3c9 fix: skip interface_zmq.py which is not ready to work without bdb (Konstantin Akimov)
45fc8a4863 fix: autobackup influences an exclusive locks made by SQLite (Konstantin Akimov)
e542cd2d34 fix: missing changes from bitcoin#21634 (Konstantin Akimov)
2de7aecf6f Merge #19502: Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (Samuel Dobson)
c172605cd7 Merge #19077: wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets (Samuel Dobson)
2439247e93 Merge bitcoin/bitcoin#23608: test: fix `feature_rbf.py --descriptors` and add to test runner (fanquake)
f6b3614754 fix: descriptor wallets follow-up to merge bitcoin#20202: Make BDB support optional (Konstantin Akimov)
a340ad641e Merge #20262: tests: Skip --descriptor tests if sqlite is not compiled (Samuel Dobson)
7d55046dfb Merge #20125: rpc, wallet: Expose database format in getwalletinfo (Samuel Dobson)
343d4b07d3 fix: descriptor wallets follow-up for bitcoin#20156: Make sqlite support optional (compile-time) (Konstantin Akimov)
fa30777494 Merge #20198: Show name, format and if uses descriptors in bitcoin-wallet tool (MarcoFalke)
14121ec5f3 Merge #18888: test: Remove RPCOverloadWrapper boilerplate (MarcoFalke)
b18351e415 Merge #20153: wallet: do not import a descriptor with hardened derivations into a watch-only wallet (Wladimir J. van der Laan)
c995e5d957 Merge #20266: wallet: fix change detection of imported internal descriptors (Wladimir J. van der Laan)
c86458250c Merge #18787: wallet: descriptor wallet release notes and cleanups (Samuel Dobson)
0949c08996 Merge #18782: wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction (Samuel Dobson)
baa6959068 Merge #18805: tests: Add missing sync_all to wallet_importdescriptors.py (MarcoFalke)
76e08f9b3d Merge #18027: "PSBT Operations" dialog (Samuel Dobson)
c1b94b6f52 fix: wallet should be unlocked before generating keys for Descriptor wallet (Konstantin Akimov)
f293c046f4 Merge #16528: Native Descriptor Wallets using DescriptorScriptPubKeyMan (Andrew Chow)
4064334732 fix: get receiving address for Descriptor Wallets (Konstantin Akimov)
bdbd0b14a7 chore: dashification of descriptor implementation in dash (UdjinM6)
b02fc0b2ce fix: counting calculation of internal keys for Descriptor Wallets (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
This PR is a batch of backports and related fixes to add a support of native descriptor wallets to Dash Core.
There're more related backports, but this PR is a minimal package of backports to get descriptor wallets working and unit/functional tests to succeed. To do: bitcoin#20226, bitcoin#21049, bitcoin#18788, bitcoin#20267, bitcoin#19230, bitcoin#19239, bitcoin#19441, bitcoin#19568, bitcoin#19979, bitcoin-core/gui#96, bitcoin#19136, bitcoin#21277, bitcoin#21063, bitcoin#21302, bitcoin#19651, bitcoin#20191, bitcoin#22446 and other.
Prior work:
- https://github.com/dashpay/dash/pull/5580
- https://github.com/dashpay/dash/pull/5807
## What was done?
backports:
- bitcoin/bitcoin#16528
- bitcoin/bitcoin#18027
- bitcoin/bitcoin#18805
- bitcoin/bitcoin#18782
- bitcoin/bitcoin#18787
- bitcoin/bitcoin#20266
- bitcoin/bitcoin#20153
- bitcoin/bitcoin#18888
- bitcoin/bitcoin#20198
- bitcoin/bitcoin#20125
- bitcoin/bitcoin#20262
- bitcoin/bitcoin#23608
- bitcoin/bitcoin#19077
- bitcoin/bitcoin#19502
- bitcoin/bitcoin#29510
and extra fixes and missing changes for bitcoin#20156, bitcoin#20202, bitcoin#20267, bitcoin#21634 + fix of auto-backup for sqlite wallets.
## How Has This Been Tested?
There're 2 new functional tests: `wallet_importdescriptors.py` and `wallet_descriptor.py`
Beside that many functional tests run twice now: using legacy wallet and descriptor wallets: `wallet_hd.py`, `wallet_basic.py`, `wallet_labels.py`, `wallet_keypool_topup.py`, `wallet_avoidreuse.py`, `rpc_psbt.py`, `wallet_keypool_hd.py`, `rpc_createmultisig.py`, `wallet_encryption.py`.
With bitcoin#18788 expected to more tests run.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
Rebase looks good; utACK 6b71f274ae
PastaPastaPasta:
utACK 6b71f274ae
UdjinM6:
utACK 6b71f27
kwvg:
utACK 6b71f274ae
Tree-SHA512: 776c5dfe1eec2b5bebc8d606476cd981c810ac81965b348e78c13e96fff23be500c495ae68c93f669403941c96eccdd3775f2b96572163c34175900e15549b5d
even it maybe useful lint message for some particular case, sometimes it asks
to make an refactoring that will be over-complex.
For example, it asks to refactor external loop to std::any_of:
```
for (const auto& inner_entry : vecEntries) {
if (ranges::any_of(inner_entry.vecTxDSIn,
[&txin](const auto& txdsin){
return txdsin.prevout == txin.prevout;
})) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: already have this txin in entries\n", __func__);
nMessageIDRet = ERR_ALREADY_HAVE;
// Two peers sent the same input? Can't really say who is the malicious one here,
// could be that someone is picking someone else's inputs randomly trying to force
// collateral consumption. Do not punish.
return false;
}
}
```
That's possible to refactor, but that's unreasonable complexity to have an
lambda inside an lambda... That's unreasonable.
Some other suggestion are also non-trivial.
One more suppression for any_of in llmq/commitment which is false-alarm:
There's used index but linter doesn't see it:
```
for (const auto i : irange::range(members.size(), size_t(llmq_params.size))) {
if (validMembers[i]) {
LogPrintfFinalCommitment("q[%s] invalid validMembers bitset. bit %d should not be set\n", quorumHash.ToString(), i);
return false;
}
if (signers[i]) {
LogPrintfFinalCommitment("q[%s] invalid signers bitset. bit %d should not be set\n", quorumHash.ToString(), i);
return false;
}
}
```
## Issue being fixed or feature implemented
On my local kubuntu linters have way too much spam
## What was done?
See each commit
## How Has This Been Tested?
Run locally. Amount of warnings decreased from thousands to fewer
amount. Excluding typos, they are:
```
src/coinjoin/client.cpp:1420:5: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/client.cpp:1426:5: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/client.cpp:655:26: warning: Consider using std::copy_if algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/server.cpp:593:33: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/server.cpp:630:106: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/governance/governance.cpp:1057:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1068:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1079:13: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1086:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1094:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1099:5: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1486:34: warning: Consider using std::copy_if algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/commitment.cpp:102:5: warning: Consider using std::all_of or std::none_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/instantsend.cpp:820:38: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/quorums.cpp:831:102: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/quorums.h:300:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:301:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:302:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:303:17: warning: C-style pointer casting [cstyleCast]
src/spork.cpp:119:58: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/statsd_client.cpp:234:63: warning: C-style pointer casting [cstyleCast]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
^---- failure generated from test/lint/lint-cppcheck-dash.sh
Consider install flake8-cached for cached flake8 results.
test/functional/data/invalid_txs.py: error: Source file found twice under different module names: "invalid_txs" and "data.invalid_txs"
test/functional/data/invalid_txs.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
test/functional/data/invalid_txs.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)
^---- failure generated from test/lint/lint-python.s
```
## 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
## Issue being fixed or feature implemented
The architecture of bitcoin assumes that there's no any external class
that processes network messages and knows anything about PeerManager
from net_processing; no any external call for PeerManager::Misbehaving
in bitcoin. All logic related to processing messages are located in
net_processing.
Dash has many many extra types of network messages and many of them
processed by external components such as llmq/signing or
coinjoin/client. Current architecture creates multiple circular
dependency.
## What was done?
That's part II of refactorings.
This PR removes PeerManager from several constructor and let LLMQContext
to forget about PeerManager.
Prior work in this PR: https://github.com/dashpay/dash/pull/5782
## What else to do?
Some network messages are processed asynchronously in external
components such as llmq/signing, llmq/instantsend,
llmq/dkgsessionhandler. It doesn't let to refactor them easily, because
they can't just simple return status of processing; status of processing
would be available sometime later and there's need callback or other way
to pass result code without spreading PeerManager over codebase.
## How Has This Been Tested?
- Run unit/functional tests
- run a linter test/lint/lint-circular-dependencies.sh
## 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
31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0 [util] add RunCommandParseJSON (Sjors Provoost)
c17f54ee535faaedf9033717403e1f775b5f1530 [ci] use boost::process (Sjors Provoost)
32128ba682033560d6eb2e4848a9f77a842016d2 [doc] include Doxygen comments for HAVE_BOOST_PROCESS (Sjors Provoost)
3c84d85f7d218fa27e9343c5cd1a55e519218980 [build] msvc: add boost::process (Sjors Provoost)
c47e4bbf0b44f2de1278f9538124ec98ee0815bb [build] make boost-process opt-in (Sjors Provoost)
929cda5470f98d1ef85c05b1cad4e2fb9227e3b0 configure: add ax_boost_process (Sjors Provoost)
8314c23d7b39fc36dde8b40b03b6efbe96f85698 [depends] boost: patch unused variable in boost_process (Sjors Provoost)
Pull request description:
Prerequisite for external signer support in #16546. Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).
This adds a new dependency [boost process](https://github.com/boostorg/process/tree/boost-1.64.0). This is part of Boost since 1.64 which is part of `depends`. Because the minimum Boost version is 1.47, this functionality is skipped for older versions of Boost.
Use `./configure --with-boost-process` to opt in, which checks for the presence of Boost::Process.
We add `UniValue runCommandParseJSON(const std::string& strCommand)` to `system.{h,cpp}` which calls an arbitrary command and processes the JSON returned by it. This is currently only called by the test suite.
~For testing purposes this adds a new regtest-only RPC method `runcommand`, as well as `test/mocks/command.py` used by functional tests.~ (this is no longer the case)
TODO:
- [ ] review boost process in #15440
ACKs for top commit:
achow101:
ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0
hebasto:
re-ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, only rebased (verified with `git range-diff`) and removed an unintentional tab character since the [previous](https://github.com/bitcoin/bitcoin/pull/15382#pullrequestreview-458371035) review.
meshcollider:
Very light utACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, although I am not very confident with build stuff.
promag:
Code review ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, don't mind the nit.
ryanofsky:
Code review ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0. I left some comments below that could be ignored or followed up later. The current change is clean and comprehensive.
Tree-SHA512: c506e747014b263606e1f538ed4624a8ad7bcf4e025cb700c12cc5739964e254dc04a2bbb848996b170e2ccec3fbfa4fe9e2b3976b191222cfb82fc3e6ab182d
BACKPORT NOTICE
fixup psbt. all missing changes belongs to src/wallet/scriptpubkeyman.h/cpp ----- they are related to descriptor wallet!
-------------------
931dd4760855e036c176a23ec2de367c460e4243 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d1b4dcc55c8826873f340ab4196af21 [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29d327d01aebb98b0504f317eb19c3dc [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa3aeaa69d8a3a716f902f450d5eaaec FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b73387600d175aff8bd5e6624dd51f86e7 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd4760855e036c176a23ec2de367c460e4243
jb55:
ACK 931dd4760855e036c176a23ec2de367c460e4243
achow101:
ACK 931dd4760855e036c176a23ec2de367c460e4243
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
BACKPORT NOTICE
There's some difference with original PR but that's not important because we do not actually use travis.
The variable TRAVIS_BRANCH would be removed anyway in bitcoin#20697 - let's just skip it for simplicity
--------------
a91ab86fae91d416d664d19d2f482a8d19c115a6 lint: Use TRAVIS_BRANCH in lint-git-commit-check.sh (Fabian Jahr)
c11dc995c98e908dfd9cea64d4b34329b1dbb5c6 lint: Don't use TRAVIS_COMMIT_RANGE in whitespace linter (Fabian Jahr)
1b41ce8f5f3debae03ca60e4acada14680df9185 lint: Don't use TRAVIS_COMMIT_RANGE for commit-script-check (Fabian Jahr)
Pull request description:
This is causing problems again, very similar to #19654.
UPDATE: This now removes all remaining usages of TRAVIS_COMMIT_RANGE and instead uses TRAVIS_BRANCH for the range, including `lint-git-commit-check` where TRAVIS_COMMIT_RANGE had already been removed. For builds triggered by a pull request, TRAVIS_BRANCH is the name of the branch targeted by the pull request. In the linters there is still a fallback that assumes master as the target branch.
ACKs for top commit:
sipa:
ACK a91ab86fae91d416d664d19d2f482a8d19c115a6. See test I tried in #20075.
Tree-SHA512: 1378bdebd5d8787a83fbda5d9999cc9447209423e7f0218fe5eb240e6a32dc1b51d1cd53b4f8cd1f71574d935ac5e22e203dfe09cce17e9976a48416038e1263
## Issue being fixed or feature implemented
`llmq/utils` has simple util code that used all over code base and also
have too heavy code for calculation quorums such as:
`GetAllQuorumMembers`, `EnsureQuorumConnections` and other.
These helpers for calculation quorums are used only by
evo/deterministicmns, evo/simplifiedmns and llmq/* modules, but
llmq/utils is included in many other modules for various trivial
helpers.
## What was done?
Prior work:
- https://github.com/dashpay/dash/pull/5753
- #5486
See also #4798
This PR remove all non-quorum calculation code from llmq/utils.
Eventually it happens that easier to take everything out rather than
move Quorum Calculation to new place atm:
- new module llmq/options have a code related to various params, command
line options, spork-related etc
- llmq/utils is not included in various files which do not use any
llmq/utils code
- helper `BuildCommitmentHash` goes to llmq/commitment
- helper `BuildSignHash` goes to llmq/signing
- helper `GetLLMQParam` inlined since it's trivial (it has not been
trivial when introduced ages ago)
- removed dependency of `IsQuorumEnabled` on CQuorumManager which means
`quorumManager` deglobalization is done for 90%
## How Has This Been Tested?
- Run unit functional tests
- updated circular dependencies
`test/lint/lint-circular-dependencies.sh`
- check that llmq/utils is not included without needs to calculate
Quorums Members
```
$ grep -r include src/ 2> /dev/null | grep -v .Po: | grep -vE 'llmq/utils.(h|cpp)': | grep llmq/utils
src/evo/mnauth.cpp:#include <llmq/utils.h>
src/evo/deterministicmns.cpp:#include <llmq/utils.h>
src/llmq/quorums.cpp:#include <llmq/utils.h>
src/llmq/blockprocessor.cpp:#include <llmq/utils.h>
src/llmq/commitment.cpp:#include <llmq/utils.h>
src/llmq/debug.cpp:#include <llmq/utils.h>
src/llmq/dkgsessionhandler.cpp:#include <llmq/utils.h>
src/llmq/dkgsession.cpp:#include <llmq/utils.h>
src/llmq/dkgsessionmgr.cpp:#include <llmq/utils.h>
src/rpc/quorums.cpp:#include <llmq/utils.h>
```
## 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
b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e swapped "is" for "==" in literal comparison (Tyler Chambers)
Pull request description:
In Python 3.8+ literal comparisons using "is" instead of "==" produce a SyntaxWarning [source](https://docs.python.org/3.8/whatsnew/3.8.html#changes-in-python-behavior).
I checked the entire devtools directory, this seems to be the only occurrence.
This is a small fix, but removes the SyntaxWarning.
Fixes: #20338
ACKs for top commit:
hebasto:
re-ACK b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e, only squashed since my [previous](https://github.com/bitcoin/bitcoin/pull/20346#pullrequestreview-525934568) review.
practicalswift:
re-ACK b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e: patch still looks correct
theStack:
utACK b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e
Tree-SHA512: 82a43495d6552fbaa3b02b58f0930b049d27aa937fe44b47714e3c059f844cc494de20674557371cbccf24fb8873ecb7376fb965ae326847eed2b855ed2d59c6
It is partial de-circularisation of dependencies between that includes net_processing
Classes that still depends on net_processing but should not:
- llmq/dkgsessionmgr
- llmq/signing
- llmq/instantsend
They have asynchronous processing and with current impl that's impossible to do
5d77549d8b287eb773db695b88c165ebe3be1005 doc: Add mypy to test dependencies (Hennadii Stepanov)
7dda912e1c28b02723c9f24fa6c4e9003d928978 test: Do not swallow flake8 exit code (Hennadii Stepanov)
Pull request description:
After #18210 the `flake8` exit code in `test/lint/lint-python.sh` just not used that makes the linter broken.
This PR:
- combines exit codes of `flake8` and `mypy` into the `test/lint/lint-python.sh` exit code
- documents `mypy` as the test dependency
ACKs for top commit:
MarcoFalke:
Approach ACK 5d77549d8b287eb773db695b88c165ebe3be1005, fine with me
practicalswift:
ACK 5d77549d8b287eb773db695b88c165ebe3be1005
Tree-SHA512: e948ba04dc4d73393967ebf3c6a26c40d428d33766382a0310fc64746cb7972e027bd62e7ea76898b742a656cf7d0fcda2fdd61560a21bfd7be249cea27f3d41
bd7e530f010d43816bb05d6f1590d1cd36cdaa2c This PR adds initial support for type hints checking in python scripts. (Kiminuo)
Pull request description:
This PR adds initial support for type hints checking in python scripts.
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
[Mypy](https://mypy.readthedocs.io/en/latest/index.html) is used in `lint-python.sh` to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
**Notes:**
* [--ignore-missing-imports](https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports) switch is passed on to `mypy` checker for now. The effect of this is that one does not need `# type: ignore` for `import zmq`. More information about import processing can be found [here](https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports). This can be changed in a follow-up PR, if it is deemed useful.
* We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change:
```python
_opcode_instances = [] # type: List[CScriptOp]
```
to
```python
_opcode_instances:List[CScriptOp] = []
```
for type hints that are **not** function parameters and function return types.
**Useful resources:**
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
ACKs for top commit:
fanquake:
ACK bd7e530f010d43816bb05d6f1590d1cd36cdaa2c - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat).
MarcoFalke:
ACK bd7e530f010d43816bb05d6f1590d1cd36cdaa2c fine with me
Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a
e8e48fa82bdce3f0c1da0693148867befa221de7 Converted lint-python-mutable-default-parameters.sh to python (TakeshiMusgrave)
Pull request description:
This converts one of the linter scripts to Python. Reference issue: https://github.com/bitcoin/bitcoin/issues/24783
The approach is to just call git grep using subprocess.run.
Alternative approaches could be to use Python instead of git grep (I'm not sure how) or use ```pylint --disable=all --enable=W0102```, though that requires installation of pylint.
ACKs for top commit:
MarcoFalke:
review ACK e8e48fa82bdce3f0c1da0693148867befa221de7
Tree-SHA512: 7f6f4887dee02c9751b225a6a131fb705868859c4a9af25bb3485cda2358650486b110f17adf89d96a20f212d7d94899922a07aab12c8dc11984cfd5feb7a076
b72925e7cea11522aca65580c136dbacb2753e83 lint: remove qt SIGNAL/SLOT lint (fanquake)
Pull request description:
I think we are past the point where we need to lint for this, the CPU
can probably be better utilized.
ACKs for top commit:
laanwj:
ACK b72925e7cea11522aca65580c136dbacb2753e83
Tree-SHA512: 3da6e4811cdd16ff64c7e26f641f7b24f0405cc86cec36666de58691d447eca8662c924df31c6c60b3523c13590bdc62205a3237b1b1794dd8cdef35519309b3
ab9c34237ab7b056394e0bd1f7cb131ffd95754c release: remove gitian (fanquake)
Pull request description:
Note that this doesn't yet touch any glibc back compat related code.
ACKs for top commit:
laanwj:
Code review ACK ab9c34237ab7b056394e0bd1f7cb131ffd95754c
Tree-SHA512: 8e2fe3ec1097f54bb11ab9136b43818d90eab5dbb0a663ad6a552966ada4bdb49cc12ff4e66f0ec0ec5400bda5c81f3a3ce70a9ebb6fe1e0db612da9f00a51a7
34c80d9eee7d21755f2bb80f7c97fd30d2c7b656 test: Add option to git-subtree-check to do full check, add help (Wladimir J. van der Laan)
Pull request description:
This adds a brief help text to `git-subtree-check.sh` and adds an option to do a full remote check instead of having two different code paths with a successful exit status. Also make it explicit that the CI is not doing this.
ACKs for top commit:
fjahr:
tested ACK 34c80d9eee7d21755f2bb80f7c97fd30d2c7b656
Tree-SHA512: 20f672fd3b3c1d633eccf9998fdd738194cdd7d10cc206691f2dcc28bbbf8187b8d06b87814f875a06145b179f5ca1f4f4f9922972be72759cf5ac6e0c11abd1
a4a3fc4cd2e6f53cdffcc2962fd152a4e40c7413 doc: improve subtree check instructions (Sjors Provoost)
Pull request description:
Running `git-subtree-check.sh` requires adding the subtree repository as a remote. I learned that several years ago and then forgot again.
This PR also improves the error message if the subtree commit can't be found.
ACKs for top commit:
laanwj:
ACK a4a3fc4cd2e6f53cdffcc2962fd152a4e40c7413
fanquake:
ACK a4a3fc4cd2e6f53cdffcc2962fd152a4e40c7413 - this looks ok.
Tree-SHA512: 959bd923726c172d17f9f97f8a56988bf2df5a94d3131e5152a66150b941394cee9e82fdc6b86e09c0ba91d123a496599f07ca454212168d8d301738394c12c8
487aff421 Check subtree consistency in Travis (Pieter Wuille)
e1d0cc23a Improve git-subtree-check.sh (Pieter Wuille)
Pull request description:
Apparently many of our subtrees get modified by PRs in this repository, without getting noticed.
To improve upon this:
* Make git-subtree-check.sh capable of doing a weaker consistency check (that doesn't need access to external repositories), but which should be sufficient to detect unintended changes. It can be fooled by a fake subtree merge commit, but that would hopefully be obvious to reviewers.
* Make Travis invoke this subtree check for each of our subtrees.
Note that Travis is currently expected to fail on this PR, as 2 out of 4 subtrees (`src/secp156k1` and `src/univalue` have been modified directly in master).
Tree-SHA512: 465b680392d3daf38a8c1dda77d6f74b1d1c23324c378774777fb95aa673e119a8f7e3ccc124e41d97b5ac8975f3d79f3015797d2d309666582394364917ec4e
aaaaad6ac95b402fe18d019d67897ced6b316ee0 scripted-diff: Bump copyright of files changed in 2019 (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0
promag:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0 🎉
fanquake:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0 - going to merge this now because the year is over and conflicts are minimal.
Tree-SHA512: 58cb1f53bc4c1395b2766f36fabc7e2332e213780a802762fff0afd59468dad0c3265f553714d761c7a2c44ff90f7dc250f04458f4b2eb8eef8b94f8c9891321
f685a13bef0418663015ea6d8f448f075510c0ec doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f0882a1a2bb20474648419979af6e383d refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef0418663015ea6d8f448f075510c0ec
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef0418663015ea6d8f448f075510c0ec
LarryRuane:
Code review, test ACK f685a13bef0418663015ea6d8f448f075510c0ec
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
## Issue being fixed or feature implemented
Some headers include other heavy headers, such as `logging.h`,
`tinyformat.h`, `iostream`. These headers are heavy and increase
compilation time on scale of whole project drastically because can be
used in many other headers.
## What was done?
Moved many heavy includes from headers to cpp files to optimize
compilation time.
In some places added forward declarations if it is reasonable.
As side effect removed 2 circular dependencies:
```
"llmq/debug -> llmq/dkgsessionhandler -> llmq/debug"
"llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug"
```
## How Has This Been Tested?
Run build 2 times before refactoring and after refactoring: `make clean
&& sleep 10s; time make -j18`
Before refactoring:
```
real 5m37,826s
user 77m12,075s
sys 6m20,547s
real 5m32,626s
user 76m51,143s
sys 6m24,511s
```
After refactoring:
```
real 5m18,509s
user 73m32,133s
sys 6m21,590s
real 5m14,466s
user 73m20,942s
sys 6m17,868s
```
~5% of improvement for compilation time. That's not huge, but that's
worth to get merged
There're several more refactorings TODO but better to do them later by
backports:
- bitcoin/bitcoin#27636
- bitcoin/bitcoin#26286
- bitcoin/bitcoin#27238
- and maybe this one: bitcoin/bitcoin#28200
## 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
## Issue being fixed or feature implemented
`ConnectBlock` can fail after `ProcessSpecialTxsInBlock`, we shouldn't
be notifying too early. Same for `DisconnectBlock` but that's less of an
issue imo.
## What was done?
Move notifications to the end of `ConnectBlock`/`DisconnectBlock`. There
is no `connman` in `CChainState` and I don't want to pass it in updates
struct so I changed `NotifyMasternodeListChanged` and used `connman`
from `CDSNotificationInterface` instead.
## How Has This Been Tested?
run unit test, run testnet qt wallet
## Breaking Changes
## 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)_
