💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670617785)
I kept this here intentionally to make the diff easier to review. We don't have to consider whether it breaks anything if we move it up.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670623673)
If you decide to keep it, yes. Otherwise removing it seems like a better option to me.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670623930)
Thanks for checking!

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670635101)
nit: you haven't touched this line, but Sonar complains that: `"std::move" should not be called on an object of a type with neither move constructor nor move-assignment operator.` - ignore it, if it's unrelated (the rest from https://corecheck.dev/bitcoin/bitcoin/pulls/28280 are also not very useful, but I can't judge the severity of this one)

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670639971)
Hmm but `CCoinsCacheEntry` does have a move constructor https://github.com/bitcoin/bitcoin/pull/28280/files#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R156.

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2217899733)
> would be good to explain how this is different from the existing approach, introduced in "fuzz: version handshake" https://github.com/bitcoin/bitcoin/pull/20370, which was a replacement for "fuzz: Version handshake" https://github.com/bitcoin/bitcoin/pull/20097 .

The main difference is that this harness focuses on the version handshake, where a modification to the existing `process_messages` harness would further widen the space of reachable code. It could make sense to have both but I woul
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670645132)
Which test should I run to check?
I tried `src/test/test_bitcoin --run_test=coins_tests` and `make check`, they all passed when I commented out the throw. Also, code coverage (I know it's incomplete, but still) indicates, this line is not exercised
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/db474f58-eb25-4a8d-805d-3bf2236f892e">

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670649108)
Oh, right I meant don't clear the flags during a BatchWrite and this line will throw, breaking the tests.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670652213)
So if I understood you correctly, there is no point in testing that this exception is thrown, right?

💬 maflcko commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2217916691)
> Happy to take that out of the op, the primary goal is to have fuzzing coverage for the version handshake (non-existent currently), which would be able to find such bugs.

Are you sure it is completely non-existent? I am sure I did fuzz the version handshake at some point in time and the current coverage report (https://maflcko.github.io/b-c-cov/fuzz.coverage/src/net_processing.cpp.gcov.html) seems to agree. Maybe I am missing something.

No objection in any case. I am just trying to unders
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670655362)
dunno: https://sonarcloud.io/project/issues?id=aureleoules_bitcoin&branch=28280-e1076541e62972464193afb1f8d90ff178665ecb&resolved=false&open=AZCVwk_WMNEdWlV7XnLo

💬 glozow commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1670656006)
Btw, this file should go in the `doc/` dir instead of `doc/release-notes/` (which is the archive of old release notes by version)

📝 glozow opened a pull request: "[doc] archive v26.2 release notes"
(https://github.com/bitcoin/bitcoin/pull/30414)

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2217941499)
> Are you sure it is completely non-existent?

The coverage we do have comes from the `process_messages` harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.

I'll generate a coverage report to show the difference

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2217946806)
The [benchmarks](https://corecheck.dev/bitcoin/bitcoin/pulls/28280) indicate that `BlockToJsonVerboseWrite` was slowed down

<img src="https://github.com/bitcoin/bitcoin/assets/1841944/8b5d5299-a4c2-4b26-9a74-44dd2504a20c">

I ran a before/after against `master`, found only a minor (4%) diff - is this expected?
> make -j10 && ./src/bench/bench_bitcoin --filter=BlockToJsonVerboseWrite --min-time=10000


before:
| ns/op | op/s | err% | total | benchmark
...

🤔 glozow reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166427591)
lgtm, do you plan to take any of the suggestions?

💬 glozow commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1670590969)
nitty nit: this diff should be in f8594553fab6f865b13be4d5873b4ff55aae62d2, not e1349beed448cbecb276991019e78f0fccf579cd

💬 glozow commented on issue "Mini miner `AncestorFeerateComparator` Signed Integer Overflow":
(https://github.com/bitcoin/bitcoin/issues/30284#issuecomment-2217978085)
Ah, I think #30412 might fix this since `FeeFrac::Mul` handles the overflow? I'm not sure how to get the input from that CI task so I can check.

💬 glozow commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1670728220)
instead of b1b28a21e185dfbc0c75f92ac55401a9ba8a5b08, I'd recommend rebasing on #30412
I was able to reproduce the ub crash and verify that 6254c253e995e77196b853784cfd0f42619b324d fixes it.

🤔 ismaelsadeeq reviewed a pull request: "MiniMiner: use FeeFrac in AncestorFeerateComparator"
(https://github.com/bitcoin/bitcoin/pull/30412#pullrequestreview-2166670402)
Concept ACK 271469b8bcb84af8f7a64c530e9cee7e0bf96352