💬 maflcko commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3086818557)
See https://github.com/bitcoin/bitcoin/tree/master/contrib/guix#bootstrappable-bitcoin-core-builds

💬 maflcko commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2214958175)
> I kinda like seeing where it got interrupted

Yeah, I realize it is useful to debug timeouts. Pushed a fresh commit for `KeyboardInterrupt`.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3087403014)
> This increases the scope quite a bit, and if we're going to demonstrate mining, I think it should be done accurately.

I don't think the goal is to _demonstrate_ mining, it's to increase (functional) test coverage.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2215166231)
It seems fine to mine on mainnet for a fraction of a second.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2215171067)
For the purpose of test coverage, it's good to call it this with an invalid solution.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2215175895)
Regtest has an extremely low difficulty, so it's (almost?) impossible to not find a solution. However it does seem more correct to handle the failure case.

💬 maflcko commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569)
This doesn't mean mask is non-zero if non-empty, if that is the goal. Also, if it isn't, IntSan will notify about it:

```
echo 'AQAAAID+/v7+/v7+/n8=' | base64 --decode > /tmp/f.crash

UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=clusterlin_ancestor_finder ./bld/bin/fuzz -runs=1 /tmp/f.crash
```

```
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1905069321
INFO: Loaded 1 modules (588821
...

💬 maflcko commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3088548960)
review ACK 8810642b571e1d8482375e962a1129b691d5d226 🥁

<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 8810642b571e
...

🤔 janb84 reviewed a pull request: "cmake: Create subdirectories in build tree in advance"
(https://github.com/bitcoin/bitcoin/pull/32773#pullrequestreview-3032634011)
re ACK fa3beb6316d426b7e388d71fa2f09aed4d2da3d2

Changes sinds last ACK:
- Solved Merge conflict.

Retested, still worked as described. ✅
Code review ✅

💬 mprenditore commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-3088660266)
Hello, is possible to review it again? It should be all good now.
Thanks a lot for the support :)

💬 rkrux commented on pull request "Have createwalletdescriptor auto-detect an unused(KEY)":
(https://github.com/bitcoin/bitcoin/pull/32861#issuecomment-3088814090)
> This PR makes createwalletdescriptor a bit smarter so it just finds the unused(KEY) generated by hdkey and uses that.

Once the unused(KEY) descriptor is used, doesn't it become used? Keeping it as an unused descriptor later might come across as confusing.

💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215602479)
Nice this can be prevented by only incrementing by one when the mask is empty
```diff
diff --git a/src/test/fuzz/cluster_linearize.cpp b/src/test/fuzz/cluster_linearize.cpp
index ef20eeaea0d..9c5db4f1495 100644
--- a/src/test/fuzz/cluster_linearize.cpp
+++ b/src/test/fuzz/cluster_linearize.cpp
@@ -304,7 +304,7 @@ SetType ReadTopologicalSet(const DepGraph<SetType>& depgraph, const SetType& tod
try {
reader >> VARINT(mask);
} catch(const std::ios_base::failure&) {}
-
...

💬 rkrux commented on pull request "Have createwalletdescriptor auto-detect an unused(KEY)":
(https://github.com/bitcoin/bitcoin/pull/32861#discussion_r2215603326)
In e8b1440aa3ed7efe3a9c2d10afb05e7247fff1f6 "rpc: make createwalletdescriptor smarter"

Though this check here seems more thorough, but the presence of more than one `spkm` also seems sufficient to throw this error?

💬 Sjors commented on pull request "Have createwalletdescriptor auto-detect an unused(KEY)":
(https://github.com/bitcoin/bitcoin/pull/32861#issuecomment-3088848116)
> Keeping it as an unused descriptor later might come across as confusing.

I agree, and it was also brought up here: https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-1868202696 It's orthogonal to this PR.

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2215623353)
This is not surprising at all you can easily see that by looking at the subscribers of this notification.
Fwiw, I like the change, but I think the last comment in the sentence should be removed. We're not updating the fee logic inside the subroutine; it's done in the scheduler thread in the background. Hence, I’m approaching ~0 as-is.
Based on https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review

💬 maflcko commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3088867703)
review ACK 96da68a38fa295d2414685739c41b8626e198d27 🚋

<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 96da68a38fa2
...

💬 maflcko commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2215637564)
It is also updating the fee logic (`lastRollingFee`), so the comment isn't wrong. No strong opinion, though 😅

💬 maflcko commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215643371)
Could also use `mask|=non_empty;` (Maybe the `+` was a typo and meant `|`?)

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2215648241)
Okay, I see that :)
Yeah the comment matches that update, I guess that ~0 does not apply any more, can be resolved.

👍 ismaelsadeeq approved a pull request: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3032959527)
Code review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50