refactor: wrap CCoinsViewCursor in unique

jamesob commented at 0:35 am on June 17, 2021: member

I tripped over this one for a few hours at the beginning of the week, so I’ve sort of got a personal vendetta against CCoinsView::Cursor() returning a raw pointer.

Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.).

This is a pretty simple change.

DrahtBot added the label Refactoring on Jun 17, 2021

DrahtBot added the label RPC/REST/ZMQ on Jun 17, 2021

DrahtBot added the label UTXO Db and Indexes on Jun 17, 2021

in src/txdb.cpp:174 in a9bc8df697 outdated

167@@ -168,9 +168,10 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) {
168     return Read(DB_LAST_BLOCK, nFile);
169 }
170 
171-CCoinsViewCursor *CCoinsViewDB::Cursor() const
172+std::unique_ptr<CCoinsViewCursor> CCoinsViewDB::Cursor() const
173 {
174-    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
175+    std::unique_ptr<CCoinsViewDBCursor> i{
176+        new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock())};

MarcoFalke commented at 5:50 am on June 17, 2021:

0    auto i{std::make_unique<CCoinsViewDBCursor>(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock())};

Can be written shorter with make_unique?

jamesob commented at 1:48 pm on June 17, 2021:

Good call!

MarcoFalke commented at 5:51 am on June 17, 2021: member

Concept ACK

MarcoFalke removed the label RPC/REST/ZMQ on Jun 17, 2021

MarcoFalke removed the label UTXO Db and Indexes on Jun 17, 2021

DrahtBot commented at 8:26 am on June 17, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22266 (refactor: call GetBestBlock() before NewIterator() by endjkv)
#22242 (Move CBlockTreeDB to node/blockstorage by MarcoFalke)
#17487 (coins: allow write to disk without cache drop by jamesob)

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.

practicalswift commented at 8:56 am on June 17, 2021: contributor

Concept ACK: nice catch!

Non-blocking nit: The list[1] of naked new:s in our code base is fairly short nowadays so it would be nice to avoid adding an unnecessary new to that list. Avoided by using std::make_unique as suggested by MarcoFalke :)

[1]

0$ git grep -E 'new [A-Za-z0-9_]+\(' -- "*.cpp" "*.h" ":(exclude)src/qt/" ":(exclude)src/test/" \
1      ":(exclude)src/bench/" ":(exclude)src/leveldb/"

in src/test/fuzz/coins_view.cpp:186 in a9bc8df697 outdated

182@@ -183,8 +183,8 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
183     }
184 
185     {
186-        const CCoinsViewCursor* coins_view_cursor = backend_coins_view.Cursor();
187-        assert(coins_view_cursor == nullptr);
188+        const std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();

jnewbery commented at 9:41 am on June 17, 2021:

Note that this isn’t equivalent. Previously coins_view_cursor was a pointer to const data, and this is now a const pointer to mutable data. The equivalent of what we had before would be:

0        std::unique_ptr<const CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();

It doesn’t really matter here - I’d suggest just removing the const keyword entirely.

jamesob commented at 1:48 pm on June 17, 2021:

Good point, thanks.

jnewbery commented at 10:07 am on June 17, 2021: member

Concept ACK. I agree with @MarcoFalke that we should use the standard std::make_unique() function when creating new unique pointers.

refactor: wrap CCoinsViewCursor in unique_ptr

Specifically with CCoinsViewDB, if a raw cursor is allocated and
not freed, a cryptic leveldb assertion failure occurs on
CCoinsViewDB destruction.

See: https://github.com/google/leveldb/issues/142#issuecomment-414418135

615c1adfb0

jamesob force-pushed on Jun 17, 2021

jamesob commented at 1:49 pm on June 17, 2021: member

Thanks for the quick looks everyone. I’ve made both changes suggested, though it’s worth noting that I had to make the CCoinsViewDBCursor constructor public (from private) in order to allow std::make_unique to compile.

jnewbery commented at 3:07 pm on June 17, 2021: member

Code review ACK 615c1adfb07b9b466173166dc2e53ace540e4b32

in src/txdb.h:77 in 615c1adfb0 outdated

73@@ -74,6 +74,8 @@ class CCoinsViewDB final : public CCoinsView
74 class CCoinsViewDBCursor: public CCoinsViewCursor
75 {
76 public:
77+    CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256 &hashBlockIn):

jonatack commented at 11:12 am on June 18, 2021:

Verified the ctor needs to be public to not see error: calling a private constructor of class 'CCoinsViewDBCursor' when building. Could clang-format if you retouch:

0    CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256& hashBlockIn):

MarcoFalke commented at 3:32 pm on June 18, 2021:

Is there a reason to put this into the header? Previously the constructor was private, thus disallowing “illegal” access. Now, with it being public, the whole class could be moved to the cpp file?

jamesob commented at 6:16 pm on June 18, 2021:

Good call! No reason this needs to be exposed; moved to the .cpp file.

in src/test/fuzz/coins_view.cpp:187 in 615c1adfb0 outdated

