feat: implement portal_*Ping rpc with ping extension support #64

fearlessfe · 2025-03-18T14:22:21Z

fixed #53

r4f4ss · 2025-03-18T23:26:21Z

I am doing this request, with payloadType=0 without a payload:

{
    "jsonrpc": "2.0",
    "method": "portal_historyPing",
    "params": [
        "enr:-I24QGMQnf1FhP_-tjr7AdT3aJbowJeowuAktBOmoTaxu3WsNPlB1MaD704orcQO8kncLKhEQPOCTv1LSkU27AUldyoEY4d1IDAuMC4xgmlkgnY0gmlwhKRc9-KJc2VjcDI1NmsxoQLJhXByb3LmxHQaqgLDtIGUmpANXaBbFw3ybZWzGqb9-IN1ZHCCE4k",0
    ],
    "id": 0
}

getting the erro:

ERROR[03-18|20:20:04.753] RPC method portal_historyPing crashed: runtime error: invalid memory address or nil pointer dereference
...
WARN [03-18|20:20:04.753] Served portal_historyPing                conn=127.0.0.1:56460 reqid=0 duration="324.719µs" err="method handler crashed"

but I guess it should generate the default payload, according to specs

my trace:

goroutine 1773 [running]:
github.com/ethereum/go-ethereum/rpc.(*callback).call.func1()
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/service.go:199 +0x85
panic({0x13b6560?, 0x22dd160?})
	/home/user/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/runtime/panic.go:787 +0x132
github.com/zen-eth/shisui/portalwire.(*PortalProtocolAPI).Ping(0xc00011e328, {0xc001449ba0, 0xc3}, 0xc002819194, 0x0)
	/media/user/geth/shisui/portalwire/api.go:351 +0xd9
github.com/zen-eth/shisui/history.(*API).HistoryPing(0x4?, {0xc001449ba0?, 0x2?}, 0x2?, 0x0?)
	/media/user/geth/shisui/history/api.go:32 +0x1b
reflect.Value.call({0xc00020cee0?, 0xc00011e470?, 0xc000302ba0?}, {0x159875b, 0x4}, {0xc00270d600, 0x4, 0xc000ad8a98?})
	/home/user/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/reflect/value.go:581 +0xca6
reflect.Value.Call({0xc00020cee0?, 0xc00011e470?, 0x16?}, {0xc00270d600?, 0x10?, 0x47b099?})
	/home/user/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.linux-amd64/src/reflect/value.go:365 +0xb9
github.com/ethereum/go-ethereum/rpc.(*callback).call(0xc000111560, {0x18777f0, 0xc00208a5a0}, {0xc00276dbd8, 0x12}, {0xc00208a5f0, 0x3, 0x4d3a73?})
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/service.go:205 +0x2ec
github.com/ethereum/go-ethereum/rpc.(*handler).runMethod(0xc000a38380?, {0x18777f0?, 0xc00208a5a0?}, 0xc001f630a0, 0x3?, {0xc00208a5f0?, 0x47b099?, 0x30?})
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/handler.go:568 +0x3c
github.com/ethereum/go-ethereum/rpc.(*handler).handleCall(0xc000a217c0, 0xc001f7c720, 0xc001f630a0)
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/handler.go:515 +0x229
github.com/ethereum/go-ethereum/rpc.(*handler).handleCallMsg(0xc000a217c0, 0xc001f7c720, 0xc001f630a0)
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/handler.go:473 +0x22d
github.com/ethereum/go-ethereum/rpc.(*handler).handleNonBatchCall(0xc000a217c0, 0xc001f7c720, 0xc001f630a0)
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/handler.go:299 +0x189
github.com/ethereum/go-ethereum/rpc.(*handler).handleMsg.func1.1(0x18777f0?)
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/handler.go:272 +0x25
github.com/ethereum/go-ethereum/rpc.(*handler).startCallProc.func1()
	/home/user/go/pkg/mod/github.com/optimism-java/shisui@v1.14.6-0.20250316084923-b39f81b66dd2/rpc/handler.go:390 +0xbb
created by github.com/ethereum/go-ethereum/rpc.(*handler).startCallProc in goroutine 1771

Copilot

Pull Request Overview

This PR implements support for payload extension in portal_*Ping RPC calls. Key changes include:

Adding new payload conversion and error handling logic in the ping_ext/rpc.go package.
Updating the API interfaces in portalwire/api.go, beacon/api.go, state/api.go, and history/api.go to accept new payload parameters.
Refactoring the ping functionality in portal_protocol.go to use the new payload processing functions.

Reviewed Changes

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

File	Description
portalwire/ping_ext/rpc.go	Introduces payload JSON-to-SSZ conversion and error types.
portalwire/api.go	Updates Ping signature and payload processing logic.
portalwire/portal_protocol.go	Refactors ping functions to support payloads via pingInnerWithPayload.
beacon/api.go, state/api.go, history/api.go	Updates API functions to pass the new payload parameters.

Copilot · 2025-03-19T05:55:31Z

portalwire/api.go

+			}
+		}
+	}
+


Dereferencing payloadType without confirming it is non-nil can lead to a runtime panic. Consider checking if payloadType is nil and handling that case (e.g. by applying a default value or returning an explicit error).

Suggested change

if payloadType == nil {

return nil, errors.New("payloadType is required")

}

Copilot

Pull Request Overview

This PR implements support for ping extensions in the portal protocols by adding new RPC handlers, error types, and JSON/SSZ conversion functions. Key changes include:

