1331 Commits

Author SHA1 Message Date
Gloria Zhao
65c05db660
Merge bitcoin/bitcoin#29013: test: doc: follow-up #28368
b1318dcc56a0181783ee7ddbd388ae878a0efc52 test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq)
5615e16b705d74bf6ebb7c39523844f97a41cb6f tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq)
fcd429664818f14cace580513e7e6159335b5416 doc: fix typo and update incorrect comment (ismaelsadeeq)
562664d26374331d291b97e2e2f7fca1f0fd467b test: wait for fee estimator to catch up before estimating fees (ismaelsadeeq)

Pull request description:

  This is a simple PR that does two things
  1.   Fixes #29000 by waiting for the fee estimator to catch up after `removeForBlock` calls before calling `estimateFee` in the `BlockPolicyEstimates` unit test.

  2. Addressed some outstanding review comments from #28368
  - Updated `NewMempoolTransactionInfo::m_from_disconnected_block` to `NewMempoolTransactionInfo::m_mempool_limit_bypassed` which now correctly indicates what the boolean does.
  - Changed  input of `processTransaction`'s tx_info  `m_submitted_in_package` input from false to fuzz data provider boolean.
  - Fixed some typos, and update incorrect comment

ACKs for top commit:
  martinus:
    re-ACK b1318dcc56a0181783ee7ddbd388ae878a0efc52
  glozow:
    utACK b1318dcc56a0181783ee7ddbd388ae878a0efc52

Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1
2024-01-03 11:23:27 +00:00
ismaelsadeeq
b1318dcc56 test: change m_submitted_in_package input to fuzz data provider boolean
In reality some mempool transaction might be submitted in a package,
so change m_submitted_in_package to fuzz data provider boolean just like
m_has_no_mempool_parents.
2024-01-02 12:41:01 +01:00
ismaelsadeeq
5615e16b70 tx fees: update m_from_disconnected_block to m_mempool_limit_bypassed
The boolean indicates whether the transaction was added without enforcing mempool
fee limits. m_mempool_limit_bypassed is the correct variable name.

Also changes NewMempoolTransactionInfo booleans descriptions to the format that
is consistent with the codebase.
2024-01-02 12:41:01 +01:00
fanquake
eef19c4ce2
Merge bitcoin/bitcoin#29064: fuzz: Improve fuzzing stability for minisketch harness
b2fc7a2eda103724ac8cbeaf99df3ce6f5b7d974 [fuzz] Improve fuzzing stability for minisketch harness (dergoegge)

Pull request description:

  The `minisketch` harness has low stability due to:
  * Rng internal to minisketch
  * Benchmarkning for the best minisketch impl

  Fix this by seeding the rng and letting the fuzzer choose the impl.

  Also see #29018.

ACKs for top commit:
  maflcko:
    review ACK b2fc7a2eda103724ac8cbeaf99df3ce6f5b7d974

Tree-SHA512: 3d81414299c6803c34e928a53bcf843722fa8c38e1d3676cde7fa80923f9058b1ad4b9a2941f718303a6641b17eeb28b4a22eda09678102e9fb7c4e31d06f8f2
2023-12-18 13:54:00 +00:00
fanquake
4b94578fd8
Merge bitcoin/bitcoin#29079: fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH
fa769d3e41daec696452b8a0a8753ba511b0a4b5 fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH (MarcoFalke)

Pull request description:

  Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65039

ACKs for top commit:
  dergoegge:
    utACK fa769d3e41daec696452b8a0a8753ba511b0a4b5
  brunoerg:
    crACK fa769d3e41daec696452b8a0a8753ba511b0a4b5

