[Collection] Rate limiting transaction by payer #5218

zhangchiqing · 2024-01-09T19:33:08Z

This PR adds a rate limiter to the collection node when adding to the transaction mempool as a short term solution for preventing from attacks.

codecov-commenter · 2024-01-09T19:38:24Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (690a250) 55.47% compared to head (74e0be0) 55.47%.

Files	Patch %	Lines
engine/collection/ingest/rate_limiter.go	68.05%	22 Missing and 1 partial ⚠️
model/flow/address.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5218   +/-   ##
=======================================
  Coverage   55.47%   55.47%           
=======================================
  Files         996      997    +1     
  Lines       95787    95866   +79     
=======================================
+ Hits        53136    53181   +45     
- Misses      38661    38689   +28     
- Partials     3990     3996    +6

Flag	Coverage Δ
unittests	`55.47% <68.29%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yhassanzadeh13 · 2024-01-10T21:16:36Z

engine/collection/ingest/engine.go

@@ -70,6 +70,7 @@ func New(
 			MaxTransactionByteSize: config.MaxTransactionByteSize,
 			MaxCollectionByteSize:  config.MaxCollectionByteSize,
 		},
+		access.NewNoopLimit(),


Is it intended?

This is a placeholder. The actual rate limiter is not implemented, which needs some discussion.

This PR implemented the check for rate limiting transactions when they are received and before being added to the mempool.

zhangchiqing · 2024-01-16T18:58:40Z

access/validator.go

+	}
+}
+
+func NewTransactionValidatorWithLimiter(


Adding a new method, so that the emulator can still depend on the existing NewTransactionValidator method

zhangchiqing · 2024-01-16T18:59:46Z

access/errors.go

+}
+
+func (e InvalidTxRateLimittedError) Error() string {
+	return fmt.Sprintf("transaction rate limitted for payer (%s)", e.Payer)


This is the error message returned to the client which sends the transaction to the AN.

Is it possible to alter the error message?

zhangchiqing · 2024-01-16T19:01:08Z

engine/collection/ingest/rate_limiter.go

+}
+
+// AddressRateLimiter limits the rate of messages sent to a given address
+// It allows the given "limit" amount messages per second with a "burst" amount of messages to be sent at once


@kc1116 could you review the rate limiter implementation? Please check if it's implementing the correct behavior explained here.

Looks good this is the expected behavior, you can also cross reference with the godoc for the rate package

jordanschalm · 2024-01-17T17:36:05Z

access/errors.go

@@ -90,3 +90,11 @@ type InvalidTxByteSizeError struct {
 func (e InvalidTxByteSizeError) Error() string {
 	return fmt.Sprintf("transaction byte size (%d) exceeds the maximum byte size allowed for a transaction (%d)", e.Actual, e.Maximum)
 }
+
+type InvalidTxRateLimittedError struct {


Suggested change

type InvalidTxRateLimittedError struct {

type InvalidTxRateLimitedError struct {

A few other similar typos in the PR, could do a find+replace limitted->limited.

access/validator.go

admin/commands/collection/tx_rate_limiter.go

cmd/collection/main.go

engine/collection/ingest/rate_limiter.go

jordanschalm · 2024-01-17T20:54:20Z

engine/collection/ingest/rate_limiter.go

+		limit:    limit,
+		burst:    burst,


I think it is worth documenting in this new component how these rate limiter configs set on individual nodes will translate to network-wide effective rate limits. I think the assumption of someone setting these configs would otherwise reasonably be that the limit value they set will translate more or less directly to the network-wide effect limit, but this isn't the case.

For simplicity, let's assume that all Collection Nodes in the network are configured with the same limit, burst, and payerAddresses config.

The question is, if I configure all Collection Nodes with this common individual config, with limit $L_i$ and burst $B_i$, what will the effective network-wide rate limit for a given payer be? Let's call the effective global rate limit $L_g$ and burst $B_g$

There are two parameters I can think of that will impact this:

The number of clusters

The propagation redundancy factor - how many nodes we replicate a transaction to upon receiving it

The cluster count ($k$) will proportionally increase the effective $L_g$ and $B_g$, because each Collection Node only ingests at most $\frac{1}{k}$ transactions. For Mainnet, where we have 3 clusters, we would have $L_g=3L_i$ and $B_g=3B_i$.

The impact of the propagation redundancy factor is less straightforward, but will further increase $L_g$ and $B_g$. Roughly, I think it will increase the effective limits proportionally to $\frac{ClusterSize}{PropagationRedundancyFactor}$.

There's some uncertainty in the second term, but overall, my gut feeling is that setting a rate limit of for example 1TPS in this config would result in an effective rate limit of somewhere in the range of 5-10TPS. That's fairly unintuitive, so we should be aware of it when using this, and document it.

jordanschalm · 2024-01-17T20:56:18Z

engine/collection/ingest/rate_limiter_test.go

+	good1 := unittest.RandomAddressFixture()
+	limited1 := unittest.RandomAddressFixture()
+	limited2 := unittest.RandomAddressFixture()
+	fmt.Println(limited1, limited2)


should remove or change to t.Log

jordanschalm · 2024-01-17T20:57:54Z

engine/collection/ingest/rate_limiter_test.go

+	l.AddAddress(limited2)
+	list := l.GetAddresses()
+	require.Len(t, list, 2)
+	require.Contains(t, list, limited1)


Suggested change

require.Contains(t, list, limited1)

require.Contains(t, list, limited2)

Should this be limited2 since we just added it?

engine/collection/ingest/rate_limiter_test.go

jordanschalm · 2024-01-17T21:01:28Z

engine/collection/ingest/rate_limiter_test.go

+	require.False(t, l.IsRateLimited(addr1))
+	require.True(t, l.IsRateLimited(addr1))
+
+	time.Sleep(time.Millisecond * 1100) // wait 1.1 second


Suggested change

time.Sleep(time.Millisecond * 1100) // wait 1.1 second

time.Sleep(time.Millisecond * 1100) // wait 1.1 second

Could we set numPerSec to eg. 0.1 and then wait 110ms?

Better to minimize sleeps in tests if possible.

Use require.Eventually

require.Eventually(t, func() bool { return l.IsRateLimited(addr1) == false && l.IsRateLimited(addr1) == true }, 2*time.Second, 100*time.Millisecond)

access/validator.go

kc1116 · 2024-01-19T20:06:24Z

engine/collection/ingest/rate_limiter_test.go

@@ -84,17 +83,26 @@ func TestLimiterWaitLongEnough(t *testing.T) {

 	addr1 := unittest.RandomAddressFixture()

-	numPerSec := rate.Limit(1)
+	// with limit set to 10, it means we allow 10 messages per second,


jordanschalm

Nice work 🎸

I added some code suggestions for documentation from the comment about the effective network-wide rate limit in my last review
There is also this comment from the last review, which hasn't been addressed. Could you check to confirm that line is correct the way it is, if you haven't already? (If it is, feel free to resolve the comment)

access/validator.go

cmd/collection/main.go

engine/collection/ingest/rate_limiter.go

peterargue

looks good other than comment about lock

engine/collection/ingest/rate_limiter.go

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Co-authored-by: Khalil Claybon <khalil.claybon@dapperlabs.com>

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>

yhassanzadeh13 reviewed Jan 10, 2024

View reviewed changes

zhangchiqing force-pushed the leo/rate-limit-transaction-by-payer branch from b658972 to 62dec77 Compare January 16, 2024 19:03

zhangchiqing commented Jan 16, 2024

View reviewed changes

zhangchiqing marked this pull request as ready for review January 16, 2024 23:12

zhangchiqing requested review from peterargue and jordanschalm as code owners January 16, 2024 23:12

zhangchiqing requested a review from kc1116 January 16, 2024 23:12

jordanschalm reviewed Jan 17, 2024

View reviewed changes

kc1116 reviewed Jan 19, 2024

View reviewed changes

access/validator.go Outdated Show resolved Hide resolved

kc1116 reviewed Jan 19, 2024

View reviewed changes

zhangchiqing force-pushed the leo/rate-limit-transaction-by-payer branch from 1469927 to 657fe8b Compare January 20, 2024 00:14

zhangchiqing requested review from jordanschalm, kc1116 and yhassanzadeh13 January 20, 2024 01:32

jordanschalm approved these changes Jan 22, 2024

View reviewed changes

access/validator.go Outdated Show resolved Hide resolved

cmd/collection/main.go Show resolved Hide resolved

engine/collection/ingest/rate_limiter.go Show resolved Hide resolved

zhangchiqing force-pushed the leo/rate-limit-transaction-by-payer branch 2 times, most recently from c893dc7 to 680e815 Compare January 24, 2024 19:19

peterargue approved these changes Jan 29, 2024

View reviewed changes

engine/collection/ingest/rate_limiter.go Show resolved Hide resolved

zhangchiqing added 11 commits January 29, 2024 13:40

rate limiting transaction by payer

a648d6f

add limiter

55a324e

add rate limiter

2e0b7c2

add test cases

df21354

add burst

4481ef5

fix lint

279a814

add rate limiter

4a6ad47

add admin tool for adding rate limiter

f02c967

use default limiter

eb9bc60

add comments

b8d4ad3

add flag

a770036

zhangchiqing and others added 21 commits January 29, 2024 13:40

Apply suggestions from code review

afd7ad2

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

limitted -> limited

04202a4

speed up rate limit tests

b5e73c4

add comments

d7106d6

add comment

f5aea8b

admin to config rate limiter

a6261d2

allow limiter to be configured

02038bc

update comments

8501337

log

2ff49dd

fix readme

05e6756

read config

f30197b

tmp

49d9467

update readme

ef63bdb

update tests

1984408

Update engine/collection/ingest/rate_limiter.go

afb775b

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Update access/validator.go

fb9d5e6

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

fix lint

cd2cac6

Apply suggestions from code review

c4fe1c6

Co-authored-by: Khalil Claybon <khalil.claybon@dapperlabs.com>

Apply suggestions from code review

00598fe

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

fix lint

a9577ed

Apply suggestions from code review

e090e15

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>

zhangchiqing force-pushed the leo/rate-limit-transaction-by-payer branch from 4469cf0 to e090e15 Compare January 29, 2024 21:40

zhangchiqing enabled auto-merge January 29, 2024 21:40

zhangchiqing added this pull request to the merge queue Jan 30, 2024

Merged via the queue into master with commit cc807f3 Jan 30, 2024
51 checks passed

zhangchiqing deleted the leo/rate-limit-transaction-by-payer branch January 30, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Collection] Rate limiting transaction by payer #5218

[Collection] Rate limiting transaction by payer #5218

zhangchiqing commented Jan 9, 2024 •

edited

Loading

codecov-commenter commented Jan 9, 2024 •

edited

Loading

yhassanzadeh13 Jan 10, 2024

zhangchiqing Jan 11, 2024

zhangchiqing Jan 16, 2024

zhangchiqing Jan 16, 2024

zhangchiqing Jan 16, 2024

kc1116 Jan 19, 2024

jordanschalm Jan 17, 2024

jordanschalm Jan 17, 2024 •

edited

Loading

jordanschalm Jan 17, 2024

jordanschalm Jan 17, 2024

jordanschalm Jan 17, 2024

kc1116 Jan 19, 2024

kc1116 Jan 19, 2024

jordanschalm left a comment

peterargue left a comment

	type InvalidTxRateLimittedError struct {
	type InvalidTxRateLimitedError struct {

	require.Contains(t, list, limited1)
	require.Contains(t, list, limited2)

	time.Sleep(time.Millisecond * 1100) // wait 1.1 second
	time.Sleep(time.Millisecond * 1100) // wait 1.1 second

[Collection] Rate limiting transaction by payer #5218

[Collection] Rate limiting transaction by payer #5218

Conversation

zhangchiqing commented Jan 9, 2024 • edited Loading

codecov-commenter commented Jan 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm left a comment

Choose a reason for hiding this comment

peterargue left a comment

Choose a reason for hiding this comment

zhangchiqing commented Jan 9, 2024 •

edited

Loading

codecov-commenter commented Jan 9, 2024 •

edited

Loading

jordanschalm Jan 17, 2024 •

edited

Loading