💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075614393)
Looks correct "horas" means hours, and this isn't about hours. It is introducing a redundant space before the dot here.

💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075622675)
Right! At some point we should refactor entries like this to
```c++
box.setText(tr("Are you sure you wish to close the wallet %1?").arg("<i>" + GUIUtil::HtmlEscape(wallet_model->getDisplayName() + "</i>")));
```
But not for this version ofc.

👍 laanwj approved a pull request: "[29.x] qt: 29.1 translations update"
(https://github.com/bitcoin/bitcoin/pull/32352#pullrequestreview-2818619507)
ACK fc60337733a9dffaa42e08fcbff0ab24b5f679a4

i looked over the changes and did some spot checks and found nothing that looks malicious or appears out of the ordinary.

💬 ryanofsky commented on pull request "[DRAFT] ipc: add windows support":
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2854925140)
Updated f215e742045401ab386f0cd67d06b358558ecf89 -> 5f272b7cc3b72fe4a59f590825b68dc2c342baca ([`pr/ipc-win.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.5) -> [`pr/ipc-win.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.5..pr/ipc-win.6)) with more fixes.

With these fixes, IPC is fully working on windows: both creating child processes and communicating over socket pairs, and listening and making con
...

🤔 rkrux reviewed a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2818659786)
> The importdescriptors RPC sets the range for all non-ranged descriptors to [0,1] in every request so this error cannot be triggered using the RPC.

I checked that the `importdescriptors` adds a 1 to range end if a range is given in the request.

> that this error may be unexpectedly triggered when " experimenting with non-ranged descriptors in custom wallet workflows.

The issue seems quite peculiar in that it highlights an issue within an internal function instead of the full functional
...

💬 sr-gi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2854968169)
I think the limits have proven ineffective and would be in favour of dropping them. However, I also acknowledge this has created friction amongst users who think they should have a saying in what their node configuration should be. While I think this is a placebo at best and most likely a shot in the foot, I'd be in favour of increasing the limit but not removing the configuration option.

We can revisit this in the future, backed up with data on how the increase has affected the inclusion of
...

🤔 marcofleon reviewed a pull request: "test: added fuzz coverage for consensus/merkle.cpp"
(https://github.com/bitcoin/bitcoin/pull/32243#pullrequestreview-2818609635)
Good stuff, nice to have new coverage.

I think it'd be worth it to add an oracle in the test where you reconstruct the merkle root with the path you get from `TransactionMerklePath` and verify that it matches the root from the earlier `ComputeMerkleRoot` call.

💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2075711453)
Meant to remove this line?

💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2075666086)
This should be 0 to `num_txs - 1` no? Just based on the [coverage](https://marcofleon.github.io/coverage/merkle/coverage/root/bitcoin/src/consensus/merkle.cpp.html) it seems the fuzzer is choosing `num_txs` which means `matchh` won't be set to true here:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/consensus/merkle.cpp#L111

💬 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