[317787106]

What does this PR do?

Fix the sync-stall regression introduced by #6717 by addressing block decoding and sync-request cleanup:

• Reject unmatched top-level END_GROUP tags in BlockMessage(byte[]) with checkLastTagWas(0), before the message reaches BlockMsgHandler. This check applies with protobuf filtering both enabled and disabled.
• Keep defensive cleanup for unexpected deferred deserialization failures: remove the syncBlockInProcess marker, invalidate the provider's global requestBlockIds entry, and disconnect with BAD_BLOCK. Include the provider address in the failure log.

Why are these changes required?

Before this fix, the initial Block.parseFrom(CodedInputStream) call did not verify the final tag. With protobuf filtering disabled, an otherwise valid block followed by an unmatched 0x0c could pass ingress, then fail the deferred BlockCapsule(byte[]) parse.

By then, the request had moved out of syncBlockRequested and into syncBlockInProcess. Logging and returning left the global reservation behind, preventing other peers from supplying the block until cache expiration or eviction; the default TTL is 60 minutes. Disconnect cleanup alone could not release that reservation because it only visits outstanding syncBlockRequested entries.

The new ingress check rejects this payload before that state transition. The request remains outstanding, so existing disconnect cleanup can release it for another peer. The deferred catch also releases sync state defensively if queued-data deserialization unexpectedly fails.

This PR has been tested by:

• 46 focused tests passed on ARM64 / JDK 17 across BlockMessageTest, SyncServiceTest, BlockMsgHandlerTest, PeerStatusCheckTest, PeerStatusCheckMockTest, and SanitizeUnknownFieldsTest.
• Ingress tests reject unmatched END_GROUP tags under both filter settings, including a tag followed by additional bytes, while accepting a legitimate 0x0c inside a bytes field.

Follow up

None.

Extra details

Existing protobuf filtering and contract-format checks remain in place. The new end-tag check validates message structure; it does not compare the original payload with reserialized bytes. A byte value of 0x0c inside a legitimate field remains valid.

The deferred-failure BAD_BLOCK policy follows the existing processSyncBlock() failure handling. The trailing-0x0c network payload is now rejected at ingress and is no longer used to reach that catch.

[bladehan1]

[NIT] Document the two-parse-entry asymmetry and proto-filter precondition at this catch site

This failure path is only reachable because the network-side parse (BlockMessage(byte[]) -> Block.parseFrom(CodedInputStream)) accepts a trailing 0x0c (END_GROUP) byte while the deferred BlockCapsule(byte[]) parse (Block.parseFrom(byte[])) rejects it — verified against protobuf-java 3.25.8. With proto filtering enabled (allowProtoFilterNum == 1), the entry path's Message.compareBytes length check rejects the payload first, so this catch is nearly unreachable there; on filter-disabled networks it is reachable. If a future protobuf upgrade unifies the two parse entries, or a maintainer 'unifies' them by hand, the reachability of this path changes silently and the regression test degrades into testing dead code.

Suggestion: add a short comment above this catch block noting that the second parse is stricter than the entry parse and that a failure here means the supplying peer is not trustworthy; re-verify the regression test semantics whenever protobuf-java is upgraded.

[317787106]

Updated in f12c112. BlockMessage now calls checkLastTagWas(0) after the initial parse, rejecting unmatched top-level END_GROUP tags regardless of the proto-filter setting. A comment at that check explains the EOF requirement.

The deferred-failure regression now explicitly injects damaged queued data to verify defensive cleanup independently of ingress validation. Separate tests cover ingress rejection and legitimate 0x0c bytes inside fields.

[bladehan1]

[DISCUSS] Disconnect on first malformed block — no reputation-score gradient (recording the accepted tradeoff)

Before this fix a malformed-block provider was only logged and stayed connected (while its block pinned the global request for up to 60 minutes); now a single malformed block triggers an immediate BAD_BLOCK disconnect. This matches the existing processSyncBlock() failure policy and the pre-#6717 entry-parse behavior, so the direction is right. The tradeoff worth recording: unlike Bitcoin Core's Misbehaving, the TRON p2p layer has no reputation-score gradient, so a peer that persistently supplies corrupted blocks — e.g. due to a serialization bug or a faulty middlebox rather than malice — will be disconnected and reconnected in a loop.

Impact is low: one bad block means one disconnect, and the reconnect can still serve other blocks; bad-block supply is also bounded by message rate limiting. No change is needed in this PR — recording this so the tradeoff is on the thread. If reconnect churn is ever observed in the field, a follow-up could accumulate disconnect reasons in the p2p layer before escalating to a ban.

[317787106]

Thanks for raising this. One clarification: BAD_BLOCK already calls channel.close(BAD_PEER_BAN_TIME), which records a one-hour IP penalty.

The deferred-failure handling retains the existing processSyncBlock policy. With f12c112, the trailing-0x0c payload is rejected at ingress through the existing parse-error handling and no longer reaches this BAD_BLOCK branch.

[bladehan1]

[NIT] Declare the non-singleton isolation premise for these tests (class-level javadoc)

