💬 maflcko commented on pull request "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job":
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2376077438)
review ACK c1832584bfd1b352095bc41a13ff17564e456d43

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2376088471)
> ⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
>
> * Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
> * Is it no longer relevant? ➡️ Please close.
> * Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Will close if I don't get to it by this weekend, sorry for my del
...

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2376088936)
@ryanofsky looks like I copy-pasted the wrong hash. On my machine I have 1a332817665f77f55090fa166930fec0e5500727 (before fetching today).

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2376091479)
I'm not sure, I'll do another post-merge look.

💬 vasild commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1776500085)
Without a socket (second argument to `CNode` constructor is `nullptr`) this will not test much of the handshake:

https://github.com/bitcoin/bitcoin/blob/65f6e7078b17e6e73d74dfeb00159878099fee1e/src/net.cpp#L1600-L1605

Any special reason to go socket-less? I am working on a `CConnman` refactor that is changing this anyway, should I assign a fuzzed socket to the node?

💬 maflcko commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1776526638)
Send and receive doesn't go through sockets in this test. `Handshake` is just a method to initialize some fields with dummy or mocked data.

💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1776538036)
> I think the reason this code is written to use a while loop is because it wants to wake up every 1000ms to check m_interrupt in case `m_interrupt` was signaled without `m_tip_block_cv` being signalled

IIRC that's indeed why.

💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1776524682)
86bddb439bff846630b1efdea777119439ba56b2: did you mean to drop this comment? (the ` if (node.notifications)` check is still there)

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1776548257)
I didn't author the commit, but I don't think it matters. In any case the comment was wrong anyway, because it seems odd to say "between 4a and 7", which implies that it exists before 4a, which would be wrong.

💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2376175429)
Commit 86bddb439bff846630b1efdea777119439ba56b2 is a bit over my head, but seems like a good idea.

Lightly reviewed fa7c2c9f18f5468baf81c04177b75e2b684a327a.

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1776550909)
> The Sv2 TemplateProvider unit tests rely on `SetMockTime`

mocktime only affects the `NodeClock`, neither the system clock, nor the steady clock (obviously), so I don't think this will affect your unit tests and you don't have to change anything.

💬 Sjors commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2376225530)
Rebased after #30510.

💬 fanquake commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376227995)
> Are you syncing from random peers or do you have some local setup?

This was syncing from random peers.

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2376260403)
Now that #30409 landed this is a single commit, and ready for review.

👋 Sjors's pull request is ready for review: "rpc: add optional blockhash to waitfornewblock"
(https://github.com/bitcoin/bitcoin/pull/30635)

👍 itornaza approved a pull request: "Mining interface: getCoinbaseMerklePath() and submitSolution()"
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2330484303)
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5

In order to verify that `MerkleComputation()`, `ComputeMerkleBranch()`, `BlockMerkleBranch()` were copied verbatim from their resting place at `src/test/merkle_tests.cpp` to their new home `src/consensus/merkle.cpp`, I copied the two versions of the functions in separate files and compared them with `diff`. The only difference I noticed is that now the function `BlockMerkleBranch()` is not declared as `static` which is correct, because
...

💬 Sjors commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2376282083)
@itornaza you may also find `git diff --color-moved=dimmed-zebra` useful.

📝 fanquake opened a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973)
The current order is incorrect:
```bash
./build/src/bitcoin-cli loadtxoutset -rpcclienttimeout=0 utxo-840000.dat
error code: -1
error message:
loadtxoutset "path"
```

💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1776654445)
> clang-format has a nice "BinPack" and "Align" options

Last time I checked they wouldn't take any effect with `ColumnLimit: 0`, but maybe I am wrong, or this may have changed. However, once there is a line break, I think using a line break for all is cleaner (`cargo fmt` agrees with me and consistently either uses a line-separator or space-separator, but never mixes them [1]). It would be nice if there was a clang-format config that did what `cargo fmt` does by default.


[1] https://
...

💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2376345627)
Reviewed, but did not test the retry flow in the GUI

review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpw
...