Support height queries for queriers

[alexanderbez]

A (working) POC stab at supporting height queries for querier based queries (wow what a mouthful). This POC does not handle merkle proofs in any way. Doing so warrants further discussion and is not immediately feasible because the node may modify state returned from any internal queries (eg. staking).

If this POC is agreed upon, perhaps we may potentially include it in a release with a clear stipulation that proofs are not supported.

closes: #4318

Follow up: Create an issue to allow a height query param to be provided to REST endpoints.

/cc @zmanian @cwgoes @jackzampolin @mossid

---

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added a relevant changelog entry: clog add [section] [stanza] [message]

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #4382 into master will increase coverage by 0.04%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master    #4382      +/-   ##
==========================================
+ Coverage   54.53%   54.58%   +0.04%     
==========================================
  Files         247      248       +1     
  Lines       15908    15967      +59     
==========================================
+ Hits         8676     8716      +40     
- Misses       6602     6617      +15     
- Partials      630      634       +4

[zmanian]

If I remember right, app.go used call LoadVersion(0) which basically loaded all versions in the store into memory. It appears not to do this any more.

Then this would be correct and provide the expected lazy loading behavior.

[alexanderbez]

*MutableTree#LoadVersion still loads all the node roots into memory for each store on startup.

[alexanderbez]

/cc @jordansexton

[zmanian]

With 876f0d8, does this now support proofs?

[alexanderbez]

With 876f0d8, does this now support proofs?

@zmanian unfortunately not. That commit relates to direct key lookups. Querier-based queries will need a bit more thought (eg. tracing each lookup). I'm brainstorming...

[zmanian]

Oh okay. the loss of proofs is not a regression.

[alexanderbez]

Oh okay. the loss of proofs is not a regression.

Correct. More like the continuance of their absence.

[cwgoes]

Looks reasonable to me, although there could be efficiency concerns with particular usecases - e.g. querying all height sequentially would call LoadVersion each time, which might be inefficient (not sure if the IAVL library would keep the existing mostly-the-same version in memory or reload everything from disk).

[jackzampolin]

Good with the tradeoffs here. This also gets us into a better place wrt proofs it looks like.

[alexanderbez]

Looks reasonable to me, although there could be efficiency concerns with particular usecases - e.g. querying all height sequentially would call LoadVersion each time, which might be inefficient (not sure if the IAVL library would keep the existing mostly-the-same version in memory or reload everything from disk).

Hmmm, iirc, this is not true with my implementation (I could very well be wrong). What makes you assume this? Querier queries call CacheMultiStoreWithVersion, which ultimately calls GetImmutable on the IAVL tree store which in turn only loads a single root node/height (if it even exists).

---

EDIT: I see you wrote, sequentially. Ahhh, then yes, it would. Do you have any suggestions on mitigation? Looks like tree.ndb.getRoot(version) just does a DB lookup which may already be cached (depending on the tuned parameters/config of the underlying DB).

[alexanderbez]

@mossid I will merge this today, try to review if you can.

[jackzampolin]

🔥 🔥 🔥 🔥 🔥 🔥

[alexanderbez]

Merged, but still would appreciate a review @mossid