fa1904e5f0 net: Remove dead logging code (MarcoFalke)
fac12ebf4f net: Avoid redundant and confusing FAILED log (MarcoFalke)
Pull request description:
Remove a redundant and confusing "FAILED" log message and gets rid of the unused return type in `ProcessMessage`
ACKs for top commit:
jnewbery:
utACK fa1904e5f0
gzhao408:
utACK fa1904e5f0
troygiorshev:
ACK fa1904e5f0
naumenkogs:
utACK fa1904e
Tree-SHA512: bfa553d5efa022727ed17877fb7c08c14849d804fe6d6a7ce172d513857beba35de41ea40b27ff1aedf68b81e2cda7b2a948ac985fcaaf1b6cfb96cce4837c90
f52d403b81 [net] split PushInventory() (John Newbery)
Pull request description:
PushInventory() is currently called with a CInv object, which can be a
MSG_TX or MSG_BLOCK. PushInventory() only uses the type to determine
whether to add the hash to setInventoryTxToSend or
vInventoryBlockToSend.
Since the caller always knows what type of inventory they're pushing,
the CInv is wastefully constructed and thrown away, and tx/block relay
is being split out, we split the function into PushTxInventory() and
PushBlockInventory().
ACKs for top commit:
amitiuttarwar:
utACK f52d403b81. nice cleanup, this has bothered me :)
naumenkogs:
utACK f52d403
sipa:
utACK f52d403b81
Tree-SHA512: 331495199a3b1a2620e6a62beb336e494291b725d8fd64bb44726c02e80807f3974ff4f329bb0f059088e65cd7d41eff276c1065806d2dd6e72c5a9f368e82cd
83fd3a6d73 init: use std::thread for ThreadImport() (fanquake)
Pull request description:
[Mentioned](https://github.com/bitcoin/bitcoin/pull/19142#issuecomment-638090759) in #19142, which removed the `boost::interruption_point()`
in `ThreadImport()`.
ACKs for top commit:
hebasto:
ACK 83fd3a6d73, I have reviewed the code and it looks OK, I agree it can be merged.
donaloconnor:
ACK 83fd3a6
laanwj:
Code review ACK 83fd3a6d73
MarcoFalke:
ACK 83fd3a6d73
Tree-SHA512: 0644947d669feb61eed3a944012dad1bd3dd75cf994aa2630013043c213a335b162b63e20aa37e0997740d8e3a3ec367b660b5196007a09e13f0ac455b36c821
da7a83c5ee Remove WalletDatabase::Create, CreateMock, and CreateDummy (Andrew Chow)
d6045d0ac6 scripted-diff: Replace WalletDatabase::Create* with CreateWalletDatabase (Andrew Chow)
45c08f8a7b Add Create*WalletDatabase functions (Andrew Chow)
Pull request description:
Instead of having `Create`, `CreateMock`, and `CreateDummy` being static functions in `BerkeleyDatabase`, move these to standalone functions in `walletdb.cpp`. This prepares us for having different `WalletDatabase` classes.
Part of #18971. This was originally one commit but has been split into 3 to make it (hopefully) easier to review.
ACKs for top commit:
MarcoFalke:
ACK da7a83c5ee🎂
ryanofsky:
Code review ACK da7a83c5ee. Easy review, nice scripted-diff
Tree-SHA512: 1feb7cb3889168c555154bf3701a49095fd6b8cab911d44b7f7efbf6fcee2280ccb3d4afec8a83755b39a592ecd13b90a318faa655c321f87bdabdf1e2312327
PushInventory() is currently called with a CInv object, which can be a
MSG_TX or MSG_BLOCK. PushInventory() only uses the type to determine
whether to add the hash to setInventoryTxToSend or
vInventoryBlockToSend.
Since the caller always knows what type of inventory they're pushing,
the CInv is wastefully constructed and thrown away, and tx/block relay
is being split out, we split the function into PushTxInventory() and
PushBlockInventory().
66666d55b1 doc: Mention repo split in the READMEs (MarcoFalke)
faceed753a doc: Add redirect for GUI issues and pull requests (MarcoFalke)
Pull request description:
## 🥅 Goals
Splitting up the GUI (and splitting out modules in general) has been brought up often in recent years. Now that the GUI is primarily connected through (internal) interfaces with the node, it seems an appropriate time to revive this discussion.
Before looking for solutions, we should define a set of goals that we want to achieve. I will start with some ideas to get started and I hope that others will chime in to share and prioritize their goals.
### Separate issue and patch management
It is currently not possible to subscribe to only a subset of modules in Bitcoin Core, or exclude modules from issue and patch notifications. While it is possible to reactively mute conversations in the stream of all ongoing discussions, there is no way to proactively achieve this. Moreover, the list of open issues and pull request will always include GUI related ones by default. Only with [filters](https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+-label%3AGUI) it is possible to hide them.
### More focused review and interests
Long term goals of the GUI are partially unclear #17395 . Bitcoin Core developers are generally fluent on the command line. Thus, they might not be interested or motivated to review improvements to the GUI, which might not affect their workflow on the command line at all. Splitting up the GUI will hopefully attract similar minded people to a project whose primary goal is to build and improve the GUI.
### Maintain high quality assurance
The quality of the GUI (and even more importantly Bitcoin Core in general) must not degrade. This means that code review itself is not negatively affected by splitting the GUI, but also the integration of the GUI into the rest of Bitcoin Core. One issue could arise when arbitrary version-combinations are allowed. We are struggling hard to test against all supported versions of Boost. Making the GUI version another dimension is going to make testing impossible.
### The GUI *is* Bitcoin Core
When a user downloads Bitcoin Core from our website (or another package manager) they expect the GUI to be included. This should not change (at least not as a result of splitting up the GUI into another project).
Similarly, when building Bitcoin Core, the gui should still be built when `--with-gui` is specified.
## 🌳 Proposed solution: Monotree
TLDR. Everything stays the same, the development process for the GUI changes slightly.
Long version:
* An exact mirror of the master development branch is hosted at `bitcoin-core/gui`. The new repository is used to track gui-only issues and pull requests. Global changes that happen to touch gui code still go to the *main* repo.
* All pull requests will be merged into `bitcoin/bitcoin`.
* Decision making process and maintainers will be identical for both repos.
### Disadvantages
* Review activity might decrease?
* It doesn't go far enough. bitcoin/bitcoin#3440 is proposing a modularized Bitcoin Core. The GUI could be an "add-on", connected over RPC or capnproto (bitcoin/bitcoin#10102). Thus, the gui could even be hosted as a subtree or completely separate project.
### Advantages
* Review activity might increase? It is impossible to predict the future, but for example the `libsecp256k1` subtree has a lot of domain specific experts, maintainers and reviewers. I think longer term it makes sense to at least try this route for the gui as well.
* A smaller step is easier to undo when it turns out to come with any unforeseen downsides.
* No substantial changes to the decision making progress.
* Nothing changes in how developers set up their dev environment or how users build from the source. Also, the release binaries and process will stay exactly the same. No version drift. Finally, code sharing between the GUI and Bitcoin Core is not made any harder.
* The organizational side. There are 72 open issues (~14%) and 61 open PRs (~16%) with the GUI label. If moved to its own repo, non-GUI developers wouldn't have to be distracted with GUI-only issues and PRs and GUI enhancements. GUI developers have their own repo to focus on GUI development exclusively.
### Implementation (outstanding TODOs)
* Adjust maintainer merge script https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/57
* Create bitcoin-core/gui repository (empty or with master branch only)
* Assign all existing bitcoin core maintainers to the new repo
* Celebrate? 🥳
* Long-term: Think how long the grace period is for existing GUI related issues and pull requests. Issues can be transferred with a script after a grace period of some months?
ACKs for top commit:
fjahr:
ACK 66666d55b1
Sjors:
ACK 66666d55b1
troygiorshev:
re-ACK 66666d5
practicalswift:
re-ACK 66666d55b1
hebasto:
re-ACK 66666d55b1
Tree-SHA512: 2e1a8de945fa6995583059a2e322621763fccce74a869f9aa750f73546b26350487c4acc4222c03cb3ac1f88e80f0b9d9a3a80a200432fee0d785f52c5cb6174
a389ed52e8 walletdb: refactor Read, Write, Erase, and Exists into non-template func (Andrew Chow)
Pull request description:
In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated.
The functions `ReadKey`, `WriteKey`, `EraseKey`, and `HasKey` are introduced to handle the actual interaction with the database.
This is mostly a moveonly.
Based on #19290
ACKs for top commit:
ryanofsky:
Code review ACK a389ed52e8. No changes since last review, just non-conflicting rebase
Sjors:
utACK a389ed52e8
MarcoFalke:
ACK a389ed52e8🔳
Tree-SHA512: 73bd2fe9ddc4a132d4db6b97e77f5d5f8aa68b8cb25192384f3bacd826365947763a9eee73672331d34578e3f5ade85ee6aa550ff4d89eb62e482250dd5973e4
26acc8dd9b Add sanity check asserts to span when -DDEBUG (Pieter Wuille)
2676aeadfa Simplify usage of Span in several places (Pieter Wuille)
ab303a16d1 Add Span constructors for arrays and vectors (Pieter Wuille)
bb3d38fc06 Make pointer-based Span construction safer (Pieter Wuille)
1f790a1147 Make Span size type unsigned (Pieter Wuille)
Pull request description:
This improves our Span class by making it closer to the C++20 `std::span` one:
* ~~Support conversion between compatible Spans (e.g. `Span<char>` to `Span<const char>`).~~ (done in #18591)
* Make the size type `std::size_t` rather than `std::ptrdiff_t` (the C++20 one underwent the same change).
* Support construction of Spans directly from arrays, `std::string`s, `std::array`s, `std::vector`s, `prevector`s, ... (for all but arrays, this only works for const containers to prevent surprises).
And then make use of those improvements in various call sites.
I realize the template magic used looks scary, but it's only needed to make overload resultion make the right choices. Note that the operations done on values are all extremely simple: no casts, explicit conversions, or warning-silencing constructions. That should hopefully make it simpler to review.
ACKs for top commit:
laanwj:
Code review ACK 26acc8dd9b
promag:
Code review ACK 26acc8dd9b.
Tree-SHA512: 5a5bd346a140edf782b5b3b3f04d9160c7b9e9def35159814a07780ab1dd352545b88d3cc491e0f80d161f829c49ebfb952fddc9180f1a56f1257aa51f38788a
80d4423f99 Test buffered valid message (Troy Giorshev)
Pull request description:
This PR is a tweak of #19302. This sends a valid message.
Additionally, this test includes logging in the same vein as #19272.
ACKs for top commit:
MarcoFalke:
tested ACK 80d4423f99 (added an assert(false) to observe deterministic coverage) 🌦
gzhao408:
ACK 80d4423f99👊
Tree-SHA512: 3b1aa5ec480a1661917354788923d64595e2886448c9697ec0606a81293e8b4a4642b2b3cc9afb2206ce6f74e5c6d687308c5ad19cb73c5b354d3071ad8496f8
fa84edb93c build: don't warn when doxygen isn't found (fanquake)
Pull request description:
Doxygen isn't so important that we need to warn when it is missing. I'd
assume it might even be missing more often than not for most builds.
ACKs for top commit:
MarcoFalke:
Fine with me ACK fa84edb93c
hebasto:
ACK fa84edb93c, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 793ebf01a8a5d48b78a70fdef0022633fca59b30074c960ebb21589e3bd98992b8304621a2d999195d12172ed30fe9eefeeb2a952d58853cf58e8d9902b0090c
A message can be broken across two buffers, with the split inside its
header. Usually this will occur when sending many messages, such that
the first buffer fills.
This test uses the RPC to verify that the message is actually being
received in two pieces.
There is a very rare chance of a race condition where the test framework
sends a message in between the two halves of the message under test. In
this case the peer will almost certainly disconnect and the test will
fail. An assert has been added to help debugging that rare case.
fa195d4eba test: Add missing sync_blocks (MarcoFalke)
Pull request description:
Bitcoin Core does not sort block and tx announcements for other peers, so generating 100 blocks and then sending out a transaction might reject it if it arrives too early. (non-final)
Fix that by syncing the blocks first.
Fix#19265Fix#19311
ACKs for top commit:
Sjors:
utACK fa195d4eba6397262e3e76c8525eb48dcb5e7f2d: sounds plausible
Tree-SHA512: fdc46aed59595e4189509e71bd4a3607a93893933cc01d806cec2ee7701d54d7422c5f22dd83b81ddb021f9113b3119a688fdd8cf8a6474fc12fea422aedd064
In order to override these later, the specific details of how the Read,
Write, Erase, and Exists functions interact with the actual database
file need to go into functions that are not templated.
fa02b47313 refactor: Use AbortError in FatalError (MarcoFalke)
Pull request description:
`FatalError` has been copied from `AbortNode`, so the two should use the same style to avoid confusion.
Follow-up to #18927
ACKs for top commit:
hebasto:
ACK fa02b47313, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2cf6d18a6ffb5c2e5cf54f0a072a7cef6dc7e924152b2fee44e6ff2c6c53bad962afd364eda30d8a73883d656429ea68391090e6a27057e69eaefd7c4dad0a33
9a40cfc558 [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb414e [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d2e1 [test] logging and style followups for bloomfilter tests (gzhao408)
Pull request description:
Followup to #19083 which adds bloomfilter-related tests.
1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests.
2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784)
ACKs for top commit:
jonatack:
Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
MarcoFalke:
ACK 9a40cfc558🐂
Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87
f8213c05f0 Add means to handle negative capabilities in thread safety annotations (Hennadii Stepanov)
Pull request description:
This commit is separated from #19238, and it adds support of [Negative Capabilities](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) in the Clang Thread Safety Analysis attributes.
> Negative requirements are an alternative `EXCLUDES` [`LOCKS_EXCLUDED`] that provide a stronger safety guarantee. A negative requirement uses the `REQUIRES` [`EXCLUSIVE_LOCKS_REQUIRED`] attribute, in conjunction with the ! operator, to indicate that a capability should not be held.
Examples of usage:
- #19238 (for a class)
- https://github.com/hebasto/bitcoin/tree/200610-addrman-tsn (for the whole code base)
ACKs for top commit:
MarcoFalke:
Approach ACK f8213c05f0
vasild:
ACK f8213c05
Tree-SHA512: 86d992826b87579661bd228712ae5ee6acca6f70b885ef7e96458974eac184e4874a525c669607ba6b6c861aa4806409a8792d100e6914c858bcab43d31cfb1b
61c16339da walletdb: Move BDB specific things into bdb.{cpp/h} (Andrew Chow)
8f033642a8 walletdb: moveonly: Move BerkeleyBatch Cursor and Txn funcs to cpp (Andrew Chow)
25a655794a walletdb: move IsWalletLoaded to walletdb.cpp (Andrew Chow)
f6fc5f3849 walletdb: Add IsBDBWalletLoaded to look for BDB wallets specifically (Andrew Chow)
c3538f435a walletdb: Make SpliWalletFilePath non-static (Andrew Chow)
Pull request description:
Moves the BDB specific classes from db.{cpp/h} to bdb.{cpp/h}.
To do this, `SplitWalletFilePath` is first made non-static. Then `IsWalletLoaded` functionality is moved to `IsBDBWalletLoaded` which is called by `IsWalletLoaded`. Then the bulk of db.{cpp/h} is moved to a new file bdb.{cpp/h}.
While doing some moveonly stuff, an additional commit moves the `*Cursor` and `Txn*` implementations out of the header file and into the cpp file.
Part of #18971
ACKs for top commit:
laanwj:
Code review ACK 61c16339da
promag:
Code review ACK 61c16339da.
meshcollider:
utACK 61c16339da
Tree-SHA512: cb676cd34c9cd3c838a4fef230d84711efe4cf0d2eefa64ebfd7f787ddc6f7379db0b29454874ddc46ca7ffee0f18f6f3fb96a85513cd10164048948fd03a80c
47b49a05ea contrib: Fix SyntaxWarning in Python base58 implementation (Alex Willmer)
Pull request description:
In Python integers should be compared for equality (`i == j`), not identity (`i is j`). Recent versions of CPython 3.x emit a SyntaxWarning when they encounter this incorrect usage, e.g.
```
$ python3 base58.py
base58.py:110: SyntaxWarning: "is" with a literal. Did you mean "=="?
assert get_bcaddress_version('15VjRaDX9zpbA8LVnbrCAFzrVzN7ixHNsC') is 0
Tests passed
```
ACKs for top commit:
MarcoFalke:
ACK 47b49a05ea
Tree-SHA512: 9f8962025dcdfa062c0515c68a1864f5bbeb86bd0510c0ec0e413a5edb6afbfd5f41b4c0255784e53db8eaf39c68b7cfa7cc8a33a2e5214aae463fda374f8719
fa193c6b1b Add missing includes to fix compile errors (MarcoFalke)
fa09ec83f3 Remove unused variables (MarcoFalke)
Pull request description:
This is required for #19183, but seems like good cleanup that can go in upfront.
ACKs for top commit:
practicalswift:
ACK fa193c6b1b -- patch looks correct
hebasto:
ACK fa193c6b1b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 79b94e7f7ee3a1a8a8fb2ea1ecdf61f130f8b133a37865894da3dbbbf311979e7d1fc013b923fdd7dbf19a221e0232f664defbdb57aa44e0b8c45bfff3c71dcb
fa41b0a6da pep-8 test/functional/test_framework/util.py (MarcoFalke)
faa841bc97 test: refactor: Inline adjust_bitcoin_conf_for_pre_17 (MarcoFalke)
Pull request description:
This removes mental and code complexity as well as attack surface for bikeshedding
ACKs for top commit:
Sjors:
utACK fa41b0a6da
Tree-SHA512: 6e3c872e66d98ffaa7aecdfd64aa7dd8fbb51815a8fdaba170ce0772b4c3360084d0ebab4a5feac768ab5df50d04528d7daafc51ba07c15445c1ef94fa3efd34
fs.cpp:35:17: error: no member named 'strerror' in namespace 'std'
return std::strerror(errno);
~~~~~^
fs.cpp:49:9: error: use of undeclared identifier 'close'
close(fd);
^
2 errors generated.
./interfaces/chain.h:265:55: error: ‘std::function’ has not been declared
virtual void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) = 0;
^~~
44cc75f80e wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm)
Pull request description:
This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.
ACKs for top commit:
Sjors:
utACK 44cc75f80e (rebased)
fjahr:
re-ACK 44cc75f80e
Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
313a081b90 [net] Add seed.bitcoin.wiz.biz to DNS seeds (wiz)
Pull request description:
I've created the `seed.bitcoin.wiz.biz` DNS seed for the benefit of the Bitcoin community, and will operate it in accordance with the [Bitcoin DNS seed operator policy](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md). Since this is my first PR to the Bitcoin Core project, I also ACK the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md).
The data for this DNS seed is generated using redundant instances of TheBlueMatt's [dnsseed-rust implementation](https://github.com/TheBlueMatt/dnsseed-rust), which connects to all discoverable Bitcoin nodes to verify their capabilities and speed, and utilizes the full AS-MAP data from my network's BGP tables to select Bitcoin nodes which are fairly distributed across different networks.
As for my qualifications, I currently operate Bitcoin nodes for the [mempool.space](https://mempool.space/) open-source block explorer project (mempool) and the [Bisq Network](https://bisq.network/) open-source P2P trading community (bisq-network). I have 20 years experience as a network engineer, and all of [my Bitcoin nodes](https://bitnodes.io/nodes/?q=AS54415) are hosted on [my own network](https://ipinfo.io/AS54415) across multiple datacenters. For personal references, the current Bitcoin DNS seed operators Emzy and TheBlueMatt can probably vouch for me.
The DNS responses served from this instance are currently served with a TTL of 60 seconds, and the DNS resolvers do not log queries from users. Any inquiries related to the operation of this DNS seed can be sent to <noc@wiz.biz>.
Here is a rough diagram of the `seed.bitcoin.wiz.biz` DNS seed architecture:
ACKs for top commit:
jonasschnelli:
Tested ACK 313a081b90.
laanwj:
ACK 313a081b90
Tree-SHA512: 9e4ea7a929b7888eba748933c1581328aefcba4de503af96f99630d797d794859b22c99999c25c3fc90f6efaed2598f32784d3acea3e428d84bae3aa37f92a25
9fe71a57a6 test: use subprocess.run() in test-security-check.py (fanquake)
968aaae940 tests: run test-security-check.py in CI (fanquake)
Pull request description:
[Wladimir asked](https://github.com/bitcoin/bitcoin/pull/18415#issuecomment-603843094) about running the `test-security-check.py` script in our CI. This PR adds a target for that: `make test-security` and adds it to a few CI jobs.
ACKs for top commit:
laanwj:
ACK 9fe71a57a6
Tree-SHA512: d00ebbefbd57ab22436f284837c320f73238ec9967495adc4f2f9a4d574b3b1595c19ce41d53ff4060d5cd7174dbc311235d5877c90e8af2f5587735e7236056
-Waiting is important to avoid race conditions,
especially if testing peer info through rpc later.
-Wait for mininodes to be disconnected only, even
though it's more complex, because we may still want
to be connected to test nodes.
-Use peer to refer to mininodes instead of node
because they are not bitcoind nodes.
-Use log.debug for logs that give helpful but
not super necessary information.
-Adhere to style guidelines (newlines, capitalization).
5527be0627 refactor: Add AbortError alias (Hennadii Stepanov)
d924f2a596 Drop MSG_NOPREFIX flag (Hennadii Stepanov)
083daf7fba Pass bilingual_str argument to AbortNode() (Hennadii Stepanov)
d1cca129b4 refactor: Use bilingual_str::empty() (Hennadii Stepanov)
Pull request description:
This PR is a [followup](https://github.com/bitcoin/bitcoin/issues/16218#issuecomment-625919724) of #16224, and it adds `bilingual_str` type argument support to the `AbortNode()` functions.
ACKs for top commit:
MarcoFalke:
ACK 5527be0627👟
Tree-SHA512: bf8b15b14912b1f672e6e588fffa1e6eb6f00b4b23d15d0ced7f18fbdf76919244427feb7217007fe29617049308e13def893a03a87358db819cca9692f59905
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
Every `return false` is preceeded by a detailed debug log message to
explain that a disconnect or misbehavior happened. Logging another
generic "FAILED" message seems redundant.
Also, the size of the message and the message type has already been
logged and is thus redundant as well.
Finally, claiming that message processing FAILED seems odd, because the
message was fully processed to the point where it was concluded that the
peer should be either disconnected or marked as misbehaving.
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
fanquake
