79c02c88b3 Randomize message processing peer order (Pieter Wuille)
Pull request description:
Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the `vNodes` vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a "first-served-by-loop-first" basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays.
As there isn't anything particularly optimal about the current ordering, just make it unpredictable by randomizing.
Reported by Crypt-iQ.
ACKs for top commit:
jnewbery:
ACK 79c02c88b3
Crypt-iQ:
ACK 79c02c88b3
sdaftuar:
utACK 79c02c88b3
achow101:
Code Review ACK 79c02c88b3
jamesob:
crACK 79c02c88b3
jonatack:
ACK 79c02c88b3
vasild:
ACK 79c02c88b3
theStack:
ACK 79c02c88b3
Tree-SHA512: 9a87c4dcad47c2d61b76c4f37f59674876b78f33f45943089bf159902a23e12de7a5feae1a73b17cbc3f2e37c980ecf0f7fd86af9e6fa3a68099537a3c82c106
aab7fd0f8d Switch Appveyor CI to VS2019 stable image (Aaron Clauson)
Pull request description:
The current appveyor config is using the VS2019 preview image so the latest prebuilt Qt5.12.11 binaries can be used, see #22224.
Appveyor updated the Visual Studio 2019 image to msbuild v16.10.1 on 14th of June. This is the version used to build the latest Qt binaries and removes the need to use the Appveyor VS2019 preview image.
ACKs for top commit:
MarcoFalke:
review ACK aab7fd0f8d if green
practicalswift:
cr ACK aab7fd0f8ddb34437a63d636170f5051aae285b4: patch looks correct
hebasto:
ACK aab7fd0f8d
Tree-SHA512: 42ea4e6e27a2099ddeed99a3352e8ff014df1e93fbcb1f0f2ebd6f22ec2fb71212275a2adb2a7858516203e566c6ba053367b4ac7e0b74457f7a35e941d6fdd4
e4c916a0ea Bugfix: GUI: Use a different shortcut for "1 d&ay" banning, due to conflict with "&Disconnect" (Luke Dashjr)
94e7cdd7e0 GUI: Add keyboard shortcuts for other context menus (Luke Dashjr)
02b5263cd4 GUI: Restore keyboard shortcuts for context menu entries (Luke Dashjr)
Pull request description:
Various keyboard shortcuts were lost in #263; this restores them, and also adds new ones for other context menus.
Note that with a context menu open, simply the shortcut by itself (no Alt) is used.
ACKs for top commit:
jarolrod:
Code Review ACK e4c916a
hebasto:
ACK e4c916a0ea, tested on Linux Mint 20.1 (Qt 5.12.8).
Tree-SHA512: 949461acf7aac592bc48a1c5abad41b167365830e0cedb3aa11b6a87bd347e16126830ea87936f9c9efc4b7df5b09d3833fae784964d6d119ed45703cfba2ffd
The current appveyor config is using the VS2019 preview image so the latest prebuilt Qt5.12.11 binaries can be used, see #22224.
Appveyor updated the Visual Studio 2019 image to msbuild v16.10.1 on 14th of June. This is the version used to build the latest Qt binaries and removes the need to use the Appveyor VS2019 preview image.
ae98aec9c0 refactor: Make CAddrMan::cs non-recursive (Hennadii Stepanov)
f5d1c7fac7 Add AssertLockHeld to CAddrMan private functions (Hennadii Stepanov)
5ef1d0b698 Add thread safety annotations to CAddrMan public functions (Hennadii Stepanov)
b138973a8b refactor: Avoid recursive locking in CAddrMan::Clear (Hennadii Stepanov)
f79a664314 refactor: Apply consistent pattern for CAddrMan::Check usage (Hennadii Stepanov)
187b7d2bb3 refactor: Avoid recursive locking in CAddrMan::Check (Hennadii Stepanov)
f77d9c79aa refactor: Fix CAddrMan::Check style (Hennadii Stepanov)
06703973c7 Make CAddrMan::Check private (Hennadii Stepanov)
efc6fac951 refactor: Avoid recursive locking in CAddrMan::size (Hennadii Stepanov)
2da95545ea test: Drop excessive locking in CAddrManTest::SimConnFail (Hennadii Stepanov)
Pull request description:
This PR replaces `RecursiveMutex CAddrMan::cs` with `Mutex CAddrMan::cs`.
All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident.
Related to #19303.
Based on #22025, and first three commits belong to it.
ACKs for top commit:
vasild:
ACK ae98aec9c0
Tree-SHA512: c3a2d3d955a75befd7e497a802b8c10730e393be9111ca263ad0464d32fae6c7edf9bd173ffb6bc9bb61c4b39073a74eba12979d47f26b0b7b4a861d100942df
The unit test is single threaded, so there's no need to hold the mutex
between Good() and Attempt().
This change avoids recursive locking in the CAddrMan::Attempt function.
Co-authored-by: John Newbery <john@johnnewbery.com>
1b1088d52f test: add combined I2P/onion/localhost eviction protection tests (Jon Atack)
7c2284eda2 test: add tests for inbound eviction protection of I2P peers (Jon Atack)
ce02dd1ef1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack)
70bbc62711 test: add combined onion/localhost eviction protection coverage (Jon Atack)
045cb40192 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack)
310fab4928 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack)
9e889e8a5c p2p: remove unused CompareOnionTimeConnected() (Jon Atack)
787d46bb2a p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack)
1e15acf478 p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack)
3f8105c4d2 test: remove combined onion/localhost eviction protection tests (Jon Atack)
38a81a8e20 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack)
4ee7aec47e p2p: add m_network to NodeEvictionCandidate struct (Jon Atack)
7321e6f2fe p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack)
ec590f1d91 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack)
4a19f501ab test: add ALL_NETWORKS to test utilities (Jon Atack)
519e76bb64 test: speed up and simplify peer_eviction_test (Jon Atack)
1cde800523 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack)
Pull request description:
Continuing the work in #20197 and #20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic.
It then adds eviction protection for peers connected over I2P. As described in https://github.com/bitcoin/bitcoin/pull/20685#issuecomment-767486499, we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
The goal of this logic is to favorise the diversity of our peer connections.
The individual commit messages describe each change in more detail.
Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates.
ACKs for top commit:
laanwj:
Code review re-ACK 1b1088d52f
vasild:
ACK 1b1088d52f
Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90
This commit extends our inbound eviction protection to I2P peers to
favorise the diversity of peer connections, as peers connected
through the I2P network are otherwise disadvantaged by our eviction
criteria for their higher latency (higher min ping times) relative
to IPv4 and IPv6 peers, as well as relative to Tor onion peers.
The `networks` array is order-dependent in the case of a tie in
candidate counts between networks (earlier array members receive
priority in the case of a tie).
Therefore, we place I2P candidates before localhost and onion ones
in terms of opportunity to recover unused remaining protected slots
from the previous iteration, guesstimating that most nodes allowing
both onion and I2P inbounds will have more onion peers, followed by
localhost, then I2P, as I2P support is only being added in the
upcoming v22.0 release.
with a more abstract framework to allow easily extending inbound
eviction protection to peers connected through new higher-latency
networks that are disadvantaged by our inbound eviction criteria,
such as I2P and perhaps other BIP155 networks in the future like
CJDNS. This is a change in behavior.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
Localhost peers are treated as a network like Tor or I2P by aliasing
them to an unused Network enumerator: Network::NET_MAX.
The goal is to favorise diversity of our inbound connections.
Credit to Vasil Dimov for improving the algorithm from single-pass
to multi-pass to better allocate unused protection slots.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
9edd713c18 build: Fix MSVC linker /SubSystem option for bitcoin-qt.exe (Hennadii Stepanov)
Pull request description:
On master (6f3fbc062f), running `bitcoin-qt.exe`, which was built with MSVC, causes a terminal window open along with the GUI.
This PR fixes such behavior. See Microsoft [docs](https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem?view=msvc-160).
It is still possible to use the `-printtoconsole` option for debug builds.
ACKs for top commit:
sipsorcery:
tACK 9edd713c18.
Tree-SHA512: 02f2874b13e484f98344f6a7e3b01fa82a78a39865787c77bd674ead22a84a7f98a1849ccad26bd2b8c8603b3e29dcc1633b0ad731ce7d61be2d6b1f9584839c
e25ea54dbf Update msvc and appveyor builds to use Qt5.12.11 binaries. (Aaron Clauson)
Pull request description:
Synchronises the Qt version used in the msvc and Appveyor builds with #22054.
I needed to use switch to the `Visual Studio 2019 Preview` Appveyor image because the compiler version on the non-preview image is too far behind and I had difficulty building a compatible Qt version for it. Once the main Appveyor `Visual Studio 2019` image reaches version `16.10.1` it can be used.
ACKs for top commit:
hebasto:
ACK e25ea54dbf
Tree-SHA512: c5e8dcafa342df7cd8ff7c349a8186bee4cdf7fd748c5d94039e30698775058bae8099dd75a50a5079f3cbb5251e695be187bae615159e3cd45054a972c4e6bd
as we are about the change the behavior sufficiently that when we
have multiple disadvantaged networks and a small number of peers
under test, the number of protected peers per network can be different.
This speeds up the test significantly, which helps when
running it repeatedly.
Suggest reviewing the diff with:
colorMoved = dimmed-zebra
colorMovedWs = allow-indentation-change
faf7623106 fuzz: Call const member functions in addrman fuzz test only once (MarcoFalke)
Pull request description:
Logically based on #21940
Currently the fuzz test may spend a long time generating random numbers:
Fix that by calling const member functions only once.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34224
ACKs for top commit:
practicalswift:
cr ACK faf7623106af2e106a3d9777d714d6afeb2bbd83: touches only `src/test/fuzz/addrman.cpp`
Tree-SHA512: 0fe9e0111eb1706fc39bd2f90d4b87a771882bada54c01e96d8e79c2afca2f1081139d5ab680285a81835cc5142e74ada422a181db34b01904975d1e167e64c2
01eedf3821 test: doc: improve doc for chain_transaction() helper (Sebastian Falbesoner)
6e63e366d6 test: refactor: dedup utility function chain_transaction() (Sebastian Falbesoner)
Pull request description:
Both tests `mempool_packages.py` and `mempool_package_onemore.py` define a utility function `chain_transaction` with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties:
* pass a list of parent_txids/vouts instead of single values
* always mark the BIP125-replaceable flag for txs, created via `createrawtransaction` (this is needed by the `mempool_package_onemore.py` test, but doesn't hurt the other one)
This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function `chain_transaction` in `rpc_packages.py`, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx.
ACKs for top commit:
mjdietzx:
reACK 01eedf3821
klementtan:
Code review ACK 01eedf3821
MarcoFalke:
review ACK 01eedf3821🙅
Tree-SHA512: ac7105d02c23f53d76d4ec9dc8de1074dd8faefeecd44b107921b78665279498966152fed312ecbe252a1c34a9643d531166329a4fea0e773df3bb75d43092b0
fbf485c9b2 Allow tr() import only when Taproot is active (Andrew Chow)
Pull request description:
To avoid issues around fund loss, only allow descriptor wallets to import `tr()` descriptors after taproot has activated.
ACKs for top commit:
sipa:
utACK fbf485c9b2
fjahr:
Code review ACK fbf485c9b2
laanwj:
Code review ACK fbf485c9b2
prayank23:
utACK fbf485c9b2
Tree-SHA512: 83c43376515eea523dbc89bc5a0fde53e54aec492e49a40c2a33d80fc94aac459e232ae07b024b4bd75b58078c8d090bc7a2d69541c5d3d4834d2f4cfc9c8208
f507681baa qt: Connect WalletView signal to BitcoinGUI slot directly (Hennadii Stepanov)
bd50ff9290 qt: Drop redundant OverviewPage::handleOutOfSyncWarningClicks slot (Hennadii Stepanov)
793f19599b qt: Drop redundant WalletView::requestedSyncWarningInfo slot (Hennadii Stepanov)
Pull request description:
This PR:
- removes slots whose only job is to emit a signal, since we can use the signal as a slot
- connects the`WalletView::outOfSyncWarningClicked` signal to the `BitcoinGUI::showModalOverlay` slot directly, and removes intermediate `WalletFrame` slot and signal
- split from #29
This PR does not change behavior.
ACKs for top commit:
Talkless:
tACK f507681baa, tested on Debian Sid with Qt 5.15.2, no any behavioral changes noticed.
promag:
Code review ACK f507681baa.
Tree-SHA512: cd636a7e61881b2cbee84d5425d2107a8e39683b8eb32d79dc9ea942db55d5c1979be2f70da1660eaee5de622d10ed5a92f11fc2351de21b84324b10b23d0c96
2ad034a890 doc: update release notes with removal of tor v2 support (Jon Atack)
49938eee9c doc: update tor.md with removal of tor v2 support (Jon Atack)
Pull request description:
Follow-up documentation to #22050 that removed support for Tor version 2 hidden services from Bitcoin Core.
ACKs for top commit:
laanwj:
ACK 2ad034a890
Tree-SHA512: 0f13f9d1db7e11f1e3d9967b6d17b8dc3144b3ab3a258c706464c5e6ac5cbcf2ce2db4ea54be9939f05a82ebd1e7f325f50b435f9822c08b4f21ed4ac58de0af
fa334b4054 refactor: Pass block reference instead of pointer to PeerManagerImpl::BlockRequested (MarcoFalke)
Pull request description:
This allows to remove an assert and at the same time make it more obvious that the block is never nullptr.
Also, add missing `{}` while touching the function.
ACKs for top commit:
jnewbery:
Code review ACK fa334b4054
mjdietzx:
crACK fa334b4054
theStack:
Code review ACK fa334b4054
Tree-SHA512: 9733d3e20e048fcb2ac7510eae3539ce8aaa7397bd944a265123f1ffd90e15637cdaad19dba16f76d83f3f0d1888f1b7014c191bb430e410a106c49ca61a725c
James O'Beirne
