💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3289368227)
Since most of the reviewers are in favour of using a single table instead of two tables for string and convert params. My recent commit does this; the implementation is motivated by the ryanofsky suggested commit [b998cc5](https://github.com/bitcoin/bitcoin/commit/b998cc52d51b48db9271fdba0bd69e9aaccb7999).
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3289368227)
Since most of the reviewers are in favour of using a single table instead of two tables for string and convert params. My recent commit does this; the implementation is motivated by the ryanofsky suggested commit [b998cc5](https://github.com/bitcoin/bitcoin/commit/b998cc52d51b48db9271fdba0bd69e9aaccb7999).
⚠️ fanquake pinned an issue: "30.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/33369)
This issue is to discuss the [30.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/30.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
Note: This is for feedback on the document, not on Bitcoin Core or on the 30.0 changes. Create a new issue or give feedback in the [v30.0 Testing](https://github.com/bitcoin/bitcoin/issues/33368) issue
Thank you for taking a look at the guide and leaving your feedbac
...
(https://github.com/bitcoin/bitcoin/issues/33369)
This issue is to discuss the [30.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/30.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
Note: This is for feedback on the document, not on Bitcoin Core or on the 30.0 changes. Create a new issue or give feedback in the [v30.0 Testing](https://github.com/bitcoin/bitcoin/issues/33368) issue
Thank you for taking a look at the guide and leaving your feedbac
...
⚠️ TheCharlatan opened an issue: "Assertion hit on shutdown of bitcoin-node with connected mining interface client"
(https://github.com/bitcoin/bitcoin/issues/33387)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Running c0894a0a2be032cd9a5d5945643689230ab10255 `bitcoin-node` with Sjor's [sv2-tp] connected resulted in an unclean shutdown:
```
2025-09-14T10:24:34Z CreateNewBlock(): block weight: 460455 txs: 371 fees: 230840 sigops 1336
2025-09-14T10:25:05Z CreateNewBlock(): block weight: 513153 txs: 441 fees: 290027 sigops 1489
2025-09-14T10:25:36Z CreateNewBlock(): block weight: 635048 txs: 541 f
...
(https://github.com/bitcoin/bitcoin/issues/33387)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Running c0894a0a2be032cd9a5d5945643689230ab10255 `bitcoin-node` with Sjor's [sv2-tp] connected resulted in an unclean shutdown:
```
2025-09-14T10:24:34Z CreateNewBlock(): block weight: 460455 txs: 371 fees: 230840 sigops 1336
2025-09-14T10:25:05Z CreateNewBlock(): block weight: 513153 txs: 441 fees: 290027 sigops 1489
2025-09-14T10:25:36Z CreateNewBlock(): block weight: 635048 txs: 541 f
...
✅ TheCharlatan closed an issue: "Assertion hit on shutdown of bitcoin-node with connected mining interface client"
(https://github.com/bitcoin/bitcoin/issues/33387)
(https://github.com/bitcoin/bitcoin/issues/33387)
💬 TheCharlatan commented on issue "Assertion hit on shutdown of bitcoin-node with connected mining interface client":
(https://github.com/bitcoin/bitcoin/issues/33387#issuecomment-3289467468)
Tried reproducing it again from scratch and could not. I think this was just a version mismatch. There is a crash on the sv2 side, but that is for a different issue tracker. Closing.
(https://github.com/bitcoin/bitcoin/issues/33387#issuecomment-3289467468)
Tried reproducing it again from scratch and could not. I think this was just a version mismatch. There is a crash on the sv2 side, but that is for a different issue tracker. Closing.
💬 jonasnick commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3289485882)
rc1 passes the nix-bitcoin integration test framework (except for the joinmarket tests because the latest joinmarket release requires the legacy wallet). You can run the tests with [this script](https://gist.github.com/jonasnick/cd52b4463b720a5fb1406c2cbabaf936) (requires nix).
It seems like Cap'n Proto is a new dependency. Should it be mentioned in the release notes?
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3289485882)
rc1 passes the nix-bitcoin integration test framework (except for the joinmarket tests because the latest joinmarket release requires the legacy wallet). You can run the tests with [this script](https://gist.github.com/jonasnick/cd52b4463b720a5fb1406c2cbabaf936) (requires nix).
It seems like Cap'n Proto is a new dependency. Should it be mentioned in the release notes?
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347326031)
Currently we trigger an assertion failure if a passed in index is out of bounds when we have a function that allows the caller to first check the allowed range. In the chain data structure, we don't have a direct function for this at the moment, the user is forced to first call `btck_chain_get_tip` and then call `btck_chain_get_by_height`. I think we should probably add another function there to get the height of the chain directly to make the api a bit more symmetric. We could be a bit friendli
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347326031)
Currently we trigger an assertion failure if a passed in index is out of bounds when we have a function that allows the caller to first check the allowed range. In the chain data structure, we don't have a direct function for this at the moment, the user is forced to first call `btck_chain_get_tip` and then call `btck_chain_get_by_height`. I think we should probably add another function there to get the height of the chain directly to make the api a bit more symmetric. We could be a bit friendli
...
💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2347345010)
This works:
```c++
std::sort(iters.begin(), iters.end(), [this](const auto& a, const auto& b) EXCLUSIVE_LOCKS_REQUIRED(cs) noexcept {
return this->m_txgraph->CompareMainOrder(*a, *b) < 0;
});
```
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2347345010)
This works:
```c++
std::sort(iters.begin(), iters.end(), [this](const auto& a, const auto& b) EXCLUSIVE_LOCKS_REQUIRED(cs) noexcept {
return this->m_txgraph->CompareMainOrder(*a, *b) < 0;
});
```
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347348366)
It is not clear to me what other mutable data structures we can have in the future, but at least for the chain it seems better to return nullptr than allow potential crashes in race conditions (though I am not sure how often it could happen in practice).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347348366)
It is not clear to me what other mutable data structures we can have in the future, but at least for the chain it seems better to return nullptr than allow potential crashes in race conditions (though I am not sure how often it could happen in practice).
💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3289572395)
See https://github.com/sipa/bitcoin/commits/pr28676 for a branch with:
* Rebase after #33354 merge.
* Updated #33157 as base.
* Addressed https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328805583
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3289572395)
See https://github.com/sipa/bitcoin/commits/pr28676 for a branch with:
* Rebase after #33354 merge.
* Updated #33157 as base.
* Addressed https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328805583
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347350134)
Yes (I also meant for `btck_logging_set_level_category`).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347350134)
Yes (I also meant for `btck_logging_set_level_category`).
💬 stickies-v commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347370849)
Is there a reason you’re using `kernel_cache_sizes.coins` instead of just inspecting `-dbcache`? Using the latter is more in line with the what we’re telling the user we’re doing, and makes the log in line with the value the user actually provided.
Also, would prefer carving this out into separate function to minimize bloating an already long init sequence.
Finally, nit: using `DEFAULT_DB_CACHE` instead of `DEFAULT_KERNEL_CACHE` seems more appropriate, even if they currently have the same
...
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347370849)
Is there a reason you’re using `kernel_cache_sizes.coins` instead of just inspecting `-dbcache`? Using the latter is more in line with the what we’re telling the user we’re doing, and makes the log in line with the value the user actually provided.
Also, would prefer carving this out into separate function to minimize bloating an already long init sequence.
Finally, nit: using `DEFAULT_DB_CACHE` instead of `DEFAULT_KERNEL_CACHE` seems more appropriate, even if they currently have the same
...
💬 stickies-v commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347370905)
It's a bit longer, but I find using a commonly used std lib function over a one-off macro more readable and more safe, but no big deal either way.
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347370905)
It's a bit longer, but I find using a commonly used std lib function over a one-off macro more readable and more safe, but no big deal either way.
💬 jesterhodl commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3289763460)
> An internal name.
How about `uacomments` in `Total length of network version string () exceeds maximum length (). Reduce the number or size of uacomments.`
_msg1028
Similar to WalletDescriptor
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3289763460)
> An internal name.
How about `uacomments` in `Total length of network version string () exceeds maximum length (). Reduce the number or size of uacomments.`
_msg1028
Similar to WalletDescriptor
💬 ryanofsky commented on issue "Assertion hit on shutdown of bitcoin-node with connected mining interface client":
(https://github.com/bitcoin/bitcoin/issues/33387#issuecomment-3289801227)
Nice find, I think this probably is a real bug. I wouldn't expect it to happen normally because there is an `Ipc::disconnectIncoming()` call in `Shutdown()` to close all IPC connections before the chainman object is freed. However, the `disconnectIncoming()` implementation is only closing the IPC connections without waiting for the objects and threads associated with them to be freed. So if an IPC client makes a request and the node receives it, and the node starts to shut down before the reques
...
(https://github.com/bitcoin/bitcoin/issues/33387#issuecomment-3289801227)
Nice find, I think this probably is a real bug. I wouldn't expect it to happen normally because there is an `Ipc::disconnectIncoming()` call in `Shutdown()` to close all IPC connections before the chainman object is freed. However, the `disconnectIncoming()` implementation is only closing the IPC connections without waiting for the objects and threads associated with them to be freed. So if an IPC client makes a request and the node receives it, and the node starts to shut down before the reques
...
💬 151henry151 commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3289823893)
## v30.0rc1 Testing
Tested on Debian 12 (bookworm) with kernel 6.1.0-37-amd64, 4-core x86_64 system. Ran through all the features from the testing guide:
- Datacarrier size: Created a transaction with 83 bytes of data in an OP_RETURN output and successfully sent it
- bitcoin command: Used `bitcoin node` to start a node and `bitcoin rpc` to make RPC calls - unified interface works well
- Bumpfee: Created a transaction and successfully used `bumpfee` with `replaceable: false`
- TRUC: Created rep
...
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3289823893)
## v30.0rc1 Testing
Tested on Debian 12 (bookworm) with kernel 6.1.0-37-amd64, 4-core x86_64 system. Ran through all the features from the testing guide:
- Datacarrier size: Created a transaction with 83 bytes of data in an OP_RETURN output and successfully sent it
- bitcoin command: Used `bitcoin node` to start a node and `bitcoin rpc` to make RPC calls - unified interface works well
- Bumpfee: Created a transaction and successfully used `bumpfee` with `replaceable: false`
- TRUC: Created rep
...
🤔 l0rinc reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3222272959)
Thanks a lot for the comments and reproducers, added a @hodlinator and @stickies-v as coauthors.
I have split the PR into 3 commits, explained the descisions in the messages (cc: @willcl-ark), added unit tests for whatever I could extract, extracted the warning message to a method and exposed the warning for RPC (thanks @stickies-v).
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3222272959)
Thanks a lot for the comments and reproducers, added a @hodlinator and @stickies-v as coauthors.
I have split the PR into 3 commits, explained the descisions in the messages (cc: @willcl-ark), added unit tests for whatever I could extract, extracted the warning message to a method and exposed the warning for RPC (thanks @stickies-v).
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347458082)
If any of you feel strongly, I can change it, but it was a deliberate choice from my part.
See https://godbolt.org/z/o3sKqefT5 for an ugly error on Windows that also convinced me to just use the shorter version. I could of course hack it with:
```C++
#ifdef _WIN32
#include <windows.h>
#undef max
#else
```
or just hope that https://github.com/bitcoin/bitcoin/blob/9f744fffc39d93c9966dec1d61f113a7521983ad/CMakeLists.txt#L261 fixes it, but `SIZE_MAX` seemed simpler and shorter in this case :/
-
...
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347458082)
If any of you feel strongly, I can change it, but it was a deliberate choice from my part.
See https://godbolt.org/z/o3sKqefT5 for an ugly error on Windows that also convinced me to just use the shorter version. I could of course hack it with:
```C++
#ifdef _WIN32
#include <windows.h>
#undef max
#else
```
or just hope that https://github.com/bitcoin/bitcoin/blob/9f744fffc39d93c9966dec1d61f113a7521983ad/CMakeLists.txt#L261 fixes it, but `SIZE_MAX` seemed simpler and shorter in this case :/
-
...
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347560408)
Added this in a separate commit (I'm not on my phone anymore :p), reviewers can check the commit message for instructions for how to reproduce it
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347560408)
Added this in a separate commit (I'm not on my phone anymore :p), reviewers can check the commit message for instructions for how to reproduce it
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347457548)
> using kernel_cache_sizes.coins instead of just inspecting -dbcache
* dbcache isn't always set (we'd duplicate work by setting the default explicitly). But we already have a duplicate strategy for very small memory environments, so this should be fine.
* `dbcache` is split to other indexes, this is the final value that is actually being used for dbcache. But you could argue that for total memory that's irrelevant.
> prefer carving this out into separate function
Agree
> using DEFAULT_DB_CAC
...
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347457548)
> using kernel_cache_sizes.coins instead of just inspecting -dbcache
* dbcache isn't always set (we'd duplicate work by setting the default explicitly). But we already have a duplicate strategy for very small memory environments, so this should be fine.
* `dbcache` is split to other indexes, this is the final value that is actually being used for dbcache. But you could argue that for total memory that's irrelevant.
> prefer carving this out into separate function
Agree
> using DEFAULT_DB_CAC
...