In the presence of smaller transactions on the network, blocks can sustain a
higher relay rate than 7tx/second. In this event, the per-peer inventory queues
can grow too large.
This commit bumps the rate up to 14 tx/s (for inbound peers), increasing the
safety margin by a factor of 2.
Outbound peers continue to receive relayed transactions at 2.5x the rate of
inbound peers, for a rate of 35tx/second.
Co-Authored-By: Suhas Daftuar <sdaftuar@gmail.com>
Github-Pull: #28592
Rebased-From: b81f37031c8f2ccad9346f1b65ee0f8083c44796
Previously in debug builds, this would cause an Assume crash if
FillBlock had been called previously. This could happen when multiple
blocktxn messages were received.
Co-Authored-By: Greg Sanders <gsanders87@gmail.com>
1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3 kernel: improve BlockChecked ownership semantics (stickies-v)
9ba1fff29e4794615c599e59ef453848a9bdb880 kernel: refactor: ConnectTip to pass block pointer by value (stickies-v)
Pull request description:
Subscribers to the BlockChecked validation interface event may need access to the block outside of the callback scope. Currently, this is only possible by copying the block, which makes exposing this validation interface event publicly either cumbersome or with significant copy overhead.
By using shared_ptr, we make the shared ownership explicit and allow users to safely use the block outside of the callback scope. By using a const-ref shared_ptr, no atomic reference count cost is incurred if a subscriber does not require block ownership.
For example: in #30595, this would allow us to drop the `kernel_BlockPointer` handle entirely, and generalize everything into `kernel_Block`. This PoC is implemented in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/.
---
### Performance
I have added a benchmark in a [separate branch](https://github.com/stickies-v/bitcoin/commits/2025-07/validation-interface-ownership-benched/), to ensure this change does not lead to a problematic performance regression. Since most of the overhead comes from the subscribers, I have added scenarios for `One`, `Two`, and `Ten` subscribers. From these results, it appears there is no meaningful performance difference on my machine.
When `BlockChecked()` takes a `const CBlock&` reference _(master)_:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 170.09 | 5,879,308.26 | 0.3% | 0.01 | `BlockCheckedOne`
| 1,603.95 | 623,460.10 | 0.5% | 0.01 | `BlockCheckedTen`
| 336.00 | 2,976,173.37 | 1.1% | 0.01 | `BlockCheckedTwo`
When `BlockChecked()` takes a `const std::shared_ptr<const CBlock>&` _(this PR)_:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 172.20 | 5,807,155.33 | 0.1% | 0.01 | `BlockCheckedOne`
| 1,596.79 | 626,254.52 | 0.0% | 0.01 | `BlockCheckedTen`
| 333.38 | 2,999,603.17 | 0.3% | 0.01 | `BlockCheckedTwo`
ACKs for top commit:
achow101:
ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3
w0xlt:
reACK 1d9f1cb4bd
ryanofsky:
Code review ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3. These all seem like simple changes that make sense
TheCharlatan:
ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3
yuvicc:
Code Review ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3
Tree-SHA512: 7ed0cccb7883dbb1885917ef749ab7cae5d60ee803b7e3145b2954d885e81ba8c9d5ab1edb9694ce6b308235c621094c029024eaf99f1aab1b47311c40958095
de0675f9de5feae1f070840ad7218b1378fb880b refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de17951dc459bc8637e5de15b84ca445 Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72edb2085edfa75296d6b42fab34433d9 refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d89c9b089b55bc96fdf93b023be8dda policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d23128f742dfdda253752cba7db9bb0679 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f783305c923ee7667c46ca0ff7e1b96ed45c refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f24472487dc7f447839136db2ccf60833e9a2 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a7250e80c56ff8c351cf1670e32c1a2 Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de5feae1f070840ad7218b1378fb880b
dergoegge:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b
theStack:
Code-review ACK de0675f9de5feae1f070840ad7218b1378fb880b
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
Do not discourage nodes even when they send us consensus invalid
transactions.
Because we do not discourage nodes for transactions we consider
non-standard, we don't get any DoS protection from this check in
adversarial scenarios, so remove the check entirely both to simplify the
code and reduce the risk of splitting the network due to changes in tx
relay policy.
c0642e558a02319ade33dc1014e7ae981663ea46 [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92d42809e74377e4380abdc70f74de5d scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b924489238220710326e9031c7aaa0d606c9064 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505 [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298d4d2b7dddfecdbeb0edc624b8e6eb2 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c67043a2a46950fd3f055afbe4a93922f75 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f151600f00c94a2732d2595446011295 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d70580ae47da228e2f88cd53c40c675 scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df6e6e2cc9b9aeb3c3c8e1c78fa5be1d [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e016969098c486f413f4f57dcffe35241785 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa7f19177b5f422f596a3ab2bd1e9633 [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3c3ac090b68e370efc6dd9374b6ad3b [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a02319ade33dc1014e7ae981663ea46
marcofleon:
Nice, ACK c0642e558a02319ade33dc1014e7ae981663ea46
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
Subscribers to the BlockChecked validation interface event may need
access to the block outside of the callback scope. Currently, this
is only possible by copying the block, which makes exposing this
validation interface event publicly either cumbersome or with significant
copy overhead.
By using shared_ptr, we make the shared ownership explicit and allow
users to safely use the block outside of the callback scope.
Simplifies `m_tx_inventory_to_send` a bit by making it a set of Wtxids.
Wtxid relay is prevalent throughout the network, so the complexity of
dealing with a GenTxid in this data structure isn't necessary.
For peers that aren't wtxid relay, the txid is now retrieved from our
mempool entry when the inv is constructed.
1cb23997033c395d9ecd7bf2f54787b134485f41 doc: clarify the GetAddresses/GetAddressesUnsafe documentation (Daniela Brozzoni)
e5a7dfd79f618b04e0140ec2c50c6e95c2a2e2e4 p2p: rename GetAddresses -> GetAddressesUnsafe (Daniela Brozzoni)
Pull request description:
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P.
Additionally, better reflect in the documentation that the two methods should be used in different contexts.
Also update the outdated "call the function without a parameter" phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc8a669b30717525597e3f468eaecf79, 81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each
function, and the previous commit changed the function naming completely.
ACKs for top commit:
stickies-v:
re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
l0rinc:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
luisschwab:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
brunoerg:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
theStack:
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
mzumsande:
Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
Tree-SHA512: 02c05d88436abcdfabad994f47ec5144e9ba47668667a2c1818f57bf8710727505faf8426fd0672c63de14fcf20b96f17cea2acc39fe3c1f56abbc2b1a9e9c23
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
Move towards a model where TxOrphanage is initialized with limits that
it remembers throughout its lifetime.
Remove the param. Limiting by number of unique orphans will be removed
in a later commit.
Now that -maxorphantx is gone, this does not change the node behavior.
The parameter is only used in tests.
a60f863d3e276534444571282f432b913d3967db scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba1995986323cd9e76097acc1f15eed7c60943 Remove old GenTxid class (marcofleon)
072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c79497ae19f7e481439e350533c7cd1a Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b0780aa3754af35beffbcfeb31f28045b Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec0b04851bea0a688d7681b6aaca4cb7 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2bea83c53635dee9a49c8c273a12440dd move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3019a0ea4ebbc9c41781813ad574a86 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d59071f3e43a42f3809706790495b17ffc refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb8f0c3094934b3fef45871f73bb216a Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e276534444571282f432b913d3967db
maflcko:
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
theStack:
Code-review ACK a60f863d3e276534444571282f432b913d3967db
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using
`std::variant` we have no portable zero-overhead way of accessing them,
so use `std::visit` and drop `bool wtxid` in-parameter.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
28299ce77636d7563ec545d043cf1b61bd2f01c1 p2p: remove vestigial READ_STATUS_CHECKBLOCK_FAILED (Greg Sanders)
bac9ee4830664c86c1cb3d38a5b19c722aae2f54 p2p: Add witness mutation check inside FillBlock (Greg Sanders)
Pull request description:
Since #29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside `PartiallyDownloadedBlock::FillBlock`, immediately before returning `READ_STATUS_OK`.
ACKs for top commit:
Crypt-iQ:
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
achow101:
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
stratospher:
ACK 28299ce7.
dergoegge:
Code review ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
Tree-SHA512: 883d7c12ca096234b425e6fe12e46b0611607600916e6ac8d1c8112224aa76924b7b074754910163ac2ec15379075d618a9ece3642649ac7629cddb0d4e432ea
9341b5333ad54ccdb7c16802ff06c51b956948e7 blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee0b108ebbb8afedc47d73ce0f97d272 test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6dd0cc23175a1484e8ebb6cdc6e2183 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)
Pull request description:
A follow-up to https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165, after which validating the hash of a read block from disk doesn't incur the cost of calculating its hash anymore.
### Summary
This PR adds explicit checks that the read block header's hash matches the one we were expecting.
### Context
After the previous PR, validating a block's hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block's expected hash (or `std::nullopt`), preventing silent failures caused by corrupted or mismatched data. Most `ReadBlock` usages were updated with expected hashes and now fail on mismatch.
### Changes
* added hash assertions in `ProcessGetBlockData` and `ProcessMessage` to validate that the block read from disk matches the expected hash;
* updated tests and benchmark to pass the correct block hash to `ReadBlock()`, ensuring the hash validation is tested - or none if we already expect PoW failure;
* removed the default value for `expected_hash`, requiring an explicit hash for all block reads.
### Why is the hash still optional (but no longer has a default value)
* for header-error tests, where the goal is to trigger failures early in the parsing process;
* for out-of-order orphan blocks, where the child hash isn't available before the initial disk read.
ACKs for top commit:
maflcko:
review ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7 🕙
achow101:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
hodlinator:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
janb84:
re ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
The non-recent-block code path in `ProcessGetBlockData` already has `inv.hash` available (equaling `pindex->GetBlockHash()`).
Pass it to `ReadBlock()` and assert that the on-disk header matches the requested hash.
The `GETBLOCKTXN` message handler in `ProcessMessage` receives `req.blockhash` from the peer (equaling `pindex->GetBlockHash()`).
Pass this hash to `ReadBlock()` for verification and assert that the index lookup matches.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Since #29412, we have not allowed mutated blocks to continue
being processed immediately the block is received, but this
is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow
similar mutation strategies to affect relay by honest peers
by applying the check inside
PartiallyDownloadedBlock::FillBlock, immediately before
returning READ_STATUS_OK.
This also removes the extraneous CheckBlock call.
Previously ChainstateManager::AcceptBlockHeader would log when it
saw a new header. This commit moves logging to the call site(s) in
net_processing. The next commits will then log which peer sent it
and whether it was part of a compact block.
This commit changes behavior:
- when multiple headers are received in a single message, only the
last one is logged
- if any of the headers are invalid, the valid ones are not logged
This happens because net_processing calls ProcessNewBlockHeaders
with multiple headers, which then calls AcceptBlockHeader one
header at a time.
Additionally:
- when the header is received via a compact block, there's no more
duplicate log (a later commit also unifies logging code paths)
fa21f83d2983d97006ec1e3c47634dc0fe0349dc ci: Use G++ in valgrind tasks (MarcoFalke)
fabd05bf651138679f76728f974f141ac8ce99a9 refactor: Fix net_processing iwyu includes (MarcoFalke)
fa1622db208025e1744e78c4f5b135db11b293d4 refactor: Make node_id a const& in RemoveBlockRequest (MarcoFalke)
Pull request description:
Currently, `valgrind` is not usable on a default build with GCC. Specifically, `p2p_compactblocks.py --valgrind` gives a false-positive in `RemoveBlockRequest` when comparing `node_id` with `from_peer`. According to the upstream bug report, this happens because both symbols are on the stack and the compiler can more aggressively optimize the compare (order). See https://bugs.kde.org/show_bug.cgi?id=472329#c7
It is possible to work around this bug by pulling at least one value from the stack. For example, by making `from_peer` a `const` reference. Alternatively, by replacing `auto [node_id, list_it]` with `const auto& [node_id, list_it]`, which is done here.
I think this workaround is acceptable, because it does not look like valgrind can trivially fix this. The alternative would be to add a (temporary?) suppression.
Fixes https://github.com/bitcoin/bitcoin/issues/27741
Also, fix iwyu includes, while touching this module.
Also, switch the CI valgrind scripts to use G++.
ACKs for top commit:
achow101:
ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
TheCharlatan:
ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
darosior:
utACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
ryanofsky:
Code review ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc. Code changes all look good but I'm a little confused about purpose of the third commit, so left a question about that
Tree-SHA512: 7b92cdafd525a5ac53ae2c1a7a92e599bc9b5fd5d315a694b493cd5079ac323d884393b57aa18581b7789247a588c9a27d47698de25b340bc76fc9f1dd1850b4
eb0724f0dee307d6d14e47ebd3077b7ffd50f507 doc: banman: reference past vuln due to unbounded banlist (Antoine Poinsot)
ad616b6c013e69221f61b695c4ae09a3471c3f7c doc: net: mention past vulnerability as rationale to limit incoming message size (Antoine Poinsot)
4489117c3f6720ef92a328d3462cec8c0f466ae5 doc: txrequest: point to past censorship vulnerability in tx re-request handling (Antoine Poinsot)
68ac9542c451c9088c59a3ec6124d87cfd3382a3 doc: net_proc: reference past DoS vulnerability in orphan processing (Antoine Poinsot)
c02d9f6dd53989f41375f13a2d39270fa5d58a04 doc: net_proc: reference past defect regarding invalid GETDATA types (Antoine Poinsot)
5e3d9f21df21a822dc210d73a000faba084e6067 doc: validation: add a reference to historical header spam vulnerability (Antoine Poinsot)
Pull request description:
It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.
ACKs for top commit:
ryanofsky:
Code review ACK eb0724f0dee307d6d14e47ebd3077b7ffd50f507. No changes since last review other than rebase
Tree-SHA512: 271902f45b8130d44153d793bc1096cd22b6ce05494e67c665a5bc45754e3fc72573d303ec8fc7db4098d473760282ddbf0c1cf316947539501dfd8d7d5b8828
ffff4a293ad878494e12f8f00108cc99ee2b713e bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97bcb1790a7cd4363a13fda7c80c3dd90 refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b40c97375af0722f32f7575bca3af819 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179c062b7ca92d120455ce02a9f4e9e19 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e6e80e3da1ab6448b6212244bafa5d3 scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c03db00a2be3f703aa7e5eec4312bd2e refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717ec18d64b7ff7bba71b8f0c202ba31d test: Fix broken span_tests (MarcoFalke)
fadf02ef8bf96ad5b3b8e34fd425b31b555f4371 refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be17fa9e7c91188710e6a04939ceab11 refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293ad878494e12f8f00108cc99ee2b713e
theuni:
ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
551a09486c495e1a3cfc296eafdf95e914856bff net: Switch to DisconnectMsg in CConnman (Hodlinator)
bbac17608d1ad3f8af5b32efad5d573c70989361 net: Bring back log message when resetting socket (Hodlinator)
04b848e4827f502d0784c5975bc8e652fc459cc8 net: Specify context in disconnecting log message (Hodlinator)
0c4954ac7d9676774434e5779bb5fd88e789bbb6 net_processing: Add missing use of DisconnectMsg (Hodlinator)
Pull request description:
- Add missing calls to `DisconnectMsg()` - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
- Specify context when stopping nodes - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890780754
- Bring back log message when resetting socket in case new entrypoints are added - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890795074
- Use `DisconnectMsg()` in `CConnman` as well - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791797716
ACKs for top commit:
Sjors:
re-utACK 551a09486c495e1a3cfc296eafdf95e914856bff
l0rinc:
utACK 551a09486c495e1a3cfc296eafdf95e914856bff
davidgumberg:
Tested and Review ACK 551a09486c
achow101:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
danielabrozzoni:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
Tree-SHA512: 95ab8e7436e20ca3abc949ea09697facb6fbeb19981ddc7e0bf294e7ec914e72cbf836c21184a2a887f04cb264f26daf5b0cbcbebc9db633a7b1672b4e488063
223081ece651dc616ff63d9ac447eedc5c2a28fa scripted-diff: rename block and undo functions for consistency (Lőrinc)
baaa3b284671ba28dbbcbb43851ea46175fd2b13 refactor,blocks: remove costly asserts and modernize affected logs (Lőrinc)
fa39f27a0f8b8d14f6769d48f43999a3a1148e4f refactor,blocks: deduplicate block's serialized size calculations (Lőrinc)
dfb2f9d004860c95fc6f0d4a016a9c038d53a475 refactor,blocks: inline `WriteBlockToDisk` (Lőrinc)
42bc4914658d9834a653bd1763aa8f0d54355480 refactor,blocks: inline `UndoWriteToDisk` (Lőrinc)
86b85bb11f8999eb59e34bd026b0791dc866f2eb bench: add SaveBlockBench (Lőrinc)
34f9a0157aad7c10ac364b7e4602c5f74c1f9e20 refactor,bench: rename bench/readblock.cpp to bench/readwriteblock.cpp (Lőrinc)
Pull request description:
`UndoWriteToDisk` and `WriteBlockToDisk` were delegating a subset of their functionality to single-use methods that didn't optimally capture a meaningful chunk of the algorithm, resulting in calculating things twice (serialized size, header size).
This change inlines the awkward methods (asserting that all previous behavior was retained), and in separate commits makes the usages less confusing.
Besides making the methods slightly more intuitive, the refactorings reduce duplicate calculations as well.
The speed difference is insignificant for now (~0.5% for the new `SaveBlockToDiskBench`), but are a cleanup for follow-ups such as https://github.com/bitcoin/bitcoin/pull/31539
ACKs for top commit:
ryanofsky:
Code review ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.
hodlinator:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
TheCharlatan:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
andrewtoth:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
Tree-SHA512: 951bc8ad3504c510988afd95c561e3e259c6212bd14f6536fe56e8eb5bf5c35c32a368bbdb1d5aea1acc473d7e5bd9cdcde02008a148b05af1f955e413062d5c
86d7135e36efd39781cf4c969011df99f0cbb69d [p2p] only attempt 1p1c when both txns provided by the same peer (glozow)
f7658d9b1475ecaa5cb8e543e5c66a3a3a2dc1fb [cleanup] remove p2p_inv from AddTxAnnouncement (glozow)
063c1324c143d98e6d5108bb51b3ca59b45f9b85 [functional test] getorphantxs reflects multiple announcers (glozow)
0da693f7e129fccaecf9a2c177083d2e80d37781 [functional test] orphan handling with multiple announcers (glozow)
b6ea4a9afe2d8bbf49b6b6c42f0a3ce4390c4535 [p2p] try multiple peers for orphan resolution (glozow)
1d2e1d709ce3d95d409254c860347bc3fedf30e1 [refactor] move creation of unique_parents to helper function (glozow)
c6893b0f0b7b205c8da4b9d281a55c9eb843b582 [txdownload] remove unique_parents that we already have (glozow)
163aaf285af91b49c2d788463dc1e1654c88ade6 [fuzz] orphanage multiple announcer functions (glozow)
22b023b09da3e2fe00467c77b105a61c1961081f [unit test] multiple orphan announcers (glozow)
96c1a822a274689611f409246ef1573906b0083e [unit test] TxOrphanage EraseForBlock (glozow)
04448ce32a3bc4b6d12293f71e40333abe35c224 [txorphanage] add GetTx so that orphan vin can be read (glozow)
e810842acda6fe56e0536ebecfbb9d17d26e1513 [txorphanage] support multiple announcers (glozow)
62a9ff187076686b39dca64ad4f2f439da0875d1 [refactor] change type of unique_parents to Txid (glozow)
6951ddcefd9e05f31ee7634bbfbf1d19e04ec00e [txrequest] GetCandidatePeers (glozow)
Pull request description:
Part of #27463.
(Transaction) **orphan resolution** is a process that kicks off when we are missing UTXOs to validate an unconfirmed transaction. We currently request missing parents by txid; BIP 331 also defines a way to [explicitly request ancestors](https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better).
Currently, when we find that a transaction is an orphan, we only try to resolve it with the peer who provided the `tx`. If this doesn't work out (e.g. they send a `notfound` or don't respond), we do not try again. We actually can't, because we've already forgotten who else could resolve this orphan (i.e. all the other peers who announced the transaction).
What is wrong with this? It makes transaction download less reliable, particularly for 1p1c packages which must go through orphan resolution in order to be downloaded.
Can we fix this with BIP 331 / is this "duct tape" before the real solution?
BIP 331 (receiver-initiated ancestor package relay) is also based on the idea that there is an orphan that needs resolution, but it's just a new way of communicating information. It's not inherently more honest; you can request ancestor package information and get a `notfound`. So ancestor package relay still requires some kind of procedure for retrying when an orphan resolution attempt fails. See the #27742 implementation which builds on this orphan resolution tracker to keep track of what packages to download (it just isn't rebased on this exact branch). The difference when using BIP 331 is that we request `ancpkginfo` and then `pkgtxns` instead of the parent txids.
Zooming out, we'd like orphan handling to be:
- Bandwidth-efficient: don't have too many requests out at once. As already implemented today, transaction requests for orphan parents and regular download both go through the `TxRequestTracker` so that we don't have duplicate requests out.
- Not vulnerable to censorship: don't give up too easily, use all candidate peers. See e.g. https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for/
- Load-balance between peers: don't overload peers; use all peers available. This is also useful for when we introduce per-peer orphan protection, since each peer will have limited slots.
The approach taken in this PR is to think of each peer who announces an orphan as a potential "orphan resolution candidate." These candidates include:
- the peer who sent us the orphan tx
- any peers who announced the orphan prior to us downloading it
- any peers who subsequently announce the orphan after we have started trying to resolve it
For each orphan resolution candidate, we treat them as having "announced" all of the missing parents to us at the time of receipt of this orphan transaction (or at the time they announced the tx if they do so after we've already started tracking it as an orphan). We add the missing parents as entries to `m_txrequest`, incorporating the logic of typical txrequest processing, which means we prefer outbounds, try not to have duplicate requests in flight, don't overload peers, etc.
ACKs for top commit:
marcofleon:
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
instagibbs:
reACK 86d7135e36efd39781cf4c969011df99f0cbb69d
dergoegge:
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
mzumsande:
ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
Tree-SHA512: 618d523b86e60c3ea039e88326d50db4e55e8e18309c6a20e8f2b10ed9e076f1de0315c335fd3b8abdabcc8b53cbceb66fb59147d05470ea25b83a2b4bd9c877
