Open
Conversation
2ccfdb5 build: avoid exporting secp256k1 symbols (Cory Fields) Pull request description: Take advantage of the [new secp256k1 option to avoid visibility attributes](bitcoin-core/secp256k1#1696) on API functions. While most users of a shared libsecp always want API functions exported so that they can actually be linked against, we always build it statically. When that static lib is linked into a (static or shared) libbitcoinkernel, by default its symbols end up exported there as well. As libsecp is an implementation detail of the kernel (and any future Core lib), its symbols should never be exported. [This was the intended use for the above PR](bitcoin-core/secp256k1#1696 (comment)), looks like we just forgot to follow-up and actually hook it up. This is most easily tested by building with `-DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON` (with or without `-DREDUCE_EXPORTS=ON`) and inspecting via: ```bash nm -CD lib/libbitcoinkernel.so | grep secp ``` Before this change, secp's symbols will show up there. After, they should be absent. This should finally solve secp symbol visibility once and for all :) ACKs for top commit: hebasto: ACK 2ccfdb5, this is implemented exactly as I [tested](bitcoin-core/secp256k1#1696 (review)) the upstream PR. Tested on Fedora 43. stickies-v: tACK 2ccfdb5 Tree-SHA512: 664ea7a6f811c2743ad1b4d8913c61aab9b358931ee77895d35cdf8a5607fbb08facda085877c53d731afbf42a7220dcc752fc365a7625ee679c1547e1c674d0
The vcpkg tools cache was using the combined actions/cache action, which saves on every run regardless of branch. Split it into the restore/save pattern used by the other caches, so that saves only happen on default branch pushes.
- Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()` to use it for `BTC/kvB` vs `sat/vB` output formatting. - Handle all enum values, hence remove default case in `CFeeRate::ToString()` and `assert(False)` when a `FeeRateFormat` value is not handled. - Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format values from `FeeEstimateMode`. - Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format from fee-estimation mode selection.
The function signature for the `send` RPC is:
```
send [{"address":amount,...},{"data":"hex"},...] ( conf_target "estimate_mode" fee_rate options version )
```
The last example in the manpage is missing the `fee_rate` arg, but is trying to specify the `options` arg, by index.
The parser confuses the intended `options` arg as the missing `fee_rate` arg.
See:
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
error code: -8
error message:
Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.
```
vs
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical null '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
{
"psbt": "cHNidP8BAHECAAAAAa2qY4+SieuSo9idL5tiM5h9bW6xNnkYprdIyR1HGn4LAAAAAAD9////AkR2DwQAAAAAFgAUpLDwJu+wFRHLQAgKAb0psk7UVd2AlpgAAAAAABYAFOQ3U4t/E4cVYgZrq9H7q3K0+6lYAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQC4wMA/////wLIF6gEAAAAABYAFLMY1zihXrefAA0DA5nld4MCPjkrAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfyBeoBAAAAAAWABSzGNc4oV63nwANAwOZ5XeDAj45KwEIawJHMEQCIElTV4pbUrsPR9qHWcioowVv3QVWHizxwevfD0u/I8YyAiBCY3OzF81PSLM00h4ueQkehYuxDFZu7Jk51iejphKnnwEhA0VKdYVSyBpWoxBwTDOupB58Fi3mEBs+u+OOqEYVd2sZACICA98YLWyH7dBCfXVxe7woiLSTgV1mJN8Zc8KgZ77pVSg+GNBMeT5UAACAAQAAgAAAAIABAAAAbAAAAAAA",
"txid": "625b71b314a6ac4f738634e29dc007cd5edc0427c1ae96ab706d06a62910cea2",
"hex": "02000000000101adaa638f9289eb92a3d89d2f9b6233987d6d6eb1367918a6b748c91d471a7e0b0000000000fdffffff0244760f0400000000160014a4b0f026efb01511cb40080a01bd29b24ed455dd8096980000000000160014e437538b7f13871562066babd1fbab72b4fba9580247304402204953578a5b52bb0f47da8759c8a8a3056fdd05561e2cf1c1ebdf0f4bbf23c6320220426373b317cd4f48b334d21e2e79091e858bb10c566eec9939d627a3a612a79f012103454a758552c81a56a310704c33aea41e7c162de6101b3ebbe38ea84615776b1900000000",
"complete": true
}
```
… ThreadPool 38fd85c http: replace WorkQueue and threads handling for ThreadPool (furszy) c323f88 fuzz: add test case for threadpool (TheCharlatan) c528dd5 util: introduce general purpose thread pool (furszy) 6354b4f tests: log node JSON-RPC errors during test setup (furszy) 45930a7 http-server: guard against crashes from unhandled exceptions (furszy) Pull request description: This has been a recent discovery; the general thread pool class created for #26966, cleanly integrates into the HTTP server. It simplifies init, shutdown and requests execution logic. Replacing code that was never unit tested for code that is properly unit and fuzz tested. Although our functional test framework extensively uses this RPC interface (that’s how we’ve been ensuring its correct behavior so far - which is not the best). This clearly separates the responsibilities: The HTTP server now focuses solely on receiving and dispatching requests, while ThreadPool handles concurrency, queuing, and execution. This will also allows us to experiment with further performance improvements at the task queuing and execution level, such as a lock-free structure or task prioritization or any other implementation detail like coroutines in the future, without having to deal with HTTP code that lives on a different layer. Note: The rationale behind introducing the ThreadPool first is to be able to easily cherry-pick it across different working paths. Some of the ones that are benefited from it are #26966 for the parallelization of the indexes initial sync, #31132 for the parallelization of the inputs fetching procedure, #32061 for the libevent replacement, the kernel API #30595 (#30595 (comment)) to avoid blocking validation among others use cases not publicly available. Note 2: I could have created a wrapper around the existing code and replaced the `WorkQueue` in a subsequent commit, but it didn’t seem worth the extra commits and review effort. The `ThreadPool` implements essentially the same functionality in a more modern and cleaner way. ACKs for top commit: Eunovo: ReACK 38fd85c sedited: Re-ACK 38fd85c pinheadmz: ACK 38fd85c Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
6f113cb txgraph: use fallback order to sort chunks (feature) (Pieter Wuille) 0a33519 txgraph: use fallback order when linearizing (feature) (Pieter Wuille) fba004a txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille) 941c432 txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille) 39d0052 clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille) 8bfbba3 txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille) e0bc73b clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille) 6c1bcb2 txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille) 7427c7d txgraph: update chunk index on Compact (preparation) (Pieter Wuille) 3ddafce txgraph: initialize Ref in AddTransaction (preparation) (Pieter Wuille) Pull request description: Part of #30289. TxGraph's fundamental responsibility is deciding the order of transactions in the mempool. It relies on the `cluster_linearize.h` code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of: * Deciding based on **internal identifiers** (`Cluster::m_sequence`, `DepGraphIndex`). This is very simple, but risks leaking information about transaction receive order. * Deciding **randomly**, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior. * Deciding **based on txid**, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven't really seen such behavior). * Deciding **based on size** (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It's also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing. As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above. This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic. The transactions within a chunk are sorted according to: 1. `PostLinearize` (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below. 2. Topology (parents before children). 3. Individual transaction feerate (high to low) 4. Individual transaction weight (small to large) 5. Txid (low to high txid) The chunks within a cluster are sorted according to: 1. Topology (chunks after their dependencies) 2. Chunk feerate (high to low) 3. Chunk weight (small to large) 4. Max-txid (chunk with lowest maximum-txid first) The chunks across clusters are sorted according to: 1. Feerate (high to low) 2. Equal-feerate-chunk-prefix weight (small to large) 3. Max-txid (chunk with lowest maximum-txid first) The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order. ACKs for top commit: ajtowns: reACK 6f113cb it was good before and now it's better instagibbs: ACK 6f113cb marcofleon: light crACK 6f113cb Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
…pollution 65134c7 depends: Prefix include path for headers-only `systemtap` package (Hennadii Stepanov) 94a692b cmake: Add missed `USDT::headers` (Hennadii Stepanov) b5375c4 depends: Prefix include path for headers-only `boost` package (Hennadii Stepanov) d73378f cmake: Add missed `Boost::headers` (Hennadii Stepanov) Pull request description: Currently, header-only dependencies in the depends subsystem are installed into the standard `include/` subdirectory. This inadvertently exposes their headers to the compiler via `-I` flags brought in by other dependencies (e.g., `libevent` or `sqlite`). This "include path pollution" masks missing dependencies in the build configuration. While the build might succeed by accident due to this overlap, it creates a fragile state. If the overlapping library is removed, the build will break, or, worse, the compiler may silently fall back to the host system's default paths (e.g., `/usr/include`). This PR improves build system security and hygiene by enforcing strict, distinguished include paths for header-only dependencies. The missing dependencies revealed by this change (`Boost::headers`, `USDT::headers`) have been fixed in separate commits. ACKs for top commit: theuni: re-ACK 65134c7 fanquake: ACK 65134c7 Tree-SHA512: 41667b46c3bd2f872951a5651b30f7d1468f49f1265196b7868233ed44b2eb0e33f1f69a1af348b55f07a8d1f594e276eb49b724e80b8eae85aed1c9bacae197
Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com> Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Vasil Dimov <vd@freebsd.org>
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
b623fab mining: enforce minimum reserved weight for IPC (Sjors Provoost) d3e4952 mining: fix -blockreservedweight shadows IPC option (Sjors Provoost) 418b799 test: have mining template helpers return None (Sjors Provoost) Pull request description: Also enforce `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients. The `-blockreservedweight` startup option should only affect RPC code, because IPC clients (currently) do not have a way to signal their intent to use the node default (the `BlockCreateOptions` struct defaults merely document a recommendation for client software). Before this PR however, if the user set `-blockreservedweight` then `ApplyArgsManOptions` would cause the `block_reserved_weight` option passed by IPC clients to be ignored. _Users who don't set this value were not affected._ Fix this by making BlockCreateOptions::block_reserved_weight an std::optional. Internal interface users, such as the RPC call sites, don't set a value so -blockreservedweight is used. Whereas IPC clients do set a value which is no longer ignored. Test coverage is added, with a preliminary commit that refactors the `create_block_template` and `wait_next_template` helpers. `mining_basic.py` already ensured `-blockreservedweight` is enforced by mining RPC methods. The second commit adds coverage for Mining interface IPC clients. It also verifies that `-blockreservedweight` has no effect on them. The third commit enforces `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients. Previously lower values were quietly clamped. --- Merge order preference: #34452 should ideally go first. ACKs for top commit: sedited: Re-ACK b623fab ryanofsky: Code review ACK b623fab. Was rebased and test split up and comment updated since last review. ismaelsadeeq: ACK b623fab Tree-SHA512: 9e651a520d8e4aeadb330da86788744b6ecad8060fa21d50dc8e6012a60083e7b262aaa08a64676b9ef18ba65b651bc1272d8383d184030342e4c0f2c6a9866d
There should be no change in behavior Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Allow `expected_stderr` option passed to `wait_until_stopped` and `is_node_stopped` helper functions to be a regex pattern instead of just a fixed string. Allow `expected_ret_code` be list of possible exit codes instead of a single error code to handle the case where exit codes vary depending on OS and libc.
libmultiprocess currently handles uncaught exceptions from IPC methods badly when an `mp.Context` parameter is passed and the IPC call executes on an a worker thread, with the uncaught exception leading to a std::terminate call. bitcoin-core/libmultiprocess#218 was created to fix this, but before that change is available, update an IPC test which can trigger this behavior to handle it and recover when mp.Context parameters are added in the an upcoming commit. Having this workaround makes the test a little more complicated and less strict but reduces dependencies between pending PRs so they don't need to be reviewed or merged in a particular order.
This commit only declares constants without using them. They will be applied in seperate commit since changing struct default field values in cap'n proto is not backwards compatible.
This change copies default option values from the C++ mining interface to the Cap'n Proto interface. Currently, no capnp default values are set, so they are implicitly all false or 0, which is inconvenient for the rust and python clients and inconsistent with the C++ client. Warning: This is an intermediate, review-only commit. Binaries built from it should not be distributed or used to connect to other clients or servers. It makes incompatible changes to the `mining.capnp` schema without updating the `Init.makeMining` version, causing binaries to advertise support for a schema they do not actually implement. Mixed versions may therefore exchange garbage requests/responses instead of producing clear errors. The final commit in this series bumps the mining interface number to ensure mismatches are detected. git-bisect-skip: yes
This change removes deprecated methods from the ipc mining interface. Warning: This is an intermediate, review-only commit. Binaries built from it should not be distributed or used to connect to other clients or servers. It makes incompatible changes to the `mining.capnp` schema without updating the `Init.makeMining` version, causing binaries to advertise support for a schema they do not actually implement. Mixed versions may therefore exchange garbage requests/responses instead of producing clear errors. The final commit in this series bumps the mining interface number to ensure mismatches are detected. git-bisect-skip: yes
…le schema change) Adding a context parameter ensures that these methods are run in their own thread and don't block other calls. They were missing for: - createNewBlock() - checkBlock() The missing parameters were first pointed out by plebhash in #33575 (comment) and adding them should prevent possible performance problems and lockups, especially with #34184 which can make the createNewBlock method block for a long time before returning. It would be straightforward to make this change in a backward compatible way (#34184 (comment)) but nice to not need to go through the trouble. Warning: This is an intermediate, review-only commit. Binaries built from it should not be distributed or used to connect to other clients or servers. It makes incompatible changes to the `mining.capnp` schema without updating the `Init.makeMining` version, causing binaries to advertise support for a schema they do not actually implement. Mixed versions may therefore exchange garbage requests/responses instead of producing clear errors. The final commit in this series bumps the mining interface number to ensure mismatches are detected. git-bisect-skip: yes Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This increments the field number of the `Init.makeMining` method and makes the old `makeMining` method return an error, so existing IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface. Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well. Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly, so a few of these were implemented in commits immediately preceding this one. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
faba426 lint: Flatten lint image entry points (MarcoFalke) 1111fff lint: Add missing --platform=linux to docker build command (MarcoFalke) Pull request description: Two fixups to the lint container: * Add a missing `--platform=linux` to avoid running a non-native arch, like s390x, which happens with podman if such a container was most recently used. * Flatten the entry points to remove the bash-based one: Previously, an additional entry point into the container that spawned a bash was supported. The bash had an alias `lint` to run all lint scripts. However, such a use-case seems limited (because it only runs inside the container), inflexible (because it only allows running all lint scripts), and possibly brittle (because it can miss re-building the image when the cache is stale). So remove it and just offer the single entry point via the `./ci/lint.py` script. If there is a use-case to skip the image building, it should be trivial to add an env var setting the the lint Python script like `DANGER_SKIP_IMAGE_RE_BUILD=1` (or so) in the future. ACKs for top commit: willcl-ark: ACK faba426 Tree-SHA512: 9afda16723c215602c6c42fa3a286d1828c887c8f6ff9512c8ec162ec8997789695f0c464d389cae94e67acf8b5e0f1a55e2ee0d60131a2eee091cf281f91514
Introduce a helper to look up a Coin through a stack of CCoinsViewCache layers without populating parent caches. This is useful for ephemeral views (e.g. during ConnectBlock) that want to avoid polluting CoinsTip() when validating invalid blocks. Co-authored-by: l0rinc <pap.lorinc@gmail.com> Co-authored-by: Pieter Wuille <pieter@wuille.net> Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads coins without mutating the underlying cache via `FetchCoin()`. Use `PeekCoin()` to look up a Coin through a stack of `CCoinsViewCache` layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once `Flush()` is called on the view, these inputs will be added as spent to `coinsCache` in the main cache via `BatchWrite()`. This is the foundation for async input fetching, where worker threads must not mutate shared state. Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Improve dependencies.md to document IPC dependencies better: - Link to native_capnp.mk file not capnp.mk file so it's possible to see what version of Cap'n Proto is being used in release binaries. This is important since #31895 dropped the "Version Used" column and the capnp.mk file does not include version number. - Indicate Capn"Proto is used for IPC and link to multiprocess.md documenting the feature. - Link to correct PR requiring Cap'n Proto 0.7.1. Previous link was pointing at PR which required 0.7.0. - Mention libmultiprocess as a dependency even though it is included as a git subtree and can be built as a cmake subproject. Libmultiprocess still needs to be built separately when cross compiling, and is useful to build separately when developing, and is still a depends package. Based on 2cf352f from #33623 by willcl-ark which made similar changes in the 29.x branch.
Reduce the number of blocks that need to be generated before pruning the blockchain. Unload the wallet that was restored in a prior test because it is not needed anymore after the test. Both the above steps should reduce the number of chain notifications that need to be processed by the wallet(s) when an erroneous scenario of restoring wallet is checked.
Remove the unused functions that were ported many years back.
…e_broadcast.py da7f70a test: use port 0 for I2P addresses in p2p_private_broadcast.py (Vasil Dimov) a8ebcfd test: let connections happen in any order in p2p_private_broadcast.py (Vasil Dimov) 67696b2 net: extend log message to include attempted connection type (Vasil Dimov) Pull request description: If the following two events happen: * (likely) the automatic 10 initial connections are not made to all networks * (unlikely) the network-specific logic kicks in almost immediately. It is using exponential distribution with a mean of 5 minutes (`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`). So if both happen, then the 11th connection may not be the expected private broadcast, but a network-specific connection. Fix this by retrieving the connection type from `destinations_factory()`. This is more flexible because it allows connections to happen in any order and does not break if e.g. the 11th connection is not the expected first private broadcast. This also makes the test run faster: before: 19-44 sec now: 10-25 sec because for example there is no need to wait for the initial 10 automatic outbound connections to be made in order to proceed. Fixes: #34387 ACKs for top commit: achow101: ACK da7f70a andrewtoth: ACK da7f70a mzumsande: Code Review ACK da7f70a Tree-SHA512: 7c293e59c15c148a438e0119343b05eb278205640658c99336d4caf4848c5bae92b48e15f325fa616cbc9d5f394649abfa02406a76e802cffbd3d312a22a6885
Avoid race condition in run_deprecated_mining_test caused by creating and immediately destroying an unused worker thread. This leads to test failures reported by maflcko in #34711
…connecting chain notifications 98e8af4 wallet: Drain validation interface queue after notifications disconnect (Ava Chow) 52992eb interfaces: Add waitForNotifications() to call SyncWithValidationInterfaceQueue() (Ava Chow) Pull request description: When the wallet disconnects chain notifications, it is expecting no further notifications to execute, but this is not the case. This results in test failures such as in #34354. Instead of disconnecting the notifications and continuing shutdown, we should wait for the validation interface queue to be drained before the rest of wallet shutdown. This is achieved by adding an `interfaces::Chain::waitForNotifications()` function which calls `SyncWithValidationInterfaceQueue()`. Fixes #34354 ACKs for top commit: stickies-v: utACK 98e8af4 furszy: ACK 98e8af4 rkrux: crACK 98e8af4 sedited: ACK 98e8af4 Tree-SHA512: 263628556f740cb633d3970c22a0dfdb52a644bd1d0cd5a69c2970524edbb0e25d592cb39fc9bf1d0c281eebce09578526e2958dffee9026fc7473db35bd0dec
…ust IPC client 8fe91f3 test: Updates needed after bitcoin-core/libmultiprocess#240 (Ryan Ofsky) b7ca3bf Squashed 'src/ipc/libmultiprocess/' changes from 1fc65008f7d..1868a84451f (Ryan Ofsky) 1fea3ba ipc, test: Add tests for unclean disconnect and thread busy behavior (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#241 - bitcoin-core/libmultiprocess#240 - bitcoin-core/libmultiprocess#244 - bitcoin-core/libmultiprocess#245 The main change is bitcoin-core/libmultiprocess#240 which fixes issues with asynchronous requests (#33923) and unclean disconnects (#34250) that happen with the rust mining client. It also adds tests for these fixes which had some previous review in #34284 (that PR was closed to simplify dependencies between PRs). The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) Resolves #33923 and #34250 ACKs for top commit: Sjors: re-ACK 8fe91f3 janb84: reACK 8fe91f3 Eunovo: ACK 8fe91f3 Tree-SHA512: 7e8923610502ebd8603bbea703f82178ab9e956874d394da3451f5268afda2b964d0eeb399a74d49c4123e728a14c27c0296118577a6063ff03b2b8203a257ce
Otherwise this check will "pass", like: ```bash -- Detecting CXX compile features - done -- Found LLVM 21.1.8 -- Found clang-tidy: CLANG_TIDY_EXE-NOTFOUND -- Configuring done (0.5s) ```
```bash
15 | #warning The ClangTidyModuleRegistry.h header is deprecated and will be removed in LLVM 24. All of the symbols it used to define have been moved into ClangTidyModule.h.
| ^~~~~~~
[100%] Linking CXX shared module libbitcoin-tidy.so
```
Added -config-file as otherwise run-clang-tidy no-longer seemed able to find the config file.
5e35a90 interpreter: remove clang-tidy suppression (fanquake) 4089682 ci: use Clang 22 in tidy task (fanquake) 7ea076f tidy: remove deprecated header (fanquake) eb17f29 tidy: clang-tidy is required (fanquake) Pull request description: Changes needed for moving to Clang 22 in the tidy job. ACKs for top commit: maflcko: lgtm ACK 5e35a90 hebasto: ACK 5e35a90, I have reviewed the code and it looks OK. Tree-SHA512: 9ca6e841f7480b8abd78d5621d08a5bf80c2ff4facd7a0d76038ac1771bbf3d37dc2df19fa27583679177e4618db6294e2f2bb2129d9c25a53338b49ed71aac2
fa7612f ci: Download script_assets_test.json for Windows CI (MarcoFalke) 7777a13 test: Move Fetching-print to download_from_url util (MarcoFalke) faf9628 test: move-only download_from_url to stand-alone util file (MarcoFalke) fa0cc1c test: [doc] Remove outdated comment (MarcoFalke) Pull request description: Fixes #34670 by adding a new `download_script_assets` Python helper and calling it. ACKs for top commit: hebasto: re-ACK fa7612f. janb84: re ACK fa7612f hodlinator: utACK fa7612f Tree-SHA512: 73c2cb3a31f231174566fb880b82de92734b1679ef000f8d793d774b7e5f5a7b8c7994a3998ca78821115bdc80c16aada69cf596e92c083cf9b9a95c7cee16ea
b87a1c2 doc: Improve dependencies.md IPC documentation (Ryan Ofsky) Pull request description: Improve dependencies.md to document IPC dependencies better ([preview](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-depdoc/doc/dependencies.md#build-1)). Specific changes are listed in the commit message. This PR is based on #33623 by willcl-ark which made similar changes in the 29.x branch. This PR could also be backported to 30.x (it merges cleanly, and master and 30.x both have the same version requirements). ACKs for top commit: l0rinc: ACK b87a1c2 sedited: ACK b87a1c2 Tree-SHA512: 566b5372d189f0ad04f978ddefbd8c200dcc19b25af02c73275c5faf7529943680ec59703bda11640cf7920466b4cdd46305cac4d3770e2303de37694ae78909
…ference 1c1de33 test: avoid interface_ipc.py race and null pointer dereference (Ryan Ofsky) Pull request description: Avoid race condition in `run_deprecated_mining_test` caused by creating and immediately destroying an unused worker thread. This is leading to test failures reported by maflcko in #34711 ACKs for top commit: optout21: utACK 1c1de33 l0rinc: Tested ACK 1c1de33 w0xlt: ACK 1c1de33 enirox001: ACK 1c1de33 Tree-SHA512: d0af9676a46e991a3f4fda3795c02d1998d30de24991436b8ada425585c6699ff32a7057ca7a0ef2925f782fd3bf73e0381f5d4325e4f1c09f487fce1de49e45
…eserialization f51665b psbt: validate pubkeys in MuSig2 pubnonce/partial sig deserialization (tboy1337) Pull request description: The previous fix for invalid MuSig2 pubkeys (#34010) only addressed the PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS field. However, the PSBT_IN_MUSIG2_PUB_NONCE and PSBT_IN_MUSIG2_PARTIAL_SIG fields also deserialize pubkeys without validation, which could lead to crashes when invalid pubkeys are processed. This commit adds validation to the DeserializeMuSig2ParticipantDataIdentifier function to ensure all pubkeys in MuSig2 pubnonce and partial signature fields are fully valid elliptic curve points. The fix: - Validates both aggregate and participant pubkeys in MuSig2 pubnonce and partial signature deserialization - Throws std::ios_base::failure with descriptive error messages for invalid pubkeys - Prevents potential crashes from invalid elliptic curve points - Maintains backward compatibility for valid PSBTs This completes the fix for issues [#33999](#33999) and [#34201](#34201). ACKs for top commit: rkrux: lgtm ACK f51665b w0xlt: ACK f51665b darosior: utACK f51665b Tree-SHA512: 8454d77a05aa003a3121b0a5975e8a000125ee0d62343bfa625a75db113358ba7a210ae0376ca1666957b7de7005e06e5a54c95170430ee5e9e1416719b8af53
This will prevent us from creating a serialization we do not accept going forward.
faa68ed test: Fix intermittent issue in wallet_assumeutxo.py (MarcoFalke) Pull request description: The test has many issues: * It fails intermittently, due to the use of `-stopatheight` (#34710) * Using `-stopatheight` is expensive, because it requires two restarts, making the test slow * The overwritten `def setup_network` does not store the extra args via the `add_nodes` call, so if code is added in the future to restart a node, it may not pick up its global extra args. Fix all issues by: * Adding and using a fast `dumb_sync_blocks` util helper to achieve what `-stopatheight` doesn't achieve * Calling `self.add_nodes(self.num_nodes, self.extra_args)` in the overridden `setup_network` Can be tested via this diff: ```diff diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index ab0e5cc..49384c10b8 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -61,2 +61,3 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state if (m_stop_at_height && index.nHeight >= m_stop_at_height) { + LogInfo("Send shutdown signal after reaching stop height"); if (!m_shutdown_request()) { diff --git a/src/util/tokenpipe.cpp b/src/util/tokenpipe.cpp index c982fa6..a5565ebd36 100644 --- a/src/util/tokenpipe.cpp +++ b/src/util/tokenpipe.cpp @@ -4,2 +4,3 @@ #include <util/tokenpipe.h> +#include <util/time.h> @@ -60,2 +61,3 @@ int TokenPipeEnd::TokenRead() ssize_t result = read(m_fd, &token, 1); + UninterruptibleSleep(500ms); if (result < 0) { ``` On master: The test fails On this pull: The test passes Fixes #34710 ACKs for top commit: kevkevinpal: ACK [faa68ed](faa68ed) achow101: ACK faa68ed w0xlt: ACK faa68ed Tree-SHA512: 6fcd52b6f6a5fa5a5e41e7b3cf5c733af62af9c60271e7d22c266aca90f2af67f91ffe80a3ed8b8d1a91d001700870f493207998bac988c4e3671a3a0dba7ba7
d76ec4d fuzz: make sure PSBT serialization roundtrips (Antoine Poinsot) Pull request description: ~~Invalid public keys were accepted in Musig2 partial signatures. Because we serialize invalid keys as the empty byte string, this would lead us to creating an invalid PSBT serializations.~~ ~~This can be checked by reverting the first commit with the fix and simply running the target against the existing qa-assets corpus for the `psbt` harness.~~ This patch found the issue fixed in #34219 with a single run against the existing qa-assets corpus. It is useful to make sure there are no similar bugs, and we don't introduce roundtrip regressions outside of the specifc instance of accepting invalid public keys in Musig2 fields. *(Edited on March 4 to only contain the fuzz harness patch)* ACKs for top commit: davidgumberg: crACK d76ec4d achow101: ACK d76ec4d dergoegge: utACK d76ec4d brunoerg: code review ACK d76ec4d Tree-SHA512: ab5f8d4e6a1781ecdef825e1a0e2793a6b553f36c923a4a35cb1af4070eead9d9780f6cc9a76235aa03462e52a129d15e61f631490b43651dc4395f3f1c005f3
5c00536 test: improve `wallet_backup` test (rkrux) 04d9515 test: improve `wallet_assumeutxo` func test (rkrux) Pull request description: Relates to #34354 While the actual fix of the issue is in another PR, this one improves the affected tests by trying to reduce the chain notifications that need to be processed while simulating erroneous wallet restoration scenarios. ACKs for top commit: maflcko: lgtm ACK 5c00536 furszy: ACK 5c00536 w0xlt: ACK 5c00536 brunoerg: code review ACK 5c00536 Tree-SHA512: 176e3ea8275c7aa082af695f5b76d82c079ff9a7178855b4ce95504edb8ce89b59a772e2d38dd43e997a5bea3d64be700b74cfec7bbc6992538f837877ab7222
38a7a67 cmake: Provide `install_name_tool` stub instead of disabling it (Hennadii Stepanov) 80dc435 cmake: Apply workaround for `install_name_tool` conditionally (Hennadii Stepanov) Pull request description: We stopped using `install_name_tool` in 3bee514 (#29890), which [required](hebasto#180) a CMake-specific hack:https://github.com/bitcoin/bitcoin/blob/b65ff0e5a1fd4ea2ae75e204729b8008c4ebb9ab/CMakeLists.txt#L72-L76 Due to recent changes in CMake, this hack has become problematic for the following reasons: 1. It is no longer needed when using CMake 4.2 or newer. See upstream [Issue #27069](https://gitlab.kitware.com/cmake/cmake/-/issues/27069) and [MR #10955](https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10955). 2. It causes an [issue](#34513) when using CMake 4.0.5 or newer. See upstream [Issue #26814](https://gitlab.kitware.com/cmake/cmake/-/issues/26814) and [MR #10721](https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10721). This PR addresses both of these issues. Please see the individual commits for more details. Fixes #34513. ACKs for top commit: fanquake: ACK 38a7a67 Tree-SHA512: 7a11aa5b7f72034c798f20043ae89aa6c125bf5adf9c808a469aa30c2a56ad0a7aaf69919cb40a9eec08e029915890685968a9b7d5998e1337810f9cd2f6caf2
bff8a7a subprocess: replace __USING_WINDOWS__ with WIN32 (kevkevinpal) Pull request description: ## Summary Motivated by #34385 (review) In `subprocess.h` we are now renaming `__USING_WINDOWS__` with `WIN32` In the rest of the codebase, we are using `WIN32`, so it makes sense to update `subprocess.h` to match that. --- Use the following `grep` to assert there is no `__USING_WINDOWS__` in the codebase ``` grep -nri --exclude-dir=build "WIN32" ./ -I rep -nri --exclude-dir=build "__USING_WINDOWS__" ./ -I ``` ACKs for top commit: sedited: ACK bff8a7a hebasto: ACK bff8a7a, I have reviewed the code and it looks OK. Tree-SHA512: 18c3c8b87cf880053bbf69f837a0a135c5da51cfb15ab1d9fd554d8f931b2ea8202cf0f4d5e6f317d6234480128c2f41a7a1a9d9bd0504697a3c4c6a21797762
2a7a4f6 depends: Allow building Qt packages after interruption (Hennadii Stepanov) Pull request description: If a build is interrupted, a standard `mkdir` command may fail if the directory already exists. Switching to `mkdir -p` ensures the build can resume gracefully. Fix #34712. ACKs for top commit: sedited: ACK 2a7a4f6 Tree-SHA512: fad2dce89a7cb68a8a539924d98698fe650802d19c84f216fa65e3c23c1a33ab6acf9f4c98c27381194c2958efa92e9dd8fb5e6bd40098efbcc60f156fd45370
37d49f5 doc: mention Miniscript expressions inside reference (Pieter Wuille) 771f764 doc: fix typo in descriptors.md (Pieter Wuille) 708b849 doc: reference descriptor BIPs in descriptors.md (Pieter Wuille) 8f2a869 doc: do not list descriptor RPCs or history (Pieter Wuille) 65a8b6c doc: mention musig() in descriptors.md (Pieter Wuille) Pull request description: This brings doc/descriptors.md up to date: * Stop trying to exhaustively list all RPCs that involve descriptors. They're used everywhere. * Stop trying to give the history of descriptor support, we have release notes for that. * Mention that wallets are now built around descriptors (especially with legacy wallets gone). * Mention `musig()` KEY expressions in the specification part. * Mention the miniscript expressions in the specification part. * Reference the relevant output descriptor BIPs in the text. ACKs for top commit: achow101: ACK 37d49f5 darosior: utACK 37d49f5 Tree-SHA512: 2581be9b5d1a7099806d6f830b3a452505f8493d0e493a80b8a50e383f93f3e2c8a2d72a64fdae0adfe63d3c2eeb54a61a059108cd861e58c3d85f2bc576364b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Avance