A few follow-ups to #17487 (coins write without cache drop)

sipa commented at 5:34 pm on January 30, 2023: member

This addresses a few nits left open in #17487.

DrahtBot commented at 5:34 pm on January 30, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, jamesob, jonatack, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25325 (Add pool based memory resource by martinus)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jamesob commented at 5:47 pm on January 30, 2023: member

Github code review ACK

Thanks for doing these! I’ll clone and test locally soon.

in src/test/coins_tests.cpp:58 in 34d63e457d outdated

54@@ -55,7 +55,7 @@ class CCoinsViewTest : public CCoinsView
55 
56     bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override
57     {
58-        for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : ++it) {
59+        for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) {

jonatack commented at 5:59 pm on January 30, 2023:

✅ This change addresses #17487 (review).

in src/coins.cpp:254 in 34d63e457d outdated

249@@ -250,7 +250,10 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
250 
251 bool CCoinsViewCache::Flush() {
252     bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ true);
253-    cacheCoins.clear();
254+    if (cacheCoins.size() != 0) {
255+        /* BatchWrite must erase all cacheCoins elements when erase=true. */

jonatack commented at 6:00 pm on January 30, 2023:

✅ This change addresses #17487 (review). A few suggestions.

 0 
 1 bool CCoinsViewCache::Flush() {
 2-    bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ true);
 3-    if (cacheCoins.size() != 0) {
 4-        /* BatchWrite must erase all cacheCoins elements when erase=true. */
 5+    bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true);
 6+    if (fOK && !cacheCoins.empty()) {
 7+        // BatchWrite must erase all cacheCoins elements when erase=true.
 8         throw std::logic_error("Not all cached coins were erased");
 9     }
10     cachedCoinsUsage = 0;
11@@ -260,7 +260,7 @@ bool CCoinsViewCache::Flush() {
12 
13 bool CCoinsViewCache::Sync()
14 {
15-    bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ false);
16+    bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false);
17     // Instead of clearing `cacheCoins` as we would in Flush(), just clear the

sipa commented at 6:14 pm on January 30, 2023:

Done.

in src/test/fuzz/coins_view.cpp:79 in 34d63e457d outdated

73@@ -74,6 +74,9 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
74             [&] {
75                 (void)coins_view_cache.Flush();
76             },
77+            [&] {
78+                (void)coins_view_cache.Sync();
79+            },

jonatack commented at 6:02 pm on January 30, 2023:

✅ This change addresses #17487 (comment)

jonatack commented at 6:04 pm on January 30, 2023: contributor

ACK 34d63e457d271af41f36172d5cd35469e4a3675e

Edit, the feebumper unit test CI failure seems unrelated.

Make test/fuzz/coins_view exercise CCoinsViewCache::Sync() bb00357add

Follow coding style for named arguments 98db35c2f8

Avoid unclear {it = ++it;} 941feb6ca2

Add assertions that BatchWrite(erase=true) erases 2e16054a66

sipa force-pushed on Jan 30, 2023

jonatack commented at 6:37 pm on January 30, 2023: contributor

ACK 2e16054a66b0576ec93d4032d6b74f0930a44fef

Sjors commented at 7:45 pm on January 30, 2023: member

utACK 2e16054a66b0576ec93d4032d6b74f0930a44fef

MemorySanitizer: use-of-uninitialized-value CI failure in feebumper_tests.cpp seems unrelated. cc @achow101

jamesob approved

jamesob commented at 7:53 pm on January 30, 2023: member

ACK 2e16054a66b0576ec93d4032d6b74f0930a44fef (jamesob/ackr/26999.1.sipa.a_few_follow_ups_to_1748)

Built, reviewed diff, ran tests locally.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 2e16054a66b0576ec93d4032d6b74f0930a44fef ([`jamesob/ackr/26999.1.sipa.a_few_follow_ups_to_1748`](https://github.com/jamesob/bitcoin/tree/ackr/26999.1.sipa.a_few_follow_ups_to_1748))
 4
 5Built, reviewed diff, ran tests locally.
 6
 7-----BEGIN PGP SIGNATURE-----
 8
 9iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmPYIBkACgkQepNdrbLE
10TwVNIxAAiyTgokcHrKdzbD4NbrweHcjs8t0jXl4iSLQ6vMHHk/DU/BPMLbJaw1V8
11bXNueFYw8Z6ptbS5yqd9BdETdMTqvnCUW+zV5p6pdaOxBCNFjKyz5obQBksv/RZc
12u+0gJfhwHJm5Pm0KUCzGjdttC1S8gsD5thE6bo4VxGBd8KmMCCrN78ejzd+G/+vx
13Zfhw3z6feivkg8QjDJRejRCGkFdSqmlSn5YspEOsVd5Au9e51KywLFTdV9ESnLJ8
14By7JUiXjN4b5Sk9FyAPx1Lb2MDt1CE30/AcDy2oEWNtePEBXrkhYK08c5dEl1xdx
153ZOhjeiLcLWaucGOTF8OwoYUOvtFeZUnvtqhtHYKIe5qKV9TytWr5RGn0K4kwfq8
16jiz766M2kHePRHkYvYpkA38PAtjpvJD0neOg78ke9ekPpFNAERTipbO0YPP00qhw
17bUYJ88cIEZqEbLTp6oM6Wgn8Sp+Z/PCoTe5HwcWWh4UhQJoC3dvpGpzxdBStVnhD
18pLcil5eaCZB4xZH2hWgUrRqNPGVgvh8vW48S1l3dj15PRK/hETgnmDE67YZ/7gUI
19as4NxkMXU6kmQ9c9wTZ42Qe8nhmu3dUszMppFmEOMsBXrE8uge6lo+cRVidzoU5c
20W6LHVtZRtMLWI4ydJPXDlNCCBDyFtuIh04qTQ515WobVnCeRuHs=
21=RV3M
22-----END PGP SIGNATURE-----

0Tested on Linux-6.1.6-arch1-3-x86_64-with-glibc2.36
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin2/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin2/db4/include/ 'CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis' --enable-wallet --enable-debug --with-daemon --enable-natpmp-default
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i
5
6Compiler version: clang version 15.0.7

jonatack commented at 9:01 pm on January 30, 2023: contributor

The MSan CI failure is due to #27001 that should be fixed in #26998.

achow101 commented at 9:53 pm on January 30, 2023: member

ACK 2e16054a66b0576ec93d4032d6b74f0930a44fef

achow101 merged this on Jan 30, 2023

achow101 closed this on Jan 30, 2023

sidhujag referenced this in commit 04f83d37fb on Jan 30, 2023

bitcoin locked this on Jan 30, 2024

A few follow-ups to #17487 (coins write without cache drop) #26999

Reviews

Conflicts