Tree-SHA512: 46f70d1acf4e2f95055c70162909010c6322f8504a810906e1ab4db470dc2525f9a494b8427b254279bc68b1c8b87338c943787fd5249df7113556740701a51a
2023-12-18 12:52:59 +00:00
Ava Chow
4ad5c71adb
Merge bitcoin/bitcoin#28051: Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly
6db04be102807ee0120981a9b8de62a55439dabb Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625a6a4885fcbdfe236629a5f381eeb05 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816affa790e02e7ba0780c4ef33d2310ff refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1e74624cf149ed20abd9145c49d614a refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edbb1812dc353084c62772ebb1024d632 util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368d3aaa426b97837ef475ec5aa612f5f refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d9710ebebda5de356fab01dd7c149d5fa refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa9cc09546eabac18d0ea35274dd5d72 refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f03e786a84dd7763d1c04665e6371f2 refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336bee74fe2d58474ac36bca28c219e85 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f0082c60516acced1b03abb8e4d8f9ee46 refactor: Remove call to ShutdownRequested from chainstate init (Ryan Ofsky)

Pull request description:

  This change drops `shutdown.h` and `shutdown.cpp` files, replacing them with a `NodeContext::shutdown` member which is used to trigger shutdowns directly. This gets rid of an unnecessary layer of indirection, and allows getting rid of the `kernel::g_context` global.

  Additionally, this PR tries to improve error handling of `SignalInterrupt` code by marking relevant methods `[[nodiscard]]` to avoid the possibility of uncaught exceptions mentioned https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.

  Behavior is changing In a few cases which are noted in individual commit messages. Particularly: GUI code more consistently interrupts RPCs when it is shutting down, shutdown state no longer persists between unit tests, the stop RPC now returns an RPC error if requesting shutdown fails instead of aborting, and other failed shutdown calls now log errors instead of aborting.

  This PR is a net reduction in lines of code, but in some cases the explicit error handling and lack of global shutdown functions do make it more verbose. The verbosity can be seen as good thing if it discourages more code from directly triggering shutdowns, and instead encourages code to return errors or send notifications that could be translated into shutdowns. Probably a number of existing shutdown calls could just be replaced by better error handling.

ACKs for top commit:
  achow101:
    ACK 6db04be102807ee0120981a9b8de62a55439dabb
  TheCharlatan:
    Re-ACK 6db04be102807ee0120981a9b8de62a55439dabb
  maflcko:
    ACK 6db04be102807ee0120981a9b8de62a55439dabb 👗
  stickies-v:
    re-ACK 6db04be102807ee0120981a9b8de62a55439dabb

Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634
2023-12-14 15:14:00 -05:00
dergoegge
b2fc7a2eda [fuzz] Improve fuzzing stability for minisketch harness
* Seed minisketch rng
* Use fuzzer chosen minisketch impl instead of benchmarking for the best
  impl
2023-12-14 20:10:21 +00:00
MarcoFalke
fa769d3e41
fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH 2023-12-14 12:39:02 +01:00
fanquake
255004fc5e
Merge bitcoin/bitcoin#29009: fuzz: p2p: Detect peer deadlocks
9f265d88253ed464413dea5614fa13dea0d8cfd5 fuzz: Detect deadlocks in process_message (dergoegge)
fae1e7e012571201fd51c547ba4fb6044be9aeb5 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 9f265d88253ed464413dea5614fa13dea0d8cfd5
  dergoegge:
    ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5
  brunoerg:
    ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5

Tree-SHA512: da83ff90962bb679aae00e8e9dba639c180b7aaba544e0c4d0978d36e28a9ff1cd7a2e13009d8ab407ef57767656aca1ebc767a7d2f1bc880284f8f57c197a50
2023-12-11 15:05:40 +00:00
fanquake
40bc501bf4
Merge bitcoin/bitcoin#29031: fuzz: Improve fuzzing stability for txorphan harness
15f5a0d0c8ce6b306cdeba6a4777334b848a76aa 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 15f5a0d0c8ce6b306cdeba6a4777334b848a76aa
  brunoerg:
    utACK 15f5a0d0c8ce6b306cdeba6a4777334b848a76aa