182@@ -183,8 +183,8 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
183     }
184 
185     {
186-        const CCoinsViewCursor* coins_view_cursor = backend_coins_view.Cursor();
187-        assert(coins_view_cursor == nullptr);
188+        std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();
189+        assert(!coins_view_cursor);

jonatack commented at 11:16 am on June 18, 2021:

The change in this line seems to be style only.

jonatack commented at 11:26 am on June 18, 2021: member

ACK 615c1adfb07b9b466173166dc2e53ace540e4b32

in src/txdb.h:91 in 615c1adfb0 outdated

85@@ -84,8 +86,6 @@ class CCoinsViewDBCursor: public CCoinsViewCursor
86     void Next() override;
87 
88 private:
89-    CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256 &hashBlockIn):
90-        CCoinsViewCursor(hashBlockIn), pcursor(pcursorIn) {}
91     std::unique_ptr<CDBIterator> pcursor;
92     std::pair<char, COutPoint> keyTmp;
93

MarcoFalke commented at 3:32 pm on June 18, 2021:

line 92 still needed?

jamesob commented at 6:03 pm on June 18, 2021:

Yes, for e.g. pcursor access in CCoinsViewDB::Cursor().

MarcoFalke changes_requested

ryanofsky approved

ryanofsky commented at 5:45 pm on June 18, 2021: member

Code review ACK 615c1adfb07b9b466173166dc2e53ace540e4b32. Changes all look correct and should be safer.

I am confused by this comment though: #22263 (comment)

it’s worth noting that I had to make the CCoinsViewDBCursor constructor public (from private) in order to allow std::make_unique to compile.

I’m not sure if you think it is better or worse for the constructor to be public. If it is unsafe or bad to use to use the constructor, it would be nice to either:

Document the new public constructor saying when it should or shouldn’t be called
Or keep the constructor private and just use unique_ptr<CCoinsViewDBCursor>(new CCoinsViewDBCursor...) with a comment about why a private constructor needs to be called.

These are basically just the two recommendations from https://abseil.io/tips/134#recommendations

move-only(ish): don't expose CCoinsViewDBCursor

No need for this to be a part of the header anymore.

Includes a small reference type style change.

0f8a5a4dd5

doc: add comment about CCoinsViewDBCursor constructor 7ad414f4bf

jamesob commented at 6:17 pm on June 18, 2021: member

Thanks for the feedback all. I’ve added doc, fixed the reference type style, and made the CCoinsViewDBCursor private to the unit.

MarcoFalke approved

MarcoFalke commented at 8:05 pm on June 18, 2021: member

review ACK 7ad414f4bfa74595ee5726e66f3527045c02a977 🔎

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 7ad414f4bfa74595ee5726e66f3527045c02a977 🔎
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiK0Av/YpJeCqfCFF8r/xKrmy/2fsY3yRopzMKExwCG6ZPVfJ09R0xfpAtoezgw
 893OlloYTOppLdwCadZUt3tkY/Dd4YyHDH8mtlJif9B2NZtHQPRMkvy0QbB7I+XCc
 9O6JcW/tQi5FYsbXKUDLCzKqLC0VVJdhqijybnst0RQDXAYXHjKXwUZX1Tlywj4PC
105nPGFbQH5VbM9y54LgqjhBTogQP88N9Zn2T7jkm9vPgCX/FBhrnTqXHvIMssyjS1
11cS6DUUxpF7iePYI4PugliofOl8bmlW7AIQcRj83qk8JCH9Bdr1v/WMC69kBGm06v
12UpzsEJ0F9ZbmgHhvzT7Nkdgtq2QNu3R90HQlnVDd6m3qXSaSGUmZfrwCl67neHdw
13BUlF5dQoXBgHdn+yms3iElNFxpfnwTRMzyWcBAjFJTGNQuzVIuHBSDRgJpjtTi67
145Wjay1OlBZNRkuej28hfTDD0UWqsxitPo1Me7C25c39WSeaN3wu5tw5hICkO4IIK
15Pp60bF6D
16=VU9g
17-----END PGP SIGNATURE-----

Timestamp of file with hash 6d7187a8c395c0137963dbd7752b6ff666a4fce6cdbe0e710567344055391df2 -

in src/txdb.cpp:177 in 7ad414f4bf

174 {
175-    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
176+public:
177+    // Prefer using CCoinsViewDB::Cursor() since we want to perform some
178+    // cache warmup on instantiation.
179+    CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256&hashBlockIn):

jonatack commented at 12:20 pm on June 19, 2021:

I’d suggest doing the formatting change in the first commit and making the second commit purely move-only.

0    CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256& hashBlockIn):

jamesob commented at 3:08 pm on June 21, 2021:

Agree in principle but this commit is so easy to verify that I don’t think it makes sense to rebase.

jonatack commented at 12:23 pm on June 19, 2021: member

re-ACK 7ad414f4bfa74595ee5726e66f3527045c02a977 modulo suggestion

jonatack commented at 12:26 pm on June 19, 2021: member

These are basically just the two recommendations from https://abseil.io/tips/134#recommendations

Thanks for the informative link @ryanofsky!

ryanofsky approved

ryanofsky commented at 3:45 pm on June 23, 2021: member

Code review ACK 7ad414f4bfa74595ee5726e66f3527045c02a977. Two new commits look good and thanks for clarifying constructor comment

MarcoFalke merged this on Jun 23, 2021

MarcoFalke closed this on Jun 23, 2021

sidhujag referenced this in commit 2dabf43c8f on Jun 24, 2021

gwillen referenced this in commit 3c48d95401 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

refactor: wrap CCoinsViewCursor in unique_ptr #22263

Conflicts