💬 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?
(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?
(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.
(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.
(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.
(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.
(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
...
(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
...
(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
...
(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
...
(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.
(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.
(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
...
(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.
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1833445876)
Thanks, taken.
💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2463344981)
> I wrote a regression unit test for this: [darosior@d23ffe0](https://github.com/darosior/bitcoin/commit/d23ffe08ef42475dcd40a13857cd775ac98e0270).
Added the test commit as well.
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2463344981)
> I wrote a regression unit test for this: [darosior@d23ffe0](https://github.com/darosior/bitcoin/commit/d23ffe08ef42475dcd40a13857cd775ac98e0270).
Added the test commit as well.
💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1833458457)
Sorry, fixed. Makes me even less certain on keeping the coinbase error around. I'll sleep on it.
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1833458457)
Sorry, fixed. Makes me even less certain on keeping the coinbase error around. I'll sleep on it.
📝 threewebcode opened a pull request: "fix: recitfy typos"
(https://github.com/bitcoin/bitcoin/pull/31253)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31253)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 NicolasDorier commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2463719253)
Given BIP373 doesn't have test vectors, it would be very useful that either this PR or the BIP include some hard coded PSBT examples to ensure every implementations are on the same page.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2463719253)
Given BIP373 doesn't have test vectors, it would be very useful that either this PR or the BIP include some hard coded PSBT examples to ensure every implementations are on the same page.
💬 vasild commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1833763202)
I am fine either way and slightly prefer the softer approach of capping it to 100 instead of asserting that it is <=100. There was a discussion some time ago about crashing the program way too easily and reserving the assert/crash only for cases where the continuation of the program really does not make sense. I kind of agree with that.
Yes, `Assume()` is some middle ground between `assert()` and the softer approach. `Assume()` + cap in release builds looks reasonable to me too. Plus document
...
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1833763202)
I am fine either way and slightly prefer the softer approach of capping it to 100 instead of asserting that it is <=100. There was a discussion some time ago about crashing the program way too easily and reserving the assert/crash only for cases where the continuation of the program really does not make sense. I kind of agree with that.
Yes, `Assume()` is some middle ground between `assert()` and the softer approach. `Assume()` + cap in release builds looks reasonable to me too. Plus document
...
👍 rkrux approved a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2422844752)
tACK 47f50c7af5572520fd986b313a63a44a76d3c859
Successful make and functional tests. Certainly an improvement for all the executables we have. I generated the man pages for all 6 of them and verified the contents using `man ./doc/man/<executable-name>.1`. Also, compiled and ran the GUI & checked the help section over there. PFB the screenshots from my setup.
<img width="789" alt="Screenshot 2024-11-08 at 12 22 09 PM" src="https://github.com/user-attachments/assets/0db4fa0c-5be0-4f70-b7ca-68
...
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2422844752)
tACK 47f50c7af5572520fd986b313a63a44a76d3c859
Successful make and functional tests. Certainly an improvement for all the executables we have. I generated the man pages for all 6 of them and verified the contents using `man ./doc/man/<executable-name>.1`. Also, compiled and ran the GUI & checked the help section over there. PFB the screenshots from my setup.
<img width="789" alt="Screenshot 2024-11-08 at 12 22 09 PM" src="https://github.com/user-attachments/assets/0db4fa0c-5be0-4f70-b7ca-68
...