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

💬 willcl-ark commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2514748930)
https://cirrus-ci.com/task/6357677415071744?logs=ci#L2640

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867848425)
just to check: this is just a belt and suspenders? It seems each call site to `state.Invalid` in the loop is followed by a `break`.

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

Yes, they are. The limit is not based on the number of keys, it is on the size of the redeem script. Which can go up to 520 bytes max (which limits the number of keys inside a p2sh to 15).

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1867871199)
Via `subprocess`?

👋 hebasto's pull request is ready for review: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410)

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867881135)
What's the advantage of the comment stating exactly what the code already states?
We're already testing `\0` and non-trailing `=`, what else do these ones add?

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867882293)
I'll consider it if I need to touch again

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867886473)
You're suggesting an empty line here only, right?
I'll consider it if I need to edit again