Codecov Report

Merging #5865 (9fba99d) into master (03a6fb2) will decrease coverage by 0.20%.
The diff coverage is 70.76%.

@@            Coverage Diff             @@
##           master    #5865      +/-   ##
==========================================
- Coverage   60.08%   59.88%   -0.21%     
==========================================
  Files         269      269              
  Lines       24499    24527      +28     
==========================================
- Hits        14721    14687      -34     
- Misses       8193     8237      +44     
- Partials     1585     1603      +18     

The height key is, in a way, a type of event key with a defined composite prefix of "tx.height" and a value of it's height (hence why we have height repeated twice - once as a string and another as an int64). All other event keys are chosen by the application and could have for example a composite prefix of "account.name" and a value of "Callum". I was thinking that because tendermint already reserves the composite prefix "tx.height" that we want to check that the application doesn't also use it as an event.

Other than that, the only place where keys could get shuffled together is with the hash key which has no prefix to it. For example if the hash of a tx just so happened to start with "tx.height" it would get inserted somewhere amongst the tx.height section. I was thinking we could add a "tx.hash" prefix to the hash key and reserve this composite prefix as well.

The height key is, in a way, a type of event key with a defined composite prefix of "tx.height" and a value of it's height (hence why we have height repeated twice - once as a string and another as an int64).

Got it. Any reason for handling it as if it was some other key then? Can't we just have one key type?

Also, storing it as a string prevents us from doing range scans on it. Not sure if that's relevant, just saying (e.g. if someone queries tx.height < 10 we'll have to scan all heights rather than just the ones below 10).

the only place where keys could get shuffled together is with the hash key which has no prefix to it. For example if the hash of a tx just so happened to start with "tx.height" it would get inserted somewhere amongst the tx.height section. I was thinking we could add a "tx.hash" prefix to the hash key and reserve this composite prefix as well.

That might make sense. In that case, shouldn't this be reserved, specified, and handled in the same place as e.g. tx.height is? Which I'm presuming is outside of the kv package?

Can't we just have one key type?

I've sort of already made it one key. There is a function called key() that makes keys of the following format string/string/int64/uint32. Both the keyForEvent and keyForHeight are utility wrapper functions that generate keys of this format.

I guess the problem with combining height with the event key and forcing it to have the same format is, as you say, at the loss of more performant range operations. Note that this is just for queries where tx.height is the only condition. For example say the query was account.name = callum AND tx.height > 10 then we would still perform a range query with the prefix as "account.name" the value as "callum" and then the height which comes next in the key (at least that's what I think we would do).

That might make sense. In that case, shouldn't this be reserved, specified, and handled in the same place as e.g. tx.height is? Which I'm presuming is outside of the kv package?

Yup turns out we already have tx.hash prefix but it isn't reserved (neither is tx.height for that matter), i.e. I could create an event called tx.height set a different value and the tx would also be saved there. Also the tx.hash key isn't actually used in the kvstore only as part of querying. I will change this.

I guess in general there is a lot of thought that can be spent on this and probably lots of fully featured libraries that do a good job of parsing queries and implementing secondary indexes. It's quite interesting to me but I think I should save properly delving into it for a later time so for now I would ideally like to focus on just tweaking a small part of the keys

I've sort of already made it one key. There is a function called key() that makes keys of the following format string/string/int64/uint32. Both the keyForEvent and keyForHeight are utility wrapper functions that generate keys of this format.

Right, ok -- in that case we shouldn't really need all of the checks that skip erroring/invalid keys, since they should all be in a correct format. Makes sense to unify the tx.hash stuff then I think.

I guess in general there is a lot of thought that can be spent on this and probably lots of fully featured libraries that do a good job of parsing queries and implementing secondary indexes.

Yeah. Honestly, I don't really see any point in implementing this ourselves -- I'd just use SQLite and call it a day.