feat(vstorage): Disambiguate empty strings and deletions #6971

mhofman · 2023-02-10T20:59:54Z

As usual, best reviewed commit-by-commit

Description

As preliminary changes for #5542, we need to support empty strings as actual values in vstorage, potentially for both leaf and ancestors. golang non-nullable values leaked through the design of vstorage, and setting a value of '' previously meant deletion.

To support empty strings as actual values, we need to:

Support explicit deletion
Encode noData in the cosmos KV store differently than empty string when an entry must exist (ancestor key)

Security Considerations

None

Scaling Considerations

An alternative would have been to encode or hilbert the state-sync values to avoid empty strings, but that would have been costly as state sync export is likely to become the biggest data generator, and would have leaked further a golang design choice.

Furthermore changing the interface allows batch setting multiple entries to avoid repeated call across the JS -> golang boundary. (Current vstorage users do not yet take advantage of this change, the mailbox probably could with some further refactoring)

Documentation Considerations

vstorage clients need to be updated if they iterate over keys, as they may encounter a value with an unexpected prefix. The casting library is updated as part of this PR.

A migration (or export/import) will be needed at the next upgrade containing this change. The migration was implemented as part of this change, and a note left in the current upgrade handler highlighting the need to call migration at the next upgrade.

Testing Considerations

Added unit tests, updated existing ones.

michaelfig

Just a few questions around the Golang code. JS looks good to me!

@gibson042 feel free to abstain if that's good enough for you, otherwise I won't approve until you've weighed in.

golang/cosmos/app/app.go

golang/cosmos/x/vstorage/keeper/keeper.go

golang/cosmos/x/vstorage/types/types.go

packages/internal/src/lib-chainStorage.js

mhofman · 2023-02-22T01:17:30Z

After a discussion with @warner and @gibson042 this morning, I realized that this change relied too much on JS producing entry tuples of the right length. For example in JS JSON.stringify(["foo", undefined]) === '["foo", null]' and then in golang var entry []string; json.Unmarshal(msg, &entry) results in ["foo", ""].

I have thus changed the Unmarshal path to explicitly check the type of the value, and added a couple constructors for StorageEntry, at which point I decided there was no reason to keep going with the []string representation.

I am still very uncertain whether any of this if colloquial golang, and I'm looking for experts opinions.

gibson042

What is the overall purpose of differentiating empty-string values and nil values? I'm not against it, but I'm not clear on what such a change supports—what clients care about the difference. Is this for something like cascading deletion?

golang/cosmos/x/swingset/keeper/keeper.go

golang/cosmos/x/vstorage/keeper/keeper.go

golang/cosmos/x/vstorage/types/path_keys.go

gibson042 · 2023-02-22T15:39:36Z

golang/cosmos/x/vstorage/vstorage.go

 	case "set":
-		keeper.SetStorageAndNotify(cctx.Context, msg.Path, msg.Value)
+		for _, arg := range msg.Args {
+			var entry types.StorageEntry
+			entry, err = types.UnmarshalStorageEntry(arg)
+			if err != nil {
+				return
+			}
+			keeper.SetStorageAndNotify(cctx.Context, entry)
+		}


If I'm reading correctly, "set" and all the other write methods now affect an arbitrary number of entries. Would it instead make sense to separate single-entry vs. multi-entry methods?

I didn't see a reason to have different methods for single vs multiple entry updates if the length of the args indicates that intent.

golang/cosmos/x/vstorage/vstorage.go

gibson042 · 2023-02-22T15:46:10Z

golang/cosmos/x/vstorage/vstorage.go

 			return "null", nil
 		}
-		//fmt.Printf("Keeper.GetStorage gave us %bz\n", value)
-		bz, err := json.Marshal(value)
+		//fmt.Printf("Keeper.GetStorage gave us %bz\n", entry.Value())


I think this line has been carried forward long enough.

Suggested change

//fmt.Printf("Keeper.GetStorage gave us %bz\n", entry.Value())

gibson042 · 2023-02-22T15:48:07Z

golang/cosmos/x/vstorage/vstorage.go

 			DataPrefixBytes: string(keeper.GetDataPrefix()),
+			NoDataValue:     string(keeper.GetNoDataValue()),


Oof, it stinks to leak these implementation details. What would be the consequence of not exporting NoDataValue here?

The encoding details were unfortunately already leaking to anyone that reads vstorage data directly from the chain, both in the path encoding, and in the data prefixing. It was a very unfortunate realization I couldn't get away with an internal only change. I don't fully grok how external clients like the casting package fetch data, and maybe we could have an adapter on the RPC server side to avoid leaking these details to clients? However I'd consider this out of scope for this PR. Ping @michaelfig

