Add limit by thinkAfCod · Pull Request #185 · zen-eth/shisui

thinkAfCod · 2025-05-23T07:40:13Z

No description provided.

Copilot

Pull Request Overview

This PR introduces a limit on the outbound permit release behavior in the PortalProtocol's content lookup and reorganizes some definitions for clarity. Key changes include refactoring the handleFindContent functionality into two functions, updating the dependency version in go.mod, and moving constants and type definitions for better grouping.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
portalwire/portal_protocol.go	Refactored and reorganized functions and constants; introduced a new helper function for content lookup limits
go.mod	Upgraded the utp-go dependency and updated the replacement directive

Copilot · 2025-05-23T13:27:04Z

+						var n int
+						n, err = conn.Write(writeCtx, content)
+						conn.Close()
+						permit.Release()


The permit.Release() call appears both in a defer statement and explicitly within the same goroutine, which may lead to a double release. Consider removing the explicit call to rely solely on the deferred release.

Suggested change

permit.Release()

Copilot

Pull Request Overview

This PR refactors UTP connection permits by introducing a Permit enum, replacing the old Release() interface, and updating related tests and rate-limit logic.

Replace ReleasePermit interface with Permit enum and associated controller methods.
Update tests to use new getPermit/releasePermit API.
Refactor portal protocol and API to use the new permit-based rate-limiting and bump DefaultUtpConnSize.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
portalwire/utp_transport.go	Introduce `Permit` enum, refactor controller methods.
portalwire/utp_transport_test.go	Update tests to call `getPermit`/`releasePermit`.
portalwire/portal_protocol.go	Integrate permit API, update rate-limit logic.
portalwire/portal_protocol_test.go	Adjust tests for new `Permit`, add rate-limit test.
portalwire/api.go	Update API calls to pass `PermitNotLimit`.
go.mod	Bump `utp-go` dependency version.

Comments suppressed due to low confidence (5)

portalwire/portal_protocol_test.go:1040

The test name 'TestAcceptCode_Ratelmit' has a typo. Rename to 'TestAcceptCode_RateLimit' for clarity.

