And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
567cec9a05 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe5192891 test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d0163 init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142e gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182 proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548ef net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6f netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d31 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51d configure: test for unix domain sockets (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`
I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):
`tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`
`bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`
Prep work for this feature includes:
- Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
- Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)
Future work:
- Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
- Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
- Enable UNIX sockets on windows where supported
- Update Network Proxies dialog in GUI to support UNIX sockets
ACKs for top commit:
Sjors:
re-ACK 567cec9a05
tdb3:
re ACK for 567cec9a05.
achow101:
ACK 567cec9a05
vasild:
ACK 567cec9a05
Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
0a533613fb docs: add release notes for #27114 (brunoerg)
e6b8f19de9 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854c test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d17 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5 net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)
Pull request description:
Revives #17167. It allows whitelisting manual connections. Fixes #9923
Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
- Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
- https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
- https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
- Force-relay/mempool permissions for a node you intentionally connected to.
ACKs for top commit:
achow101:
ACK 0a533613fb
sr-gi:
re-ACK [0a53361](0a533613fb)
pinheadmz:
ACK 0a533613fb
Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
d27e2d87b9 test: test_bitcoin: allow -testdatadir=<datadir> (Larry Ruane)
Pull request description:
This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.
This small patch does three things:
- If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp/<test-name>/datadir` as the working directory
- When the test starts, remove `<datadir>/test_temp/<test-name>/datadir` if it exists from an earlier run (currently, it's presumed not to exist due to the long random string)
- Don't delete the working directory at the end of the test if a custom data directory is being used
Example usage, which will remove, create, use `/somewhere/test_temp/getarg_tests/boolarg`, and leave it afterward:
```
$ test_bitcoin --run_test=getarg_tests/boolarg -- -testdatadir=/somewhere
Running 1 test case...
Test directory (will not be deleted): "/somewhere/test_temp/getarg_tests/boolarg/datadir"
*** No errors detected
$ ls -l /somewhere/test_temp/getarg_tests/boolarg/datadir
total 8
drwxrwxr-x 2 larry larry 4096 Feb 22 10:28 blocks
-rw-rw-r-- 1 larry larry 1273 Feb 22 10:28 debug.log
```
(A relative pathname also works.)
This change affects only `test_bitcoin`; it could also be applied to `test_bitcoin-qt` but that's slightly more involved so I'm skipping that for now.
The rationale for this change is that, when running the test using the debugger, it's often useful to watch `debug.log` as the test runs and inspect some of the other files (I've looked at the generated `blknnnn.dat` files for example). Currently, that requires figuring out where the test's working directory is since it changes on every test run. Tests can be run with `-printtoconsole=1` to show debug logging to the terminal, but it's nice to keep `debug.log` continuously open in an editor, for example.
Even if not using a debugger, it's sometimes helpful to see `debug.log` and other artifacts after the test completes.
Similar functionality is already possible with the functional tests using the `--tmpdir=` and `--nocleanup` arguments.
ACKs for top commit:
davidgumberg:
ACK d27e2d87b9
tdb3:
re-ACK for d27e2d87b9
achow101:
ACK d27e2d87b9
cbergqvist:
ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a! (Already did some testing with `fs::remove()` to make sure it was compatible with the `util::Lock/UnlockDirectory` implementation).
marcofleon:
ACK d27e2d87b9. I ran all the tests with my previous open file limit and no errors were detected. Also ran some individual tests with no, relative, and absolute paths and everything looks good.
furszy:
ACK d27e2d8
Tree-SHA512: a8f535f34a48b6699cb440f97f5562ec643f3bfba4ea685768980b871fc8b6e1135f70fc05dbe19aa2c8bacb1ddeaff212d63473605a7422ff76332b3a6b1f68
d5228efb53 kernel: Remove dependency on CScheduler (TheCharlatan)
06069b3913 scripted-diff: Rename MainSignals to ValidationSignals (TheCharlatan)
0d6d2b650d scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner (TheCharlatan)
4abde2c4e3 [refactor] Make MainSignals RAII styled (TheCharlatan)
84f5c135b8 refactor: De-globalize g_signals (TheCharlatan)
473dd4b97a [refactor] Prepare for g_signals de-globalization (TheCharlatan)
3fba3d5dee [refactor] Make signals optional in mempool and chainman (TheCharlatan)
Pull request description:
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
Renames `CMainSignals` to `ValidationSignals`, which more accurately describes its purpose and scope.
Also make the `ValidationSignals` in the `ChainstateManager` and CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It improves the kernel API and removes two modules from the kernel library.
ACKs for top commit:
maflcko:
re-ACK d5228efb53🌄
ryanofsky:
Code review ACK d5228efb53. Just comment change since last review.
vasild:
ACK d5228efb53
furszy:
diff ACK d5228ef
Tree-SHA512: e93a5f10eb6182effb84bb981859a7ce750e466efd8171045d8d9e7fe46e4065631d9f6f533c5967c4d34c9bb7d7a67e9f4593bd4c5b30cd7b3bbad7be7b331b
Specifying this argument overrides the path location for test_bitcoin;
it becomes <datadir>/test_common_Bitcoin Core/<testname>/datadir. Also,
this directory isn't removed after the test completes. This can make it
easier for developers to study the results of a test (see the state of
the data directory after the test runs), and also (for example) have an
editor open on debug.log to monitor it across multiple test runs instead
of having to re-open a different pathname each time.
Example usage (note the "--" is needed):
test_bitcoin --run_test=getarg_tests/boolarg -- \
-testdatadir=/somewhere/mydatadir
This will create (if necessary) and use the data directory:
/somewhere/mydatadir/test_common_Bitcoin Core/getarg_tests/boolarg/datadir
Co-authored-by: furszy <mfurszy@protonmail.com>
86b7f28d6c serialization: use internal endian conversion functions (Cory Fields)
432b18ca8d serialization: detect byteswap builtins without autoconf tests (Cory Fields)
297367b3bb crypto: replace CountBits with std::bit_width (Cory Fields)
52f9bba889 crypto: replace non-standard CLZ builtins with c++20's bit_width (Cory Fields)
Pull request description:
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks.
This[ matches how libc++ implements std::byteswap for c++23](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/byteswap.h#L26).
I suggest we move/rename `compat/endian.h`, but I left that out of this PR to avoid bikeshedding.
#29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate.
ACKs for top commit:
maflcko:
ACK 86b7f28d6c📘
fanquake:
ACK 86b7f28d6c - we can finish pruning out the __builtin_clz* checks/usage once the minisketch code has been updated. This is more good cleanup pre-CMake & for the kernal.
Tree-SHA512: 715a32ec190c70505ffbce70bfe81fc7b6aa33e376b60292e801f60cf17025aabfcab4e8c53ebb2e28ffc5cf4c20b74fe3dd8548371ad772085c13aec8b7970e
864e2e9097 fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (brunoerg)
Pull request description:
The string `s` represents the value from `-whitelist`/`-whitebind` (e.g. "bloom,forcerelay,noban@1.2.3.4:32") and it is used in `NetWhitelistPermissions::TryParse` and `NetWhitebindPermissions::TryParse`. However, a max length of 32 is not enough to cover a lot of cases. Even disconsidering the permissions, 32 would not be enough to cover a lot of addresses. This PR fixes it.
ACKs for top commit:
maflcko:
lgtm ACK 864e2e9097
epiccurious:
utACK 864e2e9097.
vasild:
ACK 864e2e9097
Tree-SHA512: 2b89031b9f2ea92d636f05fd167b1e5ac726742a7e7c1af8ddaeaf90236e659731aaa6b7c23f65ec16ce52ac1b9e68e7b16e23c59e355312d057e001976d172a
29029df5c7 [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea795e [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5c62 [functional test] v3 transaction submission (glozow)
27c8786ba9 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea55b2 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2e7d [policy] add v3 policy rules (glozow)
9a29d470fb [rpc] return full string for package_msg and package-error (glozow)
158623b8e0 [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)
Pull request description:
See #27463 for overall package relay tracking.
Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418
Rationale:
- There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
- Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.
V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.
Immediate benefits:
- You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
- Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.
This also enables some other cool things (again see #27463 for overall roadmap):
- Ephemeral Anchors
- Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
- We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
- We can switch to a cluster-based mempool [5] (#27677#28676), which removes CPFP carve out [6].
[1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
[2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
[3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
[4]: Original PR #25038 also contains a lot of the discussion
[5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
[6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12
ACKs for top commit:
sdaftuar:
ACK 29029df5c7
achow101:
ACK 29029df5c7
instagibbs:
ACK 29029df5c7 modulo that
Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
Ensure we are checking sigop-adjusted virtual size by creating setups
and packages where sigop cost is larger than bip141 vsize.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
b851c5385d fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses (Vasil Dimov)
Pull request description:
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
ACKs for top commit:
achow101:
ACK b851c5385d
mzumsande:
ACK b851c5385d
brunoerg:
utACK b851c5385d
Tree-SHA512: 9905acff0e996f30ddac0c14e5ee9e1db926c7751472c06d6441111304242b563f7c942b162b209d80e8fb65a97249792eef9ae0a96100419565bf7f59f59676
27f260aa6e net: remove now unused global 'g_initial_block_download_completed' (furszy)
aff7d92b15 test: add coverage for peerman adaptive connections service flags (furszy)
6ed53602ac net: peer manager, dynamically adjust desirable services flag (furszy)
9f36e591c5 net: move state dependent peer services flags (furszy)
f9ac96b8d6 net: decouple state independent service flags from desirable ones (furszy)
97df4e3887 net: store best block tip time inside PeerManager (furszy)
Pull request description:
Derived from #28120 discussion.
By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.
The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections and remained inactive for a duration longer than the limited
peers threshold (the timeframe within which limited peers can provide blocks). In
such cases, upon reconnecting to the network, the node might only establish
connections with limited peers, filling up all available outbound slots. Resulting
in an inability to synchronize the chain (because limited peers will not provide
blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold).
ACKs for top commit:
achow101:
ACK 27f260aa6e
vasild:
ACK 27f260aa6e
naumenkogs:
ACK 27f260aa6e
mzumsande:
Light Code Review ACK 27f260aa6e
andrewtoth:
ACK 27f260aa6e
Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953
In the process of doing so, refactor `ConsumeNetAddr()` to generate the
addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way -
by preparing some random stream and deserializing from it. Similar code
was already found in `RandAddr()`.
No behavior change. Just an intermediate refactoring.
By relocating the peer desirable services flags into the peer
manager, we allow the connections acceptance process to handle
post-IBD potential stalling scenarios.
In the follow-up commit(s), the desirable service flags will be
dynamically adjusted to detect post-IBD stalling scenarios (such
as a +48-hour inactive node that must prefer full node connections
instead of limited peer connections because they cannot provide
historical blocks). Additionally, this encapsulation enable us
to customize the connections decision-making process based on
new user's configurations in the future.
aaaace2fd1 fuzz: Assume presence of __builtin_*_overflow, without checks (MarcoFalke)
fa223ba5eb Revert "build: Fix undefined reference to __mulodi4" (MarcoFalke)
fa7c751bd9 build: Bump clang minimum supported version to 14 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-14 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang (`clang-14`)
* https://packages.ubuntu.com/jammy/clang (`clang-14`)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang17`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bullseye/g++ (g++-10)
* https://packages.ubuntu.com/focal/g++-10
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
ACKs for top commit:
fanquake:
ACK aaaace2fd1
Tree-SHA512: 81d066b14cc568d27312f1cc814b09540b038a10a0a8e9d71fc9745b024fb6c32a959af673e6819b817ea7cef98da4abfa63dff16cffb7821b40083016b0291f
`CPubKey::VerifyPubKey` uses rng internally which leads to instability
in the fuzz test.
We fix this by avoiding `VerifyPubKey` in the test and verifying the
decoded public key with a fuzzer chosen message instead.
e5b9ee0221 fuzz: set `nMaxOutboundLimit` in connman target (brunoerg)
Pull request description:
Setting `nMaxOutboundLimit` (`-maxuploadtarget`) will make fuzz to reach more coverage in connman target. This value is used in `GetMaxOutboundTimeLeftInCycle`, `OutboundTargetReached` and `GetOutboundTargetBytesLeft`.
ACKs for top commit:
dergoegge:
utACK e5b9ee0221
jonatack:
ACK e5b9ee0221
Tree-SHA512: d19c83602b0a487e6da0e3be539aa2abc95b8bbf36cf9a3e391a4af53b959f68ca38548a96d27d56742e3b772f648da04e2bf8973dfc0ab1cdabf4f2e8d44de6
fa87f8feb7 doc: Clarify C++20 comments (MarcoFalke)
Pull request description:
Turns out "class template argument deduction for aggregates" is one of the few things implemented only in recent compilers, see https://en.cppreference.com/w/cpp/compiler_support/20
So clarify the comments.
ACKs for top commit:
hebasto:
ACK fa87f8feb7, I verified the code with clang-{16,17}.
Tree-SHA512: f6d20f946cb6f8e34db224e074ed8f9dfa598377c066d1b58a8feb9e64d007444f1e2c0399e91a3e282fd5d59f90e0d7df90aa3956824d96bc78070ee12f603c
a44808fb43 fuzz: rule-out too deep derivation paths in descriptor parsing targets (Antoine Poinsot)
Pull request description:
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
ACKs for top commit:
sipa:
utACK a44808fb43
achow101:
ACK a44808fb43
dergoegge:
ACK a44808fb43 - Not running into timeouts anymore
TheCharlatan:
ACK a44808fb43
Tree-SHA512: a5dd1dbe9adf8f088bdc435addab88b56f435e6d7d2065bd6d5c6d80a32e3f1f97d3d2323131ab233618cd6dcc477c458abe3c4c865ab569449b8bc176231e93
b1318dcc56 test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq)
5615e16b70 tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq)
fcd4296648 doc: fix typo and update incorrect comment (ismaelsadeeq)
562664d263 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 b1318dcc56
glozow:
utACK b1318dcc56
Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1
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.
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.
b2fc7a2eda [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 b2fc7a2eda
Tree-SHA512: 3d81414299c6803c34e928a53bcf843722fa8c38e1d3676cde7fa80923f9058b1ad4b9a2941f718303a6641b17eeb28b4a22eda09678102e9fb7c4e31d06f8f2
6db04be102 Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816a refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1 refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edb util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368 refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d97 refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f008 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 6db04be102
TheCharlatan:
Re-ACK 6db04be102
maflcko:
ACK 6db04be102👗
stickies-v:
re-ACK 6db04be102
Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634
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
