8f06ac9dfa Merge bitcoin/bitcoin#22172: doc: update tor.md, release notes with removal of tor v2 support (W. J. van der Laan)
9b22501a4d Merge bitcoin/bitcoin#22122: ci: Bump macOS image to big-sur-xcode-12.5 (MarcoFalke)
3b05a99b50 Merge bitcoin/bitcoin#22106: refactor: address ProcessNewBlock comments from #21713 (fanquake)
c8725560c9 Merge bitcoin/bitcoin#21856: doc: add OSS-Fuzz section to fuzzing.md doc (MarcoFalke)
facf685285 Merge bitcoin/bitcoin#22261: [p2p/mempool] Two small fixes to node broadcast logic (fanquake)
1430897fc4 Merge bitcoin/bitcoin#22445: fuzz: Move implementations of non-template fuzz helpers from util.h to util.cpp (MarcoFalke)
f0c62d50a5 Merge bitcoin/bitcoin#22447: test: whitelist rpc_rawtransaction peers to speed up tests (fanquake)
b609514142 Merge #22381: guix: Test security-check sanity before performing them (Carl Dong)
9ef68d1905 Merge bitcoin/bitcoin#22061: ci: Bump multiprocess memory (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## What was done?
See commits
## How Has This Been Tested?
Run unit and 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:
UdjinM6:
utACK 8f06ac9dfa
PastaPastaPasta:
utACK 8f06ac9dfa
Tree-SHA512: f800b7ca8d357f2d02ce5cb1fc4951c2765242676c5494efd5e22e8f6d41d889e1fa2f888930f72aded75813413c6488d8a7d96baa2cf4820e3461464708658e
6431f71b3a Merge bitcoin-core/gui#393: Fix regression in "Encrypt Wallet" menu item (Hennadii Stepanov)
fc900a8aea Merge bitcoin-core/gui#333: refactor: Signal-slot connections cleanup (Hennadii Stepanov)
9ca2aad0b3 Merge bitcoin-core/gui#164: Handle peer addition/removal in a right way (Hennadii Stepanov)
7d9ce32562 Merge bitcoin-core/gui#29: refactor: Optimize signal-slot connections logic (Hennadii Stepanov)
3be79a9ed9 Merge bitcoin-core/gui#256: Save/restore column sizes of the tables in the Peers tab (Hennadii Stepanov)
f4fccd31cb Merge bitcoin-core/gui#329: Make console buttons look clickable (Hennadii Stepanov)
5a0d524506 Merge bitcoin-core/gui#123: rpc: Do not accept command while executing another one (Hennadii Stepanov)
19310646e0 Merge bitcoin-core/gui#331: Make RPC console welcome message translation-friendly (Hennadii Stepanov)
69a1305978 Merge bitcoin-core/gui#309: Add access to the Peers tab from the network icon (Hennadii Stepanov)
c858325d40 Merge bitcoin-core/gui#346: English translations update (Hennadii Stepanov)
412445afb5 Merge bitcoin-core/gui#313: qt: Optimize string concatenation by default (W. J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
Gui related backports from bitcoin v22
## What was done?
See commits
## How Has This Been Tested?
Run unit/functional tests
See also:
<img alt="right menu" src="https://user-images.githubusercontent.com/32963518/116794314-d64b9b80-aad4-11eb-89ca-7f75c7442ba8.gif"/>
## 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:
UdjinM6:
light ACK 6431f71b3a
PastaPastaPasta:
utACK 6431f71b3a
Tree-SHA512: bb14de71c9375b10da695db6c521c26686815b8b5ca2748bfe3bd2eafa9d332acd60acd85a1f2eed3aa831d16e5741ecc7570130ce9cf5bff011c065b55d62b2
62cb8d98d27e7f316f01f177f35ad0ed6f8cd9ce qt: Drop BitcoinGUI* WalletFrame data member (Hennadii Stepanov)
f73e5c972ab096e0f80cb9e753fa221d17313358 qt: Move CreateWalletActivity connection from WalletFrame to BitcoinGUI (Hennadii Stepanov)
20e2e24e90d782219e853ef0676ac66dc6a9de6a qt: Move WalletView connections from WalletFrame to BitcoinGUI (Hennadii Stepanov)
Pull request description:
This PR:
- implements an idea from https://github.com/bitcoin/bitcoin/pull/17937#issuecomment-575991765
- simplifies `WalletFrame` class interface
- as a side effect, removes `bitcoingui` -> `walletframe` -> `bitcoingui` circular dependency
- is an alternative to https://github.com/bitcoin/bitcoin/pull/17500
ACKs for top commit:
promag:
Tested ACK 62cb8d98d27e7f316f01f177f35ad0ed6f8cd9ce on macos 11.2.3 with depends build.
jarolrod:
ACK 62cb8d98d27e7f316f01f177f35ad0ed6f8cd9ce
Tree-SHA512: 633b526a8499ba9ab4b16928daf4de4f6d610284bb9fa51891cad35300a03bde740df3466a71b46e87a62121330fcc9e606eac7666ea5e45fa6d5785b60dcbbd
BACKPORT NOTICE: it includes missing commit: d6ef3543ae16847d5a91fa9271acee9bd2164b32
lint: Run mypy with --show-error-codes
When using mypy ignore directives, the error code needs to be specified.
Somehow mypy doesn't print it by default...
In upcoming commits, message sending will be split off into a separate
file and stats capabilities will be fleshed out. Prepare for that by
giving it its own directory.
Also get rid of `statsd` namespace, it is entirely unnecessary.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
cc998abec1 fmt: apply `clang-format-diff.py` suggestions (Kittywhiskers Van Gogh)
0401c581eb stats: `const`-ify variables and arguments (Kittywhiskers Van Gogh)
9f96723774 stats: stop using error codes, switch over to `bool` (Kittywhiskers Van Gogh)
1a81979c1e stats: initialize socket after we have a valid socket address (Kittywhiskers Van Gogh)
dbbfc8d766 stats: use `Socks` wrapper, use `CService` to generate our `sockaddr` (Kittywhiskers Van Gogh)
2def905044 stats: move init logic into constructor (Kittywhiskers Van Gogh)
4bc727cd6c stats: clean up randomization code, move `FastRandomContext` inward (Kittywhiskers Van Gogh)
840241eefd stats: cleanup error logging, improve code sanity (Kittywhiskers Van Gogh)
85890ddb13 docs: add copyright notice to source file, update notice in header (Kittywhiskers Van Gogh)
a9d1b1494d stats: move `_StatsdClientData` variables into `StatsdClient` (Kittywhiskers Van Gogh)
30c30c1397 stats: fetch all arguments needed when constructing `g_stats_client` (Kittywhiskers Van Gogh)
5133d88415 stats: s/statsClient/g_stats_client/g (Kittywhiskers Van Gogh)
f81951dd00 stats: make `statsClient` a `std::unique_ptr`, denote as global variable (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Support for transmitting stats to a Statsd server has been courtesy of Statoshi ([repo](https://github.com/jlopp/statoshi)), implemented Dec, 2020 by [dash#2515](https://github.com/dashpay/dash/pull/2515) but since then, it hasn't gotten much attention aside from benefiting from codebase-wide changes and the occasional compiler appeasement. This pull request aims to give our statistics code some TLC.
Changes include:
* Limiting initialization to solely during construction and moving the responsibility of fetching arguments outside of `statsd::StatsdClient`.
* Using the RAII `Socks` wrapper as early as possible (we still need to construct a raw socket ourselves but this is done in the initializer and control is moved to the wrapper and everywhere else, the wrapper is used)
* Utilizing existing networking code to generate the socket address
* This lets us trivially allow IPv6 connections as the responsibility to construct it safely is moved to `CService`.
* Using `std::string` and our string manipulation capabilities (replacing `snprintf` with `strprintf`), replacing platform-specific types (replacing `short` with `uint16_t`).
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK [cc998ab](cc998abec1)
UdjinM6:
utACK cc998abec1
Tree-SHA512: 433c92160d6ac7ebb8582ada3cbb65ead7913618266b773619a528c90dfe0e286aafa46dc3b0bca62f246938e5948a732080e2cddba942d3627f007ca6efcc1f
- Use `uint16_t` instead of `short`, `int64_t` instead of `size_t`
- Get rid of the `errmsg` buffer and use `LogPrintf` to report errors
- Use `strprintf` instead of `snprintf`
- Rephrase networking error logs to allow inclusion of error strings
DSQ messages are 142 bytes.
Previously, assuming a relatively highly connected masternode hosting 100 connection, each round of coinjoin will result in 14.2KB (100*142) of inbound and outbound traffic each.
Now, using the inventory system, a message will first use 36 bytes per peer (sending and receiving), plus the size of a `getdata` message and the actual message itself. As a result, bandwidth usage for 1 round of mixing would be closer to 36 * 100 + 142 (dsq) + 36 (getdata) = ~3.8KB, a reduction of around ~73%
BACKPORT NOTICE:
we keep and maintain cppcheck linter as lint-cppcheck-dash.sh
efae252f3072da598160670691757a0d60b9beb4 test: Remove extended lint (cppcheck) (laanwj)
Pull request description:
These are unreferenced in the CI and documentation, and have been since 2019 (see #17549).
I'm not sure the cppcheck is worthwhile. It takes a long time to run (I think this is why it isn't in the normal lints), and right
now it only appears to find implicit constructors. The list of exceptions is out of date. But if anyone wants to bring it back at any
time in the future they can do so from git history (and port it to Python).
ACKs for top commit:
fanquake:
ACK efae252f3072da598160670691757a0d60b9beb4
Tree-SHA512: 1a770b5d20ff1199d0d6bc471ae3d2c3438f0f0b169ce8d2fe73480daf8d3a7146c066b799afc90aa7898982c5fee79c1daca10e16e2bff0a7b38850aedd55b2
f4cb0fbfe1 fix: no need to relay quorum commitment in case of block undo (Konstantin Akimov)
0431a33919 fix: follow-up changes for bitcoin#14193. (Konstantin Akimov)
86b76d19b6 Merge bitcoin/bitcoin#21812: ci: Enable D_GLIBCXX_DEBUG for multiprocess task (fanquake)
334496ea7e Merge bitcoin/bitcoin#21775: p2p: Limit m_block_inv_mutex (MarcoFalke)
23b83109ea Merge bitcoin/bitcoin#21750: net: remove unnecessary check of CNode::cs_vSend (MarcoFalke)
b34514191f Merge bitcoin/bitcoin#21738: test: Use clang-12 for ASAN, Add missing suppression (fanquake)
3411577473 Merge bitcoin/bitcoin#19160: multiprocess: Add basic spawn and IPC support (W. J. van der Laan)
970048d917 fix: missing changes from bitcoin#19267 - run multiprocess on CI (Konstantin Akimov)
f2b7ee73db fix: follow-up bitcoin#15402 - removed dead code (Konstantin Akimov)
274068cdbc fix: follow-up bitcoin/bitcoin#21732 - minor missing typo (MarcoFalke)
e9450a8b36 Merge #21669: test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke)
ef92c3065c Merge #21663: ci: Fix macOS brew install command (W. J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
Just regular backports from v22
## What was done?
See commits for backports.
Also there're 2 bugs are fixed which became visible after backporting bitcoin#21775 - both are related to possible deadlocks in net_processing
## How Has This Been Tested?
Run unit and functional tests. Enabled multiprocess builds on CI
## 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:
UdjinM6:
utACK f4cb0fbfe1
PastaPastaPasta:
utACK f4cb0fbfe1
Tree-SHA512: 3204c2aa243fa4834ccf4ff4672d183cf9b35f87b857df8543572cd685729e15fca39f85b27194233e57cbc1746e36b556efab95ce20d0aa0a7d4476a9f3c6c0
84934bf70e11fe4cda1cfda60113a54895d4fdd5 multiprocess: Add echoipc RPC method and test (Russell Yanofsky)
7d76cf667eff512043a28d4407cc89f58796c42b multiprocess: Add comments and documentation (Russell Yanofsky)
ddf7ecc8dfc64cf121099fb047e1ac871de94f4c multiprocess: Add bitcoin-node process spawning support (Russell Yanofsky)
10afdf0280fa93bfffb0a7665c60dc155cd84514 multiprocess: Add Ipc interface implementation (Russell Yanofsky)
745c9cebd50fea1664efef571dc1ee1bddc96102 multiprocess: Add Ipc and Init interface definitions (Russell Yanofsky)
5d62d7f6cd48bbc4e9f37ecc369f38d5e1e0036c Update libmultiprocess library (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR adds basic process spawning and IPC method call support to `bitcoin-node` executables built with `--enable-multiprocess`[*].
These changes are used in https://github.com/bitcoin/bitcoin/pull/10102 to let node, gui, and wallet functionality run in different processes, and extended in https://github.com/bitcoin/bitcoin/pull/19460 and https://github.com/bitcoin/bitcoin/pull/19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket.
These changes can also be used to implement new functionality outside the `bitcoin-node` process like external indexes or pluggable transports (https://github.com/bitcoin/bitcoin/pull/18988). The `Ipc::spawnProcess` and `Ipc::serveProcess` methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test."
Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins.
Additional notes about this PR can be found at https://bitcoincore.reviews/19160
[*] Note: the `--enable-multiprocess` feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in [doc/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md)
ACKs for top commit:
fjahr:
re-ACK 84934bf70e11fe4cda1cfda60113a54895d4fdd5
ariard:
ACK 84934bf. Changes since last ACK fixes the silent merge conflict about `EnsureAnyNodeContext()`. Rebuilt and checked again debug command `echoipc`.
Tree-SHA512: 52a948b5e18a26d7d7a09b83003eaae9b1ed2981978c36c959fe9a55abf70ae6a627c4ff913a3428be17400a3dace30c58b5057fa75c319662c3be98f19810c6
1bf0bf492f merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh)
5c1eb67c42 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh)
c440304c85 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh)
e303a4ec45 merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh)
301163c65e merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh)
732e871a6b merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh)
b402fd57fa merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh)
a08f2f48bf merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh)
472caa048a merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh)
d69ca833df merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh)
6df927fc60 chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6078
* Dependent on https://github.com/dashpay/dash/pull/6074
* Dependent on https://github.com/dashpay/dash/pull/6083
* Dependent on https://github.com/dashpay/dash/pull/6119
* Dependency for https://github.com/dashpay/dash/pull/6138
* In [bitcoin#24050](https://github.com/bitcoin/bitcoin/pull/24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic.
* Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](37e026a038/src/spork.cpp (L44))) and upstream's usage of it to process translatable strings ([source](37e026a038/src/util/translation.h (L55-L62))).
Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore.
## Breaking Changes
None expected
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(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:
UdjinM6:
utACK 1bf0bf492f (with one nit)
knst:
utACK 1bf0bf492f
PastaPastaPasta:
utACK 1bf0bf492f
Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
Bitcoin uses underscore in util/translation.h for translatable strings
but underscores are also a placeholder used in multiple languages, incl.
C++. The next commit will be introducing backports, one of them will be
placing util/translation.h into validation.h, which is a header used by
Dash-specific code.
This causes a conflict between the normal usage of underscore as a
placeholder and Bitcoin's usage of it as a function name, which is
reported by the Dash-specific linter. We need to exclude shadowing
warnings from the linter to account for this.
fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204 rpc: Remove duplicate name and argNames from CRPCCommand (MarcoFalke)
fa92912b4bb4629addcbfdfb7cc000be701614af rpc: Use RPCHelpMan for check-rpc-mappings linter (MarcoFalke)
faf835680be39811827504f77005b6603165f53e rpc: [refactor] Use concise C++11 code in CRPCConvertTable constructor (MarcoFalke)
Pull request description:
Currently, the RPC argument names are specified twice to simplify consistency linting. To avoid having to specify the argnames twice when adding new arguments, remove the linter and add an equivalent test based on RPCHelpMan.
ACKs for top commit:
laanwj:
ACK fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204
Tree-SHA512: 3f5f32f5a09b22d879f24aa67031639d2612cff481d6aebc6cfe6fd757cafb3e7bf72120b30466f59292a260747b71e57322c189d5478b668519b9f32fcde31a
faaf9c58e4aa809019d4ca12747dd47411988e37 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke)
fa19bb2cd8c575593583138a84e6bb3444d6196d remove dead rpc code (MarcoFalke)
Pull request description:
Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37
promag:
Tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37.
ryanofsky:
Code review ACK faaf9c58e4aa809019d4ca12747dd47411988e37. Two obviously good simplifications.
Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee
fbffe06dad fix: suppress lint warnings for edge due to both missing epoll and kpoll (Konstantin Akimov)
b799683d60 fix: pass reference instead copy of argument in util/edge (Konstantin Akimov)
9d941aacb9 fix: removed unused assigned (Konstantin Akimov)
d9e2e47685 fix: add a missing file util/wpipe to linter lists (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Some source files in src/util is missing to specify as dash specific for linters
## What was done?
Added to list of dash's linters
## How Has This Been Tested?
Run `test/lint/lint-all.sh`
## 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:
UdjinM6:
utACK fbffe06dad
PastaPastaPasta:
utACK fbffe06dad
kwvg:
utACK fbffe06dad
Tree-SHA512: 05bd40f987a93b47ca939f37d6e5b62e2044f2acce7a6ea6c361594eace808da17b17e0149095daecd0fb5418c6cbedbc70ad07ac3b7a1156a94188b04c2fd00
172c2333f03aecb4c347c791537e13c296adbde2 Porting lint-assertions.sh to lint-assertions.py (hiago)
Pull request description:
This PR is converting `test/lint/lint-assertions.sh` to `test/lint/lint-assertions.py`. It's an item of #24783.
ACKs for top commit:
laanwj:
Tested ACK 172c2333f03aecb4c347c791537e13c296adbde2
Tree-SHA512: 94d5b03acfeaf2303fad95d489d6c3aa7bd655889ddaa807cc97e0613b8eb8f5ef094feee2a98d974606890deb554e76490a5c523d64eb5bc55afa6a43221aae
908fb7e2ec37fe68675d38dbfee4df9f861bb2b5 test: Use permissions from git in `lint-files.py` (laanwj)
48d2e80a7479a44b0ab09e87542c8cb7a8f72223 test: Don't use shell=True in `lint-files.py` (laanwj)
Pull request description:
Improvements to the `lint-files.py` script:
- Avoid use of `shell=True`.
- Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to `git ls-files`.
(what triggered this change was `File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755.` errors running the script locally).
ACKs for top commit:
vincenzopalazzo:
re-tACK 908fb7e2ec
Tree-SHA512: 2eaf868c55a9c3508b12658a5b3ac429963fd0551e645332d0ac54be56fefccee95115e4667386df24b46b545593cb0d0bf8c6cecab73f9cb19d37ddf704c614
fae211c0ae0dd90876a3390eb21449b7b0bb45c4 lint: Start to use py lint scripts (MarcoFalke)
fa82e890e7950fe5ba6d4fa88fcd922cc929dc47 Move lint script and data file to avoid lint- prefix (MarcoFalke)
Pull request description:
ACKs for top commit:
fjahr:
tACK fae211c0ae0dd90876a3390eb21449b7b0bb45c4
Tree-SHA512: f8272a1bab9efb8203cac121710baae68f01f79e520ad71ff15aa516d19763d61c088b411b019de105a6a30e7ee3c274814d59963f6ac22ba1084560fb601f45
2227fc4e6203064b14e99bcf453601bd263a0196 test: minor fixes & improvements for files linter test (windsok)
Pull request description:
Couple of minor fixes & improvements for files linter test added in #21740
- Use a context manager when opening files, so that files are closed are we are done with them
- Use the `-z` flag when shelling out to `git ls-files` so that we can catch newlines and other weird control characters in filenames.
From the `git ls-files` manpage:
```
-z \0 line termination on output and do not quote filenames. See OUTPUT below for more information.
Without the -z option, pathnames with "unusual" characters are quoted as explained for the configuration variable
core.quotePath (see git-config(1)). Using -z the filename is output verbatim and the line is terminated by a NUL byte.
```
ACKs for top commit:
MarcoFalke:
cr ACK 2227fc4e6203064b14e99bcf453601bd263a0196
practicalswift:
cr ACK 2227fc4e6203064b14e99bcf453601bd263a0196: patch looks correct
Tree-SHA512: af059a805f4a7614162de85dea856052a45ab531895cb0431087e7fc9e037513fa7501bb5eb2fe43238adf5f09e77712ebdbb15b1486983359ad3661a3da0c60
46b025e00df40724175735eb5606ac73067cb3b8 test: add new python linter to check file names and permissions (windsok)
6f6bb3ebc7cb8e17a5dfc8ef55aa2d3f2dc6bdea test: fix file permissions on various scripts (windsok)
Pull request description:
Adds a new python linter test which tests for correct filenames and file permissions in the repository.
Replaces the existing tests in the `test/lint/lint-filenames.sh` and `test/lint/lint-shebang.sh` linter tests, as well as adding some new and increased testing. This increased coverage is intended to catch issues such as in #21728 and https://github.com/bitcoin/bitcoin/pull/16807/files#r345547050
Summary of tests:
* Checks every file in the repository against an allowed regexp to make sure only lowercase or uppercase alphanumerics (a-zA-Z0-9), underscores (_), hyphens (-), at (@) and dots (.) are used in repository filenames.
* Checks only source files (*.cpp, *.h, *.py, *.sh) against a stricter allowed regexp to make sure only lowercase alphanumerics (a-z0-9), underscores (_), hyphens (-) and dots (.) are used in source code filenames. Additionally there is an exception regexp for directories or files which are excepted from matching this regexp (This should replicate the existing `test/lint/lint-filenames.sh` test)
* Checks all files in the repository match an allowed executable or non-executable file permission octal. Additionally checks that for executable files, the file contains a shebang line.
* Checks that for executable `.py` and `.sh` files, the shebang line used matches an allowable list of shebangs (This should replicate the existing `test/lint/lint-shebang.sh` test)
* Checks every file that contains a shebang line to ensure it has an executable permission
Additionally updates the permissions on various files to comply with the new tests.
Fixes#21729
ACKs for top commit:
practicalswift:
cr re-ACK 46b025e00df40724175735eb5606ac73067cb3b8: patch still looks correct
kiminuo:
code review ACK 46b025e00df40724175735eb5606ac73067cb3b8 if `contrib/gitian-descriptors/assign_DISTNAME` permission change is deemed OK.
laanwj:
Code review ACK 46b025e00df40724175735eb5606ac73067cb3b8
Tree-SHA512: 1c8201a2cee0d9cbce15652b68cec9a6458a8b493fcd5392f98560aca0b1a12e668baab65a47100f116f626dadc3f591deb47f7368468c6a46c6c712c2533455
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
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
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
`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
21ad71c578 Merge #21676: test: Use mocktime to avoid intermittent failure in rpc_tests (MarcoFalke)
76a41eb245 Merge #21602: rpc: add additional ban time fields to listbanned (MarcoFalke)
adea52a5fe Merge bitcoin-core/gui#260: Handle exceptions instead of crash (W. J. van der Laan)
7e023c394f Merge #17934: doc: Use CONFIG_SITE variable instead of --prefix option (fanquake)
bc6e3ed6e4 Merge #21606: fuzz: Extend psbt fuzz target a bit (MarcoFalke)
233fb245f7 Merge #21445: cirrus: Use SSD cluster for speedup (fanquake)
a224b800e4 Merge #21609: ci: increase CPU count of sanitizer job to increase memory limit (MarcoFalke)
ad947099a0 test: remove exception for util::Ref which doesn't exist more (Konstantin Akimov)
6674ee85ab Merge #21390: test: Test improvements for UTXO set hash tests (MarcoFalke)
e10eec249b Merge #21338: test: add functional test for anchors.dat (MarcoFalke)
d9c31d6817 Merge #21411: test: add logging, reduce blocks, move sync_all in wallet_ groups (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## Note for reviewers:
PRs bitcoin#17934 and bitcoin#21606 have been backported partially in past.
## What was done?
Removed unused sanitizer rules (see bitcoin#21366 and dashpay/dash#5055)
- bitcoin/bitcoin#21338
- bitcoin/bitcoin#21390
- bitcoin/bitcoin#21609
- bitcoin/bitcoin#21445
- bitcoin/bitcoin#21606
- bitcoin/bitcoin#17934
- bitcoin-core/gui#260
- bitcoin/bitcoin#21602
- bitcoin/bitcoin#21676
## 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 21ad71c578
Tree-SHA512: 94276d56255300d7d8c056d15b468720ba028d83cc585b16396e8bad90157e9e010490be239e09cccd4362f575f8df2d56dde3fb505745376c7790d70e3635cd
fa0074e2d82928016a43ca408717154a1c70a4db scripted-diff: Bump copyright headers (MarcoFalke)
Pull request description:
Needs to be done because no one has removed the years yet
ACKs for top commit:
practicalswift:
ACK fa0074e2d82928016a43ca408717154a1c70a4db
Tree-SHA512: 210e92acd7d400b556cf8259c3ec9967797420cfd19f0c2a4fa54cb2b3d32ad9ae27e771269201e7d554c0f4cd73a8b1c1a42c9f65d8685ca4d52e5134b071a3
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)_
fea75ad3caa29972db32d3ce7e0fe125ec77a0eb refactor: Drop `boost/algorithm/string/replace.hpp` dependency (Hennadii Stepanov)
857526e8cbb0847a865e9c2509425960d458f535 test: Add test case for `ReplaceAll()` function (Hennadii Stepanov)
Pull request description:
A new implementation of the `ReplaceAll()` seems enough for all of our purposes.
ACKs for top commit:
adam2k:
ACK Tested fea75ad3caa29972db32d3ce7e0fe125ec77a0eb
theStack:
Code-review ACK fea75ad3caa29972db32d3ce7e0fe125ec77a0eb
Tree-SHA512: dacfffc9d2bd1fb9f034baf8c045b1e8657b766db2f0a7f8ef7e25ee6cd888f315b0124c54aba7a29ae59186b176ef9868a8b709dc995ea215c6b4ce58e174d9
## Issue being fixed or feature implemented
Current implementation relies either on asserts or sometimes checks then
returning a special value; In the case of asserts (or no assert where we
use the value without checks) it'd be better to make it explicit to
function caller that the ptr must be not_null; otherwise gsl::not_null
will call terminate.
See
https://github.com/microsoft/GSL/blob/main/docs/headers.md#user-content-H-pointers-not_null
and
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-nullptr
I'm interested in a conceptual review; specifically on if this is
beneficial over just converting these ptrs to be a reference?
## What was done?
*Partial* implementation on using gsl::not_null in dash code
## How Has This Been Tested?
Building
## Breaking Changes
None
## 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
- [ ] 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)_
---------
Signed-off-by: pasta <pasta@dashboost.org>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Implementation of accepted proposal:
https://www.dashcentral.org/p/TREASURY-REALLOCATION-60-20-20
## What was done?
Once Masternode Reward Location Reallocation activates:
- Treasury is bumped to 20% of block subsidy.
- Block reward shares are immediately set to 75% for MN and 25% miners.
(Previous reallocation periods are dropped)
MN reward share should be 75% of block reward in order to represent 60%
of the block subsidy. (according to the proposal)
- `governancebudget` is returned from `getgovernanceinfo` RPC.
## How Has This Been Tested?
`block_reward_reallocation_tests`
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [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 _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Unneeded suppressions were present
## What was done?
Removed them
## How Has This Been Tested?
Running linter
## Breaking Changes
None
## 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
- [ ] 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)_
## Motivation
CoinJoin's subsystems are initialized by variables and managers that
occupy the global context. The _extent_ to which these subsystems
entrench themselves into the codebase is difficult to assess and moving
them out of the global context forces us to enumerate the subsystems in
the codebase that rely on CoinJoin logic and enumerate the order in
which components are initialized and destroyed.
Keeping this in mind, the scope of this pull request aims to:
* Reduce the amount of CoinJoin-specific entities present in the global
scope
* Make the remaining usage of these entities in the global scope
explicit and easily searchable
## Additional Information
* The initialization of `CCoinJoinClientQueueManager` is dependent on
blocks-only mode being disabled (which can be alternatively interpreted
as enabling the relay of transactions). The same applies to
`CBlockPolicyEstimator`, which `CCoinJoinClientQueueManager` depends.
Therefore, `CCoinJoinClientQueueManager` is only initialized if
transaction relaying is enabled and so is its scheduled maintenance
task. This can be found by looking at `init.cpp`
[here](93f8df1c31/src/init.cpp (L1681-L1683)),
[here](93f8df1c31/src/init.cpp (L2253-L2255))
and
[here](93f8df1c31/src/init.cpp (L2326-L2327)).
For this reason, `CBlockPolicyEstimator` is not a member of `CJContext`
and its usage is fulfilled by passing it as a reference when
initializing the scheduling task.
* `CJClientManager` has not used `CConnman` or `CTxMemPool` as `const`
as existing code that is outside the scope of this PR would cast away
constness, which would be unacceptable. Furthermore, some logical paths
are taken that will grind to a halt if they are stored as `const`.
Examples of such a call chains would be:
* `CJClientManager::DoMaintenance >
CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating >
CCoinJoinClientSession::DoAutomaticDenominating >
CCoinJoinClientSession::StartNewQueue > CConnman::AddPendingMasternode`
which modifies `CConnman::vPendingMasternodes`, which is non-const
behaviour
* `CJClientManager::DoMaintenance >
CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating >
CCoinJoin::IsCollateralValid > AcceptToMemoryPool` which adds a
transaction to the memory pool, which is non-const behaviour
* There were cppcheck [linter
failures](https://github.com/dashpay/dash/pull/5337#issuecomment-1685084688)
that seemed to be caused by the usage of `Assert` in
`coinjoin/client.h`. This seems to be resolved by backporting
[bitcoin#24714](https://github.com/bitcoin/bitcoin/pull/24714). (Thanks
@knst!)
* Depends on #5546
---------
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
