📝 PiRK opened a pull request: "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage"
(https://github.com/bitcoin/bitcoin/pull/33381)
If the random coin in this test has a non-null scriptpubkey, `cache->SanityCheck();` fails an assertion because the `BOOST_CHECK_THROW` line leaves `CCoinsViewCache::cachedCoinsUsage` in a corrupted state: the erroring `AddCoin` call decrements the value to 0 even though the coin is still in the cache, then the next `SpendCoin` call decrements the value again causing a `size_t` underflow.
Move the `BOOST_CHECK_THROW` line to the very end of the test so it does not break following tests.
(https://github.com/bitcoin/bitcoin/pull/33381)
If the random coin in this test has a non-null scriptpubkey, `cache->SanityCheck();` fails an assertion because the `BOOST_CHECK_THROW` line leaves `CCoinsViewCache::cachedCoinsUsage` in a corrupted state: the erroring `AddCoin` call decrements the value to 0 even though the coin is still in the cache, then the next `SpendCoin` call decrements the value again causing a `size_t` underflow.
Move the `BOOST_CHECK_THROW` line to the very end of the test so it does not break following tests.
💬 BenWestgate commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2345324305)
Okay, I've updated the tutorial to use `bitcoin rpc` everywhere possible (getcoins.py won't accept `bitcoin rpc -signet` yet).
I also fixed two bugs where we forgot to specify `-signet`.
https://github.com/bitcoin/bitcoin/pull/33286/commits/3d42607fb5966d8e3b79fdb878d59483432625d9
https://github.com/bitcoin/bitcoin/pull/33286/commits/3d42607fb5966d8e3b79fdb878d59483432625d9
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2345324305)
Okay, I've updated the tutorial to use `bitcoin rpc` everywhere possible (getcoins.py won't accept `bitcoin rpc -signet` yet).
I also fixed two bugs where we forgot to specify `-signet`.
https://github.com/bitcoin/bitcoin/pull/33286/commits/3d42607fb5966d8e3b79fdb878d59483432625d9
https://github.com/bitcoin/bitcoin/pull/33286/commits/3d42607fb5966d8e3b79fdb878d59483432625d9
💬 PiRK commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3286670785)
I force pushed to fix a use-after-move bug
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3286670785)
I force pushed to fix a use-after-move bug
✅ Raimo33 closed a pull request: "[WIP] cache: remove redundant find() call"
(https://github.com/bitcoin/bitcoin/pull/33376)
(https://github.com/bitcoin/bitcoin/pull/33376)
💬 Raimo33 commented on pull request "[WIP] cache: remove redundant find() call":
(https://github.com/bitcoin/bitcoin/pull/33376#issuecomment-3286747889)
Following up in https://github.com/l0rinc/bitcoin/pull/34/commits/b23c6a1fc8f9a44561add79427d2f3fff4c3718f as reviewer/coauthor
(https://github.com/bitcoin/bitcoin/pull/33376#issuecomment-3286747889)
Following up in https://github.com/l0rinc/bitcoin/pull/34/commits/b23c6a1fc8f9a44561add79427d2f3fff4c3718f as reviewer/coauthor
💬 hodlinator commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3286779725)
(I'd be in slight favor of changing raw arrays to `std::array` as done in https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 since some debug builds in CI run with extra bounds-checking for std-types. Although maybe MSAN builds already catch out of bounds access for raw arrays, and I don't suspect we would detect any new issues for `base_uint`/`arith_uint256`).
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3286779725)
(I'd be in slight favor of changing raw arrays to `std::array` as done in https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 since some debug builds in CI run with extra bounds-checking for std-types. Although maybe MSAN builds already catch out of bounds access for raw arrays, and I don't suspect we would detect any new issues for `base_uint`/`arith_uint256`).
💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3286813683)
same. i think https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 is worth a PR even if it's a minor change. Maybe you can pack it with other commits where `std::array` is needed.
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3286813683)
same. i think https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 is worth a PR even if it's a minor change. Maybe you can pack it with other commits where `std::array` is needed.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345431507)
I think docs should be updated for these two about "not being thread safe" now that we have `LOCK(cs_main)` calls in the implementations?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345431507)
I think docs should be updated for these two about "not being thread safe" now that we have `LOCK(cs_main)` calls in the implementations?
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345384878)
Is there a reason `cs_main` isn't acquired for the first two functions?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345384878)
Is there a reason `cs_main` isn't acquired for the first two functions?
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345483298)
Also noticed in this part that the two functions alter the same global value (`m_categories`) which happens to be atomic. We have acquired `cs_main` for one of them (but not the other?). Since the global value is already updated atomically, I assume locking `cs_main` serves a different purpose? In general, the `cs_main` locks for logging functions are a bit unclear to me. I would appreciate it if you could elaborate a bit on what you had in mind when adding them.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345483298)
Also noticed in this part that the two functions alter the same global value (`m_categories`) which happens to be atomic. We have acquired `cs_main` for one of them (but not the other?). Since the global value is already updated atomically, I assume locking `cs_main` serves a different purpose? In general, the `cs_main` locks for logging functions are a bit unclear to me. I would appreciate it if you could elaborate a bit on what you had in mind when adding them.
💬 mzumsande commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3286964369)
Maybe covered by the previous comments, but running `test_bitcoin` on master now prints out a warning, caused by `node_init_tests`:
```
Running 671 test cases...
Warning: Disk space for "/tmp/test_common bitcoin/node_init_tests/init_test/bf78678cb7723a3e84b5/blocks" may not accommodate the block files. Approximately 810 GB of data will be stored in this directory.
```
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3286964369)
Maybe covered by the previous comments, but running `test_bitcoin` on master now prints out a warning, caused by `node_init_tests`:
```
Running 671 test cases...
Warning: Disk space for "/tmp/test_common bitcoin/node_init_tests/init_test/bf78678cb7723a3e84b5/blocks" may not accommodate the block files. Approximately 810 GB of data will be stored in this directory.
```
🤔 hodlinator reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3218940692)
Concept ACK
Confirmed to work on Windows:
```
$ ./build/bin/Release/bitcoind.exe -regtest -dbcache=64534
2025-09-12T22:14:09Z Bitcoin Core version v29.99.0-080a21cb127f-dirty (release build)
...
2025-09-12T22:14:09Z [warning] A 64524 MiB dbcache may be too large for a system memory of only 65413 MiB.
```
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3218940692)
Concept ACK
Confirmed to work on Windows:
```
$ ./build/bin/Release/bitcoind.exe -regtest -dbcache=64534
2025-09-12T22:14:09Z Bitcoin Core version v29.99.0-080a21cb127f-dirty (release build)
...
2025-09-12T22:14:09Z [warning] A 64524 MiB dbcache may be too large for a system memory of only 65413 MiB.
```
📝 caesrcd opened a pull request: "Bash completion for bitcoin(1) wrapper"
(https://github.com/bitcoin/bitcoin/pull/33382)
Uses `bitcoin --help` / `bitcoin help` to list available commands and options for the `bitcoin` wrapper.
Integrates completions for `bitcoind`, `bitcoin-cli`, and `bitcoin-tx`.
Placed under `contrib/` so it can be installed or used easily by distros/users who want completion.
(https://github.com/bitcoin/bitcoin/pull/33382)
Uses `bitcoin --help` / `bitcoin help` to list available commands and options for the `bitcoin` wrapper.
Integrates completions for `bitcoind`, `bitcoin-cli`, and `bitcoin-tx`.
Placed under `contrib/` so it can be installed or used easily by distros/users who want completion.
💬 davidgumberg commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3287201170)
I am [skeptical](https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2655685417) of trying to measure end-to-end performance of Bitcoin Core systems like block connection including LevelDB performance using a microbenchmark suite. In my opinion, these benchmarks work best at measuring the performance of specific components and functions a-la unit tests, and I would not object to adding more scenarios to `ConnectBlock()` that are meant to exercise the performance of certain segments of blo
...
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3287201170)
I am [skeptical](https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2655685417) of trying to measure end-to-end performance of Bitcoin Core systems like block connection including LevelDB performance using a microbenchmark suite. In my opinion, these benchmarks work best at measuring the performance of specific components and functions a-la unit tests, and I would not object to adding more scenarios to `ConnectBlock()` that are meant to exercise the performance of certain segments of blo
...
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2345636999)
I was likely thinking of cases where the max tx fee is set below the relay fee rate for any standard tx size but still above the dust limit (iff this is ever possible..).
I assume this already fails on the feerate checks you added, but it might make sense to disallow such a max tx fee in the first place.
It seems we could compute the feerate for the smallest possible standard tx the wallet could create and reject a max tx fee that would fall below the relay feerate.
---------------
T
...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2345636999)
I was likely thinking of cases where the max tx fee is set below the relay fee rate for any standard tx size but still above the dust limit (iff this is ever possible..).
I assume this already fails on the feerate checks you added, but it might make sense to disallow such a max tx fee in the first place.
It seems we could compute the feerate for the smallest possible standard tx the wallet could create and reject a max tx fee that would fall below the relay feerate.
---------------
T
...
📝 caesrcd opened a pull request: "Bash completion for the new bitcoin wrapper command"
(https://github.com/bitcoin/bitcoin/pull/33383)
This PR adds a Bash completion script for the new `bitcoin` command-line tool introduced in #31375. The `bitcoin` wrapper provides a unified interface for Bitcoin Core executables, making features more discoverable and convenient. It does **not implement functionality itself**, but acts as a front-end to existing tools:
* `bitcoin node` → `bitcoind`
* `bitcoin gui` → `bitcoin-qt`
* `bitcoin rpc` → `bitcoin-cli -named`
The completion script parses `bitcoin --help` and `bitcoin help` to dy
...
(https://github.com/bitcoin/bitcoin/pull/33383)
This PR adds a Bash completion script for the new `bitcoin` command-line tool introduced in #31375. The `bitcoin` wrapper provides a unified interface for Bitcoin Core executables, making features more discoverable and convenient. It does **not implement functionality itself**, but acts as a front-end to existing tools:
* `bitcoin node` → `bitcoind`
* `bitcoin gui` → `bitcoin-qt`
* `bitcoin rpc` → `bitcoin-cli -named`
The completion script parses `bitcoin --help` and `bitcoin help` to dy
...
💬 caesrcd commented on pull request "Bash completion for the new bitcoin wrapper command":
(https://github.com/bitcoin/bitcoin/pull/33383#issuecomment-3287424966)
Hi,
DrahtBot closed this PR automatically (similar to my previous attempt in #33382),
but I have since updated the commit with a detailed and properly formatted message: caesrcd/bitcoin@f03423f
Could this PR please be reopened for review?
Thank you!
(https://github.com/bitcoin/bitcoin/pull/33383#issuecomment-3287424966)
Hi,
DrahtBot closed this PR automatically (similar to my previous attempt in #33382),
but I have since updated the commit with a detailed and properly formatted message: caesrcd/bitcoin@f03423f
Could this PR please be reopened for review?
Thank you!
💬 hodlinator commented on pull request "p2p: Correct unrealistic headerssync unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3287698877)
Latest push resolves conflict with #33332.
(Also changed PR-title (category) from "headerssync:" to "p2p:" based off https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/config.yml and the fact that #25717 used that topic. Maybe one could advocate for expanding the number of categories, but I'll punt on it for now. Hat-tip to l0rinc for the DrahtBot reference).
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3287698877)
Latest push resolves conflict with #33332.
(Also changed PR-title (category) from "headerssync:" to "p2p:" based off https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/config.yml and the fact that #25717 used that topic. Maybe one could advocate for expanding the number of categories, but I'll punt on it for now. Hat-tip to l0rinc for the DrahtBot reference).
💬 fanquake commented on pull request "Bash completion for the new bitcoin wrapper command":
(https://github.com/bitcoin/bitcoin/pull/33383#issuecomment-3287721141)
@caesrcd It's no-longer possible to reopen this PR, as you've pushed to the branch since it was closed. You'll need to open a new (third) one. Apologies about the bot. You can see the kind of heuristics it's using to determine PR closures, here: https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/src/features/spam_detection.rs#L118.
(https://github.com/bitcoin/bitcoin/pull/33383#issuecomment-3287721141)
@caesrcd It's no-longer possible to reopen this PR, as you've pushed to the branch since it was closed. You'll need to open a new (third) one. Apologies about the bot. You can see the kind of heuristics it's using to determine PR closures, here: https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/src/features/spam_detection.rs#L118.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346045827)
Initially it was not needed, because we always retrieve it from a chainstate manager with a loaded chainstate, meaning the chain will never be empty, so there is no possible race condition. I think it might be more defensive to take the lock now, since we could permit `Chain` data structures that are empty in the future. In the `get_by_height` case we take the lock such that there is no race condition between the assert and retrieving the entry.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346045827)
Initially it was not needed, because we always retrieve it from a chainstate manager with a loaded chainstate, meaning the chain will never be empty, so there is no possible race condition. I think it might be more defensive to take the lock now, since we could permit `Chain` data structures that are empty in the future. In the `get_by_height` case we take the lock such that there is no race condition between the assert and retrieving the entry.