Fix incorrect usage of the claim index #197

Merged
lbrynaut merged 1 commit from claimindex-fix into master 2018-09-03 18:25:16 +02:00
3 changed files with 132 additions and 20 deletions

View file

@ -27,7 +27,7 @@ uint256 getValueHash(COutPoint outPoint, int nHeightOfLastTakeover)
txHasher.Write(outPoint.hash.begin(), outPoint.hash.size()); txHasher.Write(outPoint.hash.begin(), outPoint.hash.size());
std::vector<unsigned char> vchtxHash(txHasher.OUTPUT_SIZE); std::vector<unsigned char> vchtxHash(txHasher.OUTPUT_SIZE);
txHasher.Finalize(&(vchtxHash[0])); txHasher.Finalize(&(vchtxHash[0]));
CHash256 nOutHasher; CHash256 nOutHasher;
std::stringstream ss; std::stringstream ss;
ss << outPoint.n; ss << outPoint.n;
@ -48,7 +48,7 @@ uint256 getValueHash(COutPoint outPoint, int nHeightOfLastTakeover)
hasher.Write(vchTakeoverHash.data(), vchTakeoverHash.size()); hasher.Write(vchTakeoverHash.data(), vchTakeoverHash.size());
std::vector<unsigned char> vchHash(hasher.OUTPUT_SIZE); std::vector<unsigned char> vchHash(hasher.OUTPUT_SIZE);
hasher.Finalize(&(vchHash[0])); hasher.Finalize(&(vchHash[0]));
uint256 valueHash(vchHash); uint256 valueHash(vchHash);
return valueHash; return valueHash;
} }
@ -116,7 +116,7 @@ bool CClaimTrieNode::haveClaim(const COutPoint& outPoint) const
void CClaimTrieNode::reorderClaims(supportMapEntryType& supports) void CClaimTrieNode::reorderClaims(supportMapEntryType& supports)
{ {
std::vector<CClaimValue>::iterator itclaim; std::vector<CClaimValue>::iterator itclaim;
for (itclaim = claims.begin(); itclaim != claims.end(); ++itclaim) for (itclaim = claims.begin(); itclaim != claims.end(); ++itclaim)
{ {
itclaim->nEffectiveAmount = itclaim->nAmount; itclaim->nEffectiveAmount = itclaim->nAmount;
@ -133,7 +133,7 @@ void CClaimTrieNode::reorderClaims(supportMapEntryType& supports)
} }
} }
} }
std::make_heap(claims.begin(), claims.end()); std::make_heap(claims.begin(), claims.end());
} }
@ -606,12 +606,12 @@ void CClaimTrie::addToClaimIndex(const std::string& name, const CClaimValue& cla
{ {
CClaimIndexElement element = { name, claim }; CClaimIndexElement element = { name, claim };
LogPrintf("%s: ClaimIndex[%s] updated %s\n", __func__, claim.claimId.GetHex(), name); LogPrintf("%s: ClaimIndex[%s] updated %s\n", __func__, claim.claimId.GetHex(), name);
db.Write(std::make_pair(CLAIM_BY_ID, claim.claimId), element); db.Write(std::make_pair(CLAIM_BY_ID, claim.claimId), element);
} }
void CClaimTrie::removeFromClaimIndex(const CClaimValue& claim) void CClaimTrie::removeFromClaimIndex(const CClaimValue& claim)
{ {
LogPrintf("%s: ClaimIndex[%s] removed\n", __func__, claim.claimId.GetHex());
db.Erase(std::make_pair(CLAIM_BY_ID, claim.claimId)); db.Erase(std::make_pair(CLAIM_BY_ID, claim.claimId));
} }
@ -620,9 +620,14 @@ bool CClaimTrie::getClaimById(const uint160 claimId, std::string& name, CClaimVa
CClaimIndexElement element; CClaimIndexElement element;
if (db.Read(std::make_pair(CLAIM_BY_ID, claimId), element)) if (db.Read(std::make_pair(CLAIM_BY_ID, claimId), element))
{ {
name = element.name; if (element.claim.claimId == claimId) {
claim = element.claim; name = element.name;
return true; claim = element.claim;
return true;
} else {
LogPrintf("%s: ClaimIndex[%s] returned unmatched claimId %s when looking for %s\n",
__func__, claimId.GetHex(), element.claim.claimId.GetHex(), name);
}
} }
return false; return false;
} }
@ -1357,7 +1362,6 @@ bool CClaimTrieCache::insertClaimIntoTrie(const std::string& name, CClaimValue c
{ {
fChanged = true; fChanged = true;
currentNode->insertClaim(claim); currentNode->insertClaim(claim);
BrannonKing commented 2018-08-24 01:00:28 +02:00 (Migrated from github.com)
Review

Is this working around a problem in the base class? Why not fix it there?

Is this working around a problem in the base class? Why not fix it there?
lbrynaut commented 2018-08-24 02:33:23 +02:00 (Migrated from github.com)
Review

No.

No.
base->addToClaimIndex(name, claim);
} }
else else
{ {
@ -1369,6 +1373,7 @@ bool CClaimTrieCache::insertClaimIntoTrie(const std::string& name, CClaimValue c
if (currentTop != currentNode->claims.front()) if (currentTop != currentNode->claims.front())
fChanged = true; fChanged = true;
} }
if (fChanged) if (fChanged)
{ {
for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur) for (std::string::const_iterator itCur = name.begin(); itCur != name.end(); ++itCur)
@ -1432,8 +1437,6 @@ bool CClaimTrieCache::removeClaimFromTrie(const std::string& name, const COutPoi
CClaimValue currentTop = currentNode->claims.front(); CClaimValue currentTop = currentNode->claims.front();
success = currentNode->removeClaim(outPoint, claim); success = currentNode->removeClaim(outPoint, claim);
base->removeFromClaimIndex(claim);
if (!currentNode->claims.empty()) if (!currentNode->claims.empty())
{ {
supportMapEntryType node; supportMapEntryType node;
@ -1444,7 +1447,7 @@ bool CClaimTrieCache::removeClaimFromTrie(const std::string& name, const COutPoi
} }
else else
fChanged = true; fChanged = true;
if (!success) if (!success)
{ {
LogPrintf("%s: Removing a claim was unsuccessful. name = %s, txhash = %s, nOut = %d", __func__, name.c_str(), outPoint.hash.GetHex(), outPoint.n); LogPrintf("%s: Removing a claim was unsuccessful. name = %s, txhash = %s, nOut = %d", __func__, name.c_str(), outPoint.hash.GetHex(), outPoint.n);
@ -1612,6 +1615,8 @@ bool CClaimTrieCache::undoSpendClaim(const std::string& name, const COutPoint& o
{ {
nameOutPointType entry(name, claim.outPoint); nameOutPointType entry(name, claim.outPoint);
addToExpirationQueue(claim.nHeight + base->nExpirationTime, entry); addToExpirationQueue(claim.nHeight + base->nExpirationTime, entry);
CClaimIndexElement element = {name, claim};
claimsToAdd.push_back(element);
return insertClaimIntoTrie(name, claim, false); return insertClaimIntoTrie(name, claim, false);
} }
else else
@ -1630,7 +1635,8 @@ bool CClaimTrieCache::addClaimToQueues(const std::string& name, CClaimValue& cla
itQueueNameRow->second.push_back(outPointHeightType(claim.outPoint, claim.nValidAtHeight)); itQueueNameRow->second.push_back(outPointHeightType(claim.outPoint, claim.nValidAtHeight));
nameOutPointType expireEntry(name, claim.outPoint); nameOutPointType expireEntry(name, claim.outPoint);
addToExpirationQueue(claim.nHeight + base->nExpirationTime, expireEntry); addToExpirationQueue(claim.nHeight + base->nExpirationTime, expireEntry);
base->addToClaimIndex(name, claim); CClaimIndexElement element = {name, claim};
claimsToAdd.push_back(element);
return true; return true;
} }
@ -1669,8 +1675,6 @@ bool CClaimTrieCache::removeClaimFromQueue(const std::string& name, const COutPo
std::swap(claim, itQueue->second); std::swap(claim, itQueue->second);
itQueueNameRow->second.erase(itQueueName); itQueueNameRow->second.erase(itQueueName);
itQueueRow->second.erase(itQueue); itQueueRow->second.erase(itQueue);
base->removeFromClaimIndex(claim);
return true; return true;
} }
} }
@ -1707,6 +1711,7 @@ bool CClaimTrieCache::removeClaim(const std::string& name, const COutPoint& outP
nValidAtHeight = claim.nValidAtHeight; nValidAtHeight = claim.nValidAtHeight;
int expirationHeight = nHeight + base->nExpirationTime; int expirationHeight = nHeight + base->nExpirationTime;
removeFromExpirationQueue(name, outPoint, expirationHeight); removeFromExpirationQueue(name, outPoint, expirationHeight);
claimsToDelete.insert(claim);
} }
return removed; return removed;
} }
@ -2154,6 +2159,7 @@ bool CClaimTrieCache::incrementBlock(insertUndoType& insertUndo, claimQueueRowTy
{ {
CClaimValue claim; CClaimValue claim;
assert(removeClaimFromTrie(itEntry->name, itEntry->outPoint, claim, true)); assert(removeClaimFromTrie(itEntry->name, itEntry->outPoint, claim, true));
claimsToDelete.insert(claim);
expireUndo.push_back(std::make_pair(itEntry->name, claim)); expireUndo.push_back(std::make_pair(itEntry->name, claim));
LogPrintf("Expiring claim %s: %s, nHeight: %d, nValidAtHeight: %d\n", claim.claimId.GetHex(), itEntry->name, claim.nHeight, claim.nValidAtHeight); LogPrintf("Expiring claim %s: %s, nHeight: %d, nValidAtHeight: %d\n", claim.claimId.GetHex(), itEntry->name, claim.nHeight, claim.nValidAtHeight);
} }
@ -2345,7 +2351,7 @@ bool CClaimTrieCache::incrementBlock(insertUndoType& insertUndo, claimQueueRowTy
// remove all supports from the queue for that name // remove all supports from the queue for that name
itSupportQueueNameRow->second.clear(); itSupportQueueNameRow->second.clear();
} }
// save the old last height so that it can be restored if the block is undone // save the old last height so that it can be restored if the block is undone
if (haveClaimInTrie) if (haveClaimInTrie)
{ {
@ -2378,7 +2384,7 @@ bool CClaimTrieCache::decrementBlock(insertUndoType& insertUndo, claimQueueRowTy
{ {
LogPrintf("%s: nCurrentHeight (before decrement): %d\n", __func__, nCurrentHeight); LogPrintf("%s: nCurrentHeight (before decrement): %d\n", __func__, nCurrentHeight);
nCurrentHeight--; nCurrentHeight--;
if (expireSupportUndo.begin() != expireSupportUndo.end()) if (expireSupportUndo.begin() != expireSupportUndo.end())
{ {
expirationQueueType::iterator itSupportExpireRow = getSupportExpirationQueueCacheRow(nCurrentHeight, true); expirationQueueType::iterator itSupportExpireRow = getSupportExpirationQueueCacheRow(nCurrentHeight, true);
@ -2388,7 +2394,7 @@ bool CClaimTrieCache::decrementBlock(insertUndoType& insertUndo, claimQueueRowTy
itSupportExpireRow->second.push_back(nameOutPointType(itSupportExpireUndo->first, itSupportExpireUndo->second.outPoint)); itSupportExpireRow->second.push_back(nameOutPointType(itSupportExpireUndo->first, itSupportExpireUndo->second.outPoint));
} }
} }
for (insertUndoType::iterator itSupportUndo = insertSupportUndo.begin(); itSupportUndo != insertSupportUndo.end(); ++itSupportUndo) for (insertUndoType::iterator itSupportUndo = insertSupportUndo.begin(); itSupportUndo != insertSupportUndo.end(); ++itSupportUndo)
{ {
supportQueueType::iterator itSupportRow = getSupportQueueCacheRow(itSupportUndo->nHeight, true); supportQueueType::iterator itSupportRow = getSupportQueueCacheRow(itSupportUndo->nHeight, true);
@ -2398,13 +2404,15 @@ bool CClaimTrieCache::decrementBlock(insertUndoType& insertUndo, claimQueueRowTy
itSupportRow->second.push_back(std::make_pair(itSupportUndo->name, support)); itSupportRow->second.push_back(std::make_pair(itSupportUndo->name, support));
itSupportNameRow->second.push_back(outPointHeightType(support.outPoint, support.nValidAtHeight)); itSupportNameRow->second.push_back(outPointHeightType(support.outPoint, support.nValidAtHeight));
} }
if (expireUndo.begin() != expireUndo.end()) if (expireUndo.begin() != expireUndo.end())
{ {
expirationQueueType::iterator itExpireRow = getExpirationQueueCacheRow(nCurrentHeight, true); expirationQueueType::iterator itExpireRow = getExpirationQueueCacheRow(nCurrentHeight, true);
for (claimQueueRowType::iterator itExpireUndo = expireUndo.begin(); itExpireUndo != expireUndo.end(); ++itExpireUndo) for (claimQueueRowType::iterator itExpireUndo = expireUndo.begin(); itExpireUndo != expireUndo.end(); ++itExpireUndo)
{ {
insertClaimIntoTrie(itExpireUndo->first, itExpireUndo->second, false); insertClaimIntoTrie(itExpireUndo->first, itExpireUndo->second, false);
CClaimIndexElement element = {itExpireUndo->first, itExpireUndo->second};
claimsToAdd.push_back(element);
itExpireRow->second.push_back(nameOutPointType(itExpireUndo->first, itExpireUndo->second.outPoint)); itExpireRow->second.push_back(nameOutPointType(itExpireUndo->first, itExpireUndo->second.outPoint));
} }
} }
@ -2418,7 +2426,7 @@ bool CClaimTrieCache::decrementBlock(insertUndoType& insertUndo, claimQueueRowTy
itQueueRow->second.push_back(std::make_pair(itInsertUndo->name, claim)); itQueueRow->second.push_back(std::make_pair(itInsertUndo->name, claim));
itQueueNameRow->second.push_back(outPointHeightType(itInsertUndo->outPoint, itInsertUndo->nHeight)); itQueueNameRow->second.push_back(outPointHeightType(itInsertUndo->outPoint, itInsertUndo->nHeight));
} }
for (std::vector<std::pair<std::string, int> >::iterator itTakeoverHeightUndo = takeoverHeightUndo.begin(); itTakeoverHeightUndo != takeoverHeightUndo.end(); ++itTakeoverHeightUndo) for (std::vector<std::pair<std::string, int> >::iterator itTakeoverHeightUndo = takeoverHeightUndo.begin(); itTakeoverHeightUndo != takeoverHeightUndo.end(); ++itTakeoverHeightUndo)
{ {
cacheTakeoverHeights[itTakeoverHeightUndo->first] = itTakeoverHeightUndo->second; cacheTakeoverHeights[itTakeoverHeightUndo->first] = itTakeoverHeightUndo->second;
@ -2531,6 +2539,18 @@ bool CClaimTrieCache::flush()
{ {
if (dirty()) if (dirty())
getMerkleHash(); getMerkleHash();
if (!claimsToDelete.empty()) {
for (claimIndexClaimListType::iterator it = claimsToDelete.begin(); it != claimsToDelete.end(); ++it)
base->removeFromClaimIndex(*it);
claimsToDelete.clear();
}
if (!claimsToAdd.empty()) {
for (claimIndexElementListType::iterator it = claimsToAdd.begin(); it != claimsToAdd.end(); ++it)
base->addToClaimIndex(it->name, it->claim);
claimsToAdd.clear();
}
bool success = base->update(cache, cacheHashes, cacheTakeoverHeights, getBestBlock(), claimQueueCache, claimQueueNameCache, expirationQueueCache, nCurrentHeight, supportCache, supportQueueCache, supportQueueNameCache, supportExpirationQueueCache); bool success = base->update(cache, cacheHashes, cacheTakeoverHeights, getBestBlock(), claimQueueCache, claimQueueNameCache, expirationQueueCache, nCurrentHeight, supportCache, supportQueueCache, supportQueueNameCache, supportExpirationQueueCache);
if (success) if (success)
{ {

View file

@ -283,6 +283,9 @@ typedef std::map<std::string, CClaimTrieNode*, nodenamecompare> nodeCacheType;
typedef std::map<std::string, uint256> hashMapType; typedef std::map<std::string, uint256> hashMapType;
typedef std::set<CClaimValue> claimIndexClaimListType;
typedef std::vector<CClaimIndexElement> claimIndexElementListType;
struct claimsForNameType struct claimsForNameType
{ {
std::vector<CClaimValue> claims; std::vector<CClaimValue> claims;
@ -546,6 +549,8 @@ protected:
mutable std::map<std::string, int> cacheTakeoverHeights; mutable std::map<std::string, int> cacheTakeoverHeights;
mutable int nCurrentHeight; // Height of the block that is being worked on, which is mutable int nCurrentHeight; // Height of the block that is being worked on, which is
// one greater than the height of the chain's tip // one greater than the height of the chain's tip
mutable claimIndexElementListType claimsToAdd;
mutable claimIndexClaimListType claimsToDelete;
uint256 hashBlock; uint256 hashBlock;

View file

@ -3032,5 +3032,92 @@ BOOST_AUTO_TEST_CASE(bogus_claimtrie_hash_test)
delete pblockTemp; delete pblockTemp;
} }
/*
* tests for CClaimTrie::getClaimById basic consistency checks
*/
BOOST_AUTO_TEST_CASE(get_claim_by_id_test_2)
{
ClaimTrieChainFixture fixture;
const std::string name = "test";
CMutableTransaction tx1 = fixture.MakeClaim(fixture.GetCoinbase(), name, "one", 1);
uint160 claimId = ClaimIdHash(tx1.GetHash(), 0);
CMutableTransaction txx = fixture.MakeClaim(fixture.GetCoinbase(), name, "one", 1);
uint160 claimIdx = ClaimIdHash(txx.GetHash(), 0);
fixture.IncrementBlocks(1);
CClaimValue claimValue;
std::string claimName;
pclaimTrie->getClaimById(claimId, claimName, claimValue);
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId);
CMutableTransaction txa = fixture.Spend(tx1);
CMutableTransaction txb = fixture.Spend(txx);
fixture.IncrementBlocks(1);
BOOST_CHECK(!pclaimTrie->getClaimById(claimId, claimName, claimValue));
BOOST_CHECK(!pclaimTrie->getClaimById(claimIdx, claimName, claimValue));
fixture.DecrementBlocks(1);
pclaimTrie->getClaimById(claimId, claimName, claimValue);
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId);
// this test fails
pclaimTrie->getClaimById(claimIdx, claimName, claimValue);
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimIdx);
}
BOOST_AUTO_TEST_CASE(get_claim_by_id_test_3)
{
ClaimTrieChainFixture fixture;
pclaimTrie->setExpirationTime(5);
const std::string name = "test";
CMutableTransaction tx1 = fixture.MakeClaim(fixture.GetCoinbase(), name, "one", 1);
uint160 claimId = ClaimIdHash(tx1.GetHash(), 0);
fixture.IncrementBlocks(1);
CClaimValue claimValue;
std::string claimName;
pclaimTrie->getClaimById(claimId, claimName, claimValue);
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId);
// make second claim with activation delay 1
CMutableTransaction tx2 = fixture.MakeClaim(fixture.GetCoinbase(), name, "one", 2);
uint160 claimId2 = ClaimIdHash(tx2.GetHash(), 0);
fixture.IncrementBlocks(1);
// second claim is not activated yet, but can still access by claim id
BOOST_CHECK(is_best_claim(name, tx1));
BOOST_CHECK(pclaimTrie->getClaimById(claimId2, claimName, claimValue));
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId2);
fixture.IncrementBlocks(1);
// second claim has activated
BOOST_CHECK(is_best_claim(name, tx2));
BOOST_CHECK(pclaimTrie->getClaimById(claimId2, claimName, claimValue));
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId2);
fixture.DecrementBlocks(1);
// second claim has been deactivated via decrement
// should still be accesible via claim id
BOOST_CHECK(is_best_claim(name, tx1));
BOOST_CHECK(pclaimTrie->getClaimById(claimId2, claimName, claimValue));
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId2);
fixture.IncrementBlocks(1);
// second claim should have been re activated via increment
BOOST_CHECK(is_best_claim(name, tx2));
BOOST_CHECK(pclaimTrie->getClaimById(claimId2, claimName, claimValue));
BOOST_CHECK(claimName == name);
BOOST_CHECK(claimValue.claimId == claimId2);
}
BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE_END()