`tmpnet`: Add support for subnets #2492

marun · 2023-12-15T19:36:14Z

Add subnet support to tmpnet fixture to support both in-repo subnet testing and tmpnet usage by subnet-evm.

Follow-on PRs that use this code include:

e2e: Add basic warp test with xsvm #2043
vms: Migrate timestampvm #2493
- e2e is passing (validating tmpnet's subnet support), but a bit more work is required to get this ready for review
e2e: Switch to the tmpnet fixture subnet-evm#1027
- e2e tests are capable of passing locally, need to figure out why that isn't replicating in CI

TODO

Update README to reflect subnet support
Improve output of subnet creation process

abi87

left a few questions

abi87 · 2024-01-09T14:58:23Z

tests/fixture/e2e/env.go

-	}
+	Env = &TestEnvironment{}
 	require.NoError(json.Unmarshal(envBytes, Env))
+	Env.require = require


q: why this change? It's fine, just wonder why.

It's a subtle change, but it avoids the problem of the unmarshaled value overwriting the local instance of require with an unusable value.

Because require is a non-exported variable, the unmarshaller won't touch it. But I think this does probably make things more clear.

abi87 · 2024-01-09T15:02:14Z

tests/fixture/e2e/env.go

 	} else {
-		network = StartNetwork(flagVars.AvalancheGoExecPath(), DefaultNetworkDir)
+		network = desiredNetwork


q: looks to me as if we don't really need to do the tmpnet.ReadNetwork above if desiredNetwork is specified. If this is correct can we just skip it?

If networkDir is specified, then it's necessary to read the network from that path. desiredNetwork is then only used to configure subnets. This supports using an existing network (e.g. --use-existing-network) to reduce the setup cost for iterative test development (e.g. setup network once, use --use-existing-network to reuse it for multiple suite invocations).

tests/fixture/e2e/helpers.go

abi87 · 2024-01-09T15:06:03Z

tests/fixture/tmpnet/README.md

+            │   └── raZ51bwfepaSaZ1MNSRNYNs3ZPfj...U7pa3
+            │       └── config.json                      // Custom chain configuration for all nodes


tests/fixture/tmpnet/network.go

abi87 · 2024-01-09T15:21:25Z

tests/fixture/tmpnet/utils.go

@@ -87,3 +87,12 @@ func NewPrivateKeys(keyCount int) ([]*secp256k1.PrivateKey, error) {
 	}
 	return keys, nil
 }
