CGovernanceManager::IsValid() returns true only if its db is successfully
initialized. If we attempt to initialize it and fail, init logic will
report to us that it failed. If we don't attempt to initialize it at all,
it will remain false.
Since fDisableGovernance is the same as not initializing it at all and
the other case where IsValid() is false is dealt with in init, we can
use IsValid() to infer if governance is enabled.
CActiveMasternodeManager no longer exists as a global variable, it is a
conditionally initialized smart pointer. If it exists, then it's in masternode
mode, no need to check if we're in masternode mode anymore.
7cc77f3a30 Merge #21373: test: generate fewer blocks in feature_nulldummy to fix timeouts, speed up (MarcoFalke)
a933a60b1a feat: new command line argument -bip147height for bitcoin#21373 (Konstantin Akimov)
51911388f2 Merge #21377: Speedy trial support for versionbits (fanquake)
ecade9bc39 Merge bitcoin/bitcoin#21749: test: Bump shellcheck version (W. J. van der Laan)
eeec2f2799 Merge bitcoin/bitcoin#21884: fuzz: Remove unused --enable-danger-fuzz-link-all option (fanquake)
51633d70ea Merge bitcoin/bitcoin#21874: fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT (MarcoFalke)
a02a2c0322 Merge bitcoin/bitcoin#21681: validation: fix ActivateSnapshot to use hardcoded nChainTx (MarcoFalke)
71f23d6e33 Merge bitcoin/bitcoin#21814: test: Fix feature_config_args.py intermittent issue (MarcoFalke)
de4d2a839d Merge bitcoin/bitcoin#21714: refactor: Drop CCoinControl::SetNull (MarcoFalke)
a63f9c31cc Merge bitcoin-core/gui#284: refactor: Simplify SendCoinsDialog::updateCoinControlState (Hennadii Stepanov)
b2d889380c Merge bitcoin/bitcoin#21792: test: Fix intermittent issue in p2p_segwit.py (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular batch of backports from bitcoin v22
## What was done?
Implemented new commandline argument `-bip147height` for RegTest.
Backports:
- bitcoin/bitcoin#21377
- bitcoin/bitcoin#21792
- bitcoin-core/gui#284
- bitcoin/bitcoin#21714
- bitcoin/bitcoin#21373
- bitcoin/bitcoin#21814
- bitcoin/bitcoin#21681
- bitcoin/bitcoin#21874
- bitcoin/bitcoin#21884
- bitcoin/bitcoin#21749
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 7cc77f3a30
Tree-SHA512: d46218667430af19c445d67d6411b1e7f19920e85f86e6e74ae7b9062aeb3763637a8613587c203ba8d285ccc3ee755f936141010944cfae8627397e8b8584d3
f2fe39fadc trivial: add `Asserts` for `m_peerman` pointer container uses (Kittywhiskers Van Gogh)
35be4e2ebe llmq: pass PeerManager to llmq::CInstantSendManager constructor (Kittywhiskers Van Gogh)
bfd33cd2b4 net: move CConnman::RelayInv{Filtered} into PeerManager (Kittywhiskers Van Gogh)
313a7e9a50 trivial: cleanup unnecessary headers in context files (Kittywhiskers Van Gogh)
c3f1ac2291 net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction (Kittywhiskers Van Gogh)
0323c6ca17 net: move Relay{Inv, InvFiltered, Transaction} out of CConnman (Kittywhiskers Van Gogh)
d54ba447a7 net: use ForEachNode instead of manually iterating through vNodes (Kittywhiskers Van Gogh)
Pull request description:
## Additional information
* Dependency for https://github.com/dashpay/dash/pull/5982
* Unfortunately, we are stuck with using the `unique_ptr` const-ref of `PeerManager` for initializing {`CJ`, `LLMQ`}`Context` due to some unit tests depending on Dash-specific entities but based on a testing setup that doesn't initialize `PeerManager` (specifically `validation_chainstatemanager_tests`, which uses `ChainTestingSetup`, as opposed to `TestingSetup`, which initializes `PeerManager`).
Attempting to dereference it earlier will result in crashing (see CI pipeline [here](https://gitlab.com/dashpay/dash/-/pipelines/1245924304))
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
re-utACK [f2fe39f](f2fe39fadc)
knst:
utACK [f2fe39f](f2fe39fadc)
Tree-SHA512: 5e9ca47ba9f7f0c5fdc93bf6cfd99d91be6d134610d2bd3dccde17443a956c7d8848b3406082ee8b6ee3fd25f586ce7bac45bc89b568424479b8fc149172ae51
f5f8bd9e9a doc: replace gfd() function to recommendation to use `git diff-range` (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
`gfd` is less convenient to use, `git range-diff` is more functional. Dropped recommendation to use it.
## What was done?
Updated `CONTRIBUTION.md` file
## How Has This Been Tested?
Run `git range-diff` to ensure that it indeed works
## 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
- [x] 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: 08663e5371753b6851b216da66ef72bb6bfb80acd42c31c57de9f8d52f00766e3121adf960e27236451d6520088f0c2c927f5f4f3001a40c591feb62074951e0
Required to avoid crashes when calling RelayInvFiltered in situations
where the PeerManager* atomic hasn't been set (possible when ProcessMessage
isn't called, leaving the value unset, while a separate thread traverses
the ProcessPendingInstantSendLocks > ProcessPendingInstantSendLocks[1] >
ProcessInstantSendLock > RelayInvFiltered call chain).
[1] - There is a function with the exact same name but with multiple
arguments
ccd976dd3dbb8f991dc1203ada2043f1736be5a4 test: use 327 fewer blocks in feature_nulldummy (Jon Atack)
68c280f19732fb96bc29113ce9c8007d0101868c test, refactor: abstract the feature_nulldummy blockheight values (Jon Atack)
Pull request description:
The resolved timeout issue seen in the CI can be reproduced locally by running `test/functional/feature_nulldummy.py --valgrind --loglevel=debug`
Speeds up the normal test runtime for me from 3.8 to 2.2 seconds (debug build). Thanks to Marco Falke for the approach suggestion.
ACKs for top commit:
AnthonyRonning:
reACK ccd976dd3dbb8f991dc1203ada2043f1736be5a4 - ran a few times with the rest of the tests and still passing for me with just the fewer block change.
MarcoFalke:
review ACK ccd976dd3dbb8f991dc1203ada2043f1736be5a4 🏝
Tree-SHA512: 38339dca4276d1972e3a5a5ee436da64e9e58fd3b50b186e34b07ade9523ac4c03f6c3869c5f2a59c23b43c44f87e712f8297a01a8d83202049c081e3eeb4445
ffe33dfbd4c3b11e3475b022b6c1dd077613de79 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2c2ce5fea84cf8681013f85a444e7ea versionbits: simplify state transitions (Anthony Towns)
55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48040dc7eae46bc7941db48d98a669fd fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c1702c8ae259610fe55050ba212e21e tests: test versionbits delayed activation (Anthony Towns)
73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa205eee5c3b99343988adcb8d320460 tests: clean up versionbits test (Anthony Towns)
593274445004506c921d5d851361aefb3434d744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a4760c0c0f784029849cb5d21ee088abb tests: pull ComputeBlockVersion test into its own function (Anthony Towns)
Pull request description:
BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
Edge cases are tested by fuzzing added in #21380.
ACKs for top commit:
instagibbs:
tACK ffe33dfbd4
jnewbery:
utACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
MarcoFalke:
review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
achow101:
re-ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
gmaxwell:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
benthecarman:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
Sjors:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
jonatack:
Initial approach ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
ariard:
Code Review ACK ffe33df
Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
08f3dbb1b0cd5ca01d87e488a2fa905adf7df057 test: Bump shellcheck version (Hennadii Stepanov)
Pull request description:
The changelog for v0.7.2 is available [here](https://github.com/koalaman/shellcheck/blob/v0.7.2/CHANGELOG.md).
Only [SC2268](https://github.com/koalaman/shellcheck/wiki/SC2268) requires to update our code.
ACKs for top commit:
jarolrod:
ACK 08f3dbb1b0cd5ca01d87e488a2fa905adf7df057
Tree-SHA512: 4585cd1f4d9def2fbaafe5a2a57761288d432781eb8c6c6d37064727d7ca8fc3f35c552e6a2ffdf0820d753d4bde2c8e43e5f3f57d242f5f57591a9b1b03558d
fa27d6d3ac065684a1219e9a948514d27929cf7c fuzz: Remove unused --enable-danger-fuzz-link-all option (MarcoFalke)
Pull request description:
Remove the unused build option, which was *dangerous* (as the name implies). Also remove the fuzzbuzz config, which was never used as part of this repo and seems redundant now that we integrate with oss-fuzz.
ACKs for top commit:
practicalswift:
cr ACK fa27d6d3ac065684a1219e9a948514d27929cf7c: patch looks correct and rationale makes sense
hebasto:
ACK fa27d6d3ac065684a1219e9a948514d27929cf7c, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 9bd65ed6a76d13d8d9c7a88aaae30f701215d5d0619693a3115d5ec350808aaf6a1aa4737466a5b96f3948513ec4d063808fe16219818366720e247880a15177
fa5cb6b268554fe0f272833794a98106872ac6a5 fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT (MarcoFalke)
Pull request description:
This is needed when stdout is polluted by the fuzz engine. stderr can't be used instead because it is polluted by aborting the program.
Top commit has no ACKs.
Tree-SHA512: bf0a2a6bcd964ff1f0f3ef6e7e297b4c780430c4d6312332ed99ace0e1c58243c1483fd387e39405837d39b36072dfeb9ae03d2a7aa728ad6955159754fd5766
91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672 validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b24a89aba884cb18c13fa67ccca339ee8c validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)
Pull request description:
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
See: https://github.com/bitcoin/bitcoin/pull/19806#discussion_r612165410
ACKs for top commit:
Sjors:
utACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672
ryanofsky:
Code review ACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
5f438d66c1fbc0e524d12fef233f2ed2952e6f17 refactor, qt: Simplify SendCoinsDialog::updateCoinControlState (João Barbosa)
Pull request description:
This PR doesn't change behaviour, removes the coin control argument from `updateCoinControlState` since it's a class member.
ACKs for top commit:
hebasto:
ACK 5f438d66c1fbc0e524d12fef233f2ed2952e6f17, I have reviewed the code and it looks OK, I agree it can be merged.
jonatack:
Code review ACK 5f438d66c1fbc0e524d12fef233f2ed2952e6f17
kristapsk:
utACK 5f438d66c1fbc0e524d12fef233f2ed2952e6f17. Code looks correct.
Tree-SHA512: 14abaa3d561f8c8854fed989b6aca886dcca42135880bac76070043f61c0042ec8967f2b83e50bbbb82050ef0f074209e97fa300cb4dc51ee182316e0846506d
18169f4957 Merge #20786: net: [refactor] Prefer integral types in CNodeStats (MarcoFalke)
751c9e66c4 Merge bitcoin/bitcoin#21719: refactor: Add and use EnsureConnman in rpc code (MarcoFalke)
74b20eb309 Merge bitcoin/bitcoin#22013: net: ignore block-relay-only peers when skipping DNS seed (fanquake)
366ca98b39 Merge bitcoin/bitcoin#22144: Randomize message processing peer order (fanquake)
4d20cb7173 Merge #20373: refactor, net: Increase CNode data member encapsulation (MarcoFalke)
b8b3c4c289 Merge bitcoin-core/gui#163: Peer details: replace Direction with Connection Type (MarcoFalke)
00b828c0f5 Merge bitcoin/bitcoin#21939: refactor: Replace memset calls with array initialization (W. J. van der Laan)
7bcc56c9b6 Merge bitcoin/bitcoin#22138: script: fix spelling linter raising spuriously on "invokable" (MarcoFalke)
Pull request description:
bitcoin backports
Top commit has no ACKs.
Tree-SHA512: 28dd3c672be847a376aaf1683a665b658b132364b4903fa360810dead7f30bd6496c231b6496e55572e09f1908febdba385d863e5e1d714cb784cc4350126be9
faecb74562d012a336837d3b39572c235ad2eb9d Expose integral m_conn_type in CNodeStats, remove m_conn_type_string (Jon Atack)
Pull request description:
Currently, strings are stored for what are actually integral (strong) enum types. This is fine, because the strings are only used as-is for the debug log and RPC. However, it complicates using them in the GUI. User facing strings in the GUI should be translated and only string literals can be picked up for translation, not runtime `std::string`s.
Fix that by removing the `std::string` members and replace them by strong enum integral types.
ACKs for top commit:
jonatack:
Code review ACK faecb74562d012a336837d3b39572c235ad2eb9d
theStack:
Code review ACK faecb74562d012a336837d3b39572c235ad2eb9d 🌲
Tree-SHA512: 24df2bd0645432060e393eb44b8abaf20fe296457d07a867b0e735c3e2e75af7b03fc6bfeca734ec33ab816a7c8e1f8591a5ec342f3afe3098a4e41f5c2cfebb
fafb68add5e16e8bd5b9428bcffcaee2639747cf refactor: Add and use EnsureConnman in rpc code (MarcoFalke)
faabeb854a6e46b46e4f26b22dc2c81e68e2d863 refactor: Mark member functions const (MarcoFalke)
Pull request description:
This removes the 10 occurrences of `throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");` and replaces them with `EnsureConnman`.
ACKs for top commit:
jarolrod:
re-ACK fafb68add5e16e8bd5b9428bcffcaee2639747cf
theStack:
ACK fafb68add5e16e8bd5b9428bcffcaee2639747cf
ryanofsky:
Code review ACK fafb68add5e16e8bd5b9428bcffcaee2639747cf
Tree-SHA512: 84c63cfe31e548645d906f7191a3526c7bea99ed0d54c2a75c2041452a44fe149ede343d8e1943b0e7770816c828bb047dfec8bc541a1f2b89920a126ee54d68
fe3d17df04decc4e856121eb311636977d60f80f net: ignore block-relay-only peers when skipping DNS seed (Anthony Towns)
Pull request description:
Since #17428 bitcoind will attempt to reconnect to two block-relay-only anchors before doing any other outbound connections. When determining whether to use DNS seeds, it will currently see these two peers and decide "we're connected to the p2p network, so no need to lookup DNS" -- but block-relay-only peers don't do address relay, so if your address book is full of invalid addresses (apart from your anchors) this behaviour will prevent you from recovering from that situation.
This patch changes it so that it only skips use of DNS seeds when there are two full-outbound peers, not just block-relay-only peers.
ACKs for top commit:
Sjors:
utACK fe3d17d
amitiuttarwar:
ACK fe3d17df04decc4e856121eb311636977d60f80f, this impacts the very common case where we stop/start a node, persisting anchors & have a non-empty addrman (although, to be clear, wouldn't be particularly problematic in the common cases where the addrman has valid addresses)
mzumsande:
ACK fe3d17df04decc4e856121eb311636977d60f80f
jonatack:
ACK fe3d17df04decc4e856121eb311636977d60f80f
prayank23:
tACK fe3d17df04
Tree-SHA512: 9814b0d84321d7f45b5013eb40c420a0dd93bf9430f5ef12dce50d1912a18d5de2070d890a8c6fe737a3329b31059b823bc660b432d5ba21f02881dc1d951e94
79c02c88b347f1408a2db307db2654917f9b0bcc Randomize message processing peer order (Pieter Wuille)
Pull request description:
Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the `vNodes` vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a "first-served-by-loop-first" basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays.
As there isn't anything particularly optimal about the current ordering, just make it unpredictable by randomizing.
Reported by Crypt-iQ.
ACKs for top commit:
jnewbery:
ACK 79c02c88b3
Crypt-iQ:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
sdaftuar:
utACK 79c02c88b347f1408a2db307db2654917f9b0bcc
achow101:
Code Review ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
jamesob:
crACK 79c02c88b3
jonatack:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
vasild:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
theStack:
ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
Tree-SHA512: 9a87c4dcad47c2d61b76c4f37f59674876b78f33f45943089bf159902a23e12de7a5feae1a73b17cbc3f2e37c980ecf0f7fd86af9e6fa3a68099537a3c82c106
3642b2ed34e6609e8de558b352516daadb12cac1 refactor, net: Increase CNode data member encapsulation (Hennadii Stepanov)
acebb79d3f45eb18f820ca5bbc1e16e80fac55f1 refactor, move-only: Relocate CNode private members (Hennadii Stepanov)
Pull request description:
All protected `CNode` data members could be private.
ACKs for top commit:
jnewbery:
utACK 3642b2ed34e6609e8de558b352516daadb12cac1
MarcoFalke:
review ACK 3642b2ed34e6609e8de558b352516daadb12cac1 🏛
Tree-SHA512: 8435e3c43c3b7a3107d58cb809b8b5e1a1c0068677e249bdf0fc6ed24140ac4fc4efe2a280a1ee86df180d738c0c9e10772308690607954db6713000cf6e728d
06ba9b300866f33e21512af9d7d2891ee1501bf4 rpc: move getpeerinfo connection_type help to correct place (Jon Atack)
c95fe6e38f542f9fe8ddb1522b5ff1cac17db4bf gui: improve connection type tooltip (Hennadii Stepanov)
2c19ba2e1d26e2077da8b469f44588c20af87e23 gui: replace Direction with Connection Type in peer details (Jon Atack)
7e2beab2d28d8ab039d9554744a11592a7d7dc76 gui: create GUIUtil::ConnectionTypeToQString utility function (Jon Atack)
Pull request description:
Closes#159.
ACKs for top commit:
hebasto:
re-ACK 06ba9b300866f33e21512af9d7d2891ee1501bf4, the tooltip content is organized as unordered list.
jarolrod:
re-ACK 06ba9b300866f33e21512af9d7d2891ee1501bf4
Tree-SHA512: 24f46494ceb42ed308e4a4f2a543dbc4f4e6409a6f738c145a9f16e175bf69d411cbc944a4fd969f1829d57644dfbc194182fa8d4e9e6bce82acbeca8c673748
8050eb43bf15501e33ec5312918d926e47e4fc8d script: fix spelling linter raising spuriously on "invokable" (Jon Atack)
Pull request description:
"invokable" is a valid word that means to be callable, but the linter is raising on it:
```
$ test/lint/lint-spelling.sh
contrib/guix/guix-attest:18: invokable ==> invocable
contrib/guix/guix-clean:18: invokable ==> invocable
contrib/guix/guix-verify:18: invokable ==> invocable
^ 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
```
ACKs for top commit:
MarcoFalke:
cr ACK 8050eb43bf15501e33ec5312918d926e47e4fc8d
Tree-SHA512: 43f8dc7b7adb00ae563ccfe04a64a7ceb50237f24ff87209062bf57b2564b4d38a409df80e0183aa4f40ab306b5e07d7a5fad1600d41705bd3c443ed66a6d1c1
740d25c062 merge bitcoin#24406: Fix Wambiguous-reversed-operator compiler warnings (Kittywhiskers Van Gogh)
3265b54a2f merge bitcoin#24624: Avoid potential -Wdeprecated-enum-enum-conversion warnings (Kittywhiskers Van Gogh)
65585e68e6 merge bitcoin#25550: remove note on arm cross-compilation from build-unix.md (Kittywhiskers Van Gogh)
fbc6bc88cf init: use local ArgsManager variable instead of global when possible (Kittywhiskers Van Gogh)
9ae39f9ba7 qt: drop leftover `boost::function` usage in Qt, drop header from list (Kittywhiskers Van Gogh)
c47e9e78ed bench: drop leftover `boost::lexical_cast` benches, drop header from list (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
When building Dash with `clang-16`, I use the following `CXXFLAGS`:
```
-Werror -Werror=reorder -Werror=thread-safety -Wno-unused-command-line-argument -Wno-deprecated-builtins -Wno-deprecated-volatile -Wno-ambiguous-reversed-operator -Wno-deprecated-enum-enum-conversion
```
The explanations of the `-Wno*` flags are as follows:
| Argument | Reason |
| -------------------------------------- | ------------------------------------------------------------ |
| `-Wno-unused-command-line-argument` | Lingering use of `-static-libstdc++`, a GCC-ism that isn't recognized as a valid argument in Clang (no longer an issue thanks to https://github.com/dashpay/dash/pull/5958, thanks knst!) |
| `-Wno-deprecated-builtins` | Due to usage of [deprecated builtins](2dacfb08bd/src/bench/nanobench.h (L104-L110)) in `nanobench` that exist even in the [most current version](a5a50c2b33/src/include/nanobench.h (L115-L121)) (see below) |
| `-Wno-deprecated-volatile` | The detection code for C++20 in the CI image (based on Ubuntu `focal`) relies on deprecated volatiles (see below) |
| `-Wno-ambiguous-reversed-operator` | Due to an ambiguous operator in a Dash unit test ([source](2dacfb08bd/src/test/fuzz/addrman.cpp (L145))) (see below) |
| `-Wno-deprecated-enum-enum-conversion` | Due to key combination code in Dash Qt ([source](2dacfb08bd/src/qt/bitcoingui.cpp (L376))) (see below) |
<details>
<summary>-Wno-deprecated-builtins</summary>
```
> make
Making all in src
make[1]: Entering directory '/src/dash/src'
make[2]: Entering directory '/src/dash/src'
make[3]: Entering directory '/src/dash'
make[3]: Leaving directory '/src/dash'
CXX bench/bench_dash-addrman.o
In file included from bench/addrman.cpp:6:
In file included from ./bench/bench.h:16:
./bench/nanobench.h:952:13: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins]
return !ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(Decayed) || sizeof(Decayed) > sizeof(long) || std::is_pointer<Decayed>::value;
^
./bench/nanobench.h:107:57: note: expanded from macro 'ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE'
# define ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__)
^
1 error generated.
make[2]: *** [Makefile:14567: bench/bench_dash-addrman.o] Error 1
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18568: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:800: all-recursive] Error 1
```
</details>
<details>
<summary>-Wno-deprecated-volatile</summary>
```
> ./configure [...]
[...]
checking whether clang++-16 supports C++20 features with -h std=c++20... no
checking whether clang++-16 supports C++20 features with -std=c++2a... no
checking whether clang++-16 supports C++20 features with +std=c++2a... no
checking whether clang++-16 supports C++20 features with -h std=c++2a... no
configure: error: *** A compiler with support for C++20 language features is required.
> cat config.log
This file contains any messages produced by compilers while
running configure, to aid debugging if configure makes a mistake.
It was created by Dash Core configure 21.0.0, which was
generated by GNU Autoconf 2.69. Invocation command line was
$ ./configure --prefix=/src/dash/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --disable-crash-hooks --disable-stacktraces --enable-c++20 --enable-suppress-external-warnings --enable-debug --disable-ccache --with-sanitizers=thread
[...]
configure:4450: checking whether clang++-16 accepts -g
configure:4470: clang++-16 -c -g -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5
configure:4470: $? = 0
configure:4511: result: yes
configure:4537: checking whether make supports the include directive
configure:4552: make -f confmf.GNU && cat confinc.out
this is the am__doit target
configure:4555: $? = 0
configure:4574: result: yes (GNU style)
configure:4599: checking dependency style of clang++-16
configure:4710: result: gcc3
configure:4756: checking whether clang++-16 supports C++20 features with -std=c++20
configure:5558: clang++-16 -std=c++20 -c -pipe -O2 -Werror -Werror=reorder -Werror=thread-safety -Wno-deprecated-builtins -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5
conftest.cpp:109:36: error: volatile-qualified parameter type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
test(const int c, volatile int v) // 'volatile is deprecated in C++20'
^
1 error generated.
```
</details>
<details>
<summary>-Wno-ambiguous-reversed-operator</summary>
```
> make
Making all in src
make[1]: Entering directory '/src/dash/src'
make[2]: Entering directory '/src/dash/src'
make[3]: Entering directory '/src/dash'
make[3]: Leaving directory '/src/dash'
[...]
CXX test/fuzz/fuzz-addrman.o
test/fuzz/addrman.cpp:329:22: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'CAddrManDeterministic' and 'CAddrManDeterministic') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator]
assert(addr_man1 == addr_man2);
~~~~~~~~~ ^ ~~~~~~~~~
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
test/fuzz/addrman.cpp:145:10: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
bool operator==(const CAddrManDeterministic& other)
^
test/fuzz/addrman.cpp:145:10: note: mark 'operator==' as const or add a matching 'operator!=' to resolve the ambiguity
1 error generated.
make[2]: *** [Makefile:15393: test/fuzz/fuzz-addrman.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[3]: Leaving directory '/src/dash/src/dashbls'
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18568: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:800: all-recursive] Error 1
```
</details>
<details>
<summary>-Wno-deprecated-enum-enum-conversion</summary>
```
> make
Making all in src
make[1]: Entering directory '/src/dash/src'
make[2]: Entering directory '/src/dash/src'
make[3]: Entering directory '/src/dash'
make[3]: Leaving directory '/src/dash'
[...]
CXX qt/libbitcoinqt_a-bitcoingui.o
qt/bitcoingui.cpp:376:51: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
quitAction->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_Q));
~~~~~~~~ ^ ~~~~~~~~~
qt/bitcoingui.cpp:609:56: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
minimize_action->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_M));
~~~~~~~~ ^ ~~~~~~~~~
2 errors generated.
make[2]: *** [Makefile:12803: qt/libbitcoinqt_a-bitcoingui.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18568: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:800: all-recursive] Error 1
```
</details>
This pull request removes the need for the last two `-Wno*` flags, removes some leftover `boost::`{`function`, `lexical_cast`} usage and some `gArgs` usage where a local variable would be more applicable.
Additionally, in some builds, there is an deprecation warning (`-Wdeprecation`) for using `[=]` and relying on its implicit capture of `this` (see below), this can be seen during GCC builds but not Clang builds and correspond to a proposal to the C++20 standard ([proposal](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0806r2.html)).
It has also been mentioned at https://en.cppreference.com/w/cpp/language/lambda, "The implicit capture of `*this` when the capture default is `=` is deprecated. _(since C++20)_". This has also been remedied as part of this PR.
<details>
<summary>-Wdeprecated</summary>
```
> make -j10
[...]
CXX qt/libbitcoinqt_a-trafficgraphwidget.o
qt/optionsdialog.cpp: In lambda function:
qt/optionsdialog.cpp:195:63: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated]
195 | connect(appearance, &AppearanceWidget::appearanceChanged, [=](){
| ^
qt/optionsdialog.cpp:195:63: note: add explicit 'this' or '*this' capture
qt/optionsdialog.cpp: In lambda function:
qt/optionsdialog.cpp:277:55: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated]
277 | connect(ui->coinJoinEnabled, &QCheckBox::clicked, [=](bool fChecked) {
| ^
qt/optionsdialog.cpp:277:55: note: add explicit 'this' or '*this' capture
qt/optionsdialog.cpp: In lambda function:
qt/optionsdialog.cpp:293:45: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated]
293 | connect(this, &OptionsDialog::rejected, [=]() {
| ^
qt/optionsdialog.cpp:293:45: note: add explicit 'this' or '*this' capture
CXX qt/libbitcoinqt_a-utilitydialog.o
CXX qt/libbitcoinqt_a-addressbookpage.o
[...]
```
</details>
## 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
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 740d25c062
knst:
utACK 740d25c062
Tree-SHA512: 52793f9862192ccb556debcac2ec766c4f86e69ec4bedc2fdbaaff965ab4ac478b65b3983b1cb3a92610db85aee6f8668b0ded3494c617c5af7d8801284e461b
We don't touch `CleanupBlockRevFiles` as the function is moved to
blockstorage in bitcoin#21727, where it would need access to the global.
GetBlocksDirPath() is non-const and invocations of that aren't included
either.
`boost::lexical_cast` isn't used anywhere in Dash Core, the sole remaining
use being in a benchmark, despite it no longer being used in Dash Core.
Let's drop the benchmark and drop `boost/lexical_cast.hpp` from allowed
Boost headers
10a006e626 test: disable ipv6 tests for now (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Our CI nodes on Amazon have issues running tests using ipv6.
## What was done?
Pretend we don't have ipv6 when we run tests for now
## How Has This Been Tested?
See CI results for this PR.
## Breaking Changes
n/a but we'll have ipv6 not being tested for some time.
## 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:
kwvg:
ACK 10a006e626
PastaPastaPasta:
utACK 10a006e
Tree-SHA512: 75da180b916a5a99b1363f152494cbdd1031c7daf2d5eb370ca74547dee694ad04db3e9c677f0f738b9dbb15760d685a6b43b032dde642a4c1d49fff671e2aac