Tree-SHA512: 854ec34b3a0f16f26db6dc419096c6e7a380e8400119534aa278d6b1d54c253b572aa2fad13c383c796c431d8ff4263956e6f60326e99f8bf6abd16d9a280e97
2023-12-11 12:34:41 +00:00
dergoegge
15f5a0d0c8 fuzz: Improve fuzzing stability for txorphan harness 2023-12-08 13:14:46 +00:00
MarcoFalke
fa6e50d6c7
fuzz: Use C++20 starts_with in rpc.cpp 2023-12-07 11:06:16 +01:00
MarcoFalke
fae3b77a87
refactor: Drop unused _Pragma to ignore -Wgnu-zero-variadic-macro-arguments 2023-12-07 11:06:05 +01:00
fanquake
2e8ec6b338
Merge bitcoin/bitcoin#29012: fuzz: Avoid timeout in bitdeque
fad1903b8a85506378101c1f857ba47b4a058fb4 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 fad1903b8a85506378101c1f857ba47b4a058fb4
  dergoegge:
    utACK fad1903b8a85506378101c1f857ba47b4a058fb4
  brunoerg:
    crACK fad1903b8a85506378101c1f857ba47b4a058fb4

Tree-SHA512: d3d83acb3e736b8fcaf5d17ce225ac82a9f9a2efea048512d2fed594ba6c76c25bae72eb0fab3276d4db37baec0752e5367cecfb18161301b921fed09693045e
2023-12-06 17:16:17 +00:00
Andrew Chow
c46cc8d3c1
Merge bitcoin/bitcoin#27581: net: Continuous ASMap health check
3ea54e5db7d53da5afa321e1800c29aa269dd3b3 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff826a69f3f8e58139dbffb611cc5947 test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37aed1276ff8527328c956c70c6e02ee13 fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547fc72a5a2902927aa7138e43c0fb7c8 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 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
  mzumsande:
    ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
  brunoerg:
    crACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3

Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
2023-12-06 11:22:42 -05:00
dergoegge
9f265d8825 fuzz: Detect deadlocks in process_message 2023-12-06 16:04:21 +00:00
MarcoFalke
fad1903b8a
fuzz: Avoid timeout in bitdeque 2023-12-06 15:44:38 +01:00
MarcoFalke
fae1e7e012
fuzz: p2p: Detect peer deadlocks 2023-12-06 13:11:38 +01:00
Greg Sanders
38816ff64e fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid 2023-12-04 14:42:13 -05:00
Ryan Ofsky
42e5829d97 refactor: Remove call to ShutdownRequested from HTTPRequest
Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.

There is no change in behavior in this commit.
2023-12-04 15:39:15 -04:00
Andrew Chow
a97a89244e
Merge bitcoin/bitcoin#28368: Fee Estimator updates from Validation Interface/CScheduler thread
91504cbe0de2b74ef1aa2709761aaf0597ec66a2 rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918ba2b853fc69bee6b04a33ba0c828bf5 tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b9944cbb56126ba37a8da180d1327ba39 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd38223d7d04166e05de11d0d0b55e60f13 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368fc0dc43827a8969a37b7e038d5ca79 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987294d4ef2814abfca16d8e2a0c5f541 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549d2ba4dd3af12b9ce65e29158f59078 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 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
  TheCharlatan:
    Re-ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
  willcl-ark:
    ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
2023-12-01 15:07:23 -05:00
Andrew Chow
498994b6f5
Merge bitcoin/bitcoin#26762: bugfix: Make CCheckQueue RAII-styled (attempt 2)
5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd refactor: Move `{MAX,DEFAULT}_SCRIPTCHECK_THREADS` constants (Hennadii Stepanov)
6e17b3168072ab77ed7170ab81327c017877133a refactor: Make `CCheckQueue` non-copyable and non-movable explicitly (Hennadii Stepanov)
8111e74653dc5c93cb510672d99048c3f741d8dc refactor: Drop unneeded declaration (Hennadii Stepanov)
9cf89f7a5b81197e38f58b24be0793b28fe41477 refactor: Make `CCheckQueue` constructor start worker threads (Hennadii Stepanov)
d03eaacbcfb276fb638db1b423113ff43bd7ec41 Make `CCheckQueue` destructor stop worker threads (Hennadii Stepanov)
be4ff3060b7b43b496dfb5a2c02b114b2b717106 Move global `scriptcheckqueue` into `ChainstateManager` class (Hennadii Stepanov)

