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

🚀 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*? 😄