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

Refactor komodo_state struct #479

Open
jmjatlanta opened this issue Aug 17, 2021 · 2 comments
Open

Refactor komodo_state struct #479

jmjatlanta opened this issue Aug 17, 2021 · 2 comments

Comments

@jmjatlanta
Copy link

jmjatlanta commented Aug 17, 2021

The komodo_state struct is used in many areas of Komodo. Refactoring 2 areas will improve reliability, memory use, and code readability:

  1. NPOINTS - This is implemented as an array
    • A different collection from the standard library will make adding and removing elements easier to understand
    • The collection is often searched linearly. last_NPOINTSi tries to help. But a std::map or something similar may bring a performance improvement.
  2. There is an array of komodo_state structs, indexed by CURRENCY, with some tricks around the chain token (always index 0) and KMD (always index 34). It seems CURRENCY actually has little involvement, and the link between komodo_state and CURRENCY should probably be broken. This may be a good use for a std::list
@jmjatlanta
Copy link
Author

jmjatlanta commented Aug 18, 2021

I have built some tests to verify the functionality of the methods related to NPOINTS. These questions remain:

  1. The code currently works mainly with the NPOINTS in the order they were received. Linear searches (forward and backward) are done to find desired values. Beyond the possible performance penalty of linear searches,
    1. Asking for the "last" NPOINT doesn't necessarily equate to the "highest". Also, asking the komodo_state object for its cached values for last does not reflect "highest"
    2. There are almost no validations for incoming notarizations. Multiple of the same heights and notarized_heights are permitted. Is that by design?

If all of the above is desired behavior, we may still benefit from using something like boost MultiIndex containers. The question becomes should it be made to work exactly as it has been (minus the memory leak from not deallocating the memory used by NPOINTS), or is now a good time to fix such issues?

Supplemental information:
Searches through NPOINTS are done in komodo_npptr (search by notarized_height), and komodo_notarizeddata (search by nHeight). komodo_prevMoMheight just looks for the last added element to the array.

Elements are added to the array in komodo_notarize_update.

last_NPOINTi is only used in komodo_notarizeddata, and its purpose seems to be to cache the search point to speed subsequent lookups. Its effectiveness is questionable in my mind, but real-world results may prove me wrong.

@jmjatlanta
Copy link
Author

jmjatlanta commented Aug 18, 2021

The search for nheight in komodo_notarizeddata is only used in komodo_checkpoint. That method seems to indicate that only 1 nHeight should be in the collection.

The search for notarized_height in komodo_npptr is used in komodo_MoMdata and komodo_rwccdata. Those methods seem to indicate that only 1 notarized_height should be in the collection.

who-biz pushed a commit to who-biz/komodo that referenced this issue Mar 14, 2023
Update static seed nodes and seed node DNS.
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

No branches or pull requests

1 participant