feat(snap): add extra checks; update snap discovery #2860

Vovchyk · 2024-11-22T15:56:46Z

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

github-actions · 2024-11-22T15:57:06Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

fmacleal

praise:

Very good job! I can see that this was a very difficult feature change.
I have left some small comments and also some questions, I would like to have a call to discuss better the changes and understand all the additions.

fmacleal · 2024-11-27T22:08:22Z

rskj-core/src/main/java/co/rsk/net/SnapshotProcessor.java

-        generateChunkRequestTasks(state);
-        startRequestingChunks(state);
+
+        if (blocksVerified(state)) {


question:

What exactly means the method blocksVerified? Does it mean that all blocks were verified?

yes, correct - that the block headers were verified in reverse order till the latest connected block

fmacleal · 2024-11-29T09:46:10Z

rskj-core/src/main/java/co/rsk/net/SnapshotProcessor.java

        state.setRemoteRootHash(lastBlock.getStateRoot());
        state.setRemoteTrieSize(responseMessage.getTrieSize());

-        for (int i = 0; i < blocksFromResponse.size(); i++) {
-            state.addBlock(new ImmutablePair<>(blocksFromResponse.get(i), difficultiesFromResponse.get(i)));
+        if (!validateAndSaveBlocks(state, sender, blocksFromResponse, difficultiesFromResponse)) {


suggestion:

Would be interesting to have a log here to identify that it left the loop with invalid blocks? Or the ones inside the method are already enough?

it's being logged inside that method

fmacleal · 2024-11-29T09:53:57Z

rskj-core/src/main/java/co/rsk/net/SnapshotProcessor.java

+        requestNextBlockHeadersChunk(state, sender);
+    }
+
+    private boolean validateBlockHeaders(SnapSyncState state, Peer sender, List<BlockHeader> blockHeaders) {


issue:

the sender parameter is never used

fmacleal · 2024-11-29T10:08:11Z

rskj-core/src/main/java/co/rsk/net/SnapshotProcessor.java

+        if (blocksVerified(state)) {
+            logger.info("CLIENT - Finished Snap blocks request sending.");
+
+            generateChunkRequestTasks(state);
+            startRequestingChunks(state);
+        } else if (nextChunk > lastRequiredBlock) {
            requestBlocksChunk(sender, nextChunk);
        } else {
            logger.info("CLIENT - Finished Snap blocks request sending.");
+
+            generateChunkRequestTasks(state);
+            startRequestingChunks(state);
+
+            requestNextBlockHeadersChunk(state, sender);
        }


suggestion:

I see that the difference between the first if and the last else it's because we are requiring headers chunk if block is not verified. Maybe it could be simplified and avoid a bit of DRY:

Suggested change

if (blocksVerified(state)) {

logger.info("CLIENT - Finished Snap blocks request sending.");

generateChunkRequestTasks(state);

startRequestingChunks(state);

} else if (nextChunk > lastRequiredBlock) {

requestBlocksChunk(sender, nextChunk);

} else {

logger.info("CLIENT - Finished Snap blocks request sending.");

generateChunkRequestTasks(state);

startRequestingChunks(state);

requestNextBlockHeadersChunk(state, sender);

}

if (nextChunk > lastRequiredBlock) {

requestBlocksChunk(sender, nextChunk);

} else {

logger.info("CLIENT - Finished Snap blocks request sending.");

generateChunkRequestTasks(state);

startRequestingChunks(state);

if(!blocksVerified(state)) {

requestNextBlockHeadersChunk(state, sender);

}

}

fmacleal · 2024-11-29T10:12:11Z

rskj-core/src/main/java/co/rsk/net/SyncProcessor.java

@@ -289,8 +289,8 @@ public void startBlockForwardSyncing(Peer peer) {
    }

    @Override
-    public void startSnapSync() {
-        logger.info("Start Snap syncing");
+    public void startSnapSync(Peer peer) {


question:

Does it mean that we will accept the sync only with a single peer for now right? 🤔
I don't recall if it was possible to receive from multiple peers.

yes, we start with one peer - we send SnapStatusRequestMessage to get initial params for syncing, like state root hash, etc.

fmacleal · 2024-11-29T10:29:36Z

rskj-core/src/test/java/co/rsk/core/bc/BlockValidatorBuilder.java

suggestion:

The previous suggestions to use the any(Class<T>) also applies here.

fmacleal · 2024-11-29T10:29:42Z

rskj-core/src/test/java/co/rsk/core/bc/BlockValidatorTest.java

suggestion:

The previous suggestions to use the any(Class<T>) also applies here.

fmacleal · 2024-11-29T10:30:08Z

rskj-core/src/test/java/co/rsk/core/bc/BlockValidatorBuilder.java

@@ -99,7 +99,7 @@ public BlockValidatorBuilder addBlockUnclesValidationRule(BlockStore blockStore)
        Mockito.when(validationRule.isValid(Mockito.any())).thenReturn(true);

        BlockHeaderParentDependantValidationRule parentValidationRule = Mockito.mock(BlockHeaderParentDependantValidationRule.class);
-        Mockito.when(parentValidationRule.isValid(Mockito.any(), Mockito.any())).thenReturn(true);
+        Mockito.when(parentValidationRule.isValid(Mockito.any(), (Block) Mockito.any())).thenReturn(true);


suggestion:

I would use static import here to import Mockito.

fmacleal · 2024-11-29T10:30:15Z

rskj-core/src/test/java/co/rsk/core/bc/BlockValidatorTest.java

@@ -288,7 +288,7 @@ void invalidPOWUncles() {
        store.saveBlock(uncle1a, TEST_DIFFICULTY, false);

        BlockHeaderParentDependantValidationRule parentValidationRule = mock(BlockHeaderParentDependantValidationRule.class);
-        when(parentValidationRule.isValid(Mockito.any(), Mockito.any())).thenReturn(true);
+        when(parentValidationRule.isValid(Mockito.any(), (Block) Mockito.any())).thenReturn(true);


suggestion:

I would use static import here to import Mockito.

fmacleal · 2024-11-29T10:31:32Z

rskj-core/src/test/java/co/rsk/net/sync/BlockConnectorHelperTest.java

praise:

It looks like the test stayed way simpler. Congrats on the refactor.

sonarqubecloud · 2024-12-05T16:15:18Z

Quality Gate failed

Failed conditions
50.9% Coverage on New Code (required ≥ 55%)

See analysis details on SonarQube Cloud

feat(snap): add extra checks; update snap discovery

31ee435

Vovchyk added 2 commits November 25, 2024 13:30

feat(snap): change headers featching stop condition; cleanup

3161ac6

feat(snap): fixed tests; more refactoring and cleanup

bb0801a

fmacleal reviewed Nov 29, 2024

View reviewed changes

Vovchyk added 4 commits November 29, 2024 14:29

feat(snap): fix block header fetching and snap client config

6d41374

feat(snap): more refactoring

8fe4cc9

feat(snap): update logging and error handling in SnapSyncState

f9ee554

feat(snap): remove snap-state-chunk-specific timeout config

58edd8e

Vovchyk force-pushed the vovchyk/snap/extra-checks branch from 3e9075c to 58edd8e Compare December 5, 2024 15:07

feat(snap): address comments; add some tests

a4acb49

Vovchyk marked this pull request as ready for review December 5, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(snap): add extra checks; update snap discovery #2860

feat(snap): add extra checks; update snap discovery #2860

Vovchyk commented Nov 22, 2024

github-actions bot commented Nov 22, 2024 •

edited

Loading

fmacleal left a comment

fmacleal Nov 27, 2024

Vovchyk Dec 5, 2024

fmacleal Nov 29, 2024

Vovchyk Dec 5, 2024

fmacleal Nov 29, 2024

fmacleal Nov 29, 2024

fmacleal Nov 29, 2024

Vovchyk Dec 5, 2024

fmacleal Nov 29, 2024

fmacleal Nov 29, 2024

fmacleal Nov 29, 2024

fmacleal Nov 29, 2024

fmacleal Nov 29, 2024

sonarqubecloud bot commented Dec 5, 2024

feat(snap): add extra checks; update snap discovery #2860

Are you sure you want to change the base?

feat(snap): add extra checks; update snap discovery #2860

Conversation

Vovchyk commented Nov 22, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

github-actions bot commented Nov 22, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

fmacleal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 5, 2024

Quality Gate failed

github-actions bot commented Nov 22, 2024 •

edited

Loading