💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533657576)
I'll go for the latter approach.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075)
The BIP says:

> For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.

So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".

💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533700190)
Went with separate counters and also added the selected network to the log output for clarity.
Example log entry:
```
[addrman] GetAddr returned 62823 random addresses from ipv4; 1104 filtered (0 network, 0 quality, 1104 policy)
[addrman] GetAddr returned 0 random addresses from ipv6; 63927 filtered (63927 network, 0 quality, 0 policy)
[addrman] GetAddr returned 0 random addresses from onion; 63927 filtered (66927 network, 0 quality, 0 policy)
```

💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533706305)
Updated comments for more clarity
in addrman.h:
```cpp
/** Predicate used to exclude addresses during selection.
* Return true to skip the given address.
*
* Runs while holding AddrMan::cs, so it must be non-blocking, must not
* attempt to reacquire AddrMan::cs, and must preserve lock ordering to
* avoid deadlocks.
*/
```
in net.cpp -> GetAddressesUnsafe()
```cpp
// This runs under AddrMan::cs, and BanMan checks take m_banned_mutex.

...

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