Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
326 stars 205 forks source link

feat: vat-safe denomHash using noble sha256 #9576

Closed dckc closed 2 months ago

dckc commented 3 months ago

refs: #9211

Description

Prototyping for #9211 suggests we'll want to be derive IBC denoms using a hash of a path such as transfer/channel-0/uatom -> ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2.

I tried to use the sha256 function from @cosmjs/crypto, but I got error's about node's crypto module. @cosmjs/crypto imports an old version of @noble/hashes. The current version seems to be vat-safe.

Scaling Considerations

Compute performance: Doing a sha256 computation on XS in pure JS seems a little whacky. We could consider dropping down to C like we do for base64. But since this is a constant time operation, it doesn't seem worthwhile.

Bundle size: A base64 encoded bundle with just this module seems to be around 500k.

┌──────────────────────────────┬────────┐
│           (index)            │ Values │
├──────────────────────────────┼────────┤
│ @agoric/orchestration-v0.1.0 │  1702  │
│     @noble/hashes-v1.4.0     │ 18820  │
└──────────────────────────────┴────────┘
total size: 36623

Security Considerations

supply chain: this adds a dependency on the current @noble/hashes, which is reasonably popular (3m weekly downloads) and seems to be well audited.

Documentation Considerations

I'm not sure where this shows up in the orchestration API yet.

Testing Considerations

A test that it works in a compartment is included.

Upgrade Considerations

Unrelated to any deployed code.

cloudflare-workers-and-pages[bot] commented 3 months ago

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 58ff687
Status: ✅  Deploy successful!
Preview URL: https://0945a778.agoric-sdk.pages.dev
Branch Preview URL: https://dc-vatsafe-denomhash.agoric-sdk.pages.dev

View logs

dckc commented 3 months ago

@kriskowal @0xpatrickdev PTAAL.

It turns out that after I got it working in a Compartment, I changed the @noble/hashes version to match @cosmjs/crypto... but that version is the reason I didn't import sha256 from @cosmjs/crypto in the first place. Good thing we have ci :)

Also, I flipped the build-vs-buy switch on bytesToHex.

gibson042 commented 2 months ago

@dckc Out of curiosity, how did you measure the bundle size? I'm wondering if it would be reduced by a roll-your-own bytesToHex:

const hexForByte = Array.from({ length: 256 }, (_, i) => i.toString(16).padStart(2, '0'));
const bytesToHex = bytes => {
  let hex = '';
  for (let i = 0; i < bytes.length; i += 1) {
    hex += hexForByte[bytes[i]];
  }
  return hex;
};

(but I suspect not, since sha256.ts also imports from utils.js so the savings would only manifest with fine-grained tree-shaking)

dckc commented 2 months ago

@dckc Out of curiosity, how did you measure the bundle size?

silly me knows better than to not show his work

IIRC, I bundled orchestration/test/utils/denomHashEx.js (or a slight variation) and used a recent improvement to bundle-source to get that table.