refactor: call GetBestBlock() before NewIterator() #22266

1. 
   endjkv commented at 8:24 am on June 17, 2021: none

   This fixes the potential memory leak caused by NewIterator() when GetBestBlock() throws an exception. (When compiled with clang, the parameters are evaluated from left to right)
2. 
   maflcko commented at 8:33 am on June 17, 2021: member

   Isn’t this solved by std::make_unique already? #22263 (review)

   Edit: Oh never mind, it is not.

3. DrahtBot added the label Refactoring on Jun 17, 2021
4. DrahtBot added the label UTXO Db and Indexes on Jun 17, 2021
5. 
   DrahtBot commented at 6:08 pm on June 17, 2021: contributor

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

   Conflicts

   No conflicts as of last run.

6. 
   Talkless commented at 6:50 pm on June 17, 2021: none

   Edit: Oh never mind, it is not.

   Right, it there would be no need for this PR if CDBWrapper::NewIterator() would return std::unique_ptr.

7. in src/txdb.cpp:197 in 6dc9aefea7 outdated

   169@@ -170,7 +170,8 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) {
   170 
   171 CCoinsViewCursor *CCoinsViewDB::Cursor() const
   172 {
   173-    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
   174+    uint256 hashBestChain = GetBestBlock();
   

   ---

   ---

   

   Talkless commented at 6:53 pm on June 17, 2021:

   nit: could be const
8. in src/txdb.cpp:174 in 6dc9aefea7 outdated

   169@@ -170,7 +170,8 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) {
   170 
   171 CCoinsViewCursor *CCoinsViewDB::Cursor() const
   172 {
   173-    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
   174+    uint256 hashBestChain = GetBestBlock();
   175+    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), hashBestChain);
   

   ---

   ---

   

   Talkless commented at 6:56 pm on June 17, 2021:

   nit: could be * const i if we are changing that line in any case :) . Pointer itself is never changed in the function.
9. 
   Talkless commented at 6:56 pm on June 17, 2021: none

   Concept ACK
10. 
    endjkv commented at 1:56 am on June 23, 2021: none

    Thanks for the reply. I will check this PR again after #22263 has been merged. I’m not sure whether CDBWrapper::NewIterator() will be wrapped by a std::unique_ptr or not. If so, there’s no need for this PR.
11. DrahtBot added the label Needs rebase on Jun 23, 2021
12. refactor: call GetBestBlock() before NewIterator() ae29fffbc7
13. endjkv force-pushed on Jun 24, 2021
14. DrahtBot removed the label Needs rebase on Jun 24, 2021
15. 
    achow101 commented at 6:32 pm on October 12, 2022: member

    Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
16. achow101 closed this on Oct 12, 2022

    ---

17. bitcoin locked this on Oct 12, 2023

Contributors
endjkv maflcko DrahtBot Talkless achow101

Labels
Refactoring UTXO Db and Indexes