The only way an external client can verify a Merkle proof for some on-chain data from an RPC node (trusting the validators but not the integrity of the RPC node) is by querying the exact IAVL key/value representation in bytes, and then knowing how they correspond to the application layer. This verification process necessarily crosses abstractions in an uncomfortable way.

If you're proposing to remove getStoreKey and nothing in the SDK uses it, that's fine with me.

This verification process necessarily crosses abstractions in an uncomfortable way.

Makes sense

If you're proposing to remove getStoreKey and nothing in the SDK uses it, that's fine with me.

I remember seeing some potential uses. I didn't feel comfortable enough changing that wiring.

packages/casting/src/casting-spec.js

mhofman

What is the overall purpose of differentiating empty-string values and nil values? I'm not against it, but I'm not clear on what such a change supports—what clients care about the difference. Is this for something like cascading deletion?

The Swingstore replication into IAVL / the MultiStore will be done through a new vstorage path, and it requires being able to store empty strings. An empty string is a valid string value that is simply different than null in JS :)

golang/cosmos/x/swingset/keeper/keeper.go

golang/cosmos/x/vstorage/keeper/keeper.go

golang/cosmos/x/vstorage/types/types.go

mhofman · 2023-02-22T19:22:29Z

golang/cosmos/x/vstorage/vstorage.go

-	Path   string `json:"key"` // TODO: rename JSON to "path"
-	Value  string `json:"value"`
+	Method string            `json:"method"`
+	Args   []json.RawMessage `json:"args"`


This is not backwards compatible, but the JS layer is updated in the same commit. There are no external clients.

mhofman · 2023-02-22T19:24:12Z

golang/cosmos/x/vstorage/vstorage.go

 	case "set":
-		keeper.SetStorageAndNotify(cctx.Context, msg.Path, msg.Value)
+		for _, arg := range msg.Args {
+			var entry types.StorageEntry
+			entry, err = types.UnmarshalStorageEntry(arg)
+			if err != nil {
+				return
+			}
+			keeper.SetStorageAndNotify(cctx.Context, entry)
+		}


I didn't see a reason to have different methods for single vs multiple entry updates if the length of the args indicates that intent.

golang/cosmos/x/vstorage/vstorage.go

mhofman · 2023-02-22T19:29:06Z

golang/cosmos/x/vstorage/vstorage.go

 			DataPrefixBytes: string(keeper.GetDataPrefix()),
+			NoDataValue:     string(keeper.GetNoDataValue()),


The encoding details were unfortunately already leaking to anyone that reads vstorage data directly from the chain, both in the path encoding, and in the data prefixing. It was a very unfortunate realization I couldn't get away with an internal only change. I don't fully grok how external clients like the casting package fetch data, and maybe we could have an adapter on the RPC server side to avoid leaking these details to clients? However I'd consider this out of scope for this PR. Ping @michaelfig

mhofman · 2023-02-23T13:56:57Z

This PR has grown to a LOT of fixups to address review feedback. Here is what the rebased with squashing looks like: https://github.com/Agoric/agoric-sdk/compare/mhofman/5542-refactor-vstorage-rebase

Happy to push that (ultimately identical) version to this branch if all reviewers prefer.

gibson042

LGTM after resolving remaining comments.

golang/cosmos/x/swingset/keeper/keeper.go

golang/cosmos/x/vstorage/keeper/keeper.go

golang/cosmos/x/vstorage/types/path_keys.go

golang/cosmos/x/vstorage/vstorage.go

mhofman · 2023-02-28T21:25:35Z

Ugh I forgot to push the changes addressing feedback yesterday. PTAL @gibson042

golang/cosmos/x/vstorage/keeper/keeper.go

mhofman · 2023-03-01T17:43:13Z

Clean squash/fixup of all the review feedback: cd1cf14..14ed493

Use method dependent args in vstorage messages to allow expressing updates (set and append) as entries (list of [key, value] tuples). Enables explicit deletion through setting an entry without value

JimLarson

One potential future cleanuup.

JimLarson · 2023-03-10T19:45:21Z

golang/cosmos/x/vstorage/types/types.go

+	var generic [2]interface{}
+	err = json.Unmarshal(msg, &generic)
+
+	if err != nil {
+		return
+	}
+
+	path, ok := generic[0].(string)
+	if !ok {
+		err = fmt.Errorf("invalid storage entry path: %q", generic[0])
+		return
+	}
+
+	switch generic[1].(type) {
+	case string:
+		entry = NewStorageEntry(path, generic[1].(string))
+	case nil:
+		entry = NewStorageEntryWithNoData(path)
+	default:
+		err = fmt.Errorf("invalid storage entry value: %q", generic[1])
+	}
+	return


