💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1916382498)
thx, added a commit

💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1916382700)
leaving as-is for now

💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1916383028)
thx, pushed doc fixup

👍 stickies-v approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2552286452)
re-ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e

No changes except for consistent re-use of `TranslateFn`, minor docstring change and un-inline-ing test translate function.

🤔 Sjors reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2552280742)
> > ... dropping `AddSocketPermissionFlags`. Can you move that to a separate (earlier?) commit?
>
> Done.

Thanks. Don't forget to drop "Also, separate the listening socket from the permissions" from the commit description of e5d36eea015efc31aa38d540af4cf39c9e2e46b0.

Along similar lines, though less important, you could also drop `ListenSocket` and first introduce `m_listen` as a member of `CConnman`, before moving it to `Sockman`.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916382790)
fd81820214e695ba228a954506397c3d781fe3fe: do want to add an `Assume` here, given that `Bind` always adds an entry?

✅ stickies-v closed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928)

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2592463052)
Closing this PR. Thanks to related PRs like #31174, #31072 and #31061, tfm::format error throwing should be significantly reduced, so there's increasingly little point in making this behaviour change.

🤔 rkrux reviewed a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2552298207)
Concept ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424

nit in pr title:

```diff
- "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
+ "descriptors: Try pubkeys of both parities when retrieving the private keys for an xonly pubkey in a descriptor"
```

💬 rkrux commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1916393178)
Why does `GetPrivKey` of `ConstPubkeyProvider` not end up using the `pos` argument? Is it because it's supposed to be a "constant" provider?

📝 fanquake reopened a pull request: "depends: Qt 5.15.16"
(https://github.com/bitcoin/bitcoin/pull/30774)
Contains a handful of miscellaneous bug fixes.
We can drop a few of our patches.

See https://github.com/qt/qtbase/compare/v5.15.14-lts-lgpl...v5.15.16-lts-lgpl.

💬 hebasto commented on pull request "depends: Always set `CMAKE_BUILD_TYPE=None` globally":
(https://github.com/bitcoin/bitcoin/pull/31661#issuecomment-2592506573)
My Guix build:
```
aarch64
86fcefb01cf8fb3d3bfc0639b270f4df96567066620cdaf9d1a3fc70664c1755 guix-build-dd7868a173fb/output/aarch64-linux-gnu/SHA256SUMS.part
ebca518cd64e7f45d96a758ed8d06c3a2689aa1783f75366e3a5f4a32fd79bca guix-build-dd7868a173fb/output/aarch64-linux-gnu/bitcoin-dd7868a173fb-aarch64-linux-gnu-debug.tar.gz
b93ef69b68418482fab8977b5a590fbacd4aa37ae5d8f1da1cc5b071df9b6c17 guix-build-dd7868a173fb/output/aarch64-linux-gnu/bitcoin-dd7868a173fb-aarch64-linux-gnu.tar.gz
62f77252
...

🤔 rkrux reviewed a pull request: "test: add coverage for immediate orphanage eviction case"
(https://github.com/bitcoin/bitcoin/pull/31628#pullrequestreview-2552403718)
Seems like a very specific condition is being tested. Would it be possible to add this inside the `test_max_orphan_amount` test instead of creating a new one?

Here right after the orphanage length is asserted and right before the orphanage is cleared?
https://github.com/bitcoin/bitcoin/pull/31628/files#diff-f7a7f89dc6ff73d829dbf856a922767096b11fe34ffcae1a482ca1c544611981R649

🤔 Sjors reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2552366143)
Reviewed up to 1b05e1d4ba55a42ba74026b68fa4e616b973e06d.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916457190)
1b05e1d4ba55a42ba74026b68fa4e616b973e06d: maybe `ConnectionId`?

And then in net.h use `typedef ConnectionId NodeId;`

https://github.com/Sjors/bitcoin/commit/f33049b45b7013022ed4c75c0bc52d878fe15fd5

Most of the churn would be in `sockman.{h,cpp}`, so that seems acceptable.

That said, I don't think it's very important.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916431512)
0241b04cf406d482abfac3fddfad9a9c28725f32: previously this would not get called if `addr.SetSockAddr` failed in `AcceptConnection`. I suspect it doesn't matter though.

💬 brunoerg commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1916575817)
nit: Maybe this `BOOST_CHECK` is not needed anymore since you're checking the result itself.

👍 brunoerg approved a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656#pullrequestreview-2552630267)
code review ACK dd4f894c58a8f220da224bd220cdad8cd810099a

This is just a simple addition to the test case "Test that two less valuable UTXOs with a combined lower weight are preferred over a more valuable heavier UTXO" and does not increase any test coverage.

For this case, we currently only check whether coin grinder was able to return a result. Now, it checks if the result is the expected one.

🤔 Missjones2829 reviewed a pull request: "[28.x] 28.1 backports and final changes"
(https://github.com/bitcoin/bitcoin/pull/31594#pullrequestreview-2552722992)
Merge

💬 laanwj commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1916637752)
i would really prefer not to bring back use of `strtol` in C++ code; it has some known issues with locale-dependence (especially on Linux). what about:

```c++
#include <charconv>
...
std::vector<unsigned char> hex_string_to_char_vec(const std::string& hex)
{
std::vector<unsigned char> bytes;

for (size_t i{0}; i < hex.length(); i += 2) {
unsigned int val{0};
auto [p, ec] = std::from_chars(hex.data() + i, hex.data() + i + 2, val, 16);
if (ec == std::er
...