🤔 josibake reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2330478429)
Concept ACK

Conceptually, this makes a lot more sense and makes the code much easier to understand, IMO. Left some comments while doing an initial pass, more questions than concrete suggestions. I did find it a bit hard to parse what was going with a lot of the code being move-only, but from across different files. Not sure if much can be done here, but it would make this easier to follow if you could separate as much of the move-only code changes into their own commit(s)? Specifically, I'm t
...

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776635876)
Assuming this is just a comment explaining what was already happening and documenting a behaviour change introduced in this PR? (note to self to test this).

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776630244)
Might be premature at this stage, but in theory we could be opening both the block database and txdb at this stage. I haven't looked at `dbwrapper_error` yet so not sure if this is possible, but perhaps we could bubble up the actual db name that failed to open instead of assuming it's always the block database?

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776602376)
I'm guessing you moved the values up from below (L109-L112) so that you could set the value once and use `cache_sizes.block_tree_db` to set `block_tree_db_cache_size` on L127? Otherwise it's not clear to me why those values were moved.

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776649353)
This code feels a bit smelly, so nice to see you are dropping it by moving the database initialisation into the `BlockManager`. Will need to look at the old code more closely to understand what "previous loop" this comment is referring to.

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776638088)
Nice! Makes a lot more sense to me to have block tree database options in the `BlockManagerOptions`

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776620730)
Note to self (and other reviewers): this move wasn't immediately obvious to me, until I noticed `CacheSizes` is set on L1521, and then the value `cache_sizes.block_tree_db` is used when creating the `BlockManager::Options`. As a nice side effect, it seems better to have the logs for cache configuration first, and then proceed with setting and creating the `BlockManager`.

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776651167)
Nevermind, I can see this a move only from `src/chainstate.h`

💬 maflcko commented on pull request "doc: fix `loadtxoutset` example":
(https://github.com/bitcoin/bitcoin/pull/30973#issuecomment-2376362674)
review ACK 286725168ae0309e427b1204a0724a4ba7cbe860

🚀 fanquake merged a pull request: "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job"
(https://github.com/bitcoin/bitcoin/pull/30961)

💬 maflcko commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376384339)
Are all random peers limited peers, which wouldn't serve blocks?

💬 fanquake commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376402542)
> Are all random peers limited peers,

I don't think so, given some peers would have to be serving (historic) blocks for the background chain to continue syncing? I've now restarted this node, and the behaviour hasn't persisted, both chains are currently syncing. If this gets to the point that the snapshot load completes, I'll close this.

💬 maflcko commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2376413368)
Should be trivial to rebase it, no?


```diff
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 366c648ae7..1f3bf2b018 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -92,8 +92,12 @@ typedef char* sockopt_arg_type;
// building with a binutils < 2.36 is subject to this ld bug.
#define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
#else
+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
+#define MAIN_FUNCTION HFND_FUZZING_ENTRY_FUNCTION_CXX(int
...

👍 theStack approved a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973#pullrequestreview-2330645298)
ACK 286725168ae0309e427b1204a0724a4ba7cbe860

📝 maflcko opened a pull request: "ci: Require git to be installed on workers"
(https://github.com/bitcoin/bitcoin/pull/30974)
The fallback `bash -c "$PACKAGE_MANAGER_INSTALL git"` is mostly dead code and trivial to add back, if it is ever needed again.

For now, just require git and remove `PACKAGE_MANAGER_INSTALL`. Also, add some other packages which are needed by podman to docs.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1776775938)
We could. I left it as for simplicity, given this case should not be too frequent, so having a higher fanout under these conditions shouldn't be too bad.

I'll consider adding it if it does not make the code much harder to read, or if there is a clear counterargument in favor of it.

👍 BrandonOdiwuor approved a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973#pullrequestreview-2330783910)
ACK 286725168ae0309e427b1204a0724a4ba7cbe860

🚀 fanquake merged a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973)

📝 Sjors opened a pull request: "guix: add multiprocess binaries"
(https://github.com/bitcoin/bitcoin/pull/30975)
This changes the Guix build process to use `MULTIPROCESS=1` for its `depends` build, and as a result add `bitcoin-node`, `bitcoin-gui` and `bitcoin-wallet` to the release binaries.

Combined with #30955 and https://github.com/Sjors/bitcoin/pull/52 this makes it feasible to implement a Stratum v2 Template Provider (or something similar) as an external tool that communicates with the node via IPC.

A complete implementation can be seen in https://github.com/Sjors/bitcoin/pull/48. That implemen
...

💬 Sjors commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1776802441)
8b2546c35de37ea15aa7e22865943747b3006b61: it turns out that doing `make MULTIPROCESS=0` in `depends` causes libmultiprocess to be built anyway. Not sure if that's a known behavior or a cmake regression. cc @hebasto