Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 13, 2024

No description provided.

fanquake and others added 30 commits November 13, 2023 09:41
380e365 guix: switch to 6.1 kernel headers over 5.15 (fanquake)

Pull request description:

  6.1 is the current longterm release: https://kernel.org/.

  Note that using an older version of the kernel headers inside Guix, is not a "hack" for compatibility, and is explicitly recommended against by glibc:

  https://sourceware.org/glibc/wiki/FAQ#What_version_of_the_Linux_kernel_headers_should_be_used.3F.

  so using the latest version of the longterm headers seems appropriate.

  The last time we changed this was when we consolidated all builds to 5.15, in bitcoin#25006.

ACKs for top commit:
  TheCharlatan:
    ACK 380e365

Tree-SHA512: 78eb601e10261d99afd030dd7d039d962c106c48a57f16deb1c65b68fee4831e1070e4c35201f567fd24bbdab30a2b00804ddd118e1fee1dc8cdac7a3fb32ac5
ebc7063 doc: update docs for CHECK_ATOMIC macro (fanquake)

Pull request description:

  Clarify that supported versions of GCC are not affected, and that Clang
  prior to version 15 still requires the explicit `-latomic` linking, when
  compiling for 32-bit.

ACKs for top commit:
  hebasto:
    ACK ebc7063.

Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
44445ae test: Avoid intermittent failures in feature_init (MarcoFalke)

Pull request description:

  The code not only modifies block dat files, but also leveldb files, which may be of smaller size. Such corruption may not force leveldb to abort, according to the intermittent test failures.

  Fix the intermittent test failures by reverting bitcoin@5ab6419 .

ACKs for top commit:
  kevkevinpal:
    lgtm ACK [44445ae](bitcoin@44445ae)
  fjahr:
    ACK 44445ae
  theStack:
    ACK 44445ae

Tree-SHA512: 8084e4aeb8a976c1706a1898d7854c55d0c4ec7b5a08f65f97ffc173c935f9b0e0c1caef7be1538a458e4c018f7bd1948173349ec76ca48bc4013a63f284bb0f
…lds, remove some external mapTx access

