feat: vat-safe denomHash using noble sha256 #9576

dckc · 2024-06-24T23:00:09Z

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 · 2024-06-24T23:03:55Z

Deploying agoric-sdk with 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

kriskowal

Should work. Shame about the bundle bloat but I’m sure we’re not waiting for XS native support.

kriskowal · 2024-06-25T22:37:01Z

packages/orchestration/src/utils/denomHash.js

@@ -0,0 +1,30 @@
+// @ts-check
+import { sha256 } from '@noble/hashes/sha256';


Watch out for bundler support for "exports" since this pattern will only work when the tools account for package exports.

is there an alternative?

Not with this package. It’s pretty narrowly prescribed:

https://github.com/paulmillr/noble-hashes/blob/main/package.json#L141-L145

That is, this package cannot be used with bundlers that have not caught up with package exports. There are very few such tools, but node -r esm (loadgen) is among them. Hopefully, there is no risk that this package will get caught up in that, but if you see integration errors to the tune of “I can’t import this”, this is why.

felgercarb!

loadgen ci failed. :-/

now what do I do? sigh.

We could fork or PR upstream to package export package relative paths to files with .js extensions. I’m not against forking this into Endo. We do that for Base64. We could harden the library. We would have recourse to fall through to native on XS if that becomes available.

I’m not against forking this into Endo. We do that for Base64.

I like that idea enough to reify it as an issue:

add sha256 a la base64 endojs/endo#2335

I'll pursue other options in parallel, so it's not necessarily critical path.

loadgen fixed with 58ff687

Downstream imports won't get the patch but they don't need it.

kriskowal · 2024-06-25T22:40:53Z

packages/orchestration/src/utils/denomHash.js

+ return [...new Uint8Array(buffer)]
+ .map(x => x.toString(16).padStart(2, '0'))
+ .join('');


Reasonably concise. An intermediate allocation might be avoidable with Array.prototype.map.call(new Uint8Array(buffer), x => x.toString(16).padStart(2, '0').

turns out @noble/hashes/utils has a bytesToHex, so I'm going with that.

kriskowal · 2024-06-25T22:45:17Z

packages/orchestration/src/utils/denomHash.js

+ path = `${portId}/${channelId}`,
+ denom,
+}) => {
+ const h = sha256.create().update(`${path}/${denom}`).digest();


Could avoid an allocation with digestInto, using the same Uint8Array view for both this and toHex.

dckc · 2024-06-26T17:30:53Z

@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.

0xpatrickdev

The denomHash logic LGTM

kriskowal · 2024-06-26T22:31:04Z

packages/orchestration/src/utils/denomHash.js

@@ -0,0 +1,30 @@
+// @ts-check
+import { sha256 } from '@noble/hashes/sha256';


We could fork or PR upstream to package export package relative paths to files with .js extensions. I’m not against forking this into Endo. We do that for Base64. We could harden the library. We would have recourse to fall through to native on XS if that becomes available.

- @noble/hashes dependency - unit test using bundle Co-Authored-By: Turadg Aleahmad <turadg@agoric.com>

to get paulmillr/noble-hashes@1fddf3d to solve Agoric/testnet-load-generator#115

gibson042 · 2024-07-12T22:12:50Z

@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 · 2024-07-16T22:13:12Z

@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.

dckc requested review from kriskowal and 0xpatrickdev June 24, 2024 23:00

dckc force-pushed the dc-vatSafe-denomHash branch from 66a8aa2 to 28ee394 Compare June 25, 2024 20:01

kriskowal approved these changes Jun 25, 2024

View reviewed changes

dckc marked this pull request as draft June 26, 2024 16:00

dckc force-pushed the dc-vatSafe-denomHash branch from f6d787b to ec76c10 Compare June 26, 2024 17:19

dckc requested a review from kriskowal June 26, 2024 17:30

dckc marked this pull request as ready for review June 26, 2024 17:31

0xpatrickdev approved these changes Jun 26, 2024

View reviewed changes

dckc added the automerge:rebase Automatically rebase updates, then merge label Jun 26, 2024

dckc force-pushed the dc-vatSafe-denomHash branch from ec76c10 to 7e2e90d Compare June 26, 2024 17:57

kriskowal approved these changes Jun 26, 2024

View reviewed changes

This was referenced Jun 27, 2024

lack of package export support (node -r esm) Agoric/testnet-load-generator#115

Open

add sha256 a la base64 endojs/endo#2335

Open

dckc force-pushed the dc-vatSafe-denomHash branch from 7e2e90d to e33b31c Compare June 28, 2024 04:25

turadg force-pushed the dc-vatSafe-denomHash branch 2 times, most recently from 0a9b736 to 92a0751 Compare July 11, 2024 23:22

turadg added force:integration Force integration tests to run on PR and removed automerge:rebase Automatically rebase updates, then merge labels Jul 11, 2024

turadg self-assigned this Jul 12, 2024

dckc and others added 2 commits July 12, 2024 06:41

feat: vat-safe denomHash using noble sha256

dedde44

- @noble/hashes dependency - unit test using bundle Co-Authored-By: Turadg Aleahmad <turadg@agoric.com>

build: source prerelease @noble/hashes

58ff687

to get paulmillr/noble-hashes@1fddf3d to solve Agoric/testnet-load-generator#115

turadg force-pushed the dc-vatSafe-denomHash branch from 92a0751 to 58ff687 Compare July 12, 2024 13:42

turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 12, 2024

mergify bot merged commit 24528f1 into master Jul 12, 2024
81 of 86 checks passed

mergify bot deleted the dc-vatSafe-denomHash branch July 12, 2024 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: vat-safe denomHash using noble sha256 #9576

feat: vat-safe denomHash using noble sha256 #9576

dckc commented Jun 24, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 24, 2024 •

edited

Loading

kriskowal left a comment

kriskowal Jun 25, 2024

dckc Jun 25, 2024

kriskowal Jun 25, 2024 •

edited

Loading

dckc Jun 26, 2024

kriskowal Jun 26, 2024

dckc Jun 27, 2024

turadg Jul 12, 2024

kriskowal Jun 25, 2024

dckc Jun 26, 2024

kriskowal Jun 25, 2024

dckc commented Jun 26, 2024

0xpatrickdev left a comment

kriskowal Jun 26, 2024

gibson042 commented Jul 12, 2024

dckc commented Jul 16, 2024

		@@ -0,0 +1,30 @@
		// @ts-check
		import { sha256 } from '@noble/hashes/sha256';

feat: vat-safe denomHash using noble sha256 #9576

feat: vat-safe denomHash using noble sha256 #9576

Conversation

dckc commented Jun 24, 2024 • edited Loading

Description

Scaling Considerations

Security Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jun 24, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal Jun 25, 2024 • edited Loading

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

dckc commented Jun 26, 2024

0xpatrickdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Jul 12, 2024

dckc commented Jul 16, 2024

dckc commented Jun 24, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 24, 2024 •

edited

Loading

kriskowal Jun 25, 2024 •

edited

Loading