Swing-store export data outside of genesis file

[mhofman]

Closes #9567

Description

Stores the swing-store export data outside of the genesis file, and only include a hash of it in genesis for validation.

This is the bulk of the data in the genesis file (on mainnet 6GB vs 300MB for the rest), and it serves little purpose to keep it in there. Furthermore golevelDB chokes when cosmos attempts to store the genesis file inside the DB (limit of 4GB documents)

Security Considerations

None, the data remains validated

Scaling Considerations

Reduces memory usage on genesis export / import. Furthermore we avoid iterating over the data twice on import, which is painfully slow for IAVL.

Documentation Considerations

None

Testing Considerations

Added a3p acceptance test

Manually tested as follow:

cd packages/cosmic-swingset
make scenario2-setup
make scenario2-run-chain
# wait until there are blocks, then kill
mkdir t1/n0/export
agd --home t1/n0 export --export-dir t1/n0/export
agd --home t1/n0 tendermint unsafe-reset-all
mv t1/n0/export/* t1/n0/config
make scenario2-run-chain
# verify blocks are being produced after genesis restart

Upgrade Considerations

There is a compatibility mode to load genesis files with export data embedded.

[toliaqat]

@mhofman, This is a great direction and will help us faster imports and exports. Thanks for working on this.
Can you please add new tests covering new code paths?

[mhofman]

Can you please add new tests covering new code paths?

There are not unit tests covering genesis import / export, so it'd be a significant effort to add those now. I'll try to revive the integration test that existed, but given this feature is not used for normal operations, I'd prefer to not block on tests.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	17a5374
Status:	✅  Deploy successful!
Preview URL:	https://2f6439ed.agoric-sdk.pages.dev
Branch Preview URL:	https://mhofman-genesis-external-swi.agoric-sdk.pages.dev

View logs

[mhofman]

@toliaqat I added an integration test of mainfork. See https://github.com/Agoric/agoric-sdk/actions/runs/9621035132/job/26540251077?pr=9549#step:9:2017

PTAL

[warner]

I'm not qualified to review the implementation, but I've thrown in some thoughts on the protocol. I'm merely -0 on hashing a derivative of the data, not -1: if you can't find any other way to do it, and we believe this isn't too hard to change in the future (genesis export is only for debugging right now, we can revisit it before considering relying upon it for an agoric4 replacement chain), then I'm ok with this protocol being landed.

[warner]

nit: I'd name this expectedSHA256Hash or expectedHash

[warner]

I don't know that I'm reading this correctly, but it looks like something else is reading and parsing an artifact to produce entry.Key() and maybe-nil values, then this function JSON-encodes the entry we got, and then we hash the resulting JSON-encoded bytes. Is that about right?

I'd prefer that we hash the original, and then do whatever parsing we need to use the data. This approach is vulnerable to disagreements between the first parser and the JSON encoder. I can't name any cases that would actually trigger it, but say json.NewEncoder does many-to-one Unicode normalization of some codepoint X into bytes Z1Z2, and also serializes codepoint Y into the same Z1Z2. Then this could be tricked into passing Y into the swingstore data when the creator meant (and hashed) X.

[mhofman]

I went back and forth on this until I realized that there is no canonical JSON encoding. As such, hashing the original file isn't quite right. Of course even with a specified JSON encoding of each entry, there is no canonical way in which the entries should appear. In theory we should use an IAVL tree of the data to have a canonical hash :)

Basing the hash on the re-encoding to JSON by the golang encoder is thus motivated both by practicality (it fit better in the abstraction already in place of "export data reader"), and slightly increases the canonical status of the hash.

That said I was also careful that the hash computed this way is exactly equal to sha256sum of the file created by golang serialization.

many-to-one Unicode normalization of some codepoint X into bytes Z1Z2, and also serializes codepoint Y into the same Z1Z2. Then this could be tricked into passing Y into the swingstore data when the creator meant (and hashed) X.

As mentioned above, it's actually the opposite. There are multiple ways to escape the same string value in JSON encodings, and here we're settling on the one that golang implements

[warner]

there is no canonical JSON encoding
there is no canonical way in which the entries should appear

Yeah, that's the clue that we can't/shouldn't rely upon "canonical" encoding at all. In general, the creator of the hashed/signed artifact can encode it any way they want: doesn't matter, nobody else will be attempting to encode it in the same way. What matters is that everybody's decoder will treat the hashed/signed bytes in the same way.

If we find ourselves wanting a canonical encoder, that might mean we're doing encoding on the decoding/verification side, and that's where vulnerabilities live.

[warner]

Why does the hasher need to be reset here? Is it used more than once? I want to make sure we don't have a bug where we only actually hash the last bit of the data, because we reset it on every loop or something.

[mhofman]

Technically GetExportDataReader could be called more than once

[warner]

oh, I had assumed that the hasher was being created one-per-export, but maybe the eventHandler that it lives in is allocated statically, one-per-process?

[mhofman]

the artifact provided is passed to WriteSwingStoreExportToDirectory. That function is well behaved and will only read the stream export data once, but technically it could do it multiple times.

I agree the layering is weird, and in a perfects world I'd decouple the generation of the hash from the consumer that writes the data to disk, however I wanted to avoid that inefficiency. I suppose I could break boundaries, grab the iterator once on the outer scope, assert we only wrap the iterator once, and at the end verify that the iterator has been fully consumed.

[warner]

Hm, I can see that the "what do we hash" question is tied into the "what does golang encode" question. Giving golang the responsibility for framing/unframing, and then JSON-encoding the key/value pairs for hashing, is safe against concatenation/framing-confusion attacks, even if it's vulnerable to some hypothetical non-injectiveness in JSON.

If we were to, e.g., skip JSON, and just feed the key into the hasher, then feed the value into the hasher, then repeat for each pair: that would be vulnerable to framing confusion ({ ab: 'cd', ef: 'gh'} would hash the same as {a: 'b', c: 'de', fg: 'h'}). Without some kind of encoding/escaping, there's no way to avoid that (simply using a newline as a framing separator could still be confused with newlines in the keys or values).

One option would be to JSON-encode the key/value pairs first, then have the golang codec (struct? proto?) encode the resulting strings (one string per entry, not one for the key and a second for the value). Then the hasher could be fed those JSON.stringify([key,value]) strings, which would be safely escaped, and unambiguously framed. It would look weird, eschewing the built-in encoding support in favor of manual JSON, but it would meet my maxim of "hash (sign) what you say, only use what you hash (sign)". Down with "canonical encoding" in secure protocols!

[mhofman]

Then the hasher could be fed those JSON.stringify([key,value]) strings, which would be safely escaped, and unambiguously framed.

That's exactly what this does. Bonus is that the hash ends up strictly equal to hashing the resulting file, which is a concatenation of all the JSON stringified entries terminated by new lines

[warner]

Just to make sure I was clear, you wouldn't extract key and value from the golang-encoded data and then feed JSON.stringify([key,value]) to the hasher: that's the vulnerable-to-JSON-weirdness order. Rather, you'd feed JSON.stringify([key,value]) to the golang encoder, and have golang do whatever additional/native encoding it wants on that single-string-per-entry. Then, on the decoding side, you'd first hash that single-string-per-entry, then you do JSON.parse to get the key/value that you need to act upon. The rule could also be summarized as "no encoding during decode".

But I hear you, we're fighting against the golang codec pattern, which has already frustrated me in cosmos txn generation/signing. And we're dependent upon their codec being possible to use securely, which isn't a given (the confusion between nil and an empty list comes to mind).

[mhofman]

The missing bit is that the incoming stream of key value pairs may not be JSON encoded in the first place. It happens that's how we currently feed golang when dealing with export/import, but that may not always be the case.

[ivanlei]

Please address the fix comment.

Please choose to address the fix or not or comment and briefly say why it's not useful/necessary/a priority at this time.

[ivanlei]

fix or not: perf concern:

This is reallocating every one of the keys and values to convert from string to []byte. Because genesis export/import is RAM intensive, this might be worth fixing with either unsafe or sync.Pool. I leave it to you to decide if this is worth it.

https://josestg.medium.com/140x-faster-string-to-byte-and-byte-to-string-conversions-with-zero-allocation-in-go-200b4d7105fc has an example with a 0-alloc conversion.

func StringToBytes(s string) []byte {
 p := unsafe.StringData(s)
 b := unsafe.Slice(p, len(s))
 return b
}

[mhofman]

decided to leave as-is for now given the lack of positive impact in rudimentary perf tests

[ivanlei]

fix or not:

Consider using err = fmt.Errorf("...%w...",err,...) or sdkioerrors.Wrapf so the code can explicitly message that a hook failed while not losing info about the underlying error.

[michaelfig]

Our agoric-sdk/.github/workflows/golangci-lint.yml explicitly warns Found %w in ./golang/cosmos; please use %s instead. in order to discourage making validator-specific source file locations in stack traces part of consensus.

Wrapf without %w is safe because it doesn't actually stringify the stack trace of the error, but still exposes it if there's a panic, much like Hardened JS's disciplined stack trace handling for Error objects.

[mhofman]

I'm confused about this feedback. The hook is only called if there was no error in the first place, so there are no errors to wrap.

[ivanlei]

fix or not: perf concern

Once again using []byte() in 2 spots in this function which double the allocation.

[michaelfig]

LGTM conditional on satisfactorily addressing other reviewers' comments.

[michaelfig]

Our agoric-sdk/.github/workflows/golangci-lint.yml explicitly warns Found %w in ./golang/cosmos; please use %s instead. in order to discourage making validator-specific source file locations in stack traces part of consensus.

Wrapf without %w is safe because it doesn't actually stringify the stack trace of the error, but still exposes it if there's a panic, much like Hardened JS's disciplined stack trace handling for Error objects.

[mhofman]

I unlayered this from #9568 and will merge as-is given all other feedback has been addressed

addressed