📝 fanquake converted_to_draft a pull request: "tests: Improve stderr validation in test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/31966)
This PR implements the improvement suggested in a TODO comment in test/util/test_runner.py.
It adds validation that stderr is empty when no errors are expected in test cases.

The change adds a check that verifies stderr is empty when no error_txt is specified in
the test object, with a special exception for bitcoin-tx running under Wine, which was
mentioned in the original TODO comment.

This improvement helps catch unexpected error messages that might otherwise go unnoticed
during
...

💬 fanquake commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2716138203)
> Sorry, my bad. I removed TODO

You partially removed it? You should also read https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches in regards to fixing your commits messages & fixing your commits.

🚀 fanquake merged a pull request: "test: Fix authproxy named args debug logging"
(https://github.com/bitcoin/bitcoin/pull/31955)

💬 fanquake commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2716192596)
> but maybe it's easier to do that in the final PR that drops BDB itself?

I don't see why, and it just leaves things broken/confusing in the interim. If we are going to move forward with this PR, the `NO_SQLITE` depends option should be removed at the same time.

💬 luke-jr commented on issue "build: document `BITCOIN_GENBUILD_NO_GIT` environment variable?":
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2716226975)
> The last comment (7 years ago) suggests that there was no issue with 0.13.0, even though the fix from #7522 only went into 0.15.0?

Likely it was backported

> It'd be good if someone could demonstrate an issue today, with the current source code.

```
mkdir test
cd test
git init .
tar -xvpf /path/to/a/release/tarball.tar.gz
cd bitcoin-*
sandbox # set up to complain about reads to /tmp/test/.git
# do whatever to build
```

Note that `/tmp` is not a suitable place to run the test, as tools oft
...

💬 fanquake commented on issue "build: document `BITCOIN_GENBUILD_NO_GIT` environment variable?":
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2716235199)
> set up to complain about reads to /tmp/test/.git
> you need to have the access attempt trigger an alert or kill the process somehow.
> do whatever to build

Multiple of the steps here are "just do thing", where it sounds like there isn't actually a build issue in the default case. Please provide actual, full steps to reproduce (with a tarball produced via the release process, from current master).

💬 fanquake commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2716287055)
We'll just have to use workarounds for `29.x`; hopefully CMake will fix it at some point. Somewhat suprising that we are the first to raise the issue, given the `-static-pie` option has existed in GCC for ~8 years.

🚀 fanquake merged a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161)

💬 fanquake commented on pull request "[POC] build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r1990498934)
In 76659d92f1925f561736ffe0f070ed0b9fef8d23: It'd be good if commits like this could contain actual details, rather than just "Adjust diagnostic flags"; especially given this is touching non-Windows code. Also, any reason to not fix the issues in leveldb, rather than adding more suppressions?

💬 fanquake commented on pull request "qa: Enable feature_init.py on Windows":
(https://github.com/bitcoin/bitcoin/pull/32021#discussion_r1990503238)
No. `CTRL_BREAK_EVENT` only exists on Windows: https://docs.python.org/3/library/signal.html#signal.CTRL_BREAK_EVENT.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2716480960)
`4492abbf0a...8c0ce1ca1c`: address https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1989217778

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1990577831)
Removed the declaration as well, thanks!

💬 aiswaryasankar commented on pull request "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`":
(https://github.com/bitcoin/bitcoin/pull/32025#discussion_r1990699940)
The change from `Txid` to `Wtxid` in `CheckEphemeralSpends` calls could cause issues with transaction identification. The function now stores witness transaction IDs instead of regular transaction IDs, which might affect how transactions are tracked and identified in error cases.

📝 kevincatty opened a pull request: "chore: remove redundant words in comment"
(https://github.com/bitcoin/bitcoin/pull/32037)
remove redundant words in comment

✅ fanquake closed a pull request: "chore: remove redundant words in comment"
(https://github.com/bitcoin/bitcoin/pull/32037)

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2716994263)
No strong opinion about depends (follow-up or here), otherwise:

review ACK ac6cde731314d981391bc313c98d671c68211d33 🐥

<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+krx
...

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1990912432)
In any case, this is unrelated to the changes here. If this is done, it would have to be done for all python packages like py-zmq, not only py-sqlite, in a separate change.

💬 fanquake commented on pull request "depends: patch around PlacementNew issue in capnp":
(https://github.com/bitcoin/bitcoin/pull/31998#discussion_r1990914356)
Thanks, I've adjusted this to use `-p2` (should have just done this in the first place rather than adjusting the paths).

💬 maflcko commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2717005621)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 fanquake commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1990943885)
We also don't do this for other similar targets, like `docs`. Where we just print `"Error: Doxygen not found"`.