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

Same heat as https://github.com/bitcoin/bitcoin/pull/28130 2 years ago. This should have been closed already.

⚠️ willcl-ark opened an issue: "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install"
(https://github.com/bitcoin/bitcoin/issues/32428)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

On a fresh NixOS install when building using depends, the generated toolchain does not pass along enough info for the depends packages to be found by `cmake`:

- Following the reproduction steps below will cause `cmake -B build --toolchain <toolchain>` to error with:

```log
-- The CXX compiler identification is Clang 19.1.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI
...

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2855056187)
> It's unfortunate to lose this much performance, as suggested before, maybe the change from 24 hours to 1 hour was too big - what if we tried an average of 100 minutes instead?

I'm not sure it's worth bike shedding over the amount of time to periodically flush. The difference with an HDD is 6 hours 13 minutes vs 6 hours 53 minutes. So we could be saving a maximum of 40 minutes over something that takes over 6 hours anyways.

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2855076711)
Reverted the added `Q_DECLARE_METATYPE` following https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2854604811. I'm assuming a lot of the metatype code will be dropped eventually now that we use Qt 6.

💬 Fiach-Dubh commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2855079387)
> @moth-oss config options are not removed in this pull request.

This config option is, however, marked for future removal and is being marketed for future removal by the author of the PR by being defined as "DEPRECATED". I assume this will mean the removal of this toggle eventually when the controversy has died down and people aren't paying attention.

I believe this "DEPRECATED" definition is up for debate. Marking a toggle that has worked for my mempool policy and will work for my mempo
...

💬 va7wv commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855081873)
This is really sad. How is this an improvement to soverignty to eliminate a configuration option for node runners?

💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075768390)
These parenthesized additions in non-Thai letters like (Song Soot Thai) here are a bit strange (Google translate can't make anything from it). Might want to ask someone who knows the language.

💬 ryanofsky 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-2855091032)
I debugged this issue at some point before and have been working around it since by setting the PATH variable when using depends like:

```
export PATH=$PATH:$PWD/depends/x86_64-pc-linux-gnu/include
export PATH=$PATH:$PWD/depends/x86_64-pc-linux-gnu/lib
cmake -B build -DCMAKE_TOOLCHAIN_FILE=depends/x86_64-pc-linux-gnu/toolchain.cmake
```

The problem is caused by `set(CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH OFF)` in the toolchain file:

https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a
...

👍 theStack approved a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2818667962)
Code-review ACK b6101b1849fbc2ec510c56cfae39d7de244a50cc

Left some nits below, nothing critical. As others have already noted, some more test coverage for certain areas would be nice, but can be done in a follow-up, e.g.:
* valid signature encoding check in parsing of `PSBT_IN_PARTIAL_SIG` (in particular, that a [high-s signature is accepted](https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2064644621))
* [`PSBTError::INCOMPLETE`](https://github.com/bitcoin/bitcoin/pull/31622#discu
...

💬 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_r2075699230)
Rather than sending and mining only one finalized transaction, could instead use `testmempoolaccept` within the loop above, so the full check if the transaction is valid can be done for both the `wallet_psbt` and `desc_psbt` result (currently it's only done for the latter; the first `fin_hex` result is overwritten).

💬 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_r2075730975)
nit: `addr` is never used anywhere else, could directly construct the `outputs` object in one go

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