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

🤔 maflcko reviewed a pull request: "rpc, rest: Improve getblock error when only header is available"
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2166165697)
Thanks for picking this up.

Seems fine to fix it this way.

However, you forgot `GetBlockChecked` (and all the other functions that can fail in the same way)?

My preference to fix it would be to move the checks to the inside of the block manager. This way,

* module-external cs_main locking (and races) are avoided, because a prune call happening after the `IsBlockPruned` check (and leaving the scope of cs_main) could be detected, if wanted, by doing the check again in the block manage
...

🤔 maflcko reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166186487)
left two nits.