✅ maflcko closed an issue: "ci: Replace wine tests with real tests on Windows?"
(https://github.com/bitcoin/bitcoin/issues/31071)

💬 maflcko commented on issue "ci: Replace wine tests with real tests on Windows?":
(https://github.com/bitcoin/bitcoin/issues/31071#issuecomment-2514611461)
Let's continue discussion in #31176

💬 brunoerg commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1867758148)
In bdd42ed8de8a92ae3e69e7b2dfceae7330f32676: I think it can be simplified. See:
```diff
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 928ab4acb4..06bc4e1fbb 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1070,12 +1070,7 @@ class WalletMigrationTest(BitcoinTestFramework):
wpkh_desc = descsum_create(f'wpkh({pubkey_hex})')
expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_d
...

💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514616942)
> `cmake` >= 3.17 does support a [`Python_FIND_VIRTUALENV`](https://cmake.org/cmake/help/latest/module/FindPython.html#hints) variable, but this doesn't seem to support all venv providers (I use `uv`).

A quick note. This seems incompatible with https://github.com/bitcoin/bitcoin/pull/31233.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867761722)
It would be a mess. I added an assert to make it clear that a test which inserts duplicate ids is broken.

💬 willcl-ark commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514623089)
Thanks hebasto, I had not seen this PR. Will compare/review.

It has come to my attention that having these checks complete in a dev environment is not that useful anyway, so perhaps "making things easier for me to run locally" is not a great motivation for this change in any case...

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2514625472)
I don't think the replacement has received any review/acks, so I think it still makes sense to merge this one in the meantime. I don't think anyone benefits from the flood of false positives here.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867768931)
Done

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2514637932)
`2b0103705f...645f625e29`: rebase and address suggestions

👍 brunoerg approved a pull request: "wallet: fix crash during watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/31374#pullrequestreview-2475779752)
ACK bdd42ed8de8a92ae3e69e7b2dfceae7330f32676

I verified that `wallet_migration` test fails on master. Also, I ran the `spkm_migration` fuzz target (from #29694) for over 24 hours and did not got any crash.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867807271)
`<string>` is still needlessly added in c0b4b2c1eef95c0b626573a9a2e217f4a541a880.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867774375)
Using `const` for value type parameters (`CAmount is a typedef of `int64_t`), when a function doesn't contain any logic is noisy to me and not used in the codebase at large.
```suggestion
constexpr CoinEntry(CAmount v, State s) : value{v}, state{s} {}
```

It is useful in cases like this where the function is actually doing something:
https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/src/uint256.cpp#L20-L44

Same goes for `ToState`, `SetCoinsValue`, `Si
...

💬 brunoerg commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#discussion_r1867820477)
nit: Maybe we should have a constant for this number (3)? Then we can use here and in `IsStandard`.

📝 maflcko opened a pull request: " rpc: Remove deprecated RPC arg names "
(https://github.com/bitcoin/bitcoin/pull/31412)
For compatibility, a single RPC argument may have multiple names. This is largely unused, because legacy code relying on it isn't generally using names arguments anyway. Also, it is confusing to see in existing code (https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2477250414) and makes writing new RPC code harder.

Fix all issues by removing them.

In the future, if such a feature is needed, it would be clearer to implement via `-rpcdeprecated=${rpc_name}_${field_old_name}` to t
...

💬 hebasto commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2514717948)
> ... so I think it still makes sense to merge this one in the meantime.

I did not mean to delay the merge of this PR.

💬 furszy commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1867838543)
> I don't see why we need to check if pubkey is into the descriptors, we expect all of them to have it.

Thanks. I just blindly re-used the code I applied for the first check.
Suggestion applied, and also inlined the first check too.

💬 maflcko commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1867840289)
Would it be possible to achieve the same with `icacls` on Windows?

💬 furszy commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#discussion_r1867841506)
yeah, thats also something that popped-up in (https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1866127089). Will bring that commit here too.

👍 instagibbs approved a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2475882265)
ACK 492e1f09943fcb6145c21d470299305a19e17d8b

Not an expert in the changes in 1ac1c33f3f120bbe0bde4fa948299bc07cac47ee to the `CCheckQueue::Loop` but things seem to map correctly, other changes read clean to me.

💬 brunoerg commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#discussion_r1867843990)
In d64193142792e68636bf61ef4b083dfb8c1d20d7: A p2sh multisig with 20 keys is invalid? I thought 20 keys are the limit, but still valid.