👍 rkrux approved a pull request: "wallet: Expand MuSig test coverage and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33636#pullrequestreview-3510456391)
lgtm ACK 217dbbb

Thanks for accepting all the suggestions.

🚀 fanquake merged a pull request: "depends: latest config.guess & config.sub"
(https://github.com/bitcoin/bitcoin/pull/33945)

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#issuecomment-3580889312)
In PR description:

```diff
- primarily adds test coverage for some of the most prominent failure cases in the first commit.
+ primarily adds test coverage for some of the most prominent failure cases in the last commit.
```

⚠️ maflcko opened an issue: "node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed."
(https://github.com/bitcoin/bitcoin/issues/33948)
Just ran into this crash in the nightly CI, which was using llvm `22~++20251125085442+ed95c4d6ecf0-1~exp1~20251125085501.1311`. It may be a compiler error, but it is odd that only one test was failing:

https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19692916493/job/56412271437#step:10:6430

```
node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed.
```

...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564638844)
> The first two are just optimizations for logarithmic operations

Just a minor correction, not relevant with the recent changes, but anyway - the first container, "by txid" is where the transactions are stored. The 2nd and 3rd are the indexes. "by priority" is (was) logarithmic and "by node" was `O(1)`.

In the latest push I removed those.

Also, "priority" is something that changes when the transaction is sent more times. So, I went in the direction of storing how many times and when a t
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564640474)
"Resolving" since this code was removed.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564641818)
"Resolving" since this code was removed.

👍 stickies-v approved a pull request: "Change Parse descriptor argument to string_view"
(https://github.com/bitcoin/bitcoin/pull/33914#pullrequestreview-3510482245)
ACK c0bfe72f6e1f63e05772eda959137b3d0bbbf6c3

Thanks for improving this. I switched to taking a span in b3bf18f0bac0ffe18206ee20642e11264ba0c99d to minimize code changes, but didn't think of the string literal case. For a string parsing function, passing string literals should just work out of the box, so this approach is definitely better.

💬 stickies-v commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2564635705)
nit: I don't think `sp` is a particularly helpful variable name, would prefer `descriptor_span`

💬 stickies-v commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2564629268)
nit: doesn't make a functional difference because it's the end of the test case, but this should be `CHECK` since nothing further depends on `desc`'s non-emptiness

```suggestion
BOOST_CHECK_MESSAGE(!descs.empty(), err);
```

💬 maflcko commented on pull request "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO":
(https://github.com/bitcoin/bitcoin/pull/33702#issuecomment-3580916853)
rebased for a one-line conflict and a one-line silent conflict. Should be trivial to re-review via git range-diff

👍 stickies-v approved a pull request: "fix: remove redundant mempool lock in ChainImpl::isInMempool()"
(https://github.com/bitcoin/bitcoin/pull/33946#pullrequestreview-3510522471)
ACK 2909655fba91a7cc59c484fc74afafdf7ccc0cfa

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3580941170)
review ACK 2e27bd9c3af91eb9fcc626fe65d065df0a80974d 🖊

<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 2e27bd9c3af9
...

💬 maflcko commented on pull request "interfaces: remove redundant mempool lock in ChainImpl::isInMempool()":
(https://github.com/bitcoin/bitcoin/pull/33946#issuecomment-3580958940)
lgtm ACK 2909655fba91a7cc59c484fc74afafdf7ccc0cfa

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2564710993)
I agree but we use `sp` in related functions too.

👍 theStack approved a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3510583287)
re-ACK fad61185861a6a9ed806c387aa63d2b31262b1db

as per `$ git range-diff fa2fd0ba1f...fad6118586`

💬 Sjors commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2564711983)
Will change if I need to retouch.

💬 stickies-v commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2564733112)
There is value in consistency, but I don't think we shouldn't improve naming because it's used somewhere else, especially when it's an implementation detail.

That said, this is a nit, and I don't think it's worth invalidating acks as it looks RFM.

💬 stickies-v commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3581039213)
ACK 3e01b5d0e7 modulo the new .tar SDK being uploaded

💬 naiyoma commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2564780295)
Done