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

💬 fjahr commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2217409014)
Concept ACK

🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2165763930)
I think we can remove some of the cursor's state, otherwise I'm mostly OK with the change, thanks for your perseverance!

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670186109)
you basically reverting https://github.com/bitcoin/bitcoin/commit/2e16054a66b0576ec93d4032d6b74f0930a44fef here, right?
Was https://github.com/bitcoin/bitcoin/pull/17487#discussion_r1087087728 just a temporary sanity check that we don't need anymore?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670195903)
nit: if we keep it, is `curr` an commonly used abbreviation or could we expand it?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670199990)
nit: are you deliberately keeping the original `&` placements when refactoring, or when do you use `X& y` and when [`X &y`](https://github.com/bitcoin/bitcoin/pull/28280/commits/adcccaae6b67e0ac574eaab16842bed1ba15389f#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R285)?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670230733)
nit: to be sure that the `Next` method is indeed a `noexcept`, we could annotate https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L47 as well (it seems to be `noexcept` based on the impl), so maybe in a followup PR we could do that as well

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670252649)
nit: we have a few of extra parenthesis now
```suggestion
fresh = !it->second.IsDirty();
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670340407)
nit: now that you've extracted the first complex parameter, it might make the code slightly more readable if we extracted the second one as well

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670224611)
is this important enough to cover it with a test?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670344719)
nit: does the update have to be in the middle or can it go to the top, like the rest (the related tests pass that way)?

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-2217430823)
Now might be a good time to rebase this and the send and receive branches? The "pre-work" commits mentioned in the description are merged and the libsecp PR seems to be in reasonable shape.

💬 glozow commented on issue "fuzz: mini_miner_selection: ASSERT: mock_template_txids.size() <= blocktemplate->block.vtx.size()":
(https://github.com/bitcoin/bitcoin/issues/30367#issuecomment-2217439274)
Ooh fun, this found a difference between `MiniMiner` and `CTxMemPool`'s comparators , which boils down to a lack of precision in `CFeeRate`.

Debugging, the 3 entries in mempool are

A <- B <- C
A: 1sat/264vB
B: 0sat/178vB
C:1sat/178vB

The minimum feerate, i.e. `target_feerate` or `blockMinFeeRate` is 1sat/vB.

`BlockAssembler` selects A. `MiniMiner` selects ABC. They are both wrong to do so, since all transactions have a feerate below 1sat/vB. This is because `CFeeRate` rounds `minf
...

💬 glozow commented on issue "fuzz: mini_miner_selection: ASSERT: mock_template_txids.size() <= blocktemplate->block.vtx.size()":
(https://github.com/bitcoin/bitcoin/issues/30367#issuecomment-2217459257)
I think the best way to align them is to have them store and compare `FeeFrac`s. I can open a PR to change this for `MiniMiner`, which by itself would fix the imprecision bug, though I'm not sure if that'll make it identical to txmempool.

Changing txmempool seems less important imo since there is no current imprecision bug, and perhaps cluster mempool plans to do this anyway.