💬 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
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2211498969)
Side note: I think it's weird that we would not include the sizes when exceeding the max fee rate - one would think that it would be interesting to know then too. But it seems to have been the same since 2233a93a109b10b6fe0f5f26c2bb529c8de3dde7 when `vsize` was added. Not suggesting it be changed in this PR, but possible follow-up if others agree.

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2215907801)
the diff is already at more than 100 lines, so i wanted to keep it minimal for now

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2215916022)
I disagree, because:


* All parameter interaction is (and was) info level (see also `LogInfo("parameter interaction: -blocksonly=1 -> setting -maxmempool=%d\n", DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB);` in init cpp)
* A warning is for the node operator to investigate and possibly fix. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging
* The `-blocksonly` docs already explain the interaction, so if someone ignores the official docs, hiding a warning in the debug l
...