## Issue being fixed or feature implemented
same as #5392, alternative solution
~based on #5402 atm, will rebase later~
## What was done?
pls see individual commits
## How Has This Been Tested?
reorg mainnet around forkpoint with a patched client (to allow low
difficulty), run tests
## Breaking Changes
Another evodb migration is required. Going back to an older version or
migrating after the fork requires reindexing.
## 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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Many objects created and functions called by passing `const
std::unique_ptr<Obj>& obj` instead directly passing `Obj& obj`
In some cases it is indeed needed, but in most cases it is just extra
complexity that is better to avoid.
Motivation:
- providing reference to object instead `unique_ptr` is giving warranty
that there's no `nullptr` and no need to keep it in mind
- value inside unique_ptr by reference can be changed externally and
instead `nullptr` it can turn to real object later (or in opposite)
- code is shorter but cleaner
Based on that this refactoring is useful as it reduces mental load when
reading or writing code.
`std::unique` should be used ONLY for owning object, but not for passing
it everywhere.
## What was done?
Replaced most of usages `std::unique_ptr<Obj>& obj` to `Obj& obj`.
Btw, in several cases implementation assumes that object can be nullptr
and replacement to reference is not possible.
Even using raw pointer is not possible, because the empty
std::unique_ptr can be initialized later somewhere in code.
For example, in `src/init.cpp` there's called `PeerManager::make` and
pass unique_ptr to the `node.llmq_ctx` that would be initialized way
later.
That is out of scope this PR.
List of cases, where reference to `std::unique_ptr` stayed as they are:
- `std::unique_ptr<LLMQContext>& llmq_ctx` in `PeerManagerImpl`,
`PeerManager` and `CDSNotificationInterface`
- `std::unique_ptr<CDeterministicMNManager>& dmnman` in
`CDSNotificationInterface`
Also `CChainState` have 3 references to `unique_ptr` that can't be
replaced too:
- `std::unique_ptr<llmq::CChainLocksHandler>& m_clhandler;`
- `std::unique_ptr<llmq::CInstantSendManager>& m_isman;`
- `std::unique_ptr<llmq::CQuorumBlockProcessor>&
m_quorum_block_processor;`
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
No breaking changes, all of these changes - are internal APIs for Dash
Core developers only.
## 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
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
5aab011805ceb12801644170700b1a62e0bf4a5d test: add unit test for non-standard "scriptsig-not-pushonly" txs (Sebastian Falbesoner)
Pull request description:
Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason "scriptsig-not-pushonly" if any one of the input's scriptSig consists of any other ops than just PUSHs.
ACKs for top commit:
MarcoFalke:
ACK 5aab011805ceb12801644170700b1a62e0bf4a5d 🍟
practicalswift:
ACK 5aab011805ceb12801644170700b1a62e0bf4a5d -- patch looks correct
Tree-SHA512: fbe25bcf57e5f0c8d2397eb67e61fe8d9145ba83032789adb2b67d6fcbcd87e6427e9d965e8cd7bbaaea482e39ec2f110f71ef2de079c7d1fba2712848caa9ba
5e7e4c9f6e57f5333bd17a20b0c85a78d032998e refactor: replace RecursiveMutex g_maplocalhost_mutex with Mutex (w0xlt)
a7da1409bc9f614009f76c1bfc55f029ff1265e4 scripted-diff: rename cs_mapLocalHost -> g_maplocalhost_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex cs_mapLocalHost`.
ACKs for top commit:
shaavan:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e
theStack:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e
hebasto:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 961171e346fe385e16db9830115a8096f4ca2499bbea11a08c02ca808638dfb63c434ab9d66392c71e85be6352c8a2b6a0054b5a61aaabd28d71581fed5beae7
10efc0487c442bccb0e4a9ac29452af1592a3cf2 Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc9b7dbbda63e00195e0a962f51e4d2c Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef2892478c28cd660e53223c6dd1dc0187 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 🐱
ajtowns:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
7e80f646b24a2abf3c031a649bcc706a695f80da Get the OutputType for a descriptor (Andrew Chow)
Pull request description:
Adds a `GetOutputType()` method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an `Optional<OutputType>`. For descriptors with indeterminate OutputType, we return `nullopt`.
`addr()` and `raw()` use OutputTypes as determined by the CTxDestination they have. For simplicity, `ScriptHash` destinations are `LEGACY` even though they could be `P2SH_SEGWIT`.
`combo()`, `pk()`, and `multi()` are `nullopt` as they either don't have an OutputType or they have multiple. `DescriptorImpl` defaults to `nullopt`.
`pkh()` is `LEGACY` as expected
`wpkh()` and `wsh()` are `BECH32` as expected.
`sh()` checks whether the sub-descriptor is `BECH32`. If so, it is `P2SH_SEGWIT`. Otherwise it is `LEGACY`.
The descriptor tests are updated to check the OutputType too.
ACKs for top commit:
fjahr:
ACK 7e80f646b24a2abf3c031a649bcc706a695f80da
meshcollider:
utACK 7e80f646b24a2abf3c031a649bcc706a695f80da
instagibbs:
cursory ACK 7e80f646b2
Sjors:
Code review ACK 7e80f646b24a2abf3c031a649bcc706a695f80da
jonatack:
ACK 7e80f64 code review/build/tests
Tree-SHA512: c5a813447b62e982435e1c948066f8d6c148c9ebffb0a5eb5a9028b173b01d5ead2f076a5ca3f7f37698538baa346f82a977ee48f583d89cb4e5ebd9111b2341
0e0fa27acb74b4f0075afcf59a0dff51a21baddb Get rid of VARINT default argument (Pieter Wuille)
Pull request description:
This removes the need for the non-strandard use of variadic macros.
ACKs for top commit:
ryanofsky:
Code review ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb. Only change since last review reverting outdated documentation change from earlier version of pr
jonatack:
ACK 0e0fa27 code review, built/ran tests/bitcoind
practicalswift:
ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb -- diff looks correct
MarcoFalke:
ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb 📯
Tree-SHA512: 6e335e4b586d62112b7260a12481cd949d1b3bbdb83edf8db690348f0a01852e68504336ff3e072e5131a7c8cb404ef11a2f786f842b8d08bbf6ea0e688777b1
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
## Issue being fixed or feature implemented
This changes are follow up for backport bitcoin/bitcoin#16060
## What was done?
Buried all hardened dash deployments
## How Has This Been Tested?
Run unit/functional tests.
Run dash with option `-reindex` for both mainnet/testnet - both succeed.
## Breaking Changes
No breaking changes, it should be fully compatible.
## 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
We had forgotten to harden dip20 and dip24 activation
## What was done?
Hardened dip20 and dip24 activation
## How Has This Been Tested?
Hasn't yet; should do an assumevalid=0 reindex
## Breaking Changes
Hopefully 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
- [ ] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
3bf9d8cac09fc88727ef2f2a2bea33b90b625e50 Testchains: Qt: Simplify network/chain styles (Jorge Timón)
052c54ecb02695e5d2694e8e0cbf5ccc89de86e8 Testchains: Generic selection with -chain=<str> in addition of -testnet and -regtest (Jorge Timón)
Pull request description:
Separated from #8994 as suggested by MarcoFalke and Sjors in https://github.com/bitcoin/bitcoin/pull/8994#issuecomment-522555390
You can't really test the qt changes on their own, so to test them, use #8994 .
ACKs for top commit:
MarcoFalke:
ACK 3bf9d8cac09fc88727ef2f2a2bea33b90b625e50
Tree-SHA512: 5b5e6083ebc0a44505a507fac633e7af18037c85e5e73f5d1e6f7e730575d3297ba8a31d1c2441df623b273f061c32d8fa324f4aa6bead01d23e88582029b568
## Issue being fixed or feature implemented
The benefits of using custom struct `maybe_error` is less significant
since the interface `TxValidationState` became simpler after
refactorings from bitcoin#15921 and bitcoin#17004
The refactoring of `maybe_error` as a class result from PR #5109 is
still useful but not for case of TxValidationState.
## What was done?
Unified using TxValidationState in dash's related code.
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
No breaking changes, logic 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
78e283e656bf1643944ffdb76185f3468eb25895 [test] move wallet helper functions into test library (Martin Zumsande)
f613e5dfdafe708f63ebb5193c44e2bc770c6651 [test] move mining helper functions into test library (Martin Zumsande)
2cb4e8bdc7ef75ae8d95c246af1e8e1f9c7045bd [test] move string helper functions into test library (Martin Zumsande)
Pull request description:
This disbands `test/util.h` and `test/util.cpp` and moves the content into the test utility library recently created in #17542, so that all test utility functions are in one place.
The content of the original files are split into three modules:
1) string helper functions go to `test/util/str`
2) mining helper functions go to the newly created `test/util/mining`
3) wallet helper functions go to the newly created `test/util/wallet`
ACKs for top commit:
MarcoFalke:
ACK 78e283e656bf1643944ffdb76185f3468eb25895 🔧
Tree-SHA512: f182a61e86e76c32bcb84e37f44904d3a4a9c5a321f7a8efdda5368a6623cb8b5a5384ec4f96e67f0357b0c22099f6e3ecd0ac4cb467e3fa3f3128f8d36edfb8
3bd8db80d8d335ab63ece4f110b0fadd562e80b7 [validation] fix comments in CheckInputScripts() (John Newbery)
6f6465cefcd599c89c00f7b51f42a4b87a5ffb0b scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery)
Pull request description:
CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e074, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().
Also fix incorrect comments.
ACKs for top commit:
MarcoFalke:
re-ACK 3bd8db80d8d335ab63ece4f110b0fadd562e80b7, did the rebase myself, checked the scripted diff 👡
promag:
ACK 3bd8db80d8d335ab63ece4f110b0fadd562e80b7 :trollface:
Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c
e9fd366044e271632dc0e4f96e1c14f8e87213ae refactor: Remove null setting check in GetSetting() (Russell Yanofsky)
cba2710220d76bbe790b04088839cbbd410436de scripted-diff: Remove unused ArgsManager type flags in tests (Russell Yanofsky)
425bb307252cf4dec9b3ef6426e6548b2be7a303 refactor: Add util_CheckValue test (Russell Yanofsky)
0fa54358b06b58f4d17073bcc8a959eb9498aadc refactor: Add ArgsManager::GetSettingsList method (Russell Yanofsky)
3e185522ace1678e0a25b9cf8a5553a4bc279bea refactor: Get rid of ArgsManagerHelper class (Russell Yanofsky)
dc0f1480746b34aa3ca2d9c0f1ec764083026b40 refactor: Replace FlagsOfKnownArg with GetArgFlags (Russell Yanofsky)
57e8b7a7273567aa4a4aee87cce18e9bff8f3196 refactor: Clean up includeconf comments (Russell Yanofsky)
3f7dc9b808316c1e5d677af8d9a99112568c8ccb refactor: Clean up long lines in settings code (Russell Yanofsky)
Pull request description:
This PR doesn't change behavior. It just implements some suggestions from #15934 and #16545 and few other small cleanups.
ACKs for top commit:
jnewbery:
Code review ACK e9fd366044e271632dc0e4f96e1c14f8e87213ae
MarcoFalke:
ACK e9fd366044 🚟
Tree-SHA512: 6e100d92c72f72bc39567187ab97a3547b3c06e5fcf1a1b74023358b8bca552124ca6a53c0ab53179b7f1329c03d9a73faaef6d73d2cd1a2321568a0286525e2
356988e200b1debaa80d210d502d2d085c72dc64 util: make EncodeBase58Check consume Spans (Sebastian Falbesoner)
f0fce0675d56b2226a993253731690ca864066c8 util: make EncodeBase58 consume Spans (Sebastian Falbesoner)
Pull request description:
This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks).
ACKs for top commit:
laanwj:
Code review ACK 356988e200b1debaa80d210d502d2d085c72dc64
Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
b6d2183858975abc961207c125c15791e531edcc Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2 added asserts to check m_addr_known when it's used (User)
090b75c14be6b9ba2efe38a17d141c6e6af575cb p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
3645e4ca0033bb6365f41ef710111780c139370f Add missing newline in util_ChainMerge test (Russell Yanofsky)
Pull request description:
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin 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 Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
laanwj:
ACK 3645e4ca0033bb6365f41ef710111780c139370f
practicalswift:
ACK 3645e4ca0033bb6365f41ef710111780c139370f -- diff looks correct
Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165
5b44a75493a1a098404d5e21dc384e74eae1892e refactor: Remove unused CExt{Pub,}Key (de)serialization methods (Sebastian Falbesoner)
Pull request description:
As pointed out in issue #17130, the serialization/deserialization methods for the classes `CExtKey` and
`CExtPubKey` are only used in the BIP32 unit tests and hence can be removed (see comments https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543750290, https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543794408 and https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543814727).
ACKs for top commit:
practicalswift:
ACK 5b44a75493a1a098404d5e21dc384e74eae1892e -- -60 LOC diff looks correct :)
promag:
ACK 5b44a75493a1a098404d5e21dc384e74eae1892e.
MarcoFalke:
unsigned ACK 5b44a75493a1a098404d5e21dc384e74eae1892e
fjahr:
ACK 5b44a75
jonatack:
Light ACK 5b44a75493a1a098404d5e21dc384e74eae1892e. Built, ran tests and bitcoind. `git blame` shows most of the last changes are from commit 90604f16af in 2015 to add bip32 pubkey serialization.
Tree-SHA512: 6887573b76b9e54e117a076557407b6f7908719b2202fb9eea498522baf9f30198b3f78b87a62efcd17ad1ab0886196f099239992ce7cbbaee79979ffe9e5f2c
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
9075d13153ce06cd59a45644831ecc43126e1e82 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec0f06fb4fce1c4f93500752f05dede8 [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34ff2d538d8f5315efd9908bf24d0fdc [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714323c1694c834fdca74f064a1a33529 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99fc8cee30ba5976dc36b47b1f6532ab [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153ce06cd59a45644831ecc43126e1e82, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153ce06cd59a45644831ecc43126e1e82. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
faa86b71acefc8f2e366746a1c251888e6e686dd fuzz: Use ConsumeUInt256 helper to simplify rolling_bloom_filter fuzz test (MarcoFalke)
aaaa61fd306e25379e6222e31bf160a6eb04f74e fuzz: Speed up rolling_bloom_filter fuzz test (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log](https://github.com/bitcoin/bitcoin/files/6822159/clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log)
ACKs for top commit:
practicalswift:
cr ACK faa86b71acefc8f2e366746a1c251888e6e686dd
theStack:
Concept and code review ACK faa86b71acefc8f2e366746a1c251888e6e686dd
Tree-SHA512: eace588509dfddb2ba97baf86379fa713fa6eb758184abff676cb95807ff8ff36905eeaddeba05665b8464c35c57e2138f88caec71cbfb255e546bbe76558da0
faafda232e1d4f79ee64dbfee699a8018f25b0bc fuzz: Speed up prevector fuzz target (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35981
ACKs for top commit:
practicalswift:
cr ACK faafda232e1d4f79ee64dbfee699a8018f25b0bc
Tree-SHA512: 1bf166c4a99a8ce88bdc030cd6a32ce1da5251b73873772e0e9c001ec2bacafebb183f7c8c88806d0ab633aada2cff8b78791f5c9c0c6f2cc8ef5f0875c4b2ef
aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 fuzz: Extend addrman fuzz test with deserialize (MarcoFalke)
Pull request description:
Requested on IRC:
```
[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely
ACKs for top commit:
jonatack:
ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 per `git diff fa74025 aaaa9c6`
vasild:
ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0
Tree-SHA512: f57d0aecf22a933e48d3181d7398218949588dd0de31218d1d28c825649e55fd60b0de6fbc92d2497cf5639a4adc2061c9bf8216546a2be916feac4f03f16e8f
906d7913117c8f10934b37afa27ae8ac565da042 fuzz: add missing ECCVerifyHandle to base_encode_decode (Andrew Poelstra)
Pull request description:
It is possible to trigger a fuzztest failure in the `base_encode_decode` by asking it to decode any PSBT that has HD keypaths in it. For example, this one
```
cHNidP8BAFUCAAAAASeaIyOl37UfxF8iD6WLD8E+HjNCeSqF1+Ns1jM7XLw5AAAAAAD/////AaBa6gsAAAAAGXapFP/pwAYQl8w7Y28ssEYPpPxCfStFiKwAAAAAAAEBIJVe6gsAAAAAF6kUY0UgD2jRieGtwN8cTRbqjxTA2+uHIgIDsTQcy6doO2r08SOM1ul+cWfVafrEfx5I1HVBhENVvUZGMEMCIAQktY7/qqaU4VWepck7v9SokGQiQFXN8HC2dxRpRC0HAh9cjrD+plFtYLisszrWTt5g6Hhb+zqpS5m9+GFR25qaAQEEIgAgdx/RitRZZm3Unz1WTj28QvTIR3TjYK2haBao7UiNVoEBBUdSIQOxNBzLp2g7avTxI4zW6X5xZ9Vp+sR/HkjUdUGEQ1W9RiED3lXR4drIBeP4pYwfv5uUwC89uq/hJ/78pJlfJvggg71SriIGA7E0HMunaDtq9PEjjNbpfnFn1Wn6xH8eSNR1QYRDVb1GELSmumcAAACAAAAAgAQAAIAiBgPeVdHh2sgF4/iljB+/m5TALz26r+En/vykmV8m+CCDvRC0prpnAAAAgAAAAIAFAACAAAA=
```
which I took straight from the PSBT test vectors. The reason is that in src/psbt.h we call `DeserializeHDKeypaths`, which in turn calls `CPubKey::IsFullyValid`, which in turn asserts that a secp context has been created.
The error appears to be masked on many systems by the definition of `instance_of_eccryptoclosure` in src/script/bitcoinconsensus.cpp, which defines a static object which contains an `ECCVerifyHandle`. If you just comment out that line you can reliably trigger the fuzz test failure, e.g. by creating a file `crash` with the above PSBT, and runnnig
```
ASAN_OPTIONS=symbolize=0:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1 UBSAN_OPTIONS=suppressions=./test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1 FUZZ=base_encode_decode ./src/test/fuzz/fuzz -seed_inputs=crash
```
ACKs for top commit:
practicalswift:
cr ACK 906d7913117c8f10934b37afa27ae8ac565da042
Tree-SHA512: b98b60573c21efe28503fe351883c6f0d9ac99d0dd6f100537b16ac53476617b8a3f899faf0c23d893d34a01b3bbe4a784499ec6f9c7000292e850bed449bd85
9550dffa0c61df6d1591c62d09629b4c5731e1b7 fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner)
Pull request description:
This PR is a (quite late) follow-up to #19237 (https://github.com/bitcoin/bitcoin/pull/19237#issuecomment-642203251). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold.
ACKs for top commit:
jamesob:
crACK 9550dffa0c pending CI
Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55
fa483e9f68b8b4171dabb25cc88dc2eada454a99 fuzz: Speed up crypto fuzz target (MarcoFalke)
Pull request description:
May fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34962
Similar solution to https://github.com/bitcoin/bitcoin/pull/22005
ACKs for top commit:
practicalswift:
cr ACK fa483e9f68b8b4171dabb25cc88dc2eada454a99: patch looks correct and rationale makes sense
Tree-SHA512: 3788cf9f6ba0f7a0a217cd3a6a825839689425e99e4d6d657981d291a001b0da7c5abb50a68b4ee1c2a8300b87fb92e4e3ccc1171907792b40251e467c33bd53
1be6267ce1ee142c3b90baed1925a82eab6514aa fuzz: don't try and use fopencookie when building for Android (fanquake)
Pull request description:
When building for Android, `_GNU_SOURCE` will be defined:
```bash
/home/ubuntu/android-sdk/ndk/22.1.7171670/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android30-clang++ -dM -E -x c++ - < /dev/null
#define _GNU_SOURCE 1
#define _LP64 1
#define __AARCH64EL__ 1
#define __AARCH64_CMODEL_SMALL__ 1
#define __ANDROID_API__ 30
#define __ANDROID__ 1
#define __ARM_64BIT_STATE 1
.....
```
but it doesn't have the [`fopencookie()` function](https://www.gnu.org/software/libc/manual/html_node/Streams-and-Cookies.html), or define the `cookie_io_functions_t` type, which results in compile failures:
```bash
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:388:15: error: unknown type name 'cookie_io_functions_t'
const cookie_io_functions_t io_hooks = {
^
15 warnings and 1 error generated.
```
Just skip trying to use it if we are building for Android. Should fix#22062.
ACKs for top commit:
practicalswift:
cr ACK 1be6267ce1ee142c3b90baed1925a82eab6514aa
Tree-SHA512: d62f63d0624af04b76c7e07b0332c71eca2bf9cd9e096a60aea9e212b7bbc1548e9fa9a76d065ec719bb345fe8726619c3bd2d0631f54d877c82972b7b289321
fa18553d382a7d8c447cd6698b36e293fb7ecf1f fuzz: Remove addrdb fuzz target (MarcoFalke)
Pull request description:
The target has several issues:
* It is named incorrectly (`addrdb`, but it constructs a `CBanEntry`)
* It doesn't do anything meaningful, other than consuming one integer and passing it to a constructor
* It consumes CPU time that can be used for the other targets
* It is redundant with the banman fuzz target
Fix all by removing it.
ACKs for top commit:
amitiuttarwar:
ACK fa18553d382a7d8c447cd6698b36e293fb7ecf1f, thanks for the cleanup
Tree-SHA512: 3f8944d3f80913bf466c03062fed070e96073fb72d0938b2bc9a2586960c86879d6f251e16fd81cfeb4e6685ff9eef6bccb25cd3901b218a100c90f25a3c9240
37ae687f95c82f2d64ed880533d158060d4fc3de Add tests for CPubKey serialization/unserialization (Elichai Turkel)
9b8907faded8e4ec312c0dd4b4b15e1793876acd Check size after Unserializing CPubKey (Elichai Turkel)
Pull request description:
Found by practicalswift, closes#19235
Currently all the public API(except the pointer-like API) in CPubKey that sets/constructs a pubkey goes through `CPubKey::Set` which checks if that the length and size match and if not invalidates the key.
This adds the same check to `CPubKey::Unserialize`, sadly I don't see an easy way to just push this to the existing checks in `CPubKey::Set` but it's only a simple condition.
The problem with not invalidating is that if you write a pubkey like: `{0x02,0x00}` it will think the actual length is 33(because of `size()`) and will access uninitialized memory if you call any of the functions on CPubKey.
ACKs for top commit:
practicalswift:
re-ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
jonatack:
Code review re-ACK 37ae687 per `git diff eab8ee3 37ae687` only change since last review at eab8ee3 is passing the `pubkey` param by reference to const instead of by value in `src/test/key_tests.cpp::CmpSerializationPubkey`
MarcoFalke:
ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
Tree-SHA512: 30173755555dfc76d6263fb6a59f41be36049ffae7b4e1b92b922d668f5e5e2331f7374d5fa10d5d59fc53020d2966156905ffcfa8b8129c1f6d0ca062174ff1
