Split storage sync P2P protocol by martintomazic · Pull Request #6262 · oasisprotocol/oasis-core

martintomazic · 2025-07-10T14:04:46Z

Closes #5751, blocked by #6270.

Likely solves root cause of #5750.

Open for discussion:

E2e test to be more confident in integration part, even though existing integration test was helpful at catching bugs.
- Cons: Additional config parameter, e2e test, and wrapping on the node side that delegates to appropriate client.
- Alternatives:
  - Adapt oasis tesnet runner to receive path to "legacy" binary (would also require to change CI).
  - Bootstrap a few nodes on the testnet and manually monitor p2p dashboard before release.
Extending rpc client (458f76f) with fallback protocol is likely the way to go.
- Cons: Since existing storage sync has few methods, it might be simpler for the new clients to take the legacy client and fallback to it manually, avoiding the need for this change and additional fallback manager.
Should new protocols alias legacy protocol definitions/methods given they are semantically equivalent?
- Pros: Less duplication && less type fighting.
- Cons1: When removing the legacy protocol, things will have to be copied again (double work).
- Cons2: In a way not aliasing protects against accidental changes in one manifesting in other.

netlify · 2025-07-10T14:04:53Z

✅ Deploy Preview for oasisprotocol-oasis-core canceled.

Name	Link
🔨 Latest commit	`96be6bd`
🔍 Latest deploy log	https://app.netlify.com/projects/oasisprotocol-oasis-core/deploys/6882189d76fc1800085fd0f1

go/worker/storage/p2p/diff/protocol.go

martintomazic

I would like to reach consensus on 1...3 from the context before proceeding.

Personally, I think only e2e test would be simplest and make me most confident. There would be some boilerplate though + more resources on the CI, although it will be removed eventually.

Otherwise totally in for making our code testable via mocks etc...

martintomazic · 2025-07-14T11:23:09Z

go/worker/storage/p2p/sync_test/interoperable_test.go

+	// Test diff sync protocol.
+	diffClient := diffsync.NewClient(host, chainContext, runtimeID)
+	time.Sleep(1 * time.Second)


This is already flaky in the CI. Ideally there should be explicit synchronization or increase the timeout.

Let's see if we opt for e2e only with config.

go/worker/storage/p2p/sync_test/interoperable_test.go

go/worker/storage/p2p/checkpointsync/client.go

go/worker/storage/p2p/diffsync/client.go

kostko · 2025-07-17T11:23:24Z

go/worker/storage/p2p/checkpointsync/protocol.go

