💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2018030340)
I don't think so. Will investigate it today.

💬 maflcko commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018038275)
what is the point of the error_message, given that it is unused and highly fragile:

* It is not a named arg, nor type-safe, so someone writing `assert_not_equal("a", "b", "c")` or `assert_not_equal(1, 2, 3)` will be wrong and confusing at best.
* It is not used anywhere else in this file for another assert

💬 maflcko commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2018040253)
Do you still know how often it happened and what the traceback looked like?

💬 laanwj commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2760413574)
> I have got a patch which works for me on x64 Ubuntu: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:pcp-gateway-detect

Neat! Please open a PR.

To be fair, the auto-forwarding setup is meant for straightforward customer ISP cases. i think throwing up our hands and going "set up forwarding yourself because you obviously can do this" in the case of complex network setups and routing tables is fine. Especially in multi-homing setups. Not sure tailscale should be counted a
...

💬 maflcko commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2760418494)
> Left some comments but I am still ~0 on this change and would much rather prefer that someone tries to spend more time on investigating the issue. I tried for a bit but wasn't successful so far.

I tend to agree. It would be good to check:

* Does it happen with a self-compiled clang. If yes, then it is likely an upstream brew issues. If no, then it is likely an upstream clang/llvm issue.

Unrelated to that, it would be good to minimize the `DataStream` crash, so that there is a single m
...

💬 stratospher commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#issuecomment-2760456444)
reACK adcbb2a. nice catch regarding `isValid()` usage in `ResetBlockFailureFlags`!

🤔 janb84 reviewed a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2724707044)
ACK [b544d1c](https://github.com/bitcoin/bitcoin/pull/31640/commits/b544d1c1aeb701cc7895fa38bf3f0745806c64fd)

- Code review ✅
- Tested ✅

💬 Sjors commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2760609125)
Tested with Stratum v2 (https://github.com/Sjors/bitcoin/pull/68 + first commit here, SRI pool role connected to their custom signet, sv2 -> sv1 translator and a BitAxe). Indeed I'm seeing blocks at height N with `locktime` values of N-1 and `sequence` of 4294967294 (`0xFFFFFFFE`). I explained below why this just works(tm), unlike with Stratum v1.

There's a question about sequencing here, since it's a preparatory step towards the Great Consensus Cleanup proposal https://github.com/bitcoin/bip
...

👍 willcl-ark approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2724763601)
utACK 25b56fd9b469f8e5d36f0132c3b79a5214e3372a

I primarily reviewed the CI logs to check everything there was working as expected, as I'm not very familiar with Windows commands. Left a minor nit about using `pwsh` in the new job, but it doesn't matter each way.

I do wonder how much our limited GH cache may suffer by adding the new 'Linux->Windows cross, no tests' job? I think this takes us to 8 GHA jobs now, although not all have a cache-save step.

💬 willcl-ark commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2018197779)
Any reason not to use bash + sed here? (first commit was making bash default shell after all)?

Something like:

```yml
- name: Adjust paths in test/config.ini
shell: bash
run: |
sed -i 's|^SRCDIR=.*|SRCDIR=${{ github.workspace }}|' test/config.ini
sed -i 's|^BUILDDIR=.*|BUILDDIR=${{ github.workspace }}|' test/config.ini
sed -i 's|^RPCAUTH=.*|RPCAUTH=${{ github.workspace }}/share/rpcauth/rpcauth.py|' test/config.ini
cat test/config.ini
```

👍 l0rinc approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2724801948)
lightweight re-review-ack 6bbfb3eeef2e35560dd357bb2a8a7378e2b49eb5

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2018221082)
super-nit:
```suggestion
r"FailedToStartError: \[node 0\] bitcoind exited with status 1 during initialization\. Error: Error parsing command line arguments: Invalid parameter -nonexistentarg"
```

💬 Sjors commented on issue "Consensus code without testing":
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2760715632)
@1440000bytes hopefully I've addressed some of your concerns here: https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2760609125

I'll keep an eye on this thread as well.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2760729847)
Rebased 97d1edcdafe074e910ed647dcb6beedd24744b17 -> a0d24ff9a9337770dae668d7b0ea0a6e62ed086a ([kernelApi_33](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_33) -> [kernelApi_34](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_34), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_33..kernelApi_34))

* Integrated the new `bitcoin-chainstate` functional tests from #32145 to demonstrate that it is still working.

👍 maflcko approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2724844797)
didn't look at any logs.

review-only ACK 25b56fd9b469f8e5d36f0132c3b79a5214e3372a 🍎

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted
...

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2018251327)
nit in https://github.com/bitcoin/bitcoin/commit/3501bca8c7e54923242fd3cfd21e7ef1c5d51d9d:

Shouldn't the restore key be set, to allow cache re-use on typo fixes or doc-only changes in the affected files?

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2018245705)
nit in https://github.com/bitcoin/bitcoin/commit/3501bca8c7e54923242fd3cfd21e7ef1c5d51d9d:

Not sure about using the latest. This should be identical to the version in the ci env file. Otherwise, this may break at any time.

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2760756041)
> I do wonder how much our limited GH cache may suffer by adding the new 'Linux->Windows cross, no tests' job? I think this takes us to 8 GHA jobs now, although not all have a cache-save step.

This is probably the last task (or second-to-last task) that can be moved to GHA without degrading cache performance.

👍 hodlinator approved a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2724862931)
ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6

This change adds the missing opposite of `assert_equal()` and decreases usage of built-in `assert` for performing always-on checks during tests.

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018256501)
Thread https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2017708099:
Thanks for taking my suggestion!