💬 Sjors commented on pull request "mining: getCoinbase() returns struct instead of raw tx":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3626113055)
Trivial rebase after #29641.

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597954811)
> Yeah, I tend to agree and will probably do this in a follow-up.

:heart:

> In Rust, one can use several let _ in the same scope.

Well, Rust practically encourages shadowing variables. :vomiting_face:

> However, in C++, using several auto _ in the same scope is only possible in C++26, in which case the [[maybe_unused]] is redundant anyway.

Nice!

💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3626116939)
Probably also related: https://codeberg.org/guix/guix/pulls/4737

💬 Sjors commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3626123055)
Holding off on trivial #29641 rebase until #33965 is merged or needs changes.

💬 plebhash commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3626128379)
> Can you enumerate which RPC calls you're polling?

`getrawmempool` is the only polled RPC, under a user-defined period

-`health` is called ad-hod at startup
-`getrawtransaction` is called ad-hoc after each `getrawmempool` polling cycle
- `submitblock` is called ad-hoc when there's a block available

🤔 danielabrozzoni reviewed a pull request: "kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3526048144)
light ACK f31f7c21ff7093a4c90199cd0bb2128d19bf2d33

Code looks good to me, and the interface is cleaner now. I'm only light-acking sicne I'm not familiar with the kernel :)

As I pointed out in a comment, I don't think we needed the `Assume(!headers.empty())` at the start of `ProcessNewBlockHeaders`, but I'm ok with keeping it.

💬 danielabrozzoni commented on pull request "kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2577884374)
I'm ok with adding the Assume, but I don't think it would have been a problem, as BlockValidationState is initialized to valid: https://github.com/bitcoin/bitcoin/blob/6356041e58d1ba86695e2e7c219c68ee5abe583f/src/consensus/validation.h#L82

I tested locally with:
```
diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 9973b33b57..f9080ef40f 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -363,4 +363,9
...

💬 laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3626326892)
New reproduction branch in https://github.com/laanwj/bitcoin/tree/2025-12-repro-33775 :

```
x86_64
ad9ef7b0ee55a233ad01dc71b91c435df6525a5b20cc2781feae715aba712fe7 test-original.cpp.obj
e07ba1f07faf254d2b192bf8494949769af2c13a3f75cba926120eb90b2c60f5 test-reverted.cpp.obj
```
```
aarch64
7d2cc20e0528d9f2a72ece935ae72a158f9f2483cd4bdbe56950db9e28f3c762 test-original.cpp.obj
e07ba1f07faf254d2b192bf8494949769af2c13a3f75cba926120eb90b2c60f5 test-reverted.cpp.obj
```

The first vari
...

🤔 maflcko reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3517825851)
needs rebase, otherwise this looks good.

left some nits/questions, but those can be ignored.


review ACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2 💐

<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+SkW
...

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2596329568)
nit f85ff75dad4ce8712fe65a636cf36d57b4066785: use ranges::any_of, and `const auto&`?

Also, in the commit message: `is not just ambiguous because actively confusing` -> `is not just ambiguous but actively confusing`

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2570730621)
0cb19b78cc297ceca591ee8be6f63b4ea7b494cc: Please don't add lock annotations to the cpp file. They won't be visible anyway. You'll have to add them to the .h file.

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2596330159)
nit https://github.com/bitcoin/bitcoin/commit/f85ff75dad4ce8712fe65a636cf36d57b4066785: Any reason to revert contains to count?

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2571678601)
nit in 1107e04328735abe636692b1a0179c6e46613d4e: Seems a bit odd to call the exact same assert twice

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2596366161)
f85ff75dad4ce8712fe65a636cf36d57b4066785: to delete (typo)

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2597734916)
nit in 1d477c711599b7eeac40b18a30ee9331d951a726: Could drop the year, for entirely new files?

This is commonly done, see `git log --oneline -S 'Copyright (c) The Bitcoin Core developers'`

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2596372569)
contains database -> contains a database [missing article makes the sentence ungrammatical / harder to parse]

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2596330616)
nit https://github.com/bitcoin/bitcoin/commit/f85ff75dad4ce8712fe65a636cf36d57b4066785: Any reason to revert contains to count?

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2597935686)
0cb19b7: Please don't add lock annotations to the cpp file. They won't be visible anyway. You'll have to add them to the .h file.

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2598294698)
I think this is just a refactor, and if a nullptr existed here, it would already exist on current master?

💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2598298692)
I think the current name is fine, but if we are allowed to bike-shed, then I'd replace `Detect` With `Add`, or `Load`, because the function does more than just read-only detection: `LoadAssumeutxoChainstate()`.