We comment out a newly-added assert from the Bitcoin
side, because it's unclear how to replicate it
correctly in Elements.
Co-authored-by: Glenn Willen <gwillen@nerdnet.org>
When receiving unwanted headers from a peer, first check whether we have
recent "best known block" state for that peer. If not, accept one batch
of headers to refresh it. Otherwise, we can become stuck in a state where
we need blocks, but we don't know which peers have those blocks available.
This especially happens during startup, when we don't have this information
for any peers until we start processing header messages.
ab1ea29ba1 refactor: make GetRand a template, remove GetRandInt (pasta)
Pull request description:
makes GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>()
ACKs for top commit:
laanwj:
Code review ACK ab1ea29ba1
Tree-SHA512: db5082a0e21783389f1be898ae73e097b31ab48cab1a2c0e29348a4adeb545d4098193aa72a547c6baa6e8205699aafec38d6a27b3d65522fb3246f91b4daae9
778343a379 scripted-diff: Rename PeerManagerImpl members (dergoegge)
91c339243e [net processing] Move nHighestFastAnnounce into PeerManagerImpl (dergoegge)
10b83e2aa3 [net processing] Move block cache state into PeerManagerImpl (dergoegge)
a4c55a93ef [net processing] Inline and simplify UpdatePreferredDownload (dergoegge)
490c08f96a [net processing] Move nPreferredDownload into PeerManagerImpl (dergoegge)
a292df283a [net processing] Move mapNodeState into PeerManagerImpl (dergoegge)
37ecaf3e7a [net processing] Move CNodeState declaration above PeerManagerImpl (dergoegge)
Pull request description:
This PR moves the remaining net processing globals into `PeerManagerImpl`. This will make testing the peer manager in isolation easier and also acts as a code clean up.
ACKs for top commit:
jnewbery:
Code review ACK 778343a379
MarcoFalke:
ACK 778343a379 🗒
Tree-SHA512: 4f22105d1de37b94c3ef349f38784a30cf8d450d394a6a7849e5bd78940a71e3edbffa3d25e8efb35d7f698fd255f199de7bd4c33e23af5621a6e4e67ed43cb5
2ff8f4dd81 Add tests for addr destination rotation (Gleb Naumenko)
77ccb7fce1 Use std::chrono for salting when randomizing ADDR destination (Gleb Naumenko)
Pull request description:
We currently assign a destination peer for relaying particular addresses of nodes every 24 hours, and then rotate. This is done for rate-limiting (ultimately for privacy leak reduction I think?).
Before this change, 24 hours was defined as uint. I replaced it with std::chrono, which is mockable and type-safe.
Also added couple tests for this behavior.
ACKs for top commit:
jonatack:
ACK 2ff8f4dd81
Tree-SHA512: 16f703ef3ffee13ce3afa82ca7b4baa27308af18cd2eececdce5565badfb68656a2ad9c4594b73772e4bfa99b3fb15f8e4089c1cb4be98c0bae6730a9d2f8a25
3ae7791bca refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bca
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
We inline `UpdatePreferredDownload` because it is only used in one
location during the version handshake. We simplify it by removing the
initial subtraction of `state->fPreferredDownload` from
`nPreferredDownload`. This is ok since the version handshake is only
called once per peer and `state->fPreferredDownload` will always be
false before the newly inlined code is called, making the subtraction a
noop.
1066d10f71 scripted-diff: rename TxRelay members (John Newbery)
575bbd0dea [net processing] Move tx relay data to Peer (John Newbery)
785f55f7ee [net processing] Move m_wtxid_relay to Peer (John Newbery)
36346703f8 [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
dergoegge:
ACK 1066d10f71 - This is a good layer separation improvement with no behavior changes.
glozow:
utACK 1066d10f71
Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc
This makes code that uses the helper less verbose.
Moreover, this makes net_processing C++20 compliant. Otherwise, it would
lead to a compile error (see below). C++20 disables aggregate
initialization when any constructor is declared. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf
net_processing.cpp:1627:42: error: no matching constructor for initialization of 'CSerializedNetMsg'
m_connman.PushMessage(pnode, CSerializedNetMsg{ser_cmpctblock.data, ser_cmpctblock.m_type});
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fa61dd44f9 p2p: Serialize cmpctblock at most once in NewPoWValidBlock (MarcoFalke)
Pull request description:
Instead of serializing for each peer, serialize at most once and copy the raw data for each peer.
ACKs for top commit:
shaavan:
reACK fa61dd44f9
theStack:
Code-review ACK fa61dd44f9
Tree-SHA512: ed029aeaea67fdac8ddb865069f8166bc0dd8480418c405628e3e1a43b61161584a09a1814668bcd220602e8732e188be2bfed9242aa81bdbd92c64c702ed138
Also, remove cs_main guard from m_wtxid_relay_peers and make it atomic.
This should be fine since we don't need m_wtxid_relay_peers to be
synchronized with m_wtxid_relay exactly at all times.
After this change, RelayTransaction no longer requires cs_main.
We'll move the transaction relay data into Peer in subsequent commits,
but the inbound eviction logic needs to know if the peer is relaying
txs and if the peer has loaded a bloom filter.
This is currently redundant information with m_tx_relay->fRelayTxes,
but when m_tx_relay is moved into net_processing, then we'll need these
separate fields in CNode.
faa329fd46 refactor: Release cs_main before MaybeSendFeefilter (MarcoFalke)
Pull request description:
There is no need for any lock to be held, because net processing is single threaded. So holding the validation lock cs_main for sending a feefilter is confusing and might even degrade blockchain-related RPC performance minimally.
ACKs for top commit:
ajtowns:
ACK faa329fd46 ; code review only
vasild:
ACK faa329fd46
Tree-SHA512: 3e7f9faff1631cc64c86fc1a354ada67617ad1e7a046625cc741f4711854eb41ca8aad5a51ef0d94ff65947b68dba8345c9f786b20ee0a8b7a2e8741cfced21f
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
In `PeerManagerImpl::RelayAddress()` we used just the hash of the
address that is being relayed to decide where to relay it to. Include
the port in that hash, so that e.g. `1.1.1.1:5555` and `1.1.1.1:6666`
get relayed to different peers. Those are two different, distinct
services after all.
