-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Chunk /gettransactions to avoid hitting restricted RPC limit #8388
Chunk /gettransactions to avoid hitting restricted RPC limit #8388
Conversation
// get those txes | ||
if (!txids.empty()) | ||
// get_transaction_pool_hashes.bin may return more transactions than we're allowed to request in restricted mode | ||
const size_t SLICE_SIZE = 100; // RESTRICTED_TRANSACTIONS_COUNT as defined in rpc/core_rpc_server.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we detect if the daemon is in restricted mode and chunk accordingly, as opposed to defaulting to the (rather low) value of 100?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is what happens already here (on the server side at least)
monero/src/rpc/core_rpc_server.cpp
Line 888 in 9750e1f
if (restricted && req.txs_hashes.size() > RESTRICTED_TRANSACTIONS_COUNT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but I mean but if the daemon is in unrestricted mode, then we can ask for more at once (what the old code was doing), which would definitely reduce overhead if/when the mempool gets to be the size of (for example) Bitcoin's with >1000 txs per block
should this log message be updated? Maybe to something like
Because otherwise the log is a bit confusing.
Adding the remaining tx count would also be nice but might make the log message too complicated. |
In reply to @selsa: To piggyback off of this, should this log statement be put inside the loop now since there are now multiple requests which could each individually go wrong? |
@jeffro256 it is already in the loop, or am I misunderstanding your suggestion? |
Oh you're right, I read my brackets wrong, it is inside the |
7f891e0
to
04c0da2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code reviewed and tested.
Fixes the following issue when mempool contains more than 100 new transactions when connected to a restricted node:
Error calling gettransactions daemon RPC: r 1, status Too many transactions requested in restricted mode
@selsta