implemented hard fork to include nonwinning claims in hash #209

2018-10-03T19:46:56+02:00

BrannonKing commented

2018-10-03 19:46:56 +02:00

(Migrated from github.com)

It also uses Merkle trees for hashing children and claims post-fork. This significantly reduces the output for getnameproof.

Notable things to look at:

The way the tree is verified after it is loaded form disk (now using the Cache).
The place in the Cache's (Dis)ConnectBlock methods where we rehash the whole tree.
The use of the already-included MerkleHash and MerkleBranch methods.
The names in the getnameproof data and the Proof structs.

It also uses Merkle trees for hashing children and claims post-fork. This significantly reduces the output for `getnameproof`. Notable things to look at: 1. The way the tree is verified after it is loaded form disk (now using the Cache). 2. The place in the Cache's (Dis)ConnectBlock methods where we rehash the whole tree. 3. The use of the already-included MerkleHash and MerkleBranch methods. 4. The names in the getnameproof data and the Proof structs.

lbrynaut (Migrated from github.com) reviewed

2018-10-03 19:46:56 +02:00

bvbfan (Migrated from github.com) reviewed

2018-10-05 15:08:56 +02:00

src/claimtrie.cpp

					
				@ -1265,1 +1342,4 @@

				    cachedNode = cache.find("");

				    if (cachedNode != cache.end())

				        currentNode = cachedNode->second;

				    for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur)

bvbfan (Migrated from github.com) commented

2018-10-05 15:08:56 +02:00

stringstream is slowest C++ string manipulation class, better not use it in time expensive functions. You can use string + char which is faster.

bvbfan (Migrated from github.com) reviewed

2018-10-05 15:13:38 +02:00

src/claimtrie.cpp

					
				@ -1265,3 +1344,4 @@

				        currentNode = cachedNode->second;

				    for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur)

				    {

				        std::string sCurrentSubstring(name.begin(), itCur);

bvbfan (Migrated from github.com) commented

2018-10-05 15:13:38 +02:00

count look-up through whole container constant, better use find and compare against end.

bvbfan (Migrated from github.com) reviewed

2018-10-05 15:25:25 +02:00

src/claimtrie.h

bvbfan (Migrated from github.com) commented

2018-10-05 15:25:25 +02:00

You can make 2nd string parameter const ref, here and 4 line below.

BrannonKing (Migrated from github.com) reviewed

2018-10-05 16:35:00 +02:00

src/claimtrie.cpp

					
				@ -1265,1 +1342,4 @@

				    cachedNode = cache.find("");

				    if (cachedNode != cache.end())

				        currentNode = cachedNode->second;

				    for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur)

BrannonKing (Migrated from github.com) commented

2018-10-05 16:34:59 +02:00

Sorry, the stringstream was there in the existing method that I copied to start with. There are quite a few of those in the file, unfortunately. I completely agree with you that it's both slow and a horribly asinine way to convert a char to a string. I've been dragging my feet a little on completely refactoring the trie searches (aka, moving them all to a search method that all these other methods call); I've been waiting for the upstream merge to happen before doing some major refactoring on the trie. It would eliminate all stringstream calls.

BrannonKing (Migrated from github.com) reviewed

2018-10-05 16:35:46 +02:00

src/claimtrie.cpp

					
				@ -1265,3 +1344,4 @@

				        currentNode = cachedNode->second;

				    for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur)

				    {

				        std::string sCurrentSubstring(name.begin(), itCur);

BrannonKing (Migrated from github.com) commented

2018-10-05 16:35:46 +02:00

Again, both the stringstream and the count were in the previous method; I wasn't focused on those. If I do another round of changes on this story, I'll remove them though.

bvbfan (Migrated from github.com) reviewed

2018-10-06 17:40:18 +02:00

src/claimtrie.cpp

					
				@ -1265,3 +1344,4 @@

				        currentNode = cachedNode->second;

				    for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur)

				    {

				        std::string sCurrentSubstring(name.begin(), itCur);

bvbfan (Migrated from github.com) commented

2018-10-06 17:40:18 +02:00

Yeah, i saw that but it's good time to fix it.

BrannonKing commented

2018-10-11 20:44:18 +02:00

(Migrated from github.com)

CClaimTrieCache::insertClaimIntoTrie needs to be updated to detect if there were any ordering changes.

`CClaimTrieCache::insertClaimIntoTrie` needs to be updated to detect if there were any ordering changes.

kaykurokawa (Migrated from github.com) reviewed

2019-01-10 20:43:21 +01:00

src/claimtrie.cpp

kaykurokawa (Migrated from github.com) commented

2019-01-10 20:43:21 +01:00

I would like to see a better name for this function recursiveComputeMerkleHashBinaryTree or some comments/docstrings on what its supposed to do.

kaykurokawa commented

2019-01-10 21:13:35 +01:00

(Migrated from github.com)

We should clarify how the claims are ordered when hashed.

It looks to me like the winning claim would always be the first in position of the merkle tree ( that is good because it is still important to be able to prove that a claim is the winning one). It also looks like the non winning claims are sorted from highest effective amount to lowest effective amount? I would make sure that this is the case (are claims ever stored out of order ?) .

We should clarify how the claims are ordered when hashed. It looks to me like the winning claim would always be the first in position of the merkle tree ( that is good because it is still important to be able to prove that a claim is the winning one). It also looks like the non winning claims are sorted from highest effective amount to lowest effective amount? I would make sure that this is the case (are claims ever stored out of order ?) .

BrannonKing commented

2019-01-14 12:27:56 +01:00

(Migrated from github.com)

For the claim sorting, the code has been adjusted to do a full, stable sort instead of heapify. The sort operator is complete and has been for a while (see CClaimValue::operator<). However, I think there is a scenario where we might end up with out-of-order claims: when loading blocks that were created before the addition of the full sort. Claims aren't re-sorted on load. We have been discussing doing this, though, as we've been considering the right way to recreate the effective amount in this same scenario (which isn't serialized at present).

For the claim sorting, the code has been adjusted to do a full, stable sort instead of heapify. The sort operator is complete and has been for a while (see `CClaimValue::operator<`). However, I think there is a scenario where we might end up with out-of-order claims: when loading blocks that were created before the addition of the full sort. Claims aren't re-sorted on load. We have been discussing doing this, though, as we've been considering the right way to recreate the effective amount in this same scenario (which isn't serialized at present).

BrannonKing commented

2019-02-13 18:43:04 +01:00

(Migrated from github.com)

We've had new requirements on this. It now needs to implement all necessary support for this: https://www.notion.so/lbry/Q-A-on-RPC-results-verification-44888d23efa3475a90eec997f9bf3103 . It also needs to return the claim_sequence field in all claim RPC calls.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

lbrynaut

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbrycrd#209

Rows
Columns