💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534984229)
This retrieves the best known header at a time. If you are syncing headers from a peer that seems like useful information to have?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534984229)
This retrieves the best known header at a time. If you are syncing headers from a peer that seems like useful information to have?
📝 Sjors opened a pull request: "mining: add requestedOutputs field, e.g. for merged mining "
(https://github.com/bitcoin/bitcoin/pull/33890)
This adds a `requestedOutputs` field to `BlockCreateOptions` which lets the Mining IPC client request outputs to be added before the consensus mandatory commitments.
The main use case is being able to add `OP_RETURN` outputs for merged mining without patching `BlockAssembler::CreateNewBlock()`.
It could potentially also be used to make payouts directly from the coinbase, but this is unsafe for any amount above the subsidy (this PR adds checks for that). In general a pool should add payout
...
(https://github.com/bitcoin/bitcoin/pull/33890)
This adds a `requestedOutputs` field to `BlockCreateOptions` which lets the Mining IPC client request outputs to be added before the consensus mandatory commitments.
The main use case is being able to add `OP_RETURN` outputs for merged mining without patching `BlockAssembler::CreateNewBlock()`.
It could potentially also be used to make payouts directly from the coinbase, but this is unsafe for any amount above the subsidy (this PR adds checks for that). In general a pool should add payout
...
💬 hebasto commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3543162626)
> i'm trying to do a guix build (of the master branch) on RISC-V hardware. The device is a [Scaleway EM-RV1-C4M16S128-A](https://labs.scaleway.com/en/em-rv1/) server from Scaleway.
It builds fine for me on an instance from the same provider:
```
# guix describe
Generation 3 Nov 17 2025 17:45:37 (current)
guix 5cb84f2
repository URL: https://git.guix.gnu.org/guix.git
commit: 5cb84f2013c5b1e48a7d0e617032266f1e6059e2
# guix build python-py-cpuinfo
/gnu/store/v09j8kcdxci3nbrjrqz7gik0sf5rm
...
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3543162626)
> i'm trying to do a guix build (of the master branch) on RISC-V hardware. The device is a [Scaleway EM-RV1-C4M16S128-A](https://labs.scaleway.com/en/em-rv1/) server from Scaleway.
It builds fine for me on an instance from the same provider:
```
# guix describe
Generation 3 Nov 17 2025 17:45:37 (current)
guix 5cb84f2
repository URL: https://git.guix.gnu.org/guix.git
commit: 5cb84f2013c5b1e48a7d0e617032266f1e6059e2
# guix build python-py-cpuinfo
/gnu/store/v09j8kcdxci3nbrjrqz7gik0sf5rm
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2535036758)
Even simpler would be to just rely on the fact that the dummy output is always at index 0. I didn't want to make that assumption in sv2-tp, but in our codebase it should be fine.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2535036758)
Even simpler would be to just rely on the fact that the dummy output is always at index 0. I didn't want to make that assumption in sv2-tp, but in our codebase it should be fine.
💬 plebhash commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3543241173)
> @plebhash can you try to see if it's easy for you to support both `getCoinbase()` and `getCoinbaseTx()` in [stratum-mining/sv2-apps#59](https://github.com/stratum-mining/sv2-apps/pull/59), preferring the first and falling back to the latter if it doesn't exist?
ok @Sjors second answer to this: currently, [`bitcoin-capnp-types`](https://github.com/2140-dev/bitcoin-capnp-types/blob/master/capnp/mining.capnp) only provides `getCoinbaseTx`
we would need @rustaceanrob to replicate what you ha
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3543241173)
> @plebhash can you try to see if it's easy for you to support both `getCoinbase()` and `getCoinbaseTx()` in [stratum-mining/sv2-apps#59](https://github.com/stratum-mining/sv2-apps/pull/59), preferring the first and falling back to the latter if it doesn't exist?
ok @Sjors second answer to this: currently, [`bitcoin-capnp-types`](https://github.com/2140-dev/bitcoin-capnp-types/blob/master/capnp/mining.capnp) only provides `getCoinbaseTx`
we would need @rustaceanrob to replicate what you ha
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535106877)
Missed to fully answer your previous comment related to this change, sorry. I didn't do it to not include `setup_common.h`, as it comes with way more dependencies than just `HasReason()`.
The idea is to try to reduce large Bitcoin-Core repo dependencies in this low level class, mainly when they do not add much value (this one just let us remove two lines of code with no behavior change), so this class and tests can be easily used elsewhere.
We could move `HasReason()` to another test utils f
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535106877)
Missed to fully answer your previous comment related to this change, sorry. I didn't do it to not include `setup_common.h`, as it comes with way more dependencies than just `HasReason()`.
The idea is to try to reduce large Bitcoin-Core repo dependencies in this low level class, mainly when they do not add much value (this one just let us remove two lines of code with no behavior change), so this class and tests can be easily used elsewhere.
We could move `HasReason()` to another test utils f
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3543318128)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
I left a few nits and follow-up ideas, but the code is correct as it is, every suggestion can be applied in a later PR.
Especially since making the methods `static` enables eliminating a few parameters along the call chain - which are orthogonal to this change.
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3543318128)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
I left a few nits and follow-up ideas, but the code is correct as it is, every suggestion can be applied in a later PR.
Especially since making the methods `static` enables eliminating a few parameters along the call chain - which are orthogonal to this change.
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534906456)
> nor does BlockManager instance retain references to the parameters
First I though we might as well make it `const` to add more weight to the claim that `LIFETIMEBOUND` isn't needed here:
```suggestion
bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
```
But it turns out the internally called `GetFirstBlock` can actually be `static` (which would document `LIFETIMEBOUND` a lot better, since w
...
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534906456)
> nor does BlockManager instance retain references to the parameters
First I though we might as well make it `const` to add more weight to the claim that `LIFETIMEBOUND` isn't needed here:
```suggestion
bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
```
But it turns out the internally called `GetFirstBlock` can actually be `static` (which would document `LIFETIMEBOUND` a lot better, since w
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534972779)
given that call-site `CHECK_NONFATAL`, can we simplify this to:
```suggestion
return *Assert(last_block);
```
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534972779)
given that call-site `CHECK_NONFATAL`, can we simplify this to:
```suggestion
return *Assert(last_block);
```
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534940354)
Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where [`lower_block`](https://github.com/bitcoin/bitcoin/pull/27607/files#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR413) was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534940354)
Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where [`lower_block`](https://github.com/bitcoin/bitcoin/pull/27607/files#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR413) was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535004423)
nit: `first_available_block` is also a non-nullable reference, please consider this alternative if you touch again:
```C++
CBlockIndex& first_available_block = *chainman->ActiveChain()[height_to_prune + 1];
CBlockIndex* last_pruned_block = first_available_block.pprev;
func_prune_blocks(last_pruned_block);
// 3) The last block not pruned is in-between upper-block and the genesis block
BOOST_CHECK_EQUAL(&BlockManager::GetFirstBlock(tip, BLOCK_HAVE_DATA), &first_available_block);
BOOST_CHE
...
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535004423)
nit: `first_available_block` is also a non-nullable reference, please consider this alternative if you touch again:
```C++
CBlockIndex& first_available_block = *chainman->ActiveChain()[height_to_prune + 1];
CBlockIndex* last_pruned_block = first_available_block.pprev;
func_prune_blocks(last_pruned_block);
// 3) The last block not pruned is in-between upper-block and the genesis block
BOOST_CHECK_EQUAL(&BlockManager::GetFirstBlock(tip, BLOCK_HAVE_DATA), &first_available_block);
BOOST_CHE
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535010215)
While there are no direct unit tests for `BlockManager::GetFirstBlock` setting a `lower_block`, https://github.com/bitcoin/bitcoin/blob/3789215f73466606eb111714f596a2a5e9bb1933/src/test/blockmanager_tests.cpp#L127 exercises that path implicitly.
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535010215)
While there are no direct unit tests for `BlockManager::GetFirstBlock` setting a `lower_block`, https://github.com/bitcoin/bitcoin/blob/3789215f73466606eb111714f596a2a5e9bb1933/src/test/blockmanager_tests.cpp#L127 exercises that path implicitly.
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535030418)
nit: this path is never exercised in the unit tests
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535030418)
nit: this path is never exercised in the unit tests
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535057647)
You mean similarly to
https://github.com/bitcoin/bitcoin/blob/99d012ec80a4415e1a37218fb4933550276b9a0a/src/rpc/blockchain.cpp#L867-L868
?
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535057647)
You mean similarly to
https://github.com/bitcoin/bitcoin/blob/99d012ec80a4415e1a37218fb4933550276b9a0a/src/rpc/blockchain.cpp#L867-L868
?
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3543410242)
Code review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a
The PR helps provide clearer failure output, since only missing expected messages are reported.
The lazy initialization part is not necessarily an optimization - I see it primarily as code simplification. That is why I suggested using a list comprehension for `remaining` on each iteration instead of the tail-pop optimization.
But I am also fine with it as is.
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3543410242)
Code review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a
The PR helps provide clearer failure output, since only missing expected messages are reported.
The lazy initialization part is not necessarily an optimization - I see it primarily as code simplification. That is why I suggested using a list comprehension for `remaining` on each iteration instead of the tail-pop optimization.
But I am also fine with it as is.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535227041)
While I like the extra coverage, I’m not fully convinced about adding this bit of non-determinism. That’s why I didn’t include it in the last push. It could make reproducing failures trickier, since we’d be adding another dimension to all tests; core counts behave differently across environments.
Probably a good middle ground could be adding a separate test that picks a random worker count and prints the available core count on failure. That gives us some extra coverage without making the oth
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535227041)
While I like the extra coverage, I’m not fully convinced about adding this bit of non-determinism. That’s why I didn’t include it in the last push. It could make reproducing failures trickier, since we’d be adding another dimension to all tests; core counts behave differently across environments.
Probably a good middle ground could be adding a separate test that picks a random worker count and prints the available core count on failure. That gives us some extra coverage without making the oth
...
💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535258707)
This is needed in multiple methods here, it would simplify the diff and it provides a higher level primitive instead of repeating and reimplementing what we have already built a helper for.
Not sure why we'd want to reduce dependencies here, what's the advantage of that? I value clean code a lot more, especially for a test that doesn't even have performance requirements?
We can of course move `HasReason` in a separate PR, but I think we can use it here before that as well.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535258707)
This is needed in multiple methods here, it would simplify the diff and it provides a higher level primitive instead of repeating and reimplementing what we have already built a helper for.
Not sure why we'd want to reduce dependencies here, what's the advantage of that? I value clean code a lot more, especially for a test that doesn't even have performance requirements?
We can of course move `HasReason` in a separate PR, but I think we can use it here before that as well.
💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535271000)
It's not about coverage, but rather avoiding bias that we may introduce by hard-coding arbitrary values.
I prefer a situation that is "hard to reproduce" over something that we won't find because of hard-coded values.
> core counts behave differently across environments
Exactly, let's exercise those differences, that's exactly what we want from our tests, to ruthlessly try to break the code, right?
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535271000)
It's not about coverage, but rather avoiding bias that we may introduce by hard-coding arbitrary values.
I prefer a situation that is "hard to reproduce" over something that we won't find because of hard-coded values.
> core counts behave differently across environments
Exactly, let's exercise those differences, that's exactly what we want from our tests, to ruthlessly try to break the code, right?
🤔 ryanofsky reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-3472979975)
Code review ACK 6091e947504a13df9ee3310e0fce4585c79d19db
I'm about halfway through reviewing this, and so far it seems like has a lot of nice code improvements in addition to conceptual benefits mentioned earlier https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3483436641.
One thing I don't like about the API however, is the use of integer connection ids and introduction of hash maps to to tie connection ids, sockets, and application data together:
- `CConnman::m_listen_permissions`
...
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-3472979975)
Code review ACK 6091e947504a13df9ee3310e0fce4585c79d19db
I'm about halfway through reviewing this, and so far it seems like has a lot of nice code improvements in addition to conceptual benefits mentioned earlier https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3483436641.
One thing I don't like about the API however, is the use of integer connection ids and introduction of hash maps to to tie connection ids, sockets, and application data together:
- `CConnman::m_listen_permissions`
...
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534490390)
In commit "style: modernize the style of SockMan::BindListenPort()" (6ba0aab8658a7bd553a96e441aa1d5e016e086c3)
Could be good to note in commit message what behaviors this is changing, so it clear they are intentional. After this commit:
- err_msg / strError output is no longer set if there are minor errors like failing to set SO_REUSEADDR
- "Bound to [address]" log message is dropped and moved to caller
- Other log messages are changed slightly
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534490390)
In commit "style: modernize the style of SockMan::BindListenPort()" (6ba0aab8658a7bd553a96e441aa1d5e016e086c3)
Could be good to note in commit message what behaviors this is changing, so it clear they are intentional. After this commit:
- err_msg / strError output is no longer set if there are minor errors like failing to set SO_REUSEADDR
- "Bound to [address]" log message is dropped and moved to caller
- Other log messages are changed slightly