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

added blockhash input to claimtrie RPC methods plus tests #189

Merged
BrannonKing merged 3 commits from add_minconf_to_rpc_try2 into master 2018-10-31 20:44:15 +01:00
Conversation 33 Commits 3 Files changed 7 +738 -315
BrannonKing commented 2018-08-11 01:15:53 +02:00 (Migrated from github.com)
Copy link

I'm marking this as WIP as I'm unsure on the approach and looking for feedback. You'll notice that getclaimsforname is done differently (more robustly) than the others. The others could be done like that one but not vice-versa, but I'm less sure on the consequences of that approach and the completeness of what's there.

I'm marking this as WIP as I'm unsure on the approach and looking for feedback. You'll notice that `getclaimsforname` is done differently (more robustly) than the others. The others could be done like that one but not vice-versa, but I'm less sure on the consequences of that approach and the completeness of what's there.
lbrynaut (Migrated from github.com) reviewed 2018-08-11 01:15:53 +02:00
BrannonKing commented 2018-08-16 01:28:55 +02:00 (Migrated from github.com)
Copy link

Apparently I need to add those minconf parameters to the client.cpp file for automatic CLI conversion to int. (I was testing them with unit tests rather than the CLI.)

Apparently I need to add those minconf parameters to the client.cpp file for automatic CLI conversion to int. (I was testing them with unit tests rather than the CLI.)
BrannonKing (Migrated from github.com) reviewed 2018-08-22 19:37:55 +02:00
src/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-08-22 19:37:55 +02:00
Copy link

This method existed for CClaimTrie but not for the "cache".

This method existed for CClaimTrie but not for the "cache".
BrannonKing (Migrated from github.com) reviewed 2018-08-22 19:38:33 +02:00
src/claimtrie.cpp
@ -2574,0 +2553,4 @@
else
recursiveFlattenTrie(str, it->second, nodes);
}
}
BrannonKing (Migrated from github.com) commented 2018-08-22 19:38:33 +02:00
Copy link

this method also existed for CClaimTrie but not for the cache; it was mostly duplicated here.

this method also existed for CClaimTrie but not for the cache; it was mostly duplicated here.
BrannonKing (Migrated from github.com) reviewed 2018-08-22 19:39:37 +02:00
src/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-08-22 19:39:37 +02:00
Copy link

I was making this follow the pattern of getInfoForName.

