Use new node contract in registry #107

neekolas · 2024-08-11T14:53:27Z

tl;dr

Creates new SmartContractRegistry that polls the smart contract to get the latest state of the nodes
Refactored the config out of the server to avoid circular dependencies and allow config objects to be passed in to other modules
Added a dev/run command that populates all the environment variables required
Created a notifier struct to power the SmartContractRegistry and publish changes to subscriptions

Why not use the indexer?

We could get the same result from indexing the chain based on events. This seemed more straightforward. We don't care about historical state here, so processing old events is just going to create noise and complexity. When any node starts up it just needs a full picture of all the other nodes right now. We also don't have tight latency requirements: this contract moves in the cadence of days or weeks.

Since we know the node list is going to be small forever, and this data is going to be frequently accessed, having it all live in memory is also way more performant.

neekolas · 2024-08-11T14:53:48Z

Sleep when no logs #110
Use new node contract in registry #107 👈
Update node contract #106
Scaffold smart contract event storage #105
Add contracts #103
chain streaming interfaces #102
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @neekolas and the rest of your teammates on Graphite

neekolas · 2024-08-12T23:04:41Z

README.md

@@ -8,9 +8,9 @@

 `xmtpd` (XMTP daemon) is an experimental version of XMTP node software. It is **not** the node software that currently forms the XMTP network.

-After `xmtpd` meets specific functional requirements, the plan is for it to become the node software that powers the XMTP network. 


Think my editor has a default Markdown formatter. We can make that standard issue in the repo so we don't get diffs like this.

neekolas · 2024-08-13T00:37:29Z

pkg/registry/notifier_test.go

