2a57108051 Avoid Appveyor compilation failure (Pieter Wuille)
a653aacbd6 Add logging and addr rate limiting statistics (Pieter Wuille)
aaa4833fc9 Functional tests for addr rate limiting (Pieter Wuille)
8df3e5bd84 Randomize the order of addr processing (Pieter Wuille)
83dfe6c65e Rate limit the processing of incoming addr messages (Pieter Wuille)
Pull request description:
Backport of #22387.
The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that's orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.
This PR implements a [token bucket](https://en.wikipedia.org/wiki/Token_bucket) based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers' self-announcement.
Due to the lack of the `Peer` struct in 0.21, the relevant fields have been added to `CNodeState` instead, necessitating additional locks, and slightly different structure to avoid too much `cs_main` grabbing. The last test-improving commit has also been dropped, as the code has changed too much. Most of the behavior is still tested however, just not the part that compares with RPC statistics.
ACKs for top commit:
achow101:
ACK 2a57108051
GeneFerneau:
Approach + code review ACK [2a57108](2a57108051)
jnewbery:
reACK 2a57108051
Tree-SHA512: ecf4891ac6173d732aa40b4d05fc0dce94127a613cb9051bf6188a2f95824f8234b17d386dd0b352ddf3d352202cc2ff07915ae35657d8e64907e3f80703d1d9
While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.
This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).
The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.
Github-Pull: #22387
Rebased-From: 0d64b8f709
55e941f5df test: Fix intermittent feature_taproot issue (MarcoFalke)
681f728a35 ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov)
89426c43fb ci: Fix macOS brew install command (Hennadii Stepanov)
Pull request description:
This backports a few changes to fix CI failures we are seeing with the 0.21 branch.
Backports #21663, this might be the easiest way to fix the macOS CI failures we're seeing. i.e in #22569. The underlying issue is that the older CI images are using a version of brew that without running `brew update` first, is trying to download packages like Boost, from bintray (which no-longer works).
This also includes #20182, as by fixing macOS failure, via running `brew upgrade`, we end up using a newer version of miniupnpc, which emits a GNU extension related warning, and causes the build to fail, because we use `-Werror`.
Backporting #20535 should fix#22581.
ACKs for top commit:
hebasto:
ACK 55e941f5df, I verified changes by backporting locally.
Tree-SHA512: 3ab2c5c73c707d0f5b862264f3a0179cdeee30ae55aae872f3c3e0bb81d71a5027c39ba830210c99a21f98cc86c4167c4f215e24d1a8891ec79ce512debf82df
e3f1da4bf3 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
6ca54ce2ae qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)
f220368220 qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)
Pull request description:
Backports https://github.com/bitcoin-core/gui/pull/277, https://github.com/bitcoin-core/gui/pull/365.
ACKs for top commit:
fanquake:
ACK e3f1da4bf3
jarolrod:
ACK e3f1da4bf3
Tree-SHA512: 43cc2ac48f4e5014bfdbe86cc904bb36d2be9fcd257f0fc0800c384bd727bb98466723e450a8909b06708784ad91184be599c49cf60de2e4377202774cb878f6
da816247f0 util: Properly handle -noincludeconf on command line (MarcoFalke)
513613d8a8 Cleanup -includeconf error message (MarcoFalke)
70eac6fcd0 Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke)
c5357fa415 fuzz: add missing ECCVerifyHandle to base_encode_decode (Andrew Poelstra)
Pull request description:
Backports #22279, #22002 and #22137 to fix fuzzing issues in the 0.21 branch: https://github.com/bitcoin/bitcoin/runs/2864012729.
ACKs for top commit:
achow101:
ACK da816247f0
Tree-SHA512: ab8751387e42e03ff43594ae34be8ed0dba903d7da1aaecb9f19c08366570d8995abe89ba0c9bafe37662940f3e83bef1e9e50f330e86114cd6a773becd1fd21
7b0b201d10 wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov)
Pull request description:
This change prevents infinite looping for, for example, system folders
on Windows.
Github-Pull: #21907
Rebased-From: 29c9e2c2d2
Note: Trivial backport, but in a differently-named function in another file
ACKs for top commit:
hebasto:
ACK 7b0b201d10, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d41ab267250e8bfd9289cacf1fd804cc1a3bb20fc479dc9da5a69ebf26530b552b11b2ee6b11e17a1c146ca792ee65bd64eeb2269fa5e73a70361da8a2a09925
Remove the erroneous trailing newline '\n'. Also, print only the first
value to remove needless redundancy in the error message.
Github-Pull: #22002
Rebased-From: fad0867d6a
0fe60a84ae Use latest signapple commit (Andrew Chow)
5313d6aed2 gitian: Remove codesign_allocate and pagestuff from MacOS build (Andrew Chow)
27d691b6b5 gitian: use signapple to create the MacOS code signature (Andrew Chow)
2f33e339a8 gitian: use signapple to apply the MacOS code signature (Andrew Chow)
65ce833042 gitian: install signapple in gitian-osx-signer.yml (Andrew Chow)
Pull request description:
Backport of #20880 and #22190
ACKs for top commit:
MarcoFalke:
cherry-pick-only ACK 0fe60a84ae🍀
Tree-SHA512: e864048fab02a1857161602dd53abba552ca3f859c133a47a5e62c28d3e4de9cd099bce86123a1b5892042b09f51cc1ddd2ed1b0c71bfba162710eaee3f5bf91
PF_NOBAN is a multi-flag that includes PF_DOWNLOAD, so the conditional
in CConnman::Bind() using a bitwise AND will return the same result
for both the "noban" status and the "download" status.
Example:
`PF_DOWNLOAD` is `0b1000000`
`PF_NOBAN` is `0b1010000`
This makes a check like `flags & PF_NOBAN` return `true` even if `flags`
is equal to `PF_DOWNLOAD`.
If `-whitebind=download@1.1.1.1:8765` is specified, then `1.1.1.1:8765`
should be added to the list of local addresses. We only want to avoid
adding to local addresses (that are advertised) a whitebind that has a
`noban@` flag.
As a result of a mis-check in `CConnman::Bind()` we would not have added
`1.1.1.1:8765` to the local addresses in the example above.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Github-Pull: bitcoin/bitcoin#21644
Rebased-From: dde69f20a0
deff4e763e depends: update Qt 5.9 source url (Kittywhiskers Van Gogh)
Pull request description:
## Expected Behavior
Running `make` in the depends directory should result in the successful building of host platform dependencies
## Actual Behavior
`make` terminates as `qt.mk` points to (now) outdated URL path for Qt 5.9 sources, returning error 404
## Remedy
Update `qt.mk` to point to updated source location as Qt has relocated them
ACKs for top commit:
RandyMcMillan:
tACK deff4e7
hebasto:
ACK deff4e763e, I have verified the new link, and checked that old one is unavailable:
jarolrod:
tACK deff4e763e
Tree-SHA512: d5b11a678c871a87e6884a35641178ee349728ead51da06018953be36aab5a30aef9c80b549da49802e87903eb0b7836cc343301325bd8c505cb4246f6752505
856de5bd5e build,boost: update download url. (fdov)
Pull request description:
Backports #21662 to the 0.21 branch. Boost has migrated it's download URLs due to bintrays imminent closure. The Boost site has also been updated to point to the new URLs. i.e: https://www.boost.org/users/history/version_1_70_0.html.
Github-Pull: #21662
Rebased-From: 36c10b9f4b
ACKs for top commit:
jarolrod:
ACK 856de5bd5e
Tree-SHA512: cae7262f963dd6e00af699c3e1bce2ee3ff9f76c5dcc24c8a5bf37025b53de28e4cb80de65a380208ee1841a89736d5c35605e40b728b6c7a9f026d8165d74c8
Co-authored-by: David A. Harding <dave@dtrt.org>
Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Tree-SHA512: dd9ac416ff22276833111198445d76cf8417012a6faad0c3560276f1dcf24586ff41c65ac3430fbf1e840aaa563d3dd101671cff306b0fd92aa2ee03bb7f926b
b8af67eeef fuzz: cleanups for versionbits fuzzer (Anthony Towns)
79cdb4a198 test: make sure non-IP peers get discouraged and disconnected (Vasil Dimov)
b765f41164 test: also check disconnect in denialofservice_tests/peer_discouragement (Vasil Dimov)
dfeb6c10bb test: use pointers in denialofservice_tests/peer_discouragement (Vasil Dimov)
Pull request description:
Backport tests
ACKs for top commit:
vasild:
ACK b8af67eeef
jnewbery:
ACK b8af67eeef
ajtowns:
ACK b8af67eeef ; visually compared individual commits to originals, checked original commits are in master
Tree-SHA512: 22f665560f9d452993b12508d93d93ff54e3e91dcf39f731e27aedfb891570168066c185413d455bee4fa082c011b65ea1b0eee51e3633392b07a0db008d51c8
Use `CConnmanTest` instead of `CConnman` and add the nodes to it
so that their `fDisconnect` flag is set during disconnection.
Github-Pull: #21571
Rebased-From: 637bb6da36
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.
The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.
If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.
Github-Pull: #21571
Rebased-From: 4d6e246fa4