I was making this follow the pattern of `getInfoForName`.
BrannonKing (Migrated from github.com) reviewed 2018-08-22 19:41:25 +02:00
src/rpc/claimtrie.cpp
@ -27,3 +45,3 @@
{
if (fHelp || params.size() > 0)
if (fHelp || params.size() > 1)
throw std::runtime_error(
BrannonKing (Migrated from github.com) commented 2018-08-22 19:41:25 +02:00
Copy link

This method uses the approach of rolling back the caches to put us in a state where we were some number of blocks back. The caches are never flushed.

This method uses the approach of rolling back the caches to put us in a state where we were some number of blocks back. The caches are never flushed.
BrannonKing (Migrated from github.com) reviewed 2018-08-22 19:42:52 +02:00
src/rpc/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-08-22 19:42:52 +02:00
Copy link

This particular line is not using data from the caches. It's hitting the live claimTrie itself. It appears that passing in the maxHeight would give the same result as rolling back the main trie.

This particular line is not using data from the caches. It's hitting the live claimTrie itself. It appears that passing in the maxHeight would give the same result as rolling back the main trie.
BrannonKing (Migrated from github.com) reviewed 2018-08-22 19:48:17 +02:00
src/rpc/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-08-22 19:48:17 +02:00
Copy link

In this method we are actually rolling back the real blocks -- not just the caches. We hold the locks and disconnect some number of tips and then rely on our scope-exit handler to put everything back before we leave. I think it's a horrible approach, really, but didn't see a good way to do it otherwise in the current codebase. We lock the entire system for handling this method. That's so wrong. I think Roy's approach of storing all claimtrie (nodes that have ever existed) in LevelDB is much better -- the old nodes would still be there and you could just start a search at a certain time period -- we wouldn't ever have to lock; we could run numerous calls concurrently.

In this method we are actually rolling back the real blocks -- not just the caches. We hold the locks and disconnect some number of tips and then rely on our scope-exit handler to put everything back before we leave. I think it's a horrible approach, really, but didn't see a good way to do it otherwise in the current codebase. We lock the entire system for handling this method. That's so wrong. I think Roy's approach of storing all claimtrie (nodes that have ever existed) in LevelDB is much better -- the old nodes would still be there and you could just start a search at a certain time period -- we wouldn't ever have to lock; we could run numerous calls concurrently.
BrannonKing (Migrated from github.com) reviewed 2018-08-22 21:31:33 +02:00
src/rpc/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-08-22 21:31:33 +02:00
Copy link

There was no equivalent of getClaimsForName in the cache object, and it was not obvious how to achieve this method's "support" outputs using the data in the cache.

There was no equivalent of `getClaimsForName` in the cache object, and it was not obvious how to achieve this method's "support" outputs using the data in the cache.
lbrynaut (Migrated from github.com) reviewed 2018-08-22 22:56:40 +02:00
src/claimtrie.cpp
lbrynaut (Migrated from github.com) commented 2018-08-22 22:56:40 +02:00
Copy link

What is this supposed to do? It doesn't make sense to me.

What is this supposed to do? It doesn't make sense to me.
lbrynaut (Migrated from github.com) reviewed 2018-08-22 23:02:45 +02:00
src/rpc/claimtrie.cpp
lbrynaut (Migrated from github.com) commented 2018-08-22 23:02:44 +02:00
Copy link

All (current) claimtrie nodes are stored in leveldb. Storing all changes over time (while nice for analysis) isn't actually that practical of a feature to have. Eeven concurrent performance on lookups isn't needed for just about anything practical (other than analysis), where I'd argue a relational DB is just as well suited, if not better. Horrible or not, all persistent state changes of a blockchain occur via connecting and disconnecting blocks.

Now is all this needed for this particular issue? I'm not sure yet, haven't thought about it enough.

All (current) claimtrie nodes are stored in leveldb. Storing all changes over time (while nice for analysis) isn't actually that practical of a feature to have. Eeven concurrent performance on lookups isn't needed for just about anything practical (other than analysis), where I'd argue a relational DB is just as well suited, if not better. Horrible or not, all persistent state changes of a blockchain occur via connecting and disconnecting blocks. Now is all this needed for this particular issue? I'm not sure yet, haven't thought about it enough.
BrannonKing (Migrated from github.com) reviewed 2018-09-13 00:22:23 +02:00
src/rpc/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-09-13 00:22:23 +02:00
Copy link

After further discussion, we don't want to do any flushes in RPC calls. We need to move the getClaimsForName method to the cache object. It does already have a method to get support nodes, but it has a different name than that method on the claimtrie.

After further discussion, we don't want to do any flushes in RPC calls. We need to move the getClaimsForName method to the cache object. It does already have a method to get support nodes, but it has a different name than that method on the claimtrie.
bvbfan (Migrated from github.com) reviewed 2018-10-03 18:57:54 +02:00
src/claimtrie.cpp
bvbfan (Migrated from github.com) commented 2018-10-03 18:57:41 +02:00
Copy link

We need ability read cached claim, it makes sense when we use rollback feature.

We need ability read cached claim, it makes sense when we use rollback feature.
BrannonKing (Migrated from github.com) reviewed 2018-10-04 00:30:47 +02:00
src/claimtrie.cpp
BrannonKing (Migrated from github.com) commented 2018-10-04 00:23:37 +02:00
Copy link

use getNodeForName here?

use `getNodeForName` here?
bvbfan (Migrated from github.com) reviewed 2018-10-05 06:50:45 +02:00
src/claimtrie.cpp
bvbfan (Migrated from github.com) commented 2018-10-05 06:50:45 +02:00
Copy link

I've thinking about that, but getNodeForName fallback to base (CClaimTrie) when it's not found in cache.

I've thinking about that, but getNodeForName fallback to base (CClaimTrie) when it's not found in cache.
bvbfan commented 2018-10-22 20:48:31 +02:00 (Migrated from github.com)
Copy link

I plan to standardized JSON objects names, between function they have different names which is ugly and hard to parse in a script.
claimId
txId
txn
value
presentInTrie (in claim trie)
effectiveAmount
validAtHeight
and so on, first word starts with lower letter, second word, if it presents, starts with upper. Most help does not match returned JSON object, Any other suggestions?

I plan to standardized JSON objects names, between function they have different names which is ugly and hard to parse in a script. claimId txId txn value presentInTrie (in claim trie) effectiveAmount validAtHeight and so on, first word starts with lower letter, second word, if it presents, starts with upper. Most help does not match returned JSON object, Any other suggestions?
BrannonKing commented 2018-10-22 21:09:05 +02:00 (Migrated from github.com)
Copy link

I plan to standardized JSON objects names...

We need to do that as a separate pull request. That's a breaking change, whereas adding additional outputs and taking new optional inputs are not breaking changes. I agree that the current mismatches between methods are silly. Do you know where each RPC method is used in the LBRY stack? We should locate each call and write up an epic story that includes updating each method that is necessary.

> I plan to standardized JSON objects names... We need to do that as a separate pull request. That's a breaking change, whereas adding additional outputs and taking new optional inputs are not breaking changes. I agree that the current mismatches between methods are silly. Do you know where each RPC method is used in the LBRY stack? We should locate each call and write up an epic story that includes updating each method that is necessary.
bvbfan commented 2018-10-23 08:12:30 +02:00 (Migrated from github.com)
Copy link

We need to do that as a separate pull request. That's a breaking change, whereas adding additional outputs and taking new optional inputs are not breaking changes. I agree that the current mismatches between methods are silly.

I understand, you're right.

Do you know where each RPC method is used in the LBRY stack? We should locate each call and write up an epic story that includes updating each method that is necessary.

lbry.go, i think, but @shyba knows better.

> We need to do that as a separate pull request. That's a breaking change, whereas adding additional outputs and taking new optional inputs are not breaking changes. I agree that the current mismatches between methods are silly. I understand, you're right. >Do you know where each RPC method is used in the LBRY stack? We should locate each call and write up an epic story that includes updating each method that is necessary. lbry.go, i think, but @shyba knows better.
bvbfan commented 2018-10-23 08:46:14 +02:00 (Migrated from github.com)
Copy link

Rebase to be ready for merge.

Rebase to be ready for merge.
kaykurokawa (Migrated from github.com) reviewed 2018-10-23 20:38:37 +02:00
src/claimtrie.cpp
kaykurokawa (Migrated from github.com) commented 2018-10-23 20:38:37 +02:00
Copy link

What are all these changes to getEffectiveAmoutnForClaim for? Seems like an unrelated change?

What are all these changes to getEffectiveAmoutnForClaim for? Seems like an unrelated change?
kaykurokawa (Migrated from github.com) reviewed 2018-10-23 22:51:40 +02:00
src/rpc/claimtrie.cpp
@ -32,2 +50,3 @@
"Arguments:\n"
"None\n"
"1. \"blockhash\" (string, optional) get claims in the trie\n"
" at the block specified\n"
kaykurokawa (Migrated from github.com) commented 2018-10-23 22:51:40 +02:00
Copy link

Instead of "Block too deep to regenerate it" , how about just "Block is too deep"

Instead of "Block too deep to regenerate it" , how about just "Block is too deep"
kaykurokawa (Migrated from github.com) reviewed 2018-10-23 22:56:49 +02:00
src/rpc/claimtrie.cpp
@ -46,3 +68,4 @@
" \"value\" (string) the value of this claim\n"
" }\n"
" ]\n"
" }\n"
kaykurokawa (Migrated from github.com) commented 2018-10-23 22:56:49 +02:00
Copy link

instead of "the hash of the block which is the active in the chain. If none is given, the latest block will be used", how about:

"get claims in the trie at the block specified by this block hash. If none is given, the latest active block will be used"

instead of "the hash of the block which is the active in the chain. If none is given, the latest block will be used", how about: "get claims in the trie at the block specified by this block hash. If none is given, the latest active block will be used"
kaykurokawa (Migrated from github.com) reviewed 2018-10-23 22:57:47 +02:00
src/rpc/claimtrie.cpp
kaykurokawa (Migrated from github.com) commented 2018-10-23 22:57:47 +02:00
Copy link

how about:
"get the claim trie at the block specified by this block hash. If none is given, the latest active block will be used"

how about: "get the claim trie at the block specified by this block hash. If none is given, the latest active block will be used"
kaykurokawa (Migrated from github.com) reviewed 2018-10-23 22:58:25 +02:00
src/rpc/claimtrie.cpp
kaykurokawa (Migrated from github.com) commented 2018-10-23 22:58:25 +02:00
Copy link

how about "get the value associated with the name at the block specified by this block hash. If none is given, the latest active block will be used"

how about "get the value associated with the name at the block specified by this block hash. If none is given, the latest active block will be used"
kaykurokawa (Migrated from github.com) reviewed 2018-10-23 22:59:04 +02:00
src/rpc/claimtrie.cpp
kaykurokawa (Migrated from github.com) commented 2018-10-23 22:59:04 +02:00
Copy link

how about, "get claims for name at the block specified by this block hash. If none is given, the latest active block will be used"

how about, "get claims for name at the block specified by this block hash. If none is given, the latest active block will be used"
kaykurokawa commented 2018-10-24 03:33:43 +02:00 (Migrated from github.com)
Copy link

I added a commit for some unit tests for the new CClaimtrieCache functions in claimtriecache_tests.cpp, by no means extensive but I think it'd be good to have some coverage on those.

https://github.com/lbryio/lbrycrd/pull/189/commits/0159a57765154ca2a09df4fc3845546eb3908a00

I added a commit for some unit tests for the new CClaimtrieCache functions in claimtriecache_tests.cpp, by no means extensive but I think it'd be good to have some coverage on those. https://github.com/lbryio/lbrycrd/pull/189/commits/0159a57765154ca2a09df4fc3845546eb3908a00
👍 1
bvbfan (Migrated from github.com) reviewed 2018-10-24 07:04:02 +02:00
src/claimtrie.cpp
bvbfan (Migrated from github.com) commented 2018-10-24 07:04:02 +02:00
Copy link

When I made a getEffectiveAmountForClaim in CClaimTrieCache notice it has 2 functions in here, so i just want to match them in the 2 classes. I prefer pointer to indicate it's optional or not. In plus now it has overloaded function that takes claimsForNameType if you performed getClaimsForName before to prevent double look-up. Otherwise we end-up with 4 functions

getEffectiveAmountForClaim(const std::string& name, uint160 claimId)
getEffectiveAmountForClaim(const claimsForNameType& claims, uint160 claimId)
getEffectiveAmountForClaimWithSupports(const std::string& name, uint160 claimId)
getEffectiveAmountForClaimWithSupports(const claimsForNameType& claims, uint160 claimId)
When I made a getEffectiveAmountForClaim in CClaimTrieCache notice it has 2 functions in here, so i just want to match them in the 2 classes. I prefer pointer to indicate it's optional or not. In plus now it has overloaded function that takes claimsForNameType if you performed getClaimsForName before to prevent double look-up. Otherwise we end-up with 4 functions ``` getEffectiveAmountForClaim(const std::string& name, uint160 claimId) getEffectiveAmountForClaim(const claimsForNameType& claims, uint160 claimId) getEffectiveAmountForClaimWithSupports(const std::string& name, uint160 claimId) getEffectiveAmountForClaimWithSupports(const claimsForNameType& claims, uint160 claimId) ```
kaykurokawa commented 2018-10-24 17:42:57 +02:00 (Migrated from github.com)
Copy link

Fixed formatting on the test I added.
Also noting that this will affect calls in https://github.com/lbryio/lbryumx for lbryumx/session.py , this should be the only place where this PR will affect. I will open issue there

Fixed formatting on the test I added. Also noting that this will affect calls in https://github.com/lbryio/lbryumx for lbryumx/session.py , this should be the only place where this PR will affect. I will open issue there
BrannonKing commented 2018-10-24 17:49:42 +02:00 (Migrated from github.com)
Copy link

How will libryumx be affected? The intent was that all new parameters here were optional.

How will libryumx be affected? The intent was that all new parameters here were optional.
bvbfan commented 2018-10-24 20:42:14 +02:00 (Migrated from github.com)
Copy link

Update help strings.

Update help strings.
BrannonKing (Migrated from github.com) reviewed 2018-10-24 20:54:15 +02:00
src/rpc/claimtrie.cpp
@ -32,2 +50,3 @@
"Arguments:\n"
"None\n"
"1. \"blockhash\" (string, optional) get claims in the trie\n"
" at the block specified\n"
BrannonKing (Migrated from github.com) commented 2018-10-24 20:54:15 +02:00
Copy link

getnameproof says "Block too deep to generate proof"

getnameproof says "Block too deep to generate proof"
BrannonKing commented 2018-10-24 21:45:04 +02:00 (Migrated from github.com)
Copy link

I was just doing some testing by hand and seeing a problem:

  1. start up a clean regtest daemon (after removing ~/.lbrycrd/regtest)
  2. generate 200 blocks
  3. claim a new name
  4. generate 1 block
  5. get the block hash for block 190
  6. run getclaimsintrie passing in the hash for 190
  7. the results should be empty but they aren't

Update: it looks like this is an issue of us not using the "" (aka, root) node from the cache.

I was just doing some testing by hand and seeing a problem: 1. start up a clean regtest daemon (after removing ~/.lbrycrd/regtest) 2. generate 200 blocks 3. claim a new name 4. generate 1 block 5. get the block hash for block 190 6. run getclaimsintrie passing in the hash for 190 7. the results should be empty but they aren't **Update:** it looks like this is an issue of us not using the "" (aka, root) node from the cache.
kaykurokawa commented 2018-10-29 19:17:02 +01:00 (Migrated from github.com)
Copy link

Noting here that RPC command getclaimbyid does not have a blockhash argument in this PR .

I think it might be a bit involved to implement .. since we'll have to redo the getClaimById function in CClaimTrie ( it relies on leveldb and a cache, but the cache I think does not support decrementing multiple blocks at a time, just one block at at time) . We could probably open a new PR for this instead of doing this here.

Noting here that RPC command getclaimbyid does not have a blockhash argument in this PR . I think it might be a bit involved to implement .. since we'll have to redo the getClaimById function in CClaimTrie ( it relies on leveldb and a cache, but the cache I think does not support decrementing multiple blocks at a time, just one block at at time) . We could probably open a new PR for this instead of doing this here.
Sign in to join this conversation.
Reviewers
No reviewers
lbrynaut
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#189
No description provided.
Delete branch "add_minconf_to_rpc_try2"

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?