aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2020-04-22test: check for matching object hashes in wait_for_getdataDanny Lee
2020-04-22Merge #18575: bench: Remove requirement that all benches use same testing setupMarcoFalke
fa1fdb02fccd0f670f7b08ee61c249f04d0db17f bench: Replace ::mempool globabl with test_setup.mempool (MarcoFalke) fab117096446ab63d1f38c1ef6edbc94a5d4ab52 bench: Remove requirement that all benches use RegTestingSetup (MarcoFalke) Pull request description: The benches have always set up one global testing setup. This makes it hard to pick no testing setup at all or one with different params. Fix this by removing any global state setup from the main `bench.cpp` and leave the setup to each individual bench. One reason to have one global testing setup is to set the datadir location to a tempdir to avoid reading or writing in the default datadir location. But #13687 should prevent this already. Top commit has no ACKs. Tree-SHA512: 7c98aea7725a20f4b9225221f4279b9e9f7257ed5c14712ad01ea80d87c3b0fed760b40f413892498bbb354a917ee02d4c575cbe8423a403b86755e8ee11f33b
2020-04-22Merge #18702: build: fix ASLR for bitcoin-cli on WindowsWladimir J. van der Laan
315a4d36f716341a38bc4e4de8630b3246d27dbc build: fix ASLR for bitcoin-cli on Windows (fanquake) Pull request description: ASLR is not currently working for the `bitcoin-cli.exe` binary. This is due to it not having a .reloc section, which is stripped by default by the mingw-w64 ld we use for gitian builds. A good summary of issues with ld and mingw-w64 is available in this thread: https://sourceware.org/bugzilla/show_bug.cgi?id=19011. All other Windows binaries that we distribute (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-tx and test_bitcoin) do not suffer this issue, and currently having working ASLR. This is due to them exporting (inadvertent or not) libsecp256k1 symbols, and, as a result, the .reloc section is not stripped by ld. This change is a temporary workaround, also the same one described here: https://www.kb.cert.org/vuls/id/307144/, that causes main() to be exported. Exporting a symbol will mean that the .reloc section is not stripped, and ASLR will function correctly. Ultimately, this will be fixed by using a newer version of binutils (that has this [change](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0)). Whether that's through bumping our gitian distro, or Guix. Related to #18629, which has a bunch of additional information in the PR description. If you would like to verify whether or not ASLR is indeed working, with or without this change. One easy way to check is using a tool like [VMMap](https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap). Here are the memory mappings for the 0.20.0rc1 `bitcoind.exe` and `bitcoin-cli.exe` binaries. You'll notice that over machine restarts, even though the image is marked `(ASLR)` (which I assume may be due to the header bit being set), no ASLR is actually occuring for `bitcoin-cli.exe`: #### bitcoind.exe ![bitcoind-1](https://user-images.githubusercontent.com/863730/79678203-74065c80-822b-11ea-90bc-9c883d0aeefa.png) ![bitcoind-2](https://user-images.githubusercontent.com/863730/79678204-7668b680-822b-11ea-9263-3e7ba22f904c.png) ![bitcoind-3](https://user-images.githubusercontent.com/863730/79678206-7963a700-822b-11ea-972f-af31a514b9b4.png) #### bitcoin-cli.exe ![bitcoin-cli-1](https://user-images.githubusercontent.com/863730/79678208-7ec0f180-822b-11ea-8480-a4b5d1762945.png) ![bitcoin-cli-2](https://user-images.githubusercontent.com/863730/79678213-81bbe200-822b-11ea-964d-994f58ff12b0.png) ![bitcoin-cli-3](https://user-images.githubusercontent.com/863730/79678215-84b6d280-822b-11ea-9cd6-fee2e239c003.png) ACKs for top commit: dongcarl: ACK 315a4d36f716341a38bc4e4de8630b3246d27dbc laanwj: ACK 315a4d36f716341a38bc4e4de8630b3246d27dbc Tree-SHA512: 95f4dc15420ed9bcdeacb763e11c3c7e563eec594a172746fa0346c13f97db3a8769357dffc89fea1e57ae67133f337b1013a73b584662f5b6c4d251ca20a2b1
2020-04-22Merge #18553: Avoid non-trivial global constants in SHA-NI codeWladimir J. van der Laan
850847309458f43fc7ce6c13fa08c86e1cae042a Avoid non-trivial global constants in SHA-NI code (Pieter Wuille) Pull request description: This is a potential solution for #18456. It seems that the compiler cannot turn `_mm_set_epi64x(<constant>,<constnant>)` into a constant itself, and thus emits a global initializer for the `MASK`, `INIT0`, and `INIT1` global constants in the sha-ni SHA256 implementation. Change this by turning them into dumb byte arrays, loading them into an SSE variable whenever needed. Tested on a SHA-NI capable machine. I do not observe any obvious performance impact (but this is hard to measure, it's already very fast...). ACKs for top commit: laanwj: Code review ACK 850847309458f43fc7ce6c13fa08c86e1cae042a elichai: ACK 850847309458f43fc7ce6c13fa08c86e1cae042a Tree-SHA512: 07049cf1a33624c22df2be48b814d5636c037b368861eb13ee073bdce2b7c902a56e96518218961f55a2a1631a40825ded6dbbc28d7fe0e7fec267d704e39112
2020-04-22Merge #18410: Docs: Improve commenting for coins.cpp|hWladimir J. van der Laan
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
2020-04-22Merge #18665: Do not expose and consider -logthreadnames when it does not workWladimir J. van der Laan
b91e4ae0d8ab2ae6b77585c97c52d825f56ed539 Do not expose and consider -logthreadnames when it does not work (Hennadii Stepanov) Pull request description: There are conditions when the `HAVE_THREAD_LOCAL` macro is undefined what causes the `-logthreadnames` option does not work -- instead of thread names empty strings `[]` only are printed in the `debug.log` file. This PR does not exposes the `-logthreadnames` option in such cases. Refs: - #16059 - #18652 ACKs for top commit: MarcoFalke: ACK b91e4ae0d8ab2ae6b77585c97c52d825f56ed539, looked at the diff, didn't test Tree-SHA512: 3bd58e5ea603c69686589ddc94d6fa441cab4f712004378f2f1661e12638804ca03cfb6426e6393e55b6a095b325f3161d3c5371af05d7fc79d6d328227bf40c
2020-04-22Merge #18612: script: Remove undocumented and unused operator+Wladimir J. van der Laan
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 https://github.com/bitcoin/bitcoin/commit/6ff5f718b6a67797b2b3bab8905d607ad216ee21#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd5190898ece3ac17aa3178f320d091f221df Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9
2020-04-22Merge #18713: scripts: Add MACHO stack canary check to security-check.pyfanquake
8334ee31f868f0f9baf0920d14d20174ed889dbe scripts: add MACHO LAZY_BINDINGS test to test-security-check.py (fanquake) 7b99c7454cdb74cd9cd7a5eedc2fb9d0a19df456 scripts: add MACHO Canary check to security-check.py (fanquake) Pull request description: 7b99c7454cdb74cd9cd7a5eedc2fb9d0a19df456 uses `otool -Iv` to check for `___stack_chk_fail` in the macOS binaries. Similar to the [ELF check](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/security-check.py#L105). Note that looking for a triple underscore prefixed function (as opposed to two for ELF) is correct for the macOS binaries. i.e: ```bash otool -Iv bitcoind | grep chk 0x00000001006715b8 509 ___memcpy_chk 0x00000001006715be 510 ___snprintf_chk 0x00000001006715c4 511 ___sprintf_chk 0x00000001006715ca 512 ___stack_chk_fail 0x00000001006715d6 517 ___vsnprintf_chk 0x0000000100787898 513 ___stack_chk_guard ``` 8334ee31f868f0f9baf0920d14d20174ed889dbe is a follow up to #18295 and adds test cases to `test-security-check.py` that for some reason I didn't add at the time. I'll sort out #18434 so that we can run these tests in the CI. ACKs for top commit: practicalswift: ACK 8334ee31f868f0f9baf0920d14d20174ed889dbe: Mitigations are important. Important things are worth asserting :) jonasschnelli: utACK 8334ee31f868f0f9baf0920d14d20174ed889dbe. Tree-SHA512: 1aa5ded34bbd187eddb112b27278deb328bfc21ac82316b20fab6ad894f223b239a76b53dab0ac1770d194c1760fcc40d4da91ec09959ba4fc8eadedb173936a
2020-04-22Merge #18709: doc: note why we can't use thread_local with glibc back compatfanquake
b155fcda5186c59fc4fb2a9eaaf791d132e0ab30 doc: fix typo in configure.ac (fanquake) 20a30922fbf6ba14e250ca649239af115dbbe7b0 doc: note why we can't use thread_local with glibc back compat (fanquake) Pull request description: Given that we went through a [gitian build](https://github.com/bitcoin/bitcoin/pull/18681) to remember why this is the case, we might as well make a note of it in configure.ac. [From #18681](https://github.com/bitcoin/bitcoin/pull/18681#issuecomment-615526634): Looking at the Linux build log, this has failed with: ```bash Checking glibc back compat... bitcoind: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 bitcoind: failed IMPORTED_SYMBOLS bitcoin-cli: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 bitcoin-cli: failed IMPORTED_SYMBOLS bitcoin-tx: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 bitcoin-tx: failed IMPORTED_SYMBOLS bitcoin-wallet: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 bitcoin-wallet: failed IMPORTED_SYMBOLS test/test_bitcoin: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 test/test_bitcoin: failed IMPORTED_SYMBOLS bench/bench_bitcoin: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 bench/bench_bitcoin: failed IMPORTED_SYMBOLS qt/bitcoin-qt: symbol __cxa_thread_atexit_impl from unsupported version GLIBC_2.18 ``` `__cxa_thread_atexit_impl` is used for [thread_local variable destruction](https://sourceware.org/glibc/wiki/Destructor%20support%20for%20thread_local%20variables): > To implement this support, glibc defines __cxa_thread_atexit_impl exclusively for use by libstdc++ (which has the __cxa_thread_atexit to wrap around it), that registers destructors for thread_local variables in a list. Upon thread or process exit, the destructors are called in reverse order in which they were added. As suggested, this only became available in glibc 2.18. From the [2.18 release notes](https://sourceware.org/legacy-ml/libc-alpha/2013-08/msg00160.html): > * Add support for calling C++11 thread_local object destructors on thread and program exit. This needs compiler support for offloading C++11 destructor calls to glibc. ACKs for top commit: hebasto: ACK b155fcda5186c59fc4fb2a9eaaf791d132e0ab30 Tree-SHA512: 5b9567e4a70598a4b0b91956f44ae0d93091db17c84cbf9817dac6cfa992c97d3438a8b1bb66644c74891f2149e44984daed445d22de93ca8858c5b0eabefb40
2020-04-21[docs] use consistent naming for possible_overwriteJohn Newbery
And other general comment improvements for adding coins.
2020-04-21[tests] small whitespace fixupJohn Newbery
Required after scripted-diff in previous commit.
2020-04-21scripted-diff: Rename PRUNED to SPENT in coins testsJohn Newbery
-BEGIN VERIFY SCRIPT- sed -i -e 's/PRUNED,/SPENT ,/g' ./src/test/coins_tests.cpp sed -i -e 's/PRUNED/SPENT/g' ./src/test/coins_tests.cpp -END VERIFY SCRIPT-
2020-04-21[docs] Improve commenting in coins.cpp|hJohn Newbery
Remove references to 'pruned' coins, which don't exist since the move to per-txout coins db.
2020-04-21Merge #18384: [test] more specific feature_segwit test error messages and ↵MarcoFalke
fixing incorrect comments 3c21db7b78fb0a27a0ea61b64eede8af0972791f [test] add 8 error messages to feature_segwit and change version to type (Gloria Zhao) Pull request description: Followup to [this](https://github.com/bitcoin/bitcoin/pull/15169/files#r303673472) comment on functional test feature_segwit.py verifying that unsigned witness transactions are invalid. (1) Changes 8 error messages from "mandatory-script-verify-flag" to "non-mandatory-script-verify-flag" and with more specific error messages. (2) Edits comments that incorrectly describe the test, namely that the `v` variable corresponds to using P2WSH versus P2WPKH, not witness versions. ACKs for top commit: MarcoFalke: ACK 3c21db7b78 🍾 Tree-SHA512: 3734ea3762667636c4fb20f5285634ab94d6b3527b7390fcc5e41b4582829dfe0099beabeaed42098613d168ede3385a6ffcd73989d1fa9dbd18004f5e9cf083
2020-04-21[test] add 8 error messages to feature_segwit and change version to typeGloria Zhao
P2WPKH witness program without signature -> throws "hash mismatch" error P2WSH witness program without signature -> throws "empty witness" error same errors for P2SH_P2WPKH and P2SH_P2WSH respectively when passed redeemScript but no signature P2SH_P2WPKH and P2SH_P2WSH with no signature fail with "Operation not valid with current stack size" when not signed due to missing input change VER to TYPE and constants WIT_V0 to P2WPKH=0 and WIT_V1 to P2WSH=1
2020-04-21Merge #18704: test: Increase debugging to hunt down mempool_reorg ↵MarcoFalke
intermittent failure fac2fc4dd8a28b99e17c57e4ab6580a3231f1d0a test: Increase debugging to hunt down mempool_reorg intermittent failure (MarcoFalke) Pull request description: Top commit has no ACKs. Tree-SHA512: 4094b44afaa623e58b69f8d0332e60f0150b9ae2fd8bb265210d85546d887672ab8a3435cd9b086be14f69ab5b17e0f9fae06bd8aec1e7947ca766dd72b577c4
2020-04-21Merge #18721: test: Fix linter issueMarcoFalke
60cdcf30a4ddd29907513f32b2e607e092c96179 test: Fix linter issue (Hennadii Stepanov) Pull request description: Top commit has no ACKs. Tree-SHA512: 4fa0103526fed4b1399d3a6b83cea1e509cefff36d7e16ed1499d22afdc49e72053ac3d0634d858838ec6c296af7134131ea4d509c46de99da557567bc75d711
2020-04-21test: Fix linter issueHennadii Stepanov
2020-04-21Merge #18672: test: add further BIP37 size limit checks to p2p_filter.pyMarcoFalke
c7437185589926ec8def2af6bede6a407b3d2e4a test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner) Pull request description: This is a follow-up PR to #18628. In addition to the hash-functions limit test introduced with commit https://github.com/bitcoin/bitcoin/pull/18628/commits/fa4c29bc1d2425f861845bae4f3816d9817e622a, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki): ad message type `filterload`: > The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**. ad message type `filteradd`: > The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object). Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit). Also fixes #18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with #18544) below to also use the more commonly used `assert_debug_log` method. ACKs for top commit: MarcoFalke: ACK c7437185589926ec8def2af6bede6a407b3d2e4a robot-visions: ACK c7437185589926ec8def2af6bede6a407b3d2e4a jonasschnelli: utACK c7437185589926ec8def2af6bede6a407b3d2e4a. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524 Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be
2020-04-21scripts: add MACHO LAZY_BINDINGS test to test-security-check.pyfanquake
I didn't add the relevant test in #18295.
2020-04-21scripts: add MACHO Canary check to security-check.pyfanquake
2020-04-21doc: fix typo in configure.acfanquake
s/liner/linker/
2020-04-20Merge #18190: tests: Add fuzzing harness for Golomb-Rice coding ↵MarcoFalke
(GolombRiceEncode/GolombRiceDecode) 69749fbe6a95f45eb7a695a5f89be87e55c91fb8 tests: Add fuzzing harness for Golomb-Rice coding (GolombRiceEncode/GolombRiceDecode) (practicalswift) Pull request description: Add fuzzing harness for Golomb-Rice coding (`GolombRiceEncode`/`GolombRiceDecode`). Test this PR using: ``` $ make distclean $ ./autogen.sh $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/golomb_rice … ``` Top commit has no ACKs. Tree-SHA512: 1b26512301b8c22ab3b804d9b9e4baf933f26f8c05e462d583863badcec7e694548a34849a0d7c4ff7d58b19f6338b51819976ecf642bc4659b04ef71182d748
2020-04-20Merge #18676: build: Check libevent minimum version in configure scriptWladimir J. van der Laan
b68e71796792a9da9daa0a4e759d284d15595230 build: Set libevent minimum version to 2.0.21 (Hennadii Stepanov) Pull request description: The non-`pkg-config` path is ignored as there is a hope to get rid of all of them in #18307. As xenial has [libevent 2.0.21](https://packages.ubuntu.com/xenial-updates/libevent-2.0-5) only, the default bionic Docker image is used in the _"[no depends, only system libs, sanitizers: thread (TSan), no wallet]"_ CI test. ACKs for top commit: theStack: utACK https://github.com/bitcoin/bitcoin/pull/18676/commits/b68e71796792a9da9daa0a4e759d284d15595230 laanwj: ACK b68e71796792a9da9daa0a4e759d284d15595230 Tree-SHA512: 9825c42aeb166165e99fe5eaf74dbb47c2b51aecdbe53c5ae949fe126e1b8e8b6fe8d228fdde4e8daa4243e5907954202f42eb23c71629e4b2b92a7d4eb892e4
2020-04-20test: add further BIP37 size limit checks to p2p_filter.pySebastian Falbesoner
also unified method of detecting misbehaviour (using assert_debug_log instead of checking peer's banscore)
2020-04-20tests: Add fuzzing harness for Golomb-Rice coding ↵practicalswift
(GolombRiceEncode/GolombRiceDecode)
2020-04-20Merge #18705: ci: Remove xenial tsan workaroundMarcoFalke
faebcd4e8d3bb72f0b8ce849716241be8b46c1da ci: Remove xenial tsan workaround (MarcoFalke) Pull request description: ACKs for top commit: hebasto: ACK faebcd4e8d3bb72f0b8ce849716241be8b46c1da Tree-SHA512: 5d7e15be211e526948f863f573dbb5a97005262241ba4a07858346cba3a17cb24b1473df347224e0e4f2b22201750e27fba80ffe0d1dddf85f4e8f9341a8c129
2020-04-20Merge #17579: [refactor] Merge getreceivedby tally into GetReceived functionMarcoFalke
a1d5b12ec07d2f7aa9fa955a6dd99e8a2be5cb25 Merge getreceivedby tally into GetReceived function (Andrew Toth) Pull request description: This PR merges the tally code of `getreceivedbyaddress` and `getreceivedbylabel` into a single function `GetReceived`. This reduces repeated code and makes it similar to `listreceivedbyaddress` and `listreceivedbylabel`, which use the function `ListReceived`. It will also make the change in #14707 simpler and easier to review. ACKs for top commit: theStack: re-ACK https://github.com/bitcoin/bitcoin/commit/a1d5b12ec07d2f7aa9fa955a6dd99e8a2be5cb25 meshcollider: utACK a1d5b12ec07d2f7aa9fa955a6dd99e8a2be5cb25 Tree-SHA512: 43d9cd92f7c2c6a8b9c7509aa85a9b9233a6cfec1c43a9062e3bdfb83515413d1feafa8938c828351278ba22bd31c47e62ab5341e4bddc2493103b094d73b047
2020-04-20Do not expose and consider -logthreadnames when it does not workHennadii Stepanov
2020-04-20Merge #17831: rpc: doc: Fix and extend getblockstats examplesMarcoFalke
709998467e1c1bc7980662c9f88fbc7964602d33 rpc: doc: Fix and extend getblockstats examples (Adam Soltys) Pull request description: This pull fixes the example curl command for `getblockstats` which doesn't work as is because it's missing a comma between the params and has single quotes around the second parameter. It also adds an additional example of getting block stats by hash by using a known workaround (#15412) to get bitcoin-cli to treat the hash parameter as JSON instead of a string since there is ongoing deliberation about how or whether to fix the root issue (#15448). ACKs for top commit: theStack: ACK https://github.com/bitcoin/bitcoin/pull/17831/commits/709998467e1c1bc7980662c9f88fbc7964602d33 Tree-SHA512: 84a5b7f449f06fff785bc0afbc1a7dfd55454bc76c52a8945e91556f87f3edfdc5a1780faab8fcfd6c415b734295b7c67d2e04ba7b6cfa91a77758af5dda53ae
2020-04-20Merge #18544: net: limit BIP37 filter lifespan (active between ↵MarcoFalke
'filterload'..'filterclear') a9ecbdfcaa15499644d16e9c8ad2c63dfc45b37b test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner) 5eae034996b340c19cebab9efb6c89d20fe051ef net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner) Pull request description: This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed: https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net.h#L812 and after a loaded filter is removed again through a `filterclear` message: https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3201 The behaviour was introduced by commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell). This default match-everything filter leads to some unintended side-effects: 1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L1504-L1507 2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3182-L3186 This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message. Here is a before/after comparison in behaviour: | code part / scenario | master branch | PR branch | | --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- | | `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload` | | `filteradd` processing, no filter was loaded | nothing | peer's banscore increases by 100 (i.e. disconnect) | On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch). Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set. ACKs for top commit: MarcoFalke: re-ACK a9ecbdfcaa, only change is in test code 🕙 Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
2020-04-20Merge #18691: test: add wait_for_cookie_credentials() to framework for ↵MarcoFalke
rpcwait tests 92fe537cf704dfb4ae830c8c8b382f08c4893e65 test: fix intermittent race condition in interface_bitcoin_cli.py (Jon Atack) c648e636b2f230db5f1d1137088685f74ae42454 test: add wait_for_cookie_credentials() to test framework (Jon Atack) Pull request description: This PR adds a `wait_for_cookie_credentials()` method to the test framework and calls it before the `-rpcwait` tests, to avoid an intermittent race condition on the CI run with Valgrind where the cookie file isn't written yet when the CLI call with `-rpcwait` arrives to `get_auth_cookie()`. To reproduce/test, build with ```diff diff --git a/src/httprpc.cpp b/src/httprpc.cpp index 60c4d06f12..3dd06c4758 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -291,6 +291,7 @@ static bool InitRPCAuthentication() bool StartHTTPRPC() { LogPrint(BCLog::RPC, "Starting HTTP RPC server\n"); + UninterruptibleSleep(std::chrono::seconds{11}); if (!InitRPCAuthentication()) ``` then run the test normally and with valgrind ``` test/functional/interface_bitcoin_cli.py -l debug valgrind test/functional/interface_bitcoin_cli.py -l debug ``` Thanks to Marco Falke for all the help. Closes #18684. Top commit has no ACKs. Tree-SHA512: 1b76635b5b1d6b05138affef7ab788aa3bc3fc75b0c69ba778ecdf81063cfe02a8dd7667cfd63a6c6e19b2dac47d7a8b755e334d8af5c0ab9d4026808ee96c83
2020-04-20doc: note why we can't use thread_local with glibc back compatfanquake
2020-04-19test: fix intermittent race condition in interface_bitcoin_cli.pyJon Atack
by calling wait_for_cookie_credentials() to ensure the cookie file is written and auth credentials available for testing the CLI -rpcwait option before the RPC connection is up.
2020-04-19test: add wait_for_cookie_credentials() to test frameworkJon Atack
to be able to ensure the cookie file is written and auth credentials available when testing CLI/RPC commands before the RPC connection is up.
2020-04-19Merge #18601: wallet: Refactor WalletRescanReserver to use wallet referenceMarcoFalke
fc289b7898fb90d4800675b69c0bb9b42df5599f wallet: Refactor WalletRescanReserver to use wallet reference (João Barbosa) Pull request description: Simple refactor to `WalletRescanReserver` to use wallet reference instead of pointer. Complements #18259. ACKs for top commit: MarcoFalke: ACK fc289b7898fb90d4800675b69c0bb9b42df5599f Tree-SHA512: b03e33f2d9df2870436aa3284137fd022dd89ea96a1b170fa27f8685ad4f986e6c4ba5975a84966c30d18430a4014d7d8740a1dff2f985c9ef8226ed18e69db9
2020-04-19Merge #18610: scripted-diff: test: replace command with msgtype (naming)MarcoFalke
9df32e820d83aa74e2f175d8d63b5666b8b4ef0e scripted-diff: test: replace command with msgtype (Sebastian Falbesoner) Pull request description: This is a follow-up PR to https://github.com/bitcoin/bitcoin/pull/18533, which changed the naming of `strCommand` to `msg_type` in the network processing code. The same approach is done here for the function test framework, to get rid of the wrong "command" terminology for network mesage types. (Commands are usually used in the CLI or RPC context, so using the same name in the network message context would only be confusing.) The commit was created through the following steps: 1. search for all occurences of the string "command" within the folder `test/functional` ```git grep -i command test/functional > command_finds``` 2. manually sort out all false-positives, i.e. occurences of "command" which describe commands in the correct sense (mostly CLI or RPC related, also some with Socks5) 3. put the remaining occurences into a scripted-diff (a quite simple one, actually) that renames "command" to "msgtype" in the concerned files. The name `msgtype` was intentionally chosen without the underscore `_` as classes beginning with `msg_` define concrete types of messages. ACKs for top commit: MarcoFalke: ACK 9df32e820d83aa74e2f175d8d63b5666b8b4ef0e . Makes sense that tests use the same naming as Bitcoin Core. See `NetMsgType` here: https://doxygen.bitcoincore.org/namespace_net_msg_type.html Tree-SHA512: cd0ee08a382910b7f10ce583acdaf4f8a39f9ba4a22434a914415727eedd98bac538de9bf6633574d5eb86f62558bc8dcb638a3289d99b04f8481f34e7a9a0c7
2020-04-19wallet: Refactor WalletRescanReserver to use wallet referenceJoão Barbosa
2020-04-19ci: Remove xenial tsan workaroundMarcoFalke
2020-04-19test: Increase debugging to hunt down mempool_reorg intermittent failureMarcoFalke
2020-04-19Merge #15761: Replace -upgradewallet startup option with upgradewallet RPCMarcoFalke
0d32d661481f099af572e7a08a50e17bcc165c44 Remove -upgradewallet startup option (Andrew Chow) 92263cce5b6c6b66296dadda5f29724611db0160 Add upgradewallet RPC (Andrew Chow) 1e48796c99b63aa8fa8451ce7b0c20759ea43500 Make UpgradeWallet a member function of CWallet (Andrew Chow) c988f27937bc79c90f4eed48552c72f1b66dc044 Have UpgradeWallet take the version to upgrade to and an error message out parameter (Andrew Chow) 183323712398e26ddcf3a9dc048aaa9900a91f5a Only run UpgradeWallet if the wallet needs to be upgraded (Andrew Chow) 9c16b1735f8e530ce68d678e9ca0eceb2ceb3520 Move wallet upgrading to its own function (Andrew Chow) Pull request description: `-upgradewallet` is largely incompatible with many recent wallet features and versions. For example, it was disabled if multiple wallets were used and would not work with encrypted wallets that were being upgraded to HD. This PR does away with the old method of upgrading upon startup and instead allows users to upgrade their wallets via an `upgradewallet` RPC. This does largely the same thing as the old `-upgradewallet` option but because the wallet is loaded, it can be unlocked to upgrade to HD. Furthermore it is compatible with multiwallet as it works on the individual wallet that is specified by the RPC. ACKs for top commit: meshcollider: Code review ACK 0d32d661481f099af572e7a08a50e17bcc165c44 darosior: ACK 0d32d661481f099af572e7a08a50e17bcc165c44 MarcoFalke: ACK 0d32d661481f099af572e7a08a50e17bcc165c44 🚵 Tree-SHA512: b425bf6f5d605e26506889d63c780895482f07cbc086193218e031e8504d3072d41e90d65cd41bcc98ee4c1eb048954bc5d4ac85435f7394892373aac89a3b0a
2020-04-19Merge #18675: tests: Don't initialize PrecomputedTransactionData in ↵MarcoFalke
txvalidationcache tests 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
2020-04-19Merge #18633: test: Properly raise FailedToStartError when rpc shutdown ↵MarcoFalke
before warmup finished (take 2) fa03713e133e3017112fdd5c278e0c8643054578 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (take 2) (MarcoFalke) Pull request description: actually (?) fix #18561 See most recent traceback https://travis-ci.org/github/bitcoin/bitcoin/jobs/674668692#L7062 I believe the reason the error is still there is that ConnectionResetError is derived from OSError: ConnectionResetError(ConnectionError(OSError)) And IOError is an alias for OSError since python 3.3, see https://docs.python.org/3/library/exceptions.html#IOError So fix that by renaming IOError to the alias OSError and move the less specific catch clause down a few lines. ACKs for top commit: jonatack: ACK fa03713e133e3017112fdd5c278e0c8643054578 Tree-SHA512: 6e5b214ed9101bf8ebe7472dcc1f9e9d128e2575c93ec00c8d0774ae1a9b52a8c2a653a45a0eab8d881570b08dd5ffeddf5aca88a10438c366e1f633253cb0b5
2020-04-19Merge #18695: test: Replace boost::mutex with std::mutexfanquake
27abd1a4f4c7a3d092d59edbbaa1e0f324c8b0ef test: Replace boost::mutex with std::mutex (Hennadii Stepanov) Pull request description: This PR replaces `boost::mutex` with `std::mutex` in the `scheduler_tests` test suite. ACKs for top commit: theStack: ACK https://github.com/bitcoin/bitcoin/pull/18695/commits/27abd1a4f4c7a3d092d59edbbaa1e0f324c8b0ef sipa: utACK 27abd1a4f4c7a3d092d59edbbaa1e0f324c8b0ef Tree-SHA512: 062eed360a68910fb71552fd892bfd097442718a237446cfb8350bfd5d807da7251ead2b9755e1d7022598774ed23fa5432a589ac6f8cadddab404b439883466
2020-04-19build: fix ASLR for bitcoin-cli on Windowsfanquake
ASLR is not currently working for the bitcoin-cli.exe binary. This is due to it not having a .reloc section, which is stripped by default by the mingw-w64 ld we use for gitian builds. A good summary of issues with ld and mingw-w64 is available in this thread: https://sourceware.org/bugzilla/show_bug.cgi?id=19011. All other Windows binaries that we distribute (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-tx and test_bitcoin) do not suffer this issue, and currently having working ASLR. This is due to them exporting (inadvertent or not) libsecp256k1 symbols, and, as a result, the .reloc section is not stripped by ld. This change is a temporary workaround, also the same one described here: https://www.kb.cert.org/vuls/id/307144/, that causes main() to be exported. Exporting a symbol will mean that the .reloc section is not stripped, and ASLR will function correctly.
2020-04-18test: Properly raise FailedToStartError when rpc shutdown before warmup ↵MarcoFalke
finished (take 2)
2020-04-18Merge #18692: test: Bump timeout in wallet_import_rescanMarcoFalke
fabfcad8764bb8f807b0ac5f3482b414278a4525 test: Bump timeout in wallet_import_rescan (MarcoFalke) Pull request description: Avoid timeouts when starting the node, also make error message more verbose ACKs for top commit: practicalswift: ACK fabfcad8764bb8f807b0ac5f3482b414278a4525 -- patch looks correct Tree-SHA512: 8fd60a05380349f521d0e814d2f268702dfbe57c7567a4f6e94435498dfdd32909179d75fded44757ecb1a93a4045842bc6d00bfd6cd18ba751513461359c7b0
2020-04-18Merge #17219: wallet: allow transaction without change if keypool is emptySamuel Dobson
92bcd70808b9cac56b184903aa6d37baf9641b37 [wallet] allow transaction without change if keypool is empty (Sjors Provoost) 709f8685ac37510aa145ac259753583c82280038 [wallet] CreateTransaction: simplify change address check (Sjors Provoost) 5efc25f9638866941028454cfa9bae27f1519cb4 [wallet] translate "Keypool ran out" message (Sjors Provoost) Pull request description: Extracted from #16944 First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check. Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error. ACKs for top commit: fjahr: Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 jonasschnelli: utACK 92bcd70808b9cac56b184903aa6d37baf9641b37 achow101: ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 meshcollider: Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006
2020-04-17rpc: doc: Fix and extend getblockstats examplesAdam Soltys
This fixes the example curl command for `getblockstats` which is missing a comma between the params and has single quotes around the second parameter. Besides fixing the existing example, this commit adds an additional example of getting block stats by hash by using a known workaround with bitcoin-cli to get it to treat the hash parameter as a JSON string by wrapping it in both single and double quotes. Co-Authored-By: Andrew Toth <andrewstoth@gmail.com> Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
2020-04-18test: Replace boost::mutex with std::mutexHennadii Stepanov
generated by cgit v1.2.3 (git 2.26.2) at 2024-10-30 16:21:41 +0000