💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382295705)
in commit fd52cd05d2bf11e059472e74fe6a771aa139b136: I noticed that the creation/extension of the `PSBTInput.m_musig2_{pubnonces,partial_sigs}` maps is slightly more complex here (involving a loop), in contrast to the counter-part in the other direction in `FillSignatureData`, where a single-line `.insert` is used. Is that intentional, or can they be unified in either way (i.e. also only use bare `.insert` here, or introduce loops in `FillSignatureData` as well if it's needed)?

Right now, `Fil
...

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3338565128)
`151edfaf78...f4fdf81d3a`: rebase and [remove `INTERNET_TRAFFIC_EXPECTED`](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206).

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2382314894)
Removed `INTERNET_TRAFFIC_EXPECTED`. Maybe it was an overkill in the first place.

💬 willcl-ark commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3338762732)
Ah interesting, that is quite similar in the end to my "workaround" (of building first, then a `generate_bench_tests` target).

I am Concept ACK using the python framework then, in the absence of any other benefits.

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382545727)
in commits 0829833bf418d3fae35ceac57cae6137c1e9067d, e54d27d0f81c7a9c8991516f1ed06e86d52d6c79 and b6b66426125e46deb331927a5942e157578e712c: nit, in spirit of #33399: could use `secp256k1_context_static` where sufficient. If I didn't miss anything, I think the only two secp256k1 calls introduced in this PR that need the `secp256k1_context_sign` context are:
* `secp256k1_musig_nonce_gen` and
* `secp256k1_keypair_create`

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382524112)
in commit e54d27d0f81c7a9c8991516f1ed06e86d52d6c79: consistency nit: same as in `CreateMuSig2AggregateSig`, could also check that the pubnonces and pubkey count match here, i.e.
```
// Check if enough pubnonces
if (pubnonces.size() != pubkeys.size()) return std::nullopt;

```

💬 brunoerg commented on issue "Ability to retrieve peer info by peer id":
(https://github.com/bitcoin/bitcoin/issues/33478#issuecomment-3338963332)
> for reference, this seems related to [#32741](https://github.com/bitcoin/bitcoin/pull/32741)

Should #32741 be up for graps?

💬 glozow commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3338979025)
> Tangibly, that would mean closing this PR and a new one to change the sentence
> Let me know @glozow.

It looks like you have conceptual support from folks to change the string, but strong preferences for one of two approaches. Your call whether you want to change your PR, leave it as is, open a new one, etc. Not everyone will agree.

💬 glozow commented on pull request "[28.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/33415#discussion_r2382612607)
fixed in #33476

💬 glozow commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2382621427)
fixed

💬 glozow commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#issuecomment-3339002781)
backported to 28.x in #33476

💬 glozow commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3339005617)
backported to 29.x in #33226
backported to 28.x in #33476

👋 glozow's pull request is ready for review: "[28.x] backports + 28.3rc1"
(https://github.com/bitcoin/bitcoin/pull/33476)

💬 glozow commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3339030472)
Ready for review, though I think the tidy job tripped somehow?

🤔 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-3272643818)
Code review 02994c2cbe2f051b868f49e65fac042feace2edf

Thanks for addressing the previous comments.

I'm about to wrap up my review by taking a look at the `libsecp` specific functions used in the `CreateMuSig2*` functions.

💬 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_r2382674470)
In 1181568816e32f378f3f9a50a0658d96541771de "sign: Create MuSig2 signatures for known MuSig2 aggregate keys" (but also in the 3 previous commits)

The original impetus of this suggestion was for the reader to avoid reading redundant code, but having the `sighash` calculated only once during MuSig2 signing flow for a `script_pubkey` seems better from performance POV as well, even though this redundant calculation is inside wallet and not in the more time sensitive operations of the node. This a
...

💬 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_r2382697115)
In 02994c2cbe2f051b868f49e65fac042feace2edf "test: Test MuSig2 in the wallet"

In continuation to the previous suggestion of handling the cases where the `musig` descriptor is not known to the wallet and instead only few keys are that could sign - https://github.com/bitcoin/bitcoin/pull/29675/#discussion_r2368738533 (thanks for adding the fix btw).

When I was debugging this case earlier, 2 tests failed because of incorrect number of nonces added - earlier `send` RPC was used instead of `wal
...

💬 HowHsu commented on pull request "index: handle case where pindex_prev equals chain tip in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2382744473)
> Not sure what that means, we write tests to tell us when the behavior of the application changed. You have changed the behavior and no test broke, so there's a lack of test coverage that we should fix to explain why this change is

this PR doesn't change the behavior of `NextSyncBlock()`, it just tweaks the code to make the function clearer.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3339174566)
Rebased. And addressed the changes done in [PR #33230](https://github.com/bitcoin/bitcoin/pull/33230).

💬 adam3us commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#discussion_r2382752997)
The warning "deprecated & discouraged" doesn't sound consistent with @bitschmidty's use case for miners wanting to set this parameter

> Some miners want to use Bitcoin Core for block template building but dont want to include large op_returns.

https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320079835

I didn't see any commentary disagreeing with it being reasonable for miners to want that, and in fact miners are the node type uniquely in a position to actually curb large dat
...