Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add faster shield sync #430

Merged
merged 30 commits into from
Nov 12, 2024
Merged

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Oct 17, 2024

Abstract

Add faster shield sync.
This needs pivx shield and PivxNodeController updates

Testing

  • Sync shield wallets and assert that it is faster

Copy link

netlify bot commented Oct 17, 2024

Deploy Preview for cheery-moxie-4f1121 ready!

Name Link
🔨 Latest commit 2bdd937
🔍 Latest deploy log https://app.netlify.com/sites/cheery-moxie-4f1121/deploys/673374c985cb530008c2e37a
😎 Deploy Preview https://deploy-preview-430--cheery-moxie-4f1121.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Duddino Duddino marked this pull request as draft October 17, 2024 19:37
@panleone panleone added the Enhancement New feature or request label Oct 18, 2024
@Duddino Duddino marked this pull request as ready for review October 21, 2024 08:07
@panleone panleone mentioned this pull request Nov 4, 2024
JSKitty
JSKitty previously approved these changes Nov 12, 2024
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 9284d8a

Seriously powerful improvement, speed is subjective, but even in my sub-par speed environment, a 3-4x improvement is minimally visible, I imagine this will be even better for faster connections given there's no more constant TCP-handshake overhead with thousands of requests, but a single stream.

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works really good and sync time is much faster.
I left a couple of nits and a question about why some bytes of the stream are not parsed.

The logic surely needs a big refactor (in particular we should split the part of receiving the stream from the part of parsing it), but it can be done in another PR

scripts/wallet.js Outdated Show resolved Hide resolved
},
blockHeights.length,
batchSize
);
getEventEmitter().emit('shield-sync-status-update', 0, 0, true);
} catch (e) {
debugError(DebugTopics.WALLET, e);
Copy link
Member

@panleone panleone Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for future PR: we have to change this flow

errors finish in the catch block and after that wallet is just marked as synced even if it failed

scripts/wallet.js Outdated Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
panleone
panleone previously approved these changes Nov 12, 2024
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK f8d45ee

Liquid369
Liquid369 previously approved these changes Nov 12, 2024
Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK f8d45ee
LGTM

@Duddino Duddino dismissed stale reviews from Liquid369 and panleone via 2bdd937 November 12, 2024 15:31
Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 2bdd937

Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 2bdd937

Synced, resynced, quit-halfway-and-continued, etc, various times. Looking good!

@panleone panleone merged commit 8eafc23 into PIVX-Labs:master Nov 12, 2024
6 checks passed
@JSKitty JSKitty mentioned this pull request Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

4 participants