9952242c03fe587b5dff46a9f770e319146103bf build: improve builtin_clz* detection (fanquake)
Pull request description:
Fixes#19402.
The way we currently test for `__builtin_clz*` support with `AC_CHECK_DECLS` does not work with Clang:
```bash
configure:21492: clang++-10 -std=c++11 -c -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
conftest.cpp💯10: error: builtin functions must be directly called
(void) __builtin_clz;
^
1 error generated.
```
This also removes the `__builtin_clz()` check, as we don't actually use it anywhere, and it's trvial to re-add detection if we do start using it at some point. If this is controversial then I'll add a test for it as well.
ACKs for top commit:
sipa:
ACK 9952242c03fe587b5dff46a9f770e319146103bf
laanwj:
ACK 9952242c03fe587b5dff46a9f770e319146103bf
Tree-SHA512: 695abb1a694a01a25aaa483b4fffa7d598842f2ba4fe8630fbed9ce5450b915c33bf34bb16ad16a16b702dd7c91ebf49fe509a2498b9e28254fe0ec5177bbac0
0ac09c9793cd6d25ef6df14d74fb960529e1b4e3 qt: Do not truncate node flag strings in debugwindow.ui peers details tab. (saibato)
Pull request description:
Fix: When fiddling around with new node flags other than the usual.
I saw that not all possible node flag strings i.e. the UNKNOWN[..] where
visible in peers details tab.
Since v18.2 fixed size was set to 300 and sliding is thereby limited.
A fix on my old linux cruft and small screen was to set minimumSize width to -1 or 0.
Qt will then autosize the slider to the max string length.
Thereby i had full display of all flags inclusive sliding without to fullscreen the window.
Not sure if this is even an issue for those who can afford big screens or high res macs?
Feedback welcome.
BTW: nice side effect now again easy to scroll trough long version names of the node.
can't wait to see strings like /Satoshi:0.23.99/NOX2NOX4NOX32 or what ever fits in the version string.
ACKs for top commit:
hebasto:
ACK 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
promag:
Tested ACK 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3 on macos.
Tree-SHA512: a1601b5e35f10b1fd9407b28142ca00c1b985a822be5d23be4d7d3376211450f06e17f962c44b8b40977f8f8bbbb701cac1c5abb4afb3618da76385dfac848a3
4d7369125a82214ea42b808a32b71b315a5c3c72 Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2cdbed32438bdb32623af6e06f13ecd35e4 Remove an apparently unnecessary conversion (Ben Woosley)
966a22d859db37b1775e2180e5be032fc4fdf483 Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e07fd6c174ff3f6406a619550d2f6c19360 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217f913967703b404747133be67cf2f4feac Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f144480d7b170e49087df162989d31cd20 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc46812334074d2c77a6233e8a961cd0785872 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32ce605984094c5552877cb99bc81654f2c Prefer explicit uint160 conversion (Ben Woosley)
Pull request description:
This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type.
Inspired by and built on #17924. Commits are small and focused to ease review.
Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".
ACKs for top commit:
achow101:
ACK 4d7369125a82214ea42b808a32b71b315a5c3c72
meshcollider:
re-utACK 4d7369125a82214ea42b808a32b71b315a5c3c72
Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54
25f3554351a99a0a695fbd2a6a0f293b3adc3d98 scripted-diff: Make SeparatorStyle a scoped enum (Hennadii Stepanov)
Pull request description:
This PR is [split](https://github.com/bitcoin/bitcoin/pull/17877#issuecomment-644751515) from https://github.com/bitcoin/bitcoin/pull/17877 and makes `BitcoinUnits::SeparatorStyle` a scoped enum.
ACKs for top commit:
MarcoFalke:
review ACK 25f3554351a99a0a695fbd2a6a0f293b3adc3d98 🚐
Tree-SHA512: 578f1340a476cf79faa109a83815d3c75e26d9c18873e653d7624b52428ccb2677293116db0a60ae14c949d63b64988fc5a39c7184c2352b87b00e8ddaaaf474
fa7166759789c1282609ff3ab2e80d4f70910a9f ci: Move travis workarounds to .travis.yml (MarcoFalke)
Pull request description:
It seems odd to have travis related workarounds in the general ci config files. Fix that oddity by moving the travis related workarounds to the travis yaml file.
For unexplained reasons, this should also work around and thus close#19171
ACKs for top commit:
hebasto:
ACK fa7166759789c1282609ff3ab2e80d4f70910a9f, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: b4419d38e2b41f6e4d6e6b7658f1d972c40c390a49fe78808f8640d28efd84cc6668ce292d45b7c539e65b9e2ecbad10e796cb8f9329a0f1e7d0132ce962d226
0f8f51544558d204a4e9d0607b0f375150529637 RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME (Luke Dashjr)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 357c6e0bd1b144213ca6cf0bfd649c7a482c2d6d5e98a254d20c8365d228dc71ae1b78aca4918fdbf065f8894ef82f8a475902d605204275bb99fe77d4b42fae
d49612f98add29066817b7c808b76c2d728948e5 Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov)
d1ae7c0355662481a7d181a0a458284936d53eb1 Make GetWarnings() return bilingual_str (Hennadii Stepanov)
38e33aa481cefbe12c50f344bae190c0d95fb489 refactor: Make GetWarnings() bilingual_str aware internally (Hennadii Stepanov)
Pull request description:
This is one more step for consistent usage of `bilingual_str`.
No new translation messages are defined.
ACKs for top commit:
laanwj:
Code review ACK d49612f98add29066817b7c808b76c2d728948e5
MarcoFalke:
ACK d49612f98add29066817b7c808b76c2d728948e5 🌂
Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9
There's no actual multiprocess code yet, that's just part related to build system.
To test build add an option --enable-multiprocess to configure
So far as there's no real multiprocessing yet, let's save CPU money - last commit in this PR disables gitlab's CI temporary
206f74e88cfa343d228c1d6596d3846863824ca5 Support make src/bitcoin-node and src/bitcoin-gui (João Barbosa)
Pull request description:
This change adds the following configure output variables
```
dnl Multi Process
BITCOIN_MP_NODE_NAME=bitcoin-node
BITCOIN_MP_GUI_NAME=bitcoin-gui
```
and adds support for
```sh
make src/bitcoin-node src/bitcoin-gui
```
ACKs for top commit:
laanwj:
Code review ACK 206f74e88cfa343d228c1d6596d3846863824ca5
Tree-SHA512: 4d1a694b9010ecc267ee955f4475127a58e6da72f30179ec740285ee6fe03cd91dcb6847317a47460dbd548edb88b7da6c7a98eac10f0dabe3ce4e83e0aa8093
7d0271b5c30f86e4af175a5ab7df5e593cd85195 depends: Set CMAKE_INSTALL_RPATH for native packages (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
After #19685 started setting `LDFLAGS`, the `INSTALL_RPATH_USE_LINK_PATH` cmake option used in the libmultiprocess build no longer works, so it is neccessary to set `CMAKE_INSTALL_RPATH` as a fallback.
It's unclear currently whether the bad interaction between `INSTALL_RPATH_USE_LINK_PATH` and `LDFLAGS` is a bug, but the issue is reported:
- https://github.com/bitcoin/bitcoin/issues/19981#issuecomment-696680877
- https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892Fixes#19981
ACKs for top commit:
fanquake:
ACK 7d0271b5c30f86e4af175a5ab7df5e593cd85195 - I haven't looked in depth, but I've re-read through #19981 and checked the failure by testing #19160 (with this reverted):
dongcarl:
ACK 7d0271b Looked into this a bit, it makes sense that for the things we build in depends, we want the library search to start in depends. It seems reasonable to expect this to happen automatically when `CMAKE_INSTALL_PREFIX` and `INSTALL_RPATH_USE_LINK_PATH` are set, but oh well...
Tree-SHA512: 97cc5801c3204c14cd33004423631456ca0701e2127ee5146810a76e2f4aac9de1f4b5437402a4329cda54e022dc99270fee7e38c2995765f36b3848215fa78e
b8936883573708059357a66f67fad9dc77a8bade depends: Specify LDFLAGS to cmake as well (Carl Dong)
b3f541f618fe1f3d44baf6a0dd4299173c81f752 depends: Prepend CPPFLAGS to C{,XX}FLAGS for CMake (Carl Dong)
8e121e550953711cd03d7b6c221afd065c325c5e depends: Cleanup CMake invocation (Carl Dong)
8c7cd0c6d9f295bcb6913e3c69c9dac4ce2b25ce depends: More robust cmake invocation (Carl Dong)
3ecf0eca634601da216b06f091f95456c047f39c depends: Use $($(package)_cmake) instead of cmake (Carl Dong)
Pull request description:
- Use `$($(package)_cmake)` instead of invoking `cmake` directly
- Use well-known env vars instead of overriding CMake variables
ACKs for top commit:
ryanofsky:
Code review ACK b8936883573708059357a66f67fad9dc77a8bade. Only changes since last review are new commits adding whitespace, cppflags and ldflags to cmake invocation
Tree-SHA512: cfcd8cc9dcd0b336cf48b82fca9fe4bbc7930ed397cb7a68a07066680eb4c1906a6a9b5bd2589b4b4999e8f16232fa30ee9b376b60f4456d0fff931fbf9cc19a
92bc268e4af4ebcbde08567ea00e019ac509a769 build: Detect missed pkg-config early (Hennadii Stepanov)
1739eb23d8a6d272e70f95342323b6fe48b8eb6c build: Drop unused use_pkgconfig variable (Hennadii Stepanov)
a661449a2eeaf88efda36b6a84084dcbfe5b24eb build: Drop use_pkgconfig check for libmultiprocess check (Hennadii Stepanov)
90b95e7929463d6127c1b24fe1bf457d750a045c build: Drop dead non-pkg-config code for libevent check (Hennadii Stepanov)
44a14afbb889633a6c9a322a5aeca2e1b2cbdbd8 build: Drop dead non-pkg-config code for qrencode check (Hennadii Stepanov)
10cbae0c399302b0f8b1aa847c4246ba60bf25ee build: Drop dead non-pkg-config code for ZMQ check (Hennadii Stepanov)
06cfc9cadf7c5dd43147e6525a348d5f2d299422 build: Fix indentation in UNIVALUE check (Hennadii Stepanov)
6fd2118e777d11cbc81a45313d1a7d6400e34f3f build: Drop dead non-pkg-config code for UNIVALUE check (Hennadii Stepanov)
e9edbe4dbd8c24a779de7d92e5f10c870aab5511 build: Always use pkg-config (Hennadii Stepanov)
9e2e753b0605c8cd826381a362f0c7de56eea81f build: Always define ZMQ_STATIC for MinGW (Hennadii Stepanov)
Pull request description:
This PR:
- is based on #18297 (already merged)
- drops all of the non-pkg-config paths from the `configure` script
Ref: #17768
ACKs for top commit:
fanquake:
ACK 92bc268e4af4ebcbde08567ea00e019ac509a769. I re-gitian-built. There are a couple follow-ups that I'll PR shortly. Thanks for addressing my feedback above. I took too long to get back to this.
laanwj:
ACK 92bc268e4af4ebcbde08567ea00e019ac509a769
Tree-SHA512: 83c2d9cf03518867a1ebf7e26a8fc5b6dd8962ef983fe0d84e0c7eb74717f4c36a834da02faf0e503ffd87167005351671cf040c0d4ddae57ee152a6ff84012b
e2bab2aa162ae38b2bf8195b577c982402fbee9d multiprocess: add multiprocess travis configuration (Russell Yanofsky)
603fd6a2e708c04ef6c9880f89d0a4cbaa6fc7c5 depends: add MULTIPROCESS depends option (Russell Yanofsky)
5d1377b52bfcd4edf8553aaf332bfeb92fc554cc build: multiprocess autotools changes (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR consists of build changes only. It adds an `--enable-multiprocess` autoconf option (off by default and marked experimental), that builds new `bitcoin-node` and `bitcoin-gui` binaries. These currently function the same as existing `bitcoind` and `bitcoin-qt` binaries, but are extended in #10102 with IPC features to execute node, wallet, and gui functions in separate processes.
In addition to adding the `--enable-multiprocess` config flag, it also adds a depends package and autoconf rules to build with the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library, and it adds new travis configuration to exercise the build code and run functional tests with the new binaries.
The changes in this PR were originally part of #10102 but were moved into #16367 to be able to develop and review the multiprocess build changes independently of the code changes. #16367 was briefly merged and then reverted in #18588. Only change since #16367 has been dropping the `native_boost.mk` depends package which was pointed out to be no longer necessary in https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-596484337 and https://github.com/bitcoin/bitcoin/pull/18588#pullrequestreview-391765649
ACKs for top commit:
practicalswift:
ACK e2bab2aa162ae38b2bf8195b577c982402fbee9d
Sjors:
tACK e2bab2aa162ae38b2bf8195b577c982402fbee9d on macOS 10.15.4
hebasto:
ACK e2bab2aa162ae38b2bf8195b577c982402fbee9d, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: b5a76eab5abf63d9d8b6d628cbdff4cc1888eef15cafa0a5d56369e2f9d02595fed623f4b74b2cf2830c42c05a774f0943e700f9c768a82d9d348cad199e135c
3913d1e8c1f604bdd622d5e81e5077ef52b30466 qt: Drop buggy TableViewLastColumnResizingFixer class (Hennadii Stepanov)
Pull request description:
In Qt 5 the last column resizing with dragging its left edge works out-of-the-box.
The current `TableViewLastColumnResizingFixer` implementation could put the last column content out of the view port and confuse a user:
Historical context:
- https://github.com/bitcoin/bitcoin/pull/2862
- https://github.com/bitcoin/bitcoin/pull/3626
- https://github.com/bitcoin/bitcoin/pull/3738
- https://github.com/bitcoin/bitcoin/pull/3920#205 is a nice addition.
ACKs for top commit:
jarolrod:
ACK 3913d1e8c1f604bdd622d5e81e5077ef52b30466, tested on macOS 11.1 Qt 5.15.2
Talkless:
tACK 3913d1e8c1f604bdd622d5e81e5077ef52b30466, tested on Debian Sid. Can confirm that behavior in previous commit does not produce scroll bar, last column gets "hidden". This PR makes clear that there's more to see in the view.
promag:
Tested ACK 3913d1e8c1f604bdd622d5e81e5077ef52b30466 on macos.
Tree-SHA512: 12582dfce54bb1db3d9934ae092e305d32e9760cc99b0265322e161fa7f54b7d6fb6cefedf700783f767d5c3a56a8545c8d2f5ade66596c4e67b8a5287063e8a
9266f7497f256d780178829e0f3a29ddaeb794ba util: Use std::chrono for time getters (MarcoFalke)
3c2e16be22ae04bf56663ee5ec1554d0d569741b time: add runtime sanity check (Cory Fields)
Pull request description:
I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`, but that will likely need more cross-platform testing/discussion, so have just split them up as this change is straight forward.
ACKs for top commit:
practicalswift:
Tested ACK 9266f7497f256d780178829e0f3a29ddaeb794ba
laanwj:
Code review ACK 9266f7497f256d780178829e0f3a29ddaeb794ba
Tree-SHA512: 5471a60e65e9fa8ef48320743ef637f1d162724e717e0f5509118e1e5732fc0844656a9c09d3d1300eb657dcc7a1e1e67305d8c9ef959c63be67393607dd4ceb
c943326d3c06e481c142b112c7e7a0c6ff5a76b3 doc/bips: Add BIPs 43, 44, 49, and 84 (Luke Dashjr)
Pull request description:
If you don't like what they say, please suggest alternatives ;)
ACKs for top commit:
prusnak:
ACK c943326
Tree-SHA512: 7db93f8491289657ec45df30e557eb8572b35201eb29aed1b11bf3949924fce56b4e2d71e1f0acf5d24a01278c0dec99790d632f04c15117010c4ac564368d6b
060a2a64d40d75fecb60b7d2b9946a67e46aa6fc ci: remove boost thread installation (fanquake)
06e1d7d81d5a56d136c6fc88f09a2b0654a164f9 build: don't build or use Boost Thread (fanquake)
7097add83c8596f81be9edd66971ffd2486357eb refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981ef834490c438436719f95cbaf888c4914 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)
Pull request description:
This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).
Even though [some concerns were raised](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.
Two other points:
[Cory mentioned in #21022](https://github.com/bitcoin/bitcoin/pull/21022#issuecomment-769274179):
> It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.
Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
* The version of Boost.
* The platform you're building for.
* Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
* Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](https://github.com/boostorg/thread/issues/230#issuecomment-475937761) version of `shared_mutex`.
A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.
With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.
Previous similar PRs were #19183 & #20922. The authors are included in the commits here.
Also related to #21022 - pthread sanity checking.
ACKs for top commit:
laanwj:
Code review ACK 060a2a64d40d75fecb60b7d2b9946a67e46aa6fc
vasild:
ACK 060a2a64d40d75fecb60b7d2b9946a67e46aa6fc
Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
fd0be92cff6a4b5e343e6ddae7481868354b9869 doc: Add instructions on how to fuzz the P2P layer using Honggfuzz NetDriver (practicalswift)
Pull request description:
Add instructions on how to fuzz the P2P layer using [Honggfuzz NetDriver](http://blog.swiecki.net/2018/01/fuzzing-tcp-servers.html).
Honggfuzz NetDriver allows for very easy fuzzing of TCP servers such as Bitcoin Core without having to write any custom fuzzing harness. The `bitcoind` server process is largely fuzzed without modification.
This makes the fuzzing highly realistic: a bug reachable by the fuzzer is likely also remotely triggerable by an untrusted peer.
Top commit has no ACKs.
Tree-SHA512: 9e98cb30f00664c00c8ff9fd224ff9822bff3fd849652172df48dbaeade1dd1a5fc67ae53203f1966a1d4210671b35656009a2d8b84affccf3ddf1fd86124f6e
fa650ca7f19307a9237e64ac311488c8947fc12a Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd2aed8380cc9b9e52791f662aecd6a6 test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f19307a9237e64ac311488c8947fc12a: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f19307a9237e64ac311488c8947fc12a, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
fa0a4d6c605b8ed47796f68068d6273bef7fcaef test: remove assert_blockchain_height (MarcoFalke)
Pull request description:
This simplifies the code and solves intermittent timeouts caused by commit 0d39b5848a7a341cd2b958336861cdd4098e2616.
E.g. https://cirrus-ci.com/task/5196092369272832?command=ci#L3126
```
test 2021-02-08T12:27:56.275000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 127, in main
self.run_test()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumevalid.py", line 180, in run_test
self.assert_blockchain_height(self.nodes[0], 101)
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumevalid.py", line 92, in assert_blockchain_height
assert False, "blockchain too short after timeout: %d" % current_height
AssertionError: blockchain too short after timeout: 101
ACKs for top commit:
glozow:
ACK fa0a4d6c60
Tree-SHA512: 3859b0c1581c21f03c775f119204cc3a219e5d86346883634886f6da46feaf254b9c6c49c1ec4581ffe409cdf05f6e91ec38c3976c3daf4a9e67f96ddc1e0dce
fa362064e383163a2585ffbc71ac1ea3bcc92663 rpc: Return total fee in mempool (MarcoFalke)
Pull request description:
This avoids having to loop over the whole mempool to query each entry's fee
ACKs for top commit:
achow101:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
glozow:
ACK fa362064e3🧸
jnewbery:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
74d23bf7fbb6169ec658c36af57cd1f937823d9c rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)
Pull request description:
It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.
Closes#5638
ACKs for top commit:
jonatack:
ACK 74d23bf7fbb6169ec658c36af57cd1f937823d9c
Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
9d026546778629472574b26fa73338efc63d02da doc: Fix systemd spelling and link to doc/init.md (Hennadii Stepanov)
601778c3107adbd8d96eb0bb5c16a9d0a4b81594 script: Add Documentation key to bitcoind.service (Hennadii Stepanov)
d9392b724cae53b7a16fa5f84ebe152eea496502 script: Improve robustness of bitcoind.service on startup (Hennadii Stepanov)
Pull request description:
If network interfaces are not properly up the following happens:
```
...
2021-01-08T10:17:11Z scheduler thread start
2021-01-08T10:17:11Z libevent: getaddrinfo: address family for nodename not supported
2021-01-08T10:17:11Z Binding RPC on address 127.0.0.1 port 8332 failed.
2021-01-08T10:17:11Z HTTP: creating work queue of depth 16
2021-01-08T10:17:11Z Using random cookie authentication.
2021-01-08T10:17:11Z Generated RPC authentication cookie /var/lib/bitcoind/.cookie
2021-01-08T10:17:11Z HTTP: starting 2 worker threads
2021-01-08T10:17:11Z init message: Loading banlist...
2021-01-08T10:17:11Z SetNetworkActive: true
2021-01-08T10:17:11Z Error: Cannot resolve -externalip address: <EDITED>
2021-01-08T10:17:11Z Shutdown: In progress...
2021-01-08T10:17:11Z scheduler thread exit
2021-01-08T10:17:11Z Shutdown: done
```
This PR improves robustness on startup in such cases in documented way:
https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/
Also minor doc improvements are added.
ACKs for top commit:
Sjors:
ACK 9d02654
practicalswift:
ACK 9d026546778629472574b26fa73338efc63d02da: patch looks correct
darosior:
ACK 9d026546778629472574b26fa73338efc63d02da -- been using the first patch too
Tree-SHA512: 38294f5682c09e6ea9008de7d7459098c920cf1b98ad8ef8a5d2ca01f2f781c0fec5591dc40ef36eeb19d94991b0c7fb7cb38c4e716bc7219875c9bcd0a55e1b
8775691383ff394b998232ac8e63fac3a214d18b Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" (Luke Dashjr)
Pull request description:
The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense
Originally https://github.com/bitcoin/bitcoin/pull/17463, but rewritten here much simpler based on other merged changes.
ACKs for top commit:
hebasto:
ACK 8775691383ff394b998232ac8e63fac3a214d18b, tested on Linux Mint 20.1 (x86_64, Qt 5.12.8):
Tree-SHA512: 3953cc9c09613c9a629def8b4dc061b537f148ddcb378430645602e0be0f3a9f1cff083aa685b94b2e9372300d02ec97e0d9ea89db6e3c6feec86795090f0f77
3eb94ec81b72b14f72a1f6ce5c9aa24476df755a sync: Use decltype(auto) return type for WITH_LOCK (Carl Dong)
Pull request description:
> Now that we're using C++17, we can use the decltype(auto) return type
> for functions and lambda expressions.
>
> As demonstrated in this commit, this can simplify cases where previously
> the compiler failed to deduce the correct return type.
>
> Just for reference, for the "assign to ref" cases fixed here, there are
> 3 possible solutions:
>
> - Return a pointer and immediately deref as used before this commit
> - Make sure the function/lambda returns declspec(auto) as used after
> this commit
> - Class& i = WITH_LOCK(..., return std::ref(...));
>
> -----
>
> References:
> 1. https://en.cppreference.com/w/cpp/language/function#Return_type_deduction
> 2. https://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts
> 3. https://en.cppreference.com/w/cpp/language/auto
> 4. https://en.cppreference.com/w/cpp/language/decltype
>
> Explanations:
> 1. https://stackoverflow.com/a/21369192
> 2. https://stackoverflow.com/a/21369170
Thanks to sipa and ryanofsky for helping me understand this
ACKs for top commit:
jnewbery:
utACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a
hebasto:
ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a, I have reviewed the code and it looks OK, I agree it can be merged. I have verified possible warnings:
ryanofsky:
Code review ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a
Tree-SHA512: 5f55c7722aeca8ea70e5c1a8db93e93ba0e356e8967e7f607ada38003df4b153d73c29bd2cea8d7ec1344720d37d857ea7dbfd2a88da1d92e0e9cbb9abd287df
aaaa9878405f3f38f4f61c00feca110d7f9ca481 refactor: Use C++17 std::array deduction for ALL_FEE_ESTIMATE_HORIZONS (MarcoFalke)
fa39cdd072c91eac70cda04b8b26681611f94cb7 refactor: Use C++17 std::array deduction for OUTPUT_TYPES (MarcoFalke)
Pull request description:
With the new C++17 array deduction rules, an array encompassing all values in an enum can be specified in the same header file that specifies the enum. This is useful to avoid having to repeatedly enumerate all enum values in the code. E.g. the RPC code, but also the fuzz code.
ACKs for top commit:
theStack:
cr ACK aaaa9878405f3f38f4f61c00feca110d7f9ca481 ⚙️
fanquake:
ACK aaaa9878405f3f38f4f61c00feca110d7f9ca481
Tree-SHA512: b71bd98f3ca07ddfec385735538ce89a4952e418b52dc990fb160187ccef1fc7ebc139d42988b6f7b48df24823af61f803b83d47fb7a3b82475f0c0b109bffb7
3e61b8c800180d350621cedda7ec46a48047ff04 doc: Add explicit macdeployqtplus dependencies install step (Hennadii Stepanov)
Pull request description:
This PR adds to macOS docs an explicit step to install `macdeployqtplus` script dependencies that are not part of the [Python Standard Library](https://docs.python.org/3/library/index.html):
- https://pypi.org/project/ds-store/
- https://pypi.org/project/mac-alias/
This change is required on macOS 11 Big Sur:
- #20371
- #20878Close#20878.
ACKs for top commit:
fanquake:
ACK 3e61b8c800180d350621cedda7ec46a48047ff04
Tree-SHA512: d177139ee142d47cb27ad878d721cafcd03403ef861965ff532d712da461416380ec5878f70accf223a552a1f1e65eedb1e0ad72cb7a96791f8a55536ce28645
cb0b7125c14bf97394bd8b43bf2abfb943bb1cf9 doc: libbitcoinconsensus: add missing error code description, fix NBitcoin link (Sebastian Falbesoner)
Pull request description:
This PR improves the libbitcoinconsensus description in `shared-libraries.md` in two ways:
* adds the missing error code description for `bitcoinconsensus_ERR_INVALID_FLAGS` (introduced by commit 5ca8ef299a, PR #8976)
* updates and fixes the link to the NBitcoin implementation (introduced by commit 3361edd010, PR #6430)
* the owner of the `NBitcoin` github repository changed from `NicolasDorier` to `MetacoSA` (redirection still worked though)
* instead of dynamically referring to a file in master with a fixed line number (which is obviously always quickly outdated), use a permalink with a file numbers area
ACKs for top commit:
MarcoFalke:
cr ACK cb0b7125c14bf97394bd8b43bf2abfb943bb1cf9
harding:
Code (documentation) review ACK cb0b7125c14bf97394bd8b43bf2abfb943bb1cf9. Text is clear and seems accurate, and the link checks out.
Tree-SHA512: 9840458db6fb40e71c9852104aefcec5abbaf5054c6123701181dd477cea8c81d3647f376b67692159adf577c9b6305b05b784728bf9f14a753fab5898075a4e
8008ef770f3d0b14d03e22371314500373732143 qt: unlock wallet "OK" button bugfix (Michael Dietz)
Pull request description:
When trying to send a transaction from an encrypted wallet, the ask
passphrase dialog would not allow the user to click the "OK" button
and proceed. Therefore it was impossible to send a transaction
through the gui. It was not enabling the "OK" button after the
passphrase was entered by the user, because it was using the same
form validation logic as the "Change passphrase" flow.
I reported this in a comment in https://github.com/bitcoin-core/gui/issues/136. But then I realized this seems to be a flat out bug.
ACKs for top commit:
MarcoFalke:
review ACK 8008ef770f3d0b14d03e22371314500373732143
hebasto:
ACK 8008ef770f3d0b14d03e22371314500373732143, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: cc09b34c7f3aea09729e1c7ccccff05dc11fec56fee2ad369f2d862979572b1edd8b7e738ffe6e91d35d071b819b0c3e0f5d48bf5e27427a80af4a28893f8aaf
b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e swapped "is" for "==" in literal comparison (Tyler Chambers)
Pull request description:
In Python 3.8+ literal comparisons using "is" instead of "==" produce a SyntaxWarning [source](https://docs.python.org/3.8/whatsnew/3.8.html#changes-in-python-behavior).
I checked the entire devtools directory, this seems to be the only occurrence.
This is a small fix, but removes the SyntaxWarning.
Fixes: #20338
ACKs for top commit:
hebasto:
re-ACK b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e, only squashed since my [previous](https://github.com/bitcoin/bitcoin/pull/20346#pullrequestreview-525934568) review.
practicalswift:
re-ACK b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e: patch still looks correct
theStack:
utACK b6121edf70a8d50fd16ddbba0c3168e5e49bfc2e
Tree-SHA512: 82a43495d6552fbaa3b02b58f0930b049d27aa937fe44b47714e3c059f844cc494de20674557371cbccf24fb8873ecb7376fb965ae326847eed2b855ed2d59c6
174f58c1857468252a8b9214abd187fa296e883c Add link to NetBSD release (Marnix)
Pull request description:
For consistency with other Build Guides, like `doc/build-freebsd.md` & `doc/build-openbsd.md`
ACKs for top commit:
theStack:
Code-review ACK 174f58c1857468252a8b9214abd187fa296e883c
vincenzopalazzo:
ACK 174f58c185
Tree-SHA512: 08297aac82ee8fab2bbcb486b13b9c6ca12fb4fba573e9979e94204bd7340c2d13f1e54e07b314498603c291c25e29f2e89141ee150478951f699772538b709c
5dc6d9207778c51c10c16fac4b3663bc7905bafc test: make BIP157 messages default-constructible (MESSAGEMAP compatibility) (Sebastian Falbesoner)
71e4cfefe765c58937b3fd3125782ca8407315d2 test: p2p: add missing BIP157 message types to MESSAGEMAP (Sebastian Falbesoner)
Pull request description:
The script [message-capture-parser.py](https://github.com/bitcoin/bitcoin/blob/master/contrib/message-capture/message-capture-parser.py) currently doesn't support parsing the BIP157 messages `getcfilters`, `getcfheaders` and `getcfcheckpt`, e.g.
```
$ ./contrib/message-capture/message-capture-parser.py msgs_recv.dat
...
WARNING - Unrecognized message type b'getcfcheckpt' in /home/thestack/bitcoin/msgs_recv.dat
...
```
This PR fixes this by adding the missing message type mappings to the [`MESSAGEMAP`](225e5b57b2/test/functional/test_framework/p2p.py (L95-L127)) in the test framework and add default-constructors for the corresponding `msg_`... classes.
Without the second commit, the following error message would occur:
```
File "/home/thestack/bitcoin/./contrib/message-capture/message-capture-parser.py", line 141, in process_file
msg = MESSAGEMAP[msgtype]()
TypeError: __init__() missing 2 required positional arguments: 'filter_type' and 'stop_hash'
```
ACKs for top commit:
dunxen:
tACK [5dc6d92](5dc6d92077)
Tree-SHA512: d656c4d38a856373f01d7c293ae7d2b27378a9fc248048ebf2a64725ef8b498b3ddf4f420704abdb20d0c68ca548f1777602c5e73b66821a20c97ae618f1d63f
faac67cab02a755b0ce67716c5e5889432b13b83 test: Fix intermittent race in p2p_unrequested_blocks.py (MacroFake)
Pull request description:
Disconnect may also result in an `OSError`, not only an `AssertionError`. Instead of maintaining a dead code path and enumerating disconnect reasons, just assume disconnection happens every time.
ACKs for top commit:
jamesob:
Code review ACK faac67cab0
Tree-SHA512: d2cec003168e421a5faed275cb2e1ef9fc63f9e8514f41d21da17e8964c79e5b453ccd72cd7ec62805f45293cf877be5bc8124ae98a515c0aa42d6e053409653
11e79084845a78e2421ea3abafe0de5a54ca2bde prevector: only allow trivially copyable types (Martin Leitner-Ankerl)
Pull request description:
prevector uses `memmove` to move around data, that means it can only be used with types that are trivially copyable. That implies that the types are trivially destructible, thus the checks for `is_trivially_destructible` are not needed.
ACKs for top commit:
w0xlt:
ACK 11e7908484
MarcoFalke:
review ACK 11e79084845a78e2421ea3abafe0de5a54ca2bde 🏯
ajtowns:
ACK 11e79084845a78e2421ea3abafe0de5a54ca2bde -- code review only
Tree-SHA512: cbb4d8bfa095100677874b552d92c324c7d6354fcf7adab2ed52f57bd1793762871798b5288064ed1af2d2903a0ec9dbfec48d99955fc428f18cc28d6840dccc
1df42bc262d994c30d390ea73b232b8d2548899e test: compare `/mempool/info` response with `getmempoolinfo` RPC (brunoerg)
Pull request description:
This PRs compares `/mempool/info` REST response with `getmempoolinfo` RPC in `interface_rest.py`.
Similar to #24936 and #24797.
ACKs for top commit:
theStack:
ACK 1df42bc262d994c30d390ea73b232b8d2548899e
Tree-SHA512: 2de36d70fa61612e7976f875e55f98e78b1cdb909b48cff18e6a70c55eda34b799e210bcd55361ea947388b7778d867290a73be4f799bb36afd65423ad49c487
7fd0860d12da723a228ddd3c9fb905380eb8b379 Bugfix: configure: Define defaults for enable_arm_{crc,shani} (Luke Dashjr)
Pull request description:
Fix for #17398 and #24115
Trivial, mostly for consistency (you'd have to *try* to break this)
ACKs for top commit:
pk-b2:
ACK 7fd0860d12
seejee:
ACK 7fd0860d12
vincenzopalazzo:
ACK 7fd0860d12
Tree-SHA512: 51c389787c369f431ca57071f03392438bff9fd41f128c63ce74ca30d2257213f8be225efcb5c1329ad80b714f44427d721215d4f848cc8e63060fa5bc8f1f2e
bdc6881e2f796f4a9a5873826219e24f17a96a7c wallet: Change log interval to use `steady_clock` (w0xlt)
Pull request description:
This refactors the log interval variables to use `steady_clock` as it is best suitable for measuring intervals.
ACKs for top commit:
laanwj:
This makes sense. Code review ACK bdc6881e2f796f4a9a5873826219e24f17a96a7c
dunxen:
Code review ACK bdc6881
Tree-SHA512: 738b4aa45cef01df77102320f83096a0a7d0c63d7fcf098a8c0ab16b29453a87dc789c110105590e1e215d03499db1d889a94f336dcb385b6883c8364c9d39b7
3258bad996262792ba77573d6080dafa3952929c changes color of skipped functional tests (Jacob P. Fickes)
Pull request description:
changes the color of skipped functional tests (currently grey and can be hard to read/invisible on dark backgrounds) to yellow.
resolves#24791
ACKs for top commit:
theStack:
Tested ACK 3258bad996262792ba77573d6080dafa3952929c
jarolrod:
Tested ACK 3258bad996
Tree-SHA512: 3fe5ae0d3b4902b2b6bda6e89ab780feb8bf4b7cb1ce7e8467057b94a1e0a26ddeaf3cac0bc19b06ef10d8bccaac9c495029d42740fbedab8fb0d5fdd7d02eaf
fa4652ce5995ace831b6a4d3125bfcac9563ff6f Pass lifetimebound reference to SingleThreadedSchedulerClient (MacroFake)
Pull request description:
Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference.
All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines.
ACKs for top commit:
pk-b2:
ACK fa4652ce59
jonatack:
Code review ACK fa4652ce5995ace831b6a4d3125bfcac9563ff6f rebased to master, debug build, unit tests
vincenzopalazzo:
ACK fa4652ce59
Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
e5485e8e4be7f2ee0671f58c3dcce35c68ba0ee0 test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee509025d92db5311c3f5df3b61c09cad24f validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack)
Pull request description:
along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench).
A swap must not fail; when a class has a swap member function, it should be declared noexcept.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail
ACKs for top commit:
pk-b2:
ACK e5485e8e4b
w0xlt:
ACK e5485e8e4b
Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
bef61496ab5e12e38ac5794cd0836723af070ab5 test: compare `/mempool/contents` response with `getrawmempool` RPC (brunoerg)
5bc5cbaf310f60e89c72e8ecf3f6187c85499027 doc: add reference to `getrawmempool` RPC in `/mempool/contents` REST doc (brunoerg)
Pull request description:
This PR is similar to #24797, it compares `/mempool/contents` REST response with `getrawmempool` RPC (verbose=True) since they use the same `MempoolToJSON` function.
Also, adds a reference to `getrawmempool` RPC help to get details about the fields from `/mempool/contents`.
ACKs for top commit:
0xB10C:
ACK bef6149
Tree-SHA512: b7e9e9c765ee837986ba167b9234a9b95c9ef0a9ebcc2a03d50f6be6d3aba1480bd77c78111d95df1e4023cde6dfc64bf1e7908d9e5b6f96ca46b76611a4a9b4
