💬 theStack commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2075713957)
nit: while touching these lines anyway, could fix the extra-weird formatting of the named argument comments.

💬 purpleKarrot commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2855114474)
ACK edde96376a2961dec3730331b3d171ddf972589f

> it seems like the PR is moving in the right direction and making a strict improvement by overriding less aggressively than before.

:+1:

💬 alfredopalhares commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855152427)
Concept NACK

Most of the above comments already said the important points.

I dont see why adding non financial data to the ledger is a good thing, even more just removing the option all together its scary to say the least, they are options for a reason, to let the users configure them, removing that is removing freedom to the individual node runner.

🤔 caesrcd reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2818858638)
ACK 6a9f04688f

I have reviewed the code, unit tests, and functional tests. I built the project and ran a node using the `-proxy=0=cjdns` parameter to verify the behavior. Confirmed that CJDNS correctly bypasses the general proxy as intended. Manual testing was performed on Arch Linux, and the node successfully connected over CJDNS directly without using the proxy.

💬 hebasto commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2855198133)
Also @0xB10C's https://github.com/hebasto/bitcoin/issues/121.

💬 Davidson-Souza commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2855208506)
I've tried out some of the code, specifically the API for validating transactions. I'm reporting back some of the results I've got so far, hopefully this info is useful for reviewers.

As some of you might know, I have a [project](https://github.com/vinteumorg/floresta) that uses the now deprecated (see #29189) `libbitcoinconsensus` for script validation. This is a nice feature, since script is usually the hardest part to re-implement when it comes to Bitcoin consensus. However, apart from bei
...

💬 hebasto commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2855218089)
> The problem is caused by `set(CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH OFF)` in the toolchain file:
>
> https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/depends/toolchain.cmake.in#L86
>
> and I believe it goes away if you remove that line. I commented on this previously in [#30940 (review)](https://github.com/bitcoin/bitcoin/pull/30940#pullrequestreview-2322355196)

Here is some additional context on how this code was introduced: https://github.com/hebasto/bitcoi
...

💬 theStack commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2855227905)
> > determining a (w)txid for a CTransaction instance is currently rather confusing and footgunny
>
> I don't disbelieve you exactly as I always have to double-check what I'm calling, but have we seen a series of footguns?

Not that I'm aware of any recently, but I vividly remember having wasted many hours hunting bugs in test code locally that were often caused by forgetting to rehash before accessing `.hash` or `.sha256` in my earlier contribution days (yeah, a beginner's mistake one coul
...

💬 odarboe commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855254428)
Concept NACK

This pull request serves as a litmus test for Bitcoin's governance:

Is Bitcoin truly community-driven, or is it controlled by its developers?
Are we upholding the principles of decentralization, or are we veering towards a centralized next-gen SWIFT system?

💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932)
Not related to this PR, but the [error in the `wallet_fees` target](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/14801894543/job/41562322239#step:8:484):
```
wallet_fees: succeeded against 23 files in 0s.
Run wallet_fees with args ['D:\\a\\bitcoin-core-nightly\\bitcoin-core-nightly\\build\\bin\\Debug\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_corpora/wallet_fees')]
\u26a0\ufe0f Failure generated from target with exit code 3221225477: ['D:\\a\\bitcoin-core-nightly\\bit
...

🤔 fanquake reviewed a pull request: "test: Suppress Windows abort message in CI"
(https://github.com/bitcoin/bitcoin/pull/32409#pullrequestreview-2818307258)
Not sure about compiler specific changes, that rely on the presence of an undocumented environment variable (which we don't explicitly set as far as I can tell?), to make tests work properly.

💬 fanquake commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2075491685)
The commit message says "On Windows", but the code is using `_MSC_VER`, not `WIN32`.
Is that a typo, or does this issue only affect MSVC compiled binaries; in which case, why is there a difference in runtime behaviour between the MSVC compiled test binaries & our release compiled test binaries?

💬 theStack commented on pull request "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows":
(https://github.com/bitcoin/bitcoin/pull/32419#issuecomment-2855291909)
Concept ACK

Makes sense to me to provide at least a brief description of how the key data is structured and deserialized, since this is not intuitive at all (as I also noticed in the course of reviewing https://github.com/bitcoin/bitcoin/pull/31247 a while ago).

💬 pablomartin4btc commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2075889487)
nit: the port is optional, no?
```suggestion
"ip[:<port>]|path";
```

🤔 pablomartin4btc reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2818995555)
If the user sets `-proxy=addr:port=tor` then `-onion=` shouldn't be allowed, no? Otherwise one could set 2 diff proxies for Tor(?).

Also, in [Tor's documentation](https://github.com/bitcoin/bitcoin/tree/master/doc/tor.md):

```
-proxy=ip:port Set the proxy server. If SOCKS5 is selected (default), this proxy
server will be used to try to reach .onion addresses as well.
```

So, shouldn't `-proxy=0=cjdns` be the default if `-cjdnsreachable` when `-proxy=addr:port`?

💬 pablomartin4btc commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2075906033)
nit: for the port value maybe use DEFAULT_TOR_SOCKS_PORT from `torcontrol.cpp`?

💬 yancyribbens commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#issuecomment-2855300344)
> As for the security risk, in many kinds of setups (no wallet, containerized, single-user-single-application, local-only, etc) it is an unlikely point of escalation

This probably applies to testnet and regtest too.

🚀 fanquake merged a pull request: "[29.x] qt: 29.1 translations update"
(https://github.com/bitcoin/bitcoin/pull/32352)

🚀 fanquake merged a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086)

👍 fanquake approved a pull request: "build: Fix `macdeployqtplus` after switching to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/32287#pullrequestreview-2819054059)
ACK 84de8c93e7d4979575161a2bb8f7eb64e1317b89