les/utils: UDP rate limiter #21930

zsfelfoldi · 2020-11-30T22:27:53Z

This PR is a separated part of the les4 code in order to make the review process easier.

zsfelfoldi · 2020-12-08T10:34:23Z

les/utils/limiter.go

+// selectionWeights calculates the selection weights of a node for both the address and
+// the value selector. The selection weight depends on the next request weight or the
+// summed weights of recently dropped requests. relWeight is reqWeight/maxWeight.
+func selectionWeights(relWeight, value float64) (flatWeight, valueWeight uint64) {


About selection weights: I realized that the terminology is a bit misleading because two things are called "weight". I'll think about better names to clean up the confusion. The weight of a request basically means a fixed cost. Note that we don't use CPU time as a cost metric any more, it can be a constant value depending on the request type (or a sum of fixed costs if requests are grouped in a batch).
The other "weight" is the selection weight which is is used by WeightedRandomSelect and it represents the selection probability. The two are actually inversely related, if there are two requests queued and one of them is considered 10 times heavier then it is selected with 10 times less probability. The flatWeight is 100000000/(requestWeight/sumRequestWeights). This is good because if there are many "light" and "heavy" requests then all will be served eventually but the heavy requests won't starve the light ones by slowing down the processing queue. If you send a heavy request in a crowded situation, you should be prepared that probably 10 light requests will be served before yours gets selected.
The valueWeight is also proportional to value which is an arbitrary number assigned to the sending node (maybe the name is also not the most intuitive here). It is used to ensure that once a node has been proven useful it can get a good chance even if there is a DDoS attack and there requests coming from many addresses. You can think of these as "trusted" nodes. Let's imagine a situation where request weights are all the same, there are 10000 requesting nodes and 5 of them are trusted. The address selector selects any of the requesting nodes with 1/10000 chance while the value selector selects one of the trusted nodes with 1/5 chance (assuming that they have the same "value", the same level of trust). The two selectors are both used with 1/2 chance so the good nodes still get 1/10 chance while the attackers get 1/20000. This is good because a DDoS attack can at least not starve the proven useful connections.
In reality the "trustedness" of a node is not necessarily a true/false thing so instead a 0..1 "value" is used and a node can get into the value selector with a proportional chance. It also makes sense to take the individual request weights into account similarly to flatWeight. Therefore valueWeight is calculated as 100000000*nodeValue/(requestWeight/sumRequestWeights). Note that flatWeight and valueWeight are never compared to each other because they are used in different selectors.

Thank you! Now I understand much better!

rjl493456442 · 2020-12-09T06:48:48Z

les/utils/limiter.go

+	address                 string
+	value                   float64
+	flatWeight, valueWeight uint64 // current selection weights in the address/value selectors
+	nextWeight              uint   // next request weight


Please distinguish the request weight and selection weight. It's really easy to confuse people. For example, the nextWeight, sumWeight here refer to the request weight while all other weight refers to the selection weight.

Perhaps: flatSelection, and valueSelection would be better?

Calling the selection weights "selection" also does not really describe what those numbers actually are (sounds more like it has something to do with the actual selected item).
Since we've already had the "weighted random selector" for a long time, I decided to keep the term "weight" for selection weights and renamed request weight to request cost (since we already use the term "cost" for LES requests).

rjl493456442 · 2020-12-09T06:58:12Z

les/utils/limiter.go

+	} else {
+		f = 0.001 / relWeight
+	}
+	f *= 1000000000


The number is fancy to me. Can we use some meaningful constants? So that the whole conversion logic is more clear

Done, now it's a named constant so it's hopefully easier to see what it does.

rjl493456442 · 2020-12-09T07:00:15Z

les/utils/limiter.go

+		close(process)
+		return process
+	}
+	if value > l.maxValue {


For the request weight, it's only capped with the max weight and normalize at the selectionWeights stage.
Can we do the same thing for the value?

Even more, we can cap both value and requestWeight with the maximum number, and do the normalization in the selectionWeights function internally.

les/utils/limiter.go

rjl493456442 · 2020-12-09T08:22:59Z

les/utils/limiter.go

+			sumWeight:  reqWeight,
+			groupIndex: -1,
+		}
+		nq.flatWeight, nq.valueWeight = selectionWeights(float64(reqWeight)/float64(l.maxWeight), value)


Would be nice to do the request weight normalization and value normalization in the function internally.

les/utils/limiter.go

rjl493456442 · 2020-12-09T08:25:04Z

les/utils/limiter.go