Pull request description:

  This PR:
  - makes `CCheckQueue` RAII-styled
  - gets rid of the global `scriptcheckqueue`
  - fixes https://github.com/bitcoin/bitcoin/issues/25448

  The previous attempt was in https://github.com/bitcoin/bitcoin/pull/18731.

ACKs for top commit:
  martinus:
    ACK 5b3ea5fa2e7
  achow101:
    ACK 5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd
  TheCharlatan:
    ACK 5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd

Tree-SHA512: 45cca846e7ed107e3930149f0b616ddbaf2648d6cde381f815331b861b5d67ab39e154883ae174b8abb1dae485bc904318c50c51e5d6b46923d89de51c5eadb0
2023-11-30 14:28:46 -05:00
Ryan Ofsky
ffb021612b
Merge bitcoin/bitcoin#28451: refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream
fa98a097a30bc39f2424c0efd28a7979155faae6 Rename version.h to node/protocol_version.h (MarcoFalke)
fa4fbd58169a244c14017c62218e443b18a868ef Remove unused version.h include (MarcoFalke)
fa0ae22ff2c608c94b26c85040c4a1c7e9f7cf90 Remove unused SER_NETWORK, SER_DISK (MarcoFalke)
fae00fe9c25af80024adda33d9077962964269ea Remove unused CDataStream (MarcoFalke)
fa7eb4f5c3d2438f9689cd46b22dcfd50f6bd751 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 fa98a097a30bc39f2424c0efd28a7979155faae6
  ajtowns:
    ACK fa98a097a30bc39f2424c0efd28a7979155faae6
  ryanofsky:
    Seems odd to not code review ACK fa98a097a30bc39f2424c0efd28a7979155faae6 (looks good)

Tree-SHA512: 9f1b9d9f92bda0512610bda6653e892756f637860362a9abfa439faab62de233cbad94b7df78ebacc160d9667aadfed4d9df08c0edefa618c040a049050fb913
2023-11-30 11:11:51 -05:00
fanquake
05d3f8e822
Merge bitcoin/bitcoin#28951: fuzz: BIP324: damage ciphertext/aad in full byte range
e67634ef19db310511a22f461bb1af7edb3d862b 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 e67634ef19db310511a22f461bb1af7edb3d862b

Tree-SHA512: 1bab4df28708e079874feee939beef45eff235215375c339decc696f4c9aef04e4b417322b045491c8aec6e88ec8ec2db564e27ef1b0be352b6ff4ed38bad49a
2023-11-30 15:03:53 +00:00
MarcoFalke
fa98a097a3
Rename version.h to node/protocol_version.h 2023-11-30 11:28:31 +01:00
MarcoFalke
fa4fbd5816
Remove unused version.h include 2023-11-30 11:28:19 +01:00
MarcoFalke
fae00fe9c2
Remove unused CDataStream 2023-11-30 11:27:54 +01:00
MarcoFalke
fa7eb4f5c3
fuzz: Drop unused version from fuzz input format 2023-11-30 11:27:21 +01:00
fanquake
8cf2137dbe
Merge bitcoin/bitcoin#28958: refactor: Use Txid in CMerkleBlock
fa02c08c93e5867b7ea07d79ca1c0917dcde88e0 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 fa02c08c93e5867b7ea07d79ca1c0917dcde88e0
  dergoegge:
    reACK fa02c08c93e5867b7ea07d79ca1c0917dcde88e0

