🤔 rkrux reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3233829232)
More than three quarters of code review complete at 6400f7b82f4d2c22fb05f7122ce3a750938526f7
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3233829232)
More than three quarters of code review complete at 6400f7b82f4d2c22fb05f7122ce3a750938526f7
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355125544)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
The name of the function avoids the need for this comment.
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..a83cf82f19 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -316,7 +316,6 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
plain_pub = twe
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355125544)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
The name of the function avoids the need for this comment.
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..a83cf82f19 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -316,7 +316,6 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
plain_pub = twe
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355100462)
In 1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
Otherwise `part_info` acts as a dummy object.
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..f377a32750 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -277,7 +277,7 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
XOnlyPubKey xonly_part(part_pk);
auto it = sigda
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355100462)
In 1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
Otherwise `part_info` acts as a dummy object.
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..f377a32750 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -277,7 +277,7 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
XOnlyPubKey xonly_part(part_pk);
auto it = sigda
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355117208)
In d53a70d769505b28c76d0468ea9503f7e00c1e04 "sign: Add CreateMuSig2PartialSig"
```diff
diff --git a/src/key.cpp b/src/key.cpp
index 29427689bb..a952acb260 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -401,7 +401,7 @@ std::optional<uint256> CKey::CreateMuSig2PartialSig(const uint256& sighash, cons
const auto& pn_it = pubnonces.find(part_pk);
if (pn_it == pubnonces.end()) return std::nullopt;
const std::vector<uint8_t> pubnonce = pn_it->second;
- if (
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355117208)
In d53a70d769505b28c76d0468ea9503f7e00c1e04 "sign: Add CreateMuSig2PartialSig"
```diff
diff --git a/src/key.cpp b/src/key.cpp
index 29427689bb..a952acb260 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -401,7 +401,7 @@ std::optional<uint256> CKey::CreateMuSig2PartialSig(const uint256& sighash, cons
const auto& pn_it = pubnonces.find(part_pk);
if (pn_it == pubnonces.end()) return std::nullopt;
const std::vector<uint8_t> pubnonce = pn_it->second;
- if (
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355118477)
In 09105c55e71d0614508c9111ed448ea1a370b38b "sign: Add CreateMuSig2AggregateSig"
```diff
diff --git a/src/musig.cpp b/src/musig.cpp
index c8dfe70ef9..686ec5e869 100644
--- a/src/musig.cpp
+++ b/src/musig.cpp
@@ -139,7 +139,7 @@ std::optional<std::vector<uint8_t>> CreateMuSig2AggregateSig(const std::vector<C
const auto& pn_it = pubnonces.find(part_pk);
if (pn_it == pubnonces.end()) return std::nullopt;
const std::vector<uint8_t> pubnonce = pn_it->second;
-
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355118477)
In 09105c55e71d0614508c9111ed448ea1a370b38b "sign: Add CreateMuSig2AggregateSig"
```diff
diff --git a/src/musig.cpp b/src/musig.cpp
index c8dfe70ef9..686ec5e869 100644
--- a/src/musig.cpp
+++ b/src/musig.cpp
@@ -139,7 +139,7 @@ std::optional<std::vector<uint8_t>> CreateMuSig2AggregateSig(const std::vector<C
const auto& pn_it = pubnonces.find(part_pk);
if (pn_it == pubnonces.end()) return std::nullopt;
const std::vector<uint8_t> pubnonce = pn_it->second;
-
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355537665)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
Suggesting because I ended up spending some time to understand the need for this check.
```diff
@@ -285,6 +286,7 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
std::vector<std::pair<uint256, bool>> tweaks;
CPubKey plain_pub = agg_pub;
+ // Check if we can derive the script p
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355537665)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
Suggesting because I ended up spending some time to understand the need for this check.
```diff
@@ -285,6 +286,7 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
std::vector<std::pair<uint256, bool>> tweaks;
CPubKey plain_pub = agg_pub;
+ // Check if we can derive the script p
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355503340)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
Usually avoid suggesting this kind of change but in this case it seems quite helpful for readability.
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..de17219bd5 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -286,26 +286,26 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, Sig
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355503340)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
Usually avoid suggesting this kind of change but in this case it seems quite helpful for readability.
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..de17219bd5 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -286,26 +286,26 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, Sig
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355561394)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
There are 2 instances of returning false from this function (other on line 301) in which case it is expected that the signing process would throw an error at a later stage. Do you think adding an error log here would be helpful for debugging later?
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355561394)
In https://github.com/bitcoin/bitcoin/commit/1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
There are 2 instances of returning false from this function (other on line 301) in which case it is expected that the signing process would throw an error at a later stage. Do you think adding an error log here would be helpful for debugging later?
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355476052)
In 1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
<details open>
<summary>Can reorder to return or continue early on in the process.</summary>
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..eea6cd0a9f 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -270,19 +270,6 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
for (const auto& [agg_pub,
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355476052)
In 1614a2da5ea32b3efeecf6c756036539c567b656 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
<details open>
<summary>Can reorder to return or continue early on in the process.</summary>
```diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index b60d6dab55..eea6cd0a9f 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -270,19 +270,6 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
for (const auto& [agg_pub,
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355572780)
Thanks, this reason can be mentioned explicitly in the function doc above.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355572780)
Thanks, this reason can be mentioned explicitly in the function doc above.
👋 fanquake's pull request is ready for review: "[28.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/33415)
(https://github.com/bitcoin/bitcoin/pull/33415)
👋 fanquake's pull request is ready for review: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33344)
(https://github.com/bitcoin/bitcoin/pull/33344)
👋 fanquake's pull request is ready for review: "[30.0] rc2 backports"
(https://github.com/bitcoin/bitcoin/pull/33356)
(https://github.com/bitcoin/bitcoin/pull/33356)
✅ glozow closed an issue: "Support Multiple OP_RETURNs or Raise OP_RETURN Limit"
(https://github.com/bitcoin/bitcoin/issues/33413)
(https://github.com/bitcoin/bitcoin/issues/33413)
💬 glozow commented on issue "Support Multiple OP_RETURNs or Raise OP_RETURN Limit":
(https://github.com/bitcoin/bitcoin/issues/33413#issuecomment-3303146999)
This was already done in #32406. Easy to miss - fwiw if you're looking to stay up to date with PR merges, I'd recommend the [Optech newsletter](bitcoinops.org).
(https://github.com/bitcoin/bitcoin/issues/33413#issuecomment-3303146999)
This was already done in #32406. Easy to miss - fwiw if you're looking to stay up to date with PR merges, I'd recommend the [Optech newsletter](bitcoinops.org).
💬 fanquake commented on pull request "doc: Remove wrong and redundant doxygen tag":
(https://github.com/bitcoin/bitcoin/pull/33236#issuecomment-3303236672)
Backported to 28.x in #33415.
(https://github.com/bitcoin/bitcoin/pull/33236#issuecomment-3303236672)
Backported to 28.x in #33415.
💬 fanquake commented on pull request "Fix benchmark CSV output":
(https://github.com/bitcoin/bitcoin/pull/33340#issuecomment-3303239834)
Backported to 28.x in #33415 & 29.x in #33344.
(https://github.com/bitcoin/bitcoin/pull/33340#issuecomment-3303239834)
Backported to 28.x in #33415 & 29.x in #33344.
💬 willcl-ark commented on pull request "tor: enable PoW defenses for automatically created hidden services":
(https://github.com/bitcoin/bitcoin/pull/33414#issuecomment-3303277473)
> Should we then also add PoW to the connections that we make to other nodes running behind hidden services?
Reading the linked FAQ, the feature still supports "older clients" (which don't have PoW defence capability), but they may take a lower priority when a service considers itself under DoS. So no PoW is _required_ on the client side.
When the client-side tor is new-enough, my understanding is that the puzzle-solving is automatically handled by Tor, and doesn't need client-side changes
...
(https://github.com/bitcoin/bitcoin/pull/33414#issuecomment-3303277473)
> Should we then also add PoW to the connections that we make to other nodes running behind hidden services?
Reading the linked FAQ, the feature still supports "older clients" (which don't have PoW defence capability), but they may take a lower priority when a service considers itself under DoS. So no PoW is _required_ on the client side.
When the client-side tor is new-enough, my understanding is that the puzzle-solving is automatically handled by Tor, and doesn't need client-side changes
...
💬 willcl-ark commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#issuecomment-3303296826)
Concept ACK to re-introducing this job.
Wondering whether it's worth adding an extra runner if we add a new `lg`-sized job? On the whole CI queue sizes/times have been reasonable I think since the migration, and we did reduce a few job sizes recently. But still might be worth considering.
The 16th seemed unusually active, most days top out around 20 min max queue time AFAIK, but for reference here's the previous 7 days:
<img width="3384" height="1620" alt="image" src="https://github.com
...
(https://github.com/bitcoin/bitcoin/pull/33411#issuecomment-3303296826)
Concept ACK to re-introducing this job.
Wondering whether it's worth adding an extra runner if we add a new `lg`-sized job? On the whole CI queue sizes/times have been reasonable I think since the migration, and we did reduce a few job sizes recently. But still might be worth considering.
The 16th seemed unusually active, most days top out around 20 min max queue time AFAIK, but for reference here's the previous 7 days:
<img width="3384" height="1620" alt="image" src="https://github.com
...
📝 fanquake opened a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33416)
Backports:
* #30198 (partial)
(https://github.com/bitcoin/bitcoin/pull/33416)
Backports:
* #30198 (partial)