This test (and the other new SyncService()-based tests in this class) constructs the system under test as a plain local instance and mutates it only via reflection on instance fields; PeerConnection is a prototype-scoped bean, so no Spring singleton or static mutable state is touched. That makes the isolation premise sound, but per the project's test-singleton hygiene rule (H7) the premise must be stated explicitly in the test class javadoc — otherwise every future reviewer has to re-derive it from scratch.

Suggestion: add one line to the class-level javadoc of SyncServiceTest stating that tests constructing fresh SyncService instances share no Spring singleton state and therefore need no @After reset.

[317787106]

SyncServiceTest extends BaseMethodTest, which creates a fresh Spring context for each test method and provides an inherited @After that closes the context and clears Args. The locally constructed SyncService instances are also closed in finally blocks.

That lifecycle is already documented in BaseMethodTest. Some tests here use Spring beans and modify dynamic properties, so a class-level statement that no singleton state is touched would be inaccurate. No additional @After is needed for these additions.

[xxo1shine]

[SHOULD] Please use the same deserialization path as the initial network parse:

block = new BlockMessage(unparsedBlock.getData()).getBlockCapsule();

The reproduced stall comes from using different Protobuf overloads: BlockMessage(byte[]) accepts the trailing 0x0c when filtering is disabled, while the deferred BlockCapsule(byte[]) rejects it. Reusing BlockMessage makes the two parses consistent and preserves its existing filtering and contract-format checks. This directly fixes the reproduced regression without adding a BAD_BLOCK disconnect for a representation already accepted at ingress.

Please adjust the regression to verify that, with filtering disabled, the block is processed successfully and the provider stays connected. With filtering enabled, the payload should still be rejected at the initial parse. Keep the existing catch; cleanup for other deferred-parsing failures can be considered separately.

[317787106]

I disagree with making both parsing paths accept this payload. A block message with an extra, unmatched trailing END_GROUP (0x0c) is malformed and must be rejected. The earlier ingress acceptance was caused by missing end-tag validation and should be fixed.

In f12c112, BlockMessage now calls checkLastTagWas(0) after the initial parse, rejecting this payload under both filter settings before the sync-state transition. Existing filtering and contract-format checks remain unchanged.

The tests verify ingress rejection, disconnect cleanup, and retry through another peer. Deferred-failure cleanup remains as a defensive safeguard, tested separately with damaged queued data.

[xxo1shine]

I understand that an unmatched top-level END_GROUP is malformed and that adding ingress validation has a sound basis. However, this stall occurs because ingress accepts the data while deferred parsing rejects the same bytes. Reusing BlockMessage for deferred parsing would eliminate this difference while preserving the existing acceptance behavior, filtering, and contract-format checks.

I still recommend using the same parsing path in this PR to fix the regression with a minimal change.

If we instead reject the payload at ingress, it will never reach deferred parsing. In that case, adding the ingress check is sufficient to fix this reproduction, and I suggest removing the newly added state cleanup and BAD_BLOCK disconnect from the deferred-parsing catch block for now.

[317787106]

Updated in 7b1150b0b5.

Deferred decoding now uses new BlockMessage(unparsedBlock.getData()).getBlockCapsule(), matching the ingress path. The ingress end-tag check remains, so unmatched top-level END_GROUP payloads are still rejected under both filter settings.

Removed the unconditional BAD_BLOCK disconnect from the deferred catch. The processing-marker and request-reservation cleanup remains because deferred validation can fail after successful ingress: ALLOW_PROTO_FILTER_NUM may change while a block is queued, or waiting for contract validation may be interrupted. Cleanup allows retry without attributing these failures to the provider; a short code comment explains this.

The regression now exercises a filter change after enqueueing: it verifies that deferred rejection releases sync state without disconnecting the provider, then a canonical response from another peer is processed successfully.

Validation: 49 focused tests, clean build -x test, and the CI Checkstyle and Math checks passed.

[bladehan1]

[NIT] Add one line above this catch noting it is a defensive safeguard, now that ingress rejects trailing END_GROUP

With BlockMessage now rejecting unmatched top-level END_GROUP tags at ingress (f12c112), this deferred-parse failure path is no longer reachable via the wire for that payload: every queued UnparsedBlock carries the original bytes, and the ingress and deferred parses now accept the same language. That makes this cleanup-plus-disconnect look redundant to a reader (and it is the point xxo1shine is pressing on in this thread). It is still worth keeping as defense in depth — it guards against future queue-source changes or a parse-entry divergence silently re-opening the #6717 60-minute stall — but nothing in the code says so, so each future reviewer will re-derive the same debate.

Not blocking; a single line here would settle both the reachability question and the "is this dead code" concern.

---

Suggestion: Add one comment line above the catch, e.g.: "Ingress validation rejects malformed payloads; this cleanup is a defensive safeguard for queued data damaged after receipt or for future divergence between the two parse entries — do not remove without re-checking the #6717 stall recurrence."

[317787106]

Added in 7b1150b0b5. The comment now explains that deferred validation may fail after successful ingress—for example, if ALLOW_PROTO_FILTER_NUM changes or waiting for contract validation is interrupted—and that releasing sync state allows retry.

Deferred decoding now also reuses BlockMessage. The unconditional BAD_BLOCK disconnect has been removed; the regression covers a filter change after enqueueing and verifies cleanup and retry without disconnecting the provider.