💬 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

💬 ryanofsky commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3382685189)
Just a quick code review for 2b11c47f60b85fe72ab9d093c9f8e5f597182fbc. Sorry if I missed anything obvious, my comments below are mostly asking for things to be explained a little better. Overall the changes seem positive and I did not see any problems.

- Can PR description be updated to say what types of "violations" are being "fixed" and clarify what purpose of the changes are? Would be helpful if description was self contained and did not start out referencing a comment in another PR.

-
...

💬 fanquake commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382705961)
It's also been pointed out that this has also (effectively) broken the Guix build on the release branches. Prior to this change, any download of the qrencode tarball, was falling back to https://bitcoincore.org/depends-sources. However now that the tarball has been changed (and overwritten on the server), any build for a branch that doesn't have these changes, when it falls back to the server for the download, will fail with a checksum mismatch.

Meaning anyone that tries to do a from scratch
...

📝 achow101 opened a pull request: "Revert "depends: Update URL for `qrencode` package source tarball""
(https://github.com/bitcoin/bitcoin/pull/33577)
The new URL breaks CI on the current release branches, see https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380802351.

The old URL also no longer exists so the tarball is fetched from the depends sources cache that we host, but the original tarball has already been overwritten on there. We will need to manually reinstate the original tarball.

💬 achow101 commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3382716042)
#33577 for the revert

✅ itornaza closed a pull request: "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0"
(https://github.com/bitcoin/bitcoin/pull/30994)