+	ag := l.addresses[nq.address]
+	ag.update(nq, flatWeight)
+	l.addressSelect.Update(ag)
+	if valueWeight != 0 {


Here have a corner case, the original valueWeight is not zero, but after that, the weight is 0.

Right, this was a bug, I removed the if clause because it is unnecessary (doing a no-op Update is not expensive).

les/utils/limiter.go

rjl493456442

Testing is failed, please fix it

rjl493456442 · 2020-12-14T07:09:16Z

les/utils/limiter.go

+	if value > l.maxValue {
+		l.maxValue = value
+	}
+	if value > 0 {


Does the negative value be supported?
if so here https://github.com/ethereum/go-ethereum/pull/21930/files#diff-05f453344a1370c706aca53fb8864362a541daeca84a5c82aecf9344c7af20acR165
we can't just convert it to the uint64.

No, negative values are not supported, they wouldn't make any sense. The check is there to avoid a 0/0 division. Checking either value or maxValue for > 0 or != 0 would work fine here.

les/utils/limiter.go

rjl493456442 · 2020-12-14T08:09:41Z

les/utils/limiter_test.go

+	}
+}
+
+func TestLimiter(t *testing.T) {


TestLimiter: limiter_test.go:181: Request ratio (0.751915) does not match expected value (0.500000) TestLimiter: limiter_test.go:181: Request ratio (0.121453) does not match expected value (0.250000) TestLimiter: limiter_test.go:181: Request ratio (0.126632) does not match expected value (0.250000) TestLimiter: limiter_test.go:181: Request ratio (0.895618) does not match expected value (0.500000) TestLimiter: limiter_test.go:181: Request ratio (0.052434) does not match expected value (0.250000) TestLimiter: limiter_test.go:181: Request ratio (0.051948) does not match expected value (0.250000) TestLimiter: limiter_test.go:181: Request ratio (0.750025) does not match expected value (0.500000) TestLimiter: limiter_test.go:181: Request ratio (0.126047) does not match expected value (0.250000) TestLimiter: limiter_test.go:181: Request ratio (0.123928) does not match expected value (0.250000)

@rjl493456442 I improved the probabilistic testing method a bit because sometimes some values were slightly out of range. Now the test is super stable and quick.
I couldn't reproduce the results seen above though. Your test errors are not just randomly slightly off, they are explicitly wrong. I ran the test several hundred times locally and I haven't seen anything like this. Are you sure you didn't have any code modifications locally? What parameters did you use with go test? I tried cpu=8 but didn't make any difference, my local test results are consistently good now.

rjl493456442 · 2020-12-18T06:41:21Z

I don't quite understand the unit tests, but now it passes. I would approve it, the code LGTM and it's used not yet. So I think it's safe to merge it.

rjl493456442

Please fix https://github.com/ethereum/go-ethereum/pull/21930/files#r542156190

otherwise, LGTM

…eights

zsfelfoldi added the status:in-progress label Nov 30, 2020

zsfelfoldi requested a review from rjl493456442 as a code owner November 30, 2020 22:27

zsfelfoldi mentioned this pull request Nov 30, 2020

les: les/4 protocol extensions #21676

Closed

zsfelfoldi commented Dec 8, 2020

View reviewed changes

rjl493456442 requested changes Dec 9, 2020

View reviewed changes

rjl493456442 reviewed Dec 14, 2020

View reviewed changes

rjl493456442 approved these changes Dec 18, 2020

View reviewed changes

zsfelfoldi changed the title ~~les/utils: UDP rate limiter (WIP)~~ les/utils: UDP rate limiter Jan 17, 2021

zsfelfoldi removed the status:in-progress label Jan 17, 2021

zsfelfoldi mentioned this pull request Jan 18, 2021

les: UDP pre-negotiation of available server capacity #22183

Merged

zsfelfoldi added 10 commits January 28, 2021 15:44

les/utils: Limiter

498f759

les/utils: dropped prior weight vs variable cost logic, using fixed w…

d88298c

…eights

les/utils: always create node selector in addressGroup

d4c3587

les/utils: renamed request weight to request cost

1177e32

les/utils: simplified and improved the DoS penalty mechanism

4f0defc

les/utils: minor fixes

28176c7

les/utils: made selection weight calculation nicer

7c5bcd4

les/utils: fixed linter warning

c415b70

les/utils: more precise and reliable probabilistic test

4d95f23

les/utils: fixed linter warning

b697377

zsfelfoldi force-pushed the limiter branch from d9b69f9 to b697377 Compare January 28, 2021 14:49

zsfelfoldi merged commit 7a800f9 into ethereum:master Jan 28, 2021

quorumbot mentioned this pull request Sep 3, 2021

[Upgrade] Go-Ethereum release v1.10.0 Consensys/quorum#1249

Merged

9 tasks

baptiste-b-pegasys mentioned this pull request Sep 3, 2021

[Upgrade] Go-Ethereum release v1.10.0 baptiste-b-pegasys/quorum#16

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

les/utils: UDP rate limiter #21930

les/utils: UDP rate limiter #21930

zsfelfoldi commented Nov 30, 2020

zsfelfoldi Dec 8, 2020

rjl493456442 Dec 9, 2020

rjl493456442 Dec 9, 2020

rjl493456442 Dec 9, 2020

zsfelfoldi Dec 11, 2020

rjl493456442 Dec 9, 2020

zsfelfoldi Dec 11, 2020

rjl493456442 Dec 9, 2020

zsfelfoldi Dec 11, 2020

rjl493456442 Dec 9, 2020

zsfelfoldi Dec 11, 2020

rjl493456442 Dec 9, 2020

zsfelfoldi Dec 11, 2020

rjl493456442 left a comment

rjl493456442 Dec 14, 2020

zsfelfoldi Dec 18, 2020

rjl493456442 Dec 14, 2020

zsfelfoldi Dec 16, 2020

rjl493456442 commented Dec 18, 2020

rjl493456442 left a comment

les/utils: UDP rate limiter #21930

les/utils: UDP rate limiter #21930

Conversation

zsfelfoldi commented Nov 30, 2020

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

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

rjl493456442 left a 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

rjl493456442 commented Dec 18, 2020

rjl493456442 left a comment

Choose a reason for hiding this comment