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/592: Validate length and checksum of fetched masp param #1143

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

jurevans
Copy link
Collaborator

@jurevans jurevans commented Sep 27, 2024

Resolves #592

This PR validates the fetched MASP param before storing it. This will reject the promise and prevent an invalid param from being stored should length or checksum not match.

TESTING

  • In packages/shared/lib/src/sdk/mod.ts
    const MASP_PARAM_ATTR: Record<
    MaspParam,
    { length: number; sha256sum: string }
    > = {
    [MaspParam.Output]: {
    length: 16398620,
    sha256sum:
    "ed8b5d354017d808cfaf7b31eca5c511936e65ef6d276770251f5234ec5328b8",
    },
    [MaspParam.Spend]: {
    length: 49848572,
    sha256sum:
    "62b3c60ca54bd99eb390198e949660624612f7db7942db84595fa9f1b4a29fd8",
    },
    [MaspParam.Convert]: {
    length: 22570940,
    sha256sum:
    "8e049c905e0e46f27662c7577a4e3480c0047ee1171f7f6d9c5b0de757bf71f1",
    },
    };
    modify these values to invalidate them (either length or hash, length is checked first!)
  • Rebuild the shared package: yarn wasm:build (Even though these changes are 100% TS, they are a part of shared lib and will not auto-reload in dev mode)
  • Start Namadillo with yarn dev:proxy (bypasses CORS issue with localhost)
  • If you have already successfully downloaded MASP params, just comment out the lines below in useSdk to force a download on next refresh:
    if (hasMaspParams) {
    return masp.loadMaspParams("").catch((e) => console.error(`${e}`));
    }
  • You should see errors for each param that has issues. Currently if any one of these fails, hasMaspParams() will return false, so it will retry all of them on next refresh.
  • Also, you can run yarn dev locally (without the proxy) which will trigger the retry behavior, it is currently configured to retry a total of 3 times with 3 second intervals in between before failing

TODO

  • How should we respond to a MASP param validation failure? One option would be to simply prompt the user to try again, perhaps we could have a warning modal with a Retry button that invokes the download again.

NOTES

  • What could cause an invalid MASP param download?
    • If a download is interrupted due to connectivity issues, the promise will likely just hang
    • If the user is running this locally and overrides the trusted setup params URL, they could be loading invalid params
      • If the length is valid, it will then check the checksum, which must match our constants
    • This feature is for piece of mind, so that the user can be sure they are using the trusted params. Only the trusted params will successfully be stored in IndexedDB

Copy link
Contributor

@pedrorezende pedrorezende left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -13,7 +13,21 @@ import {
} from "react";
import Proxies from "../../scripts/proxies.json";

export const SdkContext = createContext<Sdk | undefined>(undefined);
export enum MaspParamsStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but more an idea. What do you think of using QueryStatus type from @tanstack/react-query instead? So we can make this loading state type standard across the app

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

bytes: Uint8Array;
};

const MASP_PARAM_ATTR: Record<
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment telling from where these numbers / hashes come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, thanks!

@jurevans jurevans force-pushed the feat/592-validate-masp-params branch from 33fcb8e to 43b7a66 Compare October 1, 2024 11:51
@jurevans jurevans force-pushed the feat/592-validate-masp-params branch from 43b7a66 to 1808464 Compare October 1, 2024 11:59
@jurevans jurevans merged commit acb7c0b into main Oct 1, 2024
10 checks passed
@jurevans jurevans deleted the feat/592-validate-masp-params branch October 1, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mechanism for checking correctness of masp-params
2 participants