Separate disk related functions in CClaimTrieDb

[bvbfan]

Signed-off-by: Anthony Fieroni bvbfan@abv.bg

[bvbfan]

@kaykurokawa, OSX build fails, but it's not looks like pull request related.

[kaykurokawa]

Yes OSX build on travis is currently failing for all : https://github.com/lbryio/lbrycrd/issues/120

[kaykurokawa]

Hi, as you mentioned in https://github.com/lbryio/lbrycrd/issues/136 , a bit more aggressive refactor would be acceptable. To be specific , the various dirty memeber variables that is currently managed by CClaimTrie could be managed by CClaimTrieDb. This would make it much clearer exactly what kind of things are being written/read to disk and how, and should also improve some DRY problems involved in the manipulation of dirty member variables.

Please note though I think there is a slight misunderstanding you may have about CClaimTrieCache , this class is NOT a cache of the CClaimTrie. CClaimTrieCache actually contains a cache of changes that must be applied to CClaimTrie upon block increment (removing claims, adding claims, etc..). Thus CClaimTrieCache should be minimally affected by this refactor.

[bvbfan]

You're right about of CClaimTrieCache, i wrote the comment before i see the implementation, name lies to me, did you think it's proper name?

[bvbfan]

I force push new version, but it looks like not updated here :)

[bvbfan]

I can't figure out why when i write to leveldb CClaimTrieNode is not empty, but when i reads it is. Did you have any advice? Test fails about that

void CCMap<K, V>::write(size_t, CClaimTrieDb*) [with K = std::basic_string<char>; V = CClaimTrieNode; size_t = long unsigned int], , 0
void CCMap<K, V>::write(size_t, CClaimTrieDb*) [with K = std::basic_string<char>; V = CClaimTrieNode; size_t = long unsigned int], t, 0
void CCMap<K, V>::write(size_t, CClaimTrieDb*) [with K = std::basic_string<char>; V = CClaimTrieNode; size_t = long unsigned int], te, 0
void CCMap<K, V>::write(size_t, CClaimTrieDb*) [with K = std::basic_string<char>; V = CClaimTrieNode; size_t = long unsigned int], tes, 0
void CCMap<K, V>::write(size_t, CClaimTrieDb*) [with K = std::basic_string<char>; V = CClaimTrieNode; size_t = long unsigned int], test, 0
bool CClaimTrieDb::SeekFirstKey(K&, V&) const [with K = std::basic_string<char>; V = CClaimTrieNode], , 1
bool CClaimTrieDb::SeekFirstKey(K&, V&) const [with K = std::basic_string<char>; V = CClaimTrieNode], t, 1
bool CClaimTrieDb::SeekFirstKey(K&, V&) const [with K = std::basic_string<char>; V = CClaimTrieNode], te, 1
bool CClaimTrieDb::SeekFirstKey(K&, V&) const [with K = std::basic_string<char>; V = CClaimTrieNode], tes, 1
bool CClaimTrieDb::SeekFirstKey(K&, V&) const [with K = std::basic_string<char>; V = CClaimTrieNode], test, 0

[bvbfan]

Ok, Kay, can you review it? The architecture aims to provide queues by needs, for now they are maps, through CClaimTrieDb. I plan to refactor CClaimTrieCache too to benefit db cache and to be renamed, as we discuss. One test still fail, somehow, i can't see this is my fault. Hope we can merge it in refactor branch after you explain your remarks. Regards

[bvbfan]

Did you have a time to take a look on this? After all i have one pending change, last implementation uses vector, under the hood in CClaimTrieDb, to not reorder cached queues.

[kaykurokawa]

Will start looking at this today

[bvbfan]

Kay i will change hash calculation since i check older version of boost, https://wandbox.org/ helped a lot, Boost.TypeIndex started at version 1.56, but the problem is around 1.64 it's change hash calculation, which is annoying. I will implement it in different way.

[kaykurokawa]

Updated doc strings in 53cf80d99c2577582396612215b03bb1307b5a57 , See branch: seperate_disk_claim

Regarding terminology, I tried to change things so that "queue" refers to a series of data that is used by CClaimTrie (a "queue" can be on disk or in memory). "buffer" refers to a set of changes to "queue" that is stored in memory. I removed mentions to "dirty" or "dirty queue" as I think its not descriptive enough.

[bvbfan]

It looks good to me.

[kaykurokawa]

I found a problem, try syncing lbrycrdd from scratch (clear out data directory before starting lbrycrdd). See comment above, should be a simple fix.

[bvbfan]

diff --git a/src/claimtriedb.cpp b/src/claimtriedb.cpp
index 0c0eccf7..198d6641 100644
--- a/src/claimtriedb.cpp
+++ b/src/claimtriedb.cpp
@@ -136,20 +136,17 @@ bool CClaimTrieDb::seekByKey(std::map<K, V, C> &map) const
     const size_t hash = hashType<K, V>();
     boost::scoped_ptr<CDBIterator> pcursor(const_cast<CClaimTrieDb*>(this)->NewIterator());

-    bool found = false;
-
     for (pcursor->SeekToFirst(); pcursor->Valid(); pcursor->Next()) {
         std::pair<size_t, K> key;
         if (pcursor->GetKey(key)) {
             if (hash == key.first) {
                 V value;
                 if (!pcursor->GetValue(value)) return false;
-                found = true;
                 map.insert(std::make_pair(key.second, value));
             }
         }
     }
-    return found;
+    return true;
 }

 template <typename K, typename V, typename C>

[bvbfan]

So maybe we should update test as well claimtriebranching_tests.cpp:319

BOOST_CHECK(pclaimTrie->ReadFromDisk(true));

[kaykurokawa]

Yes, these changes look good, also add a docstring to seekByKey()

"Returns false if database read fails."

Can you make these changes on your branch, Also, add the docstring commit 53cf80d99c2577582396612215b03bb1307b5a57 And than squash these commits (maybe one commit is ok?) to make it ready for merge to master.

[bvbfan]

Also notice that i have pending changes to that branch https://github.com/lbryio/lbrycrd/issues/145

[kaykurokawa]

Closing as further work is continued in #160