Move port allocation in network vat to PortAllocator #9228

iomekam · 2024-04-12T17:14:07Z

closes: #9165

Description

bindPort gives a caller too much power to specify exactly what port they want allocated. With this change, we create a central object, PortAllocator that handles handing out ports to the caller. As of today, we are allocating ports for:

/ibc-port
/ibc-port/icacontroller
/ibc-port/pegasus

Security Considerations

This will prevent squatting, where callers can come and claim certain ports that they have no association with

Documentation Considerations

We've removed the bindPort method from the network vat. All the contracts inside of agoric-sdk have been changed to use portAllocator

Testing Considerations

I've included some unit test testing the new allocate APIs

packages/network/src/network.js

dckc · 2024-04-12T17:20:56Z

packages/vats/src/proposals/network-proposal.js

@@ -168,6 +168,7 @@ export const getManifestForNetwork = (_powers, { networkRef, ibcRef }) => ({
        zoe: 'zoe',
        provisioning: 'provisioning',
        vatUpgradeInfo: true,
+        portAllocator: 'portAllocator',


This isn't write up yet, I guess?

0xpatrickdev · 2024-04-18T21:32:04Z

packages/vats/src/proposals/network-proposal.js

@@ -168,6 +168,7 @@ export const getManifestForNetwork = (_powers, { networkRef, ibcRef }) => ({
        zoe: 'zoe',
        provisioning: 'provisioning',
        vatUpgradeInfo: true,
+        portAllocator: 'portAllocator',


Should this go in produce instead of consume? Then in vat-orchestration, we can consume portAllocator instead of networkVat?

cloudflare-workers-and-pages · 2024-04-22T18:39:11Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`aa5e919`
Status:	✅ Deploy successful!
Preview URL:	https://4437662c.agoric-sdk.pages.dev
Branch Preview URL:	https://iomekam-port-allocation.agoric-sdk.pages.dev

View logs

michaelfig

The single-character portId (the 1 in /ibc-port/1) needs to be fixed.

Maybe my hopes for a solution to vanity port names vs. squatting can't be satisfied. Please spend a little time thinking about that, but if there are no clear solutions, let's rip out the Pegasus special case and have it listen on just a generated allocateIBCPort instead.

packages/network/src/network.js

michaelfig · 2024-04-24T02:07:55Z

packages/network/test/test-network-misc.js

+  const portAllocator = makePortAllocator({ protocol });
+
+  const ibcPort = await when(portAllocator.allocateIBCPort());
+  t.is(ibcPort.getLocalAddress(), '/ibc-port/1');


This doesn't look like the correct result (the IBC standard states that port identifiers need to be at least 2 characters in length). The local address should be /ibc-port/port-1. Maybe a port- prefix got dropped somewhere in the PRs?

We actually mock the generatePortID method, and in that implementation, we drop the port prefix. I've updated it to be inline with what we do in production

michaelfig · 2024-04-24T02:19:51Z

packages/pegasus/src/proposals/core-proposal.js

-  const port = await E(networkVat).bindPort('/ibc-port/pegasus');
+  const port = await E(portAllocator).allocateIBCPegasusPort();


This makes me somewhat uncomfortable. It seems to illustrate that allocateIBCPegasusPort() is too specific and doesn't really solve the need for custom port names. Is there a solution that exists between the power of bindPort() and the rigidity of allocateIBCPegasusPort()?

michaelfig · 2024-04-24T02:24:43Z

packages/vats/src/proposals/network-proposal.js

- * - loopback ports: `E(networkVat).bindPort('/local/')`
- * - an echo port: `E(vats.network).bindPort('/ibc-port/echo')`
+ * - loopback ports: `E(portAllocator).allocateLocalPort()`
+ * - an echo port: `E(portAllocator).allocateIBCPort()`


This doesn't produce /ibc-port/echo, so it loses the port identity that the outside world could rely on (like how ICS-20 is usually found at /ibc-port/transfer).

0xpatrickdev · 2024-04-24T13:45:09Z

packages/orchestration/src/types.d.ts

+export type AttenuatedPortAllocator = Pick<
+  PortAllocator,
+  'allocateICAControllerPort'
+>;


I think we can get rid of this typedef (and Far('PortAllocator', ...) in orchestration-proposal.js) and just use PortAllocator directly. Feel free to leave as is and we can make this change in the course of #9198 where we'll need to add allocateICQControllerPort.

0xpatrickdev · 2024-04-24T13:45:39Z

packages/vats/src/proposals/network-proposal.js

@@ -152,7 +151,7 @@ export const setupNetworkProtocols = async (
  await registerNetworkProtocols(vats, dibcBridgeManager);

  // Add an echo listener on our ibc-port network (whether real or virtual).
-  const echoPort = await when(E(vats.network).bindPort('/ibc-port/echo'));
+  const echoPort = await when(E(allocator).allocateIBCPort());


This looks like it may be a change in behavior - /ibc-port/echo -> /ibc-port/1 - is this intentional/expected?

Intentional in that it fits with the design, but not intentional in that it breaks the ability to bind to a known /ibc-port/echo port. Trying to iterate on a solution where we can still provision this port without having access to bindPort

packages/network/src/types.js

Co-authored-by: Michael FIG <mfig@agoric.com>

…mekam-port-allocation

michaelfig

Looking good! I'd just like to see some different names, so that the "custom" address is clearly apparent:

allocateCustomLocalPort
allocateCustomIBCPort

packages/network/src/network.js

dckc reviewed Apr 12, 2024

View reviewed changes

packages/network/src/network.js Outdated Show resolved Hide resolved

dckc reviewed Apr 12, 2024

View reviewed changes

packages/network/src/network.js Outdated Show resolved Hide resolved

dckc reviewed Apr 12, 2024

View reviewed changes

0xpatrickdev reviewed Apr 18, 2024

View reviewed changes

iomekam added 3 commits April 23, 2024 12:32

feat: Allocate ports with PortAllocator

837c632

fix: add tests

84b4fd4

feat: add portAllocator to promise space

787fb5b

iomekam force-pushed the iomekam-port-allocation branch from f4c84ec to 787fb5b Compare April 23, 2024 17:32

iomekam added 4 commits April 23, 2024 12:53

fix: correct zone

1dc5cef

fix: linting

4a12122

fix: use portAllocator instead of networkVat

38bcf1f

fix: styling

6cb9010

iomekam changed the title ~~Deprecate bindPort for PortAllocator in network vat~~ Remove bindPort for PortAllocator in network vat Apr 23, 2024

iomekam changed the title ~~Remove bindPort for PortAllocator in network vat~~ Move port allocation in network vat to PortAllocator Apr 23, 2024

iomekam added 3 commits April 23, 2024 13:59

fix: documentation

f9d8ce1

fix: remove network vat from restart

f7a4fac

feat: add local port allocation

988253b

iomekam marked this pull request as ready for review April 23, 2024 20:50

iomekam requested a review from michaelfig April 23, 2024 20:51

michaelfig requested changes Apr 24, 2024

View reviewed changes

0xpatrickdev reviewed Apr 24, 2024

View reviewed changes

0xpatrickdev mentioned this pull request Apr 24, 2024

feat(orchestration): icq balance query #9198

Merged

iomekam and others added 6 commits April 25, 2024 20:49

Apply suggestions from code review

36b520f

Co-authored-by: Michael FIG <mfig@agoric.com>

Merge branch 'master' of https://github.com/Agoric/agoric-sdk into io…

fce3edb

…mekam-port-allocation

refactor: PR feedback

96b8508

feat: add param to allocateIBCPort

252a503

add IBC port name validation

7e69bed

feat: add validation of port name

a7993a8

iomekam requested a review from michaelfig April 27, 2024 00:40

michaelfig approved these changes Apr 29, 2024

View reviewed changes

packages/network/src/network.js Outdated Show resolved Hide resolved

packages/network/src/network.js Outdated Show resolved Hide resolved

refactor: add custom to name

4a64a8a

iomekam added the automerge:squash Automatically squash merge label Apr 30, 2024

Merge branch 'master' into iomekam-port-allocation

aa5e919

mergify bot merged commit d15cb41 into master Apr 30, 2024
67 checks passed

mergify bot deleted the iomekam-port-allocation branch April 30, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move port allocation in network vat to PortAllocator #9228

Move port allocation in network vat to PortAllocator #9228

iomekam commented Apr 12, 2024 •

edited

Loading

dckc Apr 12, 2024

0xpatrickdev Apr 18, 2024

cloudflare-workers-and-pages bot commented Apr 22, 2024 •

edited

Loading

michaelfig left a comment

michaelfig Apr 24, 2024

iomekam Apr 26, 2024

michaelfig Apr 24, 2024

michaelfig Apr 24, 2024

0xpatrickdev Apr 24, 2024

0xpatrickdev Apr 24, 2024

iomekam Apr 26, 2024

michaelfig left a comment

		const port = await E(networkVat).bindPort('/ibc-port/pegasus');
		const port = await E(portAllocator).allocateIBCPegasusPort();

Move port allocation in network vat to PortAllocator #9228

Move port allocation in network vat to PortAllocator #9228

Conversation

iomekam commented Apr 12, 2024 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Apr 22, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

michaelfig left a 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

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

iomekam commented Apr 12, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 22, 2024 •

edited

Loading