💬 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

👍 stickies-v approved a pull request: "test: Do not pass tests on unhandled exceptions"
(https://github.com/bitcoin/bitcoin/pull/33001#pullrequestreview-3032970350)
re-ACK faa3e684118bffa7a98cf76eeeb59243219df900

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-3088912601)
Follow ups to these last few comments can be found here: https://github.com/bitcoin/bitcoin/pull/33005

💬 rkrux commented on pull request "Have createwalletdescriptor auto-detect an unused(KEY)":
(https://github.com/bitcoin/bitcoin/pull/32861#issuecomment-3088939545)
Thanks, I recall reading this comment earlier but forgot about it later. If I am not missing anything, I don't suppose this point is orthogonal to this PR?

The linked comment also states that any RPC using the unused descriptor should delete it immediately afterwords.
> add logic to importdescriptors and generatewalletdescriptor to delete any unused(KEY) descriptor as soon as the KEY which it references is used by an new descriptor (taking into account both public and private parts of the k
...

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3088988915)
Pushed c126475ed7a17ec9030066056e31846c7124dcf3 with a CI run on master branch at https://github.com/testing-cirrus-runners/bitcoin2/actions/runs/16368410249

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215709407)
switched to `job-id` in 9458cd0e66d

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215710431)
This now uses `git rev-parse HEAD:depends` in both 868ddf55718 and 1b689cf7811

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215711488)
comment removed in 9458cd0e66d

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215715420)
All references to `master` have been switched to `github.event.repository.default_branch` where appropriate. Removed the `ref_name` from the save/restore keys, as it is indeed unnecessary.

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215717870)
9458cd0e66d now uses a depends sources cache per job_id to avoid this pitfall (thanks!):

```
key: depends-sources-${{ inputs.job-id }}-${{ env.DEPENDS_HASH }}
```

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215719446)
Moved this code into the main `ci.yml` file, so that the cache actions are *only* performing caching (rather than being responsible for setting env vars), both in 868ddf55718 and 1b689cf7811