feat(net): optimize transaction rate limiting#6714
feat(net): optimize transaction rate limiting#6714xxo1shine wants to merge 1 commit intotronprotocol:developfrom
Conversation
xxo1shine
changed the title
| receiveTcpMinDataLength = 2048 | ||
| maxTransactionPendingSize = 2000 | ||
| pendingTransactionTimeout = 60000 | ||
| maxTrxCacheSize = 50000 // total cached trx across handler queues + pending + rePush |
| TransactionsMsgHandler handler = new TransactionsMsgHandler(); | ||
|
|
||
| TronNetDelegate tronNetDelegateMock = Mockito.mock(TronNetDelegate.class); | ||
| Mockito.when(tronNetDelegateMock.getCachedTransactionSize()).thenReturn(50_001); |
There was a problem hiding this comment.
Good targeted test for the new busy logic — mocking TronNetDelegate to control the cached size is exactly the right approach 👍
Tiny nit: the magic number 50_001 couples the test to the default maxTrxCacheSize. If someone tweaks the default later, this test silently keeps passing only because 50_001 > 50_000 happens to hold. Using Args.getInstance().getMaxTrxCacheSize() + 1 would be future-proof, and adding a boundary case (e.g. exact-threshold returns false) would catch off-by-one regressions. Totally optional polish.
There was a problem hiding this comment.
Agreed — good catch. isBusy() uses strict >, so cachedSize == maxTrxCacheSize should indeed return false, and a boundary case is a clean way to guard against off-by-one regressions. Plan:
- Replace
50_001withArgs.getInstance().getMaxTrxCacheSize() + 1 - Add an assertion that
isBusy()returnsfalsewhencachedSizeequalsgetMaxTrxCacheSize()
Will push this in the next commit. Thanks for the review!
| public boolean isBusy() { | ||
| return queue.size() + smartContractQueue.size() > MAX_TRX_SIZE; | ||
| return queue.size() + smartContractQueue.size() | ||
| + tronNetDelegate.getCachedTransactionSize() > Args.getInstance().getMaxTrxCacheSize(); |
There was a problem hiding this comment.
[NIT] May be specify what happens when maxrxCacheSize <=0 .
There was a problem hiding this comment.
The minimum value has been set to 2000.
| public long pendingTransactionTimeout; | ||
| @Getter | ||
| @Setter | ||
| public int maxTrxCacheSize; // clearParam: 50000 |
There was a problem hiding this comment.
[SHOULD] Remove this comment; otherwise, you’ll need to keep two places in the code consistent.
xxo1shine
force-pushed
the
feature/trx-rate-limiting-optimization-1
branch
from
d84ec09 to
0581b72
Compare
…e check 1. Add Manager.getCachedTransactionSize() = pushTransactionQueue + pendingTransactions + rePushTransactions to expose the true cached transaction count across all three queues. 2. Fix isTooManyPending() to include pushTransactionQueue, which was previously omitted, causing the pending threshold to be underestimated. 3. Update TransactionsMsgHandler.isBusy() to factor in the Manager cache size via TronNetDelegate.getCachedTransactionSize(), so the node stops accepting TRX INV messages when the full pipeline is busy. 4. Make the busy threshold configurable via node.maxTrxCacheSize (default: 50000), replacing the hardcoded MAX_TRX_SIZE constant. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
xxo1shine
force-pushed
the
feature/trx-rate-limiting-optimization-1
branch
from
0581b72 to
4d74ef2
Compare
5 participants
What does this PR do?
Optimize transaction rate limiting with accurate cache size check.
Add
Manager.getCachedTransactionSize() = pushTransactionQueue + pendingTransactions + rePushTransactionsto expose the true cached transaction count across all three queues.Fix
isTooManyPending()to include pushTransactionQueue, which was previously omitted, causing the pending threshold to be underestimated.Update
TransactionsMsgHandler.isBusy()to factor in the Manager cache size viaTronNetDelegate.getCachedTransactionSize(), so the node stops accepting TRX INV messages when the full pipeline is busy.Make the busy threshold configurable via
node.maxTrxCacheSize (default: 50000), replacing the hardcodedMAX_TRX_SIZEconstant.Fixes #6684
Why are these changes required?
This PR has been tested by:
Follow up
Extra details