💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216093011)
nit in 20a258310ea4fe1c4cd280d26047aee063215454: Buildx always uses BuildKit. Can remove buildkit stuff.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216152725)
nit in 868ddf557180ffbc7a60ed539a5df1e1cd3e757c: this seems a bit fragile and won't work for monotree repos ( bitcoin-core/gui), and won't work for forks that accept pull requests.

Seems better to just hard-code this to cirrus, possibly add a comment: `# Run on cirrus by default, but in forks this can be replaced by 'ubuntu-latest' to run on GHA`

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216163712)
nit in 868ddf5: Yeah, seems required. Same for the other caches?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216107839)
nit in e5295f7ad64be1d834d3e0c2726edfcc67debfff: The script has tracing enabled, so i don't think this is needed?

👍 willcl-ark approved a pull request: "ci: Use optimized Debug build type in test-each-commit"
(https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-3033742204)
crACK faa3171ff22fea1c001e5a9b01d964aa425a3387

We can't see this in action in the CI here as test-each-commit job omits the top commit (with the meaningful changes in in this case), but I did run locally and tested that the flags were being correctly appended.

Left a few questions regarding the flag selections, but ACK running these checks in this job.

💬 willcl-ark commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#discussion_r2216173324)
Is `-03` chosen here to try and counteract the slow-down from `Debug` as opposed to using `-02`?

IIRC I did some tests of `-02` vs `-03` and didn't find much/any speedup, so just curious if that was the reason. If it's been chosen to expose new classes of bugs not found by `-03`, then seems fine to me.

How does `-03` actually play with `-g2`? Do the agressive optimisations play OK with debugging? (I've never tried this combination I don't think.)

💬 maflcko commented on pull request "test: fix `ReadTopologicalSet` unsigned integer overflow":
(https://github.com/bitcoin/bitcoin/pull/33007#issuecomment-3089665005)
lgtm ACK 31c4e77a256c15c221dcc278883f6d3be55660b4

💬 maflcko commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#discussion_r2216194368)
Probably doesn't matter much if this is O2 or O3. Maybe O3 is 1% - 4% faster, according to https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2660973776 ? Any of those can probably be changed, if there is a reason or need.

The main goal here is to add `Debug` (and the checks it ships with)

💬 marcofleon commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3089686217)
ReACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519

`range-diff 33332cf86a..fa501ea5ed fa11eea405..fa1a14a13a` lgtm

💬 marcofleon commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2216201422)
Nice find.

> IntSan will notify about it

I forget about IntSan...

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216249714)
If you want it to be easy to edit, I wonder if this can be a separate yaml file so that the edit only has to be done in one place?

💬 marcofleon commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3089778906)
Thanks, was able to reproduce.

```c
fuzz: ../../src/validation.cpp:428: bool CheckInputsFromMempoolAndCache(const CTransaction &, TxValidationState &, const CCoinsViewCache &, const CTxMemPool &, unsigned int, PrecomputedTransactionData &, CCoinsViewCache &, ValidationCache &): Assertion `!coinFromUTXOSet.IsSpent()' failed.
```

Now the comment for `m_view` in 98ba2b1db2eb81e08b550ba0d5069a9289f30e13 makes more sense to me as well.

💬 stickies-v commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2216305511)
> All parameter interaction is (and was) info level

Fair enough, that's at least a good reason to not do it here but in a separate pull.

> A warning is for the node operator to investigate and possibly fix.

That's exactly why I was thinking it should be warning: a node operator "misconfigured" their node. It may be fine, but the node may also be behaving differently than what they expect, so best is for the node operator to investigate and fix the interaction.

Anyway, can be marked a
...

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216313380)
Done

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216314444)
I couldn't add an assert message, but I added a comment.

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216314650)
Done

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216314937)
Done

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216315188)
Done

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216315538)
Done

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216319131)
`ToStringHelper` now accepts a pointer to `has_priv_key` in https://github.com/bitcoin/bitcoin/pull/32471/commits/2a1f1f0e32829af43436eb4220164c1be86229bb
It does look cleaner, but there's a need to check for `nullptr` now.