💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533708792)
I'll drop `input_`

💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2533714543)
Yeah, I don't think we can completely prevent UB here, just do the best and least surprising thing we can.

💬 fanquake commented on pull request "refactor: Avoid -W*-whitespace in git archive":
(https://github.com/bitcoin/bitcoin/pull/33869#issuecomment-3541325190)
ACK fa95353902b7a6f73f094e78106088ab3c16ce14

```cpp
// git will expand the next line to "#define GIT_COMMIT_ID ..." inside archives:
//
#define GIT_COMMIT_ID "84f39b282de0836ca5ad6166daafecf15e4c813d"
```

🚀 fanquake merged a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177)
That seems incorrect, because a pool (like Ocean or P2pool) might let you take part of the reward directly in an output.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533740877)
I added documentation there, but we also need it here. The `output` field is not limited to `OP_RETURN`, that's just a heuristic used by `ExtractCoinbaseTemplate`.

That heuristic won't work anymore if we add a way for clients to specify custom outputs when constructing a block, so it's important to document.

🤔 stickies-v reviewed a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877#pullrequestreview-3472331847)
Hmm, I think I'm ~0 on this, leaning towards Concept NACK. Removing unnecessary parameters is good, but I personally find an interface more ergonomic when you can call `object.set_bool(value)` instead of `if (value) object.set_bool()`. It generalizes better, and makes using the object more flexible. For example, if `value` depends on multiple factors, with the last one taking priority, the value can be updated multiple times with `object.set_bool(value)` without needing extra state.

In the ca
...

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2533769750)
It wouldn't lose the witness section @frankomosh it would just be rolled up. It would still print, it would just print in the txIn instead. However i can breaking such a fundamental log would be a problem so I'm okay with making the change less intrusive

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533772625)
Changed to "minus any non-zero required outputs" and a comment to point out that those don't exist atm.

See https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177 above, I think having non-zero outputs is quite conceivable and it would be a shame if that requires another breaking change.

👍 willcl-ark approved a pull request: "ci: Lint follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33776#pullrequestreview-3472377000)
ACK fae3618fd6c82dfcea2f296caa16a79182b32059

This all seems fine, and I agree python is superior to bash where possible. It is indeed annoying that GH doesn't persist the annotations, but after also checking myself I can't think of any better way to do it either.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541552964)
Rebased after #33745.

To keep this PR focussed I removed mention of deprecation. I have a branch [2025/11/coinbase-template-break](https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2025/11/coinbase-template-break) that shows what I have in mind for deprecating these methods and clearing the dummy coinbase. The first commit is non-breaking, so I'll PR that as a followup. I plan to integrate the rest of the branch into https://github.com/Sjors/bitcoin/pull/106, which is a collec
...

📝 maflcko opened a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886)
Also, add a test for creating a CScript from an empty byte vector.

To test: `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG' -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DBUILD_KERNEL_LIB=ON -DENABLE_WALLET=OFF -DENABLE_IPC=OFF && cmake --build ./bld-cmake --parallel $( nproc ) && valgrind --tool=none ./bld-cmake/bin/test_kernel --catch_system_error=no`

B
...

💬 maflcko commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2533874874)
removed the tests for now in https://github.com/bitcoin/bitcoin/pull/33886

👍 TheCharlatan approved a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886#pullrequestreview-3472498582)
ACK fadb4f63cb0f0b544bc95e48cb42c7636c1dec15

👍 laanwj approved a pull request: "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3472503057)
Code review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a
These are some straightforward changes that even make sense outside the context of embedded ASmap.

💬 willcl-ark commented on pull request "guix: produce a `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3541623380)
> ;; --enable-static-nss isn't used yet, because it has been broken
> ;; since 2.33: https://sourceware.org/bugzilla/show_bug.cgi?id=27959.

What are the exact implications of this? I guix-built this branch and loaded it into a scratch docker container and the dns seeds were connected to and loaded fine. Is this coming from my host system perhaps, even inside a scratch container?

I also tested the binary on alpine and it appeared to "fallback" to using libnss without issue...

🚀 fanquake merged a pull request: "ci: Lint follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33776)

💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541646050)
Good point! I understand your rationale, but wouldn't the cases you're describing be mostly for debugging/testing purposes?
It seems to me that for typical use cases, we would know whether to enable in-memory mode at compile time, not something that is derived dynamically, so it feels more natural/clear to simply call a function that does that with no input parameter.
Also, having an `update(bool_value)` function might indicate that we're supporting some meaningful toggleable functionality, w
...

💬 laanwj commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3541671065)
> The first three PRs were already part of https://github.com/bitcoin/bitcoin/pull/28792, the others are new.

i'm sure you mean the first three *commits*? 😄

💬 stickies-v commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3541688045)
Concept ACK, but a little more information in the PR description would be helpful, it's hard to parse what expected behaviour is - both from the CI, as well as from developers. Does passing CI become mandatory? Are we duplicating CI runs or moving it to a separate iwyu job? Should developers change anything in their workflow? Linking the previous discussion is helpful, but doesn't necessarily describe what's adopted in this PR.

Also, style preferences wrt includes should be added to `develope
...