4dd94ca [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e8 [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939 scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9 [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c [refactor] remove access to mapTx in validation.cpp (glozow)
333367a [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdb [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8caf [refactor] use exists() instead of mapTx.find() (glozow)
1480469 [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73a [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b481 [refactor] Add helper for iterating through mempool entries (stickies-v)

Pull request description:

  Motivation
  * It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
  * Code external to mempool should ideally use its public helper methods instead of accessing `mapTx` or its iterators directly.
  * Reduce the number of complex boost multi index type interactions
  * Also see bitcoin#28335 for further context/motivation. This PR together with bitcoin#28385 simplifies that one.

  Overview of things done in this PR:
  * Make `vTxHashes` a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with `GetEntry` instead of being cached in `vTxHashes`.
  * Introduce `GetEntry` helper method to replace the more involved `GetIter` where applicable
  * Replace `mapTx` access with `CTxMemPool` helper methods
  * Simplify `checkChainLimits` call in `node/interfaces.cpp`
  * Make `CTxMemPoolEntry`s `lockPoints`mutable such that they can be changed with a const iterator directly instead of going through `mapTx`
  * Make `BlockAssembler`'s `inBlock` and `failedTx` sets of transaction hashes.

ACKs for top commit:
  glozow:
    reACK 4dd94ca
  maflcko:
    re-ACK 4dd94ca 👝
  stickies-v:
    re-ACK 4dd94ca

Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
fca0a89 ci: remove "--exclude banman" for fuzzing in mac (brunoerg)
f9b2863 fuzz: call lookup functions before calling `Ban` (brunoerg)

Pull request description:

  Fixes bitcoin#27924

  To not have any discrepancy, it's required to call lookup functions before calling `Ban`. If we don't do it, the assertion `assert(banmap == banmap_read);` may fail because `BanMapFromJson` will call `LookupSubNet` and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (bitcoin#27924).

  Also, calling lookup functions before banning is what RPC `setban` does.

ACKs for top commit:
  maflcko:
    lgtm ACK fca0a89
  dergoegge:
    ACK fca0a89

Tree-SHA512: a3d635088a556df4507e65542157f10b41d4f87dce42927b58c3b812f262f4544b6b57f3384eef1097ffdd7c32b8dd1556aae201254960cbfbf48d45551200f7
The negative bound for script threads comes from the machine which
generates the man pages, so may only be correct for that machine. Any
other placeholder value will also be wrong for some machines. Fix this
be removing the value. This also fixes help2man incorrectly bolding the
value, as if it were a paramater.

Closes bitcoin#28850.
3b70f7b doc: fix broken doc/design/multiprocess.md links after bitcoin#24352 (Ryan Ofsky)
6d43aad span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3b util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754 interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327e interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379e streams: Add SpanReader ignore method (Russell Yanofsky)

Pull request description:

  This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR bitcoin#10102, but have been moved here to make that PR smaller.

  All of these changes are refactoring changes which do not affect behavior of current code

  ---

  This PR is part of the [process separation project](bitcoin#28722).

ACKs for top commit:
  achow101:
    ACK 3b70f7b
  naumenkogs:
    ACK 3b70f7b
  maflcko:
    re-ACK 3b70f7b  🎆

Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c
Also remove an assert that is already enforced by the compiler checking
that the length of the std::array matches.
bbbbdb0 ci: Add filesystem lint check (MarcoFalke)
fada2f9 refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)

Pull request description:

  Using `std::filesystem` is problematic:

  * There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
  * Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.

  Fix all issues by removing use of it and adding a linter to avoid using it again in the future.

ACKs for top commit:
  TheCharlatan:
    ACK  bbbbdb0
  fanquake:
    ACK bbbbdb0 🦀

Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
fa6b053 mempool: persist with XOR (MarcoFalke)

Pull request description:

  Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.

  While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.

  Fix this, similar to bitcoin#6650, by rolling a random XOR pattern over the dat file when writing or reading it.

  Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.

ACKs for top commit:
  achow101:
    re-ACK fa6b053
  glozow:
    reACK fa6b053
  ismaelsadeeq:
    ACK fa6b053

Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
92d12f1 guix: update time-machine to 77386bdbfe6b0c649c05ab37f08051d1ab3e5074 (fanquake)

Pull request description:

  python-altgraph (0.17.4) has been upstreamed, see: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=0c6198319a61d85cd8925af418466dcdccf3daff, so we can use it, and drop our package definition.

  Also includes:
  * GCC 10.4.0 -> 10.5.0: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=2fbb5398a39bf18e41235891a0740fa0bc4d7a4d.
  * Linux Kernel Headers 6.1 -> 6.1.61
  * LLVM 16 & LLVM 17 become available.

ACKs for top commit:
  hebasto:
    ACK 92d12f1.
  laanwj:
    LGTM ACK 92d12f1

Tree-SHA512: e362890ebf44d0fa6b276e023f431ce02c7a451dc8472d0ad729f72a76a8001c8c02cec322bd17680e039a1f55e654eccc4466e24a6eeccd50f0076328b3cedd
Which includes achow101/signapple#13.
We can drop macholib (and altgraph) as deps.
- Ensure that the snapshot height is higher than the pruned block height when the node is pruned.
- Validate the correctness of the file path and check if the file already exists.
- Make network activity disablement optional for the user.
- Ensure the reconsiderblock command is triggered on exit, even in the case of user interruption (Ctrl-C).

Co-authored-by: Chris Heyes <[email protected]>
Co-authored-by: Sjors Provoost <[email protected]>
49a9257 build: latest config.sub in depends (fanquake)
ced0435 build: latest config.guess in depends (fanquake)

Pull request description:

  Before we make any local modifications (i.e bitcoin#28733) pull the latest files from upstream.

ACKs for top commit:
  TheCharlatan:
    ACK 49a9257

Tree-SHA512: fbbe0d6ef72a196a652467af0550b38da23b932fe68da4965a9b0dc4795db9c869969db98f660cd360f6af3a7659b46c25e3fd398e0ef127dae71726b9a915a6
3c61c60 build: Add an old hack to remove bind_at_load from libtool. (Cory Fields)
4525760 build: remove -bind_at_load usage (fanquake)

Pull request description:

  This is deprecated on macOS:
  ```bash
  ld: warning: -bind_at_load is deprecated on macOS
  ```
  and likely redundant anyways, given the behaviour of dyld3.

  Unfortunately libtool is still injecting a `-bind_at_load`, because it's version check is broken:
  ```bash
  # Don't allow lazy linking, it breaks C++ global constructors
  # But is supposedly fixed on 10.4 or later (yay!).
  if test CXX = "$tagname"; then
    case ${MACOSX_DEPLOYMENT_TARGET-10.0} in
      10.[0123])
        func_append compile_command " $wl-bind_at_load"
        func_append finalize_command " $wl-bind_at_load"
      ;;
    esac
  fi
  ```
  so this adds another change to strip them out at the end of configure.

  Note that anywhere the ld64 warnings are being emitted, we are already not adding this flag to our hardened ldflags, because of `-Wl,-fatal_warnings`.

ACKs for top commit:
  theuni:
    utACK 3c61c60.
  hebasto:
    ACK 3c61c60, tested on macOS Sonoma 14.1.1 (23B81, Apple M1) and Ubuntu 23.10 (cross-compiling for macOS). Also I've verified the actual diff in the `libtool` script.

Tree-SHA512: 98e6a095dc2d2409f8ec3b9d462e0db3643d7873d7903a12f8acd664829e7e84e797638556fa42ca8ebc1003f13a38fe9bb8a2a50cecfa991155da818574bf08
1e5b861 test: Add test for array serialization (TheCharlatan)
d49d198 refactor: Initialize magic bytes in constructor initializer (TheCharlatan)

Pull request description:

  This is a followup-PR for bitcoin#28423

  * Initialize magic bytes in constructor
  * Add a small unit test for serializing arrays.

ACKs for top commit:
  sipa:
    utACK 1e5b861
  maflcko:
    lgtm ACK 1e5b861

Tree-SHA512: 0f58d2332dc501ca9fd419f40ed4f977c83dce0169e9a0eee1ffc9f8daa2d2ef7e7df18205ba076f55d90ae6c4a20d2b51ab303150d38470a962bcc58a66f6e7
d799ea2 doc: rewrite explanation for -par= (fanquake)

Pull request description:

  The negative bound for script threads comes from the machine which generates the man pages, so may only be correct for that machine. Any other placeholder value will also be wrong for some machines. Fix this be removing the value. This also fixes help2man incorrectly bolding the value, as if it were a paramater.

  Closes bitcoin#28850.

ACKs for top commit:
  maflcko:
    lgtm ACK d799ea2
  theStack:
    ACK d799ea2

Tree-SHA512: 2eec0086faf4cc64bbf46b22949662f84d8546d2322c3d507fc44a4e1f64d228a2901af4fa4535c0771e3e14600be8308fc5dbd407b66ae6ae4f8878d8372c0a
Tested on aarch64 using the ASAN CI job. Currently unable to test on
x86_64 due to AppArmor & podman issues.
We no-longer need this, as we no-longer build python packages.

Effectively reverts de619a3.
fanquake and others added 27 commits December 8, 2023 11:33
…bitcoin#29002)

00e0658 test: fix v2 transport intermittent test failure (bitcoin#29002) (Sebastian Falbesoner)

Pull request description:

  This PR improves the following fragile construct for detection of a new connection to the node under test in `p2p_v2_transport.py`:
  https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/test/functional/p2p_v2_transport.py#L154-L156
  Only relying on the number of peers for that suffers from race conditions, as unrelated previous peers could disconnect at anytime in-between. In the test run in bitcoin#29002, the following happens:

  - `getpeerinfo()` is called the first time -> assigned to `num_peers`
  - **previous peer disconnects**, the node's peer count is now `num_peers - 1` (in most test runs, this happens before the first getpeerinfo call)
  - new peer connects, the node's peer count is now `num_peers`
  - the condition that the node's peer count is `num_peers + 1` is never true, test fails

  Use the more robust approach of watching for an increased highest peer id instead (again using the `getpeerinfo` RPC call), with a newly introduced context manager method `TestNode.wait_for_new_peer()`. Note that for the opposite case of a disconnect, no new method is introduced; this is currently used only once in the test and is also simpler.

  Still happy to take suggestions for alternative solutions.

  Fixes bitcoin#29002.

ACKs for top commit:
  kevkevinpal:
    Concept ACK [00e0658](bitcoin@00e0658)
  maflcko:
    Ok, lgtm ACK 00e0658
  stratospher:
    ACK 00e0658.

Tree-SHA512: 0118b87f54ea5e6e080ff44f29d6af6674c757a588534b3add040da435f4359e71bf85bc0a5eb7170f99cc9956e1a03c35cce653d642d31eed41bbed1f94f44f
fa6e50d fuzz: Use C++20 starts_with in rpc.cpp (MarcoFalke)
faa4838 Revert "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings" (MarcoFalke)
fae3b77 refactor: Drop unused _Pragma to ignore -Wgnu-zero-variadic-macro-arguments (MarcoFalke)
fa02fc0 refactor: modernize-use-default-member-init for bit-fields (C++20) (MarcoFalke)
fa67f09 build: Require C++20 compiler (MarcoFalke)

Pull request description:

  C++20 allows to write safer code, because it allows to enforce more stuff at compile time (`constinit`, `conteval`, `constexpr`, `std::span`, ...).

  Also, it allows to write less verbose and easier to understand code (C++ 20 Concepts).

  See bitcoin#23363 and https://en.cppreference.com/w/cpp/compiler_support#cpp20

  With g++-10 (bitcoin#28348) and clang-13 (bitcoin#28210), there is broad support for almost all features of C++20.

  It should be fine to require a C++20 compiler for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month.

  This pull request includes three small cleanups to make use of C++20 features. If any issues are detected before or after merge, this should be easy to revert. If no issues arise, it should be fine to make use of more involved C++20 features later on.

ACKs for top commit:
  fanquake:
    ACK fa6e50d

Tree-SHA512: 244d79bfb0b750a4bdd713f40573b9ca33816fb84b6c84a58f027b9d7d4bb0cc4f18642959e4cf3d094808a69e5b8a327ca8521d7c0c08af27dacb5da3e78e71
This is the functional test counterpart of PR bitcoin#28891 /
commit 007d6f0.
Instead of having different maps for selected inputs, external inputs,
and input weight in CCoinControl, have a class PreselectedInput which
tracks stores that information for each input.
Instead of having a separate CCoinControl::SelectExternal function, we
can use the normal CCoinControl::Select function and explicitly use
PreselectedInput::SetTxOut in the caller. The semantics of what an
external input is remains.
We provide the preset nLockTime to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.
We provide the preset nVersion to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.
…saction

When creating a transaction with preset inputs, also preserve the
scriptSig and scriptWitness for those preset inputs if they are provided
(e.g. in fundrawtransaction).
…saction

Instead of making -1 a magic number meaning no change or random change
position, use an optional to have that meaning.
Instead of using the output parameters, return CreatedTransactionResult
from FundTransaction in the same way that CreateTransaction does.
Additionally, instead of modifying the original CMutableTransaction, the
result from CreateTransactionInternal is used.
Asserting for the debug log message "Added connection peer=" is
insufficient for ensuring that this new connection will show up in a
following getpeerinfo() call, as the debug message is written in the
CNode ctor, which means it hasn't necessarily been added to
CConnman.m_nodes at this point.

Solve this by using the recently introduced `wait_for_new_peer`
helper, which is more robust.

Fixes bitcoin#29030.
It is awkward not using optimization.
…coin Core.

d08e820 Add a note to msvc readme re building Qt for Bitcoin Core. (Aaron Clauson)

Pull request description:

  Updated the msvc readme with a note about avoiding path too long errors when building Qt with Bitcoin Core.

  Would have saved me half an hour if I'd remembered this from the last time I did the build.

ACKs for top commit:
  hebasto:
    ACK d08e820.
  TheCharlatan:
    ACK d08e820

Tree-SHA512: f51017b15383dbcd39ad1e5e978bb255b9205dc23d72b5e3530c6aefcbbc2dc4ec3a85e5fc8c0019c8511173c298f80b837cb35f268deac424d19365b25fb335
6e0f1d2 msvc: Optimize "Release" builds (Hennadii Stepanov)

Pull request description:

  It is awkward not using optimization.

  In addition to the obvious benefits for Windows users, this PR reduces the duration of functional tests by an hour.

  Picked from bitcoin#24773.

ACKs for top commit:
  sipsorcery:
    tACK 6e0f1d2.

Tree-SHA512: 5aa7fd38cb1a81d58ea3206756a8099891866c82a747d3b8079cab0b2afa1f40ba53adff2f32eb233efcd1227babee80ab175e35a83678fafa8a4f63c356e5ca
…penBSD

fd0bde2 test: fix `addnode` functional test failure on OpenBSD (Sebastian Falbesoner)

Pull request description:

  This is the functional test counterpart of PR bitcoin#28891 / commit 007d6f0 (unfortunately, I missed it back then and only ran the unit tests -- sorry for the noise).

  master branch on OpenBSD 7.4:
  ```
  $ ./test/functional/rpc_net.py
  2023-12-08T17:29:05.057000Z TestFramework (INFO): PRNG seed is: 6024296850131317403
  2023-12-08T17:29:05.058000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_au3zchif
  2023-12-08T17:29:05.618000Z TestFramework (INFO): Test getconnectioncount
  2023-12-08T17:29:05.618000Z TestFramework (INFO): Test getpeerinfo
  2023-12-08T17:29:06.643000Z TestFramework (INFO): Check getpeerinfo output before a version message was sent
  2023-12-08T17:29:06.709000Z TestFramework (INFO): Test getnettotals
  2023-12-08T17:29:06.773000Z TestFramework (INFO): Test getnetworkinfo
  2023-12-08T17:29:06.978000Z TestFramework (INFO): Test addnode and getaddednodeinfo
  2023-12-08T17:29:06.980000Z TestFramework (ERROR): Assertion failed
  Traceback (most recent call last):
    File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
      self.run_test()
    File "/home/thestack/bitcoin/./test/functional/rpc_net.py", line 65, in run_test
      self.test_addnode_getaddednodeinfo()
    File "/home/thestack/bitcoin/./test/functional/rpc_net.py", line 224, in test_addnode_getaddednodeinfo
      assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add')
    File "/home/thestack/bitcoin/test/functional/test_framework/util.py", line 131, in assert_raises_rpc_error
      assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
  AssertionError: No exception raised
  ```

  On the PR branch, the same call succeeds.

ACKs for top commit:
  kevkevinpal:
    ACK [fd0bde2](bitcoin@fd0bde2)

Tree-SHA512: ae20816fa4025c212e115ebd267b5e5784bfcdf0051219eb686faaade47ec4f91a3947af6d24258b159290000d2dcc3f6e65e788b83b8a9297282945dbdafbfb
…oin#29030)

ea00f98 test: fix intermittent error in rpc_net.py (bitcoin#29030) (Sebastian Falbesoner)

Pull request description:

  Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point.

  Solve this by using the recently introduced `wait_for_new_peer` helper (see bitcoin#29006,  commit 00e0658), which is more robust.

  Fixes bitcoin#29030.

ACKs for top commit:
  maflcko:
    lgtm ACK ea00f98

Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00
…tools builds

1a5dae6 msvc: Define the same `QT_...` macros as in Autotools builds (Hennadii Stepanov)

Pull request description:

  There are no reasons to have such a diversion.

  Also it fixes bitcoin#28960 (comment).

ACKs for top commit:
  sipsorcery:
    tACK 1a5dae6.
  TheCharlatan:
    ACK 1a5dae6

Tree-SHA512: 75be5eabb8fec974b8d77a023c72323015a3d95fbc13b7fd85e5f25c250ae67850ddf0bcaef143828d75fe35a49e7c9b1966976b74f3ce7d14465174e6585ceb
…ness

15f5a0d 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 bitcoin#29018.

ACKs for top commit:
  maflcko:
    lgtm ACK 15f5a0d
  brunoerg:
    utACK 15f5a0d

Tree-SHA512: 854ec34b3a0f16f26db6dc419096c6e7a380e8400119534aa278d6b1d54c253b572aa2fad13c383c796c431d8ff4263956e6f60326e99f8bf6abd16d9a280e97
9f265d8 fuzz: Detect deadlocks in process_message (dergoegge)
fae1e7e 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 bitcoin#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 1067341..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 9f265d8
  dergoegge:
    ACK 9f265d8
  brunoerg:
    ACK 9f265d8

Tree-SHA512: da83ff90962bb679aae00e8e9dba639c180b7aaba544e0c4d0978d36e28a9ff1cd7a2e13009d8ab407ef57767656aca1ebc767a7d2f1bc880284f8f57c197a50
…eset input sequences and scripts to CreateTransaction

0295b44 wallet: return CreatedTransactionResult from FundTransaction (Andrew Chow)
758501b wallet: use optional for change position as an optional in CreateTransaction (Andrew Chow)
2d39db7 wallet: Explicitly preserve scriptSig and scriptWitness in CreateTransaction (Andrew Chow)
14e5074 wallet: Explicitly preserve transaction version in CreateTransaction (Andrew Chow)
0fefcbb wallet: Explicitly preserve transaction locktime in CreateTransaction (Andrew Chow)
4d335bb wallet: Set preset input sequence through coin control (Andrew Chow)
596642c wallet: Replace SelectExternal with SetTxOut (Andrew Chow)
5321786 coincontrol: Replace HasInputWeight with returning optional from Get (Andrew Chow)
e1abfb5 wallet: Introduce and use PreselectedInput class in CCoinControl (Andrew Chow)

Pull request description:

  Currently `FundTransaction` handles transaction locktime and preset input data by extracting the selected inputs and change output from `CreateTransaction`'s results. This means that `CreateTransaction` is actually unaware of any user desired locktime or sequence numbers. This can have an effect on whether and how anti-fee-sniping works.

  This PR makes `CreateTransaction` aware of the locktime and preset input data by providing them to `CCoinControl`. `CreateTransasction` will then set the sequences, scriptSigs, scriptWItnesses, and locktime as appropriate if they are specified. This allows `FundTransaction` to actually use `CreateTransaction`'s result directly instead of having to extract the parts of it that it wants.

  Additionally `FundTransaction` will return a `CreateTransactionResult` as `CreateTransaction` does instead of having several output parameters. Lastly, instead of using `-1` as a magic number for the change output position, the change position is changed to be an optional with no value set indicating no desired change output position (when provided as an input parameter) or no change output present (in the result).

ACKs for top commit:
  josibake:
    ACK bitcoin@0295b44
  S3RK:
    Code review ACK 0295b44

Tree-SHA512: 016be4d41cbf97e1938506e70959bb5335b87006162a1c1c62fa0adb637cbe7aefb76d342b8efad5f37dc693f270c8d0a0839e239fd1ac32c6941a8172f1a710
fa8adbe build: Enable -Wunreachable-code (MarcoFalke)

Pull request description:

  It seems a bit confusing to write code after a `return`. This can even lead to bugs, or incorrect code, such as https://github.com/bitcoin/bitcoin/pull/28830/files#r1415372320 . (Edit: The linked instance is not found by clang's `-Wunreachable-code`).

  Fix all issues by enabling `-Wunreachable-code`.

  This flag also enables `-Wunreachable-code-loop-increment`, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code, so remove that.

ACKs for top commit:
  ajtowns:
    > ACK [fa8adbe](bitcoin@fa8adbe)
  stickies-v:
    ACK fa8adbe
  jonatack:
    ACK fa8adbe tested with arm64 clang 17.0.6

Tree-SHA512: 12a2f74b69ae002e62ae08038f7458837090a12051a4c154d05ae4bb26fb19fc1fa76c63aedf2b3fbb36f048c593ca3b8c0efe03fe93cf07a0fd114fc84ce1e7
@achow101 achow101 closed this Aug 13, 2024
@achow101 achow101 changed the title Patch 1 . Aug 13, 2024
Repository owner locked as spam and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.