👍 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.
(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.)
(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
(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)
(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
(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...
(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?
(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.
(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
...
(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
(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.
(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
(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
(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
(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
(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.
(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.
💬 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_r2216319565)
Done
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216319565)
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_r2216320066)
Done
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216320066)
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_r2216323468)
I'm confused here. Can you clarify what you're suggesting?
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2216323468)
I'm confused here. Can you clarify what you're suggesting?
📝 Sjors opened a pull request: "wallet: support bip388 policy with external signer"
(https://github.com/bitcoin/bitcoin/pull/33008)
External signers such as Ledger, BitBox02 and Jade, when used with multisig, use [BIP388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) to display (and constrain) descriptor policies to their users.
At least in the case of Ledger (haven't tested the others) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.
This PR adds a new RPC call `registerpolicy` which converts our descriptor(s) into a BIP388 policy (
...
(https://github.com/bitcoin/bitcoin/pull/33008)
External signers such as Ledger, BitBox02 and Jade, when used with multisig, use [BIP388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) to display (and constrain) descriptor policies to their users.
At least in the case of Ledger (haven't tested the others) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.
This PR adds a new RPC call `registerpolicy` which converts our descriptor(s) into a BIP388 policy (
...