06e434d7d96b5ebddd2ee829995101a62fa8da4e test: fix message for ECC_InitSanityCheck test (fanquake)
Pull request description:
OpenSSL is long gone.
ACKs for top commit:
laanwj:
Good catch. ACK 06e434d7d96b5ebddd2ee829995101a62fa8da4e
Tree-SHA512: 1a920fd6493e0374ca00633407e0130f987b136bc68d2062402747bda16a1e588a12bd8b0b8cdef828c9911f210386cfbdb25d478cb9b684d52769d197032064
692f8307fc1449299b90182e7d79efb81a55d7ab test: add test for witness commitment index (fanquake)
06442549f8b725f46c1c727e9eb6fde6b843503c validation: Add minimum witness commitment size constant (fanquake)
Pull request description:
16101de5f3: Per [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Commitment_structure), the witness commitment structure is at least 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
54f8c48d6a: As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning
early.
ACKs for top commit:
MarcoFalke:
ACK 692f8307fc1449299b90182e7d79efb81a55d7ab 🌵
jonatack:
Code review ACK 692f830
ajtowns:
ACK 692f8307fc1449299b90182e7d79efb81a55d7ab
jnewbery:
utACK 692f8307fc1449299b90182e7d79efb81a55d7ab
laanwj:
ACK 692f8307fc1449299b90182e7d79efb81a55d7ab
Tree-SHA512: 7af3fe4b8a52fea2cdd0aec95f7bb935351a77b73d934bc88d6625a3503311b2a062cba5190b2228f97caa76840db3889032d910fc8e318ca8e7810a8afbafa0
32b6b386a5499b1f8439f80d8fc1ee573bc31a53 tests: Sort fuzzing harnesses (practicalswift)
e1e181fad1a73e9dee38a2bd74518e1b8d446930 tests: Add fuzzing coverage for JSONRPCTransactionError(...) and RPCErrorFromTransactionError(...) (practicalswift)
103b6ecce0f8e6d1366962c8748794067b2485fe tests: Add fuzzing coverage for TransactionErrorString(...) (practicalswift)
dde508b8b03a4a144331cb1ff97f1349b491c402 tests: Add fuzzing coverage for ParseFixedPoint(...) (practicalswift)
1532259fcae8712777e1cedefc91224ee60a6aaa tests: Add fuzzing coverage for FormatHDKeypath(...) and WriteHDKeypath(...) (practicalswift)
90b635e84e432e5a3682864f15274dba6acfbded tests: Add fuzzing coverage for CHECK_NONFATAL(...) (practicalswift)
a4e3d13df6a6f48974f541de0b5b061e8078ba9a tests: Add fuzzing coverage for StringForFeeReason(...) (practicalswift)
a19598cf9851cb238a4b5caa04f9ae7281532352 tests: Add fuzzing harness for functions in system.h (ArgsManager) (practicalswift)
Pull request description:
Add fuzzing harnesses for various classes/functions in `util/`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: d27947220850c2a202c7740f44140c17545f45522596912452ccab0c2f5379abeb07cc769982c7855cb465059425206371a2b75ee1c285b03984161c9619d0b0
As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone trys to "optimise" GetWitnessCommitmentIndex() be returning
early.
Add unit test calling CreateWalletFromFile, which isn't currently called from
other unit tests, with some basic checks to make sure it rescans and registers
for notifications correctly.
Motivation for this change was to try to write a test that would fail without
the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8
from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:
ef8c6ca607/src/wallet/wallet.cpp (L3978-L3986)
However, writing a full test for the race condition that call prevents isn't
possible without the locking changes from #16426. So this PR just adds as much
test coverage as is possible now.
This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719,
since it detects the stale notifications.transactionAddedToMempool notifications
that PR eliminates.
fd8e99da57b53da29fbaec6435931c396e3b612b tests: Add fuzzing harness for functions in primitives/transaction.h (practicalswift)
d5a31b7cb4226a62931fd72672422a3d2e789e7a tests: Add fuzzing harness for functions in primitives/block.h (practicalswift)
Pull request description:
Add fuzzing harnesses for various classes/functions in `primitives/`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: ed54bd5b37ff5e40cfa8d3cd8c65d91a2f64fca87b6a5c3b8ddd6becd876ed172735fb53da4d00a86f318fb94517afd179e07cb28a43edf301ffe4dad703cca4
21fa0a44abe8c1b5c452e097eab20cf0ae988805 [docs] use consistent naming for possible_overwrite (John Newbery)
2685c214cce4b07695273503e60350e3f05fe3e2 [tests] small whitespace fixup (John Newbery)
e9936966c08bd8a6ac02828131f619ddaa1ced13 scripted-diff: Rename PRUNED to SPENT in coins tests (John Newbery)
c205979031ff4e8e32a5f05bae813405f233fccd [docs] Improve commenting in coins.cpp|h (John Newbery)
Pull request description:
- Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid
- Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (#10195).
- Rename `potential_overwrite` to `possible_overwrite` to standardize terminology (there were previously examples of both, which made searching the codebase difficult).
- Make other minor improvements to the comments
ACKs for top commit:
jonatack:
Re-ACK 21fa0a4 per `git diff 98bee55 21fa0a4` the only change since my previous review is the following code commenting diff in `src/coins.cpp::L177-179`; rebuilt/ran unit tests anyway as a sanity check on the unit test changes.
Tree-SHA512: 391e01588ef5edb417250080cec17361f982c4454bc5f8c6d78bbd528c68a2bb94373297760691295c24660ce1022ad3ef7599762f736c8eed772ce096d38c3d
ccccd5190898ece3ac17aa3178f320d091f221df script: Remove undocumented and unused operator+ (MarcoFalke)
Pull request description:
This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data.
Removing the operator is also going to protect against accidentally reintroducing bugs like this 6ff5f718b6 (diff-8458adcedc17d046942185cb709ff5c3L1135) (last time it was used).
ACKs for top commit:
laanwj:
ACK ccccd5190898ece3ac17aa3178f320d091f221df
Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9
3718ae2ef8dd2559e435bf8d7f5ed5217611ce81 [tests] Don't initialize PrecomputedTransactionData in txvalidationcache tests (John Newbery)
Pull request description:
PrecomputedTransactionData is initialized inside CheckInputScripts(). No need to pre-initialize it before calling into CheckInputScripts().
Normally, I wouldn't bother, but we're making changes to `PrecomputedTransactionData` in #17977 which would break these tests without removing these constructions. Might as well get these changes out of the way here.
ACKs for top commit:
robot-visions:
ACK 3718ae2ef8dd2559e435bf8d7f5ed5217611ce81
sipa:
utACK 3718ae2ef8dd2559e435bf8d7f5ed5217611ce81
Tree-SHA512: bc9c095035a7072a2a91941df38cdbb969e817264efbaa6dcb88cc3ab132d9264aa0751fa588d1a5e45f37b4d2bb1903cda078765f0bbcc87d9cc47cbec5356a
6f8b498d186df5aa08dbb9ca8fdeab6652f1db5e fuzz: http_request workaround for libevent < 2.1.1 (Sebastian Falbesoner)
Pull request description:
The fuzz test `http_request` calls the following two internal libevent functions:
* `evhttp_parse_firstline_`
* `evhttp_parse_headers_`
Before libevent 2.1.1 however, internal functions names didn't end with an underscore (see libevent commit 8ac3c4c25b and [Changelog for 2.1.1.-alpha](https://github.com/libevent/libevent/blob/master/ChangeLog#L1830) when the change was first mentioned) hence the build fails with a linking error.
This PR adds a preprocessor workaround to the test that checks for the libevent version (via ~`_EVENT_NUMERIC_VERSION`~ `LIBEVENT_VERSION_NUMBER`) and creates wrapper functions mapping to naming scheme without underscore in case the version is older than 2.1.1.
Tested with Ubuntu Xenial 16.04.6 LTS and clang-8.
ACKs for top commit:
hebasto:
ACK 6f8b498d186df5aa08dbb9ca8fdeab6652f1db5e, tested on xenial:
Tree-SHA512: 3b9e0147b8aea22e417d418e3b6d4905f5be131c2b0ae4b0f8b9411c5606d2e22f1b23e1ecc6980ecab907c61404de09e588aae1ac43cf70cf9e8d006bbdee73
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
69ffddc83e0f3e265bf6cf7ae31489ae629fe6be refactor: Remove unused methods CBloomFilter::reset()/clear() (Sebastian Falbesoner)
Pull request description:
The method `CBloomFilter::reset()` was introduced by commit d2d7ee0e863b286e1c9f9c54659d494fb0a7712d in 2015, but was never ever used, as far as I could find. As discovered by MarcoFalke, the method `clear()` is also unused outside of unit tests and is hence also removed.
ACKs for top commit:
MarcoFalke:
re-ACK 69ffddc83e0f3e265bf6cf7ae31489ae629fe6be
jonatack:
ACK 69ffddc83e0f3e2, code review, compiled a fuzz build and started the bloom_filter fuzz test as a sanity check.
promag:
ACK 69ffddc83e0f3e265bf6cf7ae31489ae629fe6be.
Tree-SHA512: 6c53678545ad8e2fa1ffc0a8838e450462f26748a60632f738dc020f0eb494ae2c32841e6256e266ed9140177257a78b707123421942f3819a14ffcb9a99322f
bee88b8c5887e6beb75f26f0db97888a48fa7e7c tests: have coins simulation test also use CCoinsViewDB (James O'Beirne)
Pull request description:
Before this change, the coins simulation test uses a base view of type
CCoinsViewTest, which has no relevance outside of the unittest suite. Might as
well reuse this testcase with a more realistic configuration that has
CCoinsViewDB (i.e. in-memory leveldb) at the bottom of the view structure.
This adds explicit use of CCoinsViewDB in the unittest suite.
#### Before change
```
./src/test/test_bitcoin --run_test=coins_tests --catch_system_errors=no 21.99s user 0.04s system 99% cpu 22.057 total
```
#### After change
```
./src/test/test_bitcoin --run_test=coins_tests --catch_system_errors=no 78.80s user 0.04s system 100% cpu 1:18.82 total
```
ACKs for top commit:
ryanofsky:
Code review ACK bee88b8c5887e6beb75f26f0db97888a48fa7e7c
Tree-SHA512: 75296b2bcbae2f46e780489aafb032592544a15c384d569d016005692fe79fe60d7f05857cf25cc7b0f9ab1c53b47886a6c71cca074a03fb9afec30e1f376858
fa176e253fb473767c61d4d8cd2d93e87d71a015 test: Avoid accessing free'd memory in validation_chainstatemanager_tests (MarcoFalke)
Pull request description:
ACKs for top commit:
ryanofsky:
Code review ACK fa176e253fb473767c61d4d8cd2d93e87d71a015, though if you have to update this again, would suggest separating txindex test cleanup and the chainstatemanager test fix in separate commits, or identifying which part of the change is the bugfix fix in the commit description. Also to clean up the txindex test it might make sense to call SyncWithValidationInterfaceQueue in the test destructor to prevent nondeterminism in other tests
Tree-SHA512: 34c5dca283a7c205cd42b6aa59f00a71fd1bd980bc3d6640a18b280be11470bfabb2fd8c93fadde6fb8e084bcf96c80ec3aa72bbccccfde8a8260d173eaad08f
48973402d8bccb673eaeb68b7aa86faa39d3cb8a wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes (Russell Yanofsky)
e958ff9ab5607da2cd321f29fc785a6d359e44f4 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction (Russell Yanofsky)
c0d07dc4cba7634cde4e8bf586557772f3248a42 wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions (Russell Yanofsky)
1be8ff280c78c30baabae9429c53c0bebb89c44d wallet: Avoid use of Chain::Lock in rescanblockchain (Russell Yanofsky)
3cb85ac594f115db99f96b0a0f4bfdcd69ef0590 wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime (Russell Yanofsky)
f7ba881bc669451a60fedac58a449794702a3e23 wallet: Avoid use of Chain::Lock in listsinceblock (Russell Yanofsky)
bc96a9bfc61afdb696fb92cb644ed5fc3d1793f1 wallet: Avoid use of Chain::Lock in importmulti (Russell Yanofsky)
25a9fcf9e53bfa94e8f8b19a4abfda0f444f6b2a wallet: Avoid use of Chain::Lock in importwallet and dumpwallet (Russell Yanofsky)
c1694ce6bb7e19a8722d5583cd85ad17da40bb67 wallet: Avoid use of Chain::Lock in importprunedfunds (Russell Yanofsky)
ade5f87971211bc67753f14a0d49e020142efc7c wallet refactor: Avoid use of Chain::Lock in qt wallettests (Russell Yanofsky)
f6da44ccce4cfff53433e665305a6fe0a01364e4 wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances (Russell Yanofsky)
bf30cd4922ea62577d7bf63f5029e8be62665d45 refactor: Add interfaces::FoundBlock class to selectively return block data (Russell Yanofsky)
Pull request description:
This is a set of changes updating wallet code to make fewer calls to `Chain::Lock` methods, so the `Chain::Lock` class will be easier to remove in #16426 with fewer code changes and small changes to behavior.
ACKs for top commit:
MarcoFalke:
re-ACK 48973402d8, only change is fixing bug 📀
fjahr:
re-ACK 48973402d8bccb673eaeb68b7aa86faa39d3cb8a, reviewed rebase and changes since last review, built and ran tests locally
ariard:
Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in `findCommonAncestor`
Tree-SHA512: cfd2f559f976b6faaa032794c40c9659191d5597b013abcb6c7968d36b2abb2b14d4e596f8ed8b9a077e96522365261299a241a939b3111eaf729ba0c3ef519b
