💬 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?

💬 l0rinc commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3382585168)
Thanks for the replies, I have updated the description to clarify this is `llvm-mingw` specific, let me know if that's enough.

💬 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_r2414607669)
I also thought of this, it's why I've mentioned https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html in the descripton in the latest push - but it seems string literal throws are tolerated there.
I don't mind adding such a check, but it seemed to me `contrib/devtools/bitcoin-tidy` isn't actively developed.

💬 glozow commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3382642331)
> When I went back to test the commands I pasted in https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660 I found that it was still failing silently despite the functional test failing appropriately.

Nice! This PR is limiting the UTXOs that show up when doing automatic coin selection, and thus errors with "insufficient funds" when it can't find enough UTXOs for the tx. In your test/commands, you're pre-selecting those inputs using `send`.

Btw, when I was testing `send` co
...

💬 polespinasa commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3382650926)
ACK c70781d4241cb8b30fe33e7205868382031b4670

Code reviewed and tested.
These changes are just a refactor and not functional at all, but are needed in order to implement the FeeRate Forecast Manager in a "clean" and "ordered" manner #31664