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" 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/rate-limiter/rate_limiter.ts b/yarn-project/p2p/src/services/reqresp/rate-limiter/rate_limiter.ts index 058fe6e24a0b..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 @@ -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,24 +202,17 @@ 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); - switch (rateLimitStatus) { - 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; - default: - return true; + if (rateLimitStatus === RateLimitStatus.DeniedPeer) { + this.peerScoring.penalizePeer(peerId, PeerErrorSeverity.HighToleranceError); } + return rateLimitStatus; } cleanupInactivePeers() { 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)); }); 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); }