+
+func GetVMID(vmName string) (ids.ID, error) {


maybe I am nitpicking but I have a bit of an issue with this: while a VMID must be unique it's name does not have to. Is seems that we are inforcing here a stricter relationship among VM ID and name.
It may be fine for our purposes, but maybe we should note it down?

I'm afraid I don't have much insight here - this code was copied from ANR.

How important is it for this detail to be documented if it's just an implementation detail for a test-only deployment framework?

If ANR does like that it's fine with me. That is actually a user facing product so it's the most informative I guess

I've always hated this. This is really not what VMIDs are supposed to be at all... it's just an ugly hack because it happens to work with the VMs we've developed internally.

It is not possible to determine the VMID from the name of the VM (or chain, or subnet). Which is why I didn't allow this function into avalanchego's utils previously (internal ref: https://github.com/ava-labs/avalanchego-internal/pull/1388).

Happy to do something else, I have no attachment here. I just want to be able to deploy subnets. How should it be determined?

How should it be determined?

Currently VMs arbitrarily specify their VMIDs. (so I think the user of tmpnet.Chain should just provide the VMID.

My hope in the future is that VMIDs will come from the public key of the developer of the VM or something which will fit nicely into a VM marketplace.

But I think rather than specifying names it should be expected to specify the ID.

tests/fixture/tmpnet/README.md

abi87 · 2024-01-09T16:00:53Z

tests/fixture/tmpnet/subnet.go

+			&txs.SubnetValidator{
+				Validator: txs.Validator{
+					NodeID: node.NodeID,
+					Start:  uint64(startTime.Unix()),


comment: if Durango is already active in the test network, this should not be needed.
It's fine leaving like this rn, I'll make a pass through the codebase to clean this up as soon as Durango activates (we can also simplify the API inputs)

abi87

lgtm

marun · 2024-01-12T12:29:22Z

Rebased

StephenButtolph · 2024-01-12T17:20:46Z

tests/fixture/tmpnet/subnet.go

+
+type Chain struct {
+	// Set statically
+	VMName  string


I'd prefer for this to be VMID ids.ID - avalanchego doesn't know anything about VM names. (Then I think we can remove the GetVMID function.

StephenButtolph · 2024-01-12T17:27:40Z

tests/fixture/tmpnet/utils.go

@@ -87,3 +87,12 @@ func NewPrivateKeys(keyCount int) ([]*secp256k1.PrivateKey, error) {
 	}
 	return keys, nil
 }
+
+func GetVMID(vmName string) (ids.ID, error) {


How should it be determined?

Currently VMs arbitrarily specify their VMIDs. (so I think the user of tmpnet.Chain should just provide the VMID.

My hope in the future is that VMIDs will come from the public key of the developer of the VM or something which will fit nicely into a VM marketplace.

But I think rather than specifying names it should be expected to specify the ID.

StephenButtolph · 2024-01-12T17:31:46Z

tests/fixture/tmpnet/network.go

+	subnetIDsValue := ""
+	if len(subnetIDs) > 0 {
+		subnetIDsValue = strings.Join(subnetIDs, ",")
+	}
+	flags[config.TrackSubnetsKey] = subnetIDsValue


Suggested change

subnetIDsValue := ""

if len(subnetIDs) > 0 {

subnetIDsValue = strings.Join(subnetIDs, ",")

}

flags[config.TrackSubnetsKey] = subnetIDsValue

flags[config.TrackSubnetsKey] = strings.Join(subnetIDs, ",")

marun added testing This primarily focuses on testing e2e-subnets labels Dec 15, 2023

marun self-assigned this Dec 15, 2023

marun force-pushed the tmpnet-subnets branch 5 times, most recently from d24f48e to 4e17abf Compare December 18, 2023 03:32

marun linked an issue Dec 18, 2023 that may be closed by this pull request

Add subnet support to the tmpnet fixture #2416

Closed

6 tasks

marun force-pushed the tmpnet-subnets branch 2 times, most recently from 85776ec to 6b65ce9 Compare December 18, 2023 04:56

marun force-pushed the tmpnet-refactor-network branch from b86451d to 99eecff Compare December 18, 2023 05:14

marun force-pushed the tmpnet-subnets branch 6 times, most recently from 8bdc55f to 7667986 Compare December 18, 2023 06:55

marun mentioned this pull request Dec 18, 2023

vms: Migrate timestampvm #2493

Closed

5 tasks

marun force-pushed the tmpnet-refactor-network branch 3 times, most recently from 80f6c63 to f1ec505 Compare December 19, 2023 22:43

marun force-pushed the tmpnet-subnets branch from 7667986 to d7d4751 Compare December 19, 2023 22:45

marun force-pushed the tmpnet-refactor-network branch from f1ec505 to 6939831 Compare December 21, 2023 10:28

marun force-pushed the tmpnet-subnets branch 6 times, most recently from 9187355 to 6b7231a Compare December 23, 2023 01:13

marun force-pushed the tmpnet-subnets branch from 2731409 to 4e31c66 Compare January 4, 2024 23:05

Base automatically changed from tmpnet-refactor-network to dev January 5, 2024 21:06

marun force-pushed the tmpnet-subnets branch from 4e31c66 to c4246f3 Compare January 8, 2024 12:23

marun marked this pull request as ready for review January 8, 2024 12:23

marun requested review from abi87 and gyuho as code owners January 8, 2024 12:23

abi87 reviewed Jan 9, 2024

View reviewed changes

marun commented Jan 9, 2024

View reviewed changes

tests/fixture/tmpnet/README.md Outdated Show resolved Hide resolved

abi87 reviewed Jan 9, 2024

View reviewed changes

abi87 approved these changes Jan 9, 2024

View reviewed changes

marun force-pushed the tmpnet-subnets branch from b2e0204 to f791304 Compare January 12, 2024 12:28

StephenButtolph enabled auto-merge January 12, 2024 13:58

StephenButtolph reviewed Jan 12, 2024

View reviewed changes

StephenButtolph disabled auto-merge January 12, 2024 22:20

marun added 7 commits January 17, 2024 12:37

tmpnet: Add support for subnets

430a498

tmpnet: Follow the expected chain creation procedure

b2ac3cf

tmpnet: Retain IDs of validating nodes with subnets

4367e51

fixup: Update README to reflect subnet support

57ccbdc

fixup: Add output to subnet creation

64be9ad

fixup: Add log output to subnet creation

ee47dcc

fixup: Respond to review comments

0b2fbf6

marun force-pushed the tmpnet-subnets branch from 9f44399 to 0b2fbf6 Compare January 17, 2024 11:37

StephenButtolph approved these changes Jan 17, 2024

View reviewed changes

StephenButtolph added this to the v1.10.18 milestone Jan 17, 2024

StephenButtolph added this pull request to the merge queue Jan 17, 2024

Merged via the queue into dev with commit 26e329a Jan 17, 2024
17 checks passed

StephenButtolph deleted the tmpnet-subnets branch January 17, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`tmpnet`: Add support for subnets #2492

`tmpnet`: Add support for subnets #2492

marun commented Dec 15, 2023 •

edited

Loading

abi87 left a comment

abi87 Jan 9, 2024

marun Jan 9, 2024

StephenButtolph Jan 12, 2024

abi87 Jan 9, 2024

marun Jan 12, 2024 •

edited

Loading

abi87 Jan 9, 2024

abi87 Jan 9, 2024

marun Jan 9, 2024

abi87 Jan 9, 2024

StephenButtolph Jan 12, 2024 •

edited

Loading

marun Jan 12, 2024

StephenButtolph Jan 12, 2024

marun Jan 17, 2024

abi87 Jan 9, 2024

abi87 left a comment

marun commented Jan 12, 2024

StephenButtolph Jan 12, 2024

marun Jan 17, 2024

StephenButtolph Jan 12, 2024

StephenButtolph Jan 12, 2024

marun Jan 12, 2024

		│ └── raZ51bwfepaSaZ1MNSRNYNs3ZPfj...U7pa3
		│ └── config.json // Custom chain configuration for all nodes

tmpnet: Add support for subnets #2492

tmpnet: Add support for subnets #2492

Conversation

marun commented Dec 15, 2023 • edited Loading

TODO

abi87 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

marun Jan 12, 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

StephenButtolph Jan 12, 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

abi87 left a comment

Choose a reason for hiding this comment

marun commented Jan 12, 2024

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

`tmpnet`: Add support for subnets #2492

`tmpnet`: Add support for subnets #2492

marun commented Dec 15, 2023 •

edited

Loading

marun Jan 12, 2024 •

edited

Loading

StephenButtolph Jan 12, 2024 •

edited

Loading