💬 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
...
💬 l0rinc commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3290237829)
@Raimo33 if you can provide an alternative that's reasonably simple for a micro-benchmark, I will happily review it.
But note that I already have a few micro-benchmark improvements in that area where review is needed:
* https://github.com/bitcoin/bitcoin/pull/32554 - creates a configurable block so that we can measure the composition of the block
* https://github.com/bitcoin/bitcoin/pull/32729/files#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R40 - splits checkblock mea
...
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3290237829)
@Raimo33 if you can provide an alternative that's reasonably simple for a micro-benchmark, I will happily review it.
But note that I already have a few micro-benchmark improvements in that area where review is needed:
* https://github.com/bitcoin/bitcoin/pull/32554 - creates a configurable block so that we can measure the composition of the block
* https://github.com/bitcoin/bitcoin/pull/32729/files#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R40 - splits checkblock mea
...
💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3290241610)
+1 for https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 but without the explicit stuff, it's just noise, let the compiler do the work. We can add tests or static assertions instead of making simple structures more complicated - there's a reason the compiler is willing to generate it for us.
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3290241610)
+1 for https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 but without the explicit stuff, it's just noise, let the compiler do the work. We can add tests or static assertions instead of making simple structures more complicated - there's a reason the compiler is willing to generate it for us.
💬 l0rinc commented on pull request "p2p: Correct unrealistic headerssync unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3290253231)
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
The only difference after the rebase is a removal of a `constexpr` in one of the `base_uint` constructors.
> changed PR-title (category) from "headerssync:" to "p2p
nit: the commits still state `headerssync` (fine by me, just noticed)
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3290253231)
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
The only difference after the rebase is a removal of a `constexpr` in one of the `base_uint` constructors.
> changed PR-title (category) from "headerssync:" to "p2p
nit: the commits still state `headerssync` (fine by me, just noticed)
💬 hMsats commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3290439495)
Not sure if this is useful but anyway: Compiled bitcoin-30.0rc1 (it's called v30.0.0rc1 in debug.log) with cmake on Ubuntu 24.04.3 LTS on an Acer Aspire E1-572, 64 bits laptop with a 4 TB external ssd. All tests (test_bitcoin, test_bitcoin-qt) passed for both bitcoind and bitcoin-qt and bitcoin-qt looks fine. Upgraded bitcoind from version 29.0 to 30.0rc1. Bitcoind (30.0rc1) is working great (txindex=1) with low block verification times (about 1 second for "Saw new cmpctblock header" to "Update
...
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3290439495)
Not sure if this is useful but anyway: Compiled bitcoin-30.0rc1 (it's called v30.0.0rc1 in debug.log) with cmake on Ubuntu 24.04.3 LTS on an Acer Aspire E1-572, 64 bits laptop with a 4 TB external ssd. All tests (test_bitcoin, test_bitcoin-qt) passed for both bitcoind and bitcoin-qt and bitcoin-qt looks fine. Upgraded bitcoind from version 29.0 to 30.0rc1. Bitcoind (30.0rc1) is working great (txindex=1) with low block verification times (about 1 second for "Saw new cmpctblock header" to "Update
...