+	require.Equal(t, 100, getCurrentCount())
+}
+
+func countChannel[Kind any](ch <-chan Kind) func() int {


This was to prevent go's race detection from getting mad about data races in our tests

neekolas · 2024-08-13T00:38:46Z

pkg/server/server.go

@@ -72,5 +74,5 @@ func (s *ReplicationServer) Shutdown() {
 }

 func parsePrivateKey(privateKeyString string) (*ecdsa.PrivateKey, error) {
-	return crypto.HexToECDSA(privateKeyString)
+	return crypto.HexToECDSA(strings.TrimPrefix(privateKeyString, "0x"))


Lets us re-use the PK environment variable we are using with forge to deploy the contract locally, which expects a 0x prefix. HexToECDSA does not expect the 0x

neekolas · 2024-08-13T14:52:22Z

dev/up


 if ! which forge &>/dev/null; then curl -L https://foundry.paradigm.xyz | bash ; fi
 if ! which migrate &>/dev/null; then brew install golang-migrate; fi
 if ! which golangci-lint &>/dev/null; then brew install golangci-lint; fi
 if ! which shellcheck &>/dev/null; then brew install shellcheck; fi
-if ! which mockery &>/dev/null; then brew install mockery; fi
+if ! which mockery &>/dev/null; then go install github.com/vektra/mockery/v2; fi


Because the tools.go file puts this in our go.mod, we can now get a deterministic version for the dependency when we do a Go install.

Would it be simpler to just specify the version in the install command here, rather than updating the tools.go file?

richardhuaaa

This looks really hard to get right, well done

richardhuaaa · 2024-08-13T00:36:11Z

dev/local.env

+export CHAIN_RPC_URL=$DOCKER_RPC_URL  # From contracts/.env
+export NODE_PRIVATE_KEY=$PRIVATE_KEY # From contracts/.env


Is there a reason that dev/local.env and contracts/.env are not consolidated into a single file? For example, is the PRIVATE_KEY used anywhere else?

We use the stuff in contracts/local.env to deploy the smart contract locally. Could consolidate, but I'd like to figure out just how complicated all the local smart contract dev stuff is going to be with a full set of contracts and a more sophisticated deployer. My guess is that we're going to have a lot more configuration that is specific to the contracts.

richardhuaaa · 2024-08-13T19:17:59Z

pkg/registry/contractRegistry.go

+	s.newNodesNotifier.trigger(nodes)
+
+	s.nodesMutex.Lock()
+	defer s.nodesMutex.Unlock()
+	for _, node := range nodes {
+		s.nodes[node.NodeId] = node
+	}


Can you walk through what happens in the scenario when a listener is notified of a new node, but the registry hasn't written that node into its map yet? (e.g. the listener calls GetNodes() first)

I wouldn't expect the listener to need to do that (they already have all the new nodes). But maybe I should move the trigger call to be after just to be safe.

richardhuaaa · 2024-08-13T19:21:34Z

dev/up


 if ! which forge &>/dev/null; then curl -L https://foundry.paradigm.xyz | bash ; fi
 if ! which migrate &>/dev/null; then brew install golang-migrate; fi
 if ! which golangci-lint &>/dev/null; then brew install golangci-lint; fi
 if ! which shellcheck &>/dev/null; then brew install shellcheck; fi
-if ! which mockery &>/dev/null; then brew install mockery; fi
+if ! which mockery &>/dev/null; then go install github.com/vektra/mockery/v2; fi


Would it be simpler to just specify the version in the install command here, rather than updating the tools.go file?

richardhuaaa · 2024-08-13T19:39:40Z

pkg/registry/contractRegistry.go

+	// Unmarshal the signing key.
+	// If invalid, mark the config as being invalid as well. Clients should treat the
+	// node as unhealthy in this case
+	signingKey, err := crypto.UnmarshalPubkey(rawNode.Node.SigningKeyPub)


I think we should loudly surface an error instead of quietly excluding the node.

It's not expected for an invalid key to be written to the registry, and failing to read it here would imply some inconsistency in how the key is interpreted.

This would also be an argument for why we shouldn't let node operators change the http address themselves. It might be simpler for the DAO to do all validation before writing to the registry (or having the smart contract do the validation) so that everything in the registry can be treated as correct downstream.

I can update it to at least log something loudly.

But I do think we need to treat the inputs in the smart contract as somewhat untrusted. Hopefully we'll pre-validate and this will never happen, but that's a human-driven manual process.

richardhuaaa · 2024-08-13T19:44:47Z

pkg/registry/fixedRegistry.go

+	changedNodeNotifiers      map[uint16]*notifier[Node]
+	changedNodeNotifiersMutex sync.Mutex


I don't think these notifiers ever get triggered?

This whole thing is getting deleted pretty soon

neekolas · 2024-08-13T20:59:48Z

Merge activity

Aug 13, 1:59 PM PDT: @neekolas started a stack merge that includes this pull request via Graphite.
Aug 13, 2:00 PM PDT: @neekolas merged this pull request with Graphite.

neekolas · 2024-08-13T22:43:55Z

@richardhuaaa

Would it be simpler to just specify the version in the install command here, rather than updating the tools.go file?

IDK. Having it all live in the go.mod does make upgrades nicer than having to edit some random script that's easy to be forgotten about. But I don't love either solution.

This was referenced Aug 11, 2024

chain streaming interfaces #102

Merged

Add contracts #103

Merged

Scaffold smart contract event storage #105

Merged

Update node contract #106

Merged

neekolas requested a review from richardhuaaa August 11, 2024 14:55

neekolas marked this pull request as ready for review August 11, 2024 15:02

neekolas force-pushed the 08-10-use_new_node_contract_in_registry branch 6 times, most recently from 31e878e to 47898c7 Compare August 11, 2024 16:12

neekolas force-pushed the 08-10-update_node_contract branch from 31b5549 to 5d7b02f Compare August 12, 2024 21:39

neekolas force-pushed the 08-10-use_new_node_contract_in_registry branch from d79e5fb to 2e139d8 Compare August 12, 2024 21:40

neekolas force-pushed the 08-10-update_node_contract branch from 5d7b02f to 0efeea1 Compare August 12, 2024 21:53

neekolas force-pushed the 08-10-use_new_node_contract_in_registry branch 3 times, most recently from 1afaced to 601c7c1 Compare August 12, 2024 22:24

neekolas mentioned this pull request Aug 12, 2024

Sleep when no logs #110

Merged

neekolas force-pushed the 08-10-update_node_contract branch from 0efeea1 to 894e74b Compare August 12, 2024 22:59

neekolas force-pushed the 08-10-use_new_node_contract_in_registry branch from 601c7c1 to 9bb66a2 Compare August 12, 2024 22:59

neekolas commented Aug 12, 2024

View reviewed changes

neekolas force-pushed the 08-10-use_new_node_contract_in_registry branch 2 times, most recently from 893cbec to 59a23af Compare August 12, 2024 23:58

neekolas changed the base branch from 08-10-update_node_contract to graphite-base/107 August 13, 2024 00:32

Use new node contract in registry

e2d7da7

neekolas force-pushed the 08-10-use_new_node_contract_in_registry branch from 59a23af to e2d7da7 Compare August 13, 2024 00:32

neekolas changed the base branch from graphite-base/107 to main August 13, 2024 00:33

neekolas commented Aug 13, 2024

View reviewed changes

richardhuaaa approved these changes Aug 13, 2024

View reviewed changes

neekolas merged commit 4621c3e into main Aug 13, 2024
9 checks passed

neekolas deleted the 08-10-use_new_node_contract_in_registry branch August 13, 2024 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new node contract in registry #107

Use new node contract in registry #107

neekolas commented Aug 11, 2024 •

edited

Loading

neekolas commented Aug 11, 2024 •

edited

Loading

neekolas Aug 12, 2024

neekolas Aug 13, 2024

neekolas Aug 13, 2024

neekolas Aug 13, 2024

richardhuaaa Aug 13, 2024

richardhuaaa left a comment

richardhuaaa Aug 13, 2024

neekolas Aug 14, 2024

richardhuaaa Aug 13, 2024

neekolas Aug 13, 2024

richardhuaaa Aug 13, 2024

richardhuaaa Aug 13, 2024

neekolas Aug 13, 2024

richardhuaaa Aug 13, 2024

neekolas Aug 13, 2024

neekolas commented Aug 13, 2024 •

edited

Loading

neekolas commented Aug 13, 2024 •

edited

Loading

		@@ -8,9 +8,9 @@

		`xmtpd` (XMTP daemon) is an experimental version of XMTP node software. It is not the node software that currently forms the XMTP network.

		After `xmtpd` meets specific functional requirements, the plan is for it to become the node software that powers the XMTP network.

		export CHAIN_RPC_URL=$DOCKER_RPC_URL # From contracts/.env
		export NODE_PRIVATE_KEY=$PRIVATE_KEY # From contracts/.env

		changedNodeNotifiers map[uint16]*notifier[Node]
		changedNodeNotifiersMutex sync.Mutex

Use new node contract in registry #107

Use new node contract in registry #107

Conversation

neekolas commented Aug 11, 2024 • edited Loading

tl;dr

Why not use the indexer?

neekolas commented Aug 11, 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

richardhuaaa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neekolas commented Aug 13, 2024 • edited Loading

Merge activity

neekolas commented Aug 13, 2024 • edited Loading

neekolas commented Aug 11, 2024 •

edited

Loading

neekolas commented Aug 11, 2024 •

edited

Loading

neekolas commented Aug 13, 2024 •

edited

Loading

neekolas commented Aug 13, 2024 •

edited

Loading