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

Feat: Reduce time when fetching masp #667

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

pnam1609
Copy link

@pnam1609 pnam1609 commented Mar 13, 2024

Issue

Link: Fetching shielded balance via SDK nor extension working

Solution

  • First, Make fetches by block ranges instead of all blocks.
  • Second, Use Promise pool from @supercharge/promise-pool library to process multiple fetch block ranges in parallel. It significantly reduced time when fetching block.
  • Then save the context of the rage block to a new temporary Indexed DB with key as startBlock-EndBlock and value as context after serializing this ranges block.It void loss data if the process is down. Only need fetching again missed ranges block.
  • Once finished fetching and saving all ranges blocks to temporary Indexed DB . Will call a function to get all the data from temporary Indexed DB and load it into the context.
  • The last step is saved to the root db.
  • Also the solution help get the progress of process and display in to user.

Image

  • Button to naviagte shielded sync page
    image
  • Shielded sync page
    image

Note:

@phy-chain
Copy link

Amazing @pnam1609 !
Can i test this locally or is it still too early ?
I was working on different paths to improve masp context for the web app/extension, but you're way ahead of me !
I have a spending key that's fully synced through the CLI, that would be could to see if I can get it sync in the extension too

@@ -40,13 +40,16 @@
"@ledgerhq/hw-transport": "^6.30.0",
"@ledgerhq/hw-transport-webhid": "^6.28.0",
"@ledgerhq/hw-transport-webusb": "^6.28.0",
"@material-ui/core": "^4.12.4",
Copy link

@phy-chain phy-chain Mar 14, 2024

Choose a reason for hiding this comment

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

Is that necessary ? It drags an old version babel/runtime with it, which combine are quite huge, and an open door for potentail security issues through babel runtime : GHSA-67hx-6x53-jw92 (just an example)

@phy-chain
Copy link

Would be nice too if we could load a shielded.dat file generated on a node instance for instance (maybe only in dev ?), that way, we can speed up the process of syncing if we already have a synced context via CLI

@phy-chain
Copy link

phy-chain commented Mar 14, 2024

Ping me on Discord so we can chat more easily, I want to help ! "phychain"

@mateuszjasiuk
Copy link
Collaborator

mateuszjasiuk commented Mar 14, 2024

@pnam1609 I really appriciate the effort. I think it will be hard to merge this soon. The reason is you worked under some assumptions about how things should work and look like. The proper flow would be:

  • create an issue
  • discuss solution in the created issue
  • settle on requirements
  • work on functionality
  • create PR

We plan to create contribution guide soon. This way it will be easier for both you and us to cooperate :)

@HadesGuard
Copy link

Cool, thank you for your hard work. We need it ASAP

@pnam1609
Copy link
Author

Hi @mateuszjasiuk , Thanks for your response. I hope it's available as soon as possible and i can contribute more for project.

@nodeopsdev
Copy link

@pnam1609 Really nice ! I appreciate your comprehensive solution, which outlines a systematic approach to optimize block fetching by implementing block ranges, utilizing Promise pools for parallel processing, and ensuring data integrity through temporary IndexedDB storage. Your solution not only enhances efficiency by reducing fetch times but also provides a robust mechanism for handling interruptions and displaying progress to the user. Awesome valuable contribution.

@mateuszjasiuk
Copy link
Collaborator

mateuszjasiuk commented Mar 22, 2024

@pnam1609

Hello,
I think if we want to merge this quicker we have to chage few things :):

  1. UI - We did not have a plan to create Shielded Sync button. User should not need to remember to do shielded sync every time they want to, for example, see up to date balance. So for the namada.me we call the "sync" each time we query for the shielded balance. So in general I would move all the UI changes to the separate issue so we can discuss it further :)

  2. Changes to Namada repo - I don't think your changes will be merged in near future or at all. On the upside, there is a recent change in the Namada SDK(0.32.0) that allows you to specify the starting index of the fetch function, which should help with your solution.

WDYT?

@pnam1609 pnam1609 closed this Mar 22, 2024
@pnam1609
Copy link
Author

pnam1609 commented Mar 22, 2024

Hi @mateuszjasiuk, Thanks for your reponse.

  1. For this shielded sync button and UI for that. Let me remove this.
  2. I will change the dependency to Namada SDK(0.32.0) and will check to logic code. When it complete, i will ping you later for checking code again.

@pnam1609 pnam1609 reopened this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants