This is part of the assumeutxo project:
Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
This change moves CCoinsViewErrorCatcher out of init and into coins so that it can later be included in a CoinsView instance under CChainState.
Instead of hardcoding read failure behavior that has knowledge of qt, it accepts error callbacks via AddReadErrCallback().
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
9@@ -10,13 +10,15 @@
10 #include <compressor.h>
11 #include <core_memusage.h>
12 #include <crypto/siphash.h>
13+#include <logging.h>
325+ * Writes do not need similar protection, as failure to write is handled by the caller.
326+*/
327+class CCoinsViewErrorCatcher final : public CCoinsViewBacked
328+{
329+public:
330+ typedef std::function<void()> Function;
329+public:
330+ typedef std::function<void()> Function;
331+
332+ explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
333+
334+ void AddReadErrCallback(const Function f) { m_err_callbacks.emplace_back(std::move(f)); }
Looks like modifying the vector of functions is not thread safe? Should be fine with the current code, but maybe remove this method, and make the function member const, so that it has to be provided via the constructor. Which should be thread-safe?
E.g. something like this:
0diff --git a/src/coins.cpp b/src/coins.cpp
1index 976e058c49..ee0f1ac419 100644
2--- a/src/coins.cpp
3+++ b/src/coins.cpp
4@@ -5,6 +5,7 @@
5 #include <coins.h>
6
7 #include <consensus/consensus.h>
8+#include <logging.h>
9 #include <random.h>
10 #include <version.h>
11
12@@ -263,9 +264,7 @@ bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) cons
13 try {
14 return CCoinsViewBacked::GetCoin(outpoint, coin);
15 } catch(const std::runtime_error& e) {
16- for (auto f : m_err_callbacks) {
17- f();
18- }
19+ m_err_callback();
20 LogPrintf("Error reading from database: %s\n", e.what());
21 // Starting the shutdown sequence and returning false to the caller would be
22 // interpreted as 'entry not found' (as opposed to unable to read data), and
23diff --git a/src/coins.h b/src/coins.h
24index 2ce788cebf..37bbd7c12b 100644
25--- a/src/coins.h
26+++ b/src/coins.h
27@@ -10,7 +10,6 @@
28 #include <compressor.h>
29 #include <core_memusage.h>
30 #include <crypto/siphash.h>
31-#include <logging.h>
32 #include <memusage.h>
33 #include <serialize.h>
34 #include <uint256.h>
35@@ -327,18 +326,17 @@ const Coin& AccessByTxid(const CCoinsViewCache& cache, const uint256& txid);
36 class CCoinsViewErrorCatcher final : public CCoinsViewBacked
37 {
38 public:
39- typedef std::function<void()> Function;
40-
41- explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
42-
43- void AddReadErrCallback(const Function f) { m_err_callbacks.emplace_back(std::move(f)); }
44+ CCoinsViewErrorCatcher(CCoinsView* view, const std::function<void()> err_callback)
45+ : CCoinsViewBacked{view},
46+ m_err_callback{std::move(err_callback)}
47+ {
48+ }
49
50 bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
51
52 private:
53- /** A list of callbacks to execute upon leveldb read error. */
54- std::vector<CCoinsViewErrorCatcher::Function> m_err_callbacks;
55-
56+ /** A callback to execute upon leveldb read error. */
57+ const std::function<void()> m_err_callback;
58 };
59
60 #endif // BITCOIN_COINS_H
61diff --git a/src/init.cpp b/src/init.cpp
62index 3e4ce2ff02..ca06d92ac0 100644
63--- a/src/init.cpp
64+++ b/src/init.cpp
65@@ -1517,12 +1517,12 @@ bool AppInitMain(InitInterfaces& interfaces)
66 // block tree into mapBlockIndex!
67
68 pcoinsdbview.reset(new CCoinsViewDB(nCoinDBCache, false, fReset || fReindexChainState));
69- pcoinscatcher.reset(new CCoinsViewErrorCatcher(pcoinsdbview.get()));
70- pcoinscatcher->AddReadErrCallback([]() {
71- uiInterface.ThreadSafeMessageBox(
72- _("Error reading from database, shutting down."),
73- "", CClientUIInterface::MSG_ERROR);
74- });
75+ pcoinscatcher.reset(new CCoinsViewErrorCatcher(pcoinsdbview.get(),
76+ []() {
77+ uiInterface.ThreadSafeMessageBox(
78+ _("Error reading from database, shutting down."),
79+ "", CClientUIInterface::MSG_ERROR);
80+ }));
81
82 // If necessary, upgrade from older database format.
83 // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
uiInterface (and qt) isn’t known, so we actually need to specify the callback after construction.
ACK 30de23d743ca3d9b3335061bccf6cf8ab30a8b79, mostly move-only
moving CCoinsViewErrorCatcher out of init makes sense, so that e.g. a chainstate constructor can be written and called by unit tests.
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 30de23d743ca3d9b3335061bccf6cf8ab30a8b79, mostly move-only
4
5moving CCoinsViewErrorCatcher out of init makes sense, so that e.g. a chainstate
6constructor can be written and called by unit tests.
7
8-----BEGIN PGP SIGNATURE-----
9
10iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
11pUhU3QwAzwg4UXI93uSKkvTlg+mgt5Rf5Tmg1hSj1kvY7PoMH3JniaUiJLnHuAKn
12a9Uoa1vzeu1WcBfcSxSn3RydEPDMT2TO7eRmzSWSfrBelie35uEWEaXOo7/hWdDK
13UPm0CMb4jryEUyO/jD2jyP1qfEZnEuJfn1MAE9O0npTJbowtwlBS3s+Axhbl/bRg
14Pao4A/IJJabHMnVJi8qTBXLnQhFT3VNylA+8gidVY9nuEi1+2VfFTzUTubEwaLBC
15X3oTajj4IpTNhJjQV+vrFL8e9K0TOaS7e3PvY/6C1Jrme7oZ6lTvSTEf6pdYLj0U
1686fyF7qKGJpSrbZ6c1tMz8uE9gNK175uuLh0QvMVORjNqfH3vRxfLyVq8Pbf+ZGj
17KK69B5tqAZjtD3yDNVdT43HuT7L6Tl9pCt4O8I59vPO+mMMtkXP/mzdvOdCs5D5+
18au//19ip8vu2nmCKqjjvywaC6GvDrGuLQffOdNoAmadYvsCyxjE1w4IgzC42Xz8Z
19ivx+v3++
20=Exqc
21-----END PGP SIGNATURE-----
Timestamp of file with hash 8aaa47a81852504f0e86b718713f70725fbd3105bffb9d111c31263336e34a6d -
ACK 30de23d743ca3d9b3335061bccf6cf8ab30a8b79
Code changes make sense and I tested locally.
326+class CCoinsViewErrorCatcher final : public CCoinsViewBacked
327+{
328+public:
329+ explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
330+
331+ void AddReadErrCallback(const std::function<void()> f) {
const, otherwise std::move below can’t really have any effect.
-w --color-moved=dimmed_zebra.
and into coins.cpp. This move is necessary so that we can later include a
CCoinsViewErrorCatcher instance under CChainState.
Co-authored-by: MarcoFalke <falke.marco@gmail.com>