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>
## What was done?
- remove dependency of Asset Lock txes on CCreditPool
- new case for functional tests of Asset Locks - more than one output
for Asset Lock tx.
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
Slightly changes behaviour of TxMempool. Tx can be accepted in mempool
even if Asset Unlock transaction with same index is already mined. But
final consensus rules are same.
## 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
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
fa4632c41714dfaa699bacc6a947d72668a4deef test: Move boost/stdlib includes last (MarcoFalke)
fa488f131fd4f5bab0d01376c5a5013306f1abcd scripted-diff: Bump copyright headers (MarcoFalke)
fac5c373006a9e4bcbb56843bb85f1aca4d87599 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c41714dfaa699bacc6a947d72668a4deef
jonatack:
ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
## Issue being fixed or feature implemented
There's one type of output that potentially can be useful for bloom
filter.
It's follow-up for TODO for dashpay/dash#4857.
Asset Lock transactions have:
- standard inputs (covered by regular bloom filter implementation)
- standard outputs (covered by regular bloom filter implementation)
- special outputs that have public key to proof owing this credits on
platform and claiming it.
Asset Unlock transactions have:
- no inputs (no need bloom)
- standard outputs (covered by regular bloom filter implementation)
So far as there's only one special case, let's have this data in the
bloom filter because it can potentially help to show information such as
"Deposit to platform" on mobile clients.
## What was done?
- added special case for Asset Lock transactions for bloom filter
## How Has This Been Tested?
Run unit/functional tests. Doesn't actually tested how bloom filter
works.
## 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
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb854ca827a5a3925394f9e09d29b898 lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct!
MarcoFalke:
review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
## Issue being fixed or feature implemented
This is an implementation of DIP0027 "Credit Asset Locks".
It's a mechanism to fluidly exchange between Dash and credits.
## What was done?
This pull request includes:
- Asset Lock transaction
- Asset Unlock transaction (withdrawal)
- Credit Pool in coinbase
- Unit tests for Asset Lock/Unlock tx
- New functional test `feature_asset_locks.py`
RPC: currently locked amount (credit pool) is available through rpc call
`getblock`.
## How Has This Been Tested?
There added new unit tests for basic checks of transaction validity
(asset lock/unlock).
Also added new functional test "feature_asset_locks.py" that cover
typical cases, but not all corner cases yet.
## Breaking Changes
This feature should be activated as hard-fork because:
- It adds 2 new special transaction and one of them [asset unlock tx]
requires update consensus rulels
- It adds new data in coinbase tx (credit pool)
## 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
- [ ] I have made corresponding changes to the documentation
**To release DIP 0027**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Implementation of Randomness Beacon Part 3.
Starting from v20 activation fork, members for quorums are sorted using
(if available) the best CL signature found in Coinbase.
If no CL signature is present yet, then the usual way is used (By using
Blockhash instead)
The actual new way to shuffle is already implemented in
https://github.com/dashpay/dash/pull/5366.
SPV clients also need to calculate members, but they only know block
headers.
Since Coinbase is in the actual block, then they lack the required
information to correctly calculate quorum members.
## What was done?
- Message `MNLISTIDFF` is enriched with a new field `quorumsCLSigs`.
This field holds the Chainlock Signature required for each set of
indexes corresponding to quorums in field `newQuorums`.
- Protocol version has been bumped to `70230`.
- Clients with protocol version greater or equal to `70230` will receive
the new field `quorumsCLSigs`.
- The same field is returned in `protx diff` RPC.
Note:
- Field `quorumsCLSigs` will populated only after v20 activation
- If for one or more quorums, no non-null CL sig was found in CbTx then
a null signature is returned in `quorumsCLSigs`.
## How Has This Been Tested?
- Functional test mininode's protocol version was bumped to `70230`.
- `feature_llmq_rotation.py` checks that `quorumsCLSigs` match in both
P2P and RPC messages.
## Breaking Changes
No
## 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: thephez <thephez@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
## Issue being fixed or feature implemented
It splits from https://github.com/dashpay/dash/pull/5150/ by
@PastaPastaPasta request.
## What was done?
See commits
## 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
f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb2191373e76d79a4972d6192300976096 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83810936e7a334304e5fd7c6dab8e91b Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095253225f64462ee15c595644747f376 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)
Pull request description:
Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.
This could help debug issues where there may be race conditions at play, such as #12978.
ACKs for top commit:
jnewbery:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
hebasto:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
ariard:
ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
ryanofsky:
Code review ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f. Just suggested changes since last review (thanks!)
Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236
## Issue being fixed or feature implemented
Move `BuildSimplifiedDiff` to the place it's actually used. This also
resolves 3 circular dependencies we have atm.
## What was done?
mostly trivial move-only changes
## How Has This Been Tested?
it compiles and linter is happy locally
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
fa165e954579436fe4b636e4222d8ce0c1269786 Replace stoul with ToIntegral in dbwrapper (MarcoFalke)
Pull request description:
The string is created with `%llu`. See: 7fcf53f7b4/src/leveldb/db/db_impl.cc (L1436-L1437)
So it seems odd to silently accept when parsing: whitespace, a sign character, trailing chars, overflow, ....
Fix that by using the stricter ToIntegral.
ACKs for top commit:
laanwj:
Code review ACK fa165e954579436fe4b636e4222d8ce0c1269786
practicalswift:
cr ACK fa165e954579436fe4b636e4222d8ce0c1269786
theStack:
Code-review ACK fa165e954579436fe4b636e4222d8ce0c1269786
Tree-SHA512: b87f01431ca0b971ff84610022da8679d3c33470b88cfc3f4a337e6e176a0455715588aefd40e8e2bbe7459d902dc89d7bfe34e7fd66755f631cc18dc039fa2f
## Issue being fixed or feature implemented
Upgraded version of cppcheck
## What was done?
## How Has This Been Tested?
Ran cppcheck
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
9b0e16226e6c1fb6a3550d635339f1bbb49a852f doc: Correct spelling errors in comments (Ben Woosley)
Pull request description:
And ci script output.
Identified via test/lint/lint-spelling
Before:
```
$ test/lint/lint-spelling.sh
ci/test/05_before_script.sh:29: explicitely ==> explicitly
src/compressor.h:43: Ser ==> Set
src/compressor.h:78: Ser ==> Set
src/logging/timer.h:88: outputing ==> outputting
src/node/psbt.cpp:87: minumum ==> minimum
src/qt/coincontroldialog.cpp:372: UnSelect ==> deselect
src/qt/coincontroldialog.cpp:443: unselect ==> deselect
src/qt/coincontroldialog.cpp:448: UnSelect ==> deselect
src/qt/coincontroldialog.cpp:699: UnSelect ==> deselect
src/serialize.h:211: Ser ==> Set
src/serialize.h:213: Ser ==> Set
src/serialize.h:228: Ser ==> Set
src/serialize.h:246: Ser ==> Set
src/serialize.h:484: Ser ==> Set
src/serialize.h:490: Ser ==> Set
src/serialize.h:510: Ser ==> Set
src/serialize.h:622: Ser ==> Set
src/serialize.h:740: Ser ==> Set
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
src/txmempool.h:756: incomaptible ==> incompatible
src/undo.h:26: Ser ==> Set
src/wallet/coincontrol.h:74: UnSelect ==> deselect
test/functional/feature_backwards_compatibility.py:116: Abondon ==> Abandon
test/functional/rpc_getaddressinfo_label_deprecation.py:7: superceded ==> superseded
test/lint/lint-shell.sh:44: desriptor ==> descriptor
^ 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
```
After:
```
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
test/functional/rpc_getaddressinfo_label_deprecation.py:7: superceded ==> superseded
^ 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:
practicalswift:
ACK 9b0e16226e6c1fb6a3550d635339f1bbb49a852f
MarcoFalke:
ACK 9b0e16226e6c1fb6a3550d635339f1bbb49a852f
Tree-SHA512: 9ce203700b11596e4b920b3c5b04f59bc7784fe5b495868d43423608180a9a553ec7efcc5ad70384f3ce462b036c2a682260efebce493c5e6a3d48716b268179
e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd86715039586d92176eee16e9c6644d2547f0 Avoid using mutable default parameter values (practicalswift)
Pull request description:
Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.
Examples of this gotcha caught during review:
* https://github.com/bitcoin/bitcoin/pull/16673#discussion_r317415261
* https://github.com/bitcoin/bitcoin/pull/14565#discussion_r241942304
Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:
```
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})
```
In contrast to:
```
>>> def f(i, j=None, k=None):
... if j is None:
... j = []
... if k is None:
... k = {}
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})
```
The latter is typically the intended behaviour.
This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)
ACKs for top commit:
Sjors:
Oh Python... ACK e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1. Testing tip: swap the two commits.
Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
ca185cf5a14b16d61814d7172284bc8efcd28b69 doc: Document differences in bitcoind and bitcoin-qt locale handling (practicalswift)
Pull request description:
Document differences in `bitcoind` and `bitcoin-qt` locale handling.
Since this seems to be the root cause to the locale dependency issues we've seen over the years I thought it was worth documenting :)
Note that 1.) `QLocale` (used by Qt), 2.) C locale (used by locale-sensitive C standard library functions/POSIX functions and some parts of the C++ standard library such as `std::to_string`) and 3.) C++ locale (used by the C++ input/output library) are three separate things. This comment is about the perhaps surprising interference with the C locale (2) that takes place as part of the Qt initialization.
ACKs for top commit:
hebasto:
re-ACK ca185cf5a14b16d61814d7172284bc8efcd28b69
Tree-SHA512: e51c32f3072c506b0029a001d8b108125e1acb4f2b6a48a6be721ddadda9da0ae77a9b39ff33f9d9eebabe2244c1db09e8502e3e7012d7a5d40d98e96da0dc44
797b3ed9090030f32fade81803b580562d4a90a3 script: remove gitian reference from symbol-check.py (fanquake)
15fc9a0299091bfeb3370f993ad95ff638f6ba8c guix: add additional documentation to patches (fanquake)
4516e5ec9223486fe2eba7f4320d786d074a58fd lint: exclude Guix patches from spell-checking (fanquake)
de6ca41a52d2646598daae5f4620bbe766757e21 guix: no-longer pass --enable-glibc-back-compat to Guix (fanquake)
84dd81fb5bf7308b8070b53520266854fb6efad3 build: remove glibc backcompat requirement for Linux symbol checks (fanquake)
Pull request description:
Now that our Guix toolchains are based on glibc 2.24 and 2.27 (RISCV), we don't need to use the `--enable-glibc-back-compat` option to produce binaries that don't use any symbols from glibc 2.17 and 2.27 or later.
This also adds additional documentation to some Guix patches (pointed out in #22365) and removes Guix patches from the spelling linter, because that isn't our spelling.
Symbol usage: https://gist.github.com/fanquake/d15604fc580718444c5aa4b3c3c75fdc.
Guix Builds:
```bash
bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
ed54e6a6cf4fab328557c0c72eb08c73f2a58c6c70959544cf4b1882e75ea69e guix-build-797b3ed90900/output/aarch64-linux-gnu/SHA256SUMS.part
83bd9dadc59f89f848d143fa4fc3964f16fe0b4bdf35e5093b577ff2c4bd1f43 guix-build-797b3ed90900/output/aarch64-linux-gnu/bitcoin-797b3ed90900-aarch64-linux-gnu-debug.tar.gz
94cb8c35281f12dec6ea5b390b66cad5e27ac8c45a30c42c8d38c438695d54c0 guix-build-797b3ed90900/output/aarch64-linux-gnu/bitcoin-797b3ed90900-aarch64-linux-gnu.tar.gz
7318b63d65c0aa52d2446de8e1f40658d2e47ab8fb0268820c3b7585d140fb23 guix-build-797b3ed90900/output/arm-linux-gnueabihf/SHA256SUMS.part
95e1ffb372964b73f539653ca703b70cf0c018801a9c4c0ffc46a0b63539253c guix-build-797b3ed90900/output/arm-linux-gnueabihf/bitcoin-797b3ed90900-arm-linux-gnueabihf-debug.tar.gz
039d3842e6499626cf955ae0a7590dd6b3d0935cdc217c98aaf9d156b0ebd3b4 guix-build-797b3ed90900/output/arm-linux-gnueabihf/bitcoin-797b3ed90900-arm-linux-gnueabihf.tar.gz
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-797b3ed90900/output/dist-archive/SKIPATTEST.TAG
2c4e7b6e7aff63ba811e5bf59362d16866c3a358f8844fba8739a61192870622 guix-build-797b3ed90900/output/dist-archive/bitcoin-797b3ed90900.tar.gz
955029b949c368eabd517dd33040d2f01e2ac6a55e7b4f9107907a7c6e0c6060 guix-build-797b3ed90900/output/powerpc64-linux-gnu/SHA256SUMS.part
fd6d6b137f8efedf58a879d11205b1d4649e1f97d7f91e193239ef206fcc285d guix-build-797b3ed90900/output/powerpc64-linux-gnu/bitcoin-797b3ed90900-powerpc64-linux-gnu-debug.tar.gz
51736ac8e77737999f1b5bd4c381b0016f19a8d5e40e786fe941ff04e84c11c9 guix-build-797b3ed90900/output/powerpc64-linux-gnu/bitcoin-797b3ed90900-powerpc64-linux-gnu.tar.gz
8c244c16bfa46c1efdb120e1d91fdd14d3f14eefee8d7e1fbb0a9b4664a5c315 guix-build-797b3ed90900/output/powerpc64le-linux-gnu/SHA256SUMS.part
704ee593251a1b1c65a5bebeef93b23f266af4e8cbf8ae556150c3b2e8f06a6c guix-build-797b3ed90900/output/powerpc64le-linux-gnu/bitcoin-797b3ed90900-powerpc64le-linux-gnu-debug.tar.gz
0ec06ae7d344de20d61e3965d8b383747ef20b0e9d93a3165733ea23bdf2ead8 guix-build-797b3ed90900/output/powerpc64le-linux-gnu/bitcoin-797b3ed90900-powerpc64le-linux-gnu.tar.gz
2dd6c6ecc67b0ea40ca9c43f92efca81ccd054b8db8c197ad84ad9674d510a25 guix-build-797b3ed90900/output/riscv64-linux-gnu/SHA256SUMS.part
5ebb27a855a677f7a188d83995be6b2a3ea8606be152abb7fc7832713fb0677a guix-build-797b3ed90900/output/riscv64-linux-gnu/bitcoin-797b3ed90900-riscv64-linux-gnu-debug.tar.gz
bdaf1783f5e1861597afa37c1880364e118d9a7a7af8017302d82202791019f6 guix-build-797b3ed90900/output/riscv64-linux-gnu/bitcoin-797b3ed90900-riscv64-linux-gnu.tar.gz
726c9092b60ac2e7d7e14b2c24467fcf276a6f89170a871ddab9dce6ac230699 guix-build-797b3ed90900/output/x86_64-apple-darwin18/SHA256SUMS.part
2af4d709b44952654f3c08c86593bf2ccc9a44ed422783a1b95b8a199a894db2 guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx-unsigned.dmg
fd49ba445aa6cf3d8c47019a05e9e5740cb0f53349344dd80671297127f49f1a guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx-unsigned.tar.gz
3f51cbf8cf18420d4be70e656aa993675cf5e828a255c2030047ae2e059ed5b7 guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx64.tar.gz
afd1edee1447bb88d81e972abfae4c4e065b5b1827769f033cff9472084c7c1b guix-build-797b3ed90900/output/x86_64-linux-gnu/SHA256SUMS.part
ec468ef886d25e685f4f7a18b4f7d497dedf757495e0d5beb72c23cc32ab69b5 guix-build-797b3ed90900/output/x86_64-linux-gnu/bitcoin-797b3ed90900-x86_64-linux-gnu-debug.tar.gz
1934d7294f0c9e083d38a3f68d4a61cd679defa79ce0a89f77386978692b9b18 guix-build-797b3ed90900/output/x86_64-linux-gnu/bitcoin-797b3ed90900-x86_64-linux-gnu.tar.gz
94c11c328a628052eb6f50e9816aa768f87ea7acfbbbafdab60f6928da766811 guix-build-797b3ed90900/output/x86_64-w64-mingw32/SHA256SUMS.part
fd371922ba93d81bd4a2b711d617af6756f9f0494db6d83aa0e5f491a24168ef guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win-unsigned.tar.gz
4e4ad976bc029bbbf9596ad8493accaaba8b0d5c598dd342f8da330609bbdf21 guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64-debug.zip
3a89a16b9101e9a17d98efb9234b5bdd264c0bba2c6326511017730e1a08311f guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64-setup-unsigned.exe
e285ab737e3c843fd3f1c26c2f053e421a3c39b33995747ce48281884d3f28d1 guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64.zip
```
ACKs for top commit:
sipa:
utACK 797b3ed9090030f32fade81803b580562d4a90a3
hebasto:
ACK 797b3ed9090030f32fade81803b580562d4a90a3
Tree-SHA512: 3a569702d8832c155c5ce8d2f6d823f7f12603885576078bc5192bc9038a48261ecb541800f79d1e9bc86d71fa640265c5b8b89df9d8bb680b3bb05d9d78a666
1fca9811e1331ac5dae8188f6178cc37da4929a7 lint: Skip whitespace lint for guix patches (Carl Dong)
a91c46c57d88fc399432afab7bb0fb14c3e490a7 guix: Make nsis reproducible by respecting SOURCE-DATE-EPOCH (Carl Dong)
Pull request description:
```
When building nsis, if VERSION is not specified, it defaults to
cvs_version which is non-deterministic as it includes the current date.
This patches nsis to default to SOURCE_DATE_EPOCH if it exists so that
nsis is reproducible.
Upstream change: https://github.com/kichik/nsis/pull/13
```
Sidenote: also a good demonstration of how Guix allows us to flexibly patch our tools!
Note to reviewers: if you want to compare hashes, please build after Jan 16th 2021 without my substitute server enabled!
ACKs for top commit:
fanquake:
ACK 1fca9811e1331ac5dae8188f6178cc37da4929a7
Tree-SHA512: b800e0ce5f73827ad353739effb9167ec3a6bdb362c725ae20dd3f025ce78660f85c70ce1d75cd0896facf1e8fe38a9e058459ed13dec71ab3a2fe41e20eaa5d
3f373659d732a5b1e5fdc692a45b2b8179f66bec Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c4039103670cec5f9cace897ead76e20a8 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e3402048269362ff692d49a6df35cfd Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3b74a3943e4c359270beaf3e6ff8a7b Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59e88a3e5ee248144bcc023debcd895b refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3e5583defdb76e3a280f48e98983301 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206e399981a77976deb15785d18db46ae Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e8460127a40a7895519587399feff3b682 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538008d98abe79288b92040bc186b93f3 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc57c46ae243f45d73e715d509f53a6c List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee58d3e88af36d8091b9e4017a8603b3c Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad944cad42e805228cdd58e0332f4d7184 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694d4dbaf59eef562d80fbeacb3bb7dc1 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
## Issue being fixed or feature implemented
Avoid lots of static_cast's from enums to underlying types. Communicate
intention better
## What was done?
implement c++23 inspired ToUnderlying, then see std::to_underlying and
https://en.cppreference.com/w/cpp/types/underlying_type; Then, we use
this instead of static_casts for enums -> underlying type
## How Has This Been Tested?
make check
## Breaking Changes
None
## 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
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
e57980b4738c10344baf136de3e050a3cb958ca5 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f36124972d2364f941de9c3417c65f05b6 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f527631ede1a31c7855151e5c5d91f8f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b4000fed088b8cf7b99674c328d15e1 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443cc16edf974f099b8485e04b3db1b1d7 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d759b13af68acec6d5bfa04aaa24561f8 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b4738c10344baf136de3e050a3cb958ca5, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
e20c72f9f076681def325b5b5fa53bccda2b0eab Fire TransactionRemovedFromMempool from mempool (251)
Pull request description:
This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.
It also resolves the `txmempool -> validation -> validationinterface -> txmempool` circular dependency.
Ideally, `validationinterface` is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving `txmempool` specific code out of `validationinterface` to `txmempool` where it belongs.
ACKs for top commit:
jnewbery:
ACK e20c72f9f076681def325b5b5fa53bccda2b0eab
Tree-SHA512: 354c3ff1113b21a0b511d80d604edfe3846dddae3355e43d1387f68906e54bf5dc01e7c029edc0b8e635b500b2ab97ee50362e2486eb4319f7347ee9a9e6cef3
<!--
*** Please remove the following help text before submitting: ***
Provide a general summary of your changes in the Title above
Pull requests without a rationale and clear improvement may be closed
immediately.
Please provide clear motivation for your patch and explain how it
improves
Dash Core user experience or Dash Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always
welcome.
* All other changes should have accompanying unit tests (see
`src/test/`) or
functional tests (see `test/`). Contributors should note which tests
cover
modified code. If no tests exist for a region of modified code, new
tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or
an
explanation of the potential issue as well as reasoning for the way the
bug
was fixed.
* Features are welcome, but might be rejected due to design or scope
issues.
If a feature is based on a lot of dependencies, contributors should
first
consider building the system outside of Dash Core, if possible.
-->
## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
minimizing global uses
## What was done?
<!--- Describe your changes in detail -->
Started the deglobalization, a future PR should be done to continue this
deglobalization
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
## Breaking Changes
<!--- Please describe any breaking changes your code introduces -->
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
- [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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* Squashed 'src/dashbls/' content from commit 66ee820fbc
git-subtree-dir: src/dashbls
git-subtree-split: 66ee820fbc9e3b97370db8c164904af48327a124
* build: stop tracking build-system generated relic_conf.h.in
* build: add support for building bls-signatures from local subtree
* build: add exclusions to linting scripts and filters
* build: drop bls-signatures (bls-dash) from depends
* llmq: move initialization logic to 'LLMQContext', add unique pointer to NodeContext
* llmq: add aliases to LLMQ globals, expose them to RPC via LLMQContext
* rpc: replace most global invocations with LLMQContext aliases
* rpc: replace quorum RPC global invocations with LLMQContext aliases
* llmq: replace individual global member arguments with context pointer
* llmq: pass aliased context pointer instead of individual globals in tests
* llmq: move BLS worker to LLMQContext, remove global
* llmq: move DKG debug manager to LLMQContext, remove global
* llmq: move DKG session manager to LLMQContext, remove global
* llmq: move quorum share manager to LLMQContext, remove global
* llmq: move quorum signing manager to LLMQContext, remove global
ff59bcd3213ef61f2167c0aa60fcaf5afbc20c61 gui: Drop PeerTableModel dependency to ClientModel (João Barbosa)
Pull request description:
Class `PeerTableModel` doesn't actually depend on `ClientModel`.
ACKs for top commit:
Empact:
Code Review ACK ff59bcd321
hebasto:
ACK ff59bcd3213ef61f2167c0aa60fcaf5afbc20c61, tested on Linux Mint 19.3. No changes in behavior are observed.
Tree-SHA512: 29fa3c316c05b8f7b9340e5859bbb8c3a0b826aa7c865c892cfa13b5ad30f822fcaae4e01555f7860cd1727f20b7ef555a808235522a04a6eebaaa7b605f8595
cb8a86d9f952401eaad68b2e3818ce50f7befd91 gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d10777d65b68862c6deb57594c8fc4d21ca77 gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)
Pull request description:
Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.
ACKs for top commit:
hebasto:
ACK cb8a86d9f952401eaad68b2e3818ce50f7befd91, tested on Linux Mint 19.3.
jonasschnelli:
utACK cb8a86d9f952401eaad68b2e3818ce50f7befd91
Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863 gui: Drop ShutdownWindow dependency to BitcoinGUI (João Barbosa)
61eb058cc10592cfa314ba2209fb370706100e8b gui: Drop BanTableModel dependency to ClientModel (João Barbosa)
Pull request description:
`ShutdownWindow::showShutdownWindow` just needs a widget to center the shutdown window and to borrow its title.
ACKs for top commit:
hebasto:
ACK 3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863, since previous review only suggested change `QWidget` --> `QMainWindow`
jonasschnelli:
utACK 3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863
Tree-SHA512: e15cb6ee274730bd071d3d97b540c5059e5c655248d69a37c3fd00f2aacc6cfcb36b9a65755718027e15482ec8e5e85534c1dc13d0ddb4e0680df03fbf6571f2
* coinjoin: make CCoinJoinServer managed pointer, assign CConnman during init
* coinjoin: make CCoinJoinClientQueueManager managed pointer, assign CConnman during init
* sporks: move spork validation logic downwards after CConnman initialization
* sporks: make CSporkManager a pointer, reduce global invocations
* governance: make CGovernanceManager a pointer, reduce global invocations
* llmq: migrate LLMQ subsystem raw pointers to managed pointers
* masternode: make activeMasternodeManager a managed pointer
* masternode: make masternodeSync a managed pointer, assign CConnman during init
* refactor: make instantsend helper functions class members
* fix: send empty CDeterministicMNList if pointer isn't initialized yet
* fix: refactor governance object retrieval logic across node and ui
Update src/interfaces/node.cpp
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
fa5facd3e72b6d61374b0b93b722b55e2b090020 rpc: Remove unused boost::this_thread::interruption_point (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, the rpc threads are `std::thread`, which does not have an `std:🧵:interrupt` member function to request interruption: https://dev.visucore.com/bitcoin/doxygen/httpserver_8cpp.html#ae1a63374e18b9abd348eb74e4243ea34
Thus, the interruption points can be removed.
ACKs for top commit:
laanwj:
ACK fa5facd3e72b6d61374b0b93b722b55e2b090020, this does nothing.
practicalswift:
ACK fa5facd3e72b6d61374b0b93b722b55e2b090020
jamesob:
ACK fa5facd3e7
Tree-SHA512: 4e29a44df1f2702cbd1ffdffa559440a8bb800baab64b4116e2c3d27cd64d8d1e8aafe1dc21b1a4e3988470d03be19cae294bd5669f7abf6d487685dc8fd8d7e
* Move wallet enums to walletutil.h
* MOVEONLY: Move key handling code out of wallet to keyman file
Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp
The easiest way to review this commit is to run:
git log -p -n1 --color-moved=dimmed_zebra
And check that everything is a move (other than includes and copyrights comments).
This commit is move-only and doesn't change code or affect behavior.
* Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
* Renamed classes in scriptpubkeyman
* Fixes for conflicts, compilation and linkage errors due to previous commits
* Reordered methods in scriptpubkeyman to make further backports easier
* Reordered methods in scriptpubkeyman to make further backports easier (part II)
* Remove HDChain copy from SigningProvider class
* fixes/suggestions
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* refactor(llmq): substitute memberless class llmq::CLLMQUtils with namespace llmq::utils
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* chore: mark functions internal to `llmq::utils` as `static`
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* Add HaveKey and HaveCScript to SigningProvider
* Remove CKeyStore and squash into CBasicKeyStore
* Move HaveKey static function from keystore to rpcwallet where it is used
* scripted-diff: rename CBasicKeyStore to FillableSigningProvider
-BEGIN VERIFY SCRIPT-
git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g'
-END VERIFY SCRIPT-
* Move KeyOriginInfo to its own header file
* Move various SigningProviders to signingprovider.{cpp,h}
Moves all of the various SigningProviders out of sign.{cpp,h} and
keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed.
Includes and the Makefile are updated to reflect this. Includes were largely
changed using:
git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'
* Remove CCryptoKeyStore and move all of it's functionality into CWallet
Instead of having a separate CCryptoKeyStore that handles the encryption
stuff, just roll it all into CWallet.
* Fixed cases of mess CWallet functions with CCryptoKeyStore and conflicts
* Move WatchOnly stuff from SigningProvider to CWallet
* Fixes for lint cirtular dependencies to calm linter
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
fa677d1801fb9153a95a1fc9855fd5f21fc440c0 ci: Remove redundant check for TRAVIS_OS_NAME (MarcoFalke)
fadccb263baf6b8694f750623add42f966e423a3 doc: Document that GNU tools are required for linters (MarcoFalke)
4444704ca9f66cdc24ab2d444941354db1dfed06 ci: Cleanup macOS runs (MarcoFalke)
Pull request description:
* Remove a commented out cleanup task in `before_cache`
* Remove the linter run on macOS, and document that GNU tools are required to run the linters
ACKs for top commit:
Sjors:
Code review ACK fa677d1801fb9153a95a1fc9855fd5f21fc440c0
laanwj:
ACK fa677d1801fb9153a95a1fc9855fd5f21fc440c0
ryanofsky:
Code review ACK fa677d1801fb9153a95a1fc9855fd5f21fc440c0 for new third commit replacing TRAVIS_OS_NAME check with NO_DEPENDS setting
Tree-SHA512: 9122a63bbe7887d9e379123152ea4ba44324cb18033b9e6b45bfdb1af665c10ea598564b9fcd57330d208a08e4696e41b4d6175f05f0843a3a76530da114f8c6
8f7b93047581c67f2133cdb8c7845471de66c30f Drop the leading 0 from the version number (Andrew Chow)
Pull request description:
Removes the leading 0 from the version number. The minor version, which we had been using as the major version, is now the major version. The revision, which we had been using as the minor version, is now the minor version. The revision number is dropped. The build number is promoted to being part of the version number. This also avoids issues where it was accidentally not included in the version number.
The CLIENT_VERSION remains the same format as previous as previously, as the Major version was 0 so it never actually got included in it.
The user agent string formatter is updated to follow this new versioning.
***
Honestly I'm just tired of all of the people asking for "1.0" that maybe this'll shut them up. Skip the whole 1.0 thing and go straight to version 22.0!
Also, this means that the terminology we commonly use lines up with how the variables are named. So major versions are actually bumping the major version number, etc.
ACKs for top commit:
jnewbery:
Code review ACK 8f7b930475
MarcoFalke:
review ACK 8f7b93047581c67f2133cdb8c7845471de66c30f 🎻
Tree-SHA512: b5c3fae14d4c0a9c0ab3b1db7c949ecc0ac3537646306b13d98dd0efc17c489cdd16d43f0a24aaa28e9c4a92ea360500e05480a335b03f9fb308010cdd93a436
f1a0314c537791f202dfb7c1209f0e04ba7988c3 gui: change combiner for signals to optional_last_value (Cory Fields)
Pull request description:
[`optional_last_value`](https://www.boost.org/doc/libs/1_73_0/doc/html/boost/signals2/optional_last_value.html), which does not throw, has replaced `last_value` as
Boosts default combiner. Besides being better supported, it also doesn't
trigger gcc's `-Wmaybe-unitialized` warning, presumably because exceptions no
longer bubble-up out of signals:
```bash
In file included from ui_interface.cpp:9:
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
if(value) return value.get();
^
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
optional<T> value;
^~~~~
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
if(value) return value.get();
^
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
optional<T> value;
^~~~~
```
The change in default happened in [Boost 1.39.0](https://www.boost.org/users/history/version_1_39_0.html) (along with the introduction of the Signals2 library.
More information is also available here https://www.boost.org/doc/libs/1_73_0/doc/html/signals2/rationale.html#id-1.3.36.9.4:
> The default combiner for Boost.Signals2 has changed from the last_value combiner used by default in the original Boost.Signals library.
> This is because last_value requires that at least 1 slot be connected to the signal when it is invoked (except for the last_value<void> specialization).
> In a multi-threaded environment where signal invocations and slot connections and disconnections may be happening concurrently, it is difficult to fulfill this requirement. When using optional_last_value, there is no requirement for slots to be connected when a signal is invoked, since in that case the combiner may simply return an empty boost::optional.
ACKs for top commit:
laanwj:
ACK f1a0314c537791f202dfb7c1209f0e04ba7988c3
Tree-SHA512: 3600f85019a3591b141dc9207f8a7e66d16d9996cf97fdf08f5133a212d55c591955ab835ffbdca20b5d62711578bc305d5525c75546fa957f180192e2a80c1e
* Added GET_SNAPSHOT_INFO message handling
* Quorum members by rotation
* Quorum utils functions
* Handle GET_QUORUM_ROTATION_INFO with baseBlockHash from client
* Storing QuorumSnaphots in evoDB when requesting them
* Added DIP Enforcement param
* quorumIndex cache
* Quorum Rotation deployment control
* Usage of Bitsets for storing CQuorumSnapshots
* Correct handling of early quorum quarters
* More asserts
* Corrections
* Handling of quorumIndex
* Refactoring of truncate mechanism
* Various fixes
* Interface correction
* Added template type for indexed cache
* Added quorumIndex into commitmenHash
* Various changes
* Needs to update maqQuorumsCache along with indexedQuorumsCache
* Added CFinalCommitment version 2
* Renamed variables
* Fixes
* Refactoring & correct caching of quorumMembers by rotation
* Added assertions
* Refactoring
* Interface change
* Handling of previous DKG session failure
* Applied refactoring
* Build quarter members improvments
* Merge Quorum Rotation and Decreased fee into one deployment (DIP24)
* Added new LLMQ Type
* Added functional tests + refactoring
* Refactoring
* Spreaded Quorum creation and Quorum Index adaptation
* quorumIndex adaptations
* Added quorumIndex in CFinalCommitment
* Latest work
* Final refactoring
* Batch of refactoring
* Fixes for tests
* Fix for CFinalCommitment
* Fix for Quorums
* Fix
* Small changes
* Thread sync fic
* Safety changes
* Reuse mns when needed
* Refactoring
* More refactoring
* Fixes for rotationinfo handling
* Fix for rotation of members
* Correct order of MNs lists in Quorum Snapshots
* Adding extra logs
* Sync rotation quorums + qrinfo changes
* Fix + extra logs
* Removed redundant field
* Fix for null final commitment + refactoring
* Added timers in tests
* Fix for qrinfo message: quorumdiff and merkleRootQuorums
* Small changes for rotation test
* Remove reading from scanQuorumCache
* Added quorum list output
* Crash fix
* Experimental commit
* apply changes to specialtxman.cpp from specialtx.cpp
* all the changes
* substancially speed up feature_llmq_rotation.py
* reenable asserts, add check for reorgs
* Refactoring
* Added extra logs
* format
* trivial
* drop extra boost includes
* drop ContainsMN
* fix ScanQuorums
* check quorum hash and index in CFinalCommitment::Verify
* fix/tweak tests
* IsQuorumRotationEnabled should be aware of the context
* Calculating members based on earlier block.
* Fix for Quorum Members Cache
* Removed duplicate size of baseBlockHashes
* Adaptations of qrinfo to -8 mn lists
* Introduction of llmqTypeDIP24InstantSend
* Adaptation for llmqTypeDIP24InstantSend
* Adaptations for IS
* bump protocol version
* Added feature_llmq_is_migration test
* Various cleanups
* use unordered_lru_cache for quorumSnapshotCache
* trivial refactor ComputeQuorumMembersByQuarterRotation
* Reduced CFinalCommitment::quorumIndex from 32 to 16 bits
* Keep verified LLMQ relay connections
* Experimental Relay connection fix
* Fix for EnsureQuorumConnections rotation
* Using only valid Mns for checking
* Override of nPowTargetSpacing (devnet only)
* Show penalty score in masternode rpc
* fixups
* Rotation refactoring
* Update src/chainparams.cpp
* Replaced LogPrintf with LogPrint
* IS locking fix once DIP24 activation
* Various cleanup
* Updated MIN_MASTERNODE_PROTO_VERSION
* Introduce LLMQ_TEST_INSTANTSEND reg-test only quorum and actually test switching to dip0024 quorums
* Renamed field lastQuorumHashPerIndex
* Renamed to DIP0024
* chore: update nStartTime and nTimeout for mainnet / testnet for DEPLOYMENT_DIP0024
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
d8dfc403f74858b79c87f650b5d609aa60a4dc3b script: redirecting stderr to stdout before pipelining into grep (anouar kappitou)
30df5c3dd4d39d9027b0341d01d3233400104893 script: preventing non-compatible sed binary. (anouar kappitou)
Pull request description:
This Pull request improve scripted diff by checking for non-compatible sed binary. Fixes#19815
ACKs for top commit:
shaavan:
reACK d8dfc403f74858b79c87f650b5d609aa60a4dc3b
theStack:
Tested ACK d8dfc403f74858b79c87f650b5d609aa60a4dc3b
Tree-SHA512: 4813abb02195e04b8953662e51cba4599789e6185a6d384f56255e2d37c2b16be46c7c8861e44ba1a13d42573a58eb708901472925cb308dba2f602020e9f0f6
4105a543817c47bda21739d5ad3269677f84e861 lint: remove boost::bind linter (fanquake)
Pull request description:
I don't think we need to maintain a linter for reintroducing boost::bind at this point.
ACKs for top commit:
hebasto:
ACK 4105a543817c47bda21739d5ad3269677f84e861, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 86bda91ee7ed11f0aa7ac95e9e7b62dbba626dcea75444d2851a3d40e794ab16bef09a1f0c956a716d43602b23c1cf67e1ff3a51184ea1ee7d686fbb76316cb0
54b5eb2b149a1f2a4a1dbdb9e0648c5a390d8e22 tests: Add std::locale::global to list of locale dependent functions in lint-locale-dependence.sh (practicalswift)
Pull request description:
Add `std::locale::global` to list of locale dependent functions in `lint-locale-dependence.sh`.
We currently flag `setlocale(...)` as locale dependent, but prior to this commit we didn't flag
`std::locale::global(...)` as such.
In addition to setting the global C++ locale `std::locale::global(...)` also does the equivalent of `std::setlocale(LC_ALL, ...);`.
Thus the functionality of `std::locale::global(...)` is a superset of `setlocale(...)` :)
ACKs for top commit:
MarcoFalke:
ACK 54b5eb2b149a1f2a4a1dbdb9e0648c5a390d8e22, fine with me
Tree-SHA512: bcf2f1c765add6ed09c3debca968b75eeea81602503f109c0f76ec98635911d453f4834a39e741703c3d470f123178e8952191a9b1a3429394b99c07765dcf1f
e61de6306fd89fe9aae90253062e7b1b20343f8a Change ismine to take a CWallet instead of CKeyStore (Andrew Chow)
7c611e20007bf5face34d33dffa26c8db67e29ec Move ismine to wallet module (Andrew Chow)
Pull request description:
`IsMine` isn't used outside of the wallet except for the tests. It also doesn't make sense to be outside of the wallet. This PR moves `IsMine` into the wallet module and for it to take a `CWallet` instead of `CKeyStore`. The test that used `IsMine` is also moved to the wallet tests.
This is first [prerequisites](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#ismine) for the wallet structure changes.
ACKs for commit e61de6:
MarcoFalke:
re-ACK e61de6306f (only change is rebase with git auto-merge)
meshcollider:
Very light code review ACK e61de6306f
Tree-SHA512: 1cb4ad12652aef7922ab7460c6d413e8b9d1855dca78c0a286ae49d5c0765bc7996c55f262c742001d434eb9bd4215dc2cc7aae1b371ee1a82d46b32c17e6341
Co-authored-by: MeshCollider <dobsonsa68@gmail.com>
fac86ac7b3ceac2f884412c7a9f4bd5bab5e3916 scripted-diff: Add missed copyright headers (Hennadii Stepanov)
6fde9d5e47fc9a1042b3fb68031eab5bf55e508d script: Update EXLUDE list in copyright_header.py (Hennadii Stepanov)
1998152f15fd2b0e83f5068c375a34feaf73db8c script: Add empty line after C++ copyright (Hennadii Stepanov)
071f2fc204f542c5a287ca8835115a2ee0bf2f50 script: Add ability to insert copyright to *.sh (Hennadii Stepanov)
Pull request description:
This PR improves `contrib/devtools/copyright_header.py` script and adds copyright headers to the files in `src` and `test` directories with two exceptions:
- [`src/reverse_iterator.h`](https://github.com/bitcoin/bitcoin/blob/master/src/reverse_iterator.h) (added to exceptions)
- [`src/test/fuzz/FuzzedDataProvider.h`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h) (added to exceptions)
On master 5622d8f3156a293e61d0964c33d4b21d8c9fd5e0:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
25 with zero copyrights
```
With this PR:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
2 with zero copyrights
```
~I am uncertain about our copyright policy with `build_msvc` and `contrib` directories content, so they are out of scope of this PR.~
ACKs for top commit:
MarcoFalke:
ACK fac86ac7b3ceac2f884412c7a9f4bd5bab5e3916
Tree-SHA512: d7832c4a7a1a3b7806119775b40ec35d7982f49ff0e6199b8cee4c0e0a36e68d51728b6ee9924b1c161df4bc6105bd93391b79d42914357fa522f499cb113fa8
* Merge #16291: gui: Stop translating PACKAGE_NAME
fa64b947bb3075ff8737656706b941af691908ab util: No translation of `Bitcoin Core` in the copyright (MarcoFalke)
fab85208f678ba1be53bdb73a73ce3c5c937d448 qt: Run «make translate» in ./src/ (MarcoFalke)
fabe87d2c923ab3a70b8cde2666a4d1cda8b22fb scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke)
fa5e9f157e568b7fbbea1482b393181f0733f2ba build: Stop translating PACKAGE_NAME (MarcoFalke)
Pull request description:
Generally the package name is not translated, but the package description is.
E.g. `GIMP` or `Firefox` are always called that way regardless of the system language. However, "`Firefox` webbrowser" or "`GIMP` image manipulation program" are translated.
ACKs for top commit:
hebasto:
ACK fa64b947bb3075ff8737656706b941af691908ab, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 626f811531182d0ba0ef1044930d32726773349bcb49b10261288a86ee6b80a183db30a87d817d5b0d501fad058ac22d6272311716b4f5a154f17c6f391a5a1a
* more
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
cb3511b9d Add release notes for importing key origin info change (Andrew Chow)
4c75a69f3 Test importing descriptors with key origin information (Andrew Chow)
02d6586d7 Import KeyOriginData when importing descriptors (Andrew Chow)
3d235dff5 Implement a function to add KeyOriginInfo to a wallet (Andrew Chow)
eab63bc26 Store key origin info in key metadata (Andrew Chow)
345bff601 Remove hdmasterkeyid (Andrew Chow)
bac8c676a Add a method to CWallet to write just CKeyMetadata (Andrew Chow)
e7652d3f6 Add WriteHDKeypath function and move *HDKeypath to util/bip32.{h,cpp} (Andrew Chow)
c45415f73 Refactor keymetadata writing to a separate method (Andrew Chow)
Pull request description:
This PR allows for key origin data as defined by the descriptors document to be imported to the wallet when importing a descriptor using `importmulti`. This allows the `walletprocesspsbt` to include the BIP 32 derivation paths for keys that it is watching that are from a different HD wallet.
In order to make this easier to use, a new field `hdmasterkeyfingerprint` has been added to `getaddressinfo`. Additionally I have removed `hdmasterkeyid` as was planned. I think that this API change is fine since it was going to be removed in 0.18 anyways. `CKeyMetadata` has also been extended to store key origin info to facilitate this.
Tree-SHA512: 9c7794f3c793da57e23c5abbdc3d58779ee9dea3d53168bb86c0643a4ad5a11a446264961e2f772f35eea645048cb60954ed58050002caee4e43cd9f51215097
* refactor: break circular dependencies(-13, +2)
introduces specialtxman, which handles validation of special transactions, specialtx is now simply the primitive underlying type. This breaks a lot of the circular depends
Also removes an unneeded `#include <masternode/payments.h>` in net_processing.cpp, which resolves a circular dependency. (we know it's okay to remove b/c masternode/payments.h isn't included in any header files, and removing it doesn't break compilation)
* format: make clang-format happy
* remove unrelated change
* remove some unneeded includes to `evo/deterministicmns.h`, explicitly include some previously implicitly included includes.
Resolves two circular dependencies
* refactor: remove circular depend, unused include
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
* optimize: somehow optimize circular-dependencies.py
Signed-off-by: pasta <pasta@dashboost.org>
* optimize: use parallel if available to lint in parallel
Signed-off-by: pasta <pasta@dashboost.org>
* suggestions
* more suggestions
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* src/evo/evodb.cpp:57:29: warning: Assert statement calls a function which may have desired side effects: 'IsClean'. [assertWithSideEffect]
* src/llmq/quorums.cpp:635:37: note: Null pointer dereference
src/llmq/quorums.cpp:635:37: warning: Either the condition 'pFrom==nullptr' is redundant or there is possible null pointer dereference: pFrom. [nullPointerRedundantCheck]
src/llmq/quorums.cpp:636:81: note: Assuming that condition 'pFrom==nullptr' is not redundant
* fix a bunch of cppcheck warnings
* cppcheck: run on many more files. Enable all checks except a few ignored ones.
ignored
```
"Consider using std::transform algorithm instead of a raw loop."
"Consider using std::accumulate algorithm instead of a raw loop."
```
* ci: build specific version of cppcheck instead of install from apt
* ci: use cppcheck 2.4, remove commented out line, fix symlink
cppcheck 2.6 is latest, however causes issues
```
src/spork.cpp:135:51: warning: Analysis failed. If the code is valid then please report this failure. [cppcheckError]
```
cppcheck 2.5 appears to get into an infinite loop
* no need to check presence before insertion
* use if-init, remove redundant check
* remove redundant check
* don't remove cmake? fix macOs depends build?
* cppcheck: one per line, alphabetize
* remove duplicate cmake install
faa1e0fb1712b1f94334e42794163f79988270fd qt: test: Create at most one testing setup (MarcoFalke)
Pull request description:
It is assumed that ideally only one BasicTestingSetup exists at any point in time for each process (due to use of globals).
This assumption is violated in the GUI tests, as a testing setup is created as the first step of the `main` function and then (sometimes) another one for the following test cases.
So, the gui tests create two testing setups:
* `BasicTestingSetup` in `main` (added in fa4a04a5a942d582c62773d815c7e1e9897975d0)
* a testing setup for individual test cases
Avoid that by destructing the testing setup in main after creation and then move the explicit `ECC_Stop` to the only places where it is needed (before and after `apptests`).
ACKs for top commit:
laanwj:
code review ACK faa1e0fb1712b1f94334e42794163f79988270fd
Tree-SHA512: b8edceb7e2a8749e1de3ea80bc20b6fb7d4390bf366bb9817206ada3dc8669a91416f4803c22a0e6c636c514e0c858dcfe04523221f8851b10deaf472f107d82
bfe1ba2f5b36056e0c41edf8206b93d3d83098df rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e01cce368d67092de8f0c736927d6f6aa69 build: Accomodate makensis v2.x (Carl Dong)
1f2c39a30e0f82046c7aecddfda3eb99cb536816 guix: Remove logical cores requirement (Carl Dong)
a4f6ffa71e335d4b2a6bf525b7f416968f9cd9f7 lint: Also enable source statements for non-gitian (Carl Dong)
d256f91cb1b0d6ff5170106b99b0266cbe51f5a2 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da02f9684e3fd554b687fb692ae6a23d65a nsis: Specify OutFile path only once (Carl Dong)
14701604d0904bc5bbf1c67de08f8ee6d3215523 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4f48b18f93050d77bcb23f9cf45ec34647 guix: Make source tarball using git-archive (Carl Dong)
395c1137f630dc495ffb2752a23bc1dfd470ee53 gitian: Limit sourced script to just assignments (Carl Dong)
Pull request description:
Based on: #18556
Related: https://github.com/bitcoin/bitcoin/pull/17595#discussion_r399728721
ACKs for top commit:
fanquake:
ACK bfe1ba2f5b36056e0c41edf8206b93d3d83098df - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase #18818.
Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
2aa48edec0101f8a77a2189244fc62722ff7a123 refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be044724bb49d785ca2e296a3b43343c1690 build: Drop make dist in gitian builds (Hennadii Stepanov)
Pull request description:
After the merge of #18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.
With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.
Close#16588.
Close#18547.
As a good side-effect, #18349 becomes redundant.
**Change in behavior**
The following variables 1b151e3ffc/configure.ac (L2-L6)
are no longer used for naming of directories and tarballs.
Instead of them the gitian descriptors use a git tag (if available) or a commit hash.
---
Also a small refactor commit picked from #18404.
ACKs for top commit:
dongcarl:
ACK 2aa48edec0101f8a77a2189244fc62722ff7a123
MarcoFalke:
ACK 2aa48edec0101f8a77a2189244fc62722ff7a123
fanquake:
ACK 2aa48edec0101f8a77a2189244fc62722ff7a123 - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got #18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).
Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
1ac454a3844b9b8389de0f660fa9455c0efa7140 Enable ShellCheck rules (Hennadii Stepanov)
Pull request description:
Enable some simple ShellCheck rules.
Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu.
For local tests the latest `shellcheck` version 0.6.0 should be used (see #15166).
ACKs for top commit:
practicalswift:
utACK 1ac454a3844b9b8389de0f660fa9455c0efa7140
dongcarl:
utACK 1ac454a
fanquake:
ACK 1ac454a3844b9b8389de0f660fa9455c0efa7140
Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e
c4606b84329d760d7cee144bebe05807857edaae Add Travis check for single parameter constructors not marked "explicit" (practicalswift)
Pull request description:
Make single parameter constructors `explicit` (C++11).
Rationale from the developer notes:
> - By default, declare single-argument constructors `explicit`.
> - *Rationale*: This is a precaution to avoid unintended conversions that might
> arise when single-argument constructors are used as implicit conversion
> functions.
ACKs for top commit:
laanwj:
ACK c4606b84329d760d7cee144bebe05807857edaae
Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
* merge 15638: Move CheckTransaction from lib_server to lib_consensus
* merge 15638: Move policy settings to new src/policy/settings unit
* merge 15638: Move rpc utility methods to rpc/util
* merge 15638: Move rpc rawtransaction util functions to rpc/rawtransaction_util.cpp
* merge 15638: Move several units into common libraries
* merge 15638: Move wallet load functions to wallet/load unit
* merge 15638: Document src subdirectories and different libraries
* [build] Add several util units (cleanup)
* build: resolve missing declarations by re-specifying headers
3c5254a820c892b448dfb42991f6109a032a3730 Limit Python linting to files in the repo (practicalswift)
Pull request description:
Limit Python linting to files in the repo.
Before:
```
$ test/lint/lint-python.sh
not_under_version_control.py:195:9: F841 local variable 'e' is assigned to but never used
$
```
After:
```
$ test/lint/lint-python.sh
$
```
ACKs for commit 3c5254:
fanquake:
tACK 3c5254a820
Empact:
utACK 3c5254a820
Tree-SHA512: 68733494a5f2a7764eba938af227145f5ef9ddc9ff94840134e4d2684ca7b9a819fac491ec43102f93e5e9867373bfd46b46efc9d11528329b5ecb2282fffb16
418d3230f8 Resolve the checkpoints <-> validation CD. (251)
Pull request description:
This pull request attempts to resolve the `checkpoints -> validation -> checkpoints` circular dependency.
The circular dependency is resolved by moving the `CheckPoints::GetLastCheckpoint(const CCheckpointData& data)` function to `validation.cpp` where it used exclusively by the private function `ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime)`.
ACKs for commit 418d32:
promag:
utACK 418d323, only `GetLastCheckpoint` usage is in `validation.cpp` and so makes sense to move it there.
practicalswift:
utACK 418d3230f86f77dde6e817f502baff8a54b707fa
MarcoFalke:
utACK 418d3230f86f77dde6e817f502baff8a54b707fa
sipa:
utACK 418d3230f86f77dde6e817f502baff8a54b707fa
Tree-SHA512: 03c3556bc192e65f5e3fa76fd545d4ee7d63d3fb06b132f7a1fa6131aa21ddd2e5b2d19e2222dfe524f422daaca30efde219bed188db8c74ff4b088876b5bc16
* include adjustments
* fix macOs build failure
Signed-off-by: pasta <pasta@dashboost.org>
* expcitly include array in spork.h
* sort includes in most files
* Merge #14726: Use RPCHelpMan for all RPCs
fa5e0452e875a7ca6bf6fe61fdd652d341eece40 rpc: Documentation fixups (MarcoFalke)
fa91e8eda541acdb78ca481b74605639f319c108 Use RPCHelpMan for all RPCs (MarcoFalke)
fa520e72f7b5964cea1ade666e71212914556cf3 lint: Must use RPCHelpMan to generate the RPC docs (MarcoFalke)
Pull request description:
The resulting documentation should not change unless the type in the oneline-summary was previously incorrect. (E.g. string vs bool)
Tree-SHA512: 4ff355b6a53178f02781e97a7aca7ee1d0d97ff348b6bf5a01caa1c96904ee33c704465fae54c2cd7445097427fd04c71ad3779bb7a7ed886055ef36c1b5a1d0
* Dash-specific changes to support RPCHelpMan with RPC commands
Signed-off-by: Dzutte <dzutte.tomsk@gmail.com>
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
* lint: Skip shell linting if gawk is not installed
* lint: Skip Gitian descriptor scripts checking if jq is not installed
* ci: Install gawk and jq
`yq` requires `jq`
* Fix shellcheck warnings
39d526bde48d98af4fa27906e85db0399b6aa8b1 test: Bump linter versions (Duncan Dean)
Pull request description:
As per #19346, `mypy==0.700` was incompatible with Python 3.8.
I've bumped the versions of all the linters to their latest stable versions.
Checked with both Python 3.7 and 3.8 and everything still seems to work fine.
ACKs for top commit:
hebasto:
ACK 39d526bde48d98af4fa27906e85db0399b6aa8b1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: f3ee7fda8095aa25aa68685e863076d52a6b82649770d24b0064d652763c0ceb8ebcbf9024fc74fca45c754e67b2a831dd070b3af23bc099140e6d27e89a5319
3d0a82cff8cbb809876e82dbe62d14d2adc07d94 devtools: Accomodate block-style copyright blocks (Ben Woosley)
0ef0e51fe4bb592e67255776b5a0ba04679fb8c4 lint: Bump flake8 to 3.7.8 (Ben Woosley)
838920704ad90a71cf288b700052503db8abb17e lint: Disable flake8 W504 warning (Ben Woosley)
b21680baf5391a602b295b9d7d0ef66553661cb9 test/contrib: Fix invalid escapes in regex strings (Ben Woosley)
Pull request description:
This is a second go at #15221, fixing new lints in:
W504 line break after binary operator
W605 invalid escape sequence
F841 local variable 'e' is assigned to but never used
This time around:
* One commit per rule, for easier review
* I went with the PEP-8 style of breaking before binary operators
* I looked into the raw regex newline issue, and found that raw strings with newlines embedded do work appropriately. E.g. run `re.match(r" \n ", " \n ")` to check this for yourself. `re.MULTILINE` exists to modify `^` and `$` in multiline scenarios, but all of these searches are per-line.
ACKs for top commit:
practicalswift:
ACK 3d0a82cff8cbb809876e82dbe62d14d2adc07d94 -- diff looks correct
Tree-SHA512: bea0c144cadd72e4adf2e9a4b4ee0535dd91a8e694206924cf8a389dc9253f364a717edfe9abda88108fbb67fda19b9e823f46822d7303c0aaa72e48909a6105
3f8776a1391c3978ed66144df15fd9bcb9edd35d Re-add dead code detection (flack)
Pull request description:
This re-adds unreachable code detection for Python based on `vulture`.
Effectively, this reverts f4beb4996d27f2cdaf4f0a63e7dc044bf17decce. The difference to the previous version is that this runs with the `--min-confidence 100` setting. From https://pypi.org/project/vulture/:
> Use `--min-confidence 100` to only report code that is guaranteed to be unused within the analyzed files.
So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a `return` statement). As such, there is not suppressions list.
My motivation was mainly #21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because #21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place
ACKs for top commit:
practicalswift:
ACK 3f8776a1391c3978ed66144df15fd9bcb9edd35d
Tree-SHA512: 52314ad4f627d969de1eb15375ca677ed86a2e816fe773756a1ce22421214ba407b5a09a4bf701a3aab1a10c7b336f548e4cef3327edf154acba55e987db21f6
f4beb4996d27f2cdaf4f0a63e7dc044bf17decce test: Remove python dead code linter (Wladimir J. van der Laan)
Pull request description:
Primarily I'd like to remove this because it is very imprecise, due to Python's dynamic nature, giving it a large list of false positives that need to be listed as exceptions. See for example #16906.
It's also a frequent source of complaints. I'm doubtful of the usefulness of checking for dead code in a linter in the first place.
Having some dead code in the test framework for a while is not a
disaster.
ACKs for top commit:
sdaftuar:
utACK f4beb4996d27f2cdaf4f0a63e7dc044bf17decce
practicalswift:
ACK f4beb4996d27f2cdaf4f0a63e7dc044bf17decce -- diff looks correct
jamesob:
ACK f4beb4996d
Tree-SHA512: 329b1555210311d5d15799fd2cb794b3208b0ac4d8a2ffaf4dece1bcc3e0e8b1fe952d5e7a394f94a98919cab579fb579eae7db2a796cc9a1a42ef495dd17507
fac35b21e2c2d798face7e289dcff4c1cce0e1a6 test: lint: Add DisabledOpcodeTemplates to whitelist (MarcoFalke)
Pull request description:
Fixes#16906
Top commit has no ACKs.
Tree-SHA512: 98b175bb062425fd3a8bd0d0258f4c0e0d5106980f1e037df7c2b2b2e5aa6031b11b582c026265d7b2de56049ccbadb0b7add9130d323f15886f681c6268ba0a
72a18a73af26ea551d39397787a2d178c8bbde7b tests: Add information on how to add Vulture suppressions (practicalswift)
Pull request description:
Add information on how to add `vulture` suppressions.
As requested by MarcoFalke in https://github.com/bitcoin/bitcoin/issues/16906#issuecomment-533264107 -- your wish is my command! :)
ACKs for top commit:
fanquake:
ACK 72a18a73af26ea551d39397787a2d178c8bbde7b - similar sort of message as in [lint-spelling.sh](https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-spelling.sh).
Tree-SHA512: b347f8cea33d4b0ba987a972979b0ac3423938084fea923a2c457a8081bc839a94ad818689d147477104b9197dc35be413f76a96026cd1507b4411d7513e3464
b748bf6f50dda6bb57eadf697edc320b2695e01a Fix spelling errors identified by codespell 1.15.0 (Ben Woosley)
Pull request description:
Note all changes are to comments / documentation.
After this commit, the only remaining output is:
```
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ 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
```
Note:
* I ignore several valid alternative spellings ~, but changed homogenous
to homogeneous as the latter is a more specific term according to the
Google dictionary definitions I found~
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
ACKs for commit b748bf:
practicalswift:
ACK b748bf6f50dda6bb57eadf697edc320b2695e01a
fanquake:
ACK b748bf6f50
Tree-SHA512: 9add7044643ce015e0a44d8b27a3f300d72c485ffff550fb6491a17f14528085289ec5caddfe02f291ea9b2cded38a0dd3079652a054e2d7fe2ff4f7b53db5d7
* bls: use constexpr int instead of #define
* lint: bump c++ version to 17
* test: use BOOST_CHECK_EQUAL instead of BOOST_ASSERT, call boost assert in another location
* coinjoin: fix typo
* drop redundant LLMQType cast
* use numeric_limits instead of magic value
* net_processing.cpp whitespace fixes
* Add some const
* use std::all_of instead of raw for loop
* Introduce UNINITIALIZED_SESSION_ID and use it instead of a magic number
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
