-
Notifications
You must be signed in to change notification settings - Fork 157
fix: Add ingest throttle for inner batch transactions #19103
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
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.
A critical aspect of this implementation is making sure that ingest throttles are applied "all-or-nothing" for the entire batch; see comment. 🙏
throw new PreCheckException(BUSY); | ||
checkThrottles(state, configuration, txInfo); | ||
// If the transaction is a batch transaction, we need to check the throttling for each inner transaction | ||
if (functionality == HederaFunctionality.ATOMIC_BATCH) { |
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.
A
, B
, C
. Assume that checkThrottles()
passes for A
and B
, but fails for C
.
Then this implementation will not "refund" the capacity used for A
and B
, even though those transactions are never actually handled.
This lets an attacker consume ingest throttle capacity without paying fees at consensus for that usage.
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.
Thanks, I did miss that. Can you re-review the fix.
...a-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip551/AtomicBatchTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## main #19103 +/- ##
=========================================
Coverage 69.35% 69.35%
- Complexity 23268 23272 +4
=========================================
Files 2645 2645
Lines 100710 100741 +31
Branches 10409 10414 +5
=========================================
+ Hits 69846 69868 +22
- Misses 26902 26907 +5
- Partials 3962 3966 +4
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
# Conflicts: # hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip551/AtomicBatchNegativeTest.java
Signed-off-by: ibankov <[email protected]>
Signed-off-by: ibankov <[email protected]>
Description:
Introducing an ingest throttling mechanism specifically for inner batch transactions within the consensus node.
Fixes #
#19031