search

lbryio / lbrycrd

The blockchain that provides the digital content namespace for the LBRY protocol
https://lbry.com
MIT License
2.57k stars 178 forks source link

added blockhash input to claimtrie RPC methods plus tests #189

Closed BrannonKing closed 5 years ago

BrannonKing commented 6 years ago

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.

BrannonKing commented 6 years ago

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

bvbfan commented 6 years ago

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 6 years ago

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 6 years ago

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 6 years ago

Rebase to be ready for merge.

kaykurokawa commented 6 years ago

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

kaykurokawa commented 6 years ago

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 6 years ago

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

bvbfan commented 6 years ago

Update help strings.

BrannonKing commented 6 years ago

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 5 years ago

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.