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

getclaimsintrie: insufficent info for sorting subclaims #196

Closed BrannonKing closed 5 years ago

BrannonKing commented 6 years ago

In working on the mass-import-to-regtest (#188), I discovered that nEffectiveAmount is not actually serialized with the claims. The number is set right for/when new claims come into the buffer, but when you start up a fresh launch and load everything off disk, the values are trash. We do return that value in rpc/getclaimsforname, but it is recalculated there before the return. rpc/getclaimsintrie returns all the claims for each claim name, but it lacks the details of the rpc/getclaimsforname method. Without the support info (or at least the effective amount), you can't make any judgment on the ordering of the claims returned from getclaimsintrie aside from "active is first". It's tempting to make getclaimsintrie call getclaimsforname on each name in the system, or make getclaimsforname take a wildcard and ditch the other method.

bvbfan commented 6 years ago

You mean nEffectiveAmount, right?

bvbfan commented 5 years ago

231

bvbfan commented 5 years ago

Note implementation in https://github.com/lbryio/lbrycrd/tree/claimsforname_value (it's last commit in). When someone has a time for it, can gives a try.

bvbfan commented 5 years ago

249

BrannonKing commented 5 years ago

Capturing some conversation on this:

Brannon [Today at 10:46 AM] I was going to remove the superfluous getEffectiveAmountForClaim method, but I guess I'll hold off on that a little longer as I discovered one legitimate use of it. Clarification: in the mem_improve branch, we're initializing the effective value as we load the trie. We also keep it up to date whenever we modify the trie. However, we don't keep it up to date in the CLAIM_BY_ID table. (When adding/removing supports, we don't add the affected claim into claimsToAdd.) This only affects us in the getclaimbyid RPC, and we correctly compensate for it at the moment. getclaimbyid is a little weird, though, it always returns data for the current block; there's no way to roll that one to a given time/block. I propose that we make these changes (but maybe not today). Tell me what you think:

  1. The CLAIM_BY_ID table should just hold the original name. Maybe do a 1-time table upgrade on startup.
  2. The getclaimbyid RPC should, after rolling to the correct block, look up the name by ID and then do what getclaimsforname does: pull the claim value from the trie.
  3. Eliminate the claimsToDelete collection; never delete anything from the CLAIM_BY_ID table since it doesn't track to block height.
  4. Proceed with the elimination of getEffectiveAmountForClaim.
  5. End with 100 lines less code, faster flushes, and general euphoria.

12 replies tzarebczan [1 hour ago] does this also help us get closer to https://github.com/lbryio/lbrycrd/issues/203 ?

bvbfan [1 hour ago] for getEffectiveAmountForClaim i was done a work on, it's in branch claimsforname_value commit https://github.com/lbryio/lbrycrd/commit/8e800df1d019152caab23211984343b7c6aa4181

tzarebczan it's done in branch pending_affective_amount (it has spell error on :slightly_smiling_face:)

https://github.com/lbryio/lbrycrd/commit/785ca8aeec459708c6db3b35aa5f1fdc6e9ef2e7 but we not consider that's needed

tzarebczan [1 hour ago] meaning what? Otherwise you can't tell what any other pending bids are for something. Users can't see their own effective amount during an outbid attempt.

bvbfan [42 minutes ago] meaning did you need effective amount and pending one at once? (edited)

tzarebczan [41 minutes ago] unless effective amount takes into account the state during a takeover, then yes, we need both. today it's 0.

bvbfan [36 minutes ago] i'm ok with that

Brannon [29 minutes ago] In looking at that bug, I think nEffectiveValue should never return zero. I think it should return the reservation + supports as they exist at the block specified in getclaimsforname.

In addition, I think that method should return stuff from the activation queues as well. It returns all of the valid-at-height info; it would be okay to return items that aren't valid yet.

If we were to add a "pending effective value", we would also need to add a block height value that went with it, unless it was just assumed to be the height of activation for the latest support that we know about.

tzarebczan [28 minutes ago] I;m okay with either approach, as long as there's an easy way to tell something is pending (i.e. not yet taken over). I think it was confusing at first to add it to effectiveamount since it wasn't "effective" in the sense that the vanity wasn't overtaken.

bvbfan [22 minutes ago] it returns 0, depend on validAtheight

Brannon [13 minutes ago] 0 is a valid effectiveValue for claims that are still in the activation queue, but I didn't think we were returning those at the moment.

BrannonKing commented 5 years ago

Fixed via https://github.com/lbryio/lbrycrd/pull/308