💬 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?)

💬 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.

💬 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]

💬 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
...

💬 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

💬 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.

💬 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.

🤔 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
...

👍 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.

💬 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?

💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2242260982)
I'll leave as is.

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2242286183)
Will get with the above if I re-touch

📝 waketraindev opened a pull request: "qt: clear command history when clearing the console"
(https://github.com/bitcoin/bitcoin/pull/33098)
Clears the command history in the qt console when the user clears the console output.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2242296189)
I've changed this to use to `/352h/1h`. I also edited `GenerateWalletDescriptor` to use `/352h/1h` for test chain.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2242297337)
Done

💬 Sjors commented on pull request "qt: clear command history when clearing the console":
(https://github.com/bitcoin/bitcoin/pull/33098#issuecomment-3135873608)
That seems like a good idea, but maybe open the PR on the GUI repo: https://github.com/bitcoin-core/gui

💬 ryanofsky commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3135880182)
> 29.0 is already tagged (archived), built, and released for a long time, so it isn't possible to change the release notes there now.

Thanks, this makes me think the nicest place to document compatibility information would probably be in RPC documentation itself. E.g. a note like "In version 29.0, the type and height parameters were added. Prior behavior was equivalent to passing type=latest". This way if an incompatible change is made and your script stops working, you coul just check RPC he
...

💬 Christewart commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3135891423)
Alternatively I believe 29.1 is publishing RC's, maybe it could be documented in that release with a note about how this change wasn't documented in the 29.0 release? 🤷‍♂️

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2242347967)
HI @achow101 can you please approve this PR

💬 willcl-ark commented on pull request "ci: Only pass documented env vars":
(https://github.com/bitcoin/bitcoin/pull/33002#discussion_r2242331540)
Could be worth moving the comment from `02_run_container.sh` to here as this is where this format is first contructed.

```
# Append $USER to /tmp/env to support multi-user systems and $CONTAINER_NAME
# to allow support starting multiple runs simultaneously by the same user.
```