Skip to content

refactor(proto): remove unused proto discover defintion - #7014

Open
317787106 wants to merge 2 commits into
tronprotocol:release_v4.8.3from
317787106:fix/remove_discover_proto
Open

317787106 wants to merge 2 commits into
tronprotocol:release_v4.8.3from
317787106:fix/remove_discover_proto

Conversation

@317787106

@317787106 317787106 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?
close #7013.

  • Remove the unused PingMessage, PongMessage, FindNeighbours, and Neighbours definitions from protocol/src/main/protos/core/Discover.proto.
  • Preserve Endpoint, referenced by HelloMessage.from, and BackupMessage, used by backup keepalive messages, with their existing fields and options.
  • Update the Chinese and English protocol documents to reference p2p/src/main/proto/Discover.proto and align the UDP discovery examples and field descriptions with the implementation, including IPv6 addresses, network identifiers, and response timestamps.

Why are these changes required?

These definitions are redundant leftovers from the libp2p split and are no longer referenced by java-tron's runtime code. UDP discovery uses the separate p2p definitions, brought into this repository in #6992. Removing the legacy definitions reduces maintenance overhead and avoids confusion about the active discovery protocol.

Part of the cleanup tracked in #6921.

@waynercheung waynercheung left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] Please record the pre-change serialization comparison required by #7013, including the commands, fixtures and results, or add fixed regression fixtures for Endpoint, HelloMessage and BackupMessage. The current PR does not document this comparison.

[DISCUSS] Please reconcile #7013's conditional deprecation requirement with the clarification in #6921 that these are unsupported legacy types. Record the agreed classification and describe the generated-type removal and expected descriptor change in this PR's compatibility notes.

Comment thread protocol/src/main/protos/English version of TRON Protocol document.md Outdated
@317787106

317787106 commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

[SHOULD] Please record the pre-change serialization comparison required by #7013, including the commands, fixtures and results, or add fixed regression fixtures for Endpoint, HelloMessage and BackupMessage. The current PR does not document this comparison.

[DISCUSS] Please reconcile #7013's conditional deprecation requirement with the clarification in #6921 that these are unsupported legacy types. Record the agreed classification and describe the generated-type removal and expected descriptor change in this PR's compatibility notes.

Endpoint and BackupMessage retain exactly the same fields and options, and HelloMessage is also unchanged. Given this scope, I don’t think additional serialization tests are necessary for this cleanup.

The four removed legacy message types have been unused by java-tron for years and are not supported external APIs, as clarified in #6921. Their removal therefore does not require a deprecation cycle. The removal of the generated types and the corresponding descriptor changes are intentional and can be documented in the PR’s compatibility notes.

@waynercheung

Copy link
Copy Markdown
Collaborator

Thanks. On the serialization comparison: agreed, no additional permanent regression tests are needed for this cleanup. For the record, I ran the comparison against the merge base 6d5adc4: buf breaking reports only the four removed messages under FILE rules and nothing under WIRE, and protoc --encode produces identical bytes before and after for Endpoint, HelloMessage and BackupMessage, including IPv6, default-value and negative priority cases. This addresses the pre-change serialization comparison requested in #7013. The other two items were also checked on 7875e68 (4b63f5f only changes documentation): protobuf sources regenerated and compiled in a clean worktree, and the related handshake, backup and p2p discovery tests passed (44/44).

[SHOULD] On the classification: agreed. Please add the compatibility note to the PR description before merge, and update the last paragraph of #7013 to match. Suggested text:

Compatibility: The retained messages preserve their existing wire encoding. No runtime networking or consensus changes are introduced. The generated Java types org.tron.protos.Discover.PingMessage, PongMessage, FindNeighbours and Neighbours are removed, and the core/Discover.proto descriptor changes accordingly (BackupMessage moves from message index 5 to 1). Per #6921 these are unsupported legacy types, so no deprecation cycle is required. The active UDP discovery definitions are in p2p/src/main/proto/Discover.proto (org.tron.p2p.protos.Discover), which uses a different Java package and protobuf namespace, so they are not drop-in replacements.

[NIT] Please correct "defintion" to "definition" in the PR title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants