💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2164070208)
Could also just Assume it's higher than `FeeFrac{1, 1}` like later

💬 Sjors commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3000589480)
`bitcoin-mine` refers to the Template Provider application I wrote in https://github.com/Sjors/bitcoin/pull/48. This is not intended to be include in Bitcoin Core.

It's based on the `bitcoin-mine` demo proposed by @ryanofsky in #30437. This may or may not be included in v30, but it doesn't really matter if this particular demo adds TCP support.

What does matter is that the `-ipcbind` argument of `bitcoin-node` gets TCP support.

🤔 rkrux reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2953212032)
I have finished reviewing the `descriptor: Add MuSigPubkeyProvider` commit - 4039be3f9b78c1ddb75c04f4a8002d36c4b05f79

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2163953475)
I see, `out` does need to be filled. In that case, there is an option to cache the `dummy` signing provider on line 629 when the aggregate pubkey & provider are being created (and cached) in case of non ranged participants. This could later be `FlatSigningProvider::Merge`'d with the `out` signing provider on line 672 so that `out` has information for both the participants and the aggregate.

The reason I find moving this snippet inside the `else` block here appealing is that we don't need to d
...

💬 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 :.