LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

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

Merged
kaykurokawa merged 2 commits from claim-expiration-fixes into master 2018-05-24 06:30:08 +02:00
Conversation 5 Commits 2 Files changed 7 +416 -15
kaykurokawa commented 2018-05-11 01:14:44 +02:00 (Migrated from github.com)
Copy link

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 : https://github.com/lbryio/lbrycrd/pull/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 2df3bb104b

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). 6621f50b98

The other commits should be straight forward.

**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 : https://github.com/lbryio/lbrycrd/pull/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 https://github.com/lbryio/lbrycrd/pull/137/commits/2df3bb104b29f5f1cd0615efbe33ab46bc1aca53 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). https://github.com/lbryio/lbrycrd/pull/137/commits/6621f50b985d49a5b5c4c09b32d871c58fa54121 The other commits should be straight forward.
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:34:44 +02:00
src/chainparams.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:34:44 +02:00
Copy link

Remove FIXME comment

Remove FIXME comment
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:34:54 +02:00
src/chainparams.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:34:54 +02:00
Copy link

Remove FIXME comment

Remove FIXME comment
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:37:16 +02:00
src/claimtrie.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:37:15 +02:00
Copy link

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

expiration is spelled incorrectly in several places, maybe do a quick search for them?
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:39:51 +02:00
src/main.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:39:51 +02:00
Copy link

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

I realize I initially used the view height, but we could replace with pindex->nHeight (in all places it's used in debug)
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:41:02 +02:00
src/main.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:41:02 +02:00
Copy link

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

What is v13 fork? Why not call it the claim expiration fork or something more intuitive?
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:41:28 +02:00
src/test/claimtriebranching_tests.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:41:28 +02:00
Copy link

?

?
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:41:55 +02:00
src/test/claimtriebranching_tests.cpp
@ -164,7 +163,16 @@ struct ClaimTrieChainFixture{
unsigned int num_txs;
lbrynaut (Migrated from github.com) commented 2018-05-15 19:41:55 +02:00
Copy link

Seems 1 line is enough?

Seems 1 line is enough?
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:42:34 +02:00
src/test/claimtriebranching_tests.cpp
@ -300,6 +308,17 @@ struct ClaimTrieChainFixture{
num_txs_for_next_block = 0;
}
lbrynaut (Migrated from github.com) commented 2018-05-15 19:42:34 +02:00
Copy link

s/reading it/reading/

s/reading it/reading/
lbrynaut (Migrated from github.com) reviewed 2018-05-15 19:43:26 +02:00
src/test/claimtriebranching_tests.cpp
lbrynaut (Migrated from github.com) commented 2018-05-15 19:43:25 +02:00
Copy link

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

This also doesn't do exactly what's in the comment since the Cache isn't modified/updated/rebuilt.
lbrynaut (Migrated from github.com) approved these changes 2018-05-15 19:44:41 +02:00
kaykurokawa commented 2018-05-16 18:02:38 +02:00 (Migrated from github.com)
Copy link

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.

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 (Migrated from github.com) reviewed 2018-05-16 21:27:09 +02:00
src/test/claimtriebranching_tests.cpp
kaykurokawa (Migrated from github.com) commented 2018-05-16 21:27:09 +02:00
Copy link

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

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
kaykurokawa commented 2018-05-16 21:28:31 +02:00 (Migrated from github.com)
Copy link

made fixes as reviewed by lbrynaut

made fixes as reviewed by lbrynaut
lbrynaut (Migrated from github.com) reviewed 2018-05-16 21:45:14 +02:00
src/test/claimtriebranching_tests.cpp
lbrynaut (Migrated from github.com) commented 2018-05-16 21:45:14 +02:00
Copy link

I was referring to the state of the TrieCache.

I was referring to the state of the TrieCache.
lbrynaut (Migrated from github.com) approved these changes 2018-05-16 22:28:02 +02:00
shyba (Migrated from github.com) approved these changes 2018-05-17 19:29:40 +02:00
src/main.cpp
@ -2174,1 +2174,4 @@
std::string name(vvchParams[0].begin(), vvchParams[0].end());
std::string value(vvchParams[1].begin(), vvchParams[1].end());
claimId = ClaimIdHash(hash, i);
LogPrintf("--- %s[%lu]: OP_CLAIM_NAME \"%s\" = \"%s\" with claimId %s and tx prevout %s at index %d\n",
shyba (Migrated from github.com) commented 2018-05-17 19:16:20 +02:00
Copy link

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.

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.
src/test/claimtriebranching_tests.cpp
shyba (Migrated from github.com) commented 2018-05-17 19:25:06 +02:00
Copy link

s/expiraiton/expiration/

s/expiraiton/expiration/
src/test/claimtriebranching_tests.cpp
@ -847,0 +898,4 @@
fixture.DecrementBlocks(1);
BOOST_CHECK_EQUAL(pclaimTrie->nExpirationTime, fixture.originalExpiration);
fixture.IncrementBlocks(1);
shyba (Migrated from github.com) commented 2018-05-17 19:25:21 +02:00
Copy link

s/expiraiton/expiration/

s/expiraiton/expiration/
shyba (Migrated from github.com) reviewed 2018-05-17 19:30:51 +02:00
src/test/claimtriebranching_tests.cpp
shyba (Migrated from github.com) commented 2018-05-17 19:30:51 +02:00
Copy link

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

oh, I see @lbrynaut already found that, sorry for the flood.
kaykurokawa (Migrated from github.com) reviewed 2018-05-22 03:42:55 +02:00
src/main.cpp
@ -2174,1 +2174,4 @@
std::string name(vvchParams[0].begin(), vvchParams[0].end());
std::string value(vvchParams[1].begin(), vvchParams[1].end());
claimId = ClaimIdHash(hash, i);
LogPrintf("--- %s[%lu]: OP_CLAIM_NAME \"%s\" = \"%s\" with claimId %s and tx prevout %s at index %d\n",
kaykurokawa (Migrated from github.com) commented 2018-05-22 03:42:55 +02:00
Copy link

is there a debugging log?

is there a debugging log?
kaykurokawa (Migrated from github.com) reviewed 2018-05-22 03:43:03 +02:00
src/test/claimtriebranching_tests.cpp
@ -847,0 +898,4 @@
fixture.DecrementBlocks(1);
BOOST_CHECK_EQUAL(pclaimTrie->nExpirationTime, fixture.originalExpiration);
fixture.IncrementBlocks(1);
kaykurokawa (Migrated from github.com) commented 2018-05-22 03:43:02 +02:00
Copy link

fixed

fixed
kaykurokawa commented 2018-05-22 03:46:01 +02:00 (Migrated from github.com)
Copy link

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?

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 commented 2018-05-23 05:54:26 +02:00 (Migrated from github.com)
Copy link

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

@kaykurokawa yes, I agree. Created an [issue for logging here](https://github.com/lbryio/lbrycrd/issues/144).
kaykurokawa commented 2018-05-23 21:07:46 +02:00 (Migrated from github.com)
Copy link

Rebased to two commits, rebased to master

Rebased to two commits, rebased to master
Sign in to join this conversation.
Reviewers
No reviewers
lbrynaut
shyba
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
Epic

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
hard fork

   
help wanted

   
icebox

Long-term storage

  
Invalid

   
level: 0

   
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
soft fork

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

  
work in progress

The proposed solution is still incomplete.

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
Epic
good first issue
hacktoberfest
hard fork
help wanted
icebox
Invalid
level: 0
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
soft fork
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
work in progress
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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#137
No description provided.
Delete branch "claim-expiration-fixes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?