💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2163599033)
Nice defaulting to using the public key incase private key is not present. This is a more forward facing solution that aligns with the resolution of the existing issue #32078.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2163760571)
Nit: can add an `Assume` here for `m_path` being empty keeping in mind this error.

> "musig(): Cannot have ranged participant keys if musig() also has derivation"

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2164092082)
```diff
- KeyOriginInfo info;
- CKeyID keyid = aggregate_pubkey->GetID();
- std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
- out.origins.emplace(keyid, std::make_pair(*aggregate_pubkey, info));
- out.pubkeys.emplace(aggregate_pubkey->GetID(), *aggregate_pubkey);
+ auto temp_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, aggregate_pubkey.value(), /*xonly=*/false
...

💬 josibake commented on pull request "build: add root dir to CMAKE_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3000812206)
reACK https://github.com/bitcoin/bitcoin/commit/e27a94596f2a1f5e04722a16165717cc6e891d36

The only change is to use `CMAKE_PREFIX_PATH` over `CMAKE_SYSTEM_PREFIX_PATH`, which is more in-line with CMake's documentation as mentioned in https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2953470821

💬 polespinasa commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#discussion_r2164247671)
That's right. It is checked during stopping of the node. Had to introduce that in https://github.com/bitcoin/bitcoin/commit/bf194c920cf768d1339d41aef1441a78e2f5fcbe#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76

Didn't find a better way to handle it though (at that moment). Probably we could just remove that stderr check and restart the node manually using first stop and then start. That could be clearer?

👍 l0rinc approved a pull request: "headerssync: Preempt unrealistic unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2954075447)
I find the current layout of tiny, focused commits a lot easier to follow, thanks!
Left a few comments, after that I will test it again locally and I can ACK.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164197118)
this comment is obvious from the code now: `/*exp_locator_hash=*/Params().GenesisBlock().GetHash()` or `/*exp_locator_hash=*/genesis_hash` later in 65a96b507c11b3f34efa996919892fa1a2fcf49c

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164220997)
is this comment accurate?

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164235547)
```suggestion
// 5. Sets the redownload buffer size to be large enough that we
```

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164194785)
would't an `.at(0)` suffice?

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164239931)
nice, this is exactly how comments should be used to augment what the code cannot easily tell <3

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164212315)
I'm not sure what "this many headers *on top* have been received"

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164149745)
ah, so it's jut a reassurance that it doesn't need a fancy asic, it can mine a block in no time - makes sense

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164245384)
might make sense to extract genesis hash here as well like we did in `HappyPath`

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164207447)
> makes it more likely that the number changes between releases

That sounds like a counter-argument to me, but don't have strong feelings about it, if you disagree, I don't mind, please resolve it.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164253700)
Can we add 2 helpers in that case? I really dislike that the comments don't follow the code's structure :.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164260873)
> chunks of code to minimize diffs

We've rewritten the tests at this stage, diffs and the function prototypes don't help in my opinion.
But if you insist, I don't mind, please resolve the comment

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164267943)
Not sure I understand, especially in the new code which states something like `locator == genesis` - which is basically the same as the comment

💬 polespinasa commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-3000887231)
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d

Nothing to add to the comments already done.

🤔 janb84 reviewed a pull request: "build: add root dir to CMAKE_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2954293190)
reACK e27a94596f2a1f5e04722a16165717cc6e891d36

Changes sinds last ACK:
- CMAKE_SYSTEM_PREFIX_PATH -> CMAKE_PREFIX_PATH

Validated that change still solve problem ✅