Tree-SHA512: 2e6837b9d0c90bd6e9d766330e7086d68c6ec80bb27fe2cfc4702b251b00d91a79f8bfbc76d998cbcd90bee5317402cf617f61099eee96d94e7ac8f37ba7a642
2023-11-29 10:55:18 +00:00
Andrew Chow
535424a10b
Merge bitcoin/bitcoin#28903: refactor: Make CTxMemPoolEntry only explicitly copyable
705e3f1de00bf30d728addd52a790a139d948e32 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 705e3f1de00bf30d728addd52a790a139d948e32 🌯
  achow101:
    ACK 705e3f1de00bf30d728addd52a790a139d948e32
  ajtowns:
    ACK 705e3f1de00bf30d728addd52a790a139d948e32
  ismaelsadeeq:
    ACK 705e3f1de00bf30d728addd52a790a139d948e32

Tree-SHA512: 62056905c679c919d00f9ae065ed66ac986e7e7062015aea542843d8deecda57104d7a68d002f7b20afa3164f8e9215d2d2d002c167224129540e3b1bd0712cc
2023-11-28 14:45:23 -05:00
fanquake
fe4e83f50d
Merge bitcoin/bitcoin#28912: refactor: VectorWriter and SpanReader without nVersion
fae76a1f2ac0b352cdd708e6d57f8605bf40112e scripted-diff: Use DataStream in most places (MarcoFalke)
fac39b56b723f79ba0aa9570708a1d4a600f4e12 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 fae76a1f2ac0b352cdd708e6d57f8605bf40112e
  ryanofsky:
    Code review ACK fae76a1f2ac0b352cdd708e6d57f8605bf40112e

Tree-SHA512: 3b487dba8ea380f1eacff9fdfb9197f025dbc30906813d3f4c3e6f1e9e4d9f2a169c6f163f51d135e18af538be78e2d2b13d694073ad25c5762980ae971a4c83
2023-11-28 17:35:50 +00:00
MarcoFalke
fa02c08c93
refactor: Use Txid in CMerkleBlock 2023-11-28 17:49:41 +01:00
fanquake
31ce305d46
Merge bitcoin/bitcoin#28952: fuzz: Avoid timeout in process_messages
fa825975b57bc5c4323e5d6f4bc8cd2b8189e238 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 fa825975b57bc5c4323e5d6f4bc8cd2b8189e238

Tree-SHA512: eeff732f7b0bd9a71f23aeecbf813d31fe34d355b906fd0384a43075cbc3cebc46a26df741b0f337208d8b33b3e28210c9b9437e2eed77844f03131bb8f5f2a1
2023-11-28 16:24:06 +00:00
MarcoFalke
fac39b56b7
refactor: SpanReader without nVersion
The field is unused, so remove it.

This is also required for future commits.
2023-11-28 12:42:07 +01:00
fanquake
c252a0fc0f
Merge bitcoin/bitcoin#28892: refactor: P2P transport without serialize version and type
fa79a881ce0537e1d74da296a7513730438d2a02 refactor: P2P transport without serialize version and type (MarcoFalke)
fa9b5f4fe32c0cfe2e477bb11912756f84a52cfe refactor: NetMsg::Make() without nVersion (MarcoFalke)
66669da4a5ca9edf2a40d20879d9a8aaf2b9e2ee Remove unused Make() overload in netmessagemaker.h (MarcoFalke)
fa0ed0794161d937d2d3385963c1aa5624b60d17 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 fa79a881ce0537e1d74da296a7513730438d2a02

