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>
