kaykurokawa
commented
6 years ago I think the original issue was poorly specified after thinking through this, my mistake.
There's a bit of a problem here because the "state of the claim trie" (or the winning claim) is dependent on multiple transaction, not just the singular claim. In this implementation, the age of supports is disregarded which means that if you run getvalueforname with the minconf parameter, the result will be unexpected. For example:
1) you make claim A for name "test", value 1, at block 100, 2) make claim B for name "test", value 2 at block 103, 3) support claim A for value 3 at block 106, 4) if you call getvalueforname at block 106 for name "test" with minconf 2 you will get claim A even though the support has not met the minconf requirement.
Another example is with abandons
1) make claim A for name "test", value 1 at block 100 2) make claim B for name "test", value 2 at block 101 3) abandon claim A at block 103 4) if you call getvalueforname at block 104 with minconf 2, you will get claim B although the abandon on block 103 hasn't met the minimum confirmation requirement.
So the only way to actually get the proper "state of the claim trie" is to roll back the blockchain to the proper block (i.e. if minconf is 2, roll back the blockchain 1 block) This is done in the command getnameproof for example using the argument blockhash to select the block to roll back to: https://github.com/lbryio/lbrycrd/blob/master/src/rpc/claimtrie.cpp#L693
Out of the RPC commands affected here, maybe "getclaimsintrie" is the only one where this implementation of minconf might be appropriate because this command only deals with the listing of individual claims and not the state of the claimtrie.
"getclaimsforname" lists out supports associated with claims , "getvalueforname" gives you the winning claim, and "getclaimtrie" gives you the entire claimtrie so these three commands all deal with the state of the claimtrie. In these commands, this implementation would give misleading results. To get the "proper" results, you'd actually need to roll back the blockchain to the desired location instead of manipulating the state at the current block.
for #44 . I was unsure who should handle the current height; right now the RPC commands themselves query that. (Some of them did that before.)
I also have no way to tell if the sort call negatively affects performance (vs make_heap). My guess is that we don't have enough claim competition yet to tell a difference in even our largest dataset.