Add transaction rate-limit functionality#750
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThis pull request extends error handling and transaction configuration by introducing an address-based rate limiting mechanism. A new case is added in the error handler to manage rate limit errors. Additionally, new configuration options and command-line flags are provided to control transaction submission frequency and duration. The changes also integrate a rate limiter into the transaction processing flow in the service layer, ensuring that excessive transaction submissions from the same address are curtailed. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
services/requester/requester.go (1)
172-180: Consider implementing a cleanup mechanism for the memory store.The memory store could grow unbounded as it accumulates rate limit data for different addresses. Consider implementing a cleanup mechanism or using a store with built-in TTL support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/utils.go(1 hunks)cmd/run/cmd.go(3 hunks)config/config.go(1 hunks)services/requester/requester.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (4)
config/config.go (1)
92-96: LGTM! Well-documented configuration fields.The new configuration fields for transaction rate limiting are well-typed and clearly documented.
api/utils.go (1)
134-135: LGTM! Consistent error handling.The rate limit error handling is consistent with the existing error handling pattern.
cmd/run/cmd.go (1)
290-291: LGTM! Well-configured command-line flags.The default values (1 request per 3 seconds) align well with the testnet block production time, and the flags are clearly documented.
services/requester/requester.go (1)
219-224: LGTM! Well-implemented rate limiting.The rate limiting implementation is clean and effective:
- Only applies to testnet
- Uses sender's address as key for per-address limiting
- Returns appropriate error when limit is reached
cmd/run/cmd.go
Outdated
| txDuration, err := time.ParseDuration(txDurationLimit) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid unit %s for TX duration limit: %w", txDuration, err) | ||
| } | ||
| cfg.TxDurationLimit = txDuration |
There was a problem hiding this comment.
Fix error message variable.
The error message uses txDuration instead of txDurationLimit which could be confusing.
Apply this diff to fix the error message:
- return fmt.Errorf("invalid unit %s for TX duration limit: %w", txDuration, err)
+ return fmt.Errorf("invalid unit %s for TX duration limit: %w", txDurationLimit, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| txDuration, err := time.ParseDuration(txDurationLimit) | |
| if err != nil { | |
| return fmt.Errorf("invalid unit %s for TX duration limit: %w", txDuration, err) | |
| } | |
| cfg.TxDurationLimit = txDuration | |
| txDuration, err := time.ParseDuration(txDurationLimit) | |
| if err != nil { | |
| return fmt.Errorf("invalid unit %s for TX duration limit: %w", txDurationLimit, err) | |
| } | |
| cfg.TxDurationLimit = txDuration |
There was a problem hiding this comment.
@peterargue Thanks for confirming! I'll mark this as resolved.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
services/requester/requester.go
Outdated
| _, _, _, ok, _ := e.limiter.Take(ctx, from.Hex()) | ||
| if !ok { | ||
| return common.Hash{}, errs.ErrRateLimit | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the error from limiter.Take().
The error from limiter.Take() should be handled as it could indicate issues with the rate limiter.
Apply this diff to handle the error:
- _, _, _, ok, _ := e.limiter.Take(ctx, from.Hex())
+ _, _, _, ok, err := e.limiter.Take(ctx, from.Hex())
+ if err != nil {
+ return common.Hash{}, fmt.Errorf("failed to check rate limit: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, _, _, ok, _ := e.limiter.Take(ctx, from.Hex()) | |
| if !ok { | |
| return common.Hash{}, errs.ErrRateLimit | |
| } | |
| } | |
| _, _, _, ok, err := e.limiter.Take(ctx, from.Hex()) | |
| if err != nil { | |
| return common.Hash{}, fmt.Errorf("failed to check rate limit: %w", err) | |
| } | |
| if !ok { | |
| return common.Hash{}, errs.ErrRateLimit | |
| } | |
| } |
There was a problem hiding this comment.
@peterargue Thanks for confirming! Looking forward to the improved error handling. 👍
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
services/requester/requester.go
Outdated
| return common.Hash{}, fmt.Errorf("failed to derive the sender: %w", err) | ||
| } | ||
|
|
||
| if e.config.FlowNetworkID == flowGo.Testnet { |
There was a problem hiding this comment.
I would suggest to check whether tx-request-limit > 0:
rateLimitEnabled := tx-request-limit > 0
if rateLimitEnabled {
...
}
So that by default, the rate limit is disabled, for testnet, we will add a flag tx-request-limit=1 to enable it.
There was a problem hiding this comment.
+1. let's make this flexible so it could be used anywhere, and let the operator decide.
cmd/run/cmd.go
Outdated
| txDuration, err := time.ParseDuration(txDurationLimit) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid unit %s for TX duration limit: %w", txDuration, err) | ||
| } | ||
| cfg.TxDurationLimit = txDuration |
cmd/run/cmd.go
Outdated
| Cmd.Flags().IntVar(&cfg.ProfilerPort, "profiler-port", 6060, "Port for the Profiler server") | ||
| Cmd.Flags().StringVar(&txStateValidation, "tx-state-validation", "tx-seal", "Sets the transaction validation mechanism. It can validate using the local state index, or wait for the outer Flow transaction to seal. Available values ('local-index' / 'tx-seal'), defaults to 'tx-seal'.") | ||
| Cmd.Flags().Uint64Var(&cfg.TxRequestLimit, "tx-request-limit", 1, "Number of transaction submissions to allow per the specified interval. Applies only on Testnet.") | ||
| Cmd.Flags().StringVar(&txDurationLimit, "tx-duration-limit", "3s", "Time interval upon which to enforce transaction submission rate limiting. Applies only on Testnet.") |
There was a problem hiding this comment.
you could use DurationVar which will automatically parse the value into a time.Duration
There was a problem hiding this comment.
Awesome, I missed this method 💯
Updated in b33cd9c
cmd/run/cmd.go
Outdated
| Cmd.Flags().IntVar(&cfg.ProfilerPort, "profiler-port", 6060, "Port for the Profiler server") | ||
| Cmd.Flags().StringVar(&txStateValidation, "tx-state-validation", "tx-seal", "Sets the transaction validation mechanism. It can validate using the local state index, or wait for the outer Flow transaction to seal. Available values ('local-index' / 'tx-seal'), defaults to 'tx-seal'.") | ||
| Cmd.Flags().Uint64Var(&cfg.TxRequestLimit, "tx-request-limit", 1, "Number of transaction submissions to allow per the specified interval. Applies only on Testnet.") | ||
| Cmd.Flags().StringVar(&txDurationLimit, "tx-duration-limit", "3s", "Time interval upon which to enforce transaction submission rate limiting. Applies only on Testnet.") |
There was a problem hiding this comment.
you could use DurationVar which will automatically parse the value into a time.Duration
the current naming sounds like a limit for the duration, not the other way around. Also, this applies on whichever network the gateway is configured to use
| Cmd.Flags().StringVar(&txDurationLimit, "tx-duration-limit", "3s", "Time interval upon which to enforce transaction submission rate limiting. Applies only on Testnet.") | |
| Cmd.Flags().StringVar(&txDurationLimit, "tx-request-limit-duration", "3s", "Time interval upon which to enforce transaction submission rate limiting.") |
services/requester/requester.go
Outdated
| return common.Hash{}, fmt.Errorf("failed to derive the sender: %w", err) | ||
| } | ||
|
|
||
| if e.config.FlowNetworkID == flowGo.Testnet { |
There was a problem hiding this comment.
+1. let's make this flexible so it could be used anywhere, and let the operator decide.
services/requester/requester.go
Outdated
| _, _, _, ok, _ := e.limiter.Take(ctx, from.Hex()) | ||
| if !ok { | ||
| return common.Hash{}, errs.ErrRateLimit | ||
| } | ||
| } |
m-Peter
force-pushed
the
mpeter/testnet-tx-submission-rate-limit
branch
2 times, most recently
from
5de2d4d to
7b49b08
Compare
m-Peter
force-pushed
the
mpeter/testnet-tx-submission-rate-limit
branch
from
7b49b08 to
4e60c99
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/requester/requester.go (1)
219-228: Consider adding metrics for rate limit rejections.While the rate limit implementation is correct, adding metrics would help monitor the rate limiting behavior in production.
Add metrics before returning the rate limit error:
if !ok { + e.collector.RateLimitRejection(from.Hex()) return common.Hash{}, errs.ErrRateLimit }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/utils.go(1 hunks)cmd/run/cmd.go(1 hunks)config/config.go(1 hunks)services/requester/requester.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/utils.go
- cmd/run/cmd.go
- config/config.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (4)
services/requester/requester.go (4)
24-25: LGTM! Appropriate imports added.The required imports for rate limiting functionality are correctly added.
115-115: LGTM! Rate limiter field added.The
limiterfield is appropriately added to theEVMstruct with the correct type.
172-180: LGTM! Rate limiter initialization looks good.The rate limiter is properly initialized with configuration parameters for tokens and interval.
219-228:Details
✅ Verification successful
Verify rate limit configuration in the codebase.
Let's ensure the rate limit configuration is properly documented and consistently used.
🏁 Script executed:
#!/bin/bash # Search for rate limit configuration usage rg -A 5 "TxRequestLimit|TxRequestDurationLimit"Length of output: 1598
Rate Limit Configuration Verified
The rate limit configuration is properly documented and consistently used across the codebase. In particular:
Configuration Definitions:
TheTxRequestLimitandTxRequestDurationLimitfields are documented inconfig/config.gowith clear comments and are set via CLI flags incmd/run/cmd.go.Usage Consistency:
The requester service inservices/requester/requester.gocheckse.config.TxRequestLimit > 0to enable rate limiting and correctly invokes the limiter with these configurations.No further changes are required.
m-Peter
changed the title
cmd/run/cmd.go
Outdated
| Cmd.Flags().IntVar(&cfg.ProfilerPort, "profiler-port", 6060, "Port for the Profiler server") | ||
| Cmd.Flags().StringVar(&txStateValidation, "tx-state-validation", "tx-seal", "Sets the transaction validation mechanism. It can validate using the local state index, or wait for the outer Flow transaction to seal. Available values ('local-index' / 'tx-seal'), defaults to 'tx-seal'.") | ||
| Cmd.Flags().Uint64Var(&cfg.TxRequestLimit, "tx-request-limit", 0, "Number of transaction submissions to allow per the specified interval.") | ||
| Cmd.Flags().DurationVar(&cfg.TxRequestDurationLimit, "tx-request-duration-limit", time.Second*3, "Time interval upon which to enforce transaction submission rate limiting.") |
There was a problem hiding this comment.
tx-request-duration-limit sounds like the limit is applied on the duration, not the other way around
| Cmd.Flags().DurationVar(&cfg.TxRequestDurationLimit, "tx-request-duration-limit", time.Second*3, "Time interval upon which to enforce transaction submission rate limiting.") | |
| Cmd.Flags().DurationVar(&cfg.TxRequestDurationLimit, "tx-request-limit-duration", time.Second*3, "Time interval upon which to enforce transaction submission rate limiting.") |
There was a problem hiding this comment.
Good point. Updated in eeb6ee2, in all relevant places.
peterargue
left a comment
peterargue
left a comment
There was a problem hiding this comment.
added couple small comments, but looks good
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
m-Peter
force-pushed
the
mpeter/testnet-tx-submission-rate-limit
branch
from
b2f7a03 to
2f462fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
services/requester/requester.go (2)
171-179: Add validation for TxRequestLimitDuration.While the initialization is correct, consider validating that TxRequestLimitDuration is positive to prevent potential issues with the rate limiter.
+ if config.TxRequestLimitDuration <= 0 { + return nil, fmt.Errorf("TX request duration limit must be positive, got: %v", config.TxRequestLimitDuration) + } + rateLimiter, err := memorystore.New( &memorystore.Config{ Tokens: config.TxRequestLimit, Interval: config.TxRequestLimitDuration, }, )
214-223: Improve error handling specificity.The error handling is good, but consider wrapping the rate limit error with a more specific error type to help clients distinguish between rate limit check failures and actual rate limit hits.
_, _, _, ok, err := e.rateLimiter.Take(ctx, from.Hex()) if err != nil { - return common.Hash{}, fmt.Errorf("failed to check rate limit: %w", err) + return common.Hash{}, errs.NewRateLimitCheckError(fmt.Errorf("failed to check rate limit: %w", err)) } if !ok { e.collector.RequestRateLimited("SendRawTransaction") return common.Hash{}, errs.ErrRateLimit }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/utils.go(1 hunks)cmd/run/cmd.go(1 hunks)config/config.go(1 hunks)services/requester/requester.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/utils.go
- cmd/run/cmd.go
- config/config.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (1)
services/requester/requester.go (1)
24-25: LGTM! Clean integration of rate limiter.The rate limiter integration follows Go conventions with appropriate imports and field type.
Also applies to: 114-114
4 participants
Closes: #746
Description
The default TX submission rate-limit is set for 1 request / 3s..
The
3smagic value here is the block production time ontestnet.Both of these are configurable with the flags below:
tx-request-limit(default is0, meaning no rate-limit applies)tx-request-duration-limit(default is3s, for 3 seconds)For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Summary by CodeRabbit