👍 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).
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1867871199)
Via `subprocess`?
(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)
(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?
(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
(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
(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
💬 ryanofsky commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514813001)
First commit dropping verbosity|verbose seems ok, but I think the ability for different arguments names to exist in the same position is useful for #24963. #24963 is an old PR but Luke just updated it 2 weeks ago and I acked it in
https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-2443280039, describing the benefits I think it offers.
Do you think letting multiple arguments occupy the same position makes implementing #29912 harder? I would think an automatically generated schem
...
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514813001)
First commit dropping verbosity|verbose seems ok, but I think the ability for different arguments names to exist in the same position is useful for #24963. #24963 is an old PR but Luke just updated it 2 weeks ago and I acked it in
https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-2443280039, describing the benefits I think it offers.
Do you think letting multiple arguments occupy the same position makes implementing #29912 harder? I would think an automatically generated schem
...
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867894322)
```bash
grep "std::string" bitcoin/src/test/coins_tests.cpp | wc -l
3
```
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867894322)
```bash
grep "std::string" bitcoin/src/test/coins_tests.cpp | wc -l
3
```
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867899670)
I prefer `const` here, my IDE was also asking for them, I don't mind adding them here.
Being `noisy` when `a function doesn't contain any logic` doesn't sound like a blocker to me :)
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867899670)
I prefer `const` here, my IDE was also asking for them, I don't mind adding them here.
Being `noisy` when `a function doesn't contain any logic` doesn't sound like a blocker to me :)
💬 edilmedeiros commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514827383)
Concept ACK.
This incrementally reduces complexity and is an easy fix for those that may have something broken because of it.
What I don't like is to have RPC methods with different parameter names doing the same thing.
For instance:
```
getblock "blockhash" ( verbosity )
getblockheader "blockhash" ( verbose )
```
Maybe it would be good to clean up the RPC API before fixing #29912 (which seems to be done in the foreseeable future)?
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514827383)
Concept ACK.
This incrementally reduces complexity and is an easy fix for those that may have something broken because of it.
What I don't like is to have RPC methods with different parameter names doing the same thing.
For instance:
```
getblock "blockhash" ( verbosity )
getblockheader "blockhash" ( verbose )
```
Maybe it would be good to clean up the RPC API before fixing #29912 (which seems to be done in the foreseeable future)?
👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2476055822)
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196. Looks good! Thanks for the followups.
Since last review, coins.h commits were reorganized to minimize diffs, but only overall change was to drop redundant `inline` keywords. In `coins_tests.cpp` a lot of smaller changes were made like adding `const` to amount parameters (which is not great but ok), using more brace initialization, changing CheckAddCoin parameter order, and fixing up some comments.
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2476055822)
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196. Looks good! Thanks for the followups.
Since last review, coins.h commits were reorganized to minimize diffs, but only overall change was to drop redundant `inline` keywords. In `coins_tests.cpp` a lot of smaller changes were made like adding `const` to amount parameters (which is not great but ok), using more brace initialization, changing CheckAddCoin parameter order, and fixing up some comments.
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514900738)
> I use `uv`
[This](https://github.com/astral-sh/uv) one?
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514900738)
> I use `uv`
[This](https://github.com/astral-sh/uv) one?
💬 willcl-ark commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514904974)
Yes, and I highly recommend it!
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514904974)
Yes, and I highly recommend it!
💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867966922)
This issue is a non-nit for me as a general pattern as it will affect diffs & reviews going forward.
We've got a ratio of 5/212 or roughly 1:40 for this codebase. I'm surprised that the IDE is encouraging that.
What is you rationale for not applying it to `AddFlags`? Did the specific version of your IDE not tell you to do so? Does your IDE have a documented rationale?
If there is a clear rationale, ideally with a statistical sample of % bugs discovered from changing to `const` value typ
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867966922)
This issue is a non-nit for me as a general pattern as it will affect diffs & reviews going forward.
We've got a ratio of 5/212 or roughly 1:40 for this codebase. I'm surprised that the IDE is encouraging that.
What is you rationale for not applying it to `AddFlags`? Did the specific version of your IDE not tell you to do so? Does your IDE have a documented rationale?
If there is a clear rationale, ideally with a statistical sample of % bugs discovered from changing to `const` value typ
...
💬 mzumsande commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867973096)
the state could already be invalid before entering the loop (BIP30 check a few lines above).
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867973096)
the state could already be invalid before entering the loop (BIP30 check a few lines above).
👍 ryanofsky approved a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2476098921)
Code review ACK 95a0104f2e9869799db84add108ae8c57b56d360. Looks good! Thanks for all your work on this breaking the changes down and making them simple.
Since last review, more log newlines are removed, -norpccookiefile code is reorganized a little without changing behavior, combine_logs.py assert is improved, feature_config_args.py more consistently stops nodes after tests, and using -noconf now triggers an info log instead of a warning.
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2476098921)
Code review ACK 95a0104f2e9869799db84add108ae8c57b56d360. Looks good! Thanks for all your work on this breaking the changes down and making them simple.
Since last review, more log newlines are removed, -norpccookiefile code is reorganized a little without changing behavior, combine_logs.py assert is improved, feature_config_args.py more consistently stops nodes after tests, and using -noconf now triggers an info log instead of a warning.