fix(net): harden sync message resource controls by xxo1shine · Pull Request #6966 · tronprotocol/java-tron

xxo1shine · 2026-09-11T07:27:34Z

What does this PR do?
Peer-derived sync state could bypass height validation, request rate limiting, and block-fetch deduplication at boundary conditions.

Reject invalid remain counts and overflowed heights, apply sync-chain rate limiting to every request, and size the block ID cache to cover the full valid fetch window.

Fixes #6942

Why are these changes required?

This PR has been tested by:

Unit Tests
Manual Testing

Follow up

Extra details

lxcmyf

[MUST] This review found two blocking issues; see the inline comments.

lxcmyf · 2026-09-17T08:53:21Z

-      if (lastNum + msg.getRemainNum() > maxFutureNum) {
-        throw new P2pException(TypeEnum.BAD_MESSAGE, "lastNum: " + lastNum + " + remainNum: "
-            + msg.getRemainNum() + " > futureMaxNum: " + maxFutureNum);
+      long declaredHighestNum = lastNum + msg.getRemainNum();


[MUST] The overflow check is still inside the headBlockId > 0 branch. At height 0, a message containing genesis plus continuous heights 1..1999 and remainNum = Long.MAX_VALUE skips the check, so the original overflow bypass remains. Always perform a checked addition or overflow check; only the futureMaxNum bound may remain conditional. Add a height=0 test.

This change addresses addition overflow that makes the upper-bound comparison ineffective when height validation is enabled. When headBlockId <= 0, this validation was already skipped as part of the existing initial-sync behavior; it is not an overflow bypass introduced by this PR. We will preserve that branch behavior in this change. Whether initial sync should have an independent check on the declared highest block number can be evaluated separately.

@lxcmyf I resolve it in PR https://github.com/tronprotocol/java-tron/pull/7000/changes.

lxcmyf · 2026-09-17T08:53:21Z

  @Getter
  private Cache<Sha256Hash, Long> syncBlockIdCache = CacheBuilder.newBuilder()
-      .maximumSize(2 * NetConstants.SYNC_FETCH_BATCH_NUM).recordStats().build();
+      .maximumSize(2 * NetConstants.SYNC_FETCH_BATCH_NUM + 1).recordStats().build();


[MUST] Raising maximumSize to 4001 still does not guarantee that all IDs in the active window remain cached: entries survive across moving windows, and Guava segmented eviction can evict entries before the total size reaches the limit. An evicted block still inside [last-4000,last] can be requested again. Maintain an exact height-aware window and test all 4001 IDs plus advancing, out-of-order windows.

The early eviction caused by segmentation is a valid issue. I will add concurrencyLevel(1), retain the capacity of 4001, and add tests covering retention of every ID within a fixed window, out-of-order requests, and rejection of duplicate requests. The goal of this change is to prevent repeated fetching within a fixed window caused by premature cache eviction. Strict deduplication across advancing windows is outside the scope of this change. This path also has request rate limits and a per-request block count limit, so we will not introduce exact height-aware window management at this stage.

Reject negative remain counts and overflowed declared heights when sync height validation is enabled, and apply sync-chain rate limiting to every request. Use a single-segment block ID cache with capacity for the full 4001-height fetch window. Cover complete-window retention, out-of-order requests, and duplicate-request rejection with regression tests.

halibobo1205 · 2026-10-12T09:27:16Z

  private boolean check(PeerConnection peer, SyncBlockChainMessage msg) throws P2pException {
-    if (peer.getRemainNum() > 0
-        && !peer.getP2pRateLimiter().tryAcquire(msg.getType().asByte())) {
+    if (!peer.getP2pRateLimiter().tryAcquire(msg.getType().asByte())) {


[SHOULD] Dropping the remainNum > 0 gate closes the remainNum == 0 bypass, and the default 3.0 is fine. But overflow here is disconnect(BAD_PROTOCOL), i.e. a 1 h ban (PeerConnection:298-302), and it now also applies to in-sync peers. With any rate.limiter.p2p.syncBlockChain below 1.0, honest peers get banned.

The default rate is 3.0. With a low rate-limit configuration, the previous implementation also had this behavior when remainNum > 0.

Agreed that low rates could already cause this when remainNum > 0. However, this change introduces the same failure into a normal completion path where remainNum == 0 throughout.

I reproduced this with syncBlockChain=0.5, a server at height 2, and a client at height 1. The first inventory response has remainNum=0. After processing the final block, SyncService automatically sends another SYNC_BLOCK_CHAIN request to confirm completion. In the test, this arrived 35 ms later and triggered BAD_PROTOCOL with a 3,600,000 ms ban. The same test passes on the parent commit and fails on this PR. It also passes with the default rate of 3.0.

To clarify the earlier comment, rates below 1.0 do not inevitably cause a ban; the request timing matters. The issue is that a legitimate completion sequence can now trigger one.

Please keep unconditional rate limiting, but either explicitly reject unsupported low rates at startup or allow sufficient burst capacity for normal synchronization. A regression test covering the final confirmation with remainNum=0 would help protect this behavior.

I acknowledge that this reproduction shows a legitimate synchronization completion sequence can trigger a ban with a low rate-limit configuration. The default configuration of 3.0 passes this test. This PR primarily addresses the rate-limit bypass; I prefer not to expand its scope to low-rate configuration compatibility or burst-capacity adjustments. I therefore intend to retain the current implementation and leave this configuration edge case for a follow-up.

This change closes a narrow rate-limit bypass, but it introduces a potentially more serious availability issue: with a supported rate below 1.0, a normal sync-completion sequence can trigger BAD_PROTOCOL and a one-hour ban for an honest peer.

I recommend droping this change from the PR. The rate-limit bypass can be addressed separately with a fix that also preserves normal synchronization under supported configurations.

I understand the compatibility concern and acknowledge your reproduction showing that a legitimate final confirmation can be rejected under a low rate-limit configuration. However, restoring the remainNum > 0 condition would allow chain-summary requests to bypass rate limiting again when remainNum is zero, so I prefer to retain unconditional rate limiting. The default configuration of 3.0 passed the reproduction you provided. For burst compatibility under low rate-limit configurations, I still prefer to address configuration constraints or burst capacity separately rather than remove this change from the PR.

317787106 · 2026-10-12T13:39:31Z

          + ", peer: " + blockIds.get(0).getString());
    }

    if (tronNetDelegate.getHeadBlockId().getNum() > 0) {


[SHOULD] The overflow check is still conditional on headBlockId > 0. At height 0, an inventory containing continuous block IDs 0..1999 and remainNum = Long.MAX_VALUE passes validation even though the declared highest height would overflow.

This is a pre-existing edge case, but arithmetic validity does not depend on local sync progress. Please move the overflow check outside this branch and keep only the maxFutureNum comparison conditional. That preserves the existing initial-sync exemption from the time-based height bound while rejecting an unrepresentable declared height. Please also add a height-0 regression test.

This is a pre-existing edge case in the initial synchronization path. This PR fixes the overflow bypass when the local head height is greater than zero. Skipping this check at height zero does not bypass subsequent block validation, and I have not seen evidence of actual system impact from this edge case. I therefore prefer to keep the current scope without adding special handling for height zero.

halibobo1205

LGTM. The default 3.0 is fine, so the low-rate case doesn't block this PR.

For the follow-up you mentioned: setting rate.limiter.p2p.syncBlockChain or rate.limiter.p2p.fetchInvData below 1.0 can get honest peers disconnected and banned, and raising them only weakens the protection, so there is little reason to keep them configurable. Consider removing both keys and keeping 3.0 as a constant, which rules out this kind of misconfiguration, and dropping the 0.5 example in reference.conf.

Heads-up: this PR conflicts with #7000 in ChainInventoryMsgHandler and its test, so whichever lands second needs a rebase.

lxcmyf · 2026-10-15T08:23:56Z

  private Cache<Sha256Hash, Long> syncBlockIdCache = CacheBuilder.newBuilder()
-      .maximumSize(2 * NetConstants.SYNC_FETCH_BATCH_NUM).recordStats().build();
+      .concurrencyLevel(1)
+      .maximumSize(2 * NetConstants.SYNC_FETCH_BATCH_NUM + 1).recordStats().build();


[MUST] This capacity still does not cover every block ID accepted by the handler: eligibility is checked only by encoded height, while this cache is keyed by full hash. KhaosDB can retain a fork block alongside the canonical block at the same height, so an unchanged 4,001-height window can contain 4,002 fetchable IDs; the extra ID evicts an in-window entry and allows it to be fetched again. Restrict sync fetches to IDs advertised for the current sync round, or make deduplication cover all accepted IDs.

The sync inventory is generated by height along the current canonical chain. During normal synchronization, peers request blocks from that inventory, with one block ID per height; they do not enumerate or synchronize every fork block at the same height held in KhaosDB. This cache-capacity adjustment therefore targets the canonical-chain synchronization window, and 4,001 accounts for both endpoints of that height range.

The presence of other fork blocks in KhaosDB does not mean those blocks belong to the current sync inventory. Deliberately requesting fork blocks outside the advertised inventory raises a separate question about request-scope enforcement and additional abuse prevention. This PR does not aim to turn this cache into a deduplication set covering all fork blocks, so I would prefer to retain the current implementation.

[MUST] The inventory is canonical, but the fetch handler does not enforce that scope. An unadvertised hash enters the sync path, is validated only by encoded height, and is served by full hash from KhaosDB, which retains fork blocks. A focused test with 4,001 canonical IDs plus one same-height fork ID evicts a canonical entry and allows it to be fetched again. Bind sync fetches to IDs advertised to this peer, or make deduplication cover every accepted hash.

[MUST] I agree that the generated ChainInventory contains only canonical-chain IDs. The issue is that the receiver does not enforce that invariant: an unadvertised hash enters the sync-fetch path, is validated only by its encoded height, and is resolved by full hash through KhaosDB. This does not require synchronization to select a fork; a peer can directly request a retained fork block outside the advertised inventory. The focused test confirms that this request is served and creates a 4,002nd accepted cache key. Please enforce advertised-inventory membership or cover every hash the handler accepts.

lxcmyf

[MUST] The fixed-window deduplication bypass described inline is blocking. The previously reported height-0 overflow path also remains unresolved.

Check each sync block hash against the canonical height index before adding it to the deduplication cache. This prevents retained fork blocks from evicting entries within a fixed canonical sync window. Treat a noncanonical request as a sync failure so a chain reorganization does not trigger a bad-protocol ban. Preserve advertised block fetching.

Increase the bounded sync block ID cache to 10,000 entries so advancing windows do not prematurely evict still-eligible IDs under out-of-order requests on an unchanged main chain. Consolidate peer cache sizes and the ban duration as private static final constants.

github-actions Bot requested a review from 317787106 September 11, 2026 07:27

halibobo1205 added the topic:net p2p net work, synchronization label Sep 14, 2026

halibobo1205 added this to the GreatVoyage-v4.8.3 milestone Sep 14, 2026

xxo1shine changed the base branch from develop to release_v4.8.3 September 14, 2026 03:25

lxcmyf reviewed Sep 17, 2026

View reviewed changes

xxo1shine force-pushed the fix/net-sync-message-validation branch from d4807d7 to 4ba5db7 Compare September 18, 2026 06:56

xxo1shine force-pushed the fix/net-sync-message-validation branch from 4ba5db7 to cd6f6e7 Compare September 18, 2026 06:57

SeriousCoding789 added this to java-tron Sep 22, 2026

lvs0075 requested a review from halibobo1205 September 22, 2026 09:05

halibobo1205 reviewed Oct 12, 2026

View reviewed changes

317787106 reviewed Oct 12, 2026

View reviewed changes

xxo1shine requested review from 317787106, halibobo1205 and lxcmyf October 14, 2026 08:23

halibobo1205 approved these changes Oct 14, 2026

View reviewed changes

lxcmyf reviewed Oct 15, 2026

View reviewed changes

lxcmyf requested changes Oct 15, 2026

View reviewed changes

xxo1shine requested a review from lxcmyf October 15, 2026 11:05

lxcmyf reviewed Oct 15, 2026

View reviewed changes

Comment thread framework/src/main/java/org/tron/core/net/messagehandler/FetchInvDataMsgHandler.java

lxcmyf approved these changes Oct 16, 2026

View reviewed changes

Conversation

xxo1shine commented Sep 11, 2026

Uh oh!

lxcmyf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lxcmyf Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lxcmyf Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xxo1shine Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

317787106 Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halibobo1205 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lxcmyf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lxcmyf left a comment •

edited

Loading

lxcmyf Sep 17, 2026 •

edited

Loading

lxcmyf Sep 17, 2026 •

edited

Loading

xxo1shine Oct 12, 2026 •

edited

Loading

317787106 Oct 15, 2026 •

edited

Loading