Tree-SHA512: 785b413580d980f51f0d4f70ea5e0a99ce14cd12cb065393de2f5254891be94a14f4266110c8b87bd2dbc37467676655bce13bdb295ab139749fcd8b61bd5110
2023-11-28 11:24:09 +00:00
MarcoFalke
fa825975b5
fuzz: Avoid timeout in process_messages 2023-11-28 09:58:10 +01:00
Sebastian Falbesoner
e67634ef19 fuzz: BIP324: damage ciphertext/aad in full byte range
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`).
2023-11-28 02:30:09 +01:00
fanquake
5f9fd11680
Merge bitcoin/bitcoin#28931: fuzz: Limit fuzz buffer size in script_flags target
faf1fb207fb6e9a12c864074f8c40d5922d93ff4 Fix IWYU for the script_flags fuzz target (MarcoFalke)
fa71285b7301b2993bcc68525649716afbd9abf8 fuzz: Limit fuzz buffer size in script_flags target (MarcoFalke)
fa6b87b9ee661d8ef4ec244d230ebdeb7d1841a0 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 faf1fb207fb6e9a12c864074f8c40d5922d93ff4
  brunoerg:
    utACK faf1fb207fb6e9a12c864074f8c40d5922d93ff4

Tree-SHA512: 9301917b353f7409e448b6fd3635de19330856e0742431db5ef04e62873501b5b4cd6cb78ad81ada2747fa2bdae033115b5951d10489dd5d0d320426c8b96bee
2023-11-26 12:22:50 +00:00
fanquake
b5a271334c
Merge bitcoin/bitcoin#28922: Use Txid in COutpoint
9e58c5bcd96e7ff2062274868814ccae0626589e 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 9e58c5bcd96e7ff2062274868814ccae0626589e
  stickies-v:
    ACK 9e58c5bcd96e7ff2062274868814ccae0626589e. 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 9e58c5bcd96e7ff2062274868814ccae0626589e

Tree-SHA512: 58f61ce1c58668f689513e62072a7775419c4d5af8f607669cd8cdc2e7be9645ba14af7f9e2d65da2670da3ec1ce7fc2a744037520caf799aba212fd1ac44b34
2023-11-24 14:41:58 +00:00
fanquake
f4073c5395
Merge bitcoin/bitcoin#28578: fuzz: add target for DescriptorScriptPubKeyMan
47e5c9994c087d1826ccc0d539e916845b5648fb fuzz: add target for `DescriptorScriptPubKeyMan` (brunoerg)
641dddf01812407d163520def83f5975413691e4 fuzz: create ConsumeCoins (brunoerg)
2e1833ca1341ab4dc92508a59181aa6c7c38db88 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 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓
  dergoegge:
    ACK 47e5c9994c087d1826ccc0d539e916845b5648fb

Tree-SHA512: 519acca6d7b7a3a0bfc031441b02d5980b12bfb97198bd1958a83cd815ceb9eb1499a48a3f0a7fe20e5d06d83b89335d987376fc0a014e2106b0bc0e9838dd02
2023-11-23 17:34:03 +00:00
MarcoFalke
faf1fb207f
Fix IWYU for the script_flags fuzz target
Also, export script_error.h from interpreter.h, because there should
rarely be a case where script_error.h is included without interpreter.h
2023-11-23 17:57:53 +01:00
MarcoFalke
fa71285b73
fuzz: Limit fuzz buffer size in script_flags target 2023-11-23 17:56:52 +01:00
MarcoFalke
fa6b87b9ee
fuzz: CDataStream -> DataStream in script_flags 2023-11-23 17:50:33 +01:00
MarcoFalke
fa79a881ce
refactor: P2P transport without serialize version and type 2023-11-23 13:43:39 +01:00
ismaelsadeeq
714523918b tx fees, policy: CBlockPolicyEstimator update from CValidationInterface notifications
`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>
2023-11-22 11:48:21 +01:00
ismaelsadeeq
dff5ad3b99 CValidationInterface: modify the parameter of TransactionAddedToMempool
Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of
`TransactionAddedToMempool` callback.
2023-11-22 11:48:21 +01:00
ismaelsadeeq
91532bd382 tx fees, policy: update CBlockPolicyEstimator::processBlock parameter
Update `processBlock` parameter to reference to a vector of `RemovedMempoolTransactionInfo`.
2023-11-22 11:48:21 +01:00
dergoegge
9e58c5bcd9 Use Txid in COutpoint 2023-11-21 13:15:44 +00:00
brunoerg
641dddf018 fuzz: create ConsumeCoins 2023-11-20 15:57:56 -03:00
brunoerg
2e1833ca13 fuzz: move MockedDescriptorConverter to fuzz/util 2023-11-20 15:57:50 -03:00