3e68efa615 [net] Move checks from GetLocalAddrForPeer to caller (John Newbery)
d21d2b264c [net] Change AdvertiseLocal to GetLocalAddrForPeer (John Newbery)
Pull request description:
This is the first part of #21186. It slightly disentangles addr handling in net/net_processing by making it explicit that net_processing is responsible for pushing addr records into `vAddrToSend`.
ACKs for top commit:
MarcoFalke:
re-ACK 3e68efa615🍅
Tree-SHA512: 9af50c41f5a977e2e277f24a589db38e2980b353401def5e74b108ac5f493d9b5d6b1b8bf15323a4d66321495f04bc271450fcef7aa7d1c095f051a4f8e9b15f
GetLocalAddrForPeer() is only called in one place. The checks inside that
function make more sense to be carried out be the caller:
- fSuccessfullyConnected is already checked at the top of
SendMessages(), so must be true when we call GetLocalAddrForPeer()
- fListen can go into the conditional before GetLocalAddrForPeer() is
called.
Gossiping addresses to peers is the responsibility of net processing.
Change AdvertiseLocal() in net to just return an (optional) address
for net processing to advertise. Update function name to reflect
new responsibility.
fafddfadda scripted-diff: Remove shadowing lock annotations (MarcoFalke)
Pull request description:
Would be good to not redundantly copy the lock annotation from the class declaration to the member implementation. Otherwise it may not result in a compile failure if a new lock requirement is added to the member implementation, but not the class declaration.
ACKs for top commit:
amitiuttarwar:
ACK `fafddfadda`, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :)
hebasto:
ACK fafddfadda
jonatack:
Light utACK fafddfadda verified that the removed annotations in the definitions correspond to those in their respective declarations
Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2
Can be reviewed with --word-diff-regex=.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/(PeerManagerImpl::.*\)).*LOCKS_.*\)/\1/g' ./src/net_processing.cpp
-END VERIFY SCRIPT-
6452190841 net_processing: simplify MaybeSetPeerAsAnnouncingHeaderAndIDs args (Anthony Towns)
39c2a69bc2 net_processing: move MaybeSetPeerAsAnnouncingHeadersAndIDs into PeerManagerImpl (Anthony Towns)
7b7117efd0 net_processing: simplify ProcessGetData and FindTxForGetData args (Anthony Towns)
34207b9004 net_processing: move FindTxForGetData and ProcessGetData to PeerManagerImpl (Anthony Towns)
d44084883a net_processing: simplify PeerManageImpl method args (Anthony Towns)
a490f0a056 net_processing: move MarkBlockAs*, TipMayBeStale, FindNextBlocksToDL to PeerManagerImpl (Anthony Towns)
052d9bc7e5 net_processing: simplify AlreadyHaveTx args (Anthony Towns)
eeac506250 net_processing: move AlreadyHaveTx into PeerManageImpl (Anthony Towns)
9781c08a33 net_processing: move some globals into PeerManagerImpl (Anthony Towns)
Pull request description:
Turns some globals into member variables, and simplifies the parameter list for some of net_processing's internal functions. Mostly just serves as a code cleanup at this point.
ACKs for top commit:
jnewbery:
Code review ACK 6452190841
ariard:
Code Review ACK 6452190, changes are pretty straightforward.
MarcoFalke:
Concept ACK 6452190841 I have not reviewed this, but I left a comment 🐡
Tree-SHA512: 381361f9dbfeb851a5522ead3165ce1447a0f212ddea4b483aa38975559ee5ed03a4ba69c24fd69f36847a1eddfef05785f5cbb2fcec5fe50f8b336e8047c3b1
53e716ea11 [refactor] improve style for touched code (gzhao408)
174cb5330a [refactor] const ATMPArgs and non-const Workspace (gzhao408)
f82baf0762 [refactor] return MempoolAcceptResult (gzhao408)
9db10a5506 [refactor] clean up logic in testmempoolaccept (gzhao408)
Pull request description:
This is the first 4 commits of #20833, and does refactoring only. It should be relatively simple to review, and offers a few nice things:
- It makes accessing values that don't make sense (e.g. fee) when the tx is invalid an error.
- Returning `MempoolAcceptResult` from ATMP makes the interface cleaner. The caller can get a const instead of passing in a mutable "out" param.
- We don't have to be iterating through a bunch of lists for package validation, we can just return a `std::vector<MempoolAcceptResult>`.
- We don't have to refactor all ATMP call sites again if/when we want to return more stuff from it.
ACKs for top commit:
MarcoFalke:
ACK 53e716ea11💿
jnewbery:
Code review ACK 53e716ea11
ariard:
Code Review ACK 53e716e, I did tweak a bit the touched paths to see if we had good test coverage. Didn't find holes.
Tree-SHA512: fa6ec324a08ad9e6e55948615cda324cba176255708bf0a0a0f37cedb7a75311aa334ac6f223be7d8df3c7379502b1081102b9589f9a9afa1713ad3d9ab3c24f
3ddbf22ed1 util: Disallow negative mocktime (MarcoFalke)
f5f2f97168 net: Avoid UBSan warning in ProcessMessage(...) (practicalswift)
Pull request description:
Avoid UBSan warning in `ProcessMessage(...)`.
Context: https://github.com/bitcoin/bitcoin/pull/20380#issuecomment-770427182 (thanks Crypt-iQ!)
ACKs for top commit:
MarcoFalke:
re-ACK 3ddbf22ed1 only change is adding patch written by me
ajtowns:
ACK 3ddbf22ed1 -- code review only
Tree-SHA512: e8d7af0457ca86872b75a4e406c0a93aafd841c2962e244e147e748cc7ca118c56be0fdafe53765f4b291410030b2c3cc8f76f733b37a955d34fc885ab6037b9
This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
67c9a83df1 style-only: Remove redundant sentence in ActivateBestChain comment (Carl Dong)
b8e95658d5 style-only: Make TestBlockValidity signature readable (Carl Dong)
0cdad75390 validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock (Carl Dong)
ea4fed9021 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (Carl Dong)
e0dc305727 validation: Move LoadExternalBlockFile to CChainState (Carl Dong)
5f8cd7b3a5 validation: Remove global ::ActivateBestChain (Carl Dong)
2a696472a1 validation: Pass in chainstate to ::NotifyHeaderTip (Carl Dong)
9c300cc8b3 validation: Pass in chainstate to TestBlockValidity (Carl Dong)
0e17c833cd validation: Make CChainState.m_blockman public (Carl Dong)
d363d06bf7 validation: Pass in blockman to ContextualCheckBlockHeader (Carl Dong)
f11d11600d validation: Move GetLastCheckpoint to BlockManager (Carl Dong)
e4b95eefbc validation: Move GetSpendHeight to BlockManager (Carl Dong)
b026e318c3 validation: Move FindForkInGlobalIndex to BlockManager (Carl Dong)
3664a150ac validation: Remove global LookupBlockIndex (Carl Dong)
eae54e6e60 scripted-diff: Use BlockManager::LookupBlockIndex (Carl Dong)
15d20f40e1 validation: Move LookupBlockIndex to BlockManager (Carl Dong)
f92dc6557a validation: Guard the active_chainstate with cs_main (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jnewbery:
utACK 67c9a83df1
laanwj:
re-ACK 67c9a83df1
ryanofsky:
Code review ACK 67c9a83df1. Changes since last review:
Tree-SHA512: 8744aba2dd57a40cd2fedca809b0fe24d771bc60da1bffde89601999384aa0df428057a86644a3f72fbeedbc8b04db6c4fd264ea0db2e73c279e5acc6d056cbf
nSyncStarted, mapBlockSource, g_wtxid_relay_peers,
g_outbound_peers_with_protect_from_disconnect were all only used by
PeerManagerImpl methods already.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
FindForkInGlobalIndex only acts on BlockManager.
Note to reviewers: Since FindForkInGlobalIndex is always called with
::ChainActive() as its first parameter, it is possible to move
FindForkInGlobalIndex to CChainState and remove this const CChain&
parameter to instead use m_chain. However, it seems like the original
intention was for FindForkInGlobalIndex to work with _any_ chain, not
just the current active chain. Let me know if this should be changed.
[META] In a previous commit, we moved ::LookupBlockIndex to become a
member function of BlockManager. This commit is split out from
that one since it can be expressed nicely as a scripted-diff.
-BEGIN VERIFY SCRIPT-
find_regex='LookupBlockIndex' \
&& git grep -l -E "$find_regex" -- src \
| grep -v '^src/validation\.\(cpp\|h\)$' \
| xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g"
-END VERIFY SCRIPT-
This brings PushMessage and ProcessMessages further in line with the
style guide by fixing their if statements.
LogMessage is later called, inside an if statement, inside both of these
methods.
c119ba3c9b [doc] clarify getdata limit after #14897 (Michael Polzer)
Pull request description:
GETDATA is limited to blocks and transactions now and can't be used for other non-block data
ACKs for top commit:
laanwj:
ACK c119ba3c9b
theStack:
ACK c119ba3c9b
benthecarman:
ACK c119ba3c9b
Tree-SHA512: d6e9c109bcce4ef004ec83a9ec591163279476524dec97ed5f5c34e322dca35af66a168f0878ff972bbcec79d81623903f3619fedf8f88cdced3f3f66a779173
-dropmessagestest is a command line option that causes 1 in n received
messages to be dropped. The Bitcoin P2P protocol is stateful and in
general cannot handle messages being dropped. Dropped
version/verack/ping/pong messages will cause the connection to time out
and be torn down. Other dropped messages may also cause the peer to
believe that the peer has stalled and tear down the connection.
It seems difficult to uncover any actual issues with -dropmessagestest,
and any coverage that could be generated would probably be easier to
trigger with fuzz testing.
BIP 130 (sendheaders) and BIP 152 (compact blocks) do not specify at
which stage the `sendheaders` or `sendcmpct` messages should be sent.
Therefore we should tolerate them being sent before the version-verack
handshake is complete.
7fabe0f359 net: don't relay to the address' originator (Vasil Dimov)
Pull request description:
For each address to be relayed we "randomly" pick 2 nodes to send the
address to (in `RelayAddress()`). However we do not take into
consideration that it does not make sense to relay the address back to
its originator (`CNode::PushAddress()` will do nothing in that case).
This means that if the originator is among the "randomly" picked nodes,
then we will relay to one node less than intended.
Fix this by skipping the originating node when choosing candidates to
relay to.
ACKs for top commit:
sdaftuar:
ACK 7fabe0f359 (this time I looked at the test, and verified the test breaks in expected ways if I break the code).
jnewbery:
utACK 7fabe0f359 (only net_processing changes. I haven't reviewed the test changes)
jonatack:
re-ACK 7fabe0f359 per `git range-diff b76abae fd897f8 7fabe0f`, change since last review is rebase and more readable Doxygen documentation
Tree-SHA512: c6a9d11c7afc97ab4e8960513f6416648d4a8c0c64b713c145a7482a7b9e54946f81386a3351e3ec0011e5594ba5ccff4d10c6f656bb80680d9f0d0a63366165
f6360088de [net processing] Clarify UpdatedBlockTip() (John Newbery)
94d2cc35be [net processing] Remove unnecesary nNewHeight variable in UpdatedBlockTip() (John Newbery)
8b57013473 [net processing] Remove nStartingHeight check from block relay (John Newbery)
Pull request description:
nStartingHeight was introduced in commit 7a47324c7 (Bitcoin version
0.2.9, P2P version 209) with the comment "better prevention of inventory
relaying during initial download". At that time, there was no function
to determine whether the node was still in Initial Block Download, so to
prevent syncing nodes from relaying old blocks to their peers, a check
was added to never relay a block to a peer where the height was lower
than 2000 less than the peer's best block. That check was updated
several times in later commits to ensure that we weren't relaying blocks
before the latest checkpoint if the peer didn't provide a
startingheight. The checkpoint comparison was changed to compare with an
estimate of the highest block in commit eae82d8e.
In commit 202e0194, all block relay was gated on being out of Initial
Block Download. In commit 0278fb5f, the comparison to nBlockEstimate was
removed since "we already checked IsIBD()".
We can remove the check against nStartingHeight entirely. If the node is
out of Initial Block Download, then its tip height must have been within
24 hours of current time, so should not be more than ~144 blocks behind
the most work tip.
This simplifies moving block inventory state into the `Peer` object (#19829).
ACKs for top commit:
Sjors:
utACK f636008
jonatack:
ACK f6360088de
MarcoFalke:
ACK f6360088de💽
ariard:
Code Review ACK f636008
Tree-SHA512: 4959cf35f1dcde46f34bffec1375729a157e1b2a1fd8a8ca33da9771c3c89a6c43e7050cdeeab8d90bb507b0795703db8c8bc304a1a5065ef00aae7a6992ca4f
4b7b58b3fe Update net_processing WTXID documentation per BIP339 (Jon Atack)
Pull request description:
BIP339 currently states:
*The wtxidrelay message MUST be sent in response to a version message from a peer whose protocol version is >= 70016 and prior to sending a verack. A wtxidrelay message received after a verack message MUST be ignored or treated as invalid.*
ACKs for top commit:
MarcoFalke:
ACK 4b7b58b3fe
practicalswift:
ACK 4b7b58b3fe
RiccardoMasutti:
ACK 4b7b58b
Tree-SHA512: 58ca6b197618cc73c70aa5de0a2d9d89a68b4cad9d5a708278ef17a9d6854d4362bcc384b6d29696642924977204a8fc120b31e91e2d97b6072b7b0d41c9f2dc
a33442fdc7 Remove m_is_manual_connection from CNodeState (Antoine Riard)
Pull request description:
Currently, this member is only used to exclude MANUAL peers from discouragement
in MaybePunishNodeForBlock(). Manual connections are already protected in
MaybeDiscourageAndDisconnect(), independently from their network
processing behaviors.
ACKs for top commit:
MarcoFalke:
cr ACK a33442fdc7
promag:
Code review ACK a33442fdc7.
jnewbery:
utACK a33442fdc7
amitiuttarwar:
code review ACK a33442fdc7
Tree-SHA512: cfe3f3dfa131373e3299002d34ae9e22ca6e1a966831bab32fcf06ff1d08f06095b4ab020cc4d267f3ec05ae23fbdc22373382ab828b999c0db11b8c842a4f0c
faaad1bbac p2p: Ignore version msgs after initial version msg (MarcoFalke)
fad68afcff p2p: Ignore non-version msgs before version msg (MarcoFalke)
Pull request description:
Handshake misbehaviour doesn't cost us more than any other unknown message, so it seems odd to treat it differently
ACKs for top commit:
jnewbery:
utACK faaad1bbac
practicalswift:
ACK faaad1bbac: patch looks correct
Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff
nStartingHeight was introduced in commit 7a47324c7 (Bitcoin version
0.2.9, P2P version 209) with the comment "better prevention of inventory
relaying during initial download". At that time, there was no function
to determine whether the node was still in Initial Block Download, so to
prevent syncing nodes from relaying old blocks to their peers, a check
was added to never relay a block to a peer where the height was lower
than 2000 less than the peer's best block. That check was updated
several times in later commits to ensure that we weren't relaying blocks
before the latest checkpoint if the peer didn't provide a
startingheight. The checkpoint comparison was changed to compare with an
estimate of the highest block in commit eae82d8e.
In commit 202e0194, all block relay was gated on being out of Initial
Block Download. In commit 0278fb5f, the comparison to nBlockEstimate was
removed since "we already checked IsIBD()".
We can remove the check against nStartingHeight entirely. If the node is
out of Initial Block Download, then its tip height must have been within
24 hours of current time, so should not be more than ~144 blocks behind
the most work tip.
Currently, this member is only used to exclude MANUAL peers from discouragement
in MaybePunishNodeForBlock(). Manual connections are already protected in
MaybeDiscourageAndDisconnect(), independently from their network
processing behaviors.
To make eclipse attacks more difficult, regularly initiate outbound connections
and stay connected long enough to sync headers and potentially learn of new
blocks. If we learn a new block, rotate out an existing block-relay peer in
favor of the new peer.
This augments the existing outbound peer rotation that exists -- currently we
make new full-relay connections when our tip is stale, which we disconnect
after waiting a small time to see if we learn a new block. As block-relay
connections use minimal bandwidth, we can make these connections regularly and
not just when our tip is stale.
Like feeler connections, these connections are not aggressive; whenever our
timer fires (once every 5 minutes on average), we'll try to initiate a new
block-relay connection as described, but if we fail to connect we just wait for
our timer to fire again before repeating with a new peer.
For each address to be relayed we "randomly" pick 2 nodes to send the
address to (in `RelayAddress()`). However we do not take into
consideration that it does not make sense to relay the address back to
its originator (`CNode::PushAddress()` will do nothing in that case).
This means that if the originator is among the "randomly" picked nodes,
then we will relay to one node less than intended.
Fix this by skipping the originating node when choosing candidates to
relay to.
343dc4760f test: add test for high-bandwidth mode states in getpeerinfo (Sebastian Falbesoner)
dab6583307 doc: release note for new getpeerinfo fields "bip152_hb_{from,to}" (Sebastian Falbesoner)
a7ed00f8bb rpc: expose high-bandwidth mode states via getpeerinfo (Sebastian Falbesoner)
30bc8fab68 net: save high-bandwidth mode states in CNodeStats (Sebastian Falbesoner)
Pull request description:
Fixes#19676, "_For every peer expose through getpeerinfo RPC whether or not we selected them as HB peers, and whether or not they selected us as HB peers._" See [BIP152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki), in particular the [protocol flow diagram](https://github.com/bitcoin/bips/raw/master/bip-0152/protocol-flow.png). The newly introduced states are changed on the following places in the code:
* on reception of a `SENDCMPCT` message with valid version, the field `m_highbandwidth_from` is changed depending on the first integer parameter in the message (1=high bandwidth, 0=low bandwidth), i.e. it just mirrors the field `CNodeState.fPreferHeaderAndIDs`.
* after adding a `SENDCMPCT` message to the send queue, the field `m_highbandwidth_to` is changed depending on how the first integer parameter is set (same as above)
Note that after receiving `VERACK`, the node also sends `SENDCMPCT`, but that is only to announce the preferred version and never selects high-bandwidth mode, hence there is no need to change the state variables there, which are initialized to `false` anyways.
ACKs for top commit:
naumenkogs:
reACK 343dc4760f
jonatack:
re-ACK 343dc4760f per `git range-diff 7ea6499 4df1d12 343dc47`
Tree-SHA512: f4999e6a935266812c2259a9b5dc459710037d3c9e938006d282557cc225e56128f72965faffb207fc60c6531fab1206db976dd8729a69e8ca29d4835317b99f
fa11110bff util: Allow use of C++14 chrono literals (MarcoFalke)
Pull request description:
I think we should allow the use of chrono literals for new code to make it less verbose. Obviously old code can stay as-is.
This patch pulls in the needed namespace and replaces some lines for illustrative purposes.
ACKs for top commit:
vasild:
ACK fa11110bff
jonatack:
ACK fa11110bff
Tree-SHA512: ee2b72c8f28dee07b33b9a8ee8f7c87c0bc43b05c56a17b786cf9803ef204c7628e01b02de1af1a4eb01f5cdf6fc336f69c2833e17acd606ebda20ac6917e6bb
3025ca9e77 [net processing] Add RemovePeer() (John Newbery)
a20ab22786 [net processing] Make GetPeerRef const (John Newbery)
ed7e469cee [net_processing] Move peer_map to PeerManager (John Newbery)
a529fd3e3f [net processing] Move GetNodeStateStats into PeerManager (John Newbery)
Pull request description:
This moves `g_peer_map` from a global in net_processing.cpp's unnamed namespace to being a member `m_peer_map` of `PeerManager`.
ACKs for top commit:
theuni:
Re-ACK 3025ca9e77.
dongcarl:
Re-ACK 3025ca9
hebasto:
re-ACK 3025ca9e77, since my [previous](https://github.com/bitcoin/bitcoin/pull/19910#pullrequestreview-545574237) review only reverted the change that introduced NRVO in `PeerManager::GetPeerRef`, and comments are fixed in the proper commits.
Tree-SHA512: 6369eb3c688ac5b84f89f7674115f78ff02edbed76063ac2ebb1759894c9e973883e10821a35dab92bd3d738280acc095bd5368f552a060b83cd309330387d47
1583498fb6 Send and require SENDADDRV2 before VERACK (Pieter Wuille)
c5a8919660 Don't send 'sendaddrv2' to pre-70016 software (Pieter Wuille)
Pull request description:
BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some implementations reject messages they don't know. As a courtesy, don't send it to nodes with a version before 70016, as no software is known to support BIP155 that doesn't announce at least that protocol version number.
Also move the sending of sendaddrv2 earlier (before sending verack), as proposed in https://github.com/bitcoin/bips/pull/1043. This has the side effect that local address broadcast of torv3 will work (as it'll only trigger after we know whether or not the peer supports addrv2).
ACKs for top commit:
MarcoFalke:
ACK 1583498fb6
jnewbery:
ACK 1583498fb6
jonatack:
ACK 1583498fb6
vasild:
ACK 1583498
Tree-SHA512: 3bd5833fa8c8567b6dedd99e4a9b6bb71c127aa66d5284b217503c86d597dc59aa7382c41f3a4bf561bb658b89db81d1a7703a700eef4ffc17cb916660e23a82
65273fa0e7 Clear m_addr_known before our periodic self-advertisement (Suhas Daftuar)
Pull request description:
We use a rolling bloom filter to track which addresses we've previously sent a peer, but after #7125 we no longer clear it every day before our own announcement. This looks to me like an oversight which has the effect of reducing the frequency with which we actually self-announce our own address, so this reintroduces resetting that filter.
ACKs for top commit:
naumenkogs:
ACK 65273fa0e7
laanwj:
Code review ACK 65273fa0e7
sipa:
utACK 65273fa0e7
Tree-SHA512: 602c155fb6d2249b054fcb6f1c0dd17143605ceb87132286bbd90babf26d258ff6c41f9925482c17e2be41805d33f9b83926cb447f394969ffecd4bccfa0a64f
as BIP339 currently states:
"The wtxidrelay message MUST be sent in response to a version
message from a peer whose protocol version is >= 70016 and
prior to sending a verack. A wtxidrelay message received after
a verack message MUST be ignored or treated as invalid."
1816327e53 p2p: Put disconnecting logs into BCLog::NET category (Hennadii Stepanov)
Pull request description:
It's too noisy:
```
$ cat debug.log | wc -l
28529
$ cat debug.log | grep "Disconnecting and discouraging peer" | wc -l
10177
```
ACKs for top commit:
MarcoFalke:
noban, addnode and local peers are still unconditionally logged (as they should), but this one can go into a category, so cr-ACK 1816327e53
practicalswift:
ACK 1816327e53 for the reasons MarcoFalke gave above.
ajtowns:
ACK 1816327e53
Tree-SHA512: c312c1009090840659b2cb1364d8ad9b6ab8e742fc462aef169996d93c76c248507639a00257ed9d73a6916c01176b1793491b2305e92fdded5f9de0935b6ba6
This behavior was apparently inadvertently broken in 5400ef6; without this
change our daily self-announcements frequently go unsent, because our
address is still in the peer's rolling bloom filter (for potentially many
days, depending on addr traffic).
0bfce9dc46 [addrman] Fix Connected() comment (John Newbery)
eefe194718 [net] Consolidate logic around calling CAddrMan::Connected() (John Newbery)
Pull request description:
Currently, the logic around whether we called CAddrMan::Connected() for
a peer is spread between verack processing (where we discard inbound
peers) and FinalizeNode (where we discard misbehaving and
block-relay-only peers). Consolidate that logic to a single place.
Also remove the CNode.fCurrentlyConnected bool, which is now
redundant. We can rely on CNode.fSuccessfullyConnected, since the two
bools were only ever flipped to true in the same place.
ACKs for top commit:
mzumsande:
Code review ACK 0bfce9dc46
amitiuttarwar:
code review ACK 0bfce9dc46. nice tidy, and bonus that we get to remove an unnecessary call to `cs_main`
Tree-SHA512: 1ab74dae3bc12a6846da57c853033e546bb4f91caa39f4c50bf0cf7eca59cb917bdb2ef795da55363e7e9f70949cb28bb3be004cb3afa4389f970d2fe097d932
Sending a version message after the intial version message is peer
misbehavior. Though, it seems arbitrary and confusing to disconnect only
after exactly 100 version messages. Duplicate version messages affect us
no different than any other unknown message. So remove the Misbehaving
and ignore the redundant msgs.
Sending a non-version message before the initial version message is peer
misbehavior. Though, it seems arbitrary and confusing to disconnect only
after exactly 100 non-version messages. So remove the Misbehaving and
instead rely on the existing disconnect-due-to-handshake-timeout logic.
af3b0dfc54 net: fix output of peer address in version message (Vasil Dimov)
Pull request description:
If `-logips -debug=net` is specified then we print the contents of the
version message we send to the peer, including his address. Because the
addresses in the version message use pre-BIP155 encoding they cannot
represent a Tor v3 address and we would actually send 16 `0`s instead (a
dummy IPv6 address). However we would print the full address in the log
message. Before this fix:
```
2020-10-21T12:24:17Z send version message: version 70016, blocks=653500, us=[::]:0, them=xwjtp3mj427zdp4tljiiivg2l5ijfvmt5lcsfaygtpp6cw254kykvpyd.onion:8333, peer=0
```
This is confusing because we pretend to send one thing while we actually
send another. Adjust the printout to reflect what we are sending. After
this fix:
```
2020-10-21T12:26:54Z send version message: version 70016, blocks=653500, us=[::]:0, them=[::]:0, peer=0
```
ACKs for top commit:
MarcoFalke:
review ACK af3b0dfc54
jnewbery:
utACK af3b0dfc54
Tree-SHA512: f169d7b4f07c219e541f7c37ea23b82c77e50085fc72ec62f1dd46970389916e177268d07d45c7be94dd209d1903f8f23eaff62b7fa782f6057dd36bb96bba82
Currently, the logic around whether we called CAddrMan::Connected() for
a peer is spread between verack processing (where we discard inbound
peers) and FinalizeNode (where we discard misbehaving and
block-relay-only peers). Consolidate that logic to a single place.
Also remove the CNode.fCurrentlyConnected bool, which is now
redundant. We can rely on CNode.fSuccessfullyConnected, since the two
bools were only ever flipped to true in the same place.
16d9bfc417 Avoid test-before-evict evictions of current peers (Suhas Daftuar)
e8b215a086 Refactor test for existing peer connection into own function (Suhas Daftuar)
4fe338ab3e Call CAddrMan::Good() on block-relay-only peer addresses (Suhas Daftuar)
daf5553126 Avoid calling CAddrMan::Connected() on block-relay-only peer addresses (Suhas Daftuar)
Pull request description:
This PR does two things:
* Block-relay-only interaction with addrman.
* Calling `CAddrMan::Connected()` on an address that was a block-relay-only peer causes the time we report in `addr` messages containing that peer to be updated; particularly now that we use anchor connections with a our block-relay-only peers, this risks leaking information about those peers. So, stop this.
* Avoiding calling `CAddrMan::Good()` on block-relay-only peer addresses causes the addrman logic around maintaining the new and tried table to be less good, and in particular makes it so that block-relay-only peer addresses are more likely to be evicted from the addrman (for no good reason I can think of). So, mark those addresses as good when we connect.
* Fix test-before-evict bug. There's a bug where if we get a collision in the tried table with an existing address that is one of our current peers, and the connection is long-lived enough, then `SelectTriedCollisions()` might return that existing peer address to us as a test-before-evict connection candidate. However, our logic for new outbound connections would later prevent us from actually making a connection; the result would be that when we get a collision with a long-lived current peer, that peer's address is likely to get evicted from the tried table. Fix this by checking to see if a test-before-evict candidate is a peer we're currently connected to, and if so, mark it as `Good()`.
ACKs for top commit:
sipa:
utACK 16d9bfc417
amitiuttarwar:
code review ACK 16d9bfc417
mzumsande:
Code-Review ACK 16d9bfc417.
jnewbery:
utACK 16d9bfc417
ariard:
Code Review ACK 16d9bfc.
jonatack:
Tested ACK 16d9bfc417
Tree-SHA512: 188ccb814e436937cbb91d29d73c316ce83f4b9c22f1cda56747f0949a093e10161ae724e87e4a2d85ac40f85f5f6b4e87e97d350a1ac44f80c57783f4423324
Being able to invoke Good() is important for address management (new vs tried
table, tried table eviction via test-before-evict). We mitigate potential
information leaks by not calling Connected() on these peer addresses.
If `-logips -debug=net` is specified then we print the contents of the
version message we send to the peer, including his address. Because the
addresses in the version message use pre-BIP155 encoding they cannot
represent a Tor v3 address and we would actually send 16 `0`s instead (a
dummy IPv6 address). However we would print the full address in the log
message. Before this fix:
```
2020-10-21T12:24:17Z send version message: version 70016, blocks=653500, us=[::]:0, them=xwjtp3mj427zdp4tljiiivg2l5ijfvmt5lcsfaygtpp6cw254kykvpyd.onion:8333, peer=0
```
This is confusing because we pretend to send one thing while we actually
send another. Adjust the printout to reflect what we are sending. After
this fix:
```
2020-10-21T12:26:54Z send version message: version 70016, blocks=653500, us=[::]:0, them=[::]:0, peer=0
```
da0988daf1 scripted-diff: rename vRecvGetData (Neha Narula)
ba951812ec Guard vRecvGetData (now in net processing) with its own mutex (Neha Narula)
2d9f2fca43 Move vRecvGetData to net processing (Neha Narula)
673247b58c Lock before checking if orphan_work_set is empty; indicate it is guarded (Neha Narula)
8803aee668 Move m_orphan_work_set to net_processing (Neha Narula)
9c47cb29f9 [Rename only] Rename orphan_work_set to m_orphan_work_set. (Neha Narula)
Pull request description:
Add annotations to guard `vRecvGetData` and `orphan_work_set` and fix up places where they were accessed without a lock. There is no current data race because they happen to be accessed by only one thread, but this might not always be the case.
Original discussion: https://github.com/bitcoin/bitcoin/pull/18861#discussion_r451778445
ACKs for top commit:
MarcoFalke:
review ACK da0988daf1🐬
jnewbery:
Code review ACK da0988daf1
hebasto:
ACK da0988daf1, I have reviewed the code and it looks correct, I agree it can be merged.
Tree-SHA512: 31cadd319ddc9273a87e77afc4db7339fd636e816b5e742eba5cb32927ac5cc07a672b2268d2d38a75a0f1b17d93836adab9acf7e52f26ea9a43f54efa57257e
fa1f6f237d net: Send post-verack handshake messages at most once (MarcoFalke)
Pull request description:
There is no need to send `SENDHEADERS` and `SENDCMPCT` messages as a reply to each `VERACK` that is received. For alive checks, a `PING`/`PONG` can be used.
ACKs for top commit:
jonatack:
Concept ACK fa1f6f237d this is the only code section that sets `fCurrentlyConnected` and `fSuccessfullyConnected` to true. Could add a test. I did not verify if this code is actually being called repeatedly post initial verack; was it?
hebasto:
ACK fa1f6f237d, I have reviewed the code and it looks OK, I agree it can be merged.
naumenkogs:
ACK fa1f6f237d
laanwj:
Code review ACK fa1f6f237d
Tree-SHA512: c841d5d3807254a49463bbcfac3b32881b34a9d3206899544c86322c20988e17ad2ae243cba227fd3825a914f0cb2584451edda2414aecee6d5e3f5a0636f08a
fd9a0060f0 Report and verify expirations (Pieter Wuille)
86f50ed10f Delete limitedmap as it is unused now (Pieter Wuille)
cc16fff3e4 Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille)
173a1d2d3f Expedite removal of tx requests that are no longer needed (Pieter Wuille)
de11b0a4ef Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille)
242d16477d Change transaction request logic to use txrequest (Pieter Wuille)
5b03121d60 Add txrequest fuzz tests (Pieter Wuille)
3c7fe0e5a0 Add txrequest unit tests (Pieter Wuille)
da3b8fde03 Add txrequest module (Pieter Wuille)
Pull request description:
This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests).
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 already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available).
* The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions).
This replaces #19184, rebased on #18044 and with many small changes.
ACKs for top commit:
ariard:
Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic.
MarcoFalke:
Approach ACK fd9a0060f0🏹
naumenkogs:
Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events.
jnewbery:
utACK fd9a0060f0
jonatack:
WIP light ACK fd9a0060f0 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers.
ryanofsky:
Light code review ACK fd9a0060f0, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on:
Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7
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.
dcf0cb4776 tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
353a3fdaad net: advertise support for ADDRv2 via new message (Vasil Dimov)
201a4596d9 net: CAddress & CAddrMan: (un)serialize as ADDRv2 (Vasil Dimov)
1d3ec2a1fd 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 dcf0cb4776 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 dcf0cb4776
hebasto:
re-ACK dcf0cb4776, 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 dcf0cb4776 merged on master (12a1c3ad1a).
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>
b048b275d9 [validation] Remove absurdfee from accepttomempool (John Newbery)
932564b9cf scripted-diff: update max-fee-exceeded error message to include RPC (gzhao408)
8f1290c601 [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 b048b275d9
LarryRuane:
re-ACK b048b275d9
MarcoFalke:
re-ACK b048b275d9 , only change is squashing one commit 🏦
instagibbs:
utACK b048b275d9
Tree-SHA512: 57c17ba16d230a4cae2896dd6a64c924f307757824e35784bf96da7b10aff2d8ea910710edf35e981035623a155f8766209a92a0fdb856549fde78bc3eaae4d2
675e55e013 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 675e55e013
practicalswift:
ACK 675e55e013: patch looks correct
MarcoFalke:
ACK 675e55e013
hebasto:
ACK 675e55e013, the offender peer will be eventually disconnected due to the timeout.
Tree-SHA512: 8d2b1d8b9843f2ee26b2c30f7c5ff0bfcfbe3f46b32cd0369c48ece26624151091237e83ce3f18c6da004099026602cfab1642ac916db777f047d170b365c007
d76925478e [doc] Clarify semantic of peer's m_protect w.r.t to outbound eviction logics (Antoine Riard)
ac71fe936d [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 d76925478e
jonatack:
ACK d76925478e
Tree-SHA512: 597fbd62838a6e39276024165b11514cad20a2e9d33cf9202d261cbadcb62b2df427c858e0cb57e585840d4c1d4600104aa53916bb868541f2580e4eed9b4b52
deb52711a1 Remove header checks out of net_processing (Troy Giorshev)
52d4ae46ab Give V1TransportDeserializer CChainParams& member (Troy Giorshev)
5bceef6b12 Change CMessageHeader Constructor (Troy Giorshev)
1ca20c1af8 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev)
890b1d7c2b Move checksum check from net_processing to net (Troy Giorshev)
2716647ebf 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 deb52711a1 just rebase due to conflict on adjacent line
jnewbery:
Code review ACK deb52711a1.
Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
a512925e19 [doc] Release notes (Amiti Uttarwar)
50f94b34a3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b50 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca4 [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 a512925e19.
sipa:
utACK a512925e19
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19🌇
promag:
Code review ACK a512925e19.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
0bd1184adf Remove unused LockAssertion struct (Hennadii Stepanov)
ab2a44297f Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov)
73f71e1996 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 0bd1184adf
ajtowns:
ACK 0bd1184adf
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.
ddefb5c0b7 p2p: Use the greatest common version in peer logic (Hennadii Stepanov)
e084d45562 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov)
8d2026796a refactor: Rename local variable nSendVersion (Hennadii Stepanov)
e9a6d8b13b p2p: Unify Send and Receive protocol versions (Hennadii Stepanov)
Pull request description:
On master (6fef85bfa3) `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 ddefb5c0b7
naumenkogs:
ACK ddefb5c0b7
fjahr:
Code review ACK ddefb5c0b7
amitiuttarwar:
code review but untested ACK ddefb5c0b7
benthecarman:
utACK `ddefb5c`
Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d
0d04784af1 Refactor the functional test (Gleb Naumenko)
83ad65f31b Address nits in ADDR caching (Gleb Naumenko)
81b00f8780 Add indexing ADDR cache by local socket addr (Gleb Naumenko)
42ec558542 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 0d04784af1
vasild:
ACK 0d04784
jonatack:
Code review ACK 0d04784
Tree-SHA512: bb65a34dd1ce2811186d3e4469bc33e8399cebaaa494ce13041c7cff23275870e4176a719f7a72f8d779c49f8b2344bf4fa1aeb3ea4e2626d5ae76514f00a750
a8a64acaf3 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c038126 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65c [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4 [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 a8a64acaf3
naumenkogs:
utACK a8a64acaf3
jnewbery:
utACK a8a64acaf3
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.
bb6a32ce99 [net processing] Move Misbehaving() to PeerManager (John Newbery)
aa114b1c9b [net_processing] Move SendBlockTransactions into PeerManager (John Newbery)
3115e00f75 [net processing] Move MaybePunishPeerForTx to PeerManager (John Newbery)
e662e2d42a [net processing] Move ProcessOrphanTx to PeerManager (John Newbery)
b70cd890e3 [net processing] Move MaybePunishNodeForBlock into PeerManager (John Newbery)
d7778351bf [net processing] Move ProcessHeadersMessage to PeerManager (John Newbery)
64f6162651 [whitespace] tidy up indentation after scripted diff (John Newbery)
58bd369b0d scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager (John Newbery)
2297b26b3c [net_processing] Pass chainparams to PeerLogicValidation constructor (John Newbery)
824bbd1ffb [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 bb6a32ce99, 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.
296be8f58e Get rid of unused functions CTxMemPool::GetMemPoolChildren, CTxMemPool::GetMemPoolParents (Jeremy Rubin)
46d955d196 Remove mapLinks in favor of entry inlined structs with iterator type erasure (Jeremy Rubin)
Pull request description:
Currently we have a peculiar data structure in the mempool called maplinks. Maplinks job is to track the in-pool children and parents of each transaction. This PR can be primarily understood and reviewed as a simple refactoring to remove this extra data structure, although it comes with a nice memory and performance improvement for free.
Maplinks is particularly peculiar because removing it is not as simple as just moving it's inner structure to the owning CTxMempoolEntry. Because TxLinks (the class storing the setEntries for parents and children) store txiters to each entry in the mempool corresponding to the parent or child, it means that the TxLinks type is "aware" of the boost multiindex (mapTx) it's coming from, which is in turn, aware of the entry type stored in mapTx. Thus we used maplinks to store this entry associated data we in an entirely separate data structure just to avoid a circular type reference caused by storing a txiter inside a CTxMempoolEntry.
It turns out, we can kill this circular reference by making use of iterator_to multiindex function and std::reference_wrapper. This allows us to get rid of the maplinks data structure and move the ownership of the parents/child sets to the entries themselves.
The benefit of this good all around, for any of the reasons given below the change would be acceptable, and it doesn't make the code harder to reason about or worse in any respect (as far as I can tell, there's no tradeoff).
### Simpler ownership model
No longer having to consistency check that mapLinks did have records for our CTxMempoolEntry, impossible to have a mapLinks entry outlive or incorrectly die before a CTxMempoolEntry.
### Memory Usage
We get rid of a O(Transactions) sized map in the mempool, which is a long lived data structure.
### Performance
If you have a CTxMemPoolEntry, you immediately know the address of it's children/parents, rather than having to do a O(log(Transactions)) lookup via maplinks (which we do very often). We do it in *so many* places that a true benchmark has to look at a full running node, but it is easy enough to show an improvement in this case.
The ComplexMemPool shows a good coherence check that we see the expected result of it being 12.5% faster / 1.14x faster.
```
Before:
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 1.40462, 0.277222, 0.285339, 0.279793
After:
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 1.22586, 0.243831, 0.247076, 0.244596
```
The ComplexMemPool benchmark only checks doing addUnchecked and TrimToSize for 800 transactions. While this bench does a good job of hammering the relevant types of function, it doesn't test everything.
Subbing in 5000 transactions shows a that the advantage isn't completely wiped out by other asymptotic factors (this isn't the only bottleneck in growing the mempool), but it's only a bit proportionally slower (10.8%, 1.12x), which adds evidence that this will be a good change for performance minded users.
```
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 59.1321, 11.5919, 12.235, 11.7068
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 52.1307, 10.2641, 10.5206, 10.4306
```
I don't think it's possible to come up with an example of where a maplinks based design would have better performance, but it's something for reviewers to consider.
# Discussion
## Why maplinks in the first place?
I spoke with the author of mapLinks (sdaftuar) a while back, and my recollection from our conversation was that it was implemented because he did not know how to resolve the circular dependency at the time, and there was no other reason for making it a separate map.
## Is iterator_to weird?
iterator_to is expressly for this purpose, see https://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/indices.html#iterator_to
> iterator_to provides a way to retrieve an iterator to an element from a pointer to the element, thus making iterators and pointers interchangeable for the purposes of element pointing (not so for traversal) in many situations. This notwithstanding, it is not the aim of iterator_to to promote the usage of pointers as substitutes for real iterators: the latter are specifically designed for handling the elements of a container, and not only benefit from the iterator orientation of container interfaces, but are also capable of exposing many more programming bugs than raw pointers, both at compile and run time. iterator_to is thus meant to be used in scenarios where access via iterators is not suitable or desireable:
>
> - Interoperability with preexisting APIs based on pointers or references.
> - Publication of pointer-based interfaces (for instance, when designing a C-compatible library).
> - The exposure of pointers in place of iterators can act as a type erasure barrier effectively decoupling the user of the code from the implementation detail of which particular container is being used. Similar techniques, like the famous Pimpl idiom, are used in large projects to reduce dependencies and build times.
> - Self-referencing contexts where an element acts upon its owner container and no iterator to itself is available.
In other words, iterator_to is the perfect tool for the job by the last reason given. Under the hood it should just be a simple pointer cast and have no major runtime overhead (depending on if the function call is inlined).
Edit by laanwj: removed at sign from the description
ACKs for top commit:
jonatack:
re-ACK 296be8f per `git range-diff ab338a19 3ba1665 296be8f`, sanity check gcc 10.2 debug build is clean.
hebasto:
re-ACK 296be8f58e, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19478#pullrequestreview-482400727) review (verified with `git range-diff`).
Tree-SHA512: f5c30a4936fcde6ae32a02823c303b3568a747c2681d11f87df88a149f984a6d3b4c81f391859afbeb68864ef7f6a3d8779f74a58e3de701b3d51f78e498682e
86d4cf42d9 Increase the ip address relay branching factor for unreachable networks (Pieter Wuille)
Pull request description:
Onion addresses propagate very badly among the IPv4/IPv6 network, resulting
in difficulty for those to find each other.
The branching factor 1 is probably so low that propagations die out before
they reach another onion peer. Increase it to 1.5 on average.
ACKs for top commit:
practicalswift:
ACK 86d4cf42d9 -- patch looks correct
naumenkogs:
ACK 86d4cf4
jonatack:
ACK 86d4cf42d9. Code review, built and running with some sanity check logging. `RelayAddress()` is called by `ProcessMessage() ADDR` msg handling, from within the loop while processing each new address to relay it to a limited number of other nodes. According to git blame, the line setting `nRelayNodes` hasn't been touched since 2016 in e736772c56 *Move network-msg-processing code out of main to its own file*, which moved the line but otherwise did not change it. Running a mixed clearnet/onion node with this patch and the logging below, I'm only seeing values of `fReachable 1, nRelayNodes 2`. IIUC, I need to use the settings in `init.cpp` that call `SetReachable(*, false)`. *Edit:* with `onlynet=onion` am now seeing entries of `fReachable 0` with `nRelayNodes` values of 1 and 2.
vasild:
ACK 86d4cf42d
Tree-SHA512: 22391e16d60bcfdec9a9336728da39d68a24a183b3d1b0e8fbc038d265ca6ddf71d16db018f3678745fd9f3e9281049e42197fa0a29124833c50a9170ed6f793
The else clause is dead code because the only way to not enter the if branch is
if TX_WITNESS_STRIPPED is true. In that case, it would not have a witness to
match the `tx.HasWitness()` else condition.
Co-authored-by: Adam Jonas <jonas@chaincode.com>
Co-authored-by: John Newbery <john@johnnewbery.com>
When I originally implemented the unbroadcast set in 18038, it just tracked
txids. After 18038 was merged, I offered a patch to 18044 to make the
unbroadcast changes compatible with wtxid relay. In this patch, I updated
`unbroadcast_txids` to a map of txid -> wtxid. Post merge review comments shed
light on the fact that this update was unnecessary, and distracting. So, this
commit updates the unbroadcast ids back to a set.
Previously we deduced it was a block-relay-only based on presence of the
m_tx_relay structure. Now we have the ability to identify it directly via a
connection type accessor function.
We previously identified if we relay addresses to the connection by checking
for the existence of the m_addr_known data structure. With this commit, we
answer this question based on the connection type.
IsAddrRelayPeer() checked for the existence of the m_addr_known
fb56d37612 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385e test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01ea p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453b p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd6642167 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b86 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618ca [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc9445 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f0 p2p: add CInv block message helper methods (Jon Atack)
Pull request description:
Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with `-w` to ignore spacing.
Co-authored by John Newbery.
ACKs for top commit:
laanwj:
Code review ACK fb56d37612
jnewbery:
utACK fb56d37612
vasild:
ACK fb56d3761
Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
ea74e10acf doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743fe7 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d171e Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150857 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55a72 Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)
Pull request description:
On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.
On master (65e4ecabd5) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:
```diff
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -607,7 +607,7 @@ public:
void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
- void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
+ void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
```
Clang compiles the code without any thread safety warnings.
See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR.
ACKs for top commit:
MarcoFalke:
ACK ea74e10acf 🎙
jnewbery:
ACK ea74e10acf
ajtowns:
ACK ea74e10acf
Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
This change prepares for upcoming commit "Do not hide compile-time
thread safety warnings" by replacing AssertLockHeld() with
LockAssertion() where needed.
8e35bf5906 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e673 [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159ac8 [net processing] Add Peer (John Newbery)
aba03359a6 [net processing] Remove CNodeState.name (John Newbery)
Pull request description:
We currently have two structures for per-peer data:
- `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
- `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.
This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually).
`Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount.
This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances.
For more motivation see #19398
ACKs for top commit:
laanwj:
Code review ACK 8e35bf5906
troygiorshev:
reACK 8e35bf5906 via `git range-diff master 9510938 8e35bf5`
theuni:
ACK 8e35bf5906.
jonatack:
ACK 8e35bf5906 keeping in mind Cory's comment (https://github.com/bitcoin/bitcoin/pull/19607#discussion_r470173964) for the follow-up
Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
and otherwise log that an unknown INV type was received.
In INV processing, when handling transaction type inv messages,
ToGenTxid() expects that we constructed the CInv ourselves or
that we verified that it is for a transaction type CInv.
Therefore, change this `else` branch into an `else if (inv.GenMsgTx())`
to make this safer and log any INVs that fall through.
The nFetchFlags code can be removed here because GetFetchFlags() can only add
the MSG_WITNESS_FLAG, which is added to the CInv::type field. That CInv is only
passed to AlreadyHave() or ToGenTxid(), and neither of those functions do
anything different depending on whether the CInv type is MSG_TX or
MSG_WITNESS_TX.
Co-authored by: John Newbery <john@johnnewbery.com>
daed542a12 [net_processing] Move ProcessMessage to PeerLogicValidation (John Newbery)
c556770b5e [net_processing] Change PeerLogicValidation to hold a connman reference (John Newbery)
Pull request description:
Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.
ACKs for top commit:
jonatack:
ACK daed542a12 code review and debug tested
promag:
Code review ACK daed542a12.
MarcoFalke:
re-ACK daed542a12, only change is removing second commit 🎴
theStack:
Code Review ACK daed542a12
Tree-SHA512: ddebf410d114d9ad5a9e536950018ff333a347c035d74fcc101fb4a3f20a281782c7eac2b7d1bd1c8f6bc7e59f5b5630fb52c2e1b4c32df454fa584673bd021e
Onion addresses propagate very badly among the IPv4/IPv6 network, resulting
in difficulty for those to find each other.
The branching factor 1 is probably so low that propagations die out before
they reach another onion peer. Increase it to 1.5 on average.
f5c003d3ea [test] Add test for NODE_COMPACT_FILTER. (Jim Posen)
132b30d9c8 [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. (Jim Posen)
b3fbc94d4f Apply cfilters review fixups (John Newbery)
Pull request description:
If -peerblockfilters is configured, signal the `NODE_COMPACT_FILTERS` service bit to indicate that we are able to serve compact block filters, headers and checkpoints.
ACKs for top commit:
MarcoFalke:
re-review and Concept ACK f5c003d3ea
fjahr:
Code review ACK f5c003d3ea
clarkmoody:
Concept ACK f5c003d3ea
ariard:
Concept and Code Review ACK f5c003d
jonatack:
ACK f5c003d3e
Tree-SHA512: 34d1c153530a0e55d09046fe548c9dc37344b5d6d50e00af1b4e1de1e7b49de770fca8471346a17c151de9fe164776296bb3dd5af331977f0c3ef1e6fc906f85
Hold a reference to connman rather than a pointer because:
- PeerLogicValidation can't run without a connman
- The pointer never gets reseated
The alternative is to always assert that the pointer is non-null before
dereferencing.
Change the name from connman to m_connman at the same time to conform
with current style guidelines.
37a480e0cd [net] Add addpeeraddress RPC method (John Newbery)
ae8051bbd8 [test] Test that getnodeaddresses() can return all known addresses (John Newbery)
f26502e9fc [addrman] Specify max addresses and pct when calling GetAddresses() (John Newbery)
Pull request description:
Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to `GetAddr()`. Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, `getnodeaddresses` can now return the complete addrman, which is helpful for testing and monitoring.
Also add a test-only RPC `addpeeraddress`, which adds an IP address:port to addrman. This is helpful for testing (eg #18991).
ACKs for top commit:
naumenkogs:
utACK 37a480e0cd
laanwj:
Code review and lightly manually tested ACK 37a480e0cd
Tree-SHA512: f86dcd410aaebaf6e9ca18ce6f23556e5e4649c1325577213d873aa09967298e65ab2dc19a72670641ae92211a923afda1fe124a82e9d2c1cad73d478ef27fdc
Misbehavior tracking state is now contained in Peer instead of
CNode. It is no longer guarded by cs_main, but instead by a
dedicated m_misbehavior_mutex lock.
This allows us to remove 14 cs_main locks from net_processing.
CAddrMan.GetAddr() would previously limit the number and percentage of
addresses returned (to ADDRMAN_GETADDR_MAX (1000) and
ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers
responsibility to specify the maximum addresses and percentage they want
returned.
For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and
MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the
client.
01e283068b [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b67 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc4 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df629 [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b0 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee3 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e9 [doc] Describe different connection types (Amiti Uttarwar)
442abae2ba [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb052 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812d [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e9 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)
Pull request description:
**This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.
**This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.
This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
(some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)
Overview of this PR:
* rename `oneshot` to `addrfetch`
* introduce `ConnectionType` enum
* one by one, add different connection types to the enum
* expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
* fix the bug in counting different type of connections
* some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.
ACKs for top commit:
jnewbery:
utACK 01e283068b
laanwj:
Code review ACK 01e283068b, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
vasild:
ACK 01e283068
sdaftuar:
ACK 01e283068b.
fanquake:
ACK 01e283068b - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
jb55:
wow this code was messy before... ACK 01e283068b
Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
4c0731f9c5 Deduplicate missing parents of orphan transactions (Suhas Daftuar)
8196176243 Rewrite parent txid loop of requested transactions (Suhas Daftuar)
Pull request description:
I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter.
This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan.
This addresses a couple of [related](https://github.com/bitcoin/bitcoin/pull/19109#discussion_r455197217) [comments](https://github.com/bitcoin/bitcoin/pull/19109#discussion_r456820373) left in #19109.
ACKs for top commit:
laanwj:
Code review ACK 4c0731f9c5
jonatack:
ACK 4c0731f9c5
ajtowns:
ACK 4c0731f9c5
Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
Extract logic that check multiple connection types into interface functions &
structure as switch statements. This makes it very clear what touch points are
for accessing `m_conn_type` & using the switch statements enables the compiler
to warn if a new connection type is introduced but not handled for these cases.
9f88ded82b test addition of unknown segwit spends to txid reject filter (Gregory Sanders)
7989901c7e Add txids with non-standard inputs to reject filter (Suhas Daftuar)
Pull request description:
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
ACKs for top commit:
ajtowns:
ACK 9f88ded82b - code review
jnewbery:
Code review ACK 9f88ded82b
ariard:
Code Review/Tested ACK 9f88ded
naumenkogs:
utACK 9f88ded82b
jonatack:
ACK 9f88ded82b
Tree-SHA512: 1e93c0a5b68cb432524780ffc0093db893911fdfed9e2ed17f888e59114cc75d2a07062aefad4e5ce2e87c9270886117a8abb3c78fb889c9b9f31967f1777148
In the logic for requesting missing parents of orphan transactions, parent
transactions with multiple outputs being spent by the given orphan were being
processed multiple times. Fix this by deduplicating the set of missing parent
txids first.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Previously, we would potentially add the same txid many times to the rolling
bloom filter of recently announced transactions to a peer, if many outputs of
the same txid appeared as inputs in a transaction. Eliminate this problem and
avoid redundant lookups by asking the mempool for the unique parents of a
requested transaction.
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
3bd67ba5a4 Test addr response caching (Gleb Naumenko)
cf1569e074 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43 Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b Move filtering banned addrs inside GetAddresses() (Gleb Naumenko)
Pull request description:
This is a very simple code change with a big p2p privacy benefit.
It’s currently trivial to scrape any reachable node’s AddrMan (a database of all nodes known to them along with the timestamps).
We do have a limit of one GETADDR per connection, but a spy can disconnect and reconnect even from the same IP, and send GETADDR again and again.
Since we respond with 1,000 random records at most, depending on the AddrMan size it takes probably up to 100 requests for an spy to make sure they scraped (almost) everything.
I even have a script for that. It is totally doable within couple minutes.
Then, with some extra protocol knowledge a spy can infer the direct peers of the victim, and other topological stuff.
I suggest to cache responses to GETADDR on a daily basis, so that an attacker gets at most 1,000 records per day, and can’t track the changes in real time. I will be following up with more improvements to addr relay privacy, but this one alone is a very effective. And simple!
I doubt any of the real software does *reconnect to get new addrs from a given peer*, so we shouldn’t be cutting anyone.
I also believe it doesn’t have any negative implications on the overall topology quality. And the records being “outdated” for at most a day doesn’t break any honest assumptions either.
ACKs for top commit:
jnewbery:
reACK 3bd67ba5a4
promag:
Code review ACK 3bd67ba5a4.
ariard:
Code Review ACK 3bd67ba
Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f
c251d710a4 p2p, refactoring: use CInv helpers in net_processing.cpp (Jon Atack)
4254cd9f8f p2p: add CInv transaction message helper methods (Jon Atack)
Pull request description:
Following the merge of wtxid relay in #18044, this is the first of three refactoring PRs (this one, #19610, and #19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation:
- add `CInv` transaction message helper methods, defined in the class
- use the new helpers in `net_processing.cpp` to simplify the code and improve encapsulation
Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) `p2p_segwit`, `p2p_tx_download`, `p2p_feefilter`, and `p2p_permissions`.
ACKs for top commit:
fjahr:
Code review ACK c251d710a4
laanwj:
Code review ACK c251d710a4
vasild:
ACK c251d71
theStack:
Code-Review ACK c251d710a4
hebasto:
ACK c251d710a4, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: ead034b3c9e438909b4c5010c570d7930e69063c114290b051b7cebfd9bd5b19f573218bebe8a521256d32e830797f997adad3d85b4539c64ac5762b698e656d
- Make const things const.
- Replace conditional return with assert.
- Don't log the peer's IP address.
- Log the name Misbehaving directly instead of relying on __func__.
Misbehaving() could optionally take a debug string for printing to the
log file. Make this mandatory and always provide the string.
A couple of additional minor changes:
- remove the unnecessary forward declaration of Misbehaving()
- don't include the nodeid or newline in the passed debug message.
Misbehaving() adds these itself.
655b195747 [net processing] Continue SendMessages processing if not disconnecting peer (John Newbery)
a49781e56d [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages (John Newbery)
a1d5a428a2 [net processing] Fix bad indentation in SendMessages() (John Newbery)
1a1c23f8d4 [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages() (John Newbery)
Pull request description:
The motivation for this PR is to reduce the scope of cs_main locking in misbehavior logic. It is the first set of commits from a larger branch to move the misbehavior data out of CNodeState and into a new struct that doesn't take cs_main.
There are some very minor behavior changes in this branch, such as:
- Not checking for discouragement/disconnect in `ProcessMessages()` (and instead relying on the following check in `SendMessages()`)
- Checking for discouragement/disconnect as the first action in `SendMessages()` (and not doing ping message sending first)
- Continuing through `SendMessages()` if `MaybeDiscourageAndDisconnect()` doesn't disconnect the peer (rather than dropping out of `SendMessages()`
ACKs for top commit:
jonatack:
re-ACK 655b195 per `git range-diff 505b4ed f54af5e 655b195`, code/commit messages review, a bit of code history, and debug build.
MarcoFalke:
ACK 655b195747 only some style-nits 🚁
promag:
Code review ACK 655b195747.
ariard:
Code Review ACK 655b195
Tree-SHA512: fd6d7bc6bb789f5fb7771fb6a45f61a8faba32af93b766554f562144f9631d15c9cc849a383e71743ef73e610b4ee14853666f6fbf08a3ae35176d48c76c65d3
Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.
However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.
Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
Using both txid and wtxid-based relay with peers means that we could sometimes
download the same transaction twice, if announced via two different hashes from
different peers.
Use a heuristic of delaying txid-peer-getdata requests by 2 seconds, if we have
at least one wtxid-based peer.
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
Previously, we only added txids to recentRejects if we were sure that the
transaction couldn't have had the wrong witness (either because the witness was
malleated or stripped).
In preparation for wtxid-based relay, we can observe that txid == wtxid for
transactions that have no witness, and add the wtxid of rejected transactions,
provided the transaction wasn't a witness-stripped one. This means that we now
add more data to the filter (as prior to this commit, any transaction with a
witness that failed to be accepted was being skipped for inclusion in the
filter) but witness malleation should still not interfere with relay of a valid
segwit transaction, because the txid of a segwit transaction would not be added
to the filter after failing validation.
In the future, having wtxids in the recent rejects filter will allow us to
skip downloading the same wtxid multiple times, once our peers use wtxids for
transaction relay.
This is in preparation for wtxid-based invs (we need to be able to tell whether
we AlreadyHave() a transaction based on either txid or wtxid).
This also double the size of the bloom filter, which is overkill, but still
uses a manageable amount of memory.
0c8461a88e refactor: replace CConnman pointers by references in net_processing.cpp (Sebastian Falbesoner)
Pull request description:
This is a follow-up to the recently merged PR https://github.com/bitcoin/bitcoin/pull/19053, replacing ~~two more types of~~ one more type of pointer (CConnman) by references to increase the code quality -- pointers should either check for `nullptr` or be replaced by references, and the latter strategy seems to be more reasonable.
Again, to keep the review burden managable, the changes are kept simple,
* only tackling `CConnman*` ~~and `BanMan*`~~ pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeping the names of the variables as they are
ACKs for top commit:
jnewbery:
utACK 0c8461a88e
MarcoFalke:
ACK 0c8461a88e🕧
Tree-SHA512: 79dc05144bcfb5e0bbc62180285aadcc6199f044fa3016c0f54f7b7f45037415260970037bd63b18fafefb8aef448549dae14b780bafb540fa2373f493a17f71
fa9f20b647 log: Properly log txs rejected from mempool (MarcoFalke)
Pull request description:
Currently `CheckTxInputs` rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues:
* A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user.
* A rejected p2p transaction will log the failure twice. Once with the `MEMPOOLREJ` flag, and once unconditionally.
* A rejected orphan transaction will log no failure.
Fix all issues by simply returning the state to the caller, like it is done for all other rejections.
The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review.
ACKs for top commit:
naumenkogs:
utACK fa9f20b647
rajarshimaitra:
Concept ACK. Compiled and ran tests. `fa9f20b`
jnewbery:
code review ACK fa9f20b647
Tree-SHA512: 86cc17b2a9239c01c4fc3f254ad48ee1d3883266966b9811030176338b9ac3deaea7ea5babfb8bbf739d7440154e30011fede8f9313175f199d4a062af6494f7
Although we currently don't do this, it should be possible to create a
CConnman or PeerLogicValidation without a Banman instance. Therefore
always check that banman exists before dereferencing the pointer.
Also add comments to the m_banman members of CConnman and
PeerLogicValidation to document that these may be nullptr.
f32c408f3a Make sure unconfirmed parents are requestable (Pieter Wuille)
c4626bcd21 Drop setInventoryTxToSend based filtering (Pieter Wuille)
43f02ccbff Only respond to requests for recently announced transactions (Pieter Wuille)
b24a17f039 Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille)
a9bc563803 Swap relay pool and mempool lookup (Pieter Wuille)
Pull request description:
This implements the follow-up suggested here: https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627630111 . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15.
This:
* Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627627010).
* Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see https://github.com/bitcoin/bitcoin/pull/18861#discussion_r419695831).
It adds 37 KiB of filter per peer.
This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see #17303), but that is still blocked by dealing properly with NOTFOUNDs (see #18238).
ACKs for top commit:
jnewbery:
reACK f32c408f3
jonatack:
re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review.
ajtowns:
re-ACK f32c408f3a
Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
fab5586122 doc: Use precise permission flags where possible (MarcoFalke)
Pull request description:
Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.
This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.
Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.
ACKs for top commit:
jnewbery:
reACK fab5586122
fjahr:
Code review ACK fab5586122
jonatack:
ACK fab5586122
Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
If we don't disconnect a peer in MaybeDiscourageAndDisconnect because it
has NOBAN permissions or it's a manual connection, continue SendMessages
processing rather than exiting early.
The previous behaviour was that we'd miss the SendMessages processing on
this iteration of the MessageHandler loop. That's not a problem since
SendMessages() would just be called again on the next iteration, but it
was slightly inefficient and confusing.
`nMisbehavior` is a tally in `CNodeState` that can be incremented from
anywhere. That almost always happens inside a `ProcessMessages()` call
(because we increment the misbehavior score when receiving a bad
messages from a peer), but not always. See, for example, the call to
`MaybePunishNodeForBlock()` inside `BlockChecked()`, which is an
asynchronous callback from the validation interface, executed on the
scheduler thread.
As long as `MaybeDiscourageAndDisconnect()` is called regularly for the
node, then the misbehavior score exceeding the 100 threshold will
eventually result in the peer being punished. It doesn't really matter
where that `MaybeDiscourageAndDisconnect()` happens, but it makes most
sense in `SendMessages()` which is where we do general peer
housekeeping/maintenance.
Therefore, remove the `MaybeDiscourageAndDisconnect()` call in
`ProcessMessages()` and move the `MaybeDiscourageAndDisconnect()` call
in `SendMessages()` to the top of the function. This moves it out of the
cs_main lock scope, so take that lock directly inside
`MaybeDiscourageAndDisconnect()`.
Historic note: `MaybeDiscourageAndDisconnect()` was previously
`SendRejectsAndCheckIfBanned()`, and before that was just sending
rejects. All of those things required cs_main, which is why
`MaybeDiscourageAndDisconnect()` was called after the ping logic.
This was changed to TRY_LOCK in #1117 to fix a potential deadlock
between cs_main and cs_vSend. cs_vSend was split into cs_vSend and
cs_sendProcessing in #9535 (and cs_sendProcessing was changed from a
TRY_LOCK to a LOCK in the same PR).
Since cs_vSend can no longer be taken before cs_main, revert this to a
LOCK().
This commit leaves part of the code with bad indentation. That is fixed
by the next (whitespace change only) commit.
57b0c0a93a Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater (Ben Woosley)
Pull request description:
We do not connect to peers older than 31800
ACKs for top commit:
sipa:
Code reivew ACK 57b0c0a93a
jnewbery:
Code review ACK 57b0c0a93a
vasild:
ACK 57b0c0a9
Tree-SHA512: e1ca7c9203cbad83ab7c7a2312777ad07ed6a16119169b256648b8a8738c260a5168acdd4fb33f6e4b17f51ec7e033e110b76bde55b4e3b2d444dc02c01bc2b1
fa3365430c net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2f util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
... unless they're UNCONDITIONAL_RELAY_DELAY old, or there has been
a response to a MEMPOOL request in the mean time.
This is accomplished using a rolling Bloom filter for the last
3500 announced transactions. The probability of seeing more than 100
broadcast events (which can be up to 35 txids each) in 2 minutes for
an outbound peer (where the average frequency is one per minute), is
less than 1 in a million.
This constant is set to 2 minutes, rather than 15. This is still many times
larger than the transaction broadcast interval (2s for outbound, 5s for
inbound), so it should be acceptable for peers to know what our contents of
the mempool was that long ago.
e8a2822119 [net] Don't try to take cs_inventory before deleting CNode (John Newbery)
3556227ddd [net] Make cs_inventory a non-recursive mutex (John Newbery)
344e831de5 [net processing] Remove PushBlockInventory and PushBlockHash (John Newbery)
Pull request description:
- Remove PushBlockInventory() and PushBlockHash(). These are one-line functions that can easy be inlined into the calling code. Doing so also allows us to eliminate the one place that cs_inventory is recursively locked.
- Make cs_inventory a nonrecursive mutex
- Remove a redundant TRY_LOCK of cs_inventory when deleting CNode.
ACKs for top commit:
sipa:
utACK e8a2822119
MarcoFalke:
ACK e8a2822119🍬
hebasto:
re-ACK e8a2822119
Tree-SHA512: dbc721d102cdef7b5827a8f2549daf8b54f543050266999a7ea56c9f36618565b71e31ce0beb1209ba2db43d15388be173355a03fb6db8ad24e2475b145050bd
2ad58381ff Clean up separated ban/discourage interface (Pieter Wuille)
b691f2df5f Replace automatic bans with discouragement filter (Pieter Wuille)
Pull request description:
This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They're already not actually banned since #14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full.
Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn't persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to "discouraged" to better reflect reality.
ACKs for top commit:
naumenkogs:
utACK 2ad58381ff
amitiuttarwar:
code review ACK 2ad58381ff
jonatack:
ACK 2ad5838 per changes since last review `git range-diff 3276c14 1f7e0ca 2ad5838`
jnewbery:
Code review ACK 2ad58381ff
Tree-SHA512: 5dedef401d9cbfa026812651303e6286223563dbeed7a10766ed536ac9e3f29ed4bd0df29cc6deadceeb35cbe9f066346add14ef0833958ca9f93d123fe7aab5
fab80fef61 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701ad util: Add Assert identity function (MarcoFalke)
fa457fbd33 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61.
ryanofsky:
Code review ACK fab80fef61
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.
Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.
Contains release notes and several interface improvements by
John Newbery.
fa525e4d1c net: Avoid wasting inv traffic during IBD (MarcoFalke)
fa06d7e934 refactor: block import implies IsInitialBlockDownload (MarcoFalke)
faba65e696 Add ChainstateManager::ActiveChainstate (MarcoFalke)
fabf3d64ff test: Add FeeFilterRounder test (MarcoFalke)
Pull request description:
Tx-inv messages are ignored during IBD, so it would be nice if we told peers to not send them in the first place. Do that by sending two `feefilter` messages: One when the connection is made (and the node is in IBD), and another one when the node leaves IBD.
ACKs for top commit:
jamesob:
ACK fa525e4d1c ([`jamesob/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d`](https://github.com/jamesob/bitcoin/tree/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d))
naumenkogs:
utACK fa525e4
gzhao408:
ACK fa525e4d1c
jonatack:
re-ACK fa525e4 checked diff `git range-diff 19612ca fa8a66c fa525e4`, re-reviewed, ran tests, ran a custom p2p IBD behavior test at 9321e0f223.
hebasto:
re-ACK fa525e4d1c, only rebased since the [previous](https://github.com/bitcoin/bitcoin/pull/19204#pullrequestreview-429519667) review (verified with `git range-diff`).
Tree-SHA512: 2c22a5def9822396fca45d808b165b636f1143c4bdb2eaa5c7e977f1f18e8b10c86d4c180da488def38416cf3076a26de15014dfd4d86b2a7e5af88c74afb8eb
56010f9256 test: hoist p2p values to test framework constants (Jon Atack)
75447f0893 test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack)
57960192a5 test: refactor test_large_inv() into 3 tests with common method (Jon Atack)
e2b21d8a59 test: add p2p_invalid_messages logging (Jon Atack)
9fa494dc09 net: update misbehavior logging for oversized messages (Jon Atack)
Pull request description:
...seen while reviewing #19264, #19252, #19304 and #19107:
in `net_processing.cpp`
- make the debug logging for oversized message size misbehavior the same for `addr`, `getdata`, `headers` and `inv` messages
in `p2p_invalid_messages`
- add missing logging
- improve assertions/message sends, move cleanup disconnections outside the assertion scopes
- split a slowish 3-part test into 3 order-independent tests
- add a few p2p constants to the test framework
ACKs for top commit:
troygiorshev:
reACK 56010f9256
MarcoFalke:
ACK 56010f9256 🎛
Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310
PushBlockInventory() and PushBlockHash() are functions that can
be replaced with single-line statements. This also eliminates
the single place that cs_inventory is taken recursively.
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
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().
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.
-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
If -peerblockfilters is configured, signal the NODE_COMPACT_FILTERS service
bit to indicate that we are able to serve compact block filters, headers
and checkpoints.
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)
Pull request description:
Support `getcfilters` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
Empact:
re-Code Review ACK 9e36067d8c
MarcoFalke:
re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
jkczyz:
ACK 9e36067d8c
fjahr:
Code review ACK 9e36067d8c
Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
5308c97cca [test] Add test for cfheaders (Jim Posen)
f6b58c1506 [net processing] Message handling for getcfheaders. (Jim Posen)
3bdc7c2d39 [doc] Add comment for m_headers_cache (John Newbery)
Pull request description:
Support `getcfheaders` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
jkczyz:
ACK 5308c97cca
MarcoFalke:
re-ACK 5308c97cca , only change is doc related 🗂
theStack:
ACK 5308c97cca🚀
Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7
fab6b9d18f validation: Mark g_chainman DEPRECATED (MarcoFalke)
fa1d97b256 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke)
fa24d49098 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke)
fa84b1cd84 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke)
fa05fdf0f1 net: Pass chainman into PeerLogicValidation (MarcoFalke)
fa7b626d7a node: Add chainman alias for g_chainman (MarcoFalke)
Pull request description:
The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.
The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.
I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.
ACKs for top commit:
ryanofsky:
Code review ACK fab6b9d18f. Had to be rebased but still looks good
Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f
amitiuttarwar:
ACK 651f1d816f🎉
MarcoFalke:
Review ACK 651f1d816f
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
0187d4c118 [indexes] Add compact block filter headers cache (John Newbery)
Pull request description:
Cache block filter headers at heights of multiples of 1000 in memory.
Block filter headers at height 1000x are checkpointed, and will be the most frequently requested. Cache them in memory to avoid costly disk reads.
ACKs for top commit:
jkczyz:
ACK 0187d4c118
theStack:
ACK 0187d4c118🎉
fjahr:
re-utACK 0187d4c118
laanwj:
code review ACK 0187d4c118
ariard:
Code Review ACK 0187d4c.
Tree-SHA512: 2075ae36901ebcdc4a217eae5203ebc8582181a0831fb7a53a119f031c46bca960a610a38a3d0636a9a405f713efcf4200c85f10c8559fd80139036d89473c56
0ea5d70b47 Updated comment for the condition where a transaction relay is denied (glowang)
be01449cc8 Add test for param interaction b/w -blocksonly and -whitelistforcerelay (glowang)
Pull request description:
Related to: #18428
When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.
ACKs for top commit:
MarcoFalke:
ACK 0ea5d70b47
Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
- before reattempting broadcast for unbroadcast txns, check they are in mempool and remove if not
- this protects from memory leaks and network spam just in case unbroadcast set (incorrectly) has extra txns
- check that tx is in mempool before adding to unbroadcast set to try to prevent this from happening
2896c412fa Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3dee Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf407 Abstract logic to determine whether to answer tx GETDATA (Pieter Wuille)
Pull request description:
This PR intends to improve transaction-origin privacy.
In general, we should try to not leak information about what transactions we have (recently) learned about before deciding to announce them to our peers. There is a controlled transaction dissemination process that reveals our transactions to peers that has various safeguards for privacy (it's rate-limited, delayed & batched, deterministically sorted, ...), and ideally there is no way to test which transactions we have before that controlled process reveals them. The handling of the `mempool` BIP35 message has protections in this regard as well, as it would be an obvious way to bypass these protections (handled asynchronously after a delay, also deterministically sorted).
However, currently, if we receive a GETDATA for a transaction that we have not yet announced to the requester, we will still respond to it if it was announced to *some* other peer already (because it needs to be in `mapRelay`, which only happens on the first announcement). This is a slight privacy leak.
Thankfully, this seems easy to solve: `setInventontoryTxToSend` keeps track of the txids we have yet to announce to a peer - which almost(*) exactly corresponds to the transactions we know of that we haven't revealed to that peer. By checking whether a txid is in that set before responding to a GETDATA, we can filter these out.
(*) Locally resubmitted or rebroadcasted transactions may end up in setInventoryTxToSend while the peer already knows we have them, which could result in us incorrectly claiming we don't have such transactions if coincidentally requested right after we schedule reannouncing them, but before they're actually INVed. This is made even harder by the fact that filterInventoryKnown will generally keep known reannouncements out of setInventoryTxToSend unless it overflows (which needs 50000 INVs in either direction before it happens).
The condition for responding now becomes:
```
(not in setInventoryTxToSend) AND
(
(in relay map) OR
(
(in mempool) AND
(old enough that it could have expired from relay map) AND
(older than our last getmempool response)
)
)
```
ACKs for top commit:
naumenkogs:
utACK 2896c41
ajtowns:
ACK 2896c412fa
amitiuttarwar:
code review ACK 2896c412fa
jonatack:
ACK 2896c412fa per `git diff 2b3f101 2896c41` only change since previous review is moving the recency check up to be verified first in `FindTxForGetData`, as it was originally in 353a391 (good catch), before looking up the transaction in the relay pool.
jnewbery:
code review ACK 2896c412fa
Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868
Cache block filter headers at heights of multiples of 1000 in memory.
Block filter headers at height 1000x are checkpointed, and will be the
most frequently requested. Cache them in memory to avoid costly disk
reads.
Headers-first is the primary method of announcement on the network. If a
node fell back sending blocks by inv, it's probably for a re-org. The
final block hash provided should be the highest, so send a getheaders
and then fetch the blocks we need to catch up.
23083856a5 [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25a [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e [init] Add -peerblockfilters option (Jim Posen)
Pull request description:
Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.
`NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.
ACKs for top commit:
jonatack:
Code review re-ACK 23083856a5 the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
fjahr:
re-ACK 23083856a5
jkczyz:
re-ACK 23083856a5
ariard:
re-Code Review ACK 2308385
clarkmoody:
Tested ACK 23083856a
MarcoFalke:
re-ACK 23083856a5🌳
theStack:
ACK 23083856a5
Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
9847e205bf [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e0 [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c8 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
Pull request description:
Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.
ACKs for top commit:
sipa:
utACK 9847e205bf
brakmic:
ACK 9847e205bf
luke-jr:
utACK 9847e205bf
naumenkogs:
utACK 9847e20
ajtowns:
utACK 9847e205bf
Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
1ad8ea2b73 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)
Pull request description:
The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
> the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)
For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).
The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.
ACKs for top commit:
meshcollider:
utACK 1ad8ea2b73
laanwj:
Concept and code review ACK 1ad8ea2b73
jkczyz:
ACK 1ad8ea2b73
MarcoFalke:
ACK 1ad8ea2b73
fjahr:
Code review ACK 1ad8ea2b73
Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
from a peer, or if the transaction is removed from the mempool.
a9ecbdfcaa test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)
Pull request description:
This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:
c0b389b335/src/net.h (L812)
and after a loaded filter is removed again through a `filterclear` message:
c0b389b335/src/net_processing.cpp (L3201)
The behaviour was introduced by commit 37c6389c5a (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).
This default match-everything filter leads to some unintended side-effects:
1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) c0b389b335/src/net_processing.cpp (L1504-L1507)
2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) c0b389b335/src/net_processing.cpp (L3182-L3186)
This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.
Here is a before/after comparison in behaviour:
| code part / scenario | master branch | PR branch |
| --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
| `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload` |
| `filteradd` processing, no filter was loaded | nothing | peer's banscore increases by 100 (i.e. disconnect) |
On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.
ACKs for top commit:
MarcoFalke:
re-ACK a9ecbdfcaa, only change is in test code 🕙
Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
fa1da3d4bf test: Add basic addr relay test (MarcoFalke)
fa1793c1c4 net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003 net: Make addr relay mockable (MarcoFalke)
Pull request description:
As usual:
* Switch to std::chrono time to be type-safe and mockable
* Add basic test that relies on mocktime to add code coverage
ACKs for top commit:
naumenkogs:
utACK fa1da3d
promag:
ACK fa1da3d4bf (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.
Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
Previously, a default match-everything bloom filter was set for every peer,
i.e. even before receiving a 'filterload' message and after receiving a
'filterclear' message code branches checking for the existence of the filter
by testing the pointer "pfilter" were _always_ executed.
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda
function to execute later, capturing the local variable
`consensusParams` by reference.
Presumably this was considered safe because `consensusParams` is a
reference itself to a global variable which is not supposed to change,
but it can in tests.
Fixes https://github.com/bitcoin/bitcoin/issues/18372
fa36f3a295 refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke)
fadafb83cf scheduler: Make schedule* methods type safe (MarcoFalke)
fa70ccc6c4 scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke)
Pull request description:
Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}`
ACKs for top commit:
vasild:
ACK fa36f3a (code review, not tested)
ajtowns:
ACK fa36f3a295
jonatack:
ACK fa36f3a
Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
a029e18c2b Use rolling bloom filter of recent block tx's for AlreadyHave() check (Suhas Daftuar)
Pull request description:
In order to determine whether to download or process a relayed transaction, we first try to check whether we already have the transaction -- either in the mempool, in our filter of recently rejected transactions, in our orphan pool, or already confirmed in a block.
Prior to this commit, the heuristic for checking whether a transaction was confirmed in a block is based on whether there's a coin cache entry corresponding to the 0- or 1-index vout of the tx. While that is a quick check, it is very imprecise (eg if those outputs were already spent in another block, we wouldn't detect that the transaction has already been confirmed) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will better capture the case of a transaction which has been confirmed and then fully spent.
This should reduce the bandwidth that we waste by requesting transactions which will not be accepted to the mempool.
To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
ACKs for top commit:
MarcoFalke:
re-ACK a029e18c2b only stylistic and comment fixups 🍴
sipa:
utACK a029e18c2b
jonatack:
Code review ACK a029e18c2b also built/ran tests and am running bitcoind with mempool debug logging and custom logging. Looked a bit into CRollingBloomFilter and also the mempool median time past checks mentioned above; I don't have a deep understanding of those areas yet but the concept here and changes LGTM. Tests and other optimisations could be added as a follow-up. In favor of seeing this move forward if no major immediate concerns.
Tree-SHA512: 784c9a35bcd3af5db469063ac7d26b4bac430e451e5637a34d8a538c3ffd1433abdd3f06e5584e7a84bfa9e791449e61819397b5a6c7890fa59d78ec3ba507b2
In order to determine whether to download or process a relayed transaction, we
try to determine if we already have the transaction, either in the mempool, in
our recently rejected filter, in our orphan pool, or already confirmed in the
chain itself.
Prior to this commit, the heuristic for checking the chain is based on whether
there's an output corresponding to the 0- or 1-index vout in our coin cache.
While that is a quick check, it is very imprecise (say if those outputs were
already spent in a block) -- we can do better by just keeping a rolling bloom
filter of the transactions in recent blocks, which will capture the case of a
transaction which has been confirmed and then fully spent already.
To avoid relay problems for transactions which have been included in a recent
block but then reorged out of the chain, we clear the bloom filter whenever a
block is disconnected.
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
4bdd68f301 Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3dcb6 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
Pull request description:
Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.
ACKs for top commit:
MarcoFalke:
re-ACK 4bdd68f301 (only change is adding includes) 🕕
promag:
ACK 4bdd68f301, could squash.
Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
Remove the forest of special exceptions, and simply log a short
message to the NET logging category when an exception happens during
packet processing. It is not good to panick end users with errors
that any peer can generate (let alone writing to stderr).
1a8f0d5a74 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354f [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74
MarcoFalke:
re-ACK 1a8f0d5a74
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
