💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2526752792)
The suggested comparator is a pure function, I don't see how it could be non-deterministic - unless it's broken, in which case we should definitely dig deeper.
Can you try it again an report what you see?

🚀 fanquake merged a pull request: "depends: drop Qt patches"
(https://github.com/bitcoin/bitcoin/pull/33860)

👍 TheCharlatan approved a pull request: "depends: sqlite 3.51.0; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655#pullrequestreview-3464109401)
ACK 1db74914706fcfafb22646288458604a4a7b6282

Guix build:
```
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21bf1d78cd8ebe9719b1 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706
...

👍 rkrux approved a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464238462)
lgtm ACK 0aebdac95da9a7d476264424c0107bd806ce5362

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3532063730)
I was still wondering how number of parallel threads affects this, given that it's not a cpu-bound task.

The measurements were done on i9, m4 and rpi4. First two have 16 threads, rpi has 4.
The results still indicate to me that it doesn't make sense to set parallelism based on number of cpus directly. Beyond 4-8 threads the systems didn't really perform any better.

<img width="1489" height="861" alt="image" src="https://github.com/user-attachments/assets/c6065154-0625-4609-928f-867c957ba2
...

📝 Sjors converted_to_draft a pull request: "mining: add getCoinbase()"
(https://github.com/bitcoin/bitcoin/pull/33819)
Deprecate `getCoinbaseTx()` in favor of a new method that provides a struct with everything clients need to construct a coinbase. This is safer than providing a raw dummy coinbase that clients then have to manipulate.

Also deprecate `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.

A new helper method `ExtractCoinbaseTemplate()` processes the dummy coinbase transaction generated by `BlockAssembler::CreateNewBlock` and produces a `CoinbaseTemplate`.

Expand the `interface_ipc.p
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532157521)
From https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008 I gather that we should not reuse the method ordinals. Since cap&proto doesn't allow gaps, when deprecating methods, it's best to just have them throw an error.

In https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3529071835 @ryanofsky suggested replacing the dummy coinbase with an empty transaction, so that clients are not tempted to use the dummy coinbase via `getBlock()` or `getCoinbaseTx()`.

I think
...

🤔 frankomosh reviewed a pull request: "net: Filter addrman during address selection via AddrPolicy to avoid underfill"
(https://github.com/bitcoin/bitcoin/pull/33663#pullrequestreview-3464332521)
Concept ACK

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527033639)
Is there a way/test to verify that the `BanMan` methods here are safe to call while `AddrMan`'s lock is held?

Can see a comment in `addrman.h` says the policy "must not acquire locks that can conflict with `AddrMan::cs`". So it would make sense to ensure deadlock scenario is not accidentally created.

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527062353)
Would network mismatches also count as `filtered` i this case?

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2527112930)
Ah yes, that is cleaner.

💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527173880)
> that's literally English for data_dir == nullptr && data_dir_len > 0 evaluating to true, therefore satisfying the if condition, therefore triggering the early return.

The `&&` requires both of conditions to be true to process the early return branch? The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero. So, either `data_dir == nullptr` alone is enough, or `data_dir == nullptr || data_dir_len == 0` to be
...

👍 stickies-v approved a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464527742)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362

💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527219768)
I don't think it does, afaik a write exception through `Sync` or `Flush` is just bubbled up and then logged. But I might be missing something.

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532419886)
> I think I'm going to combine these two observations and make this PR a breaking change. Marking as draft for now.

This seems ok, but isn't really what I would suggest. My suggestion would be for this PR to just introduce a new `getCoinBase()` method like it's doing and not remove other methods so existing mining clients do not immediately stop working. The other methods, which are all simple accessors, can easily be removed later.

Adding a new method now and removing older methods later
...

💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527283208)
> The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero.

*should* being the keyword here indeed. A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic. We need to return early in order to not cause any UB later on, and because the request is nonsensical. In the earlier approach on which you commented (0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but _o
...

✅ hebasto closed an issue: "Implicit conversion from `fs::path` to `std::string` when constructing file streams"
(https://github.com/bitcoin/bitcoin/issues/33545)

💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3532476657)
Fixed in https://github.com/bitcoin/bitcoin/pull/33550.

💬 willcl-ark commented on pull request "depends: sqlite 3.51.0; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#issuecomment-3532580690)
Build:

```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21
...

🤔 hebasto reviewed a pull request: "depends: drop Qt patches"
(https://github.com/bitcoin/bitcoin/pull/33860#pullrequestreview-3464798591)
Post-merge ACK.

I tested using GCC 12 with CMake 3.22 and 3.27.