🤔 marcofleon reviewed a pull request: "fuzz: compact block harness"
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3315685387)
Some possible discussion about ed813c48f826d083becf93c741b483774c850c86 for review club.

💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414451232)
As we discussed a bit offline, increasing the number of peers here to greater than 3 should be enough to hit some of the missing "eviction" logic in `MaybeSetPeerAsAnnouncingHeaderAndIDs`. Maybe increasing to 8 peers would be fine? Although not sure if that would test anything more (somewhere else) than if it were 4.

💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414480315)
I feel like I might be overlooking something here, but could we keep `m_dirty_blockindex` the same and then if `EnableFuzzDeterminism()` just sort `vBlocks` by hash before calling `WriteBatchSync()`?

I guess I'm asking is there a reason why `m_dirty_blockindex` needs to be sorted by hash from the beginning vs right before the write.

💬 ryanofsky commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3382453186)
> if we're forced to call `destroy` just to stop `waitNext`, that would lose context and forbid us from submitting this potentially valid solution

Makes sense. So it sounds like you would want a `BlockTemplate::interruptWait()` method?

💬 ryanofsky commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382493322)
> is there a way to "kill" server threads?

Indirectly, yes. The server threads will destroy themselves as soon as they are no longer referenced, either by the client or an ongoing IPC call that is using them. So as long as the client is not holding references to threads it is not using, they will be cleaned up.

There is some cost to creating threads, so it's good to reuse them between calls, and why I recommended having dedicated "waiter" and "worker" threads. But if you wanted to create more
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3382525762)
Thank you for the review @ismaelsadeeq!

Updated 2f6d5637d8cd1dc1a1444e31fdfb2dfb13152500 -> 4fce466d1a3e345836434f8fb375980f626981f1 ([kernelApi_70](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_70) -> [kernelApi_71](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_71), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_70..kernelApi_71))

* Addressed @ismaelsadeeq's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406532719), added some r
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414186905)
Just trying to cast to our internal type early on. Might be more confusing though, so just removed it.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414538874)
I could change these, but I think just copying here is preferable, since these are just wrapped pointer views. Ideally they don't depend on the instantiation of the view in the iterator, but can be be used out of the scope of it.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414217181)
I think it would be good to be consistent with these range checks here. This would also be asserted in the checks for the transaction when we call `Init` on the precomputed data.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414538801)
This was brought up before, and I'm still on the fence. Is there a scenario where the separate types would be annoying? Many existing libraries (and our code) don't use separate types, so there might be something to it, but I am not sure.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414301136)
I'm not convinced. How do you think the current naming could be confusing in a way such that a developer uses this incorrectly?

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414538998)
I think this would just get moved, but that seems very confusing. Taking your suggestion.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414236644)
This is actually the correct behaviour. The first "connected" is from 382. The second and third "connected" is from 383 (because we now have two loggers, that both receive the same message). Then we have three "disconnected" (the same just vice-versa) and finally a single "connected" and "disconnected".

🤔 TheCharlatan reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3315300032)
Comments inlined.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414367911)
Cool! The example looks correct to me. Do you think it would be useful to include the diagram in the commit message?

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414315486)
We previously had similar data structs and managed to eliminate them over time, so I'm hesitant on introducing a new one again. The current shape also seems easy to work with in the wrappers. I'll take the naming suggestion, but let me know if you think this is still too confusing.

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414553900)
Went with `std::string_view` instead, thanks.

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414554045)
Updated the description, thanks!

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#issuecomment-3382546293)
Thank you for the quick reviews!

> would it work if a string_view was thrown?

While that doesn't really help with providing more context for the failure type, I can sympathize with the dependency lowering goal for consteval/constexp functions.
https://github.com/l0rinc/bitcoin-core-nightly/pull/3 indicates this also fixes the issue, so I have used that, updated commit and PR description to reflect that.

> whether some contracts could be tightened by turning runtime error cases into pre
...

💬 hebasto commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2414572764)
If we go down this path, could the linter be assigned to this task as well?