3566aa7d49 [net] Remove CNode friends (dergoegge)
3eac5e7cd1 [net] Add CNode helper for send byte accounting (dergoegge)
60441a3432 scripted-diff: [net] Rename CNode process queue members (dergoegge)
6693c499f7 [net] Make cs_vProcessMsg a non-recursive mutex (dergoegge)
23d9352654 [net] Make CNode msg process queue members private (dergoegge)
897e342d6e [net] Encapsulate CNode message polling (dergoegge)
cc5cdf8776 [net] Deduplicate marking received message for processing (dergoegge)
ad44aa5c64 [net] Add connection type getter to CNode (dergoegge)
Pull request description:
We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and its mutex is turned into a non-recursive mutex.
ACKs for top commit:
jnewbery:
utACK 3566aa7d49
vasild:
ACK 3566aa7d49
theStack:
re-ACK 3566aa7d49
brunoerg:
re-ACK 3566aa7d49
Tree-SHA512: 26b87da5054e32401b693b2904e9c5f40e35a53937c0b6cf44b8597034ad07bacf27d87cdffc54d3e7ccfebde4231ef30a38d326f88cc18133bbb34688ead567
c9d548c91f net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e9 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20 net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59a scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91f
achow101:
ACK c9d548c91f
jonatack:
re-ACK c9d548c91f only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91f
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
faf7b4f1fc Add BlockManager::IsPruneMode() (MarcoFalke)
fae71fe27e Add BlockManager::GetPruneTarget() (MarcoFalke)
fa0f0436d8 Add BlockManager::LoadingBlocks() (MarcoFalke)
Pull request description:
Requested in https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1061323795, but adding getters seems unrelated from removing globals, so I split it out for now.
ACKs for top commit:
dergoegge:
Code review ACK faf7b4f1fc
brunoerg:
crACK faf7b4f1fc
Tree-SHA512: 204d0e9a0e8b78175482f89b4ce620fba0e65d8e49ad845d187af44d3843f4c733a01bac1ffe5a5319f524d8346123693a456778b69d6c75268c447eb8839642
39b93649c4 test: add functional test for IBD stalling logic (Martin Zumsande)
0565951f34 p2p: Make block stalling timeout adaptive (Martin Zumsande)
Pull request description:
During IBD, there is the following stalling mechanism if we can't proceed with assigning blocks from a 1024 lookahead window because all of these blocks are either already downloaded or in-flight: We'll mark the peer from which we expect the current block that would allow us to advance our tip (and thereby move the 1024 window ahead) as a possible staller. We then give this peer 2 more seconds to deliver a block (`BLOCK_STALLING_TIMEOUT`) and if it doesn't, disconnect it and assign the critical block we need to another peer.
Now the problem is that this second peer is immediately marked as a potential staller using the same mechanism and given 2 seconds as well - if our own connection is so slow that it simply takes us more than 2 seconds to download this block, that peer will also be disconnected (and so on...), leading to repeated disconnections and no progress in IBD. This has been described in #9213, and I have observed this when doing IBD on slower connections or with Tor - sometimes there would be several minutes without progress, where all we did was disconnect peers and find new ones.
The `2s` stalling timeout was introduced in #4468, when blocks weren't full and before Segwit increased the maximum possible physical size of blocks - so I think it made a lot of sense back then.
But it would be good to revisit this timeout now.
This PR makes the timout adaptive (idea by sipa):
If we disconnect a peer for stalling, we now double the timeout for the next peer (up to a maximum of 64s). If we connect a block, we half it again up to the old value of 2 seconds. That way, peers that are comparatively slower will still get disconnected, but long phases of disconnecting all peers shouldn't happen anymore.
Fixes#9213
ACKs for top commit:
achow101:
ACK 39b93649c4
RandyMcMillan:
Strong Concept ACK 39b93649c4
vasild:
ACK 39b93649c4
naumenkogs:
ACK 39b93649c4
Tree-SHA512: 85bc57093b2fb1d28d7409ed8df5a91543909405907bc129de7c6285d0810dd79bc05219e4d5aefcb55c85512b0ad5bed43a4114a17e46c35b9a3f9a983d5754
fa035fe2d6 Remove unused CDataStream::SetType (MarcoFalke)
fa29e73cda Use DataStream where possible (MarcoFalke)
fa9becfe1c streams: Add DataStream without ser-type and ser-version (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `DataStream`. `CDataStream` remains in places where it is not yet possible.
ACKs for top commit:
stickies-v:
re-ACK [fa035fe](fa035fe2d6)
aureleoules:
diff re-ACK fa035fe2d6fa0e6640ba..fa035fe2d6
Tree-SHA512: cb5e53d0df7c94319ffadc6ea1d887fc38516decaf43f0673396d79cc62d450a1a61173654a91b8c2b52d2cecea53fe4a500b8f6466596f35731471163fb051c
c58c249a5b net_processing: indicate more work to do when orphans are ready to reconsider (Anthony Towns)
ecb0a3e425 net_processing: Don't process tx after processing orphans (Anthony Towns)
c583775706 net_processing: only process orphans before messages (Anthony Towns)
be2304676b txorphange: Drop redundant originator arg from GetTxToReconsider (Anthony Towns)
a4fe09973a txorphanage: index workset by originating peer (Anthony Towns)
Pull request description:
We currently process orphans by assigning them to the peer that provided a missing parent; instead assign them to the peer that provided the orphan in the first place. This prevents a peer from being able to marginally delay another peer's transactions and also simplifies the internal API slightly. Because we're now associating orphan processing with the peer that provided the orphan originally, we no longer process orphans immediately after receiving the parent, but defer until a future call to `ProcessMessage`.
Based on #26295
ACKs for top commit:
naumenkogs:
utACK c58c249a5b
glozow:
ACK c58c249a5b
mzumsande:
Code Review ACK c58c249a5b
Tree-SHA512: 3186c346f21e60440266a2a80a9d23d7b96071414e14b2b3bfe50457c04c18b1eab109c3d8c2a7726a6b10a2eda1f0512510a52c102da112820a26f5d96f12de
When PR#15644 made orphan processing interruptible, it also introduced a
potential 100ms delay between processing of the first and second newly
reconsiderable orphan, because it didn't check if the orphan work set
was non-empty after invoking ProcessMessage(). This adds that check, so
that ProcessMessages() will return true if there are orphans to process,
usually avoiding the 100ms delay in CConnman::ThreadMessageHandler().
If we made progress on orphans, consider that enough work for this peer
for this round of ProcessMessages. This also allows cleaning up the api
for TxOrphange:GetTxToReconsider().
Previously, when we processed a new tx we would attempt to ATMP any
orphans that considered the new tx a parent immediately, but would only
accept at most one such tx, leaving any others to be considered on a
future run of ProcessMessages(). With this patch, we don't attempt any
orphan processing immediately after receiving a tx, instead deferring
all of them until the next call to ProcessMessages().
Both methods do the same thing, so simplify to having just one.
`ToString()` is too generic in this case and it is unclear what it does,
given that there are similar methods:
`ToStringAddr()` (inherited from `CNetAddr`),
`ToStringPort()` and
`ToStringAddrPort()`.
956c67059c refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande)
3c43d9db1e p2p: Don't self-advertise during VERSION processing (Gleb Naumenko)
Pull request description:
This picks up the last commit from #19843.
Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer.
This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after
the version handshake is finished.
There are a couple of differences:
1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones.
2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable.
3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`.
Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`.
ACKs for top commit:
stratospher:
ACK 956c670
naumenkogs:
ACK 956c67059c
amitiuttarwar:
reACK 956c67059c
Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
fa579f3063 refactor: Pass reference to last header, not pointer (MacroFake)
Pull request description:
It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
ACKs for top commit:
john-moffett:
ACK fa579f3
aureleoules:
ACK fa579f3063
Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
38941a703e refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov)
Pull request description:
This PR addresses the https://github.com/bitcoin/bitcoin/pull/17786#discussion_r1027818360:
> why not move it to the right place, that is to `kernel/txmempool_entry.h`?
ACKs for top commit:
MarcoFalke:
review ACK 38941a703e📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
f39d9269eb rpc: warn that nodes ignore requests for old stale blocks (Sjors Provoost)
Pull request description:
Adds warning to RPC help that `getblockfrompeer` is of little use for stale blocks that are more than a month old.
This is an anti-fingerprinting measure. See `BlockRequestAllowed` in `net_processing`.
It's been in Bitcoin Core since 2014, introduced in #2910 and later improved to not rely on checkpoints.
Older and alternative clients might still serve these blocks, so not throwing an error.
Allowing whitelisted nodes to fetch these blocks anyway might be nice.
ACKs for top commit:
fjahr:
Code review ACK f39d9269eb
Tree-SHA512: db88f9f7521289640c5e629c840dda1c2c3ab70d458e9e7136c60fbaeb02acfb36dc093502d83d4c098c331e22aab81bf8f4c4961d805e3bde0f8f3cfe68d968
1984db1d50 refactor: Rename local variable to distinguish it from type alias (Hennadii Stepanov)
Pull request description:
The `txiter` type alias is declared in the `txmempool.h`: 9e59d21fbe/src/txmempool.h (L406)
ACKs for top commit:
stickies-v:
ACK 1984db1d5
vasild:
ACK 1984db1d50
jarolrod:
ACK 1984db1d50
Tree-SHA512: 127bfb62627e2d79d8cdb0bd0ac11b3737568c3631b54b2d1e37984f673a1f60edf7bc102a269f7eb40e4bb124b910b924a89475c6a6ea978b2171219fa30685
8f2dac5409 [test] Add p2p_tx_privacy.py (dergoegge)
ce63fca13e [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
845e3a34c4 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)
Pull request description:
`TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake).
Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node.
ACKs for top commit:
MarcoFalke:
ACK 8f2dac5409🔝
jnewbery:
utACK 8f2dac5409
Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9
This commit documents our assumption about
TxRelay::m_tx_inventory_to_send being empty prior to version handshake
completion.
The added Assume acts as testing oracle for our fuzzing tests to
potentially detect if the assumption is violated.
46339d29b1 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112e p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba21 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e29 p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69c tests: stabilize sendtxrcncl test (Gleb Naumenko)
Pull request description:
Non-trivial changes include:
- Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](https://github.com/bitcoin/bips/pull/1376));
- Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`;
- Don't send `sendtxrcncl` to feeler connections.
ACKs for top commit:
vasild:
ACK 46339d29b1
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b1
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
7082ce3e88 scripted-diff: rename and de-globalise g_cs_orphans (Anthony Towns)
733d85f79c Move all g_cs_orphans locking to txorphanage (Anthony Towns)
a936f41a5d txorphanage: make m_peer_work_set private (Anthony Towns)
3614819864 txorphange: move orphan workset to txorphanage (Anthony Towns)
6f8e442ba6 net_processing: Localise orphan_work_set handling to ProcessOrphanTx (Anthony Towns)
0027174b39 net_processing: move ProcessOrphanTx docs to declaration (Anthony Towns)
9910ed755c net_processing: Pass a Peer& to ProcessOrphanTx (Anthony Towns)
89e2e0da0b net_processing: move extra transactions to msgproc mutex (Anthony Towns)
ff8d44d196 Remove unnecessary includes of txorphange.h (Anthony Towns)
Pull request description:
Moves extra transactions to be under the `m_msgproc_mutex` lock rather than `g_cs_orphans` and refactors orphan handling so that the lock can be internal to the `TxOrphange` class.
ACKs for top commit:
dergoegge:
Code review ACK 7082ce3e88
glozow:
ACK 7082ce3e88 via code review and some [basic testing](https://github.com/glozow/bitcoin/blob/review-26295/src/test/orphanage_tests.cpp#L150). I think putting txorphanage in charge of handling peer work sets is the right direction.
Tree-SHA512: 1ec454c3a69ebd45ff652770d6a55c6b183db71aba4d12639ed70f525f0035e069a81d06e9b65b66e87929c607080a1c5e5dcd2ca91eaa2cf202dc6c02aa6818
This code was a bit hard to understand, so make it less dense and
add more explanations. Doesn't change behavior.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
c8dc0e3eaa refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e5 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa
glozow:
utACK c8dc0e3eaa, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
fa24239a1c net: Avoid SetTxRelay for feeler connections (MacroFake)
Pull request description:
Seems odd to reserve memory for the struct (the heaviest member being `m_tx_inventory_known_filter`) when it is never used.
This also avoids sending out `msg_sendtxrcncl` before disconnecting. This shouldn't matter, as other messages, such as `msg_wtxidrelay`, `msg_sendaddrv2`, `msg_verack` or `msg_getaddr` are still sent. Though, it allows to test the changes here as a side-effect.
ACKs for top commit:
naumenkogs:
ACK fa24239a1c
vasild:
ACK fa24239a1c
jonatack:
ACK fa24239a1c
mzumsande:
ACK fa24239a1c
Tree-SHA512: d7604c7eb4df8f2de811e600bdd312440ee03e508d3a0f09ae79f7f2d3eeec663bfd47a2d079fa50b756d61e35dfa998de068a7b9afaf35378fa0e62a538263d
`m_headers_sync` is already reset in IsContinuationOfLowWorkHeadersSync
if there is a failure, so there is no need to also reset in
TryLowWorkHeaderSync.
aaaa7bd0ba iwyu: Add missing includes (MacroFake)
fa9ebec096 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86 Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099 Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57c Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba
ryanofsky:
Code review ACK aaaa7bd0ba. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
This makes the stalling detection mechanism (previously a fixed
timeout of 2s) adaptive:
If we disconnect a peer for stalling, double the timeout for the
next peer - and let it slowly relax back to its default
value each time the tip advances. (Idea by Pieter Wuille)
This makes situations more unlikely in which we'd keep on
disconnecting many of our peers for stalling, even though our
own bandwidth is insufficient to download a block in 2 seconds.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
7ad15d1100 [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge)
Pull request description:
This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit.
The issue is that we ignore the return value on [the first `IsContinuationOfLowWorkHeadersSync` call after a new headers sync is started](fabc031048/src/net_processing.cpp (L2553-L2568)), which leads to us passing headers to [`ProcessNewBlockHeaders`](fabc031048/src/net_processing.cpp (L2856)) when that initial `IsContinuationOfLowWorkHeadersSync` call returns `false`. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different `nBits` value than the prior headers (which fails the pre-sync logic [here](fabc031048/src/headerssync.cpp (L189))). Those 2000 headers will be passed to `ProcessNewBlockHeaders`.
I haven't included a test here so far because we can't test this without changing the default value for `CRegTestParams::consensus.fPowAllowMinDifficultyBlocks` or doing some more involved refactoring.
ACKs for top commit:
sipa:
ACK 7ad15d1100
glozow:
ACK 7ad15d1100
Tree-SHA512: 9aabb8bf3700401e79863d0accda0befd2a83c4d469a53f97d827e51139e2f826aee08cdfbc8866b311b153f61fdac9b7aa515fcfa2a21c5e2812c2bf3c03664
It is never a nullptr, otherwise an assertion would fire in
UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
dddd1acf58 net: Set relay in version msg to peers with relay permission (MacroFake)
Pull request description:
Seems odd to set the `relay` permission in -blocksonly mode and also ask the peer not to relay transactions.
ACKs for top commit:
dergoegge:
ACK dddd1acf58
naumenkogs:
ACK dddd1acf58
mzumsande:
ACK dddd1acf58
Tree-SHA512: 7bb0e964993ea4982747ae2801fe963ff88586e2ded03015b60ab83172b5b61f2d50e9cde9d7711b7ab207f8639467ecafc4d011ea151ec6c82c722f510f4df7
This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.
We optimistically pre-register a peer for txreconciliations
upon sending txreconciliation support announcement.
But if, at VERACK, we realize that the peer never sent
WTXIDRELAY message, we should unregister the peer
from txreconciliations, because txreconciliations rely on wtxids.
Once we received a reconciliation announcement support
message from a peer and it doesn't violate our protocol,
we store the negotiated parameters which will be used
for future reconciliations.
If we're connecting to the peer which might support
transaction reconciliation, we announce we want to reconcile
with them.
We store the reconciliation salt so that when the peer
responds with their salt, we are able to compute the
full reconciliation salt.
This behavior is enabled with a CLI flag.
a3789c700b Improve getpeerinfo pingtime, minping, and pingwait help docs (Jon Atack)
df660ddb1c Update getpeerinfo/-netinfo/TxRelay#m_relay_txs relaytxes docs (for v24 backport) (Jon Atack)
1f448542e7 Always return getpeerinfo "minfeefilter" field (for v24 backport) (Jon Atack)
9cd6682545 Make getpeerinfo field order consistent with its help (for v24 backport) (Jon Atack)
Pull request description:
Various updates and fixups, mostly targeting v24. Please refer to the commit messages for details.
ACKs for top commit:
achow101:
ACK a3789c700b
brunoerg:
ACK a3789c700b
vasild:
ACK a3789c700b
Tree-SHA512: b8586a9b83c1b18786b5ac1fc1dba91573c13225fc2cfc8d078f4220967c95056354f6be13327f33b4fcf3e9d5310fa4e1bdc93102cbd6574f956698993a54bf
Previously vExtraTxnForCompact and vExtraTxnForCompactIt were protected
by g_cs_orphans; protect them by g_msgproc_mutex instead, as they
are only used during message processing.
Previously, we would prepare to self-announce to a new peer while
parsing a VERSION message from that peer. This is redundant, because we
do something very similar in MaybeSendAddr(), which is called from
SendMessages() after the version handshake is finished.
There are a couple of differences:
1) MaybeSendAddr() self-advertises to all peers we do address relay with,
not just outbound ones.
2) GetLocalAddrForPeer() called from MaybeSendAddr() makes a
probabilistic decision to either advertise
what they think we are or what we think we are, while
PushAddress(self) on VERSION deterministically only does
the former if the address from the latter is unroutable.
3) During VERSION processing, we haven't received a potential sendaddrv2 message
from our peer yet, so self-advertisements with addresses from addrV2-only networks
would always be dropped in PushAddress().
Since it's confusing to have two slightly different mechanisms for self-advertising,
and the one in MaybeSendAddr() is better, remove the one in VERSION.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Follow-up to #25717. The commit "Utilize anti-DoS headers download
strategy" changed how this bool variable is computed, so that its value
is now the opposite of what it should be.
to the current p2p behavior. We only initialize the Peer::TxRelay m_relay_txs
data structure if it isn't an outbound block-relay-only connection and fRelay=true
(the peer wishes to receive tx announcements) or we're offering NODE_BLOOM to this peer.
There are many cases where we assume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.
This is an anti-fingerprinting measure. See BlockRequestAllowed in net_processing.
It has been around since 2014, but alternative clients might still serve these blocks.
See also: d8b4b49667, 85da07a5a0, a2be3b66b5, 3788a8479b
3add234546 ui: show header pre-synchronization progress (Pieter Wuille)
738421c50f Emit NotifyHeaderTip signals for pre-synchronization progress (Pieter Wuille)
376086fc5a Make validation interface capable of signalling header presync (Pieter Wuille)
93eae27031 Test large reorgs with headerssync logic (Suhas Daftuar)
355547334f Track headers presync progress and log it (Pieter Wuille)
03712dddfb Expose HeadersSyncState::m_current_height in getpeerinfo() (Suhas Daftuar)
150a5486db Test headers sync using minchainwork threshold (Suhas Daftuar)
0b6aa826b5 Add unit test for HeadersSyncState (Suhas Daftuar)
83c6a0c524 Reduce spurious messages during headers sync (Suhas Daftuar)
ed6cddd98e Require callers of AcceptBlockHeader() to perform anti-dos checks (Suhas Daftuar)
551a8d957c Utilize anti-DoS headers download strategy (Suhas Daftuar)
ed470940cd Add functions to construct locators without CChain (Pieter Wuille)
84852bb6bb Add bitdeque, an std::deque<bool> analogue that does bit packing. (Pieter Wuille)
1d4cfa4272 Add function to validate difficulty changes (Suhas Daftuar)
Pull request description:
New nodes starting up for the first time lack protection against DoS from low-difficulty headers. While checkpoints serve as our protection against headers that fork from the main chain below the known checkpointed values, this protection only applies to nodes that have been able to download the honest chain to the checkpointed heights.
We can protect all nodes from DoS from low-difficulty headers by adopting a different strategy: before we commit to storing a header in permanent storage, first verify that the header is part of a chain that has sufficiently high work (either `nMinimumChainWork`, or something comparable to our tip). This means that we will download headers from a given peer twice: once to verify the work on the chain, and a second time when permanently storing the headers.
The p2p protocol doesn't provide an easy way for us to ensure that we receive the same headers during the second download of peer's headers chain. To ensure that a peer doesn't (say) give us the main chain in phase 1 to trick us into permanently storing an alternate, low-work chain in phase 2, we store commitments to the headers during our first download, which we validate in the second download.
Some parameters must be chosen for commitment size/frequency in phase 1, and validation of commitments in phase 2. In this PR, those parameters are chosen to both (a) minimize the per-peer memory usage that an attacker could utilize, and (b) bound the expected amount of permanent memory that an attacker could get us to use to be well-below the memory growth that we'd get from the honest chain (where we expect 1 new block header every 10 minutes).
After this PR, we should be able to remove checkpoints from our code, which is a nice philosophical change for us to make as well, as there has been confusion over the years about the role checkpoints play in Bitcoin's consensus algorithm.
Thanks to Pieter Wuille for collaborating on this design.
ACKs for top commit:
Sjors:
re-tACK 3add234546
mzumsande:
re-ACK 3add234546
sipa:
re-ACK 3add234546
glozow:
ACK 3add234546
Tree-SHA512: e7789d65f62f72141b8899eb4a2fb3d0621278394d2d7adaa004675250118f89a4e4cb42777fe56649d744ec445ad95141e10f6def65f0a58b7b35b2e654a875
Delay sending SENDHEADERS (BIP 130) message until we know our peer's best
header's chain has more than nMinimumChainWork. This reduces inadvertent
headers messages received during initial headers sync due to block
announcements, which throw off our sync algorithm.
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
Avoid permanently storing headers from a peer, unless the headers are part of a
chain with sufficiently high work. This prevents memory attacks using low-work
headers.
Designed and co-authored with Pieter Wuille.
This introduces an insignificant performance penalty, as it means locator
construction needs to use the skiplist-based CBlockIndex::GetAncestor()
function instead of the lookup-based CChain, but avoids the need for
callers to have access to a relevant CChain object.
eeee5ada23 Make adjusted time type safe (MacroFake)
fa3be799fe Add time helpers (MacroFake)
Pull request description:
This makes follow-ups easier to review. Also, it makes sense by itself.
ACKs for top commit:
ryanofsky:
Code review ACK eeee5ada23. Confirmed type changes and equivalent code changes only.
Tree-SHA512: 51bf1ae5428552177286113babdd49e82459d6c710a07b6e80a0a045d373cf51045ee010461aba98e0151d8d71b9b3b5f8f73e302d46ba4558e0b55201f99e9f
This is a refactor, putting the burden to think about thread safety to
the caller. Otherwise, there is a risk that the caller will assume
thread safety where none exists, as is evident in the previous two
commits.
f6a916683d Add functional test for block announcements during initial headers sync (Suhas Daftuar)
05f7f31598 Reduce bandwidth during initial headers sync when a block is found (Suhas Daftuar)
Pull request description:
On startup, if our headers chain is more than a day behind current time, we'll pick one peer to sync headers with until our best headers chain is caught up (at that point, we'll try to sync headers with all peers).
However, if an INV for a block is received before our headers chain is caught up, we'll then start to sync headers from each peer announcing the block. This can result in doing a big headers sync with many (if not all) of our peers simultaneously, which wastes bandwidth.
This PR would reduce that overhead by picking (at most) one new peer to try syncing headers with whenever a new block is announced, prior to our headers chain being caught up.
ACKs for top commit:
LarryRuane:
ACK f6a916683d
ajtowns:
ACK f6a916683d
mzumsande:
ACK f6a916683d
dergoegge:
Code review ACK f6a916683d
achow101:
ACK f6a916683d
Tree-SHA512: 0662000bd68db146f55981de4adc2e2b07cbfda222b1176569d61c22055e5556752ffd648426f69687ed1cc203105515e7304c12b915d6270df8e41a4a0e1eaa
If our headers chain is behind on startup, then if a block is found we'll try
to catch up from all peers announcing the block, in addition to our initial
headers-sync peer. This commit changes behavior so that in this situation,
we'll choose at most one peer announcing a block to additionally sync headers
from.
fadd8b2676 addrman: Use system time instead of adjusted network time (MarcoFalke)
Pull request description:
This changes addrman to use system time for address relay instead of the network adjusted time.
This is an improvement, because network time has multiple issues:
* It is non-monotonic, even if the system time is monotonic.
* It may be wrong, even if the system time is correct.
* It may be wrong, if the system time is wrong. For example, when the node has limited number of connections (`4`), or the system time is wrong by too much (more than +-70 minutes), or the system time only got wrong after timedata collected more than half of the entries while the time was correct, ...)
This may slightly degrade addr relay for nodes where timedata successfully adjusted the time. Addr relay can already deal with minor offsets of up to 10 minutes. Offsets larger than this should still allow addr relay and not result in a DoS.
ACKs for top commit:
dergoegge:
Code review ACK fadd8b2676
Tree-SHA512: b6c178fa01161544e5bc76c4cb23e11bcc30391f7b7a64accce864923766647bcfce2e8ae21d36fb1ffc1afa07bc46415aca612405bd8d4cc1f319c92a08498f
b4b657ba57 refactor: log `nEvicted` message in `LimitOrphans` then return void (chinggg)
Pull request description:
Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49347
LimitOrphans() can log expired tx and it should log evicted tx as well instead of returning the `nEvicted` number for caller to print the message.
Since `LimitOrphans()` now returns void, the redundant assertion check in fuzz test is also removed.
Top commit has no ACKs.
Tree-SHA512: 18c41702321b0e59812590cd389f3163831d431f4ebdc3b3e1e0698496a6bdbac52288f28f779237a58813c6717da1a35e8933d509822978ff726c1b13cfc778
`LimitOrphans()` can log expired tx and it should log evicted tx as well
instead of returning the number for caller to print the message.
Since `LimitOrphans()` now return void, the redundant assertion check in
fuzz test is also removed.
8d8eeb422e [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8eea1 [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe18d [net processing] Remove CNode::nServices (John Newbery)
7d1c036934 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d51b [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb528f7 [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47d5c [net processing] Add m_our_services and m_their_services to Peer (John Newbery)
Pull request description:
Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`.
This is also a prerequisite for adding `PeerManager` unit tests (See #25515).
ACKs for top commit:
MarcoFalke:
ACK 8d8eeb422e🔑
jnewbery:
utACK 8d8eeb422e
mzumsande:
Code Review ACK 8d8eeb422e
Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
99f4785cad Replace GetTime() with NodeClock in MaybeSendGetHeaders() (Suhas Daftuar)
abf5d16c24 Don't send getheaders message when another request is outstanding (Suhas Daftuar)
ffe87db247 Cleanup received_new_header calculation to use WITH_LOCK (Suhas Daftuar)
6d95cd3e74 Move peer state updates from headers message into separate function (Suhas Daftuar)
2b341db731 Move headers direct fetch to end of ProcessHeadersMessage (Suhas Daftuar)
29c4518522 Move headers-direct-fetch logic into own function (Suhas Daftuar)
bf8ea6df75 Move additional headers fetching to own function (Suhas Daftuar)
9492e93bf9 Add helper function for checking header continuity (Suhas Daftuar)
7f2450871b Move handling of unconnecting headers into own function (Suhas Daftuar)
Pull request description:
Change `getheaders` messages so that we wait up to 2 minutes for a response to a prior `getheaders` message before issuing a new one.
Also change the handling of the `getheaders` message sent in response to a block INV, so that we no longer use the hashstop variable (including the hash stop will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a `getheaders` during IBD with an empty headers message (rather than nothing) -- this better conforms to the intent of the new logic that it's better to not ignore a peer's `getheaders` message, even if you have nothing to give. This also avoids a lot of functional tests breaking.
This PR also reworks the headers processing logic to make it more readable.
ACKs for top commit:
ajtowns:
ACK 99f4785cad ; code review, check over new logic of when to send getheaders messages
dergoegge:
Code review ACK 99f4785cad
mzumsande:
Code Review ACK 99f4785cad
sipa:
utACK 99f4785cad
w0xlt:
tACK 99f4785cad Good improvement in the code.
Tree-SHA512: b8a63f6f71ac83e292edc0200def7835ad8b06b2955dd34e3ea6fac85980fa6962efd31d689ef5ea121ff5477ec14aafa4bbe2d0db134c05f4a31a57a8ced365
d1684beabe fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30 init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c412 mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b10301 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf35 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014 mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd3 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321d scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd81 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5e init: Only determine maxmempool once (Carl Dong)
386c9472c8 mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a6 scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca6 ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe🍜
ryanofsky:
Code review ACK d1684beabe. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
Change getheaders messages so that we wait up to 2 minutes for a response to a
prior getheaders message before issuing a new one.
Also change the handling of the getheaders message sent in response to a block
INV, so that we no longer use the hashstop variable (including the hash stop
will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a getheaders during IBD with an empty headers message
(rather than nothing) -- this better conforms to the intent of the new logic
that it's better to not ignore a peer's getheaders message, even if you have
nothing to give. This also avoids a lot of functional tests breaking.
p2p_segwit.py is modified to use this same strategy, as the test logic (of
expecting a getheaders after a block inv) would otherwise be broken.
Also moves the call to happen directly after validation of a headers message
(rather than mixed in with other state updates for the peer), and removes an
incorrect comment in favor of one that explains why headers sync must continue
from the last header a peer has sent.
- Store the mempool size limit (-maxmempool) in CTxMemPool as a member.
- Remove the requirement to explicitly specify a mempool size limit for
CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the
stored mempool size limit where possible.
- Remove all now-unnecessary instances of:
gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000
The code change in CChainState::GetCoinsCacheSizeState() is correct
since the coinscache should not repurpose "extra" mempool memory
headroom for itself if the mempool doesn't even exist.
fa956e7508 Replace CountSecondsDouble with Ticks<SecondsDouble> (MacroFake)
Pull request description:
Seems odd to have two ways to say exactly the same thing when one is sufficient.
ACKs for top commit:
fanquake:
ACK fa956e7508
shaavan:
ACK fa956e7508
w0xlt:
ACK fa956e7508
Tree-SHA512: b599470e19b693da1ed1102d1e86b08cb03adaddf2048752b6d050fdf86055be117ff0ae10b6953d03e00eaaf7b0cfa350137968b67d6c5b3ca68c5aa50ca6aa
e357c89538 p2p, doc: Use MAX_BLOCKS_TO_ANNOUNCE consistently (Martin Zumsande)
Pull request description:
Block announcements via headers may have up to `MAX_BLOCKS_TO_ANNOUNCE = 8` entries according to the definition of this constant.
However, there are a few spots saying they should have a size _less than_ `MAX_BLOCKS_TO_ANNOUNCE`. Fix these.
I don't think that this is critical (this only changes behavior when we get a headers announcement with exactly `MAX_BLOCKS_TO_ANNOUNCE` blocks which we can't connect), but it would be nice to handle this limit consistently.
ACKs for top commit:
dergoegge:
utACK e357c89538 - This PR makes the usage and docs of `MAX_BLOCKS_TO_ANNOUNCE` consistent with its description.
Tree-SHA512: f3772026ab0f402e3a551127ef6e4a98fa9e7af250715fe317c05988b5b33f2f3e098a00e03960d4d28c8bd2b7a97231f7f99f22f1c152c000b2e27b658cf8f2
fa8aa0aa81 Pass Peer& to Misbehaving() (MacroFake)
Pull request description:
`Misbehaving` has several coding related issues (ignoring the conceptual issues here for now):
* It is public, but it is not supposed to be called from outside of net_processing. Fix that by making it private and creating a public `UnitTestMisbehaving` method for unit testing only.
* It doesn't do anything if a `nullptr` is passed. It would be less confusing to just skip the call instead. Fix that by passing `Peer&` to `Misbehaving()`.
* It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call to `GetPeerRef` and the no longer needed lock annotations.
ACKs for top commit:
vasild:
ACK fa8aa0aa81
w0xlt:
Code Review ACK fa8aa0aa81
Tree-SHA512: e60a6b317f2b826f9e0724285d00b632d3e2a91ded9fa5ba01c80766c5d39270b719be234c01302d46eaba600910032693836aa116ff05ee1b590c7530881cd3
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_MB@g"
-END VERIFY SCRIPT-
d273e53b6e bench/rpc_mempool: Create ChainTestingSetup, use its CTxMemPool (Carl Dong)
020caba3df bench: Use existing CTxMemPool in TestingSetup (Carl Dong)
86e732def3 scripted-diff: test: Use CTxMemPool in TestingSetup (Carl Dong)
213457e170 test/policyestimator: Use ChainTestingSetup's CTxMemPool (Carl Dong)
319f0ceeeb rest/getutxos: Don't construct empty mempool (Carl Dong)
03574b956a tree-wide: clang-format CTxMemPool references (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
This PR reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from `ArgsManager`, eventually all of libbitcoinkernel will be decoupled from `ArgsManager`.
The changes in this PR:
- Allows us to have less code churn as we modify `CTxMemPool`'s constructor in later PRs
- In many cases, we can make use of existing `CTxMemPool` instances, getting rid of extraneous constructions
- In other cases, we construct a `ChainTestingSetup` and use the `CTxMemPool` there, so that we can rely on the logic in `setup_common` to set things up correctly
## Notes for Reviewers
### A note on using existing mempools
When evaluating whether or not it's appropriate to use an existing mempool in a `*TestingSetup` struct, the key is to make sure that the mempool has the same lifetime as the `*TestingSetup` struct.
Example 1: In [`src/fuzz/tx_pool.cpp`](b4f686952a/src/test/fuzz/tx_pool.cpp), the `TestingSetup` is initialized in `initialize_tx_pool` and lives as a static global, while the `CTxMemPool` is in the `tx_pool_standard` fuzz target, meaning that each time the `tx_pool_standard` fuzz target gets run, a new `CTxMemPool` is created. If we were to use the static global `TestingSetup`'s CTxMemPool we might run into problems since its `CTxMemPool` will carry state between subsequent runs. This is why we don't modify `src/fuzz/tx_pool.cpp` in this PR.
Example 2: In [`src/bench/mempool_eviction.cpp`](b4f686952a/src/bench/mempool_eviction.cpp), we see that the `TestingSetup` is in the same scope as the constructed `CTxMemPool`, so it is safe to use its `CTxMemPool`.
### A note on checking `CTxMemPool` ctor call sites
After the "tree-wide: clang-format CTxMemPool references" commit, you can find all `CTxMemPool` ctor call sites with the following command:
```sh
git grep -E -e 'make_unique<CTxMemPool>' \
-e '\bCTxMemPool\s+[^({;]+[({]' \
-e '\bCTxMemPool\s+[^;]+;' \
-e '\bnew\s+CTxMemPool\b'
```
At the end of the PR, you will find that there are still quite a few call sites that we can seemingly get rid of:
```sh
$ git grep -E -e 'make_unique<CTxMemPool>' -e '\bCTxMemPool\s+[^({;]+[({]' -e '\bCTxMemPool\s+[^;]+;' -e '\bnew\s+CTxMemPool\b'
# rearranged for easier explication
src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
src/rpc/mining.cpp: CTxMemPool empty_mempool;
src/test/util/setup_common.cpp: CTxMemPool empty_pool;
src/bench/mempool_stress.cpp: CTxMemPool pool;
src/bench/mempool_stress.cpp: CTxMemPool pool;
src/test/fuzz/rbf.cpp: CTxMemPool pool;
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{};
src/txmempool.h: /** Create a new CTxMemPool.
```
Let's break them down one by one:
```
src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
```
Necessary
-----
```
src/rpc/mining.cpp: CTxMemPool empty_mempool;
src/test/util/setup_common.cpp: CTxMemPool empty_pool;
```
These are fixed in #25223 where we stop requiring the `BlockAssembler` to have a `CTxMemPool` if it's not going to consult it anyway (as is the case in these two call sites)
-----
```
src/bench/mempool_stress.cpp: CTxMemPool pool;
src/bench/mempool_stress.cpp: CTxMemPool pool;
```
Fixed in #24927.
-----
```
src/test/fuzz/rbf.cpp: CTxMemPool pool;
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{};
```
These are all cases where we don't want the `CTxMemPool` state to persist between runs, see the previous section "A note on using existing mempools"
-----
```
src/txmempool.h: /** Create a new CTxMemPool.
```
It's a comment (someone link me to a grep that understands syntax plz thx)
ACKs for top commit:
laanwj:
Code review ACK d273e53b6e
Tree-SHA512: c4ff3d23217a7cc4a7145defc7b901725073ef73bcac3a252ed75f672c87e98ca0368d1d8c3f606b5b49f641e7d8387d26ef802141b650b215876f191fb6d5f9
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`,
so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented
differently. For example, it seems a bit confusing to disconnect
`block-relay-only` peers with `relay` permission when they send a tx
message, but not when they send an inv message. Also, keeping track of
their inv announcements seems both wasteful and confusing, as it does
nothing. This isn't possible in practice, as outbound connections do
not have permissions assigned, but sees fragile to rely on. Especially
in light of proposed changes to make that possible:
https://github.com/bitcoin/bitcoin/pull/17167
ce893c0497 doc: Update developer notes (Anthony Towns)
d2852917ee sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497🐦
hebasto:
ACK ce893c0497
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
fa4068b4e2 Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
48262a00f5 Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bb Sync chain more readily from inbound peers during IBD (Suhas Daftuar)
Pull request description:
When in IBD, if the honest chain is only known by inbound peers, then we must
eventually sync from them in order to learn it. This change allows us to
perform initial headers sync and fetch blocks from inbound peers, if we have no
blocks in flight.
The restriction on having no blocks in flight means that we will naturally
throttle our block downloads to any such inbound peers that we may be
downloading from, until we leave IBD. This is a tradeoff between preferring
outbound peers for most of our block download, versus making sure we always
eventually will get blocks we need that are only known by inbound peers even
during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
cascading failure on the network, if a large fraction of the network managed to
get stuck in IBD).
Note that the test in the second commit fails on master, without the first commit.
ACKs for top commit:
ajtowns:
ACK 48262a00f5
sipa:
ACK 48262a00f5
Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
a35f963edf Add test for getheaders behavior (Suhas Daftuar)
ef6dbe6863 Respond to getheaders if we have sufficient chainwork (Suhas Daftuar)
Pull request description:
Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes are getting restarted during that time.
To make things simpler to reason about, just use `nMinimumChainWork` as our anti-DoS threshold for responding to a getheaders request; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed).
ACKs for top commit:
klementtan:
crACK a35f963edf
naumenkogs:
ACK a35f963edf
MarcoFalke:
review ACK a35f963edf 🗯
Tree-SHA512: 131e3872e7fe80382ea9c1ec202d6c2dc59c006355c69000aa3f4ce6bccd02a6c689c8cb8f3542b5d9bc48bfa61edcbd1a78535c0b79018971d02bed2655d284
`Misbehaving` has several coding related issues (ignoring the conceptual
issues here for now):
* It is public, but it is not supposed to be called from outside of
net_processing. Fix that by making it private and creating a public
`UnitTestMisbehaving` method for unit testing only.
* It doesn't do anything if a `nullptr` is passed. It would be less
confusing to just skip the call instead. Fix that by passing `Peer&`
to `Misbehaving()`.
* It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be
propagated. This is harmless, but verbose. Fix it by removing the no
longer needed call to `GetPeerRef` and the no longer needed lock
annotations.
fa1b76aeb0 Do not call global Params() when chainman is in scope (MacroFake)
fa30234be8 Do not pass CChainParams& to PeerManager::make (MacroFake)
fafe5c0ca2 Do not pass CChainParams& to BlockAssembler constructor (MacroFake)
faf012b438 Do not pass Consensus::Params& to Chainstate helpers (MacroFake)
fa4ee53dca Do not pass time getter to Chainstate helpers (MacroFake)
Pull request description:
It seems confusing to pass chain params, consensus params, or a time function around when it is not needed.
Fix this by:
* Inlining the passed time getter function. I don't see a use case why this should be mockable.
* Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible.
ACKs for top commit:
promag:
Code review ACK fa1b76aeb0.
vincenzopalazzo:
ACK fa1b76aeb0
Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_tx_relay_mutex
Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_most_recent_block_mutex
bf6526f4a0 [test] Remove segwit argument from build_block_on_tip() (John Newbery)
c65bf50b44 Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery)
Pull request description:
This implements two of the suggestions from code reviews of PR 20799:
- Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor
- Remove segwit argument from build_block_on_tip()
ACKs for top commit:
dergoegge:
Code review ACK bf6526f4a0
naumenkogs:
ACK bf6526f4a0
Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
9db82f1bca [net processing] Don't initialize TxRelay for non-tx-relay peers. (John Newbery)
b0a4ac9c26 [net processing] Add m_tx_relay_mutex to protect m_tx_relay ptr (John Newbery)
290a8dab02 [net processing] Comment all TxRelay members (John Newbery)
42e3250497 [net processing] [refactor] Move m_next_send_feefilter and m_fee_filter_sent (John Newbery)
Pull request description:
block-relay-only connections are additional outbound connections that bitcoind makes since v0.19. They participate in block relay, but do not propagate transactions or addresses. They were introduced in #15759.
When creating an outbound block-relay-only connection, since we know that we're never going to announce transactions over that connection, we can save on memory usage by not a `TxRelay` data structure for that connection. When receiving an inbound connection, we don't know whether the connection was opened by the peer as block-relay-only or not, and therefore we always construct a `TxRelay` data structure for inbound connections.
However, it is possible to tell whether an inbound connection will ever request that we start announcing transactions to it. The `fRelay` field in the `version` message may be set to `0` to indicate that the peer does not wish to receive transaction announcements. The peer may later request that we start announcing transactions to it by sending a `filterload` or `filterclear` message, **but only if we have offered `NODE_BLOOM` services to that peer**. `NODE_BLOOM` services are disabled by default, and it has been recommended for some time that users not enable `NODE_BLOOM` services on public connections, for privacy and anti-DoS reasons.
Therefore, if we have not offered `NODE_BLOOM` to the peer _and_ it has set `fRelay` to `0`, then we know that it will never request transaction announcements, and that we can save resources by not initializing the `TxRelay` data structure.
ACKs for top commit:
MarcoFalke:
review ACK 9db82f1bca🖖
dergoegge:
Code review ACK 9db82f1bca
naumenkogs:
ACK 9db82f1bca
Tree-SHA512: 83a449a56cd6bf6ad05369f5ab91516e51b8c471c07ae38c886d51461e942d492ca34ae63d329c46e56d96d0baf59a3e34233e4289868f911db3b567072bdc41
Delay initializing the TxRelay data structure for a peer until we receive
a version message from that peer. At that point we'll know whether it
will ever relay transactions. We only initialize the m_tx_relay
data structure if:
- this isn't an outbound block-relay-only connection; AND
- fRelay=true OR we're offering NODE_BLOOM to this peer
(NODE_BLOOM means that the peer may turn on tx relay later)
