👋 brunoerg's pull request is ready for review: "test: add option to skip large re-org test in feature_block"
(https://github.com/bitcoin/bitcoin/pull/33003)
(https://github.com/bitcoin/bitcoin/pull/33003)
👍 willcl-ark approved a pull request: "ci: Use APT_LLVM_V in msan task"
(https://github.com/bitcoin/bitcoin/pull/32999#pullrequestreview-3029741547)
ACK fad040a5787a8ac0a13aef5c54e5a675de239e92
Code changes look correct to me.
In the CI run this installed `clang-20.1.7` from `apt`, before cloning `https://github.com/llvm/llvm-project -b llvmorg-20.1.7` and building the llvm runtimes with msan enabled.
nit: I do notice that bitcoin configure output claims it's using clang from `/bin/clang++` which differs from the `update-alternatives` dir used: `/usr/bin/clang++`, but it appears to be the correct version so I guess it's an `apt inst
...
(https://github.com/bitcoin/bitcoin/pull/32999#pullrequestreview-3029741547)
ACK fad040a5787a8ac0a13aef5c54e5a675de239e92
Code changes look correct to me.
In the CI run this installed `clang-20.1.7` from `apt`, before cloning `https://github.com/llvm/llvm-project -b llvmorg-20.1.7` and building the llvm runtimes with msan enabled.
nit: I do notice that bitcoin configure output claims it's using clang from `/bin/clang++` which differs from the `update-alternatives` dir used: `/usr/bin/clang++`, but it appears to be the correct version so I guess it's an `apt inst
...
💬 Sjors commented on issue "Guix build fails on M4 macOS host with Ubuntu in UTM":
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3084264672)
> Perhaps the M4 chipset doesn't allow for this functionality either.
That's quite possible, because trying to run the arm CI job also doesn't work, see #31344.
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3084264672)
> Perhaps the M4 chipset doesn't allow for this functionality either.
That's quite possible, because trying to run the arm CI job also doesn't work, see #31344.
💬 maflcko commented on pull request "ci: Use APT_LLVM_V in msan task":
(https://github.com/bitcoin/bitcoin/pull/32999#issuecomment-3084286466)
> bin
It is a symlink:
```
# podman run --rm 'ubuntu:questing' ls -ld /bin
lrwxrwxrwx 1 root root 7 Apr 24 16:59 /bin -> usr/bin
(https://github.com/bitcoin/bitcoin/pull/32999#issuecomment-3084286466)
> bin
It is a symlink:
```
# podman run --rm 'ubuntu:questing' ls -ld /bin
lrwxrwxrwx 1 root root 7 Apr 24 16:59 /bin -> usr/bin
💬 maflcko commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2213532921)
i wonder if the small test cases can be run before the reorg, to avoid having to branch on the move_tip
(https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2213532921)
i wonder if the small test cases can be run before the reorg, to avoid having to branch on the move_tip
💬 fanquake commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3084380035)
Put this on the `v30.0` milestone.
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3084380035)
Put this on the `v30.0` milestone.
💬 instagibbs commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3084382725)
Are the first two commits related to the PR? Seem REST-related?
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3084382725)
Are the first two commits related to the PR? Seem REST-related?
💬 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);
```