💬 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

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215719957)
Comment added in db78efac768

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215720797)
Registry env vars updated in 7272f56a755

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3089009002)
Thanks for the review @maflcko, I hope I addressed all your current review comments.

💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215722531)
Maybe not because comment above explicitly mentioned add, but that can be modified to match |.

see https://github.com/ismaelsadeeq/bitcoin/commit/95ea4e737612c094dfcb6414f8005c79c6a34551

🤔 maflcko reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3033068660)
Not sure the code is correct (see comment), but I've looked at it:

review ACK 248b6a27c351690d3596711cc36b8102977adeab 🎻

<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+
...

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2215746554)
nit in e7114fc6dc3488c2584d42779ff2b102e4d1db99: Not that it matters, but you can also use a span to avoid the copy:

```diff
- return HexStr(std::bit_cast<std::array<uint8_t, KEY_SIZE>>(m_rotations[0]));
+ return HexStr(std::as_bytes(std::span{&m_rotations[0], 1})));

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2215720251)
q in 6bbf2d9311b47a8a15c17d9fe11828ee623d98e0: Is this correct? Write may throw, but Read does not? So ignoring the return value makes it hard to see how this would be correct.

Why not just keep the code as-is here and use `m_obfuscation = new_key;`, which should be trivially correct?

💬 rkrux commented on pull request "wallet: derivehdkey RPC to get xpub at arbitrary path":
(https://github.com/bitcoin/bitcoin/pull/32784#issuecomment-3089115820)
Very nice, Concept ACK.
I will review this PR.

💬 0xB10C commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3089155125)
Concept ACK!

I think finishing the migration would close https://github.com/bitcoin/bitcoin/issues/31965

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213190752)
nit: I think the end is a bit redundant due to prior sentances, better to explain why:
```suggestion
when the reported `vsize` is the BIP‑141 value due to backwards compatibility.
```

🤔 hodlinator reviewed a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3026725085)
Reviewed 8158ca70a7b6b492607918d860947497efd72cd3

Found some final things before A-C-K.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2215809980)
Realized the comment is now out of date:
```diff
- // Add sigopsize if the transaction exists in the mempool and its size is affected by it.
+ // Add sigopsize if the transaction exists in the mempool.
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213198113)
Documentation of `getrawtransaction` says:
> By default, this call only returns a transaction if it is in the mempool.

So not sure what you mean by "not yet in the mempool".

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2215831224)
When building block templates, I'm not sure we want to recommend using the sigop-adjusted vsize? That might leave space over in the block if some transactions have a lot of sigops. This would make a miner earn less fees due to not filling up the block.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213166401)
meganit: This newline was logically separating the two commented code blocks and should probably be brought back.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213183113)
nit: It would be good to hyphen-prefix to make it stand out visually (from fields like `vsize` etc) and also call it "setting" or "argument" rather than "parameter" as those are the terms used in the code.
```suggestion
calculation (sigop count multiplied by a configurable `-bytesPerSigOp` setting, which defaults to
```