9f265d8825 fuzz: Detect deadlocks in process_message (dergoegge)
fae1e7e012 fuzz: p2p: Detect peer deadlocks (MarcoFalke)
Pull request description:
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.
Fix this by detecting them in the fuzz target.
Can be tested by introducing a bug such as:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) {
- const CInv &inv = *it++;
+ const CInv& inv = *it;
if (inv.IsGenBlkMsg()) {
```
Using a fuzz input such as:
```
$ base64 ./timeout-ada0fecaba2b8c46c6e970cf637d9625b01bf7e5
kNptdNbW1tbWYghvXIpwb25vPQAA////////cwAjLv8AXAB2ZXJhY2sAQW5v/62tra3Pz///////
//////////////////////9c8GZpbHRlcmxvYWQAAAEAAwAAAABVYwC2XABmaWx0ZXJhZGQAAAAX
Fxdn/////2V0F861tcqvEmAAACEAAABjYXB0dXJldmUAAH4AgAA1PNfX11x0Z2V0ZGF0YQBDACOw
AQMAAAAGIm5GERoLWcqvEmBD61u/KMNPOl4zKh/HKLK3PPGIkQ9eE/////////8AAAAAAAAAAFtb
WyjDTzpeMSofx7K3PNfX11x0Z2V0ZGF0YQBDACMwAQMAAAAGIm5GERoLWcqvEmBD61u/KMNPOl4z
Kh/Hsrc88YiRD2/Nzc3Nzc3Nzc3NTc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3N
zWWj1NTUudTU1NTU1P///0j+P/9cdHR4AAAAAAAAy/4AAHR4AAAAAAAAP8v+AAD/+P//////////
AX55bJl8HWnz/////wAgXGF0YVPxY2RkAAAA
```
And running the fuzz target:
```
$ FUZZ=process_messages ./src/test/fuzz/fuzz -runs=1 -timeout=18 ./timeout-ada0fecaba2b8c46c6e970cf637d9625b01bf7e5
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3436516708
INFO: Loaded 1 modules (390807 inline 8-bit counters): 390807 [0x55d0d6221e80, 0x55d0d6281517),
INFO: Loaded 1 PC tables (390807 PCs): 390807 [0x55d0d6281518,0x55d0d6877e88),
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: ./timeout-ada0fecaba2b8c46c6e970cf637d9625b01bf7e5
ALARM: working on the last Unit for 19 seconds
and the timeout value is 18 (use -timeout=N to change)
==375014== ERROR: libFuzzer: timeout after 19 seconds
```
ACKs for top commit:
naumenkogs:
ACK 9f265d8825
dergoegge:
ACK 9f265d8825
brunoerg:
ACK 9f265d8825
Tree-SHA512: da83ff90962bb679aae00e8e9dba639c180b7aaba544e0c4d0978d36e28a9ff1cd7a2e13009d8ab407ef57767656aca1ebc767a7d2f1bc880284f8f57c197a50
15f5a0d0c8 fuzz: Improve fuzzing stability for txorphan harness (dergoegge)
Pull request description:
The `txorphan` harness has low stability as eviction of orphan txs is entirely random at the moment.
Fix this by passing the rng to `LimitOrphans`, which can be deterministic in tests.
Also see #29018.
ACKs for top commit:
maflcko:
lgtm ACK 15f5a0d0c8
brunoerg:
utACK 15f5a0d0c8
Tree-SHA512: 854ec34b3a0f16f26db6dc419096c6e7a380e8400119534aa278d6b1d54c253b572aa2fad13c383c796c431d8ff4263956e6f60326e99f8bf6abd16d9a280e97
fad1903b8a fuzz: Avoid timeout in bitdeque (MarcoFalke)
Pull request description:
Avoid timeouts such as https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1842914664
This is done by:
* Limiting the maximum number of iterations if the maximum size of the container is "large" (see the magic numbers in the code).
* Check the equality only once. This should be fine, because if a crash were to happen in the equality check, but the crash doesn't happen if further iterations were run, the fuzz engine should eventually find the crash by truncating the fuzz input.
ACKs for top commit:
sipa:
utACK fad1903b8a
dergoegge:
utACK fad1903b8a
brunoerg:
crACK fad1903b8a
Tree-SHA512: d3d83acb3e736b8fcaf5d17ce225ac82a9f9a2efea048512d2fed594ba6c76c25bae72eb0fab3276d4db37baec0752e5367cecfb18161301b921fed09693045e
3ea54e5db7 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37ae fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)
Pull request description:
There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.
This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.
ACKs for top commit:
achow101:
ACK 3ea54e5db7
mzumsande:
ACK 3ea54e5db7
brunoerg:
crACK 3ea54e5db7
Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
91504cbe0d rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918b tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b99 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd382 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549 tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition (ismaelsadeeq)
Pull request description:
This is an attempt to #11775
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
- Added a new callback to the Validation Interface `MempoolTransactionsRemovedForConnectedBlock`, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
- Modified the `TransactionAddedToMempool` callback parameter to include additional information about the transaction needed for fee estimation.
- Updated `CBlockPolicyEstimator` to process transactions using` CTransactionRef` instead of `CTxMempoolEntry.`
- Implemented the `CValidationInterface` interface in `CBlockPolicyEstimater` and overridden the `TransactionAddedToMempool`, `TransactionRemovedFromMempool`, and `MempoolTransactionsRemovedForConnectedBlock` methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the `removeForBlock` function in `txmempool.cpp`.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool's `cs` until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have `MempoolInterface` signals come from the mempool and `CValidationInterface` events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
Also left out some commits from #11775
- Some refactoring which are no longer needed.
- Handle reorgs much better in fee estimator.
- Track witness hash malleation in fee estimator
I believe they are a separate change that can come in a follow-up after this.
ACKs for top commit:
achow101:
ACK 91504cbe0d
TheCharlatan:
Re-ACK 91504cbe0d
willcl-ark:
ACK 91504cbe0d
Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
fa98a097a3 Rename version.h to node/protocol_version.h (MarcoFalke)
fa4fbd5816 Remove unused version.h include (MarcoFalke)
fa0ae22ff2 Remove unused SER_NETWORK, SER_DISK (MarcoFalke)
fae00fe9c2 Remove unused CDataStream (MarcoFalke)
fa7eb4f5c3 fuzz: Drop unused version from fuzz input format (MarcoFalke)
Pull request description:
Seems odd to have code that is completely dead.
Fix this by removing all of it.
ACKs for top commit:
sipa:
utACK fa98a097a3
ajtowns:
ACK fa98a097a3
ryanofsky:
Seems odd to not code review ACK fa98a097a3 (looks good)
Tree-SHA512: 9f1b9d9f92bda0512610bda6653e892756f637860362a9abfa439faab62de233cbad94b7df78ebacc160d9667aadfed4d9df08c0edefa618c040a049050fb913
e67634ef19 fuzz: BIP324: damage ciphertext/aad in full byte range (Sebastian Falbesoner)
Pull request description:
This PR is a tiny improvement for the `bip324_cipher_roundtrip` fuzz target: currently the damaging of input data for decryption (either ciphertext or aad) only ever happens in the lower nibble within the byte at the damage position, as the bit position for the `damage_val` byte was calculated with `damage_bit & 3` (corresponding to `% 4`) rather than `damage_bit & 7` (corresponding to the expected `% 8`).
Noticed while reviewing #28263 which uses similar constructs.
ACKs for top commit:
stratospher:
ACK e67634ef.
dergoegge:
utACK e67634ef19
Tree-SHA512: 1bab4df28708e079874feee939beef45eff235215375c339decc696f4c9aef04e4b417322b045491c8aec6e88ec8ec2db564e27ef1b0be352b6ff4ed38bad49a
fa02c08c93 refactor: Use Txid in CMerkleBlock (MarcoFalke)
Pull request description:
This should also fix a gcc-13 compiler warning, see https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1407856376
```
rpc/txoutproof.cpp: In lambda function:
rpc/txoutproof.cpp:72:33: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
72 | const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
| ^~~~
rpc/txoutproof.cpp:72:52: note: the temporary was destroyed at the end of the full expression ‘AccessByTxid((*(const CCoinsViewCache*)(&(& active_chainstate)->Chainstate::CoinsTip())), transaction_identifier<false>::FromUint256((* & tx)))’
72 | const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ACKs for top commit:
TheCharlatan:
Re-ACK fa02c08c93
dergoegge:
reACK fa02c08c93
Tree-SHA512: 2e6837b9d0c90bd6e9d766330e7086d68c6ec80bb27fe2cfc4702b251b00d91a79f8bfbc76d998cbcd90bee5317402cf617f61099eee96d94e7ac8f37ba7a642
705e3f1de0 refactor: Make CTxMemPoolEntry only explicitly copyable (TheCharlatan)
Pull request description:
This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954.
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
ACKs for top commit:
maflcko:
ACK 705e3f1de0🌯
achow101:
ACK 705e3f1de0
ajtowns:
ACK 705e3f1de0
ismaelsadeeq:
ACK 705e3f1de0
Tree-SHA512: 62056905c679c919d00f9ae065ed66ac986e7e7062015aea542843d8deecda57104d7a68d002f7b20afa3164f8e9215d2d2d002c167224129540e3b1bd0712cc
fae76a1f2a scripted-diff: Use DataStream in most places (MarcoFalke)
fac39b56b7 refactor: SpanReader without nVersion (MarcoFalke)
Pull request description:
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.
ACKs for top commit:
ajtowns:
ACK fae76a1f2a
ryanofsky:
Code review ACK fae76a1f2a
Tree-SHA512: 3b487dba8ea380f1eacff9fdfb9197f025dbc30906813d3f4c3e6f1e9e4d9f2a169c6f163f51d135e18af538be78e2d2b13d694073ad25c5762980ae971a4c83
fa825975b5 fuzz: Avoid timeout in process_messages (MarcoFalke)
Pull request description:
Reduce the number of messages per fuzz input. There should be no reason to have more messages than that.
This should also avoid timeouts, such as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64548. CC https://github.com/bitcoin/bitcoin/issues/28812
ACKs for top commit:
dergoegge:
utACK fa825975b5
Tree-SHA512: eeff732f7b0bd9a71f23aeecbf813d31fe34d355b906fd0384a43075cbc3cebc46a26df741b0f337208d8b33b3e28210c9b9437e2eed77844f03131bb8f5f2a1
fa79a881ce refactor: P2P transport without serialize version and type (MarcoFalke)
fa9b5f4fe3 refactor: NetMsg::Make() without nVersion (MarcoFalke)
66669da4a5 Remove unused Make() overload in netmessagemaker.h (MarcoFalke)
fa0ed07941 refactor: VectorWriter without nVersion (MarcoFalke)
Pull request description:
Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.
This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.
ACKs for top commit:
ajtowns:
reACK fa79a881ce
Tree-SHA512: 785b413580d980f51f0d4f70ea5e0a99ce14cd12cb065393de2f5254891be94a14f4266110c8b87bd2dbc37467676655bce13bdb295ab139749fcd8b61bd5110
Currently the damaging of input data for decryption (either ciphertext
or aad) only ever happens in the lower nibble within the byte at the
damage position, as the bit position for the `damage_val` byte was
calculated with `damage_bit & 3` (corresponding to `% 4`) rather than
`damage_bit & 7` (corresponding to the expected `% 8`).
faf1fb207f Fix IWYU for the script_flags fuzz target (MarcoFalke)
fa71285b73 fuzz: Limit fuzz buffer size in script_flags target (MarcoFalke)
fa6b87b9ee fuzz: CDataStream -> DataStream in script_flags (MarcoFalke)
Pull request description:
Most fuzz targets have an upper limit on the buffer size to avoid excessive runtime. Do the same for `script_flags` to avoid timeouts such as https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1824696971
Also, fix iwyu. Also, remove legacy `CDataStream`.
ACKs for top commit:
dergoegge:
ACK faf1fb207f
brunoerg:
utACK faf1fb207f
Tree-SHA512: 9301917b353f7409e448b6fd3635de19330856e0742431db5ef04e62873501b5b4cd6cb78ad81ada2747fa2bdae033115b5951d10489dd5d0d320426c8b96bee
9e58c5bcd9 Use Txid in COutpoint (dergoegge)
Pull request description:
This PR changes the type of the hash of a transaction outpoint from `uint256` to `Txid`.
ACKs for top commit:
Sjors:
ACK 9e58c5bcd9
stickies-v:
ACK 9e58c5bcd9. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.
TheCharlatan:
ACK 9e58c5bcd9
Tree-SHA512: 58f61ce1c58668f689513e62072a7775419c4d5af8f607669cd8cdc2e7be9645ba14af7f9e2d65da2670da3ec1ce7fc2a744037520caf799aba212fd1ac44b34
47e5c9994c fuzz: add target for `DescriptorScriptPubKeyMan` (brunoerg)
641dddf018 fuzz: create ConsumeCoins (brunoerg)
2e1833ca13 fuzz: move `MockedDescriptorConverter` to `fuzz/util` (brunoerg)
Pull request description:
This PR adds fuzz target for `DescriptorScriptPubKeyMan`. Also, moves `MockedDescriptorConverter` to `fuzz/util/descriptor` to be used here and in `descriptor` target.
ACKs for top commit:
maflcko:
lgtm ACK 47e5c9994c🏓
dergoegge:
ACK 47e5c9994c
Tree-SHA512: 519acca6d7b7a3a0bfc031441b02d5980b12bfb97198bd1958a83cd815ceb9eb1499a48a3f0a7fe20e5d06d83b89335d987376fc0a014e2106b0bc0e9838dd02
`CBlockPolicyEstimator` will implement `CValidationInterface` and
subscribe to its notification to process transactions added and removed
from the mempool.
Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator.
Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`.
Co-authored-by: Matt Corallo <git@bluematt.me>
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
This has the goal of prohibiting users from accidentally creating
runtime failures, e.g. by interacting with iterator_to with a copied
entry.
CTxMemPoolEntry is already implicitly not move-constructable. So be
explicit about this and use a std::list to collect the values in the
policy_estimator fuzz test instead of a std::vector.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
83986f464c Include version.h in fewer places (Anthony Towns)
c7b61fd61b Convert some CDataStream to DataStream (Anthony Towns)
1410d300df serialize: Drop useless version param from GetSerializeSize() (Anthony Towns)
bf574a7501 serialize: drop GetSerializeSizeMany (Anthony Towns)
efa9eb6d7c serialize: Drop nVersion from [C]SizeComputer (Anthony Towns)
Pull request description:
Drops the version field from `GetSerializeSize()`, simplifying the code in various places. Also drop `GetSerializeSizeMany()` (as just removing the version parameter could result in silent bugs) and remove unnecessary instances of `#include <version.h>`.
ACKs for top commit:
maflcko:
ACK 83986f464c📒
theuni:
ACK 83986f464c.
Tree-SHA512: 36617b6dfbb1b4b0afbf673e905525fc6d623d3f568d3f86e3b9d4f69820db97d099e83a88007bfff881f731ddca6755ebf1549e8d8a7762437dfadbf434c62e
faa25718b3 fuzz: AutoFile with XOR (MarcoFalke)
fab5cb9066 fuzz: Reduce LIMITED_WHILE limit for file fuzzing (MarcoFalke)
fa5388fad3 fuzz: Remove FuzzedAutoFileProvider (MarcoFalke)
Pull request description:
This should help to get fuzz coverage for https://maflcko.github.io/b-c-cov/fuzz.coverage/src/streams.cpp.gcov.html
Also, remove unused code and fix a timeout bug.
ACKs for top commit:
dergoegge:
ACK faa25718b3
Tree-SHA512: 56f1e6fd5cb2b66ffd9a7d9c09c9b8e396be3e7485feb03b35b6bd3c48e624fdaed50b472e4ffec21f09efb5e949d7ee32a13851849c9140b6b4cf25917dd7ac
43de4d3630 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