You can do this instead:

var generic [2]*string{} err = json.Unmarshal(msg, &generic) if err != nil { return } if generic[0] == nil { err = fmt.Errorf("storage entry path cannot be null") return } if generic[1] == nil { entry = NewStorageEntry(generic[0], *generic[1]) } else { entry = NewStorageEntryWithNoData(generic[9]) } return

Oh json.Unmarshal understands string pointers? Yeah that'd have been simpler.

mhofman force-pushed the mhofman/5542-refactor-vstorage branch from db9df6e to 0887c2c Compare February 11, 2023 04:47

mhofman requested review from michaelfig and gibson042 February 11, 2023 04:47

mhofman marked this pull request as ready for review February 11, 2023 04:48

michaelfig reviewed Feb 11, 2023

View reviewed changes

mhofman requested a review from JimLarson February 13, 2023 18:35

mhofman force-pushed the mhofman/5542-refactor-vstorage branch 2 times, most recently from 023f68b to 75c211d Compare February 22, 2023 00:41

gibson042 requested changes Feb 22, 2023

View reviewed changes

mhofman commented Feb 22, 2023

View reviewed changes

mhofman requested a review from gibson042 February 23, 2023 00:35

gibson042 reviewed Feb 27, 2023

View reviewed changes

gibson042 approved these changes Mar 1, 2023

View reviewed changes

golang/cosmos/x/vstorage/keeper/keeper.go Outdated Show resolved Hide resolved

mhofman force-pushed the mhofman/5542-refactor-vstorage branch from cd1cf14 to 14ed493 Compare March 1, 2023 17:41

mhofman added 6 commits March 1, 2023 17:44

feat(vstorage): Handle explicit delete of path

f46557d

feat(casting): handle noData value encoding

530bc41

feat(vstorage): Add placeholder migration logic

35a3848

refactor(cosmic-swingset): rename path import

7d795b9

refactor(vstorage): vstorageMessage is method+args

f3ac056

Use method dependent args in vstorage messages to allow expressing updates (set and append) as entries (list of [key, value] tuples). Enables explicit deletion through setting an entry without value

refactor(storage): use const when computing actionQueue key

3f32351

mhofman force-pushed the mhofman/5542-refactor-vstorage branch from 14ed493 to 3f32351 Compare March 1, 2023 17:46

mhofman added the automerge:rebase Automatically rebase updates, then merge label Mar 1, 2023

mergify bot merged commit 3080ea7 into master Mar 1, 2023

mergify bot deleted the mhofman/5542-refactor-vstorage branch March 1, 2023 18:23

JimLarson reviewed Mar 20, 2023

View reviewed changes

mhofman mentioned this pull request Apr 24, 2023

feat(cosmic-swingset): Add a config property for exporting storage to bootstrap #7489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vstorage): Disambiguate empty strings and deletions #6971

feat(vstorage): Disambiguate empty strings and deletions #6971

mhofman commented Feb 10, 2023 •

edited

Loading

michaelfig left a comment •

edited

Loading

mhofman commented Feb 22, 2023

gibson042 left a comment

gibson042 Feb 22, 2023

mhofman Feb 22, 2023

gibson042 Feb 22, 2023

gibson042 Feb 22, 2023

mhofman Feb 22, 2023

michaelfig Feb 22, 2023

michaelfig Feb 22, 2023

mhofman Feb 22, 2023

mhofman left a comment

mhofman Feb 22, 2023

mhofman Feb 22, 2023

mhofman Feb 22, 2023

mhofman commented Feb 23, 2023

gibson042 left a comment

mhofman commented Feb 28, 2023

mhofman commented Mar 1, 2023

JimLarson left a comment

JimLarson Mar 10, 2023

mhofman Mar 20, 2023

		DataPrefixBytes: string(keeper.GetDataPrefix()),
		NoDataValue: string(keeper.GetNoDataValue()),

feat(vstorage): Disambiguate empty strings and deletions #6971

feat(vstorage): Disambiguate empty strings and deletions #6971

Conversation

mhofman commented Feb 10, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

michaelfig left a comment • edited Loading

Choose a reason for hiding this comment

mhofman commented Feb 22, 2023

gibson042 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

mhofman 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

mhofman commented Feb 23, 2023

gibson042 left a comment

Choose a reason for hiding this comment

mhofman commented Feb 28, 2023

mhofman commented Mar 1, 2023

JimLarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Feb 10, 2023 •

edited

Loading

michaelfig left a comment •

edited

Loading