💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072675975)
👍 I can confirm that this passes after the PR and fails without the PERIODIC flushing moved inside the loop

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072674865)
After we've initialized the mock time we might as well use that as the basis (it also gets rid of the type hint):
```suggestion
SetMockTime(GetMockTime() + 70min);
```

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072677126)
Can you please explain where the 3 separate locks were that we're replacing (I only see 2, but don't yet have experience with how we're locking exactly)?

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072674552)
```suggestion
BOOST_CHECK_EQUAL(tip, chainstate.m_chain.Tip());
```

(and a few others, though displaying the pointer values isn't very useful, unless one of the is a `nullptr`)

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072676586)
My understanding this is equivalent to:
```suggestion
LOCK2(m_node.chainman->GetMutex(), chainstate.MempoolMutex());
```

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072675369)
Nit - [developer notes suggests](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L70-L71) to put the braces to new lines:
```md
- Braces on new lines for classes, functions, methods.
```

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072678126)
What about structs?

💬 l0rinc commented on pull request "doc: Explain that .gitignore is not for IDE-specific excludes":
(https://github.com/bitcoin/bitcoin/pull/32417#issuecomment-2849351198)
ACK fada115cbeaa8c0ca3d19178499079b66ee5f499

PR title and commit message still state IDE, but it's not important.

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072683803)
`AfterStruct` seems to fall back to `AfterClass` if unspecified https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L19

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072685646)
1. https://github.com/bitcoin/bitcoin/pull/32414/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98L3566
2. https://github.com/bitcoin/bitcoin/pull/32414/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98L3570
3. FlushStateToDisk

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072685989)
Done.

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686009)
Done.

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686020)
Done.

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686031)
Done.

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686068)
Done.

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686230)
Yeah, but isn't the locking kept in `FlushStateToDisk`? So we're reducing 3 locks to 2 locks, right? That's not what I understand from the commit message

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686728)
Done in c1e554d3e5834a140f2a53854018499a3bfe6822. I had to move `FlushStateToDisk` above possibly calling `m_chainman.snapshot_download_completed()` and breaking or breaking on `m_chainman.m_interrupt`, but that order should not make a difference.

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072687088)
If we kept the code as is, and `exited_ibd` is `true` and `m_disabled` is `false`, then we would be releasing and acquiring the lock 3 times after we release it for the inner loop.

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686596)
now that we have the constants these comments don't really add anything

👍 l0rinc approved a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414#pullrequestreview-2813539202)
Code review ACK - I'm still running the reindex / IBD benchmarks to see if this changed anything unexpected.

Do you think I should stop and restart after the locking changes?

<details>
<summary>nits</summary>

```patch
diff --git a/src/test/chainstate_write_tests.cpp b/src/test/chainstate_write_tests.cpp
index e1b82ebc12..44257d5591 100644
--- a/src/test/chainstate_write_tests.cpp
+++ b/src/test/chainstate_write_tests.cpp
@@ -16,7 +16,8 @@ BOOST_AUTO_TEST_SUITE(chainstate_write_tes
...