💬 maflcko commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3528420008)
review ACK 096924d39d644acc826cbffd39bb34038ecee6cd 📓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 096924d39d64
...

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2523975283)
> What do you mean with "(without a help from the standard library)"?

I meant without using `shared_ptr`, but surely we do use the standard library for other things. Removed this sentence altogether.

@ajtowns I took that wording, thanks!

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2523977301)
Changed.

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2523981252)
Good idea! Added some brief explanation amongst those lines, thanks!

💬 m3dwards commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3528462663)
Looks good!

ACK fae3618fd6c82dfcea2f296caa16a79182b32059

💬 yancyribbens commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524024171)
This case wouldn't return early though, even though it's an invalid state.

💬 janb84 commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3528532539)
My Guix Build Output (matching)

**Host architecture:** `aarch64`
**Commit:** `96963b8`

```shell
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/out
...

💬 darosior commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3528552716)
Sorry to be this guy again, but it seems to me unreasonable to touch such critical consensus code for such marginal benefits. Unless observable benefits can be shown, i lean toward Concept NACK.

💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524119124)
Hmm? It seems to me like we capture that in `if ((data_dir == nullptr && data_dir_len > 0) || ...) {`, after which we return early?

👍 TheCharlatan approved a pull request: "depends: drop Qt patches"
(https://github.com/bitcoin/bitcoin/pull/33860#pullrequestreview-3460632290)
ACK 3e9aca6f1b52169eed386849900a400e8cea431e

Guix build:
```
6eb3c63d243ad8b9e10bed1aa9ec545d4599a10e2d333a69b346c8aaab941fb2 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/SHA256SUMS.part
b7bf29e4e64a207eca6ec785f2a8944d0051c177636b6f8db78da5836404ae89 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b52-aarch64-linux-gnu-debug.tar.gz
dea243b0ce4f6a389ad1612d4f27f0bca816f5f11fe77c2ff4ca703a1db607ee guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b5
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524170909)
> Do we still need this after https://github.com/bitcoin/bitcoin/pull/33550?

I think it's still needed, I added it to get rid of linter warnings if `std::filesystem::copy` is instead called directly from the test code.

> Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?

Fair point, I'll rework the commits so it's a bit easier to tell where thin
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524183047)
> Can we add a simpler fuzz test which needs this in the same commit that introduces it - and extend the test with other functionality separately.

Yes, I'll do some variation of this. I think it might be cleaner to put it as one of the last commits.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524195439)
This is copied from the `process_message(s)` harness. Unlike those, this fuzz test should always construct correctly serialized messages (even if the messages themselves are deemed "protocol invalid"), so I think the catch case can just be `assert(false)`.

💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524238329)
Ah, thanks for those 2 links, that's helpful. I've been digging through cppreference and struggle to find a definitive explanation on what `std::filesystem::absolute(std::filesystem::path())` represents or why it's (il)legal, and so far haven't been able to find anything, so I just went with the behaviour of my implementation. I can see the viewpoint that an empty path does not exist in the filesystem, and thus should not have an absolute path associated with it.

Since it's (not unreasonably)
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524253265)
Will do, I think this was used in multiple places in an earlier version of this code.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524255627)
I'll incrementally add the cases so it's a bit easier to digest.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524277972)
IIRC I tried comparing the `uint256` block hashes similar to your second suggestion, and while it fixed the leveldb non-determinism, it introduced its own non-determinism.

💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524354194)
Marking as resolved as this code is now outdated anyway.

💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#issuecomment-3528952987)
Force-pushed to disallow empty and null directories, addressing feedback from @maflcko both wrt [technical concerns](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180) and [conceptual concerns](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442). Also updated PR title and description.

💬 ryanofsky commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008)
Was looking at the IRC discussion and I think a nuance it doesn't address is that there is a difference between breaking compatibility explicitly so clients see an error if they try to call a method that's been changed or removed, and breaking compatibility silently so if a client tries to call a method that's changed or removed, a different method may be called instead or arguments may be interpreted incorrectly.

#### How to break compatibility explicitly

You can break compatibility explicitl
...