💬 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.
(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.
(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...
(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.
(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
(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
(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.
(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.
(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
...
(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`.
(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
...
(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.
(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.
(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?
(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.
(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.
(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.
(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
(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`.
(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).
(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).