💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534746780)
I still see init + reserve + loop + emplace in most tests, could we apply to the rest as well?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534813432)
> we want to exercise some concurrency too

You're right, that's better indeed.
My suggestion updated to keep the vector

```C++
// Test 5, throw exceptions and catch it on the consumer side
BOOST_AUTO_TEST_CASE(task_exception_propagates_to_future)
{
ThreadPool threadPool("exception_test");
threadPool.Start(NUM_WORKERS_DEFAULT);

const auto make_err{[&](size_t n) { return strprintf("error on thread #%s", n); }};

const auto num_tasks{1 + m_rng.randrange<size_t>(20)}
...

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534845237)
Not exactly sure what you mean by that - `Note 2` does seem like a good idea to me and it's probably similar to what I meant. It would help the review process to gain more confidence in the implementation if we migrated away in smaller steps, documenting how the tests we add for the old implementation also pass when we switch over to the new implementation.

💬 waketraindev commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2534893018)
```suggestion
const uint8_t value = std::to_integer<uint8_t>(byte);
for (int bit = 0; bit < 8; ++bit) {
asmap_bits.push_back((value >> bit) & 1);
}
```

Extract `value` so the `std::to_integer` cast is only called once per byte instead of once per bit.

💬 plebhash commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3543028239)
sorry, I should retract the message above

I just realized there is a `getCoinbaseMerklePath`!

so indeed, there's no need to do a `getBlock` every time!

📝 djkazic opened a pull request: "guix: update per-host disk-space estimates in build gate"
(https://github.com/bitcoin/bitcoin/pull/33889)
Today, the build gates are obsolete due to a change made in the build process (`-static-libgcc`). This PR aims to readjust the build gate disk space per-host estimates based on some testing I did locally in my build VM.

This would prevent any surprises at build-time for guix for there not being enough free disk space.

(Copied from https://github.com/kevkevinpal/bitcoin/issues/200):

I ran `HOSTS="x86_64-w64-mingw32" ./contrib/guix/guix-build`

After (+10 GiB):
```
Filesystem 1G-b
...

💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534670188)
typo nit
```suggestion
* @brief Processes and validates the provided block header.
```

💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534865290)
This should be `LogError`

💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534945865)
I'm not sure why this function is included in this PR, it doesn't seem to be related to any of the block header functionality? Would make more sense to leave it out?

💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534716218)
nit: inconsistent capitalization
```suggestion
* @return Block header, or null on error.
```

💬 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.

💬 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`?

💬 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.

💬 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?

📝 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
...

💬 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
...

💬 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.

💬 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
...

💬 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
...

💬 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.