📝 achow101 opened a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249)
The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed.

Split from #28710

📝 achow101 opened a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250)
To prepare for the deletion of legacy wallet code, disable creating or loading new legacy wallets.

Tests for the legacy wallet specifically are deleted.

Depends on #31248

Split from https://github.com/bitcoin/bitcoin/pull/28710

📝 furszy opened a pull request: "test: report a more detailed failure during utf8 response decoding"
(https://github.com/bitcoin/bitcoin/pull/31251)
Useful for debugging issues such https://github.com/bitcoin/bitcoin/pull/31241#issuecomment-2462816933.

Prints the entire response content instead of printing only the position of the byte it can't be decoded.

The diff between the error messages can be seen by running the `wallet_migration.py` functional test with the following patch applied:
```
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
--- a/src/wallet/rpc/wallet.cpp (revision d65918c5da52c7d5035b4151dee9ffb2e
...

💬 darosior commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1833219801)
nit: above comment is now outdated (it wasn't very useful in the first place).

👍 TheCharlatan approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2407815626)
ACK ad67fd2e0bfa6f43f350066596b6cca146391362

Just nits, and all of them can be ignored. This was fun to review, the explanations in the `safegcd_implementation.md` are excellent. I also profited from the two review clubs and their notes that were done on the original MuHash introduction.

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1824375277)
Nit: Not important, but could `inline` these same as the other functions only used within this module?

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1832363289)
Just a question: Compared to the code in `modinv32_impl.h`, there are fewer bound checks done here. Is this on purpose?

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1832352138)
Nit: The constant here seems clearer in `modinv32_impl.h`. Could it get a name here too, or an explanation how it was computed?

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1831135311)
Nit: Both `3072 / SIGNED_LIMB_SIZE` and `3072 % SIGNED_LIMB_SIZE` are used a couple of times. Might it be more clear to give them their own descriptive constants? I was initially thinking `FINAL_LIMB_POSITION` and `FINAL_LIMB_MODULUS_BITS`, but not sure if that really feels clearer either.

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1832578046)
Just a question: Compared to `modinv32_normalize`, this step seems to skip the inner limbs. Is there a reason why there is a difference between the implementations here? IIUC this works out in the end because of the carry step.

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1833285990)
Note for other reviewers: The diff here is confusing, but from what I can tell, this does not actually change the `Multiply` function, but just gets rid of `Square()`, `muldbladd3`, and `square_n_mul`, which were used in the old Inverse implementation.

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1833318225)
This tripped me up at first, because something is added instead of subtracted compared to the implementation in `modinv32_impl.h`. But I think this is correct, because it subtracts the distance (I don't know what the correct terminology is for this) to the modulus instead of adding the modulus, which should work out to the same. Similarly, the operation in the for loop can also be moved to the final step, which I'm guessing is a further nice optimization.

📝 polespinasa opened a pull request: "rpc: print P2WSH redeemScript in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/31252)
This PR is motivated by https://github.com/bitcoin/bitcoin/issues/27391.
And inspired by a previous PR https://github.com/bitcoin/bitcoin/pull/8849 that proposed something similar.

This issue is partially solved as this PR only prints P2WSH (for the moment, I will try to work on P2SH).

This is an example with a real mainnet transaction (3-5 multisig) and using ``decoderawtransaction`` (also works for ``getrawtransaction 'raw' 1``).

```
{
"txid": "6346e552f62281314dfeace8f977e056f2
...

📝 polespinasa converted_to_draft a pull request: "rpc: print P2WSH witScript in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/31252)
This PR is motivated by https://github.com/bitcoin/bitcoin/issues/27391.
And inspired by a previous PR https://github.com/bitcoin/bitcoin/pull/8849 that proposed something similar.

This issue is partially solved as this PR only prints P2WSH (for the moment, I will try to work on P2SH).

This is an example with a real mainnet transaction (3-5 multisig) and using ``decoderawtransaction`` (also works for ``getrawtransaction 'raw' 1``).

```
{
"txid": "6346e552f62281314dfeace8f977e056f2
...

🤔 furszy reviewed a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2422157240)
@maflcko, have tried a good number of different mechanisms to catch the error but didn't have much luck in the CI. `ctest` seems to be swallowing the error there. It occurs on a different place, not at the initial root directory creation. It probably occurs in one of the blk**.dat or the ldb files, they are also created inside the `TestChain100Setup` constructor.

Still, we could at least move forward with something like
https://github.com/bitcoin/bitcoin/pull/31000/commits/fd5b9fc36879ead0ef
...

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2463233587)
Updated e742d66b1c77a90de502c45b452184b183b3778c -> b83d509d59dde352054cccf792d28bb87835e64f ([submitblock_prechecks_0](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_0) -> [submitblock_prechecks_1](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/submitblock_prechecks_0..submitblock_prechecks_1))


* Addressed @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/31175#pullrequestrevi
...

👍 TheCharlatan approved a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2422181431)
Re-ACK d120154d55451b6c90b709a4a093cbb6cabe48d8

Added improved rpc help and unit test case since last review.

💬 mzumsande commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1833399181)
submit `no_tx_block` instead of `bad_block` here, and if I remember correctly from yesterday, that will lead to a different error.

💬 darosior commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1833440825)
More smartpointy less segfaulty:
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 6053c853bb..b099fdfc3f 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1360,7 +1360,7 @@ public:
for (const auto& arg : m_pubkey_args) {
providers.push_back(arg->Clone());
}
- return std::make_unique<MiniscriptDescriptor>(std::move(providers), miniscript::MakeNodeRef<uint32_t>(m_node->Clone()));
+ retu
...

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1833445876)
Thanks, taken.