💬 kevkevinpal commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2375355917)
Concept ACK the for loop is confusing and refactoring it to something better is a welcomed change

💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1775532749)
nit (not really related, but I noticed it asking myself why `success=False` here): The doc "if success is False: assert that the node's tip doesn't advance" in `send_blocks_and_test` is wrong. It it is only checked that the node's tip doesn't advance to the last block provided, however it can advance to another block, as it happens here.

🤔 mzumsande reviewed a pull request: "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)"
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-2328776410)
Code Review ACK c6ca2a17ea4d76f21d7702f70ba892a9494576bf
I agree that it is more consistent to keep the same tip over a restart in case of multiple candidates (although I don't think this is much of a problem in practice).

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776227563)
I've moved it to its own commit. I left the name `iterations_left` as I think calling it "optimizations" is misleading; most steps don't optimize anything. It's true that the iterations are not actually the loop's iterations anymore, but they're still very correlated to the number of iterations the algorithm makes.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776231855)
I was hoping to do a bit better than comparing with `AncestorCandidateFinder` (which only tries ancestor sets, not unions of two of them), but it's really not worth the complexity here. I've changed it to using `AncestorCandidateFinder` (here and below), and in a separate commit added an arbitrary subset (from fuzz input) to compare with.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776232273)
You're right that it shouldn't be repeated, but I don't think it's worth much effort to keep this. I've dropped it.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1776235234)
They cannot be guaranteed to be identical; there may be multiple distinct optimal candidate sets.

👋 BrandonOdiwuor's pull request is ready for review: "Wallet: "listreceivedby*" fix"
(https://github.com/bitcoin/bitcoin/pull/30972)

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

Can you explain this a bit better? The CI task is passing, see https://cirrus-ci.com/task/6297362342084608

So I presume this is just for the case when an error happens, because otherwise the symbolizer wouldn't be asked?

💬 fanquake commented on pull request "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job":
(https://github.com/bitcoin/bitcoin/pull/30961#issuecomment-2376068115)
> So I presume this is just for the case when an error happens, because otherwise the symbolizer wouldn't be asked?

Yea. Or fuzzing with no corpus.

💬 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.