👍 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!

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018254263)
Thread https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018038275:

It would be good if all `assert_` functions supported error messages explaining why the condition should never fail, or what could be wrong if it has failed. (Built-in `assert` supports it too).

To avoid mixing the error message up with a value to be checked, one could do:
```suggestion
def assert_not_equal(thing1, thing2, *, message=None):
if thing1 == thing2:
raise AssertionError(f"Both values ar
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2018313316)
Will take if anything else forces me to re-touch.

💬 Lagrang3 commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2760837538)
Router: Archer AX1800
OS: Debian 12
Bitcoin: v29.0rc2
Result: success

```bash
$ bitcoind -natpmp=1 -debug=net
...
2025-03-28T10:11:15Z [net] pcp: Received response of 8 bytes: [removed]
2025-03-28T10:11:15Z [net] portmap: Got unsupported PCP version response, falling back to NAT-PMP
2025-03-28T10:11:15Z [net] natpmp: Requesting port mapping port 8333 from gateway 192.168.0.1
2025-03-28T10:11:15Z [net] natpmp: Received response of 12 bytes: [removed]
2025-03-28T10:11:15Z [net] natpmp: Received r
...

💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2760846407)
@maflcko / @ryanofsky, this implementation seems to fail in the same weird way as https://github.com/bitcoin/bitcoin/actions/runs/12918565902/job/36027337187?pr=31539 - with 100% tests passing.
The last PR indicates it's the batch writing part - could it be an OOM because the sanitizer extras in `DataStream#resize(1 << 20)`?
`WriteBlockBench` indicates that even a buffer size of `1 << 16` is half the speed (Linux/GCC speed is the same) - but I'll try it, let's see if it passes CI at least.

💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2760865175)
> Neat! Please open a PR.

Hmmm, further testing this morning reveals other potential (root-causes and) fixes. Seems that I have two other issues with the original code:

1. Responses can be longer than 4096 bytes. I guess tailscale is filling them up!
2. Sometimes the **first** `RTA_GATEWAY` response found (which we use) may not be for the default route (and may not support pinholing). This happens for me on IPV6 only

An alternative fix therefore is to iterate over the full response, and filte
...

💬 maflcko commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018338475)
> assert_not_equal(main_block_hash, stale_block_hash, message="node 0 chain did not reorganize")

I'd say it is already self-explanatory to see `assert_not_equal(main_block_hash, stale_block_hash)` and the message "AssertionError: Both values are (the same)". It should be clear that the chains are the same, when they should be different.

Even more so, given that the test logs before the assert: `self.log.info("Reorg node 0 to a new chain.")`.

Unique and descriptive error messages make se
...

💬 i-am-yuvi commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2760925328)
I have tested on my local machine and here are some of my observation
https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2018389378)
Pushed new version that hopefully clarifies things:
https://github.com/bitcoin/bitcoin/blob/7aa397a7d45ce0ae1b83cc6f3a65db0e41b21ee8/src/script/descriptor.cpp#L1442-L1447

💬 maflcko commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2760955554)
Does CI pass locally before and after? Measuring locally, what is the memory usage of the CI before and after?


It seems odd that using exact block sizes (this pull initially) passes, whereas using a `1<<20` approximation fails due to OOM.

💬 janb84 commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2760963857)
> I have tested on my local machine and here are some of my observation https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md

Thanks for the report ! Would you be so kind to make an extract from the PCP / NAT-PMP part your findings to a comment [here](https://github.com/bitcoin/bitcoin/issues/31663). Would be so helpful !

Thanks again

💬 Prabhat1308 commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2760973175)
> I have tested on my local machine and here are some of my observation https://github.com/i-am-yuvi/bitcoin-core-issues-prs-notes/blob/master/29.0rc_testing.md

You might have missed building with 29.0 . I Don't see a `reject-detail` field in the last response you have tested.

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018425335)
> "AssertionError: Both values are (the same)"

I don't know if you mean literally `"Both values are (the same)"` or `f"Both values are {thing1}"`. I find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.

---

What it comes down to with custom error messages is being able to directly communicate with whoever is troubleshooting a failure. If one doesn't even need to go to the logs to see what happened before, I consider it a small win.
...

✅ willcl-ark closed an issue: "Failed transactions on importing mempool"
(https://github.com/bitcoin/bitcoin/issues/32125)

💬 willcl-ark commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2761030198)
Going to close this as I don't think there's anything left to do here. Let me know if I've misunderstood if you want it re-opened.