nano-node
nano-node copied to clipboard
nanocurrency
Rpc accounts receivable sorting fix
The continuation of https://github.com/nanocurrency/nano-node/pull/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?
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
Hey Dimitrios, thanks for the feedback. So do we still stick to C++ 20 ranges?
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?
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.
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?