4308aa67e3 tests: Add fuzzing harness for functions in net_permissions.h (practicalswift)
43ff0d91f8 tests: Add fuzzing harness for functions in timedata.h (practicalswift)
a8695db785 tests: Add fuzzing harness for functions in addrdb.h (practicalswift)
Pull request description:
Add fuzzing harnesses for functions in `addrdb.h`, `net_permissions.h` and `timedata.h`.
Top commit has no ACKs.
Tree-SHA512: ea41431e7f1944ecd0c102e6ea04e70d6763dc9b6e3a0949a4f7299897a92fa3e8e7139f9f65b9508ce8d45613ea24ec0fd6d4a8be3cfd7c23136512b17770eb
5aab011805 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 5aab011805🍟
practicalswift:
ACK 5aab011805 -- patch looks correct
Tree-SHA512: fbe25bcf57e5f0c8d2397eb67e61fe8d9145ba83032789adb2b67d6fcbcd87e6427e9d965e8cd7bbaaea482e39ec2f110f71ef2de079c7d1fba2712848caa9ba
fa92af5af3 ci: Run feature_block and feature_abortnode in valgrind (MarcoFalke)
fa01febeaf test: Remove ci timeout restriction in test_runner (MarcoFalke)
Pull request description:
Also revert commit 0a4912e46a, because some tests take too long for this to be useful anymore.
Top commit has no ACKs.
Tree-SHA512: 363f14766e1f4a5860ab668a516b41acebc6fbdf11d8defb3a95a772dbf82304ca1f5f14b1dbad97f2029503e03d92e8c69df0466a8872409c20665838f617ed
f0dfac7da3 test: add executable flag for rpc_estimatefee.py (Sebastian Falbesoner)
Pull request description:
Again a functional test without executable flag set sneaked in (see e.g. https://github.com/bitcoin/bitcoin/pull/17806 and https://github.com/bitcoin/bitcoin/pull/16742 for previous similar PRs, setting the filemode from 644 to 755). Maybe a linter like suggested in https://github.com/bitcoin/bitcoin/pull/17830 would be worth considering to avoid future (trivial) PRs like this?
ACKs for top commit:
promag:
ACK f0dfac7da3.
kristapsk:
ACK f0dfac7da3
Tree-SHA512: b37c11bdef439aa9d5736c9e0e0bbcc19aff876744f0c4e099ca5c67c9ff1293f1f9140f0d167ea13fee5396ae017aa4a0f1bae4f7aec8fa80b46beb421561c1
d18bf0c0b0 rpc: add missing HelpExampleRpc for getblockfilter (Sebastian Falbesoner)
Pull request description:
From all RPCs in the "blockchain" category, `getblockfilter` is the only one where there is only a CLI example present but not a curl RPC example (all other RPCs in this category have either both or none). This PR adds the missing `HelpExampleRpc` string.
ACKs for top commit:
emilengler:
utACK d18bf0c
Tree-SHA512: b37c11bdef439aa9d5736c9e0e0bbcc19aff876744f0c4e099ca5c67c9ff1293f1f9140f0d167ea13fee5396ae017aa4a0f1bae4f7aec8fa80b46beb421561c1
b492684063 doc: Temporary note that release notes should be edited in wiki (Wladimir J. van der Laan)
Pull request description:
Replace release notes with temporary note that 0.20.0 release notes should be edited in wiki.
ACKs for top commit:
MarcoFalke:
ACK b492684063
Tree-SHA512: 7a8835f7807e3cd6e4fea2969cf4dfa21d2aab2be7bfc1a6403926ea60e1193573e967d7ff512a640395e06de4877fec7f7a5c48619856f69fd5f894d27f1875
Replace by privateKeysDisabled method to avoid need for GUI to reference
internal wallet flags.
Also remove adjacent WalletModel canGetAddresses wrapper that serves no purpose
and make Wallet::canGetAddresses non-const so it can be implemented by IPC
classes in #10102.
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [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 e57980b473, 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
84a46a9b93 doc: mention MAKE=gmake workaround when building on a BSD (emu)
Pull request description:
Fixes: #14404.
Replaces: #18129.
ACKs for top commit:
vasild:
ACK 84a46a9b93
laanwj:
ACK 84a46a9b93
Tree-SHA512: 7a28c17c5d8a5d98aaedfb849d10a3a809f0d6d4b8f03add2cd6927e9d9689613b8b5c53e62d8e0fce8f4732efcee9ed3a83b0ed325b38934ceff6057a6db163
e90e3e684f build: fix sysctl() detection on macOS (fanquake)
Pull request description:
[`sysctl()` on *BSD](https://www.unix.com/man-page/FreeBSD/3/sysctl/) takes a "const int *name", whereas [`sysctl()` on macOS](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/sysctl.3.html)
it takes an "int *name". So our configure check and `sysctl()` detection on
macOS currently fails:
```bash
/usr/include/sys/sysctl.h:759:9: note: candidate function not viable:
no known conversion from 'const int [2]' to 'int *' for 1st argument
int sysctl(int *, u_int, void *, size_t *, void *, size_t);
```
The simplest change seems to be to change the param to a "int *name", which
will work during configure on macOS and *BSD systems.
For consistency I've changed both calls, but note that macOS doesn't
have `KERN_ARND`, so that check will always fail regardless. We can revert/add
documentation if preferred.
ACKs for top commit:
laanwj:
Re-ACK e90e3e684f
Tree-SHA512: 29e9348136fc72882f63079bf10d2490e845d7656aae2c003e282bea49dd2778204a7776a67086bd88c2852af9a07dd04ba358eede7e37029e1c10f73c85d6a5
a733ad514a Add bn2vch test to functional tests (Pieter Wuille)
a3ad6459b7 Simplify bn2vch using int.to_bytes (Pieter Wuille)
Pull request description:
Alternative to #18374, fixing the incorrect padding added sometimes in `bn2vch`.
Since we're using Python 3.2+, a much simpler implementation of `bn2vch` is possible using `int.to_bytes`.
This also adds a "functional" test for bn2vch, in a new "framework_test_script.py", where the "framework_test_" prefix is intended for tests of the framework itself.
ACKs for top commit:
laanwj:
nice, ACK a733ad514a
jnewbery:
Tested ACK a733ad514a.
Tree-SHA512: aeacc4e7fd84279023d38e8b4a5175fb16d7b3a7f93c61b9dcb59cd9927547732983c76f28564b62e37088399fc0121b38a514d73b0ea38b3983836539e9ca90
sysctl() on *BSD takes a "const int *name", whereas sysctl() on macOS
it takes an "int *name". So our configure check and sysctl() detection on
macOS currently fails:
```bash
/usr/include/sys/sysctl.h:759:9: note: candidate function not viable:
no known conversion from 'const int [2]' to 'int *' for 1st argument
int sysctl(int *, u_int, void *, size_t *, void *, size_t);
```
This change removes the name argument from the sysctl() detection check,
meaning we will detect correctly on macOS and *BSD.
For consistency we also switch to using the more generic, non-const
version of the name parameter in the rest of our usage.
7d8e1dec3b net: fix use-after-free in tests (Vasil Dimov)
Pull request description:
In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda
function to execute later, capturing the local variable
`consensusParams` by reference.
Presumably this was considered safe because `consensusParams` is a
reference itself to a global variable which is not supposed to change,
but it can in tests.
Fixes https://github.com/bitcoin/bitcoin/issues/18372
<!--
*** 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:
sipa:
ACK 7d8e1dec3b
practicalswift:
ACK 7d8e1dec3b
MarcoFalke:
ACK 7d8e1dec3b
Tree-SHA512: fe0f6e5fac1976d38dfb249517eef142dcb8837e178d7d199e5e854e3ab428822c6da9d96fe312293d39b6c6cac0c97896f3b5760013db200cccd729ae1b0710
fa3fa27c45 fuzz: Remove option --export_coverage from test_runner (MarcoFalke)
aaaa055ff7 fuzz: Add option to merge input dir to test runner (MarcoFalke)
fa4fa88d76 doc: Remove --disable-ccache from docs (MarcoFalke)
Pull request description:
This is mainly useful for myself to merge pull requests like https://github.com/bitcoin-core/qa-assets/pull/4
I thought it wouldn't hurt to share the code.
Also remove the `--disable-ccache` from the docs to speed up builds when developing fuzzers.
Top commit has no ACKs.
Tree-SHA512: 818d85a90db86a7f4e8b001cc88342e5b28b02029d2bd4174440b28a8c4cc29b5406bd6348f72ddf909bb3d0f9bf7b1011976f6480e4418c8b7da5ecccae93e8
5e47b19e50 tests: Add harness which fuzzes EvalScript and VerifyScript using a fuzzed signature checker (practicalswift)
Pull request description:
Add harness which fuzzes `EvalScript` and `VerifyScript` using a fuzzed signature checker.
Test this PR using:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/signature_checker
…
```
Closes#17986.
Top commit has no ACKs.
Tree-SHA512: a9988f8fa7919fe470756ca3e4e75764a589f590769aab452c8f4c254cf41667793e52131d470a12629ec3681fa7fc20091f371b8f3e3eec105674c2769e7d7e
5b59a19731 Update merkle.cpp (4d55397500)
Pull request description:
Change comment from `The reason is that if the number of hashes in the list at a given time
is odd`, to ` The reason is that if the number of hashes in the list at a given level
is odd` (to be a bit more precise: replacing `time` with `level`)
<!--
*** 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:
MarcoFalke:
ACK 5b59a19731
instagibbs:
ACK 5b59a19731
Tree-SHA512: 30d29b9855b30de8b54033ca4884cfb5bf8ab9e52cf61da237abba0e15ebff947c65f8ba82175694bc60ee0d54f940a098cadcb0404d3c3bcf577006ab0561a5
Change comment from `The reason is that if the number of hashes in the list at a given time
is odd`, to ` The reason is that if the number of hashes in the list at a given level
is odd` (to be a bit more precise)
6afaf2f680 test: use fs namespace in dbwrapper unicodepath test (fanquake)
Pull request description:
Use our `fs` namespace rather than `boost::filesystem`. Test was added in #17641.
ACKs for top commit:
sipsorcery:
ACK 6afaf2f680.
Tree-SHA512: 5ee024a6d90183b6c344f6a94cfbcacb006973f1f6d98cc421c1c6ef08c09b590d31c78b70b86d855e825241ffea25989cfc40d1bdd53e38a75cda0718ac4489
In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda
function to execute later, capturing the local variable
`consensusParams` by reference.
Presumably this was considered safe because `consensusParams` is a
reference itself to a global variable which is not supposed to change,
but it can in tests.
Fixes https://github.com/bitcoin/bitcoin/issues/18372
fa36f3a295 refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke)
fadafb83cf scheduler: Make schedule* methods type safe (MarcoFalke)
fa70ccc6c4 scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke)
Pull request description:
Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}`
ACKs for top commit:
vasild:
ACK fa36f3a (code review, not tested)
ajtowns:
ACK fa36f3a295
jonatack:
ACK fa36f3a
Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
ec30a79f1c Fix UB with bench on genesis block (Gregory Sanders)
Pull request description:
During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock.
ACKs for top commit:
practicalswift:
ACK ec30a79f1c
sipa:
utACK ec30a79f1c
promag:
ACK ec30a79, `nBlocksTotal` is only used in logging.
Tree-SHA512: b3bdbb58d10d002a2293d7f99196b227ed9f4ca8c6cd08981e95cc964be47efed98b91fad276ee6da5cf7e6684610998ace7ce9bace172dd6c51c386d985b83c
3ed772d221 [tests] remove bignum.py (John Newbery)
f950ec2520 [tests] remove bn2bin() (John Newbery)
3b9b38579c [tests] remove bn_bytes() function (John Newbery)
a760aa14a9 [tests] remove mpi2vch() function (John Newbery)
9a60bef50d [tests] don't encode the integer size in bignum (John Newbery)
1dc68aee66 [tests] add function comments to bignum (John Newbery)
f31fc0e92e [tests] fix flake8 warnings in script.py and bignum.py (John Newbery)
Pull request description:
Only one function is imported in script.py. Just move that function to script.py and remove the bignum.py module.
Remove unused functionality and fix some flake8 warnings along the way.
Top commit has no ACKs.
Tree-SHA512: 015f543ab545b5d5451896e2751d9c19334d9155b03faacd2023781e89833a2440f7f28741e9a8ac49badd9cdc012cbb6e038cdcdebeefaf9cb9d461c0689157
f9f210d8de doc: fix GetTimeMicros() comment in random.cpp (fanquake)
a889711562 rand: remove getentropy() fallback for macOS < 10.12 (fanquake)
Pull request description:
We [no longer support macOS < 10.12](https://github.com/bitcoin/bitcoin/pull/17550) (our binaries will not run), so remove the fallback for when `getentropy()` wasn't available. From the manpage:
```bash
HISTORY
The getentropy() function appeared in OSX 10.12
```
Note that compiling on macOS you'll see a new unused function warning:
```bash
random.cpp:256:13: warning: unused function 'GetDevURandom' [-Wunused-function]
static void GetDevURandom(unsigned char *ent32)
^
1 warning generated.
```
This will likely be addressed as part of #17563.
ACKs for top commit:
vasild:
ACK f9f210d8 (code review, not tested)
elichai:
utACK f9f210d8de
practicalswift:
ACK f9f210d8de -- patch looks correct
laanwj:
ACK f9f210d8de
hebasto:
ACK f9f210d8de, tested on macOS 10.13.6: compiled
Tree-SHA512: 6bd2a721f23605a8bca0b7b51f42d628ebf92a18e74eb43194331ba745ee449223aff84119892781c40b188c70b75417447f4e390e3d9ac549292de2b1e8b308
612a931d1a tests: simplify next_block() function in feature_block (John Newbery)
Pull request description:
The solve parameter is unnecessary. Remove it and add comments.
ACKs for top commit:
MarcoFalke:
ACK 612a931d1a
TheQuantumPhysicist:
ACK 612a931
Looks good. Thanks for improving it 😄
practicalswift:
ACK 612a931d1a -- simpler is better and patch looks correct :)
Tree-SHA512: 25b4879842ea37a3f598be886f02ce4c2fb0b5a618d02b266dbd380f5cbfdd71a8bd35ddd9d6f2cf83920e37c02caf9955a841a02b17ba75ac63f88d32f8b60b
7a6627ae87 Fix mining to an invalid target + ensure that a new block has the correct hash internally in Python tests (Samer Afach)
Pull request description:
Test with block 47 in the `feature_block.py` creates a block with a hash higher than the target, which is supposed to fail. Now two issues exist there, and both have low probability of showing up:
1. The creation is done with `while (hash < target)`, which is wrong, because hash = target is a valid mined value based on the code in the function `CheckProofOfWork()` that validates the mining target:
```
if (UintToArith256(hash) > bnTarget)
return false;
```
2. As we know the hash stored in CBlock class in Python is stateful, unlike how it's in C++, where calling `CBlock::GetHash()` will actively calculate the hash and not cache it anywhere. With this, blocks that come out of the method `next_block` can have incorrect hash value when `solve=False`. This is because the `next_block` is mostly used with `solve=True`, and solving does call the function `rehash()` which calculates the hash of the block, but with `solve=False`, nothing calls that method. And since the work to be done in regtests is very low, the probably of this problem showing up is very low, but it practically happens (well, with much higher probability compared to issue No. 1 above).
This PR fixes both these issues.
Top commit has no ACKs.
Tree-SHA512: f3b54d18f5073d6f1c26eab89bfec78620dda4ac1e4dde4f1d69543f1b85a7989d64c907e091db63f3f062408f5ed1e111018b842819ba1a5f8348c7b01ade96
