Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IAVL fast node #2595

Closed
wants to merge 5 commits into from
Closed

Use IAVL fast node #2595

wants to merge 5 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jun 15, 2023

This PR enables IAVL fast node, and will become more important as more consumer chains are being added.

@jtremback
Copy link
Contributor

Does this introduce any risks of consensus breaking or can it be put out as a non-coordinated upgrade?

Can you post some stats about what the performance increase is?

@faddat
Copy link
Contributor Author

faddat commented Jun 16, 2023

The best way is to test it out for yourself, here:

https://twitter.com/gadikian/status/1669600575053508613?s=20

As for stats, I can ask Roman on the osmosis team, but I'm not fully sure. There may have been some stats, from a long time ago, the thing I can say for sure is that there was a dramatic improvement in iterator performance.

I'll make a PR that demonstrates using it as seen in the video, against the v9.x branch...

  • please note that this has in the past caused a consensus break on osmosis but that was several iavl versions ago. I think that everything ought to be fine. You'll notice much better performance from the get go though.

@faddat faddat requested review from zmanian and a team as code owners July 2, 2023 23:45
@codecov
Copy link

codecov bot commented Jul 2, 2023

Codecov Report

Merging #2595 (131197a) into main (1c8b069) will not change coverage.
The diff coverage is n/a.

❗ Current head 131197a differs from pull request most recent head 7890f28. Consider uploading reports for the commit 7890f28 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2595   +/-   ##
=======================================
  Coverage   85.16%   85.16%           
=======================================
  Files          23       23           
  Lines        1604     1604           
=======================================
  Hits         1366     1366           
  Misses        192      192           
  Partials       46       46           

see 2 files with indirect coverage changes

@faddat
Copy link
Contributor Author

faddat commented Jul 2, 2023

This can also help with the set of issues outlined in #2635

Keep in mind: when a node is pounded to death over RPC, one thing that happens is that p2p goes down.

@faddat
Copy link
Contributor Author

faddat commented Jul 9, 2023

@jtremback -- the benchmarks are... not great or formal.

To my knowledge, no one has them. Another reason that this PR is a good idea though, is the fact that it will ensure that we don't run into downtime at the 47 upgrade. Basically, would be good to roll this out across validators while we are still on v10.

@mpoke
Copy link
Contributor

mpoke commented Jul 25, 2023

@faddat At the recommendation of the SDK team, we decided not to go ahead with this change. The reason is that node operators are free to change this and many of them already do.

@mpoke mpoke closed this Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants