From dcf99b7165b02500131d748f8cfce5848146621c Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 19 Feb 2025 17:19:48 +0000 Subject: [PATCH 1/5] chore: log if rate limit status was peer / global rate limit --- .../reqresp/rate-limiter/rate_limiter.ts | 27 ++++++++++++------- .../p2p/src/services/reqresp/reqresp.ts | 15 ++++++++--- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts index 058fe6e24a0b..c9508c4f0fd6 100644 --- a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts +++ b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts @@ -72,10 +72,21 @@ interface PeerRateLimiter { lastAccess: number; } -enum RateLimitStatus { - Allowed, +export enum RateLimitStatus { DeniedGlobal, DeniedPeer, + Allowed, // Note: allowed last to prevent enum evaluating to 0 for success +} + +export function prettyPrintRateLimitStatus(status: RateLimitStatus) { + switch (status) { + case RateLimitStatus.DeniedGlobal: + return 'DeniedGlobal'; + case RateLimitStatus.DeniedPeer: + return 'DeniedPeer'; + case RateLimitStatus.Allowed: + return 'Allowed'; + } } /** @@ -191,10 +202,10 @@ export class RequestResponseRateLimiter { }, CHECK_DISCONNECTED_PEERS_INTERVAL_MS); } - allow(subProtocol: ReqRespSubProtocol, peerId: PeerId): boolean { + allow(subProtocol: ReqRespSubProtocol, peerId: PeerId): RateLimitStatus { const limiter = this.subProtocolRateLimiters.get(subProtocol); if (!limiter) { - return true; + return RateLimitStatus.Allowed; } const rateLimitStatus = limiter.allow(peerId); @@ -202,12 +213,10 @@ export class RequestResponseRateLimiter { case RateLimitStatus.DeniedPeer: // Hitting a peer specific limit, we should lightly penalise the peer this.peerScoring.penalizePeer(peerId, PeerErrorSeverity.HighToleranceError); - return false; - case RateLimitStatus.DeniedGlobal: - // Hitting a global limit, we should not penalise the peer - return false; + return RateLimitStatus.DeniedPeer; default: - return true; + // Hitting a global limit, we should not penalise the peer + return rateLimitStatus; } } diff --git a/yarn-project/p2p/src/services/reqresp/reqresp.ts b/yarn-project/p2p/src/services/reqresp/reqresp.ts index 677bf94ff49e..47addc84a36f 100644 --- a/yarn-project/p2p/src/services/reqresp/reqresp.ts +++ b/yarn-project/p2p/src/services/reqresp/reqresp.ts @@ -30,7 +30,11 @@ import { subProtocolMap, } from './interface.js'; import { ReqRespMetrics } from './metrics.js'; -import { RequestResponseRateLimiter } from './rate-limiter/rate_limiter.js'; +import { + RateLimitStatus, + RequestResponseRateLimiter, + prettyPrintRateLimitStatus, +} from './rate-limiter/rate_limiter.js'; import { ReqRespStatus, ReqRespStatusError, parseStatusChunk, prettyPrintReqRespStatus } from './status.js'; /** @@ -589,8 +593,13 @@ export class ReqResp { try { // Store a reference to from this for the async generator - if (!this.rateLimiter.allow(protocol, connection.remotePeer)) { - this.logger.warn(`Rate limit exceeded for ${protocol} from ${connection.remotePeer}`); + const rateLimitStatus = this.rateLimiter.allow(protocol, connection.remotePeer); + if (rateLimitStatus != RateLimitStatus.Allowed) { + this.logger.warn( + `Rate limit exceeded ${prettyPrintRateLimitStatus(rateLimitStatus)} for ${protocol} from ${ + connection.remotePeer + }`, + ); throw new ReqRespStatusError(ReqRespStatus.RATE_LIMIT_EXCEEDED); } From 32341368319cf666e8963a6a138d24ec64560818 Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 19 Feb 2025 17:38:08 +0000 Subject: [PATCH 2/5] chore: update tests --- .../reqresp/rate-limiter/rate_limiter.test.ts | 42 +++++++++---------- .../p2p/src/services/reqresp/reqresp.test.ts | 4 +- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.test.ts b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.test.ts index 30ab47fcff66..64d6f6b205fa 100644 --- a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.test.ts +++ b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.test.ts @@ -6,7 +6,7 @@ import { type MockProxy, mock } from 'jest-mock-extended'; import { type PeerScoring } from '../../peer-manager/peer_scoring.js'; import { ReqRespSubProtocol, type ReqRespSubProtocolRateLimits } from '../interface.js'; -import { RequestResponseRateLimiter } from './rate_limiter.js'; +import { RateLimitStatus, RequestResponseRateLimiter } from './rate_limiter.js'; class MockPeerId { private id: string; @@ -57,24 +57,24 @@ describe('rate limiter', () => { const peerId = makePeer('peer1'); // Expect to allow a burst of 5, then not allow for (let i = 0; i < 5; i++) { - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.DeniedPeer); // Smooth requests for (let i = 0; i < 5; i++) { jest.advanceTimersByTime(200); - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.DeniedPeer); // Reset after quota has passed jest.advanceTimersByTime(1000); // Second burst for (let i = 0; i < 5; i++) { - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.DeniedPeer); // Spy on the peer manager and check that penalizePeer is called expect(peerScoring.penalizePeer).toHaveBeenCalledWith(peerId, PeerErrorSeverity.HighToleranceError); @@ -84,34 +84,34 @@ describe('rate limiter', () => { // Initial burst const falingPeer = makePeer('nolettoinno'); for (let i = 0; i < 10; i++) { - expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer(`peer${i}`))).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer(`peer${i}`))).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, falingPeer)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, falingPeer)).toBe(RateLimitStatus.DeniedGlobal); // Smooth requests for (let i = 0; i < 10; i++) { jest.advanceTimersByTime(100); - expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer(`peer${i}`))).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer(`peer${i}`))).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, falingPeer)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, falingPeer)).toBe(RateLimitStatus.DeniedGlobal); // Reset after quota has passed jest.advanceTimersByTime(1000); // Second burst for (let i = 0; i < 10; i++) { - expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer(`peer${i}`))).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer(`peer${i}`))).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, falingPeer)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, falingPeer)).toBe(RateLimitStatus.DeniedGlobal); }); it('Should reset after quota has passed', () => { const peerId = makePeer('peer1'); for (let i = 0; i < 5; i++) { - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.Allowed); } - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(false); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.DeniedPeer); jest.advanceTimersByTime(1000); - expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.Allowed); }); it('Should handle multiple protocols separately', () => { @@ -143,22 +143,22 @@ describe('rate limiter', () => { // Protocol 1 for (let i = 0; i < 5; i++) { - expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(true); + expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.Allowed); } - expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(false); + expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.TX, peerId)).toBe(RateLimitStatus.DeniedPeer); // Protocol 2 for (let i = 0; i < 2; i++) { - expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.PING, peerId)).toBe(true); + expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.PING, peerId)).toBe(RateLimitStatus.Allowed); } - expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.PING, peerId)).toBe(false); + expect(multiProtocolRateLimiter.allow(ReqRespSubProtocol.PING, peerId)).toBe(RateLimitStatus.DeniedPeer); multiProtocolRateLimiter.stop(); }); it('Should allow requests if no rate limiter is configured', () => { const rateLimiter = new RequestResponseRateLimiter(peerScoring, {} as ReqRespSubProtocolRateLimits); - expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer('peer1'))).toBe(true); + expect(rateLimiter.allow(ReqRespSubProtocol.TX, makePeer('peer1'))).toBe(RateLimitStatus.Allowed); }); it('Should smooth out spam', () => { diff --git a/yarn-project/p2p/src/services/reqresp/reqresp.test.ts b/yarn-project/p2p/src/services/reqresp/reqresp.test.ts index 50693859a100..29ee02b11c0d 100644 --- a/yarn-project/p2p/src/services/reqresp/reqresp.test.ts +++ b/yarn-project/p2p/src/services/reqresp/reqresp.test.ts @@ -143,7 +143,9 @@ describe('ReqResp', () => { expect(rateLimitResponse).toBeDefined(); // Make sure the error message is logged - const errorMessage = `Rate limit exceeded for ${ReqRespSubProtocol.PING} from ${nodes[0].p2p.peerId.toString()}`; + const errorMessage = `Rate limit exceeded DeniedPeer for ${ + ReqRespSubProtocol.PING + } from ${nodes[0].p2p.peerId.toString()}`; expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining(errorMessage)); }); From 8857c65be52bd4fa2204121fb704d315b4268636 Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 19 Feb 2025 17:39:47 +0000 Subject: [PATCH 3/5] chore: simplify penalize condition --- .../src/services/reqresp/rate-limiter/rate_limiter.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts index c9508c4f0fd6..13949878c15c 100644 --- a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts +++ b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts @@ -209,15 +209,10 @@ export class RequestResponseRateLimiter { } const rateLimitStatus = limiter.allow(peerId); - switch (rateLimitStatus) { - case RateLimitStatus.DeniedPeer: - // Hitting a peer specific limit, we should lightly penalise the peer - this.peerScoring.penalizePeer(peerId, PeerErrorSeverity.HighToleranceError); - return RateLimitStatus.DeniedPeer; - default: - // Hitting a global limit, we should not penalise the peer - return rateLimitStatus; + if (rateLimitStatus === RateLimitStatus.DeniedGlobal) { + this.peerScoring.penalizePeer(peerId, PeerErrorSeverity.HighToleranceError); } + return rateLimitStatus; } cleanupInactivePeers() { From 321e5c12b3c97ab06e877aad4abc5a6127752d4c Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 19 Feb 2025 18:20:59 +0000 Subject: [PATCH 4/5] fix: condition typo --- .../p2p/src/services/reqresp/rate-limiter/rate_limiter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts index 13949878c15c..669e8d5b05c5 100644 --- a/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts +++ b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts @@ -209,7 +209,7 @@ export class RequestResponseRateLimiter { } const rateLimitStatus = limiter.allow(peerId); - if (rateLimitStatus === RateLimitStatus.DeniedGlobal) { + if (rateLimitStatus === RateLimitStatus.DeniedPeer) { this.peerScoring.penalizePeer(peerId, PeerErrorSeverity.HighToleranceError); } return rateLimitStatus; From 3b52ec75289b888e68d0b9f1cf66969c10eb24bc Mon Sep 17 00:00:00 2001 From: Maddiaa0 <47148561+Maddiaa0@users.noreply.github.com> Date: Wed, 19 Feb 2025 18:16:48 +0000 Subject: [PATCH 5/5] fix: instance name truncated to 50 ( support long branch names ) --- ci3/bootstrap_ec2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci3/bootstrap_ec2 b/ci3/bootstrap_ec2 index edbc474fe72d..7545040d4798 100755 --- a/ci3/bootstrap_ec2 +++ b/ci3/bootstrap_ec2 @@ -39,7 +39,7 @@ if [ "$REF_NAME" == "master" ]; then # Allow parallelism on master by having the instance name be the commit. instance_name="$current_commit"_$arch else - instance_name=$(echo -n "$REF_NAME" | tr -c 'a-zA-Z0-9-' '_')_$arch + instance_name=$(echo -n "$REF_NAME" | head -c 50 | tr -c 'a-zA-Z0-9-' '_')_$arch fi [ -n "${INSTANCE_POSTFIX:-}" ] && instance_name+="_$INSTANCE_POSTFIX"