💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104340229)
The original documentation is also a bit unclear.

> A Coin may be locked if it has already been used to fund a transaction that hasn't confirmed yet.

Is this referring to transactions that were generated either:
1. outside the current wallet and not in our mempool
2. by our wallet but without broadcast, e.g. PSBT workflow with `lock_unspents`

💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104348210)
11d0513379d9cbb375f5f6e8877743872225a0ff: why don't we persist this?

Could be a separate commit if we want to change the behaviour.

(ditto for the other RPC methods)

💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104326011)
Could document this to point out that this will update the wallet database if the coin is persisted.

💬 willcl-ark commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2904140202)
OK a second full guix build got the same hashes as my first:

```
src/core/worktree-alpine-MKDIRPROG on  alpine_MKDIRPROG:refs/pull/32568/head [$] via △ v3.31.6 via 🐍 v3.12.10 via ❄️ impure (nix-shell-env) took 1h31m29s
❯ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
cb7dbe6112cdd38db9757af64cd4e617cdced8a6525db8a1469f3f35d29de9f6 guix-build-df9ebbf659d5/output/aarch64-linux-gnu/SHA256SUMS.part
a0b2b6c3fd30ae9aa766
...

💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2904185367)
Guix Build:
```bash
47df12bafda434889c080ec544817f1058d95dda7d79889ea59bcc42084d73e5 guix-build-24e5fd3bedce/output/aarch64-linux-gnu/SHA256SUMS.part
b0cd4b29b7d3c3d3858aeb3496e7f7c0b0910f6bcaa3c76d537053a2f6e96f2f guix-build-24e5fd3bedce/output/aarch64-linux-gnu/bitcoin-24e5fd3bedce-aarch64-linux-gnu-debug.tar.gz
3d3228c719e4a7df4cd83c2b2d46c6ffd072d331b83cf5f70b938cc976908b91 guix-build-24e5fd3bedce/output/aarch64-linux-gnu/bitcoin-24e5fd3bedce-aarch64-linux-gnu.tar.gz
90ed4ba3126cc7f1
...

💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2904253640)
Pushed an update which now only activates the dependency provider when using the depends toolchain.

This will hopefully appease the windows CI job, as well as making more sense in general and being more expected behaviour.

🤔 janb84 reviewed a pull request: "wallet, rpc: Return normalized descriptor in parent_descs"
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2864259747)
ACK 107517ea7d29d959e7a0b94d8217dcb894680547

Before change listtransactions (and others) shows un-normalized descriptors, after the descriptors are normalized


<details>
master:

```
{
"address": "bcrt1qgymj02gx6dzrkh49d9sj0cj8rxfcrut7dyxz7r",
"parent_descs": [
"wpkh(tpubD6NzVbkrYhZ4YdzwuYwQLEypddJ64KVEG9tyZibLxLKMxyMCaru1c7RFX2S8NEQ7dZuNccdzk5qikw51rxzXYWdkeuREgxgdqbJy3ty68qy/84h/1h/0h/0/*)#tz7s9af2"
],
"category": "immature",
"amount": 50.00000000,
...

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2104498772)
Thanks! Fixed in https://github.com/bitcoin/bitcoin/pull/32540/commits/9d7e23e2f505ce6cbe830fc607cf203b1a48ba0d.

📝 hebasto opened a pull request: "test: Fix `system_tests/run_command` test"
(https://github.com/bitcoin/bitcoin/pull/32601)
This PR addresses two issues related to the "Return non-zero exit code, with error message for stderr" check in the `system_tests/run_command` test:
1. The test now checks the exact exit code, which must be 1.
2. The process invocation string is removed from the exception message before checking the stderr output, as the latter is a substring of the former.

This is an alternative to https://github.com/bitcoin/bitcoin/pull/32577 without introducing a shell wrapper and altering quotation in `
...

✅ hebasto closed a pull request: "subprocess: Let shell parse command on non-Windows systems"
(https://github.com/bitcoin/bitcoin/pull/32577)

💬 hebasto commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2904327201)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/32601.

💬 hebasto commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2104533575)
https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2902382891:

> - I don't think the added quotes in external_signer.cpp are doing anything, or it isn't clear what they are trying to do.

Descriptors contain the `(` and `)` characters, which may cause issue with shell:
```
$ echo pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)
bash: syntax error near unexpected token `('
```

💬 maflcko commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104545752)
This will just re-introduce the bugs fixed in https://github.com/bitcoin/bitcoin/pull/31542, no?

💬 maflcko commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104546796)
A better option would be to use the test's temp dir as scratch space

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104550766)
> This will just re-introduce the bugs fixed in #31542, no?

Oops, forgot about it :facepalm:

📝 hebasto converted_to_draft a pull request: "test: Fix `system_tests/run_command` test"
(https://github.com/bitcoin/bitcoin/pull/32601)
This PR addresses two issues related to the "Return non-zero exit code, with error message for stderr" check in the `system_tests/run_command` test:
1. The test now checks the exact exit code, which must be 1.
2. The process invocation string is removed from the exception message before checking the stderr output, as the latter is a substring of the former.

This is an alternative to https://github.com/bitcoin/bitcoin/pull/32577 without introducing a shell wrapper and altering quotation in `
...

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104493385)
in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: nit, could use the helper here:
```suggestion
if (IsRangedDerivation() || !m_path.empty()) {
```

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104489088)
in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: the byteswap comment is obsolete now
```suggestion
//! MuSig2 chaincode as defined by BIP 328
using namespace util::hex_literals;
constexpr uint256 MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
```

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104518220)
in commit 739f13986a49d8d7d5568ebcdd88665ea8423d42: could add a doxygen comment for the newly introduced parameter

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104576393)
Why is the `key_exp_index` parameter changed to pass by reference? Without that the unit tests fail, so it seems necessary, but I'm confused why this was not needed earlier already for similar constructs (e.g. for `multi()` expressions where the value is also incremented in the key expression parsing loop). Probably I'm missing some basic descriptors parsing knowledge.