💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534948549)
These docstrings don't add any value imo.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534948549)
These docstrings don't add any value imo.
💬 fanquake commented on pull request "guix: update per-host disk-space estimates in build gate":
(https://github.com/bitcoin/bitcoin/pull/33889#discussion_r2534956518)
> due to a change made in the build process (-static-libgcc).
Can you explain a bit more how this is related to `-static-libgcc`, given you haven't changed the allowance for any Linux builds, and macos/mingw don't use `-static-libgcc`?
(https://github.com/bitcoin/bitcoin/pull/33889#discussion_r2534956518)
> due to a change made in the build process (-static-libgcc).
Can you explain a bit more how this is related to `-static-libgcc`, given you haven't changed the allowance for any Linux builds, and macos/mingw don't use `-static-libgcc`?
💬 djkazic commented on pull request "guix: update per-host disk-space estimates in build gate":
(https://github.com/bitcoin/bitcoin/pull/33889#discussion_r2534964005)
Yeah, the details are linked in the issue on kev's fork but the issue was flagged on a commit where static was set. I tested Linux but the disk space used there was in line with expectations.
(https://github.com/bitcoin/bitcoin/pull/33889#discussion_r2534964005)
Yeah, the details are linked in the issue on kev's fork but the issue was flagged on a commit where static was set. I tested Linux but the disk space used there was in line with expectations.
💬 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.