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

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1670449484)
```suggestion
self.wait_until(lambda: len(node.getpeerinfo()) == 0)
```

nit in the last commit: I am not a fan of using `assert_debug_log` to sync the test logic. Otherwise the test may fail on slow machines.

If you want to keep it, please make the sync explicit by increasing the timeout of `assert_debug_log`. Otherwise, you can do a proper wait (as suggested) by moving the `wait_until` into the inner scope.

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1670450225)
```suggestion
peer->m_initial_outbound_version_message_sent = true;
```

nit: Could clarify the name

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2217564380)
ACK fb587ce36afcc8789214af45a36082c4f5238e50

💬 dergoegge commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1670484095)
This should nuke the whole parent tmp dir that was created for the fuzz test, not just `init_datadir`. I'm seeing tons of left over directories otherwise.

💬 dergoegge commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1670126790)
Trying to choose probabilities like this won't really work with modern fuzzing engines (because the inputs given to the harness are not uniformly random) and is probably equivalent to the following:

```suggestion
const bool is_random{fuzzed_data_provider.ConsumeBool()};
```