💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3551887180)
I have pushed the [commit](https://github.com/bitcoin/bitcoin/pull/30442/commits/bb57dd5e84108126d41cc03a8dba89c0d9f09a39) on top to not invalidate previous ACKs:
```patch
- explicit SaltedOutpointHasher(bool deterministic = false);
+ SaltedOutpointHasher(bool deterministic = false);
```

I would appreciate a re-review @achow101, @ismaelsadeeq, @jonatack, @laanwj, @ryanofsky, @sipa.

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541360330)
7f17cf6d6164460086afebbb0f921a7af4475bf9:

This example uses Makefile variable `CC` and `CXX`, but the section title refers to "Environment Variables". Both approaches work, of course, but the section could be made clearer.

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541379255)
7f17cf6d6164460086afebbb0f921a7af4475bf9

This comes across as a bit discouraging and confusing. When I say “I build depends with Clang”, I’m referring to the host-specific packages. I’m not concerned about which compiler is used for the native tools, as long as they build without issues.

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541395513)
In most situations where the compiler needs to be specified, users simply run:
```
make CC=clang CXX=clang++
```

Only in rare cases does this need to be extended to:
```
make CC=clang CXX=clang++ build_CC=clang build_CXX=clang++
```

💬 yuvicc commented on pull request "kernel: add context‑free block validation API (`btck_check_block_context_free`) with POW/Merkle flags":
(https://github.com/bitcoin/bitcoin/pull/33908#issuecomment-3551936727)
Concept ACK, Approach NACK from my side.

Currently, `btck_check_block_context_free` requires a full `btck_Context*`, but it only uses`context->m_chainparams->GetConsensus()`. Library user will need to maintain a full context object just to perform context-free block validation checks which I think is not a good idea. Also, the function doesn't clearly communicate that only consensus params are needed and not full context. I think a better approach could be to expose opaque struct for consensu
...

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541415447)
I'm not sure about "Native tools will still use the default GCC...". Perhaps they will use the default compilers (`which cc` and `which c++`). But I didn't test this hypothesis.

🤔 hebasto reviewed a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851#pullrequestreview-3481954279)
My Guix build:
```
aarch64
3a70d632167f9e57328d9b8373a5eb9936428148feddf5bc1f9834f412ea4248 guix-build-daafaef87fee/output/aarch64-linux-gnu/SHA256SUMS.part
ca1c7ad00632d0ee3fdfad498a7c6cb5246240a9e49c89bee508355ee7dbb832 guix-build-daafaef87fee/output/aarch64-linux-gnu/bitcoin-daafaef87fee-aarch64-linux-gnu-debug.tar.gz
a5a5c5a22159a912f573915727b5a4cdbb1b270fdf099f8b465c3ef940496e20 guix-build-daafaef87fee/output/aarch64-linux-gnu/bitcoin-daafaef87fee-aarch64-linux-gnu.tar.gz
a660c559c1f2ac
...

💬 maflcko commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541427389)
> I’m not concerned about which compiler is used for the native tools, as long as they build without issues.

at least for me the issue was that they did not build at all, because the gcc version was ancient on OpenSuse Leap (or Alma Linux 8, ....)

💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541444913)
If using something like a Nix devShell with no gcc available at all, compilation just fails too.

💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541453697)
> > I’m not concerned about which compiler is used for the native tools, as long as they build without issues.
>
> at least for me the issue was that they did not build at all, because the gcc version was ancient on OpenSuse Leap (or Alma Linux 8, ....)

The same issue occurs on [NetBSD](https://github.com/hebasto/bitcoin-core-nightly/blob/e405ced802f4bcc723db45a7c931b17a0c7b2590/.github/workflows/netbsd.yml#L136).

💬 maflcko commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#issuecomment-3552037406)
Not sure, this will bloat the title, so that it can't be fully read anymore:

<img width="714" height="496" alt="Screenshot 2025-11-19 at 11-39-10 ci Make the max number of commits tested explicit · bitcoin_bitcoin@d14d1b5" src="https://github.com/user-attachments/assets/733c7ea1-da8e-4b25-a153-3edde712bcae" />


Also, the title seems the wrong place to list all the shortcomings of each CI task. For example, the win-cross does not build the gui tests, should it say 'no-gui-tests'? Or the ar
...

💬 yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2541496096)
Correct. Good observation. So at this stage the `state` already represents success. If the background chain fails, overwriting it with the failure state is fine - that's what you want to return anyway. If the background chain succeeds, state gets overwritten with success - also fine. The only argument for keeping `bg_state` separate would be if you wanted clearer variable naming to show intent, but functionally reusing state works fine here.

💬 waketraindev commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3552070213)
```
f37da8124a6392893fc7b18cfd72c616cac022eaed66725b1632b9e0011e1391 guix-build-8558902e576e/output/x86_64-w64-mingw32/SHA256SUMS.part
aa44003342fd3e4fb9d8c1693d12d31788626988cd9f47840e21afe9203deef0 guix-build-8558902e576e/output/x86_64-w64-mingw32/bitcoin-8558902e576e-win64-codesigning.tar.gz
ad14fac93313dcaaba144f944cd0058ef1581bc582645d20f5c414a0db116e14 guix-build-8558902e576e/output/x86_64-w64-mingw32/bitcoin-8558902e576e-win64-debug.zip
3ae1c3daa2a7932e83a84b2ea1b9a8ebcc912fbfe2dfa
...

💬 waketraindev commented on pull request "rpc: Add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3552076557)
> the commit message is wrong?

Thanks. Must've replaced it by mistake when I rebased. Fixed now

💬 yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2541520465)
No we cannot as:

```cpp
enum class ModeState {
M_VALID, //!< everything ok
M_INVALID, //!< network rule violation (DoS value may be set)
M_ERROR, //!< run-time error
}
```
`!state.IsValid()` returns true when the state is either `M_INVALID` or `M_ERROR`
`state.IsInvalid()` returns true only when the state is `M_INVALID`, misses the `M_ERROR` or run-time error case.

💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541520923)
Have attempted to make different defafults clearer in 34c424ea500

💬 hodlinator commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#issuecomment-3552108811)
Thanks for the feedback! "test max 6 ancestor commits of H..." is still pretty informative IMO. When not bolded, on larger resolutions, one can still see the whole name. And when selecting a job one sees the full name (as in this "macOS native, no depends" example to the right).

<img width="1051" height="261" alt="image" src="https://github.com/user-attachments/assets/487adc80-48bd-4307-ad2f-3b83f5056e50" />

I think documenting at least some limitations of tests in this way makes knowledge
...

💬 purpleKarrot commented on pull request "kernel: add context‑free block validation API (`btck_check_block_context_free`) with POW/Merkle flags":
(https://github.com/bitcoin/bitcoin/pull/33908#issuecomment-3552114302)
Both @w0xlt and @yuvicc, please don't introduce more [boolean trap](https://ariya.io/2011/08/hall-of-api-shame-boolean-trap)s.

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3552171802)
Redid the rebase locally, besides an import collision the following was added:
```patch
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
--- a/src/test/miniscript_tests.cpp (revision 31196b79cabb6e4519e8a27165e1be75c5044403)
+++ b/src/test/miniscript_tests.cpp (date 1763547689484)
@@ -727,7 +727,7 @@
g_testdata.reset();
}

-// Confirm that ~Node() and Node::Clone() are stack-safe.
+// Confirm that ~Node(), Node::Clone() and operator=(Node&&) are stack-s
...

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2541499665)
nit, if you touch again, we could simply omit leaves with no subs

```suggestion
if (i.subs) queue.push_back(std::move(i.subs));
```