💬 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.
💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2242221568)
In 3854d6b56c50b26f7b61dfb37f5230726d561cf9 "tests: Check that the last hardened cache upgrade occurs"
I think I have found a way of getting rid of this kind of duplicated try-import-except-setfileconstant instances in multiple files by introducing a `inspect_database` function in the test framework. Mentioned in overall review comment.
I think it would be quite nice to have this inspect function but I don't feel like blocking the PR only for this.
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2242221568)
In 3854d6b56c50b26f7b61dfb37f5230726d561cf9 "tests: Check that the last hardened cache upgrade occurs"
I think I have found a way of getting rid of this kind of duplicated try-import-except-setfileconstant instances in multiple files by introducing a `inspect_database` function in the test framework. Mentioned in overall review comment.
I think it would be quite nice to have this inspect function but I don't feel like blocking the PR only for this.
🤔 rkrux reviewed a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3070824199)
tACK aa08126836f9acdf17790837aaa099500f2a5863
This builds on a similar change done in #32597. Adding the tests before the flag change on the migrated wallet ensures this flag was added before as well but at a slightly later state in the migration flow.
Combined diff of the inline-comments here that passes on my system.
<details>
<summary>Diff</summary>
```diff
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 639
...
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3070824199)
tACK aa08126836f9acdf17790837aaa099500f2a5863
This builds on a similar change done in #32597. Adding the tests before the flag change on the migrated wallet ensures this flag was added before as well but at a slightly later state in the migration flow.
Combined diff of the inline-comments here that passes on my system.
<details>
<summary>Diff</summary>
```diff
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 639
...
👍 willcl-ark approved a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33076#pullrequestreview-3070859539)
ACK 4d145f9f20a333cbe22579db525292e956af66c9
Checked that backports are clean, and attributed correctly.
(https://github.com/bitcoin/bitcoin/pull/33076#pullrequestreview-3070859539)
ACK 4d145f9f20a333cbe22579db525292e956af66c9
Checked that backports are clean, and attributed correctly.
💬 maflcko commented on pull request "ci: Only pass documented env vars":
(https://github.com/bitcoin/bitcoin/pull/33002#issuecomment-3135779398)
According to https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3119155140, the fix is confirmed. Maybe @willcl-ark or @m3dwards may be qualified to (n)ack this?
(https://github.com/bitcoin/bitcoin/pull/33002#issuecomment-3135779398)
According to https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3119155140, the fix is confirmed. Maybe @willcl-ark or @m3dwards may be qualified to (n)ack this?