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

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686734)
same, can be removed now I think (and 70 minutes isn't inclusive anymore, right?)

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072686476)
nit: if you edit again please reformat `TestSubscriber` as well

🤔 naiyoma reviewed a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298#pullrequestreview-2813548569)
Tested ACK https://github.com/bitcoin/bitcoin/pull/31298/commits/9d31e94bf584e12269683b74b1c96c7ac883a760

I have tested the changes on regtest and got expected output

```
bitcoin-cli -regtest combinerawtransaction "[\"$RAW_TX1\", \"$RAW_TX2\"]"
error code: -8
error message:
Transaction 1 not compatible (different transactions)
```

🤔 janb84 reviewed a pull request: "doc: Explain that .gitignore is not for IDE-specific excludes"
(https://github.com/bitcoin/bitcoin/pull/32417#pullrequestreview-2813549591)
ACK [fada115](https://github.com/bitcoin/bitcoin/commit/fada115cbeaa8c0ca3d19178499079b66ee5f499)

Will reduce issues & PR's , good to include the extra comment.

💬 hebasto commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2849384794)
Friendly ping @laanwj @Sjors :)