Conversation
| if (lastNum + msg.getRemainNum() > maxFutureNum) { | ||
| throw new P2pException(TypeEnum.BAD_MESSAGE, "lastNum: " + lastNum + " + remainNum: " | ||
| + msg.getRemainNum() + " > futureMaxNum: " + maxFutureNum); | ||
| long declaredHighestNum = lastNum + msg.getRemainNum(); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| @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(); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
d4807d7 to
4ba5db7
Compare
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.
4ba5db7 to
cd6f6e7
Compare
3 participants
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:
Follow up
Extra details