func TestAcceptCode_Ratelmit(t *testing.T) {

portalwire/utp_transport.go:35

[nitpick] The field name getInboundLimit suggests it returns a limit, but it returns a Permit. Consider renaming to getInboundPermit to match its return type.

getInboundLimit  func() Permit

portalwire/utp_transport.go:112

The comment for GetOutboundPermit incorrectly starts with GetInboundPermit. Update it to reflect GetOutboundPermit and its behavior.

// GetInboundPermit tries to acquire a permit for outbound UTP connections.

portalwire/portal_protocol.go:107

The 'overheadvar' text merges two declarations and will not compile. Split the enrOverhead comment and the expirationVersionMinutes declaration onto separate lines.

enrOverhead              = 4               // per added ENR, 4 bytes offset overheadvar expirationVersionMinutes = 5 * time.Minute // cache versionsCache expiration time in minutes

portalwire/portal_protocol_test.go:13

The test uses fmt.Sprintf and hexutil.Encode but neither fmt nor github.com/ethereum/go-ethereum/common/hexutil are imported. Add the missing imports to make the test compile.

"sync/atomic"

Copilot · 2025-05-25T03:21:58Z

-			_, err := p.offer(offerRequestWithNode.Node, offerRequestWithNode.Request, offerRequestWithNode.permit)
+			permit := p.Utp.getOutboundLimit()
+			if permit == PermitReject {
+				p.Log.Debug("utp rate limited, rejecting offer", "offerRequestWithNode", offerRequestWithNode)


When permit == PermitReject, the code logs a rate-limit but still calls p.offer, sending work even when it should be skipped. Add a continue or conditional to skip p.offer on PermitReject.

Suggested change

p.Log.Debug("utp rate limited, rejecting offer", "offerRequestWithNode", offerRequestWithNode)

p.Log.Debug("utp rate limited, rejecting offer", "offerRequestWithNode", offerRequestWithNode)

continue

Copilot · 2025-05-25T03:21:58Z

+	socket           *utp.UtpSocket
+	socketConfig     *utp.ConnectionConfig
+	ListenAddr       string
+	utpControllerRef *utpController


[nitpick] The utpControllerRef field is only used to set function pointers and never directly referenced. Consider removing it to simplify the struct.

Suggested change

utpControllerRef *utpController

Copilot

Pull Request Overview

This PR updates the UTP permit logic to use a simple integer-based Permit type (with constants PermitNotLimit, PermitReject, PermitInbound, and PermitOutbound) instead of an interface-based approach, and adjusts the associated tests, API calls, and UTP transport functionality accordingly.

Refactored permit acquisition and release functions in utp_transport.go.
Updated test functions and API calls to use new Permit constants.
Modified protocol handling functions in portal_protocol.go to incorporate the new permit patterns.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
portalwire/utp_transport_test.go	Updated test helpers to use the new permit API and release functions.
portalwire/utp_transport.go	Refactored UTP controller to use integer-based permits with new limit and release functions.
portalwire/portal_protocol_test.go	Modified tests to replace old permit types with the new Permit constants.
portalwire/portal_protocol.go	Updated offer, processOffer, and related rate limit logic to work with new permit API.
portalwire/api.go	Revised API calls to use new permit constants instead of the deprecated NoPermit type.
go.mod	Updated UTP module version and adjusted the replacement for the go-ethereum dependency.

Copilot · 2025-05-25T04:37:26Z

 }

 func (p *PortalProtocol) offer(node *enode.Node, offerRequest *OfferRequest, permit Permit) ([]byte, error) {
+	permitReleaseInProcessOffer := false


The permit release mechanism using the boolean flag 'permitReleaseInProcessOffer' is somewhat convoluted. Consider refactoring this logic (possibly by encapsulating permit management in a separate helper) to simplify the control flow and reduce the risk of accidental double-releases.

Copilot · 2025-05-25T04:37:27Z

+	permitReleaseInGo := false
 	defer func() {
-		if notStartedUtp {
-			permit.Release()
+		if !permitReleaseInGo {
+			p.Utp.releasePermit(permit)
 		}
 	}()


The use of the 'permitReleaseInGo' flag in processOffer, along with its paired deferred release, makes the permit management logic harder to follow. A refactoring to streamline the permit release flow would improve overall code clarity and maintainability.

Copilot

Pull Request Overview

This PR replaces the previous Permit interface with a Permit enum to manage UTP connection rate limits, refactors UtpTransportService to use function fields for acquiring and releasing permits, and updates the portal protocol to integrate the new permit logic. Tests and API calls are updated to pass the appropriate Permit values.

Refactor utpController to return a Permit enum instead of an interface, and wire it into UtpTransportService.
Update portal protocol’s offer/find-content logic to acquire/release permits.
Adjust tests and API usage to use the new Permit values.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
portalwire/utp_transport.go	Refactored `utpController` to use `Permit` enum and added func fields for permit operations.
portalwire/utp_transport_test.go	Updated tests to match new `Get*Permit` signatures.
portalwire/portal_protocol.go	Integrated permit acquisition/release in offer and find-content flows; moved and updated related constants/types.
portalwire/portal_protocol_test.go	Added rate-limit tests; updated imports for atomic usage.
portalwire/api.go	Changed API calls to pass `PermitNotLimit` instead of `&NoPermit{}`.
go.mod	Bumped `utp-go` dependency and replace directive.

Comments suppressed due to low confidence (5)

portalwire/utp_transport.go:35

[nitpick] The field name getInboundLimit is misleading since it acquires a permit. Consider renaming to acquireInboundPermit or getInboundPermit for clarity.

getInboundLimit  func() Permit

portalwire/utp_transport.go:36

[nitpick] Similarly, getOutboundLimit returns a permit, so renaming to acquireOutboundPermit or getOutboundPermit would better reflect its behavior.

getOutboundLimit func() Permit

portalwire/portal_protocol.go:103

The Tag constant is defined before the ClientTag type is declared, causing a compile error. Move the type ClientTag string declaration above this const.

const Tag ClientTag = "shisui"

portalwire/portal_protocol.go:107

The comment for enrOverhead unintentionally includes the definition of expirationVersionMinutes. Split this into two separate lines to avoid a syntax error.

enrOverhead              = 4               // per added ENR, 4 bytes offset overheadvar expirationVersionMinutes = 5 * time.Minute // cache versionsCache expiration time in minutes

portalwire/portal_protocol_test.go:1028

The test uses fmt.Println and hexutil.Encode but fmt and hexutil are not imported. Add import "fmt" and the appropriate hexutil package.

fmt.Println(hexutil.Encode(content))

Signed-off-by: Chen Kai <281165273grape@gmail.com>

GrapeBaBa requested a review from Copilot May 23, 2025 13:26

Copilot AI reviewed May 23, 2025

View reviewed changes

thinkAfCod force-pushed the add_limit branch from 19d115c to 1a45099 Compare May 24, 2025 08:54

GrapeBaBa requested a review from Copilot May 25, 2025 03:20

Copilot AI reviewed May 25, 2025

View reviewed changes

GrapeBaBa requested a review from Copilot May 25, 2025 04:36

Copilot AI reviewed May 25, 2025

View reviewed changes

GrapeBaBa requested a review from Copilot May 25, 2025 04:49

Copilot AI reviewed May 25, 2025

View reviewed changes

thinkAfCod force-pushed the add_limit branch from 9708d19 to f4f417e Compare May 27, 2025 03:51

GrapeBaBa and others added 20 commits May 31, 2025 18:02

fix:refactor content lookup and trace

56d63bd

Signed-off-by: Chen Kai <281165273grape@gmail.com>

handle find content add limit for utp

205086b

update shisui1 version and utp version

c296f77

fix lint

c36fc8b

fix permit limit

1d6f0e7

modify permit type

bd67624

fix lint

ac81111

fix release limit

6584b38

simply release limit

8112b3a

fix offer worker

1d608af

update shisui1 version

6e33b49

fix shutdown lookup block

7b8c025

no need to close result channel

59e1d32

revert lookup

1f2a799

update version of shisui1 and utp-go

e768cf0

refactor lookup

fa9e0be

do not close result channel

f956c2a

fit lint and move content lookup to lookup.go

85738d6

config discv5 response timeout

62c061c

fix look up content timeout

12b098f

thinkAfCod added 10 commits May 31, 2025 18:02

use pool to handle lookup content

2000973

add pool size for content lookup

564c987

fix talk request leak

b01cf0b

fix talk request can not add ctx

18624e2

update shisui1 version

6bad5b7

update shisui1 version

e024b4f

add logs for state starting a goroutine

8b9f05c

fix active queries count

cc57e81

fix test case and add logs

45e5ef8

add log and adjust metrics point

39c7322

thinkAfCod force-pushed the add_limit branch from e333d93 to 39c7322 Compare May 31, 2025 10:03

thinkAfCod added 8 commits May 31, 2025 18:10

remove find content parameter of ctx

11150f8

add ctx for find content

45f2bdc

update utp-go version

3987734

special log for contentLookup

0d2c4fa

add log and revert shisui1 version

6b23ec5

fix find content method ref

b2df074

lookup content concurrency limit set to 1

9482fda

add log

54dcc8b

Navigation Menu

Uh oh!

Add limit - #185

Add limit#185
thinkAfCod wants to merge 38 commits into
zen-eth:mainfrom
thinkAfCod:add_limit

thinkAfCod commented May 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 25, 2025

Uh oh!

Copilot AI May 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 25, 2025

Uh oh!

Copilot AI May 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	p.Log.Debug("utp rate limited, rejecting offer", "offerRequestWithNode", offerRequestWithNode)
	p.Log.Debug("utp rate limited, rejecting offer", "offerRequestWithNode", offerRequestWithNode)
	continue

Uh oh!

Conversation

thinkAfCod commented May 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants