💬 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()};
```

📝 glozow opened a pull request: "MiniMiner: use FeeFrac in AncestorFeerateComparator"
(https://github.com/bitcoin/bitcoin/pull/30412)
Fixes crash in #30367. See https://github.com/bitcoin/bitcoin/issues/30367#issuecomment-2217459257

Previously, we were only comparing feerates up to 1/1000 precision, since CFeeRate comparison just looks at their respective nSatoshisPerK. This could lead to MiniMiner selecting packages in the wrong order (i.e. by txid) if their feerates were less than 0.001sat/vB different. Fix this by creating + comparing `FeeFrac`s instead.

🤔 glozow reviewed a pull request: "refactor: use existing RNG object in ProcessGetBlockData"
(https://github.com/bitcoin/bitcoin/pull/30393#pullrequestreview-2166326876)
ACK fa2e74879a3f7423c8d01aa1376b2bd9ccf5658d

🤔 furszy reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166329252)
Code review ACK fb587ce36afcc8789214af45a36082c4f5238e50

Nice finding.

🚀 glozow merged a pull request: "refactor: use existing RNG object in ProcessGetBlockData"
(https://github.com/bitcoin/bitcoin/pull/30393)

📝 dergoegge opened a pull request: "fuzz: Version handshake"
(https://github.com/bitcoin/bitcoin/pull/30413)
This adds a fuzzing harness dedicated to the version handshake. To avoid determinism issues, the harness creates necessary components each iteration (addrman, peerman, etc). A harness like this would have caught easily https://bitcoincore.org/en/2024/07/03/disclose-timestamp-overflow/.

As a performance optimization, this PR includes a change to `PeerManager` to lazily initialize various filters (to avoid large unnecessary memory allocations each iteration).

👍 dergoegge approved a pull request: "test: [refactor] Pass TestOpts"
(https://github.com/bitcoin/bitcoin/pull/30407#pullrequestreview-2166443256)
utACK fa690c8e532672f7ab53be6f7a0bb3070858745e

💬 ismaelsadeeq commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1670578710)
Why pass `finalize=false`? the test still passes without it.

🤔 ismaelsadeeq reviewed a pull request: "Fix cases of calls to `FillPSBT` errantly returning `complete=true`"
(https://github.com/bitcoin/bitcoin/pull/30357#pullrequestreview-2166405948)
Tested ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902

I've verified that this PR fixes the issue #30077, `complete=false` is returned instead of throwing an error.

Just a single test question and a non-blocking nit.

💬 ismaelsadeeq commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1670582419)
nit
```suggestion
assert_equal(signed_psbt_incomplete["complete"], False)

```

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670609997)
Hmm yes because we no longer erase in `BatchWrite`, since we don't pass the `coinsCache` map anymore.