💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670611362)
I can expand to `m_current` if that's more readable? Or do you have a better suggestion?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670612269)
I am deliberately matching the style used where I am making changes, yes.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670613036)
Tests will fail if they are not all cleared, try it :).

💬 maflcko commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2217865662)
Concept ACK, but 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 .


Now that the timedata bug is fixed, and the module and related globals removed completely, it is a bit hard(er) to re-introduce it for testing, I guess. IIRC I did check it at some point and found that the existing
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1670615264)
The developer guides say to prefer using the recommended style over mimicking the surrounding style.

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