e80e4c6ff91e27d7d40f099a2d7942c29085234c validation: Remove RECENT_CONSENSUS_CHANGE validation result (TheCharlatan)
Pull request description:
The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests' comments their usage was never entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to be exposed by the kernel library keeping them around may also be confusing to future users of the library.
ACKs for top commit:
sipa:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
naumenkogs:
ACK e80e4c6ff9
dergoegge:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
ajtowns:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
Tree-SHA512: 0af17c4435bb1b5a4f43600da30545cbbe95a7d642419cabdefabfb82b9335d92262c1c48be7ca2f2a024078ae9447161228b6f951d2f508a51159a31947fb54
9c5775c331e02dab06c78ecbb58488542d16dda7 addrman: cap the `max_pct` to not exceed the maximum number of addresses (brunoerg)
Pull request description:
Fixes#31234
This PR fixes a bad alloc issue in `GetAddresses` by capping the value `max_pct`. In practice, values greater than 100 should be treated as 100 since it's the percentage of addresses to return. Also, it limites the value `max_pct` in connman target to exercise values between 0 and 100.
ACKs for top commit:
adamandrews1:
Code Review ACK 9c5775c331
marcofleon:
Tested ACK 9c5775c331e02dab06c78ecbb58488542d16dda7. Reproduced the crash on master and checked that this fixed it. The checks added to `GetAddr_` look reasonable.
mzumsande:
Code Review ACK 9c5775c331e02dab06c78ecbb58488542d16dda7
vasild:
ACK 9c5775c331e02dab06c78ecbb58488542d16dda7
Tree-SHA512: 2957ae561ccc37df71f43c1863216d2e563522ea70b9a4baee6990e0b4a1ddadccabdcb9115c131a9a57480367b5ebdd03e0e3d4c8583792e2b7d1911a0a06d3
5a96767e3f531ba9e8a676eec47727421f9f589f depends, libevent: Do not install *.pc files and remove patches for them (Hennadii Stepanov)
ffda355b5a2113fa0f7db8015f7b08bf1351e245 cmake, refactor: Move `HAVE_EVHTTP_...` to `libevent` interface (Hennadii Stepanov)
b619bdc3303217f4415342fe60e586e18fa48308 cmake: Revamp `FindLibevent` module (Hennadii Stepanov)
Pull request description:
This PR generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former.
Addresses https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2444700876:
> We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely.
Similar to https://github.com/bitcoin/bitcoin/pull/30903.
ACKs for top commit:
fanquake:
ACK 5a96767e3f531ba9e8a676eec47727421f9f589f
Tree-SHA512: 181020c16ccd2821e718c73f264badcdc5e62980c4a8d9691e759efe2ea00da2326e26308d1dcfdeac01e9e27930428ecace9f36941deee951b751b138d7266c
4120c7543ee32efed7396d7153411ecbbd588ad3 scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp} (Sebastian Falbesoner)
Pull request description:
The confusing "command" terminology for the 12-byte field in the (v1) p2p message header was replaced with the more proper term "message type" in other modules already years ago, see eg #18533, #18937, #24078, #24141. This PR does the same for the protocol.{h,cpp} module to complete the replacements. Note that "GetCommand" is a method name also used in the `ArgsManager` (there it makes much more sense), so the scripted-diff lists for this replacement the files explicitly, rather than using `$(git grep -l ...)`.
ACKs for top commit:
maflcko:
review ACK 4120c7543ee32efed7396d7153411ecbbd588ad3 🛒
fjahr:
Code review ACK 4120c7543ee32efed7396d7153411ecbbd588ad3
rkrux:
tACK 4120c7543ee32efed7396d7153411ecbbd588ad3
Tree-SHA512: 7b4dd30136392a145da95d2f3ba181c18c155ba6f3158e49e622d76811c6a45ef9b5c7539a979a04d8404faf18bb27f11457aa436d4e2998ece3deb2c9e59748
The *_RECENT_CONSENSUS_CHANGE variants in the validation result
enumerations were always unused. They seem to have been kept around
speculatively for a soft fork after segwit, however they were never used
for taproot either. This points at them not having a clear purpose.
Based on the original pull requests' comments their usage was never
entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to exposed by
the kernel library keeping them around may also be confusing to future
users of the library.
Previously this assertion checked MAX_PEER_TX_REQUEST_IN_FLIGHT was not
exceeded. However, this property is not actually enforced; it is just
used to determine when a peer is overloaded.
fafbf8acf419d5e2ca307e5804099361ca7471af Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target (MarcoFalke)
fae3cf0ffa619f8fe7e4ace1889fe7abbb53a532 ci: Temporarily disable macOS/Windows fuzz step (MarcoFalke)
Pull request description:
`g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See https://github.com/bitcoin/bitcoin/issues/31178
One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future.
Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to be set when executing any fuzz target.
Fixes https://github.com/bitcoin/bitcoin/issues/31178
Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like https://github.com/bitcoin/bitcoin/pull/31073
ACKs for top commit:
marcofleon:
Tested ACK fafbf8acf419d5e2ca307e5804099361ca7471af
davidgumberg:
I still ACK fafbf8acf4 for fixing the regression measured in #31178.
ryanofsky:
Code review ACK fafbf8acf419d5e2ca307e5804099361ca7471af but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one.
dergoegge:
utACK fafbf8acf419d5e2ca307e5804099361ca7471af
Tree-SHA512: 124fc2e8b35e0c4df414436556a7a0a36cd1bec4b3000b40dcf2ab8c85f32e0610bf7f70d2fd79223d62f3c3665b6c09da21241654c7b9859461b8ca340d5421
0f4bc635854597e15ea6968767fc4e5cf5bdd790 [fuzz] txdownloadman and txdownload_impl (glozow)
699643f23a1bd0346e36bd90c83ba1b0b0a5c3fe [unit test] MempoolRejectedTx (glozow)
fa584cbe727b62853a410623b3d7c738e11cbffd [p2p] add TxDownloadOptions bool to make TxRequestTracker deterministic (glozow)
f803c8ce8dd88d9d0fd7857f63d76045b1e2bcaa [p2p] filter 1p1c for child txid in recent rejects (glozow)
5269d57e6d78e90baa0b40629f60a2d1d63e2992 [p2p] don't process orphan if in recent rejects (glozow)
2266eba43a973345351f2b0a8296523fb7de5576 [p2p] don't find 1p1cs for reconsiderable txns that are AlreadyHaveTx (glozow)
fa7027d0fc1fb2eb4148ba9741e1736f61d7e164 [refactor] add CheckIsEmpty and GetOrphanTransactions, remove access to TxDownloadMan internals (glozow)
969b07237b990b7eb6f3d24914ccc872202d8a0f [refactor] wrap {Have,Get}TxToReconsider in txdownload (glozow)
f150fb94e7dbb3c1f4fca32a0abf063943ca676d [refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl (glozow)
1e08195135bc54f7a8b28560ae10943b1fef0d83 [refactor] move new tx logic to txdownload (glozow)
257568eab5baba07571fe2c68759e843d215d4a9 [refactor] move invalid package processing to TxDownload (glozow)
c4ce0c1218d0a3a2e9b22701f26391b8a9107196 [refactor] move invalid tx processing to TxDownload (glozow)
c6b21749ca0aea70908773d865e67511ca141ae6 [refactor] move valid tx processing to TxDownload (glozow)
a8cf3b6e845741e4b992beced564397779bfb7da [refactor] move Find1P1CPackage to txdownload (glozow)
f497414ce76a4cf44fa669e3665746cc17710fc6 [refactor] put peerman tasks at the end of ProcessInvalidTx (glozow)
6797bc42a762f431a986852fa74b1775aea8ba38 [p2p] restrict RecursiveDynamicUsage of orphans added to vExtraTxnForCompact (glozow)
798cc8f5aac9bf2111ea88d4a4c3817d34e089e2 [refactor] move Find1P1CPackage into ProcessInvalidTx (glozow)
416fbc952b209817a37e76c09fff5d17be7a72d0 [refactor] move new orphan handling to ProcessInvalidTx (glozow)
c8e67b9169bddc0bdfefa10e9cf7f9c22847e237 [refactor] move ProcessInvalidTx and ProcessValidTx definitions down (glozow)
3a41926d1b59dc9bbabc38cdc461c169426d94e7 [refactor] move notfound processing to txdownload (glozow)
042a97ce7fc672021cdb1dee62a550ef19c208fb [refactor] move tx inv/getdata handling to txdownload (glozow)
58e09f244b4bf07d31bc8dd4e939c2dc4dc74f3a [p2p] don't log tx invs when in IBD (glozow)
288865338f50d5b00758236aa4a59546a41c88c1 [refactor] rename maybe_add_extra_compact_tx to first_time_failure (glozow)
f48d36cd97e9b27dfa105c35e0fe67cba47056d1 [refactor] move peer (dis)connection logic to TxDownload (glozow)
f61d9e4b4b80842d520c490a1012044c0816679a [refactor] move AlreadyHaveTx to TxDownload (glozow)
84e4ef843db3443278d6eb70ff89fa254fcc6631 [txdownload] add read-only reference to mempool (glozow)
af918349de52e654927d50279de64f548a8b53d6 [refactor] move ValidationInterface functions to TxDownloadManager (glozow)
f6c860efb1221e1eadc3acebd6b0b885b9cc291a [doc] fix typo in m_lazy_recent_confirmed_transactions doc (glozow)
5f9004e1550f726ca9dc9a08c865fa8f2e4b92e8 [refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters (glozow)
Pull request description:
Part of #27463.
This PR does 3 things:
(1) It modularizes transaction download logic into a `TxDownloadManager`. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1] There should be no behavior changes. Using `--color_moved=dimmed_zebra -w` may help.
(2) It adds unit and fuzz (🪄) testing for transaction download.
(3) It makes a few small behavioral changes:
- Stop (debug-only) logging tx invs during IBD
- Just like all other transactions, require orphans have RecursiveDynamicUsage < 100k before adding to vExtraTxnForCompact
- Don't return a 1p1c that contains a parent or child in recent rejects. Don't process any orphan already in recent rejects. These cases should not happen in actual node operation; it's just to allow tighter sanity checks during fuzzing.
There are several benefits to this interface, such as:
- Unit test coverage and fuzzing for logic that currently isn't feasible to test as thoroughly (without lots of overhead) and/or currently only lightly tested through `assert_debug_log` (not good) in functional tests.
- When we add more functionality (e.g. package relay messages, more robust orphan handling), the vast majority of it will be within `TxDownloadManager` instead of `PeerManager`, making it easier to review and test. See #28031 for what this looks like.
- `PeerManager` will no longer know anything about / have access to `TxOrphanage`, `TxRequestTracker` or the rejection caches. Its primary interface with `TxDownloadManager` would be much simpler:
- Passing on `ValidationInterface` callbacks
- Telling `txdownloadman` when a peer {connects, disconnects}
- Telling `txdownloadman`when a {transaction, package} is {accepted, rejected} from mempool
- Telling `txdownloadman` when invs, notfounds, and txs are received.
- Getting instructions on what to download.
- Getting instructions on what {transactions, packages, orphans} to validate.
- Get whether a peer `HaveMoreWork` for the `ProessMessages` loop
- (todo) Thread-safety can be handled internally.
[1]: This module is concerned with tx *download*, not upload. It excludes transaction announcements/gossip which happens after we download/accept a transaction. Txreconciliation (erlay) is excluded from this module, as it only relates to deciding which `inv`s to send or helping the other peer decide which `inv`s to send. It is independent from this logic.
ACKs for top commit:
achow101:
light ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
theStack:
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
instagibbs:
reACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
naumenkogs:
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
Tree-SHA512: 84ab8ef8a0fc705eb829d7f7d6885f28944aaa42b03172f256a42605677b3e783919bb900d4e3b8589f85a0c387dfbd972bcd61d252d44a88c6aaa90e4bf920f
9f243cd7fa6654e3b71ba6bff82cceed547c5d53 Introduce `g_fuzzing` global for fuzzing checks (dergoegge)
Pull request description:
This PR introduces a global `g_fuzzing` that indicates if we are fuzzing.
If `g_fuzzing` is `true` then:
* Assume checks are enabled
* Special fuzzing paths are taken (e.g. pow check is reduced to one bit)
Closes#30950#31057
ACKs for top commit:
maflcko:
review ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53 🗜
brunoerg:
crACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53
marcofleon:
Tested ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53
Tree-SHA512: 56e4cad0555dec0c565ea5ecc529628ee4f37d20dc660c647fdc6948fbeed8291e6fe290de514bd4c2c7089654d9ce1add607dc9855462828b62be9ee45e4999
552cae243a1bf26bfec03eccd1458f3bf33e01dc fuzz: cover `ASMapHealthCheck` in connman target (brunoerg)
33b0f3ae966ffa50b55489eb867c4d93c0ed3489 fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg)
18c8a0945bda554e121c2a684105dffd55505cd7 fuzz: move `ConsumeNetGroupManager` to util (brunoerg)
fe624631aeb4b5fbad732ad6476c5cd986674b4f fuzz: fuzz `connman` with a non-empty addrman (brunoerg)
0a12cff2a8e54453de1f17e9c0e87e54bbe25a34 fuzz: move `AddrManDeterministic` to util (brunoerg)
Pull request description:
### Motivation
Currently, we fuzz connman with an addrman from `NodeContext`. However,
fuzzing connman with only empty addrman might not be effective, especially
for functions like `GetAddresses` and other ones that plays with addrman. Also,
we do not fuzz connman with ASMap, what would be good for functions that need
`GetGroup`, or even for addrman. Without it, I do not see how effective would be
fuzzing `ASMapHealthCheck`, for example.
### Changes
- Move `AddrManDeterministic` and `ConsumeNetGroupManager` to util.
- Use `ConsumeNetGroupManager` in connman target to construct a netgroupmanager
and use it for `ConnmanTestMsg`.
- Use `AddrManDeterministic` in connman target to create an addrman. It does
not slow down as "filling" the addrman (e.g. with `FillAddrman`).
- Add coverage for `ASMapHealthCheck`.
ACKs for top commit:
maflcko:
review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc 🏀
dergoegge:
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc
marcofleon:
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc. Changes match the PR description.
Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851
c495731a316d9c97ee05a08cf5087c5535f84bd4 fuzz: wallet: add target for `CreateTransaction` (brunoerg)
3db68e29ec632b29f5417dbef095520e75adc26d wallet: move `ImportDescriptors`/`FuzzedWallet` to util (brunoerg)
Pull request description:
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
if (selection_target == 0 && !coin_control.HasSelected()) {
- return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
+ // return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
}
```
Also, it moves `ImportDescriptors` function to `src/wallet/test/util.h` to avoid to duplicate same code.
ACKs for top commit:
marcofleon:
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4
maflcko:
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻
Tree-SHA512: a439f947b91b01e327e18cd18e63d5ce49f2cb9ca16ca9d56fe337b8cff239b3af4db18fe89478fe5faa5549d37ca935bd321913db7646fbf6818f825cb5d878
The txdownload_impl is similar but allows us to check specific
invariants within its implementation. It will also change a lot more
than the external interface (txdownloadman) will, so we will add more to
this target later.
4feaa28728442b0fd29a677d2b170a05fdf967c0 refactor: Rely on returned value of GetCoin instead of parameter (Lőrinc)
46dfbf169b49466de06dd16f7c23c6668419ef01 refactor: Return optional of Coin in GetCoin (Lőrinc)
e31bfb26c21f8b72f8c317e016d375862e27502e refactor: Remove unrealistic simulation state (Lőrinc)
Pull request description:
While reviewing [the removal of the unreachable combinations from the Coin cache logic](https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721727681), we've noticed that the related tests often [reflect impossible states](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464).
Browsing the Coin cache refactoring history revealed that migrating `bool GetCoin` to `optional<Coin> GetCoin` was [already proposed a few times before](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).
This refactor makes certain invalid states impossible, reducing the possibility of errors and making the code easier to understand. This will let us remove test code that exercises the impossible states as well.
The PR is done in multiple small steps, first swapping the new `optional` return value, slowly strangling out the usages of the return parameter, followed by the removal of the parameter.
Most of the invalid test states were still kept, except for https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322, where the new design prohibits invalid usage and https://github.com/bitcoin/bitcoin/pull/30673/files#r1749350258 was just marked with a TODO, will be removed in a follow-up PR.
ACKs for top commit:
andrewtoth:
re-ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
achow101:
ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
laanwj:
Code review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
theStack:
Code-review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
Tree-SHA512: 818d60b2e97f58c489a61120fe761fb67a08dffbefe7a3fce712d362fc9eb8c2cced23074f1bec55fe71c616a3561b5a8737919ad6ffb2635467ec4711683df7
This combines the clusterlin_add_dependency and clusterlin_cluster_serialization
fuzz tests into a single clusterlin_depgraph_sim fuzz test. This tests starts
from an empty DepGraph and performs a arbitrary number of AddTransaction,
AddDependencies, and RemoveTransactions operations on it, and compares the
resulting state with a naive reimplementation.
This commits introduces support in DepGraph for the transaction positions to be
non-continuous. Specifically, it adds:
* DepGraph::RemoveTransactions which removes 0 or more positions from a DepGraph.
* DepGraph::Positions() to get a set of which positions are in use.
* DepGraph::PositionRange() to get the highest used position in a DepGraph + 1.
In addition, it extends the DepGraphFormatter format to support holes in a
compatible way (it serializes non-holey DepGraphs identically to the old code,
and deserializes them the same way)
This changes DepGraph::AddDependency into DepGraph::AddDependencies, which takes
in a single child, but a set of parent transactions, making them all dependencies
at once.
This is important for performance. N transactions can have O(N^2) parents combined,
so constructing a full DepGraph using just AddDependency (which is O(N) on its own)
could take O(N^3) time, while doing the same with AddDependencies (also O(N) on its
own) only takes O(N^2).
Notably, this matters for DepGraphFormatter::Unser, which goes from O(N^3) to O(N^2).
Co-Authored-By: Greg Sanders <gsanders87@gmail.com>
A fuzz test already relies on these operations, and a future commit will need
the same logic too. Therefore, abstract them out into proper member functions,
with proper testing.
98c1536852d1de9a978b11046e7414e79ed40b46 test: add getorphantxs tests (tdb3)
93f48fceb7dd332ef980ce890ff7750b995d6077 test: add tx_in_orphanage() (tdb3)
34a9c10e8cdb3e9cd40fc3a420df8f73e0208a48 rpc: add getorphantxs (tdb3)
f511ff3654d999951a64098c8a9f2f8e29771dad refactor: move verbosity parsing to rpc/util (tdb3)
532491faf1aa90053af52cbedce403b9eccf0bc3 net: add GetOrphanTransactions() to PeerManager (tdb3)
91b65adff2aaf16f42c5ccca6e16b951e0e84f9a refactor: add OrphanTxBase for external use (tdb3)
Pull request description:
This PR adds a new hidden rpc, `getorphantxs`, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like `p2p_orphan_handling`) or providing additional data for statistics/visualization.
```
getorphantxs ( verbosity )
Shows transactions in the tx orphanage.
EXPERIMENTAL warning: this call may be changed in future releases.
Arguments:
1. verbosity (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
Result (for verbose = 0):
[ (json array)
"hex", (string) The transaction hash in hex
...
]
Result (for verbose = 1):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
]
},
...
]
Result (for verbose = 2):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
],
"hex" : "hex" (string) The serialized, hex-encoded transaction data
},
...
]
Examples:
> bitcoin-cli getorphantxs 2
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
```
```
$ build/src/bitcoin-cli getorphantxs 2
[
{
"txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
"wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
1
],
"hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
},
{
"txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
"wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
2
],
"hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
}
]
```
ACKs for top commit:
glozow:
reACK 98c1536852d
hodlinator:
re-ACK 98c1536852d1de9a978b11046e7414e79ed40b46
danielabrozzoni:
ACK 98c1536852d1de9a978b11046e7414e79ed40b46
pablomartin4btc:
tACK 98c1536852d1de9a978b11046e7414e79ed40b46
itornaza:
reACK 98c1536852d1de9a978b11046e7414e79ed40b46
Tree-SHA512: 66075f9faa83748350b87397302100d08af92cbef5fadb27f2b4903f028c08020bf34a23e17262b41abb3f379ca9f46cf6cd5459b8681f2b83bffbbaf3c03ff9
a7498cc7e26b4b3de976e111de2bd2d79b056b31 Fix bug in p2p_headers_presync harness (marcofleon)
Pull request description:
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
https://github.com/bitcoin/bitcoin/pull/30918 followup
ACKs for top commit:
dergoegge:
utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
instagibbs:
ACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
glozow:
makes sense, utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
mzumsande:
ACK a7498cc7e2
Tree-SHA512: b95f25dcf7ace220e30f1d72f50d85ee18777467927c0cc1ed8582b390cb7185ffc0e2f127309eb083044fb41f5a13fce5ebb15b7952718a899bafff26921be8
284bd17309ab3b124d9dcddfec62f5506383343b add check that chainwork doesn't exceed minimum work (marcofleon)
9aa5d1c3fcd10ecb94310ad515a8569bc2d418f8 add clarification in comment (marcofleon)
Pull request description:
A followup to https://github.com/bitcoin/bitcoin/pull/30661
The added assertion just makes sure that the fuzz test is working as intended. If we're sure that the total work of the test chain is never more than minimum chain work, then we can be sure that the later assertion failure would actually mean that a bug in the headers presync logic was found.
This PR also addresses:
https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1746614616https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764943665https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764961991
ACKs for top commit:
instagibbs:
reACK 284bd17309ab3b124d9dcddfec62f5506383343b
maflcko:
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b
achow101:
ACK 284bd17309ab3b124d9dcddfec62f5506383343b
Tree-SHA512: 76a9dffea4b6e13499c636d6ad26af06135319d25117c0eb40cf8dfcfdca6a4549c9b4d2ba835192ca355e0f8d476227aeabf8bdb68770def72a9fb521533fe5
f482d0e366a84008129913b442f0c955de79ac93 fuzz: reduce number of iterations in `crypto_aeadchacha20poly1305` target (brunoerg)
Pull request description:
By reducing the number of iterations we improve the performance of this target and may increase coverage.
Running with `-runs=100000` from qa-assets I noticed a significant performance improvement and an increase on cov:
master:
```
#100000 DONE cov: 567 ft: 4078 corp: 124/33Kb lim: 4096 exec/s: 793 rss: 499Mb
```
PR:
```
#100000 DONE cov: 568 ft: 3833 corp: 113/15188b lim: 1746 exec/s: 1250 rss: 544Mb
```
ACKs for top commit:
achow101:
ACK f482d0e366a84008129913b442f0c955de79ac93
marcofleon:
Tested ACK f482d0e366a84008129913b442f0c955de79ac93. Saw the same slight increase in coverage. Executed 100,000 runs several times and total time went from 30-35 sec to 20-25 sec.
stratospher:
ACK f482d0e. saw similar coverage stats
Tree-SHA512: 1a96dbc22a0aed396b7f8cc9b13534b7f20a461f64f167c69c650529d535e360499f1a501abc1f957f7541ee1860b36a5580aa488a1edbfa0270c9ed83ef741d
51f7668d31e2624e41c7ce77fe33162802808f3f addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e30e210bfc50dea974063053313ad3e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f
stratospher:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070 refactor: move `SignSignature` helpers to test utils (Sebastian Falbesoner)
Pull request description:
These helpers haven't been used in production code since segwit was merged more than eight years ago (see commit 605e8473, PR #8149), so it seems appropriate to move them to the test utils module. As suggested by instagibbs, see https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1697515508.
ACKs for top commit:
instagibbs:
ACK 58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070
pablomartin4btc:
ACK 58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070
Tree-SHA512: a52d3b92b477246f2ceb57c3690d0229a492b65a15dae331faeae9d96e5907f7fe1176edc1530243e0f088586984fd7ba435a0a2d2f2531c04d076fdf3f4095f
a240e150e837b5a95ed19765a2e8b7c5b6013f35 streams: remove AutoFile::Get() entirely (Pieter Wuille)
e624a9bef16b6335fd119c10698352b59bf2930a streams: cache file position within AutoFile (Pieter Wuille)
Pull request description:
Fixes#30833.
Instead of relying on frequent `ftell` calls (which appear to cause a significant slowdown on some systems) in XOR-enabled `AutoFile`s, cache the file position within `AutoFile` itself.
ACKs for top commit:
achow101:
ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
davidgumberg:
untested reACK a240e150e8
theStack:
Code-review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
Tree-SHA512: fd3681edc018afaf955dc7a41a0c953ca80d46c1129e3c5b306c87c95aae93b2fe7b900794eb8b6f10491f9211645e7939918a28838295e6873eb226fca7006f
e4e3b44e9cc7227b3ad765397c884999f57bac2e net: call `Select` with reachable networks in `ThreadOpenConnections` (brunoerg)
829becd990b504a2e8a57fa8a6ff6ac6ae8ff900 addrman: change `Select` to support multiple networks (brunoerg)
f698636ec86c004ab331994559c163b7319e6423 net: add `All()` in `ReachableNets` (brunoerg)
Pull request description:
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.
I did an experiment of calling `Select` without passing a network until it finds an address from a network that compose 20% ~ 25% of the addrman (limited to 100 tries).
ACKs for top commit:
achow101:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
vasild:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
naumenkogs:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
Tree-SHA512: e8466b72b85bbc2ad8bfb14471eb27d2c50d4e84218f5ede2c15a6fa3653af61b488cde492dbd398f7502bd847e95bfee1abb7e01092daba2236d3ce3d6d2268
a97f43d63a6e835bae20b0bc5d536df98f55d8a0 fuzz: Add harness for p2p headers sync (marcofleon)
a0eaa4749fe0f755e113eee70dee1989bdc07ad5 Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon)
a3f6f5acd89f2f5bb136ec247f259d212e8944d0 build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon)
0c02d4b2bdbc7a3fc3031a63b3b16bafa669d51c net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon)
Pull request description:
This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](ed6cddd98e) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).
It seems like the main concern in https://github.com/bitcoin/bitcoin/pull/28043 was the global mock function for proof of work. This PR aims to be an improvement by replacing the previous approach with a fuzz build configured using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. This ensures that the simplified test code will never be in a release binary. If we agree this is the way to go, there are some other places (for future targets) where this method could be used.
In this target, PoW isn't being tested, so the goal is to bypass the check and let the fuzzer do its thing. In the other harnesses where PoW is actually being fuzzed, `CheckProofOfWork` is now `CheckProofOfWorkImpl`. So, the only change to that function is in the name.
More about `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` can be found at https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode and https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#d-modifying-the-target.
ACKs for top commit:
naumenkogs:
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
dergoegge:
reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
instagibbs:
tested ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
brunoerg:
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad
