👍 stickies-v approved a pull request: "kernel: create monolithic kernel static library"
(https://github.com/bitcoin/bitcoin/pull/33077#pullrequestreview-3083677951)
tACK fdbade6f8ded63519b637c8fa6f39914a400ab5e

I rebased https://github.com/TheCharlatan/bitcoin/commits/kernelApi_Cpp_Internal_Headers/ onto fdbade6f8ded63519b637c8fa6f39914a400ab5e (resulting in https://github.com/stickies-v/bitcoin/tree/kernel-cpp-monolith), and I was able to statically link my `pbk_ext` py-bitcoinkernel nanobind extension to the monolithic libbitcoinkernel.

I reviewed the code changes and could not see any issues, but I am also a cmake novice. The monolithic approach ma
...

💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3150231458)
Would be nice to see a test run with additional logging added to `ChainstateManager::LoadExternalBlockFile`, and a log showing the size of the blk dat file.

💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2251205963)
When I first wrote the test, I had repeated issues on MacOS. the wallet’s scanning flag sometimes didn’t turn true quickly enough when I fired a full rescan. Too quickly, I convinced myself that the issue is with the tail length (I honestly never questioned it anymore until your question).

Upon inspection: Yes, I will fix it (which simplifies the code a lot).

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2251210391)
Definitely - should be fixed now:
```diff
$ git diff 784459ac79 104b40c8c9
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index c8d824bb6c..05ec73697c 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -1080,8 +1080,6 @@ bool BlockManager::ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos
return true;
}

-static const uint32_t HEADER_SIZE = ::GetSerializeSize(CBlockHeader{});
-
bool BlockManager::ReadTxFromBlock(CTransac
...

💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#issuecomment-3150239992)
Also, I will block the test on t.join(). (no timeout), so the
background rescan thread is guaranteed to exit before the test returns (which I think causes the CI
cleanup step from failing with “OSError: directory not empty”).

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2251213333)
Oops, sorry -> should be fixed now:
```diff
$ git diff b2a22ce33d 4441827ef4
diff --git a/doc/REST-interface.md b/doc/REST-interface.md
index ffb3c09af1..8f8431c29a 100644
--- a/doc/REST-interface.md
+++ b/doc/REST-interface.md
@@ -42,7 +42,7 @@ Given a block hash and transaction offset within it: returns a transaction in bi
Responds with 404 if the transaction doesn't exist.

By default, this endpoint will also deserialize the leading transactions, before reading and returning the
...

💬 hebasto commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251227924)
> transifex updated

I've pulled the most recent translations.

💬 hebasto commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251239067)
> > Are we going to pull in the updates, or leave this as-is?
>
> I've also created a preliminary review for all languages: https://github.com/maflcko/b-c-gui-translations-review/tree/main/reviews
>
> There is some obvious erroneous translations, like https://github.com/maflcko/b-c-gui-translations-review/blob/05100fec918fc44787d43004ea3b02749f5a16ee/reviews/te.md#L160-L166, but I am not familiar with those languages, so I can't evaluate if the LLM generated translation is correct.

Than
...

💬 janb84 commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150314952)
Concept NACK

What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi version build script than move it to an independent repo.

💬 hebasto commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3150347633)
> ... showing the size of the blk dat file.

You mean `blkdat.GetPos()`?

👍 Sjors approved a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3083871225)
ACK 45e06e05e7f8d638ecc91a1ba62e1b38a5133a2b

I find that the separation of 23431cde206bd6e18f274773f77bd914553bf2cc makes it easier to follow. The separation of https://github.com/bitcoin/bitcoin/commit/d2b84af58073630f87df32a7b785971fe32cb9be from https://github.com/bitcoin/bitcoin/commit/49f41141c674f83d0b66262a5b21aad623704264 is less useful, but fine by me.

💬 Sjors commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2251304473)
In 49f41141c674f83d0b66262a5b21aad623704264 _descriptor: ToPrivateString() pass if at least 1 priv key exists_: maybe for good measure:

```cpp
*has_priv_key = false;
```

In both places it seems better to check that `priv_key` exists, even if that's always the case in this fuzzer. IIUC in that case you can leave out `tmp{false}` and just pass `nullptr` below.

💬 Sjors commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2251274240)
In 64213213b3691c9d93de95f880f820a640cd3c35 _descriptors: add IsWatchOnly()_: nit: you don't need `output_type.has_value()`

💬 maflcko commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251381743)
> As for this PR, the last commit only serves only to demonstrate that the modified build system handles translation files properly. So I don't think that any translation-specific issue should block it.

Agree. Discussion can be closed.

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3150541294)
Applied a few more fixes: [`b2a22ce -> 4441827`](https://github.com/bitcoin/bitcoin/compare/b2a22ce33dc69697181547fa1e83bd0ed3321565..4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69)

💬 hodlinator commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2251460522)
Might be able to parse `CLIENT_NAME` from *build/src/bitcoin-build-config.h* and replace ` ` with `-`?

🤔 glozow reviewed a pull request: "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`"
(https://github.com/bitcoin/bitcoin/pull/33132#pullrequestreview-3084265420)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c

The fix works and makes sense to me. I suppose a concrete scenario is:
1. we have main and staging, neither optimally linearized
2. we create an observer for the main level (alternatively, it could be oversized)
3. we call DoWork() which returns true after optimally linearizing the staging level and skipping the main level
4. we delete the main level observer
5. we call CommitStaging. sims staging gets copied over.
6. even though staging was
...

👍 willcl-ark approved a pull request: "cmake: Install internal binaries to <prefix>/libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3084294348)
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f

> Sorry about that, the PR description was confusing ...

No that was my bad, I should have read the PR comments before reviewing (perhaps, although I don't like to).

I am in favour of moving binaries to libexec/, including `test_bitcoin`, as per this change.

I've tried to find other bitcoin packages which may break on this, but can't find many, and the few I have found are already outdated in other ways, so require a "manual update" alre
...

💬 willcl-ark commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150797545)
Concept NACK

Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?

🤔 furszy reviewed a pull request: "wallet: Avoid potentially writing incorrect best block locator"
(https://github.com/bitcoin/bitcoin/pull/29652#pullrequestreview-3084332144)
Since #30221 got merged, I think this one can be closed