fix(net): correct sync completion and chain summary request timeouts - #92
Merged
Merged
Conversation
…quested.getKey().peekLast()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Add a response timeout for outstanding chain-summary requests and harden chain-inventory validation, while preserving upload-direction state when the local download finishes.
syncChainRequestedagainst the existing five-secondSYNC_TIME_OUT, using the original request timestamp. The existing periodic status check disconnects the responsible peer withTIME_OUTafter the threshold is exceeded, regardless of sync direction flags. PING/PONG, valid inbound requests, and other block progress do not extend this deadline.remainNumand finish the local download. PreserveneedSyncFromUsfor both the summary tail and earlier blocks: an existing upload requirement remains active, and a new one is not inferred from the response.TronState.SYNC_COMPLETEDso a later download can restart. Unknown queued blocks still follow the fetch path, and normal multi-block validation and paged downloads remain in place.Why are these changes required?
The existing block-progress timeout does not provide a deadline tied to an individual chain-summary request. A peer must answer that request even when other traffic or block activity continues.
A single-block response also does not initiate a download on the remote peer. For example, if our summary ends at block 100 and an existing peer replies with the known block 50, setting
needSyncFromUs=truelocally can suppress normal inventory exchange while waiting for a remote download that never starts. Preserving the upload flag keeps BLOCK/TRX inventory exchange compatible with existing peers and retains any upload synchronization already in progress.This PR has been tested by:
ChainInventoryMsgHandlerTest,SyncBlockChainMsgHandlerTest,SyncServiceTest,PeerStatusCheckTest,PeerStatusCheckMockTest,PeerConnectionTest, andInventoryMsgHandlerTest../gradlew checkstyleMain checkstyleTest: passed.p/java,p/security-audit,p/owasp-top-ten): two changed production files, 93 applicable rules, zero findings or parsing errors.Follow up
Ending the local download still trusts the peer's claim that no blocks remain. A peer can echo an earlier known summary block while withholding newer blocks; this PR does not verify the peer's actual head. Chain-inventory responses do not receive block contribution credit.
When integrating with #90, retain both Hello and chain-summary timeout checks in the common status-check flow. Integration assumes the separate libp2p fixes from its
v2.3.0branch and theMessageCountconcurrency fix from another developer's PR are available. No dependency update is included here.Extra details
Targets
release_v4.8.3. Production changes are limited toChainInventoryMsgHandlerandPeerStatusCheck. Public interfaces, wire formats, database formats, and configuration remain unchanged; the implementation uses Java 8-compatible APIs. Active head probing and the block-fetch changes in #91 are outside this PR.