💬 maflcko commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213588799)
Agree; thx, done
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213588799)
Agree; thx, done
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3084437964)
> may be good to fix the aarch64 ci.
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3084437964)
> may be good to fix the aarch64 ci.
Fixed.
💬 sybot99 commented on issue "GUI bitcoin core shows wrong amount":
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3084461340)
> coin control is an expert feature, but you can enable it in
>
> ```
> Settings > Options > Wallet
> Check "Enable coin control features (experts only!)"
> ```
>
> The RPC has a manual, you can just go to "Window"->"Console" and type `help`.
Yes I understand this. But I dont understand how to send that btc(UTXO as I understand) from invisible wallet to my visible wallet with all other btc )
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3084461340)
> coin control is an expert feature, but you can enable it in
>
> ```
> Settings > Options > Wallet
> Check "Enable coin control features (experts only!)"
> ```
>
> The RPC has a manual, you can just go to "Window"->"Console" and type `help`.
Yes I understand this. But I dont understand how to send that btc(UTXO as I understand) from invisible wallet to my visible wallet with all other btc )
💬 stickies-v commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213637486)
Printing the exception `e` seems more useful than "Unexpected exception", and imo completely obviates the need for having the `CalledProcessError` and `KeyboardInterrupt` exceptions?
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213637486)
Printing the exception `e` seems more useful than "Unexpected exception", and imo completely obviates the need for having the `CalledProcessError` and `KeyboardInterrupt` exceptions?
🤔 rkrux reviewed a 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#pullrequestreview-3025316172)
Partial review bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3025316172)
Partial review bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3
💬 rkrux 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_r2210645024)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
Not a bug but `bitwise or` for boolean operations is not ideal.
```diff
- has_priv_key |= tmp;
+ has_priv_key = has_priv_key || tmp;
```
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2210645024)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
Not a bug but `bitwise or` for boolean operations is not ideal.
```diff
- has_priv_key |= tmp;
+ has_priv_key = has_priv_key || tmp;
```
💬 rkrux 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_r2210652033)
In bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3 "test: Test listdescs with priv works even with missing priv keys"
Nit: I prefer to only check for the data that we really want to assert on, helps in keeping the tests code cleaner as well besides being minutely faster. Checking `desc` for every imported descriptor in this case.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2210652033)
In bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3 "test: Test listdescs with priv works even with missing priv keys"
Nit: I prefer to only check for the data that we really want to assert on, helps in keeping the tests code cleaner as well besides being minutely faster. Checking `desc` for every imported descriptor in this case.
💬 rkrux 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_r2210641706)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
Fine to add `assert` here because for all intents and purposes I don't suppose there would ever be a case where `ToStringHelper` returns `false` here. Perhaps we can add a helpful assert message here for debugging purposes?
```diff
- assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key));
+ assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key), "");
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2210641706)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
Fine to add `assert` here because for all intents and purposes I don't suppose there would ever be a case where `ToStringHelper` returns `false` here. Perhaps we can add a helpful assert message here for debugging purposes?
```diff
- assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key));
+ assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key), "");
...
💬 rkrux 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_r2210625173)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
```diff
+ @return true if at least one private key available
```
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2210625173)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
```diff
+ @return true if at least one private key available
```
💬 rkrux 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_r2213264627)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
It would be helpful to update the function doc in `PubkeyProvider` struct to mention the new behaviour.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2213264627)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
It would be helpful to update the function doc in `PubkeyProvider` struct to mention the new behaviour.
💬 rkrux 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_r2210669794)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
Nit: To keep it consistent with the other pubkey providers and to make it explicit here. Though a default is specified in `ToString`.
```diff
- out = ToString();
+ out = ToString(StringType::PUBLIC);
```
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2210669794)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
Nit: To keep it consistent with the other pubkey providers and to make it explicit here. Though a default is specified in `ToString`.
```diff
- out = ToString();
+ out = ToString(StringType::PUBLIC);
```
💬 rkrux 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_r2213293926)
In https://github.com/bitcoin/bitcoin/commit/a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
```diff
- bool has_priv_key;
- ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, has_priv_key);
+ bool dummy;
+ ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, dummy);
```
Because there is no use of this variable in `ToString`.
Same for `ToNormalizedString` fun
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2213293926)
In https://github.com/bitcoin/bitcoin/commit/a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
```diff
- bool has_priv_key;
- ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, has_priv_key);
+ bool dummy;
+ ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, dummy);
```
Because there is no use of this variable in `ToString`.
Same for `ToNormalizedString` fun
...
💬 rkrux 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_r2213477946)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
This would also aid readability because under no condition `has_priv_key` can be `false` here.
```cpp
Assume(has_priv_key);
```
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2213477946)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
This would also aid readability because under no condition `has_priv_key` can be `false` here.
```cpp
Assume(has_priv_key);
```
💬 rkrux 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_r2213427874)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
```diff
- bool tmp{false};
- auto key_str{ctx.ToString(key, tmp)};
+ bool fragment_has_private_key{false};
+ auto key_str{ctx.ToString(key, fragment_has_private_key)};
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2213427874)
In a0a458965c944434b1f3e4d83277257793435edb "descriptor: ToPrivateString() pass if at least 1 priv key exists"
```diff
- bool tmp{false};
- auto key_str{ctx.ToString(key, tmp)};
+ bool fragment_has_private_key{false};
+ auto key_str{ctx.ToString(key, fragment_has_private_key)};
💬 stickies-v commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213651061)
Logs with special case handling:
```
2025-07-17T15:07:39.774663Z TestFramework (ERROR): Called Process failed with 'None'
2025-07-17T15:18:22.039274Z TestFramework (WARNING): Exiting after keyboard interrupt
```
Logs without special case handling:
```
2025-07-17T15:19:09.169925Z TestFramework (ERROR): CalledProcessError(1, ['ls', '--invalid-flag'])
2025-07-17T15:19:26.639162Z TestFramework (ERROR): KeyboardInterrupt()
```
Imo, they're equally informative for KeyboardInterrupt, an
...
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213651061)
Logs with special case handling:
```
2025-07-17T15:07:39.774663Z TestFramework (ERROR): Called Process failed with 'None'
2025-07-17T15:18:22.039274Z TestFramework (WARNING): Exiting after keyboard interrupt
```
Logs without special case handling:
```
2025-07-17T15:19:09.169925Z TestFramework (ERROR): CalledProcessError(1, ['ls', '--invalid-flag'])
2025-07-17T15:19:26.639162Z TestFramework (ERROR): KeyboardInterrupt()
```
Imo, they're equally informative for KeyboardInterrupt, an
...
💬 hebasto commented on issue "Avoid plural forms in non-GUI translatable strings (lacks `%n` support)":
(https://github.com/bitcoin/bitcoin/issues/31890#issuecomment-3084488155)
> Was there any followup here?
Below is the response I received:
> Hi, Hennadii
>
> Here are some pluralization guidelines I wrote for one of my clients. Feel free to share them with the team:
>
> “Use proper plural forms
>
> Pluralization (p11n) is the process of changing nouns from the singular to the plural form. Not all languages change plural forms of nouns in the same way, some don’t have any plural form.
>
> There is a difference between plural forms and plural rules
>
> Plural form
...
(https://github.com/bitcoin/bitcoin/issues/31890#issuecomment-3084488155)
> Was there any followup here?
Below is the response I received:
> Hi, Hennadii
>
> Here are some pluralization guidelines I wrote for one of my clients. Feel free to share them with the team:
>
> “Use proper plural forms
>
> Pluralization (p11n) is the process of changing nouns from the singular to the plural form. Not all languages change plural forms of nouns in the same way, some don’t have any plural form.
>
> There is a difference between plural forms and plural rules
>
> Plural form
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3084511037)
review ACK ec37e518682ca6c7272d1eeaea099dc9b3375e41 🌙
<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 ec37e518682c
...
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3084511037)
review ACK ec37e518682ca6c7272d1eeaea099dc9b3375e41 🌙
<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 ec37e518682c
...
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2213672519)
Has this been upstreamed / why is it needed?
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2213672519)
Has this been upstreamed / why is it needed?
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2213681709)
> Has this been upstreamed...
Not yet.
> why is it needed?
Otherwise, IWYU will suggest platform-specific headers instead of the standard one provided by the C library.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2213681709)
> Has this been upstreamed...
Not yet.
> why is it needed?
Otherwise, IWYU will suggest platform-specific headers instead of the standard one provided by the C library.
💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2213683784)
I'm skeptical if this is a possible scenario with today's index base code (which is much better with reorgs / unexpected restart scenarios than it was back then), but it's definitely more complicated than I thought, so this PR is not appropriate for this - I dropped the commit and will resolve this discussion.
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2213683784)
I'm skeptical if this is a possible scenario with today's index base code (which is much better with reorgs / unexpected restart scenarios than it was back then), but it's definitely more complicated than I thought, so this PR is not appropriate for this - I dropped the commit and will resolve this discussion.