🤔 maflcko reviewed a pull request: "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3481321005)
forgot to send the nits, basically just a test nit to check the returned size, but just a nit

💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2540945544)
Maybe just de-duplicate with `FromBytes` in the other test file?

💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2540940425)
nit: Here and above in the last commit: Why not check the size is equal to 10 each time?

👍 willcl-ark approved a pull request: "ci: Consistenly only cache on the default branch"
(https://github.com/bitcoin/bitcoin/pull/33905#pullrequestreview-3481757071)
ACK fa411f938e4796cf3806f234c9787b7c79492cc2

We could probably drop `github.event_name != 'pull_request' &&`? Feels like it might be redundant with `github.ref_name == github.event.repository.default_branch`...

📝 hodlinator opened a pull request: "ci: Make the max number of commits tested explicit"
(https://github.com/bitcoin/bitcoin/pull/33909)
Gives less of a false sense of security.

💬 maflcko commented on pull request "ci: Consistenly only cache on the default branch":
(https://github.com/bitcoin/bitcoin/pull/33905#issuecomment-3551842381)
Yeah, I guess even if a pull is opened from a branched named equally to the default branch, the ref_name is rewritten to `<pr_number>/merge`, according to https://docs.github.com/en/actions/reference/workflows-and-actions/contexts#github-context

However, it can't hurt to check twice explicitly, so I'll leave as-is for now :sweat_smile:

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2541346873)
Hmm, looks like this happens because there are still connections between the bitcoin node and the SOCKS5 proxy and then the bitcoin node is restarted, closing the connections. Or rather, the bitcoin node is just establishing a new connection and is interrupted in the middle of the SOCKS5 handshake because of the `self.restart_node()` call.

This is expected and the test succeeds rightfully, however that printout looks odd and scary. Should be handled more gracefully. One way would be to conver
...

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