Noble sequence search #215

grod220 · 2024-10-15T12:54:00Z

Related to: #206 & penumbra-zone/web#1855 & penumbra-zone/penumbra#4878

Adds a new Noble client and sequence searcher. Next PR will focus on implementing UI to use within Prax account selector.

grod220 · 2024-10-15T13:03:51Z

Ready for review, but dependent on penumbra-zone/web#1855

JasonMHasperhoven · 2024-10-16T09:48:32Z

packages/noble/src/client.ts

+      accountIndex,
+    );
+
+    console.log({ penumbraAddr: bech32mAddress(penumbraAddr), nobleAddrBech32 });


Is this intended?

additional console.log(res);

debug logs?

Will remove 👍

JasonMHasperhoven

Checks are failing

TalDerei

I think we should wait to merge until there's clarity around the notion of sequence numbers wrapping and what that means in practice. Specifically, if the system exhausts all sequence numbers and begins reusing them, are we certain that user deposits won't be lost or rejected due to an old address being mistakenly reused?

references penumbra-zone/penumbra#4878 (comment)

TalDerei · 2024-10-16T14:22:31Z

packages/noble/src/sequence-search.ts

+// Perform binary search to find the earliest unused noble sequence number
+export const getNextSequence = async ({


Is there a reason the binary search for obtaining a noble sequence number wasn't implemented as a wasm helper?

Because we don't really have any client fetching infra in wasm for making grpc or http requests. We could investigate this, but I'm not sure if it would add value here. Do you disagree?

iirc, I thought core exposes an external rust interface that handles the networking functionality that wasm can consume? maybe I'm wrong, but I don't disagree that it doesn't necessarily add extra value here.

Given the use of tokio, there is not a wasm-compatible request helper I know of.

TalDerei · 2024-10-16T14:25:39Z

Next PR will focus on implementing UI to use within Prax account selector.

flagging for external reviewers: the UI component won't be included until the noble one-time deposit addresses land on Noble mainnet in approximately two weeks, followed by sufficient testing.

TalDerei · 2024-10-16T14:29:57Z

package.json

+  },
+  "pnpm": {
+    "overrides": {
+      "@penumbra-zone/bech32m": "file:///Users/gabe/Desktop/repos/web/packages/bech32m/penumbra-zone-bech32m-8.0.0.tgz",
+      "@penumbra-zone/client": "file:///Users/gabe/Desktop/repos/web/packages/client/penumbra-zone-client-19.0.0.tgz",
+      "@penumbra-zone/crypto-web": "file:///Users/gabe/Desktop/repos/web/packages/crypto/penumbra-zone-crypto-web-25.0.0.tgz",
+      "@penumbra-zone/getters": "file:///Users/gabe/Desktop/repos/web/packages/getters/penumbra-zone-getters-18.0.0.tgz",
+      "@penumbra-zone/perspective": "file:///Users/gabe/Desktop/repos/web/packages/perspective/penumbra-zone-perspective-32.0.0.tgz",
+      "@penumbra-zone/protobuf": "file:///Users/gabe/Desktop/repos/web/packages/protobuf/penumbra-zone-protobuf-6.1.0.tgz",
+      "@penumbra-zone/query": "file:///Users/gabe/Desktop/repos/web/packages/query/penumbra-zone-query-33.0.0.tgz",
+      "@penumbra-zone/services": "file:///Users/gabe/Desktop/repos/web/packages/services/penumbra-zone-services-36.0.0.tgz",
+      "@penumbra-zone/storage": "file:///Users/gabe/Desktop/repos/web/packages/storage/penumbra-zone-storage-32.0.0.tgz",
+      "@penumbra-zone/transport-chrome": "file:///Users/gabe/Desktop/repos/web/packages/transport-chrome/penumbra-zone-transport-chrome-8.0.1.tgz",
+      "@penumbra-zone/transport-dom": "file:///Users/gabe/Desktop/repos/web/packages/transport-dom/penumbra-zone-transport-dom-7.5.0.tgz",
+      "@penumbra-zone/types": "file:///Users/gabe/Desktop/repos/web/packages/types/penumbra-zone-types-24.0.0.tgz",
+      "@penumbra-zone/ui": "file:///Users/gabe/Desktop/repos/web/packages/ui/penumbra-zone-ui-10.0.2.tgz",
+      "@penumbra-zone/wasm": "file:///Users/gabe/Desktop/repos/web/packages/wasm/penumbra-zone-wasm-29.1.0.tgz",
+      "@penumbra-zone/keys": "file:///Users/gabe/Desktop/repos/web/packages/keys/penumbra-zone-keys-4.2.1.tgz"
+    },
+    "peerDependencyRules": {
+      "allowAny": [
+        "@penumbra-zone/bech32m",
+        "@penumbra-zone/client",
+        "@penumbra-zone/crypto-web",
+        "@penumbra-zone/getters",
+        "@penumbra-zone/perspective",
+        "@penumbra-zone/protobuf",
+        "@penumbra-zone/query",
+        "@penumbra-zone/services",
+        "@penumbra-zone/storage",
+        "@penumbra-zone/transport-chrome",
+        "@penumbra-zone/transport-dom",
+        "@penumbra-zone/types",
+        "@penumbra-zone/ui",
+        "@penumbra-zone/wasm",
+        "@penumbra-zone/bech32m",
+        "@penumbra-zone/client",
+        "@penumbra-zone/crypto-web",
+        "@penumbra-zone/getters",
+        "@penumbra-zone/keys",
+        "@penumbra-zone/perspective",
+        "@penumbra-zone/protobuf",
+        "@penumbra-zone/query",
+        "@penumbra-zone/services",
+        "@penumbra-zone/storage",
+        "@penumbra-zone/transport-chrome",
+        "@penumbra-zone/transport-dom",
+        "@penumbra-zone/types",
+        "@penumbra-zone/ui",
+        "@penumbra-zone/wasm"
+      ]
+    }


we shouldn't commit these overrides in the package.json

grod220 · 2024-10-16T14:30:17Z

clarity around the notion of sequence numbers wrapping

Depositing to an already registered forwarding address will automatically "flush" and get forwarded. No funds will be lost. After all sequence numbers are exhausted, I inserted the logic just to pick one at random to return. I've linked this implementation in the protocol PR, so it's waiting on feedback from someone on that team.

I think we should wait to merge

Given there are 2 more PRs to consider this task completed (account selector v2 UI component & service worker polling), I don't think there is any risk in merging and editing later as needed.

TalDerei · 2024-10-16T14:45:44Z

approving optimistically; pending bumping deps and green CI

TalDerei · 2024-10-16T15:11:26Z

packages/noble/src/sequence-search.ts

+  }
+
+  const mid = Math.floor((left + right) / 2);
+  const response = await client.registerAccount({ sequence: mid, accountIndex });


thought experiment: what happens if multiple client instances try to register the same sequence number at the same time, duplicate transactions? does this need some kind of atomic registration attempt / locking?

On no deposited funds, both requests get a NeedsDeposit response back

On deposited funds, the first request gets a Success and the second a AlreadyRegistered

On already registered, both get AlreadyRegistered

That said, don't see a case when multiple clients are in use at the same time

TalDerei · 2024-10-16T15:11:49Z

packages/noble/src/client.ts

+    const client = await StargateClient.connect(this.endpoint);
+
+    try {
+      const res = await client.broadcastTx(tx.toBinary());


maybe an exponential backoff in the try / catch to avoid incomplete registrations?

The caller of this client method would be the ones responsible for re-tries I'd say. It's also easy enough for the user to re-request a noble address if an error is thrown. Can revisit this topic though as the UI comes together and we see how that would work.

TalDerei · 2024-10-16T15:14:24Z

packages/noble/src/sequence-search.ts

+  right: number;
+  client: NobleClientInterface;
+  fvk: FullViewingKey;
+  accountIndex?: number;


Is the account index validated somewhere before attempting registration?

The account index is only used on our side to generate the proper forwarding address. The function to generate the noble address would throw if it something were wrong with the account index.

grod220 · 2024-10-16T16:29:40Z

@JasonMHasperhoven given your first approval, going to interpret good to merge given checks are passing now 👍

grod220 force-pushed the noble-bin-search branch from 0fb93f1 to 70a9988 Compare October 15, 2024 13:02

grod220 requested a review from a team October 15, 2024 13:04

grod220 force-pushed the noble-bin-search branch from 70a9988 to f9ee34b Compare October 15, 2024 13:09

JasonMHasperhoven reviewed Oct 16, 2024

View reviewed changes

JasonMHasperhoven approved these changes Oct 16, 2024

View reviewed changes

JasonMHasperhoven requested changes Oct 16, 2024

View reviewed changes

TalDerei reviewed Oct 16, 2024

View reviewed changes

TalDerei self-requested a review October 16, 2024 14:31

TalDerei approved these changes Oct 16, 2024

View reviewed changes

TalDerei reviewed Oct 16, 2024

View reviewed changes

grod220 force-pushed the noble-bin-search branch from f9ee34b to efcbd5f Compare October 16, 2024 15:31

Noble forwarding addr binary search

e581843

grod220 force-pushed the noble-bin-search branch from efcbd5f to e581843 Compare October 16, 2024 16:06

grod220 merged commit 97fa552 into main Oct 16, 2024
3 checks passed

grod220 deleted the noble-bin-search branch October 16, 2024 16:29

grod220 self-assigned this Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Noble sequence search #215

Noble sequence search #215

grod220 commented Oct 15, 2024 •

edited

Loading

grod220 commented Oct 15, 2024

JasonMHasperhoven Oct 16, 2024

TalDerei Oct 16, 2024

grod220 Oct 16, 2024

JasonMHasperhoven left a comment

TalDerei left a comment •

edited

Loading

TalDerei Oct 16, 2024

grod220 Oct 16, 2024

TalDerei Oct 16, 2024 •

edited

Loading

grod220 Oct 16, 2024

TalDerei commented Oct 16, 2024 •

edited

Loading

TalDerei Oct 16, 2024

grod220 commented Oct 16, 2024 •

edited

Loading

TalDerei commented Oct 16, 2024 •

edited

Loading

TalDerei Oct 16, 2024 •

edited

Loading

grod220 Oct 16, 2024

TalDerei Oct 16, 2024 •

edited

Loading

grod220 Oct 16, 2024

TalDerei Oct 16, 2024

grod220 Oct 16, 2024

grod220 commented Oct 16, 2024

		// Perform binary search to find the earliest unused noble sequence number
		export const getNextSequence = async ({

Noble sequence search #215

Noble sequence search #215

Conversation

grod220 commented Oct 15, 2024 • edited Loading

grod220 commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonMHasperhoven left a comment

Choose a reason for hiding this comment

TalDerei left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

grod220 commented Oct 16, 2024 • edited Loading

TalDerei commented Oct 16, 2024 • edited Loading

TalDerei Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Oct 16, 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

grod220 commented Oct 16, 2024

grod220 commented Oct 15, 2024 •

edited

Loading

TalDerei left a comment •

edited

Loading

TalDerei Oct 16, 2024 •

edited

Loading

TalDerei commented Oct 16, 2024 •

edited

Loading

grod220 commented Oct 16, 2024 •

edited

Loading

TalDerei commented Oct 16, 2024 •

edited

Loading

TalDerei Oct 16, 2024 •

edited

Loading

TalDerei Oct 16, 2024 •

edited

Loading