23083856a5 [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25a [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e [init] Add -peerblockfilters option (Jim Posen)
Pull request description:
Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.
`NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.
ACKs for top commit:
jonatack:
Code review re-ACK 23083856a5 the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
fjahr:
re-ACK 23083856a5
jkczyz:
re-ACK 23083856a5
ariard:
re-Code Review ACK 2308385
clarkmoody:
Tested ACK 23083856a
MarcoFalke:
re-ACK 23083856a5🌳
theStack:
ACK 23083856a5
Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
cbd661122e Set LD_LIBRARY_PATH consistently in travis tests (Russell Yanofsky)
fa35c34df7 Remove unused ci configs that have been moved elsewhere (MarcoFalke)
3333cb9699 fuzz: Pass down MAKEJOBS to test_runner (MarcoFalke)
Pull request description:
Just how `MAKEJOBS` is passed down to the functional test `test_runner`, do the same for the fuzz `test_runner`.
Also includes a commit to remove unused config files, which have been moved elsewhere.
Top commit has no ACKs.
Tree-SHA512: 32557102c9e40599b432aeb004c8427e8fbb07cdf4048050cdc8241d1b029aaad306b1131007eeca8315a4f71c38a7efbb833310e056cd11b835676cd19b8902
83da576f44 net: use CMessageHeader::HEADER_SIZE, add missing include (Jon Atack)
Pull request description:
as suggested 16 months ago by Gleb Naumenko in https://github.com/bitcoin/bitcoin/pull/15197#issuecomment-456181865.
`static constexpr CMessageHeader::HEADER_SIZE` is already used in this file, `src/net.cpp`, in 2 instances. This commit replaces the remaining 2 integer values in the file with it and adds the explicit include header.
Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com>
ACKs for top commit:
naumenkogs:
utACK 83da576
practicalswift:
ACK 83da576f44 -- patch looks correct
theStack:
ACK 83da576f44 -- verified that its just magic number elimination refactoring and additionally checked that all tests pass 👍
Tree-SHA512: 5b915483bca4ea162c259865a1b615d73b88a1b1db3f82db05f770d10b8a42494d948f5b21badbcce2d9efa5915b8cbb6af83073867c23d2f152c0d35ac37b96
d97fac422e Add a link from ZMQ doc to ZMQ example in contrib/ (Damian Mee)
Pull request description:
No code changes :). Only a small convenience improvement in zmq doc.
ACKs for top commit:
fanquake:
ACK d97fac422e
Tree-SHA512: f05a8a7a77c0a698637fd24ffc94d0d617743b434f46695a56576a53331ede254aeece416baf3f8275ae4dfad85ae6e14d1920aa32af53150847420a176d90fb
89fea68ffd build: don't pass -w when building for Windows (fanquake)
Pull request description:
This has been around since the introduction of autotools. However at
this point I'm not sure we'd ever want to suppress all warnings when
performing a build, and given that CXX FLAGS will have been overriden
when cross-compiling for Windows (using depends), this would rarely,
if-ever be used anyways.
From https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html:
> -w
>
> Inhibit all warning messages.
ACKs for top commit:
hebasto:
ACK 89fea68ffd
Tree-SHA512: 2b5bdef7fff5c87b28199f5822cab3cdf600c90c01a40db5cd85053eef5dcb5816e2e97ff61a30ff94b4f0c6cb7be22beaef34d82235bdf05ff9da865d40b381
9847e205bf [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e0 [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c8 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
Pull request description:
Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.
ACKs for top commit:
sipa:
utACK 9847e205bf
brakmic:
ACK 9847e205bf
luke-jr:
utACK 9847e205bf
naumenkogs:
utACK 9847e20
ajtowns:
utACK 9847e205bf
Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
d044e0ec7d refactor: Remove override for final overriders (Hennadii Stepanov)
1551cea2d5 refactor: Use override for non-final overriders (Hennadii Stepanov)
Pull request description:
Two commits are split out from #16710 to make reviewing [easier](https://github.com/bitcoin/bitcoin/pull/16710#issuecomment-625760894).
From [C++ FAQ](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final):
> C.128: Virtual functions should specify exactly one of virtual, override, or final
> **Reason** Readability. Detection of mistakes. Writing explicit `virtual`, `override`, or `final` is self-documenting and enables the compiler to catch mismatch of types and/or names between base and derived classes. However, writing more than one of these three is both redundant and a potential source of errors.
ACKs for top commit:
practicalswift:
ACK d044e0ec7d: consistent use of `override` prevents bugs + patch looks correct + Travis happy
MarcoFalke:
ACK d044e0ec7d, based on my understanding that adding `override` or `final` to a function must always be correct, unless it doesn't compile!?
vasild:
ACK d044e0ec7
Tree-SHA512: 245fd9b99b8b5cbf8694061f892cb3435f3378c97ebed9f9401ce86d21890211f2234bcc39c9f0f79a4d2806cb31bf8ce41a0f9c2acef4f3a2ac5beca6b077cf
872aa25fa1 doc: add c++17-enable to fuzzing instructions (Martin Zumsande)
Pull request description:
Update the fuzzing doc because after the merge of #18901, C++17 is required for compilation.
ACKs for top commit:
practicalswift:
ACK 872aa25fa1
MarcoFalke:
ACK 872aa25fa1
Tree-SHA512: 47e37c033690de1d1fa644bf0cebb256036b32a5784021cc0d3b32e6188822d7f517d4342990dc7ec98de6d650794aeb85483157e69e141d6bd011993e124575
68537275bd build: Enable -Werror=sign-compare (Ben Woosley)
eac6a3080d refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377e30 test: Fix outstanding -Wsign-compare errors (Ben Woosley)
Pull request description:
Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.
In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.
This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee vs 75fb37ce68
ACKs for top commit:
fjahr:
re-ACK 68537275bd
practicalswift:
ACK 68537275bd
Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
Remove inconsistency between functional and unit test environments and make it
possible to substitute bitcoin-qt and bitcoin-node in place of bitcoind in
python tests, or to link bitcoind against shared libraries.
static constexpr CMessageHeader::HEADER_SIZE is already used in this file,
src/net.cpp, in 2 instances. This commit replaces the remaining 2 integer
values with it and adds the explicit include header.
Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com>
This has been around since the introduction of autotools. However at
this point I'm not sure we'd every want to suppress all warnings when
performing a build, and given that CXX FLAGS will have been overriden
when cross-compiling for Windows (using depends), this would rarely,
if-ever be used anyways.
From https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html:
-w
Inhibit all warning messages.
420fa0770f fuzz: use std::optional for sep_pos variable (Harris)
Pull request description:
This PR changes the original `size_t sep_pos` to `std::optional<size_t> sep_post_opt` to remove the warning when compiling fuzz tests.
```shell
warning: variable 'sep_pos' may be uninitialized when used here [-Wconditional-uninitialized]
```
Also, it adds `--enable-c++17` flag to CI fuzz scripts.
ACKs for top commit:
practicalswift:
ACK 420fa0770f
MarcoFalke:
ACK 420fa07
Tree-SHA512: e967d5d8ab8ee7394b243ff5b28bac72d30bd14774e4a206f8c87474fad22769da76e4ba4e03cbef83b8f60e5293e9d9293b613e2e2e59e187d4e59ae6b874ca
095bc9a106 fuzz: fix vector size problem in system fuzzer (Harris)
Pull request description:
This PR fixes a problem with vector resizing in system fuzzer (*case 7* there). Originally, this problem was discussed in PR https://github.com/bitcoin/bitcoin/pull/18908
ACKs for top commit:
MarcoFalke:
ACK 095bc9a106
practicalswift:
ACK 095bc9a106
brakmic:
> ACK [095bc9a](095bc9a106)
Tree-SHA512: 73e6004ee51d68a34b49c79d1329a8c4865c21da888801c0fcc7f1bcacb510bf371bb61675eda83e53d08e0f24712e671369719523b0ced0eb2a22607bfa1d3d
When a node is configured with --blockfilterindex=basic and
-peerblockfilters it can serve compact block filters to its peers.
This commit adds the configuration option handling. Future commits
add compact block serving and service bits signaling.
d135c29476 [ci] make list of previous releases to download a setting (Sjors Provoost)
9c246b873c [test] backwards compatibility: bump v0.19.0.1 to v0.19.1 (Sjors Provoost)
89a28e02fa [test] add v0.16.3 backwards compatibility test (Sjors Provoost)
Pull request description:
Thanks to #18774's `adjust_bitcoin_conf_for_pre_17` we can now test backwards compatibility for v0.16.3, both for sync and loading a recent wallet.
This PR bumps v0.19.0.1 to v0.19.1.
I also made the version list consistent for the `contrib/devtools/previous_release.sh` instruction, between both tests.
ACKs for top commit:
MarcoFalke:
ACK d135c29476
Tree-SHA512: 5ff137a7a934237fa220f1c2807ce9abeeb75929266558bf3e4045bec7dfcd0a8747fa74d700065c568330b18badf58c60c308eb13d1eed444d4bbfe6decc48b
1e94a2bcbc depends: Add --sysroot option to mac os native compile flags (Russell Yanofsky)
Pull request description:
Catalina SDK clang stopped automatically searching the SDK include paths when invoked without `--sysroot`:
- https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-594600985
- https://github.com/Homebrew/homebrew-core/issues/45061
This hasn't been a problem for current native depends packages because are passing their own `--sysroot` values, and hasn't been a problem for current host packages because they use `darwin_` commands instead of `build_darwin_` commands. But the current `build_darwin_CC` and `build_darwin_CXX` commands are still unnecessarily fragile, and incompatible with new native depends packages added in https://github.com/bitcoin/bitcoin/pull/18677.
Cory Fields (theuni) suggested in https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-595393546 switching compiler from SDK clang to native clang (from $PATH) to avoid this problem. This is easy and makes a certain amount of sense for building native packages, as opposed to host packages. But Michael (fanquake) pointed out in https://github.com/bitcoin/bitcoin/pull/18677#discussion_r409934309 that it would be inconsistent to switch to non-SDK compilers while still using other SDK tools like `ranlib` and `install_name_tool`. So simplest, minimal fix seems to be just adding the missing `--sysroot` option.
ACKs for top commit:
ryanofsky:
> ACK [1e94a2b](1e94a2bcbc) - I think this change is ok, and I prefer it to the previous patch.
fanquake:
ACK 1e94a2bcbc - I think this change is ok, and I prefer it to the previous patch. Thanks for the summary in the PR description. I played around with Xcode and the CLT; I think previously I didn't fully grok the slight differences between the two.
Tree-SHA512: 4d4bbb7f49acb76d934a872a15b4e14f36290b508cb9e728815f959767ec174bcfb6d2ca7dcd995cc550d86980d64d4247ea5ecfca2301f0953006e50744fdb4
fa082d0a57 travis: Remove valgrind (MarcoFalke)
Pull request description:
When the valgrind run was added, it took 2 hours. Travis kindly raised the timeout limit to the maximum possible of 3 hours.
Today, a full build of Bitcoin Core with all tests takes more than three hours. Thus, it is impossible to run all tests on travis.
Moreover, the feedback loop for developers that create a pull request takes at least 2 hours, but in some cases (when the travis queue is full) until the next day. This is unacceptable.
Fix both issues by removing the build from travis.
Please note that the `ci/test/` configurations are *not* removed. They will stay in the repo and can be executed anywhere (just not on travis).
ACKs for top commit:
jamesob:
ACK fa082d0a57
jnewbery:
utACK fa082d0a57
Tree-SHA512: 9acaa0e2d3926014fadb7dd2e86c4e01df382e9399f6ae99f989fa609da66a77bdd1b75d6ff42d2686f38f730b8564e6dc722aa597a473290c9d30c2abe7ef0f
a029805f57 build: remove -Qunused-arguments workaround for clang + ccache (fanquake)
Pull request description:
This was added in 386efb7695 to address spammy Clang warnings when building with ccache.
The issue was addressed in [ccache 3.2](https://bugzilla.samba.org/show_bug.cgi?id=8118), and from a look at most major distros, it's only Debian Jessie that has a version of ccache older than that ([3.1](https://packages.debian.org/jessie/ccache)).
Therefore I think it's acceptable to drop this workaround, and re-enable warnings for unused driver arguments (when compiling using Clang and ccache).
ACKs for top commit:
hebasto:
ACK a029805f57.
vasild:
utACK a029805f57
Tree-SHA512: f887b9bd12f9c1c8d209943b86e8dafe33cfd1572912f2cafabe08ffe403973e48f0f7289280a8c6db9263c57aad43fbd4bb72f42db762eb090f3b1ef0538f43
03da4c7781 build: make linker checks more robust (Cory Fields)
Pull request description:
Check for a flag to turn linker warnings into errors. When flags are passed to
linkers via the compiler driver using a -Wl,-foo flag, linker warnings may be
swallowed rather than bubbling up.
This is one of [Corys commits](b9acd3d33e) that I've modified to also add `-Wl,-fatal_warnings`
for darwin.
ACKs for top commit:
vasild:
re-ACK 03da4c778
Tree-SHA512: 212031d619ed88e52aaae30cf3b711681d72c4d670884406403605d1d86c784c84cb07e2e0d6c30926e659db8f14f8dabd5af3de5291637f8080d6dfee358248
748977690e Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille)
7cf97fda15 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille)
c81aefc537 Add additional effiency checks to sanity checker (Pieter Wuille)
fffd8dca2d Add asmap sanity checker (Pieter Wuille)
5feefbe6e7 Improve asmap Interpret checks and document failures (Pieter Wuille)
2b3dbfa5a6 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille)
1479007a33 Introduce Instruction enum in asmap (Pieter Wuille)
Pull request description:
This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.
In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.
I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.
ACKs for top commit:
practicalswift:
ACK 748977690e modulo feedback below.
jonatack:
ACK 748977690e code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.
fjahr:
ACK 748977690e
Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821
fa47cf9d95 wallet: Fix typo in assert that is compile-time true (MarcoFalke)
Pull request description:
Commit 92bcd70808 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`.
However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
ACKs for top commit:
Sjors:
utACK fa47cf9d95
Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
71f183a49b build: warn on potentially uninitialized reads (Vasil Dimov)
Pull request description:
* Enable `conditional-uninitialized` warning class to show potentially uninitialized
reads.
* Fix the sole such warning in Bitcoin Core in `GetRdRand()`: `r1` would be
set to `0` on `rdrand` failure, so initializing it to `0` is a non-functional
change.
ACKs for top commit:
practicalswift:
ACK 71f183a49b
laanwj:
ACK 71f183a49b
Tree-SHA512: 2c1d8caacd86424b16a9d92e5df19e0bedb51ae111eecad7e3bfa46447bc88e5fff1f32dacf6c4a28257ebb3d87e79f80f074ce2c523ce08b1a0c0a67ab44204
fa09110ebb doc: Fix typo in Coin doxygen comment (MarcoFalke)
Pull request description:
`CTxOutCompressor` has been renamed in commit 4de934b9b5, so rename it in the docs as well.
ACKs for top commit:
laanwj:
ACK fa09110ebb
hebasto:
ACK fa09110ebb
Tree-SHA512: e16a21ac3112a67ee7d5ffabb3f47103aed8f91fdebf1bf96311cd0b7bdb9b7323ed826bfa95517386d4128ff0ae2c7c13bad047a7c5a0cc2458be7a43119157
Check for a flag to turn linker warnings into errors. When flags are passed to
linkers via the compiler driver using a -Wl,-foo flag, linker warnings may be
swallowed rather than bubbling up.
Co-authored-by: fanquake <fanquake@gmail.com>
1ad8ea2b73 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)
Pull request description:
The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
> the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)
For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).
The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.
ACKs for top commit:
meshcollider:
utACK 1ad8ea2b73
laanwj:
Concept and code review ACK 1ad8ea2b73
jkczyz:
ACK 1ad8ea2b73
MarcoFalke:
ACK 1ad8ea2b73
fjahr:
Code review ACK 1ad8ea2b73
Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
bfe1ba2f5b rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e01cc build: Accomodate makensis v2.x (Carl Dong)
1f2c39a30e guix: Remove logical cores requirement (Carl Dong)
a4f6ffa71e lint: Also enable source statements for non-gitian (Carl Dong)
d256f91cb1 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da02f nsis: Specify OutFile path only once (Carl Dong)
14701604d0 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4f48 guix: Make source tarball using git-archive (Carl Dong)
395c1137f6 gitian: Limit sourced script to just assignments (Carl Dong)
Pull request description:
Based on: #18556
Related: https://github.com/bitcoin/bitcoin/pull/17595#discussion_r399728721
ACKs for top commit:
fanquake:
ACK bfe1ba2f5b - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase #18818.
Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
28b112e9bd Get rid of BindWallet (Russell Yanofsky)
d002f9d15d Disable CWalletTx copy constructor (Russell Yanofsky)
65b9d8f8dd Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky)
bd2fbc7cdb Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky)
2b9cba2065 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky)
Pull request description:
This is a pure refactoring, no behavior is changing.
Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged.
Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.
This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.
ACKs for top commit:
MarcoFalke:
Anyway, re-ACK 28b112e9bd
Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef
