💬 dergoegge commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1670008364)
This isn't fully atomic, i.e. in theory two threads could enter this section at the same time.

Not tested but maybe the following works:
```suggestion
if (!pto->IsInboundConn() && peer->m_initial_version_message_sent.compare_exchange_strong(false, true)) {
PushNodeVersion(*pto, *peer);
}
```

Alternatively, we could guard `m_initial_version_message_sent` with `NetEventsInterface::g_msgproc_mutex`, since it is only accessed from the "msghand" thread.

💬 dergoegge commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1669983154)
(not in this PR but) Since we don't want any messages send in `InitializeNode`, maybe it makes sense to disallow that somehow, perhaps by not passing a `CNode`?

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2217001579)
@pinheadmz @tdb3 I should have addressed all of your comments.

@vasild the change since your review is rather than always check twice and discard duplicates, we now only check on error. I chose not to look for the specific error as it was not POSIX and was different on different platforms.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2217017610)
Fixed the deadlock. I ran into the same issue before when working on #29432: `UpdateTip` requires holding `cs_main`. It then takes `g_best_block_mutex` in order to announces the new block. So then lines after `g_best_block_cv.wait_until` should release `g_best_block_mutex` before trying to lock `cs_main`. That's not a problem here.

👋 Sjors's pull request is ready for review: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409)

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2217076704)
@jsarenik if those resources don't help, this would be a good [Bitcoin Stack Exchange](https://bitcoin.stackexchange.com) question.

👍 alfonsoromanz approved a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2165614357)
tACK b715a13d11c6ca7e928b2dee66edcc8c6c30a8d4

The test passes successfully. I also verified that the test fails with no exception raised if the patch in `src/validation.cpp` is not applied, i.e

> File "/Users/alfonso/dev/bitcoin/test/functional/feature_assumeutxo.py", line 238, in test_snapshot_not_on_most_work_chain
> assert_raises_rpc_error(-32603, msg, node1.loadtxoutset, dump_output_path)
> File "/Users/alfonso/dev/bitcoin/test/functional/test_framework/util.py", line 148, in a
...

💬 dergoegge commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1670101598)
`fs::temp_directory_path()` points to the top level tmp dir and won't be unique every time. You'll want to do something similar to what the testing setups are doing: https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/test/util/setup_common.cpp#L150-L152

🤔 DCMTOKEN reviewed a pull request: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207#pullrequestreview-2165657538)
Ok I agree

📝 fanquake locked a pull request: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207)
Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.

While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or m
...

💬 darosior commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1670149765)
Done.

👍 dergoegge approved a pull request: "random: add benchmarks and drop unnecessary Shuffle function"
(https://github.com/bitcoin/bitcoin/pull/30396#pullrequestreview-2165745179)
Code review ACK 6ecda04fefad980872c72fba89844393f5581120

👍 dergoegge approved a pull request: "refactor: use existing RNG object in ProcessGetBlockData"
(https://github.com/bitcoin/bitcoin/pull/30393#pullrequestreview-2165746582)
Code review ACK fa2e74879a3f7423c8d01aa1376b2bd9ccf5658d

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2217223685)
> ZMQ is trying to link against realtime on macOS.

Patched out all usage of `librt` here. Landed one related change upstream: https://github.com/zeromq/libzmq/pull/4702.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2217224491)
Guix Build (aarch64):
```bash
4f3beefc6f4dc2a44829697ebd14e2f1016a35372f261bd379c66190e93db745 guix-build-f684b5d7ddfe/output/aarch64-linux-gnu/SHA256SUMS.part
b80ef38b4ad2a51bd033114884cb4b30e6b10675a6bfca08aa96680a7a754fc9 guix-build-f684b5d7ddfe/output/aarch64-linux-gnu/bitcoin-f684b5d7ddfe-aarch64-linux-gnu-debug.tar.gz
1c9c9198bda3419e9d49c809ef2a7e4e1c2b7846f9e03695e5175dbc7a5c0c8f guix-build-f684b5d7ddfe/output/aarch64-linux-gnu/bitcoin-f684b5d7ddfe-aarch64-linux-gnu.tar.gz
3a9b4b
...

💬 dergoegge commented on issue "fuzz: mini_miner_selection: ASSERT: mock_template_txids.size() <= blocktemplate->block.vtx.size()":
(https://github.com/bitcoin/bitcoin/issues/30367#issuecomment-2217242177)
cc @glozow @murchandamus (in case you missed it) would be nice to have this fixed and also have an explanation of the actual bug, since it is somewhat of a mystery why it's so hard to find

💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1670235553)
Makes sense. I think the easiest way to achieve this is by passing the `CNode` reference as `const` instead. Since that is simple enough, I've added an extra commit for this (with an example of a compiler error in the commit message).

💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1670236195)
Thanks, I've decided to take the second approach, i.e. guarding with `NetEventsInterface::g_msgproc_mutex`.

👍 dergoegge approved a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2165854678)
Code review ACK fb587ce36afcc8789214af45a36082c4f5238e50

💬 fjahr commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1670347496)
Here and in `rest.cpp`: Structuring it like this should save us the `IsBlockPruned()` call in the normal case (untested). `IsBlockPruned()` checks the same condition plus something extra so this should not change behavior.

```suggestion
if (!(blockindex.nStatus & BLOCK_HAVE_DATA)) {
if (blockman.IsBlockPruned(blockindex) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");
}
throw JSONRPCError(RPC_MISC_ERRO
...