Whenever a transaction is added to the mempool or orphan pool, both
its txid and wtxid are considered AlreadyHave, and thus will eventually
be removed from m_txrequest.
The same is true for hashes added to the reject filter, but note that sometimes
only the wtxid is added (in which case only the wtxid can be removed from
m_txrequest).
Maintaining up to 100000 INVs per peer is excessive, as that is far more
than fits in a typical mempool.
Also disable the "overload" penalty for PF_RELAY peers.
This removes most transaction request logic from net_processing, and
replaces it with calls to a global TxRequestTracker object.
The major changes are:
* Announcements from outbound (and whitelisted) peers are now always
preferred over those from inbound peers. This used to be the case for the
first request (by delaying the first request from inbound peers), and
a bias afters. The 2s delay for requests from inbound peers still exists,
but after that, if viable outbound peers remain for any given transaction,
they will always be tried first.
* No more hard cap of 100 in flight transactions per peer, as there is less
need for it (memory usage is linear in the number of announcements, but
independent from the number in flight, and CPU usage isn't affected by it).
Furthermore, if only one peer announces a transaction, and it has over 100
in flight and requestable already, we still want to request it from them.
The cap is replaced with an additional 2s delay (possibly combined with the
existing 2s delays for inbound connections, and for txid peers when wtxid
peers are available).
Includes functional tests written by Marco Falke and Antoine Riard.
dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5 tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
353a3fdaad055eea42a0baf7326bdd591f541170 net: advertise support for ADDRv2 via new message (Vasil Dimov)
201a4596d92d640d5eb7e76cc8d959228fa09dbb net: CAddress & CAddrMan: (un)serialize as ADDRv2 (Vasil Dimov)
1d3ec2a1fda7446323786a52da1fd109c01aa6fb Support bypassing range check in ReadCompactSize (Pieter Wuille)
Pull request description:
This PR contains the two remaining commits from #19031 to complete the [BIP155](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) implementation:
`net: CAddress & CAddrMan: (un)serialize as ADDRv2`
`net: advertise support for ADDRv2 via new message`
plus one more commit:
`tor: make a TORv3 hidden service instead of TORv2`
ACKs for top commit:
jonatack:
re-ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5 per `git diff 9b56a68 dcf0cb4` only change since last review is an update to the release notes which partially picked up the suggested text. Running a node on this branch and addnode-ing to 6 other Tor v3 nodes, I see "addrv2" and "sendaddrv2" messages in getpeerinfo in both the "bytesrecv_per_msg" and "bytessent_per_msg" JSON objects.
sipa:
ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5
hebasto:
re-ACK dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5, the node works flawlessly in all of the modes: Tor-only, clearnet-only, mixed.
laanwj:
Edit: I have to retract this ACK for now, I'm having some problems with this PR on a FreeBSD node. It drops all outgoing connections with this dcf0cb477699d11afd0ff37c8bfb2b1b4f7f1ee5 merged on master (12a1c3ad1a43634d2a98717e49e3f02c4acea2fe).
ariard:
Code Review ACK dcf0cb4
Tree-SHA512: 28d4d0d817b8664d2f4b18c0e0f31579b2f0f2d23310ed213f1f436a4242afea14dfbf99e07e15889bc5c5c71ad50056797e9307ff8a90e96704f588a6171308
Introduce a new message `sendaddrv2` to signal support for ADDRv2.
Send the new message immediately after sending the `VERACK` message.
Add support for receiving and parsing ADDRv2 messages.
Send ADDRv2 messages (instead of ADDR) to a peer if he has
advertised support for it.
Co-authored-by: Carl Dong <contact@carldong.me>
b048b275d9711f70847afaea5450f17a0f7e673a [validation] Remove absurdfee from accepttomempool (John Newbery)
932564b9cfda8446a957649c2316a52e868ad5d4 scripted-diff: update max-fee-exceeded error message to include RPC (gzhao408)
8f1290c60159a3171c27250bc95687548c5c1b84 [rpc/node] check for high fee before ATMP in clients (gzhao408)
Pull request description:
Picked up from #15810. Add separate fee-checking logic for clients that need to enforce max fee rates, then remove the `absurdFee` logic from ATMP.
ATMP's `nAbsurdFee` argument is used to enforce user-specific behavior (it is not policy since it isn't applied consistently: it is only ever used in RPC and wallet, and set to 0 everywhere else internally). It should be removed from `AcceptToMemoryPool` because (1) validation results/mempool behavior should not be user-specific and (2) enforcing a max fee rate should be the responsibility of the client instead of the mempool.
Note: this PR does not intend to _remove_ protection from high fees, just re-delegate the responsibility to clients.
ACKs for top commit:
jnewbery:
utACK b048b275d9711f70847afaea5450f17a0f7e673a
LarryRuane:
re-ACK b048b275d9711f70847afaea5450f17a0f7e673a
MarcoFalke:
re-ACK b048b275d9 , only change is squashing one commit 🏦
instagibbs:
utACK b048b275d9
Tree-SHA512: 57c17ba16d230a4cae2896dd6a64c924f307757824e35784bf96da7b10aff2d8ea910710edf35e981035623a155f8766209a92a0fdb856549fde78bc3eaae4d2
675e55e01392971aa56bda56cb09498b466d0902 Ignore unknown messages before VERACK (Suhas Daftuar)
Pull request description:
This allows for feature negotiation to take place with messages between VERSION and VERACK in the future, without requiring additional software changes to specifically ignore messages for features that are unimplemented by our software.
ACKs for top commit:
sipa:
utACK 675e55e01392971aa56bda56cb09498b466d0902
practicalswift:
ACK 675e55e01392971aa56bda56cb09498b466d0902: patch looks correct
MarcoFalke:
ACK 675e55e01392971aa56bda56cb09498b466d0902
hebasto:
ACK 675e55e01392971aa56bda56cb09498b466d0902, the offender peer will be eventually disconnected due to the timeout.
Tree-SHA512: 8d2b1d8b9843f2ee26b2c30f7c5ff0bfcfbe3f46b32cd0369c48ece26624151091237e83ce3f18c6da004099026602cfab1642ac916db777f047d170b365c007
d76925478efd35e6fd835370639f2139b28381e4 [doc] Clarify semantic of peer's m_protect w.r.t to outbound eviction logics (Antoine Riard)
ac71fe936da290adf5a3155fe8db5f78b485f1f1 [doc] Clarify scope of eviction protection of outbound block-relay peers (Antoine Riard)
Pull request description:
Block-relay-only peers were introduced by #15759. According to its
author, it was intented to make them only immune to outbound peer
rotation-based eviction and not from all eviction as modified comment
leans to think of.
Clearly indicate that outbound block-relay peers aren't protected
from eviction by the bad/lagging chain logic.
Fix#19863
ACKs for top commit:
naumenkogs:
ACK d76925478efd35e6fd835370639f2139b28381e4
jonatack:
ACK d76925478efd35e6fd835370639f2139b28381e4
Tree-SHA512: 597fbd62838a6e39276024165b11514cad20a2e9d33cf9202d261cbadcb62b2df427c858e0cb57e585840d4c1d4600104aa53916bb868541f2580e4eed9b4b52
deb52711a17236d0fca302701b5af585341ab42a Remove header checks out of net_processing (Troy Giorshev)
52d4ae46ab822d0f54e246a6f2364415cda149bd Give V1TransportDeserializer CChainParams& member (Troy Giorshev)
5bceef6b12fa16d20287693be377dace3dfec3e5 Change CMessageHeader Constructor (Troy Giorshev)
1ca20c1af8f08f07c407c3183c37b467ddf0f413 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev)
890b1d7c2b8312d41d048d2db124586c5dbc8a49 Move checksum check from net_processing to net (Troy Giorshev)
2716647ebf60cea05fc9edce6a18dcce4e7727ad Give V1TransportDeserializer an m_node_id member (Troy Giorshev)
Pull request description:
Inspired by #15206 and #15197, this PR moves all message header verification from the message processing layer and into the network/transport layer.
In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor.
For more context, see https://bitcoincore.reviews/15206.html#l-81.
This PR improves the separation between the p2p layers, helping improvements like [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and #18989.
ACKs for top commit:
ryanofsky:
Code review ACK deb52711a17236d0fca302701b5af585341ab42a just rebase due to conflict on adjacent line
jnewbery:
Code review ACK deb52711a17236d0fca302701b5af585341ab42a.
Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar)
50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
sipa:
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
promag:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 Remove unused LockAssertion struct (Hennadii Stepanov)
ab2a44297fd0796bf5797ae2a477e8e56d9c3c12 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov)
73f71e19965e07534eb47701f2b23c9ed59ef475 refactor: Use explicit function type instead of template (Hennadii Stepanov)
Pull request description:
This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`.
This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs
ACKs for top commit:
MarcoFalke:
ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25
ajtowns:
ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25
vasild:
ACK 0bd1184ad
Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41
This moves header size and netmagic checking out of net_processing and
into net. This check now runs in ReadHeader, so that net can exit early
out of receiving bytes from the peer. IsValid is now slimmed down, so
it no longer needs a MessageStartChars& parameter.
Additionally this removes the rest of the m_valid_* members from
CNetMessage.
This removes the m_valid_checksum member from CNetMessage. Instead,
GetMessage() returns an Optional.
Additionally, GetMessage() has been given an out parameter to be used to
hold error information. For now it is specifically a uint32_t used to
hold the raw size of the corrupt message.
The checksum check is now done in GetMessage.
In addition to adding more specificity to the log statement about the type of
connection, this change also consolidates two statements into one. Previously,
the second one should have never been hit, since block-relay connections would
match the "!IsInboundConn()" condition and return early.
ddefb5c0b759950942ac03f28c43b548af7b4033 p2p: Use the greatest common version in peer logic (Hennadii Stepanov)
e084d45562b94827b3a7873895882fcaae9f4d48 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov)
8d2026796a6f7add0c2cda9806e759817d1eae6f refactor: Rename local variable nSendVersion (Hennadii Stepanov)
e9a6d8b13b0558b17cdafbd32fd2663b4138ff11 p2p: Unify Send and Receive protocol versions (Hennadii Stepanov)
Pull request description:
On master (6fef85bfa3cd7f76e83b8b57f9e4acd63eb664ec) `CNode` has two members to keep protocol version:
- `nRecvVersion` for received messages
- `nSendVersion` for messages to send
After exchanging with `VERSION` and `VERACK` messages via protocol version `INIT_PROTO_VERSION`, both nodes set `nRecvVersion` _and_ `nSendVersion` to _the same_ value which is the greatest common protocol version.
This PR:
- replaces two `CNode` members, `nRecvVersion` `nSendVersion`, with `m_greatest_common_version`
- removes duplicated getter and setter
There is no change in behavior on the P2P network.
ACKs for top commit:
jnewbery:
ACK ddefb5c0b759950942ac03f28c43b548af7b4033
naumenkogs:
ACK ddefb5c0b759950942ac03f28c43b548af7b4033
fjahr:
Code review ACK ddefb5c0b759950942ac03f28c43b548af7b4033
amitiuttarwar:
code review but untested ACK ddefb5c0b7
benthecarman:
utACK `ddefb5c`
Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d
0d04784af151de249bbbcbad51e6e8ad9af8f5a3 Refactor the functional test (Gleb Naumenko)
83ad65f31b5c9441ae1618614082e584854a14e1 Address nits in ADDR caching (Gleb Naumenko)
81b00f87800f40cb14f2131ff27668bd2bb9e551 Add indexing ADDR cache by local socket addr (Gleb Naumenko)
42ec5585424ceb91bed07826dde15697c020661a Justify the choice of ADDR cache lifetime (Gleb Naumenko)
Pull request description:
This is a follow-up on #18991 which does 3 things:
- improves privacy of a node listening to multiple addresses via adding cache index by local socket addr (suggested [here](https://github.com/bitcoin/bitcoin/pull/18991#issuecomment-668219345))
- documents on the choice of 24h cache lifetime
- addresses nits from #18991
ACKs for top commit:
jnewbery:
utACK 0d04784af151de249bbbcbad51e6e8ad9af8f5a3
vasild:
ACK 0d04784
jonatack:
Code review ACK 0d04784
Tree-SHA512: bb65a34dd1ce2811186d3e4469bc33e8399cebaaa494ce13041c7cff23275870e4176a719f7a72f8d779c49f8b2344bf4fa1aeb3ea4e2626d5ae76514f00a750
a8a64acaf32ac21feeb885671772282b531ef9a2 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c0381266e0e05a408f8e1818501ab73d29110 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65cdc52a3b259effe0c29b5eafb1b5ff5 [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4cd06e17c8c65b36bf15c3ea2641de4 [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)
Pull request description:
Addresses some outstanding review comments from #18044
- reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
- adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
- removes some dead code
Links to comments on wtxid PR: [1](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254) [2](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460496023) [3](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r463532611)
thanks to jnewbery & adamjonas for flagging these ! !
ACKs for top commit:
sdaftuar:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
naumenkogs:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
jnewbery:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277
The field m_protect is used to protect from eviction both by bad/lagging
chain and extra outbound peers logics. Outbound block-relay peers are
always excluded from this protection.
There is a keyword that allows us to break out of loops. Use it.
There's a small change in behaviour here: if we process multiple orphans
that are still orphans, then we'll only call mempool.check() once at the
end, instead of after processing each tx.
bb6a32ce9983c72afa90f41a43a47ffd703ca006 [net processing] Move Misbehaving() to PeerManager (John Newbery)
aa114b1c9b06c2bd3ed936bbb9fb32b31f75bdb2 [net_processing] Move SendBlockTransactions into PeerManager (John Newbery)
3115e00f75b41d9765dcbb376e367b25f61a1d58 [net processing] Move MaybePunishPeerForTx to PeerManager (John Newbery)
e662e2d42afaf9c67c898634a0f3bc200255b6ea [net processing] Move ProcessOrphanTx to PeerManager (John Newbery)
b70cd890e375e904b7f36b3d959e5656f5a5cbcd [net processing] Move MaybePunishNodeForBlock into PeerManager (John Newbery)
d7778351bf60a21925a97b7fc4e9df5541b6d995 [net processing] Move ProcessHeadersMessage to PeerManager (John Newbery)
64f6162651420be2f4aa1498f0378a86780bc089 [whitespace] tidy up indentation after scripted diff (John Newbery)
58bd369b0ddd3383f7bdf7840912d18b96545f91 scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager (John Newbery)
2297b26b3ce95e935c0ebb8c38dabf19965054a5 [net_processing] Pass chainparams to PeerLogicValidation constructor (John Newbery)
824bbd1ffba3df7ffa6f5bfaa31298cd484473b1 [move only] Collect all private members of PeerLogicValidation together (John Newbery)
Pull request description:
Continues the work of moving net_processing logic into PeerLogicValidation. See https://github.com/bitcoin/bitcoin/pull/19704 and https://github.com/bitcoin/bitcoin/pull/19607#discussion_r462032894 for motivation.
This PR also renames `PeerLogicValidation` to `PeerManager` as suggested in https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618.
ACKs for top commit:
MarcoFalke:
re-ACK bb6a32ce99 only change is rebase due to conflict in struct NodeContext and variable rename 🤸
hebasto:
re-ACK bb6a32ce9983c72afa90f41a43a47ffd703ca006, only rebased, and added renaming `s/peer_logic/peerman/` into scripted-diff since my [previous](https://github.com/bitcoin/bitcoin/pull/19791#pullrequestreview-483118079) review (verified with `git range-diff`).
Tree-SHA512: a2de4a521688fd25125b401e5575402c52b328a0fa27b3010567008d4f596b960aabbd02b2d81f42658f88f4365443fadb1008150a62fbcea123fb42d85a2c21
Block-relay-only peers were introduced by #15759. According to its
author, it was intented to make them only immune to outbound peer
rotation-based eviction and not from all eviction as modified comment
leans to think of.
Clearly indicate that outbound block-relay peers aren't protected
from eviction by the bad/lagging chain logic.
-BEGIN VERIFY SCRIPT-
sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test)
sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test)
-END VERIFY SCRIPT-
PeerLogicValidation was originally net_processing's implementation to
the validation interface. It has since grown to contain much of
net_processing's logic. Therefore rename it to reflect its
responsibilities.
Suggested in
https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618.
Keep a references to chainparams, rather than calling the global
Params() function every time it's needed. This is fine, since
globalChainParams does not get updated once it's been set, and it's
available at the point of constructing the PeerLogicValidation object.
We don't have a project style for ordering class members, but it always
makes sense to have no more than one of each public/protected/private
specifier.
Also move documentation for MaybeDiscourageAndDisconnect to the header.
