16d4b3fd6d test: mempool.dat compatibility between versions (Ivan Metlushko)
Pull request description:
Rationale: Verify mempool.dat compatibility between versions
The format of mempool.dat has been changed in #18038
The tests verifies the fix made in #18807 and ensures that the file format is compatible between current version and v0.19.1
The test verifies both backward and forward compatibility.
This PR also adds a log when we fail to add a tx loaded from mempool.dat.
It was useful when debugging this test and could be potentially useful to debug other scenarios as well.
Closes#19037
ACKs for top commit:
Sjors:
tACK 16d4b3fd6d
Tree-SHA512: 00a38bf528c6478cb0da467af216488f83c1e3ca4d9166c109202ea8284023e99d87a3d6e252c4d88d08d9b5ed1a730b3e1970d6e5c0aef526fa7ced40de7490
62068381a3 [tests] Make mininode_lock non-reentrant (John Newbery)
c67c1f2c03 [tests] Don't call super twice in P2PTxInvStore.on_inv() (John Newbery)
9d80762fa0 [tests] Don't acquire mininode_lock twice in wait_for_broadcast() (John Newbery)
edae6075aa [tests] Only acquire lock once in p2p_compactblocks.py (John Newbery)
Pull request description:
There's no need for mininode_lock to be reentrant.
Use a simpler non-recursive lock.
ACKs for top commit:
MarcoFalke:
ACK 62068381a3😃
jonatack:
ACK 62068381a3
Tree-SHA512: dcbc19e6c986970051705789be0ff7bec70c69cf76d5b468c2ba4cb732883ad512b1de5c3206c2eca41fa3f1c4806999df4cabbf67fc3c463bb817458e59a19c
3a10d935ac [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408)
ff8c430c65 [test] test disconnect for filterclear (gzhao408)
1c6b787e03 [netprocessing] disconnect node that sends filterclear (gzhao408)
Pull request description:
Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages.
Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](19e919217e/src/net_processing.cpp (L2218)), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204).
Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them.
ACKs for top commit:
jnewbery:
Code review ACK 3a10d935ac
naumenkogs:
utACK 3a10d93
gillichu:
tested ACK: quick test_runner on macOS [`3a10d93`](3a10d935ac)
MarcoFalke:
re-ACK 3a10d935ac only change is replacing false with true 🚝
Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
fa71667597 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 fa71667597, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: b4419d38e2b41f6e4d6e6b7658f1d972c40c390a49fe78808f8640d28efd84cc6668ce292d45b7c539e65b9e2ecbad10e796cb8f9329a0f1e7d0132ce962d226
0f8f515445 RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME (Luke Dashjr)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 357c6e0bd1b144213ca6cf0bfd649c7a482c2d6d5e98a254d20c8365d228dc71ae1b78aca4918fdbf065f8894ef82f8a475902d605204275bb99fe77d4b42fae
-Increasing the banscore and/or banning is too harsh,
just disconnecting is enough.
-Return true from ProcessMessage because we already log
receipt of filterclear and disconnect.
-nodes not serving bloomfilters should disconnect peers
that send filterclear, just like filteradd and filterload
-nodes that want to enable/disable txrelay should use
feefilter
49236be099 [tests] Don't import asyncio to test magic bytes (John Newbery)
Pull request description:
Simplify the test for invalid start bytes. No need to import asyncio and the Network thread.
ACKs for top commit:
MarcoFalke:
review ACK 49236be099
jonatack:
ACK 49236be099
troygiorshev:
ACK 49236be. +0.1 on the additional `cut_len` reformat.
Tree-SHA512: 75cb695603cdc1be7035d7b5117dbef2a1fdb29fd4414a73d75b53d563d6fa800c31bfa9475004622c8bdea4978b51b2055d6fa7be0fe47c7ae34ccc2b0e89a0
ccf1f6ea24 refactor: Drop ::HasWallets() (João Barbosa)
Pull request description:
Minor follow-up of #19250. The global `HasWallets()` is used only once and at the call site there's already a way to know if any wallet is loaded.
ACKs for top commit:
MarcoFalke:
ACK ccf1f6ea24
hebasto:
ACK ccf1f6ea24, I have reviewed the changes and they look OK, I agree they can be merged.
Tree-SHA512: fb902c045cbd331eaf71716c04734520f2ce7f2b317db510c4ce140162bbc683327b5a40ac860f6cde5add37e069065274d39dfa147fac2091eedec505f2f7eb
8a26848c46 build: Fix m4 escaping (Hennadii Stepanov)
9123ec15db build: Remove extra tokens warning (Hennadii Stepanov)
fded4f48c3 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov)
05a93d5d96 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov)
ddbb419310 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov)
492971de35 build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov)
Pull request description:
This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](https://github.com/bitcoin/bitcoin/pull/8314#issue-76644643) by Cory Fields:
> I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG)
There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt:
- for macOS host (similar to, but not the same as #16391)
- for Windows host (regression)
The fix is ~on its way~ submitted in #18298 (as a followup).
Also this PR picks some small improvements from #17820.
ACKs for top commit:
theuni:
Code review ACK 8a26848c46
dongcarl:
Code Review ACK 8a26848c46
laanwj:
Code review ACK 8a26848c46
Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
af2a145e57 Refactor resource exhaustion test (Troy Giorshev)
5c4648d17b Fix "invalid message size" test (Troy Giorshev)
ff1e7b8844 Move size limits to module-global (Troy Giorshev)
57890abf2c Remove two unneeded tests (Troy Giorshev)
Pull request description:
This PR touches only the p2p_invalid_messages.py functional test module. There are two main goals accomplished here. First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons. Second, it refactors the file into a single consistent style. This file appears to have originally had two authors, with different styles and some test duplication.
It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and [AltNet](https://github.com/bitcoin/bitcoin/issues/18989) changes.
This should probably go in ahead of #19107, but the two are not strictly related.
ACKs for top commit:
jnewbery:
ACK af2a145e57
MarcoFalke:
re-ACK af2a145e57🍦
Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
45eff751c6 Add functional test for P2P eviction logic of inbound peers (Martin Zumsande)
Pull request description:
This adds a functional test for the eviction logic for inbound peers, which is triggered when the number of maximum connections is exceeded.
The functional test covers eviction protection for peers that have sent us blocks or txns recently, or that have faster pings. I couldn't find a way to test the logic of `CConnman::AttemptToEvictConnection` that is based on netgroup (see #14210 for related discussion)
Fixes#16660 (at least partially).
[Edit: Earlier, this PR also contained a unit test, which was removed after the discussion]
ACKs for top commit:
jonatack:
ACK 45eff751c6
naumenkogs:
Tested ACK 45eff75
fjahr:
re-ACK 45eff751c6
andrewtoth:
re-ACK 45eff751c6
Tree-SHA512: 177208ab6f30dc62da1cc5f51e654f7c9770d8c6b42aca6ae7ecb30e29d3096e04d75739578e7d149a0f29dd92652b4a707e93c0f1be8aa7ed315e6ec3ab07a4
fadf6bd04f refactor: Remove unused request.fHelp (MarcoFalke)
fad889cbf0 wallet: Make RPC help compile-time static (MarcoFalke)
Pull request description:
Currently calling `help` on a wallet RPC method will either return `help: unknown command: getnewaddress` or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing.
The fix is split into two commits:
* First, a commit that can be reviewed with the `--color-moved=dimmed-zebra` option and tested with the included test.
* Second, a commit that removes the complicated and confusing code.
ACKs for top commit:
achow101:
re-ACK fadf6bd04f
promag:
Tested ACK fadf6bd04f.
Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d
dca73941eb scripted-diff: rename node to peer for mininodes (gzhao408)
0474ea25af [test] fix race conditions and test in p2p_filter (gzhao408)
4ef80f0827 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408)
497a619386 [test] add BIP 37 test for node with fRelay=false (gzhao408)
e8acc60156 [test] add mempool msg test for node with bloomfilter enabled (gzhao408)
Pull request description:
This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_:
1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)).
2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`.
3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled.
4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py.
5. Fixes race conditions in p2p_filter.py
ACKs for top commit:
MarcoFalke:
ACK dca73941eb only changes is restoring accidentally deleted test 🍮
jonatack:
ACK dca73941eb modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to.
Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3
cf5b8f64b3 tests: Add fuzzing harness for {Read,Write}{LE,BE}{16,32,64} (crypto/common.h) (practicalswift)
4a8181b303 tests: Add std::vector<uint8_t> ConsumeFixedLengthByteVector(FuzzedDataProvider& fuzzed_data_provider, const size_t length) (practicalswift)
Pull request description:
Add fuzzing harness for `{Read,Write}{LE,BE}{16,32,64}` (`crypto/common.h`).
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK cf5b8f64b3
Tree-SHA512: 26412daa6987add1c721ad0348a5a894d68a646e724f328f2db6d9c9358a533481d8888b89d4b0743e9d1c11aa4e0e5341eb4c0d05a4da77b15ab75489327749
3a83a01694 [tests] move generate_wif_key to wallet_util.py (John Newbery)
b216b0b71f [tests] sort imports in rpc_createmultisig.py (John Newbery)
e38081846d Revert "[TESTS] Move base58 to own module to break circular dependency" (John Newbery)
Pull request description:
generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module.
This fixes the circular dependency issue in #17977
ACKs for top commit:
MarcoFalke:
ACK 3a83a01694🍪
Tree-SHA512: 24985dffb75202721ccc0c6c5b52f1fa5d1ce7963bccde24389feb913cab4dad0c265274ca67892c46c8b64e6a065a0f23263a89be4fb9134dfefbdbe5c7238a
f42f5e58f5 refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions (Russell Yanofsky)
Pull request description:
This simplifies control flow and also helps get rid of the ::vpwallets variable in #19101 since EnsureWalletIsAvailable doesn't have access to the request context.
ACKs for top commit:
MarcoFalke:
ACK f42f5e58f5 (reviewed code to check that this is a refactor) 💢
promag:
Tested ACK f42f5e58f5.
Tree-SHA512: eb10685de3db3c1d10c3a797d8da5c8c731e4a8c9024bbb7245929ba767a77a52783a739b8cb1fa7af6fcd233dcf9c8ebbe414eb8b902e2542601aac18625997
fa98e10d5e test: Remove leftover comment in mining_basic (MarcoFalke)
faedb50d89 test: pep-8 mining_basic (MarcoFalke)
Pull request description:
Remove an accidental leftover comment from #19082, which no longer applies and thus might be confusing
ACKs for top commit:
adamjonas:
code review ACK fa98e10
Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40
-A node with bloomfilters disabled should disconnect peers that send
msg_mempool, msg_filterload, or msg_filteradd.
-Renamed the test because it now has a wider scope and msg_mempool's
actual functionality makes more sense for p2p_filter.py.
-msg_mempool is currently only tested with bloomfilter disabled
(node is disconnected) in p2p_mempool.py
-msg_mempool should get mempool txns in response when bloomfilter
is enabled
-edit test that doesn't test msg_mempool as intended
c75de5da5f [TESTS] Move base58 to own module to break circular dependency (Pieter Wuille)
Pull request description:
I encountered difficulties with the test framework in #17977. This fixes them, and I think the change is generally useful.
ACKs for top commit:
laanwj:
Code review ACK c75de5da5f
MarcoFalke:
ACK c75de5da5f according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒
Tree-SHA512: 9e0493de3e279074f0c70e92c959b73ae30479ad6f2083a3c6bbf4b0191d65ef94854559a5b7c904f5dadc5e93129ed00f6dc0a8ccce6ba7921cd45f7119f74b
d49612f98a Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov)
d1ae7c0355 Make GetWarnings() return bilingual_str (Hennadii Stepanov)
38e33aa481 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 d49612f98a
MarcoFalke:
ACK d49612f98a🌂
Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9
948f1134bc gui: add missing translation.h include to fix build (fanquake)
Pull request description:
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
ACKs for top commit:
hebasto:
ACK 948f1134bc, tested on Linux Mint 19.3 (x86_64): it fixes compiling error with the `--disable-wallet` configure option.
Tree-SHA512: db0197b110b3a7d05af2ceb29fbe9eeb6521d28f53b6267aa6d07a975886adb5c6485af79506ab6c66ed101e32292feeaff3707cdbc11432e5b97400953d5631
After #19176, building the gui on Bionic is failing with:
```bash
CXX qt/qt_libbitcoinqt_a-guiutil.o
qt/bitcoin.cpp: In function 'int GuiMain(int, char**)':
qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
```
The merge commit also failed to compile with the same error:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
6fe989054f refactor: Change Node::initError to take bilingual_str (Wladimir J. van der Laan)
425e7cb8cf refactor: Put`TryParsePermissionFlags` in anonymous namespace (Wladimir J. van der Laan)
77b79fa6ef refactor: Error message bilingual_str consistency (Wladimir J. van der Laan)
Pull request description:
A straightforward and hopefully uncontroversial refactor to improve consistency.
- Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(SomeFunction(...)))`.
- Make all functions in `util/error.h` consistently return a `bilingual_str`. We've decided to use this as error message type so let's roll with it.
This has no functional changes: no messages are changed, no new translation messages are defined.
Also make a function static that can be static.
ACKs for top commit:
MarcoFalke:
ACK 6fe989054f🔣
hebasto:
ACK 6fe989054f, tested on Linux Mint 19.3 (x86_64).
Tree-SHA512: 1dd123ef285c4b50bbc429b2f11c9a63aaa669a84955a0a9b8134e9dc141bc38f863f798e8982ac68bbe83170e1067a87d1a87fe7f791928b7914e10bbc2ef8d
