ActivateBestChain (ABC) is only called in the "msghand" or one of the
RPC threads, neither of which is a boost::thread. However, ABC is also
called in ThreadImport (which currently happens to be a boost::thread).
In all cases, the interruption_point is redundant with the breakpoint in
ABC that triggers when ShutdownRequested()
VerifyDB is only called in the main thread ("init") or one of the RPC
threads, neither of which is a boost::thread.
7eaf86d3bf trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd and 7e89994133 from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bf
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bf🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
fa1c74fd03 wallet: Remove unused boost::thread_interrupted (MarcoFalke)
fa7b885f51 walletdb: Remove unsed boost/thread (MarcoFalke)
5555d978b0 wallet: Make PeriodicFlush uninterruptible (MarcoFalke)
Pull request description:
The `boost::this_thread::interruption_point()` in the code base currently block the replacement of `boost::thread` with `std::thread`. [1]
Remove them from the wallet because they are either unused or useless.
The feature to interrupt a periodic flush is useless because all wallets have just been flushed 9ccaee1d5e/src/init.cpp (L194) and another flush should be a noop. Also, they will be flushed again shortly after 9ccaee1d5e/src/init.cpp (L285), so even if repeated flushes weren't a noop, doing 3 instead of 2 shouldn't matter too much at this point. Also, the wallet is flushed every two seconds in the worst case, so if this is an expensive operation, that period should be readjusted. (Or bdb should be removed altogether #18916)
[1] Replacement of `boost::thread` with `std::thread` should happen because:
* The boost thread dependency is slow to compile
* Boost thread is less maintained than the standard lib
* Boost thread is mostly redundant to the standard lib
* Global interruption points via exceptions are hard to keep track of during review and easy to get wrong during runtime (e.g. accidental `catch (...)`)
ACKs for top commit:
fanquake:
ACK fa1c74fd03
Tree-SHA512: b166619256de2ef4325480fa1367f68bc9371ad785ec503aed61eab41ba61f1a9807aab25451a24efda3db64855c9ba0025645b98bc58557bc3ec56c5b3297d0
152e8baf08 Use salted hasher instead of nonce in sigcache (Jeremy Rubin)
5495fa5850 Add Hash Padding Microbenchmarks (Jeremy Rubin)
Pull request description:
This PR replaces nonces in two places with pre-salted hashers.
The nonce is chosen to be 64 bytes long so that it forces the SHA256 hasher to process the chunk. This leaves the next 64 (or 56 depending if final chunk) open for data. In the case of the script execution cache, this does not make a big performance improvement because the nonce was already properly padded to fit into one buffer, but does make the code a little simpler. In the case of the sig cache, this should reduce the hashing overhead slightly because we are less likely to need an additional processing step.
I haven't benchmarked this, but back of the envelope it should reduce the hashing by one buffer for all combinations except compressed public keys with compact signatures.
ACKs for top commit:
ryanofsky:
Code review ACK 152e8baf08. No code changes, just rebase since last review and expanded commit message
Tree-SHA512: b133e902fd595cfe3b54ad8814b823f4d132cb2c358c89158842ae27daee56ab5f70cde2585078deb46f77a6e7b35b4cc6bba47b65302b7befc2cff254bad93d
fa7d3a8890 test: Add missing sync_blocks to wallet_hd (MarcoFalke)
eeeed51f58 test: pep-8 wallet_hd (MarcoFalke)
Pull request description:
This fixes the ` test_framework.authproxy.JSONRPCException: non-final (-26)` error when node 1 is one block behind of node 0. (height 122 vs 123)
ACKs for top commit:
promag:
Code review ACK fa7d3a8890.
Tree-SHA512: b549dce2e08c58b949168ed2013bfa176802c963d0d7e890f643c8792da5dade14d91441dfa74372f1f1d34d696e8900a2b60b4861c0ba2dce99f2a633ab27ff
fa9c675591 Limit scope of all global std::once_flag (MarcoFalke)
Pull request description:
`once_flag` is a helper (as the name might suggest) to execute a callable only once. Thus, the scope of the flag does never need to extend beyond where the callable is called. Typically this is function scope.
Move all the flags to function scope to
* simplify code review
* avoid mistakes where similarly named flags are accidentally exchanged
* avoid polluting the global scope
ACKs for top commit:
hebasto:
ACK fa9c675591, tested on Linux Mint 19.3 (x86_64).
promag:
Code review ACK fa9c675591.
Tree-SHA512: 095a0c11d93d0ddcb82b3c71676090ecc7e3de3d5e7a2a63ab2583093be279242acac43523bbae2060b4dcfa8f92b54256a0e91fbbae78fa92d2d49e9db62e57
4f49d5222e gui, refactor: Register Qt meta types in application constructor (João Barbosa)
Pull request description:
Removes a warning when running `QT_QPA_PLATFORM=cocoa src/qt/test/test_bitcoin-qt`.
ACKs for top commit:
jonasschnelli:
Re utACK 4f49d5222e
hebasto:
ACK 4f49d5222e, tested on macOS 10.15.5.
Tree-SHA512: e931a022ba83cb0ef04d82544ebd9b18242f8fc2b41443afce4d5c4222f222e8b3517bdb484a1a4f61377c5dceca067d8ccf250da3a727299448e54bec33ed6e
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 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.
Useful resources:
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
Building with -Wunreachable-code-loop-increment causes a warning
due to always returning on the first iteration of the loop that
outputs errors on invalid args.
Collect all errors, and output them in a single error message
after the loop completes, resolving the warning and avoiding
popup hell by outputting a seperate message for each error.
Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in
the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up
in a deadlock.
`ClientModel::m_cached_tip_blocks` is protected by
`ClientModel::m_cached_tip_mutex`. There are two access paths that
lock the two mutexes in opposite order:
```
validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main
validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip()
ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost
...
qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex
```
and
```
qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex
qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash()
interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main
```
From `debug.log`:
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
m_cs_chainstate validation.cpp:2851
(1) cs_main validation.cpp:2868
::mempool.cs validation.cpp:2868
(2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255
Current lock order is:
(2) m_cached_tip_mutex qt/clientmodel.cpp:119
(1) ::cs_main interfaces/node.cpp:200
```
The possible deadlock was introduced in
https://github.com/bitcoin/bitcoin/pull/17993
fac0ed16ec doc: Sync "how to upgrade" with 0.20.0 release notes (MarcoFalke)
fa861794d3 doc: Add release notes for 17219 (MarcoFalke)
Pull request description:
ACKs for top commit:
fanquake:
ACK fac0ed16ec - seems to match the relevant changes in 79606b56b2, 36c1d981d4 and 4c17c85c6c
Tree-SHA512: f04c2b3a1cd094d7f50e3f8db06d726873d2412d651c94bf38d17fd5ee0c47c84480d6b20ec18641888ee88012e7c59e7e794467a0eed1caf44c0569feecc4ca
facef3d413 doc: Explain that anyone can work on good first issues, move text to CONTRIBUTING.md (MarcoFalke)
fae2fb2a19 doc: Expand section on Getting Started (MarcoFalke)
100000d1b2 doc: Add headings to CONTRIBUTING.md (MarcoFalke)
fab893e0ca doc: Fix unrelated typos reported by codespell (MarcoFalke)
Pull request description:
Some random doc changes:
* Add sections to docs, so that they can be linked to
* Explain that anyone (even maintainers) are allowed to work on good first issues
* Expand section on Getting Started slightly
ACKs for top commit:
hebasto:
ACK facef3d413
fanquake:
ACK facef3d413
Tree-SHA512: 8998e273a76dbf4ca77e79374c14efe4dfcc5c6df6b7d801e1e1e436711dbe6f76b436f9cbc6cacb45a56827babdd6396f3bd376a9426ee7be3bb9b8a3b8e383
f898ef65c9 tests: Add fuzzing harness for functions in script/sign.h (practicalswift)
c91d2f0615 tests: Add fuzzing harness for functions in script/sigcache.h (practicalswift)
d3d8adb79f tests: Add fuzzing harness for functions in script/interpreter.h (practicalswift)
fa80117cfd tests: Add fuzzing harness for functions in script/descriptor.h (practicalswift)
43fb8f0ca3 tests: Add fuzzing harness for functions in script/bitcoinconsensus.h (practicalswift)
8de72711c6 tests: Fill fuzzing coverage gaps for functions in script/script.h, script/script_error.h and script/standard.h (practicalswift)
c571ecb071 tests: Add fuzzing helper functions ConsumeDataStream, ConsumeTxDestination and ConsumeUInt160 (practicalswift)
Pull request description:
Add fuzzing harnesses for functions in `script/`:
* Add fuzzing helper functions `ConsumeDataStream` and `ConsumeUInt160`
* Fill fuzzing coverage gaps for functions in `script/script.h`, `script/script_error.h` and `script/standard.h`
* Add fuzzing harness for functions in `script/bitcoinconsensus.h`
* Add fuzzing harness for functions in `script/descriptor.h`
* Add fuzzing harness for functions in `script/interpreter.h`
* Add fuzzing harness for functions in `script/sigcache.h`
* Add fuzzing harness for functions in `script/sign.h`
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 :)
ACKs for top commit:
MarcoFalke:
ACK f898ef65c9🔉
Tree-SHA512: f6e77b34dc79f23de5fa9e38ac06e6554b5b946ec3e9a67e2bd982e60aca37ce844f785457ef427a5e3b45e31c305456bca8587cc9f4a0b50b3852e39726eb04
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)
Pull request description:
Support `getcfilters` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
Empact:
re-Code Review ACK 9e36067d8c
MarcoFalke:
re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
jkczyz:
ACK 9e36067d8c
fjahr:
Code review ACK 9e36067d8c
Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
9a19c9ada5 Always define the raii_event_tests test suite (Craig Andrews)
Pull request description:
The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.
This improves upon 95f97f4 actually fixing https://github.com/bitcoin/bitcoin/issues/9493
ACKs for top commit:
MarcoFalke:
ACK 9a19c9ada5🎹
Tree-SHA512: 3c42f17a9b5d56c8841f3aa9ac19da91c10aff210026266f31f7eb98a62528740d7c518c121452b68e8f801d6c80ecfb627d137ec6ed533289fa3beb08b4f176
60ed33904c tests: implement base58_decode (10xcryptodev)
Pull request description:
implements TODO: def base58_decode
ACKs for top commit:
ryanofsky:
Code review ACK 60ed33904c. Just suggested changes since last review. Thank you for taking suggestions!
Tree-SHA512: b3c06b4df041a6d88033cd077a093813a688e42d0b9aa777c715e5fd69cfba7b1bf984428bd98417d3c15232d3d48bc9c163317564f9e1d562db6611c21e2c10
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
fad21a1a7a test: Explain that a bug should be filed when the test fail (MarcoFalke)
Pull request description:
Without a bug report it is harder to fix the issue
ACKs for top commit:
hebasto:
ACK fad21a1a7a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
fanquake:
ACK fad21a1a7a
Tree-SHA512: db194e8f8c0f07b2f4c9ef27e456510959f89da69435cee71605d720e0ad06f18700973f5af25ea31a190b933eb35f2743f014878aa3f8293500e06b4907ebbd
f0d7ed10b4 depends: Propagate only specific CLI variables to sub-makes (Carl Dong)
0a33803f1c depends: boost: Use clang toolset if clang in CXX (Carl Dong)
1ce74bcde3 depends: boost: Split target-os from toolset (Carl Dong)
2d4e480813 depends: boost: Specify toolset to bootstrap.sh (Carl Dong)
3d6603e340 depends: Propagate well-known vars into depends (Carl Dong)
Pull request description:
From: https://github.com/bitcoin/bitcoin/pull/18308#issuecomment-598301117
The following monstrosity is quite useful when invoked inside `depends`, and reviewers can use it to compare the behaviour of this change against master.
```bash
make print-{{,{host,{,{i686,x86_64,riscv64}_}linux}_}{CC,CXX},boost_{cc,cxx}}
```
It would also be helpful to make sure that setting `HOST`, `CC`, and `CXX` does the right thing. The 3 hosts I found offered good coverage were: `{x86_64,i686,riscv64}-linux-gnu`. As we special-case the `x86_64` and `i686` hosts in `depends/hosts/linux.mk`, and `riscv64` is a sanity check for a non-special-cased host.
ACKs for top commit:
hebasto:
ACK f0d7ed10b4, tested on Linux Mint 19.3 (x86_64):
practicalswift:
ACK f0d7ed10b4 -- patch looks correct
laanwj:
Code review and concept ACK f0d7ed10b4
ryanofsky:
Code review ACK f0d7ed10b4. Changes since last review: adding comment explaining check for predefined make variables, dropping freetype commit, adding commit whitelisting overrides for recursive makes
Tree-SHA512: b6b8e76f713c26a0add6cd685824e2f5639109236ee9f89338f7c79cb1b1f2c3897bfb62b80b023d6d1943b5a6eb282a2f827f1f499c5e556eca015d6635fa65