Introducing new payload types and conversions in ping_ext/rpc.go.
Updating the API endpoints (in portalwire/api.go, beacon/api.go, history/api.go, and state/api.go) to accept optional payloadType and payload parameters.
Refactoring ping logic in portal_protocol.go to work with various payload types.

Reviewed Changes

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

File	Description
portalwire/ping_ext/rpc.go	New file defining JSON structures, custom error types, and SSZ conversion functions.
portalwire/api.go	Updated Ping API signature and logic for handling ping payload conversion.
portalwire/portal_protocol.go	Refactored ping logic with payload support and genPayloadByType function added.
beacon/api.go, history/api.go, state/api.go	Updated API methods to include payload parameters.

Copilot · 2025-03-19T10:51:15Z

portalwire/portal_protocol.go

@@ -448,6 +452,37 @@ func (p *PortalProtocol) pingInner(node *enode.Node) (*Pong, []byte, error) {
 	return p.processPong(node, talkResp)
 }

+func (p *PortalProtocol) genPayloadByType(payloadType uint16) ([]byte, error) {


The switch in genPayloadByType lacks a default case to handle unsupported payload types, which may lead to silent failures with a nil payload. Consider adding a default branch that returns an appropriate error (e.g., ErrPayloadTypeIsNotSupported).

Copilot

Pull Request Overview

This PR implements ping RPC extensions by adding support for payload types and their JSON/SSZ conversions.

Added new payload types (ClientInfo, BasicRadius, HistoryRadius) with associated error types and conversion functions.
Updated API endpoints (in portalwire, beacon, state, and history) to accept optional payload parameters and to utilize the new ping extension functions.
Refactored ping-related functions in PortalProtocol to leverage payload type support and streamline payload processing.

Reviewed Changes

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

File	Description
portalwire/ping_ext/rpc.go	Introduces new payload types, error types, and JSON/SSZ conversion support for ping extensions.
portalwire/api.go	Updates the Ping API to accept optional payloadType and payload parameters and returns extended response data.
portalwire/portal_protocol.go	Refactors ping logic by extracting payload-based functions and updating processPong accordingly.
beacon/api.go, state/api.go, history/api.go	Adjusts the API endpoints to pass along the new payload parameters to Ping.

Copilot · 2025-03-21T04:21:39Z

portalwire/portal_protocol.go

+			return nil, err
+		}
+	}
+	return data, nil


The switch statement in the genPayloadByType function does not include a default branch. Adding a default case that returns an error (e.g., ErrPayloadTypeIsNotSupported) would improve robustness in case an unsupported payload type is passed.

Pull Request Overview

This PR implements ping extension support by enabling the portal ping RPC to accept an optional payload type and payload along with the conversion functions between JSON and SSZ formats. Key changes include:

Adding new helper functions in the ping_ext package for JSON↔SSZ conversion.
Updating the PortalProtocolAPI Ping endpoint to optionally accept payloadType and payload.
Adjusting related API endpoints across beacon, state, and history modules to support the new payload parameters.

Reviewed Changes

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

File	Description
portalwire/ping_ext/rpc.go	New functions for JSON and SSZ payload conversion & error definitions.
portalwire/api.go	Updated Ping signature and response structure with payload support.
portalwire/portal_protocol.go	Refactored pingInnerWithPayload and genPayloadByType for payload handling.
beacon/api.go, state/api.go, history/api.go	Updated Ping endpoints to pass the new payload parameters.

Comments suppressed due to low confidence (2)

portalwire/api.go:346

[nitpick] Consider replacing the literal 0 with an explicit constant (e.g., pingext.ClientInfo) to clarify the intended default payload type.

var defaultType uint16 = 0

portalwire/portal_protocol.go:483

[nitpick] For consistency with the rest of the codebase, consider returning pingext.ErrPayloadTypeIsNotSupported{} instead of a custom error message.

return nil, fmt.Errorf("payload type %d is not supported", payloadType)

feat: impl portal_*Ping

e2c60ee

fix: handle empty payload

e1ecf3a

GrapeBaBa requested review from Copilot and GrapeBaBa March 19, 2025 05:54

Copilot AI reviewed Mar 19, 2025

View reviewed changes

fix: panic when payloadType is nil

caede44

GrapeBaBa requested a review from Copilot March 19, 2025 10:50

Copilot AI reviewed Mar 19, 2025

View reviewed changes

GrapeBaBa requested a review from Copilot March 21, 2025 04:20

Copilot AI reviewed Mar 21, 2025

View reviewed changes

fix: add default case

fb7d47d

GrapeBaBa requested a review from Copilot March 22, 2025 10:22

Copilot AI reviewed Mar 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement portal_*Ping rpc with ping extension support #64

feat: implement portal_*Ping rpc with ping extension support #64

fearlessfe commented Mar 18, 2025

r4f4ss commented Mar 18, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI Mar 19, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI left a comment

Copilot AI Mar 19, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI left a comment

Copilot AI Mar 21, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

+	if payloadType == nil {
+		return nil, errors.New("payloadType is required")
+	}

feat: implement portal_*Ping rpc with ping extension support #64

Are you sure you want to change the base?

feat: implement portal_*Ping rpc with ping extension support #64

Conversation

fearlessfe commented Mar 18, 2025

r4f4ss commented Mar 18, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Mar 19, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Mar 19, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Mar 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

r4f4ss commented Mar 18, 2025 •

edited

Loading