Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add capability to extend expiration time of pre fork claims #137

Merged
merged 2 commits into from May 24, 2018

Conversation

Projects
None yet
3 participants
@kaykurokawa
Copy link
Member

commented May 10, 2018

Details of hard fork for all:

The fork of the mainnet will be on block 400155 (will be around noon EST 7/9/18)

The fork of the testnet will be on block 280831 (will be around noon EST 5/21/18)

Claims made on and after the target hardfork blocks will expire after 2102400 blocks (approximately 10 years assuming block time 2.5 minutes).

Claims that were made before the hardfork and have not expired will have their expiration extended by the difference between the new expiration time and the previous expiration time (456 days).

Claims that already expired will remain expired.

Details for reviewers

This is further work off of : #115 in order to add the extension of claim expiration to already existing claims ( in #115 claims made before the hard fork would still expire at the old expiration time)

While working on this, I made two adjustments to #115. A) the first was that the new expiration time took effect 1 block after the target hard fork block, and not on the target hard fork block. It would be more intuitive if the new expiration time took effect on the target hard fork block. B) I fixed the logic of calling setExpirationTime() to only be called when we are at the target hard fork block, instead of on every block. This is more intuitive and in line with the soft-fork logic that is already present for Bitcoin. This means we need to also called setExpirationTime when we load the claimtrie from disk so I did that. The combination of A) and B) fixes a bug in #115 where if you loaded the claimtrie from disk (restarted lbrycrdd) than you expiration time for the first block after restart would be the old expiration time instead of the new one even after the hard fork.

The above work is on 2df3bb1

While working on extending the expiration of existing claims, I realized that #110 had the problem where it would not properly be able to remove expirations from the expiration queue properly. This is because the function removeFromExpirationQueue() : https://github.com/lbryio/lbrycrd/blob/master/src/claimtrie.cpp#L1675 was not adjusted to consider the hard fork change in expiration time hence this line: https://github.com/lbryio/lbrycrd/blob/master/src/claimtrie.cpp#L1677 could calculate the wrong expiration time for a claim that was created before the hard fork an expires after the hard fork.

The way we extend the claim is fairly straight forward. When we encounter the hard fork block, we set the new expiration time, and we look at all the entries in the expiration queue either in the dirty queue (where entries that are not saved to disk resides) or the disk database. The entries are adjusted to have their expiration time extended by the difference between the new expiration time and the old expiration time. Effectively, this makes it look like all the claims share the same new expiration time so the issue I described is solved.

The above work on extending the expiraiton of existing claims is on
b3aa62c

For the unit tests, I added a series of tests to see if supports behaved properly, and also added tests to see if the claimtrie behaved properly on the hard fork if I simulated a restart (write claimtrie to disk, clear it in memory, and than read it from disk). 6621f50

The other commits should be straight forward.

@kaykurokawa kaykurokawa force-pushed the claim-expiration-fixes branch from bc0caa6 to 6621f50 May 15, 2018

@kaykurokawa kaykurokawa changed the title [WIP] add capability to extend expiration time of pre fork claims Add capability to extend expiration time of pre fork claims May 15, 2018

@kaykurokawa kaykurokawa requested review from shyba and lbrynaut May 15, 2018

@lbry-bot lbry-bot assigned lbrynaut and shyba and unassigned kaykurokawa May 15, 2018

@@ -131,6 +131,9 @@ class CMainParams : public CChainParams {
consensus.powLimit = uint256S("0000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
consensus.nPowTargetTimespan = 150; //retarget every block
consensus.nPowTargetSpacing = 150;
consensus.nOriginalClaimExpirationTime = 262974;
consensus.nExtendedClaimExpirationTime = 2102400;
consensus.nExtendedClaimExpirationForkHeight = 400155; // FIXME: pick a real height

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

Remove FIXME comment

@@ -292,6 +298,9 @@ class CRegTestParams : public CChainParams {
consensus.powLimit = uint256S("7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
consensus.nPowTargetTimespan = 1;//14 * 24 * 60 * 60; // two weeks
consensus.nPowTargetSpacing = 1;
consensus.nOriginalClaimExpirationTime = 500;
consensus.nExtendedClaimExpirationTime = 600;
consensus.nExtendedClaimExpirationForkHeight = 800; // FIXME: pick a real height

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

Remove FIXME comment

}


//look through db for expiration queues, if we haven't already found it in dirty expiraiton queue

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

expiration is spelled incorrectly in several places, maybe do a quick search for them?

claimId = ClaimIdHash(hash, i);
LogPrintf("--- %s[%lu]: OP_CLAIM_NAME \"%s\" = \"%s\" with claimId %s and tx prevout %s at index %d\n",
__func__, view.AccessCoins(hash)->nHeight, name, SanitizeString(value),

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

I realize I initially used the view height, but we could replace with pindex->nHeight (in all places it's used in debug)

@@ -2220,6 +2232,13 @@ bool DisconnectBlock(const CBlock& block, CValidationState& state, const CBlockI
assert(trieCache.finalizeDecrement());
trieCache.setBestBlock(pindex->pprev->GetBlockHash());
assert(trieCache.getMerkleHash() == pindex->pprev->hashClaimTrie);
if (pindex->nHeight == Params().GetConsensus().nExtendedClaimExpirationForkHeight)
{
LogPrintf("Decremented past the fork v13 fork height");

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

What is v13 fork? Why not call it the claim expiration fork or something more intuitive?

@@ -18,12 +18,12 @@
#include <iostream>
#include "test/test_bitcoin.h"


This comment has been minimized.

Copy link
@lbrynaut
{
fRequireStandard = false;
BOOST_CHECK(pclaimTrie->nCurrentHeight == chainActive.Height() + 1);
LOCK(cs_main);
num_txs_for_next_block = 0;
num_txs = 0;
coinbase_txs_used = 0;



// generate coinbases to spend

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

Seems 1 line is enough?

void WriteClearReadClaimTrie()
{
// this will simulate restart of lbrycrdd by writing the claimtrie to disk,
// clearing the-in memory claimtrie, and then reading it the saved claimtrie

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

s/reading it/reading/

pclaimTrie->WriteToDisk();
pclaimTrie->clear();
pclaimTrie->ReadFromDisk(true);
}

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 15, 2018

Member

This also doesn't do exactly what's in the comment since the Cache isn't modified/updated/rebuilt.

This comment has been minimized.

Copy link
@kaykurokawa

kaykurokawa May 16, 2018

Author Member

What do you mean here exactly ? You may have misssed where WriteToDisk clears out the cache entries https://github.com/lbryio/lbrycrd/blob/master/src/claimtrie.cpp#L1047 here

This comment has been minimized.

Copy link
@lbrynaut

lbrynaut May 16, 2018

Member

I was referring to the state of the TrieCache.

@kaykurokawa

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

It was discovered that the testnet blocks haven't been progressing (mining was turned off) so I'll need to set a new fork height for the testnet.

@kaykurokawa

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

made fixes as reviewed by lbrynaut

@shyba

shyba approved these changes May 17, 2018

claimId = ClaimIdHash(hash, i);
LogPrintf("--- %s[%lu]: OP_CLAIM_NAME \"%s\" = \"%s\" with claimId %s and tx prevout %s at index %d\n",

This comment has been minimized.

Copy link
@shyba

shyba May 17, 2018

Member

maybe log this only if in debug mode? otherwise getnameproof can become verbose as it currently disconnects and reconnects blocks for generating previous block proofs.

This comment has been minimized.

Copy link
@kaykurokawa

kaykurokawa May 22, 2018

Author Member

is there a debugging log?

fixture.IncrementBlocks(fixture.expirationForkHeight - chainActive.Height());
BOOST_CHECK(is_best_claim("test2", u2));
BOOST_CHECK(best_claim_effective_amount_equals("test2",2));
// increment to original expiraiton, should not be expired

This comment has been minimized.

Copy link
@shyba

shyba May 17, 2018

Member

s/expiraiton/expiration/

This comment has been minimized.

Copy link
@shyba

shyba May 17, 2018

Member

oh, I see @lbrynaut already found that, sorry for the flood.

BOOST_CHECK_EQUAL(chainActive.Height(), fixture.expirationForkHeight);
BOOST_CHECK_EQUAL(pclaimTrie->nExpirationTime, fixture.extendedExpiration);
// make sure decrementing to before the fork height will apppropriately set back the
// expiration time to the original expiraiton time

This comment has been minimized.

Copy link
@shyba

shyba May 17, 2018

Member

s/expiraiton/expiration/

This comment has been minimized.

Copy link
@kaykurokawa

kaykurokawa May 22, 2018

Author Member

fixed

@kaykurokawa

This comment has been minimized.

Copy link
Member Author

commented May 22, 2018

Made fix as suggested by syba,
Adjusted fork height on testnet to 278160 (in about an hour from now) , and I will run this on testnet.
Maybe for debug logging comment by shyba, we can do another PR to perhaps improve/clean up different log messages?

@shyba

This comment has been minimized.

Copy link
Member

commented May 23, 2018

@kaykurokawa yes, I agree. Created an issue for logging here.

@kaykurokawa kaykurokawa force-pushed the claim-expiration-fixes branch from 01a222a to 338e7a3 May 23, 2018

lbrynaut and others added some commits Mar 21, 2018

Extend expiration of pre fork claims and supports. Adjust and add uni…
…t test for the extended expiration hardfork.

@kaykurokawa kaykurokawa force-pushed the claim-expiration-fixes branch from 338e7a3 to 99669e2 May 23, 2018

@kaykurokawa

This comment has been minimized.

Copy link
Member Author

commented May 23, 2018

Rebased to two commits, rebased to master

@kaykurokawa kaykurokawa merged commit 4e147cb into master May 24, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@lyoshenka lyoshenka deleted the claim-expiration-fixes branch May 30, 2018

tiger5226 added a commit to lbryio/chainquery that referenced this pull request Jul 9, 2018

tiger5226 added a commit to lbryio/chainquery that referenced this pull request Jul 9, 2018

tiger5226 added a commit to lbryio/lbry.tech that referenced this pull request Jul 18, 2018

@tiger5226 tiger5226 referenced this pull request Jul 18, 2018

Merged

Update lbry-claimtrie.md #129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.