💬 Sjors 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_r2241845825)
In 2c0a84f1e2ebe98bcf5066acb0de0ba5138c2101 _descriptors: add IsWatchOnly()_: this doesn't describe the actual behavior. IIUC:
```cpp
/** Whether this descriptor is missing any private key material */
```
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2241845825)
In 2c0a84f1e2ebe98bcf5066acb0de0ba5138c2101 _descriptors: add IsWatchOnly()_: this doesn't describe the actual behavior. IIUC:
```cpp
/** Whether this descriptor is missing any private key material */
```
💬 Sjors 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_r2241953374)
In b32c2b290f8e4dcb7fe54ee4c81714a3785920a7 _descriptor: ToPrivateString() pass if at least 1 priv key exists_:
To preserve existing behaviour:
```cpp
if (has_priv_key) *has_priv_key = true;
```
It's a bit worrying that the test passes whether I set this to `false` or `true`. But that's a pre-existing issue, as the following change to the original code doesn't break the test either:
```diff
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 0a32727f
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2241953374)
In b32c2b290f8e4dcb7fe54ee4c81714a3785920a7 _descriptor: ToPrivateString() pass if at least 1 priv key exists_:
To preserve existing behaviour:
```cpp
if (has_priv_key) *has_priv_key = true;
```
It's a bit worrying that the test passes whether I set this to `false` or `true`. But that's a pre-existing issue, as the following change to the original code doesn't break the test either:
```diff
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 0a32727f
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3135423918)
Rebased 3d6b3fd8f65e8d4ea2c26d61c3dee89f5da10fac -> 6a9fdf7ae58a85ccc08c5f6917f64f28f5a330ad ([kernelApi_48](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_48) -> [kernelApi_49](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_49), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_48..kernelApi_49))
* Fixed conflict with #33079
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3135423918)
Rebased 3d6b3fd8f65e8d4ea2c26d61c3dee89f5da10fac -> 6a9fdf7ae58a85ccc08c5f6917f64f28f5a330ad ([kernelApi_48](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_48) -> [kernelApi_49](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_49), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_48..kernelApi_49))
* Fixed conflict with #33079
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242080695)
> Set this to run on PRs in [fd32aa8](https://github.com/bitcoin/bitcoin/commit/fd32aa85b29f90c44e4a08f247ee6b1840ff7b69) ci: port lint by setting `CIRRUS_PR=1` via `if [ ${{ github.event_name }}" = "pull_request" ]; then` which I think should fix this?
Yes, this should fix it. This will correctly set the COMMIT_RANGE, just like it did previously. Thx.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242080695)
> Set this to run on PRs in [fd32aa8](https://github.com/bitcoin/bitcoin/commit/fd32aa85b29f90c44e4a08f247ee6b1840ff7b69) ci: port lint by setting `CIRRUS_PR=1` via `if [ ${{ github.event_name }}" = "pull_request" ]; then` which I think should fix this?
Yes, this should fix it. This will correctly set the COMMIT_RANGE, just like it did previously. Thx.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242089031)
Heh, yeah. Somewhat it seems plausible that the environment may not be available before the vm boots up. A ~4 second delay should be acceptable. And it would be trivial to revert to hardcoding thrice, if there is need to.
(Feel free to resolve)
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242089031)
Heh, yeah. Somewhat it seems plausible that the environment may not be available before the vm boots up. A ~4 second delay should be acceptable. And it would be trivial to revert to hardcoding thrice, if there is need to.
(Feel free to resolve)
💬 ajtowns commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3135568435)
> @delta1 i would suggest picking up #13990 and #13922. In this order, or make a good argument why it would be fine to lower the settings without first making the fee estimation be compatible.
I don't think fee estimation should be a blocker for the relay change. If anything, the reverse: while sub-1sat/vb txs don't relay well, it would be annoying if your fee estimates dropped resulting in your txs also not relaying well.
You could get the best of both worlds by fixing fee estimation firs
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3135568435)
> @delta1 i would suggest picking up #13990 and #13922. In this order, or make a good argument why it would be fine to lower the settings without first making the fee estimation be compatible.
I don't think fee estimation should be a blocker for the relay change. If anything, the reverse: while sub-1sat/vb txs don't relay well, it would be annoying if your fee estimates dropped resulting in your txs also not relaying well.
You could get the best of both worlds by fixing fee estimation firs
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242130550)
Correct, this is dropped in a81bf5c25a8
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242130550)
Correct, this is dropped in a81bf5c25a8
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242130904)
Also dropped in a81bf5c
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242130904)
Also dropped in a81bf5c
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242131511)
Thanks, updated the doc in c7c87775768
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242131511)
Thanks, updated the doc in c7c87775768
👋 fanquake's pull request is ready for review: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33076)
(https://github.com/bitcoin/bitcoin/pull/33076)
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242146876)
nit in a81bf5c25a8dc0b3a2f8458380921b5026726d64: Looks like a unrelated and unneeded change?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242146876)
nit in a81bf5c25a8dc0b3a2f8458380921b5026726d64: Looks like a unrelated and unneeded change?
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242148852)
Fixed in 8dd7476d496
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242148852)
Fixed in 8dd7476d496
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242156236)
Oh, this has sneaked into the wrong commit!
It is "needed" by b49dd9890b9 to print the container name for these jobs when we have a low ccache hitrate warning in the CI annotations (bottom of the page on the run summary). Otherwise they are printed as:
"low ccache hitrate in <blank>"
Will fix it up
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242156236)
Oh, this has sneaked into the wrong commit!
It is "needed" by b49dd9890b9 to print the container name for these jobs when we have a low ccache hitrate warning in the CI annotations (bottom of the page on the run summary). Otherwise they are printed as:
"low ccache hitrate in <blank>"
Will fix it up
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242164130)
Ah, but then the yaml seems like the wrong place. Would be better to put in in the sh:
```diff
diff --git a/ci/test/00_setup_env_mac_native_fuzz.sh b/ci/test/00_setup_env_mac_native_fuzz.sh
index cacf2423ac..c79b48c8ed 100755
--- a/ci/test/00_setup_env_mac_native_fuzz.sh
+++ b/ci/test/00_setup_env_mac_native_fuzz.sh
@@ -6,6 +6,7 @@
export LC_ALL=C.UTF-8
+bla...insert...here # macos does not use a container, but the env var is needed for logging
export CMAKE_GENERATOR="Ninja"
...
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242164130)
Ah, but then the yaml seems like the wrong place. Would be better to put in in the sh:
```diff
diff --git a/ci/test/00_setup_env_mac_native_fuzz.sh b/ci/test/00_setup_env_mac_native_fuzz.sh
index cacf2423ac..c79b48c8ed 100755
--- a/ci/test/00_setup_env_mac_native_fuzz.sh
+++ b/ci/test/00_setup_env_mac_native_fuzz.sh
@@ -6,6 +6,7 @@
export LC_ALL=C.UTF-8
+bla...insert...here # macos does not use a container, but the env var is needed for logging
export CMAKE_GENERATOR="Ninja"
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242168983)
But in this case we won't get:
1. 'macOS 14 native, arm64, no depends, sqlite only, gui'
2. 'macOS 14 native, arm64, fuzz'
But will fix to a single name for both jobs (not a massive issue perhaps, it's only an informational annotation?)
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242168983)
But in this case we won't get:
1. 'macOS 14 native, arm64, no depends, sqlite only, gui'
2. 'macOS 14 native, arm64, fuzz'
But will fix to a single name for both jobs (not a massive issue perhaps, it's only an informational annotation?)
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242175382)
Yes, container names must be unique over all configs and there are two macos configs (ci/test/00_setup_env_mac_native.sh) and (ci/test/00_setup_env_mac_native_fuzz.sh), so it should be possible. But just a nit. Anything is fine here.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242175382)
Yes, container names must be unique over all configs and there are two macos configs (ci/test/00_setup_env_mac_native.sh) and (ci/test/00_setup_env_mac_native_fuzz.sh), so it should be possible. But just a nit. Anything is fine here.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242184713)
from the llm:
used as with -> used with [“used as with” is redundant and makes the phrase unclear]
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242184713)
from the llm:
used as with -> used with [“used as with” is redundant and makes the phrase unclear]
💬 stringintech commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3135699830)
I think it would be nice to include how the newly added logic would also cover the assumeutxo case in the [commit](https://github.com/bitcoin/bitcoin/commit/e66556082279b21ead21e52e7b81996fa195c205) description; if my understanding is correct, adding something like:
_After snapshot loading, our tip becomes the snapshot block. Since peers are verified to have the snapshot in their chain, our tip is an ancestor of the peer's best block, hence the general advancement logic will move pindexLastCo
...
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3135699830)
I think it would be nice to include how the newly added logic would also cover the assumeutxo case in the [commit](https://github.com/bitcoin/bitcoin/commit/e66556082279b21ead21e52e7b81996fa195c205) description; if my understanding is correct, adding something like:
_After snapshot loading, our tip becomes the snapshot block. Since peers are verified to have the snapshot in their chain, our tip is an ancestor of the peer's best block, hence the general advancement logic will move pindexLastCo
...
💬 maflcko commented on issue "ci: failure in wallet_migration.py":
(https://github.com/bitcoin/bitcoin/issues/33096#issuecomment-3135706740)
https://github.com/bitcoin/bitcoin/actions/runs/16619385866/job/47020380651?pr=32989#step:12:17491
(https://github.com/bitcoin/bitcoin/issues/33096#issuecomment-3135706740)
https://github.com/bitcoin/bitcoin/actions/runs/16619385866/job/47020380651?pr=32989#step:12:17491
💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2242211462)
In 3854d6b56c50b26f7b61dfb37f5230726d561cf9 "tests: Check that the last hardened cache upgrade occurs"
At the stage where there are multiple such instances of hardcoding the hex representation of the database keys in the tests, I would introduce a `get_db_key_hex` utility. Mentioned in overall review comment.
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2242211462)
In 3854d6b56c50b26f7b61dfb37f5230726d561cf9 "tests: Check that the last hardened cache upgrade occurs"
At the stage where there are multiple such instances of hardcoding the hex representation of the database keys in the tests, I would introduce a `get_db_key_hex` utility. Mentioned in overall review comment.