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

Rpc accounts receivable sorting fix #4070

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

JerzyStanislawski
Copy link
Contributor

@JerzyStanislawski JerzyStanislawski commented Jan 24, 2023

The continuation of #3845.
A proposition to fix receivable rpc method using ranges. @pwojcikdev please take a look, if it's ok I'll apply the changes to the other rpc methods.
Edit: Workflow run failed, I think Clang should be upgraded?

@dsiganos
Copy link
Contributor

dsiganos commented Feb 1, 2023

Hi Jerzy, thank you for working on this. Here is my feedback:

  • Unfortunately, it looks like we can't use the C++20 ranges library due it not supported on macOS yet
  • Your implementation has one major flaw, when sorting, it loads all of the pending entries into RAM. It should not do that.
  • We should enforce a max limit on count parameter e.g. 1000.
  • Use a guard if to remove that very long if statement

@JerzyStanislawski
Copy link
Contributor Author

Hey Dimitrios, thanks for the feedback.
So do we still stick to C++ 20 ranges?

@dsiganos
Copy link
Contributor

dsiganos commented Feb 1, 2023

Let's drop the ranges library for now.

@JerzyStanislawski
Copy link
Contributor Author

Sorry for late response - how should we approach with the fix? I mean, do we assume we'll use C++ 20 sooner or later? If so - should we make minimal changes now (just fix sorting and enforce max count) and reapply this commit with ranges once we use C++ 20?

@dsiganos
Copy link
Contributor

dsiganos commented Feb 6, 2023

We need to decide exactly how to implement this feature in a way that doesn't allow the node to run out of resources.
In other words, it should not be possible to ask from unlimited resources or cause for unlimited resources to be accumulated in order to calculate the response.

For example, sorted output with an large offset is the worst case scenario, it means we have to load all of the records in RAM (or create a new sorted table), sort them and then apply an offset.

Since offset was a new addition, in v24.0, we are thinking of removing it.
So, we need to re-design this API in a way that makes sense and ideally backwards compatible too.

@JerzyStanislawski
Copy link
Contributor Author

Yeah, makes sense.
I thought about trying to store pending blocks sorted by amount desc, but that would require adding amount to pending_key so it doesn't seem to be the way.
How about keeping API as is and limiting max count and offset (if with sorting) as you proposed?

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.

2 participants