+func init() {
+	peermgmt.RegisterNodeHandler(&peermgmt.NodeHandlerBundle{
+		ProtocolsFn: func(n *node.Node, chainContext string) []core.ProtocolID {
+			if !n.HasRoles(node.RoleComputeWorker | node.RoleStorageRPC) {


Probably we should add observer nodes as well (also elsewhere).

kostko · 2025-07-17T11:25:34Z

go/worker/storage/p2p/sync_test/interoperable_test.go

+func (bm *backendMock) GetCheckpoints(_ context.Context, request *checkpoint.GetCheckpointsRequest) ([]*checkpoint.Metadata, error) {
+	cpVersion = request.Version
+	root = node.Root{
+		// Existing bug: storagesync p2p server does not set namespace, nor does checkpoint.ChunkProvider validates it.


Not sure if this should be considered a bug, because the protocol is always per runtime so the namespace is implicit.

You are right, I will simply ignore this like other part of the code do.

Possibly I could add a comment to the api request definition that this filed is redundant/not used.

That's a good idea.

If we use cbor codec for grpc, omitting unused field should be both forward and backward compatible? So possibly we could remove it altogether (separate PR of course). Else I will just leave a comment.

What do you mean remove it? The same root structure is used in places where it needs the namespace.

I would just leave a comment for now.

What do you mean remove it? The same root structure is used in places where it needs the namespace.

I meant removing the namespace from request (backend) not from root:

oasis-core/go/storage/mkvs/checkpoint/checkpoint.go

Line 52 in 2bb1b3b

Namespace common.Namespace `json:"namespace"`

Update: or update all callsites to pass it, add new field to storage/checkpoint sync request and validate in the backend (omitting validation in case of zero value not to be breaking).

Will go with comment for this PR as this would be out of scope. :)

kostko · 2025-07-17T11:36:16Z

Likely solves root cause of #5750.

Note that as I see it currently, you always register for the checkpoint sync protocol, even if the checkpointer is disabled? In this case, this alone will not solve the above issue.

kostko · 2025-07-17T11:51:59Z

E2e test to be more confident in integration part, even though existing integration test was helpful at catching bugs.

Yeah that could become complicated fast and the e2e suite likely needs a major refresh so I would do it separately.

Should new protocols alias legacy protocol definitions/methods given they are semantically equivalent?

I think copying is the safer option.

martintomazic · 2025-07-17T12:02:15Z

Note that as I see it currently, you always register for the checkpoint sync protocol, even if the checkpointer is disabled? In this case, this alone will not solve the above issue.

Nice catch!

I guess correct solution to it is the complementary solution from the #5750 (comment)

RegisterProtocol in addition to triggering tracking and peer management for the given protocol (fetch part), is also responsible for triggering background advertisement (serve part). Ideally, advertisement would be RegisterProtocolServer responsibility.
We could keep it simple and start advertising only after sufficient number of checkpoints. Likely we could generalize this and propagate generic readiness closure up to advertisement's retry.

^^ or independent and then only register if checkpointer is enabled.

update: This has to be independent method. E.g. registering light client protocol server should not start advertisement as peers are found here by other means.

martintomazic · 2025-07-18T14:31:53Z

Let's get #6270 before this one to also tackle #6262 (comment) here.

Will handle remaining threads later on.

go/p2p/protocol/protocol.go

peternose · 2025-07-17T12:02:53Z

go/p2p/rpc/client.go


 // NewClient creates a new RPC client for the given protocol.
-func NewClient(h host.Host, p protocol.ID) Client {
+func NewClient(h host.Host, p protocol.ID, fallback ...protocol.ID) Client {


I think you need to accept pids []protocol.ID because go-libp2p implementation doesn't guarantee that the first ID will be used if possible.

Isn't this just syntactic sugar for slice? The main problem as you point out is that interface does not promise order.

In practice, implementation favors protocols from left to right.

Ideally, we should not rely on this. If we had many methods as part of our protocol likely this would be the only sensible approach. But we don't, so maybe we can make it simpler.

go/worker/storage/p2p/sync/client.go

peternose · 2025-07-17T12:16:05Z

go/worker/storage/p2p/diffsync/server.go

+	}
+}
+
+func (s *service) handleGetDiff(ctx context.Context, request *GetDiffRequest) (*GetDiffResponse, error) {


Suggested change

func (s *service) handleGetDiff(ctx context.Context, request *GetDiffRequest) (*GetDiffResponse, error) {

func (s *service) handleGetDiff(ctx context.Context, req *GetDiffRequest) (*GetDiffResponse, error) {

peternose · 2025-07-17T12:19:41Z

go/worker/storage/p2p/diffsync/server.go

+}
+
+// NewServer creates a new storage diff protocol server.
+func NewServer(chainContext string, runtimeID common.Namespace, backend api.Backend) rpc.Server {


To avoid returning interface, I would create function NewService and let the caller create rpc.Server. Or create 2 functions.

peternose · 2025-07-17T12:58:49Z

go/worker/storage/p2p/checkpointsync/client.go

+)
+
+// Client is a checkpoints sync protocol client.
+type Client interface {


Do we need this interface?

peternose · 2025-07-21T08:04:46Z

go/worker/storage/p2p/checkpointsync/client.go

+// Warning: This client only registers the checkpoint sync protocol with the P2P
+// service. To enable advertisement of the legacy protocol, it must be registered
+// separately.
+func NewClient(p2p rpc.P2P, chainContext string, runtimeID common.Namespace) Client {


Move constructor after struct type definition.

peternose · 2025-07-21T08:07:33Z

go/worker/storage/p2p/checkpointsync/client.go

+// separately.
+func NewClient(p2p rpc.P2P, chainContext string, runtimeID common.Namespace) Client {
+	pid := protocol.NewRuntimeProtocolID(chainContext, runtimeID, CheckpointSyncProtocolID, CheckpointSyncProtocolVersion)
+	fallbackPid := sync.GetStorageSyncProtocolID(chainContext, runtimeID)


I would also prepare PR which will clean these things.

peternose · 2025-07-21T08:10:30Z

go/worker/storage/p2p/checkpointsync/client.go

+}
+
+func (c *client) getBestPeers(opts ...rpc.BestPeersOption) []core.PeerID {
+	return append(c.mgr.GetBestPeers(opts...), c.fallbackMgr.GetBestPeers(opts...)...)


I'm not sure if this will work as expected. Maybe we should have just 2 clients, everyone with its own protocol ID and then we can switch to the legacy one in methods like GetCheckpointChunk if we don't get an answer from the first one.

Why? I think the test confirms it works, but indeed this is probably simpler and thus better, given that we only have 3 methods to fallback manually. Will think a bit after rebasing this PR on top of #6270.

In general this would also enable proper e2e testing (we could remove existing integration test). E2e test can be registered to not execute by default and used later as sanity again when removing support for both protocols.

Probably to make this work we should refactor existing storage and checkpoint sync to avoid returning custom Checkpoint type that wraps metadata and slice of peer feedbacks, but instead return slice of metadata with slice of slices of peer feedback. See. Or handle this type discrepancy on the call site. Uglier I would want but doable

Tried this in new PR #6277

peternose · 2025-07-21T08:12:14Z

go/worker/storage/p2p/checkpointsync/protocol.go

+	Checkpoints []*checkpoint.Metadata `json:"checkpoints,omitempty"`
+}
+
+// Constants related to the GetCheckpointChunk method.


Every constant should have its own comment.

martintomazic · 2025-07-23T13:42:28Z

Will consider simplifying with 2 clients again and then I tag you all for a final review :).

Avoid global state to enable writting unit tests with multiple peers in the same process.

Legacy storage sync protocol is still advertised and served to enable seamless rolling upgrades of the network.

Maybe I will replace this with e2e alltogether. To be consistent I should probably extend other p2p clients. On the other hand I dislike methods for testing only.

kostko reviewed Jul 11, 2025

View reviewed changes

go/worker/storage/p2p/diff/protocol.go Outdated Show resolved Hide resolved

martintomazic force-pushed the martin/feature/split-storagesync-p2p branch 3 times, most recently from e7dc2b2 to 230747b Compare July 14, 2025 11:12

martintomazic commented Jul 14, 2025

View reviewed changes

martintomazic marked this pull request as ready for review July 14, 2025 11:42

martintomazic requested review from peterjgilbert, peternose, pro-wh and ptrus as code owners July 14, 2025 11:42

martintomazic requested a review from kostko July 17, 2025 07:58

kostko reviewed Jul 17, 2025

View reviewed changes

martintomazic mentioned this pull request Jul 18, 2025

go/p2p: Ensure only server providers advertise themselves #6270

Merged

martintomazic force-pushed the martin/feature/split-storagesync-p2p branch 2 times, most recently from a9bd6b5 to 462a7eb Compare July 18, 2025 14:30

martintomazic marked this pull request as draft July 18, 2025 14:38

peternose reviewed Jul 21, 2025

View reviewed changes

martintomazic force-pushed the martin/feature/split-storagesync-p2p branch from 462a7eb to 32a6ad1 Compare July 23, 2025 13:34

martintomazic force-pushed the martin/feature/split-storagesync-p2p branch from 32a6ad1 to ebbbcb1 Compare July 23, 2025 13:59

martintomazic mentioned this pull request Jul 23, 2025

Split storage sync P2P protocol (manual fallback) #6277

Merged

martintomazic added 2 commits July 24, 2025 13:02

go/p2p/protocol: Replace global registry with parametric

28979a5

Avoid global state to enable writting unit tests with multiple peers in the same process.

go/p2p/rpc: Add fallback protocols to RPC client

916410a

martintomazic added 4 commits July 24, 2025 13:02

go/worker/storage/p2p: Split storage sync protocol

f5fe8ac

go/worker/storage: Integrate new p2p protocols

57ff38b

Legacy storage sync protocol is still advertised and served to enable seamless rolling upgrades of the network.

fixup

d2f53ce

Fixup: Fix synchronization issues in the test

96be6bd

Maybe I will replace this with e2e alltogether. To be consistent I should probably extend other p2p clients. On the other hand I dislike methods for testing only.

martintomazic force-pushed the martin/feature/split-storagesync-p2p branch from ebbbcb1 to 96be6bd Compare July 24, 2025 11:27

martintomazic closed this Aug 6, 2025

martintomazic mentioned this pull request Oct 2, 2025

go/p2p: Add Observer nodes to the target set of registered nodes for the runtime protocols #6342

Closed

	func (s service) handleGetDiff(ctx context.Context, request GetDiffRequest) (*GetDiffResponse, error) {
	func (s service) handleGetDiff(ctx context.Context, req GetDiffRequest) (*GetDiffResponse, error) {

Conversation

martintomazic commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for oasisprotocol-oasis-core canceled.

Uh oh!

Uh oh!

martintomazic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martintomazic Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostko commented Jul 17, 2025

Uh oh!

kostko commented Jul 17, 2025

Uh oh!

martintomazic commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martintomazic commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martintomazic Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martintomazic commented Jul 23, 2025

Uh oh!

Reviewers

Assignees

Labels

martintomazic commented Jul 10, 2025 •

edited

Loading

netlify bot commented Jul 10, 2025 •

edited

Loading

martintomazic Jul 19, 2025 •

edited

Loading

martintomazic commented Jul 17, 2025 •

edited

Loading

martintomazic commented Jul 18, 2025 •

edited

Loading

martintomazic Jul 21, 2025 •

edited

Loading