💬 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 :)

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2072702883)
Whenever I tried going back to pre-cmake times, I couldn't get it to compile and run again (on Mac or on Linux). Not the first time I try to do a git bisect, but it always turns bad really quickly.

I tried pushing a PR to my fork with the [original PR](https://github.com/bitcoin/bitcoin/pull/28865) that introduced the suppressions + removing `DynamicMemoryUsage`, `SpendCoin` and `Uncache` suppressionss from ubsan.

The build is [failing with similar problems](https://github.com/l0rinc/bitco
...

💬 naiyoma commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2072704239)
nit: IMO, this could be a bit more descriptive

```diff
// Can't merge < 2 items
if (txs.size() < 2) {
- throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
+ throw JSONRPCError(
+ RPC_INVALID_PARAMETER,
+ txs.size() == 0
+ ? "Missing transactions. At least two transactions required."
+ : "Insufficient transactions. At least two transactions required."
+ );
}
```

✅ furszy closed a pull request: "[WIP] wallet: standardize change output detection process"
(https://github.com/bitcoin/bitcoin/pull/25979)

💬 furszy commented on pull request "[WIP] wallet: standardize change output detection process":
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-2849481894)
> The dependency was closed more than a year ago: [#27601 (comment)](https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1989555740)

Time flies. Closing for now as it is not in my priorities.