-
Notifications
You must be signed in to change notification settings - Fork 254
feat(cosmos): mitigate install-bundle RPC limits by chunking
#11202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
agd tx swingset install-bundle RPC size limits by chunkinginstall-bundle RPC limits by chunking
418cd9d to
cd634f7
Compare
Deploying agoric-sdk with
|
| Latest commit: |
9e534bf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bc1ac97d.agoric-sdk.pages.dev |
| Branch Preview URL: | https://mfig-bundle-chunks.agoric-sdk.pages.dev |
7fdba5d to
6dcbe8c
Compare
gibson042
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First-pass comments, limited to the RPC/protobuf API.
| service Msg { | ||
| // Install a JavaScript sources bundle on the chain's SwingSet controller. | ||
| rpc InstallBundle(MsgInstallBundle) returns (MsgInstallBundleResponse); | ||
| // Send a chunk of a bundle to tolerate bandwidth constraints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Send a chunk of a bundle to tolerate bandwidth constraints. | |
| // Send a chunk of an artifact to tolerate RPC message size constraints. |
| (gogoproto.moretags) = "yaml:\"submitter\"" | ||
| ]; | ||
| // Either bundle or compressed_bundle will be set. | ||
| // Either bundle, compressed_bundle, or bundle_chunks will be set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Either bundle, compressed_bundle, or bundle_chunks will be set. | |
| // Of the fields for (complete, uncompressed) bundle, (complete) compressed | |
| // bundle, and chunked bundle, exactly one must be non-empty. |
| // MsgInstallBundleResponse is an empty acknowledgement that an install bundle | ||
| // message has been queued for the SwingSet kernel's consideration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // MsgInstallBundleResponse is an empty acknowledgement that an install bundle | |
| // message has been queued for the SwingSet kernel's consideration. | |
| // MsgInstallBundleResponse is either an empty acknowledgement that an install | |
| // bundle message has been queued for the SwingSet kernel's consideration, or | |
| // (for install bundle messages that specify chunking) a container for the | |
| // assigned id with which chunks must be associated. |
| // message has been queued for the SwingSet kernel's consideration. | ||
| message MsgInstallBundleResponse {} | ||
| message MsgInstallBundleResponse { | ||
| // The assigned pending installation, if chunks were specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // The assigned pending installation, if chunks were specified. |
| // The hash of the complete bundle. | ||
| string bundle_hash = 1 [ | ||
| (gogoproto.jsontag) = "bundle_hash", | ||
| (gogoproto.moretags) = "yaml:\"bundle_hash\"" | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no analog for this in MsgInstallBundle for the full uploads... what purpose is it serving here? If we're worried about corruption from someone uploading a chunk of a bundle without authority to do so, then I think we should instead be using cryptographic security (e.g., the initiator indicates authorized public keys). But in any case, we can't have such a field without documenting how to determine the associated algorithm (e.g., SHA-512).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m making a note that the field is the SHA-512 of the compartment-map.json inside the bundled zip file.
I am going to continue to think on whether we need this property or if we can do better with another mechanism.
We mostly use this algorithm because it solves two problems at once, integrity being one. We use the hash as a key in the SwingStore, and it conveniently converges if the same bundle gets submitted more than once. The convergence is nice but not critical. The fact that the keeper is assigning sequential artifact identifiers suggests we could have gone a different direction, but this solution builds upon the prior. For example, this hash will show up in vstorage in the published bundles topic, for acknowledging that the bundle was received and either accepted or rejected.
We may be able to do better by always providing an artifact identifier or the anticipated bundle hash and using that to report errors that occur before the hash can be inferred in the SwingSet controller. At this time, we do not reliably publish an error to vstorage for all the ways that installation can fail, like submitting your README.md or package.json instead of a bundle, since we can’t infer the bundle hash for either of those.
MsgBundleInstall doesn’t request the bundle hash because, if it has one, it can be inferred by attempting to parse the Zip, extracting the compartment-map.json, and computing the hash thereof.
| int64 start_block = 4 [ | ||
| (gogoproto.jsontag) = "start_block", | ||
| (gogoproto.moretags) = "yaml:\"start_block\"" | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int64 start_block = 4 [ | |
| (gogoproto.jsontag) = "start_block", | |
| (gogoproto.moretags) = "yaml:\"start_block\"" | |
| ]; | |
| int64 start_height = 4 [ | |
| (gogoproto.jsontag) = "start_height", | |
| (gogoproto.moretags) = "yaml:\"start_height\"" | |
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I’m intending to revise all the JSON and YAML tags to be conventionally camelCase and speak now or hold peace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC the Cosmos protobuf style is to use snake_case. Our JS codegen has the option to make them camelCase. Seems better to me to stick with the casing style in each language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but noticed along the way that the prevailing style is snake_case and decided to back off from this change, since it would make these inconsistent within the same file, and changing the convention would probably break something.
I’m now stuck on the very different issue that there are places where we rely on these structures to be trivially converted to JSON with JSON.stringify and uint64 is reïfied as BigInt in some codgens, string for JSON-Proto, and it’s just an inescapable mire.
| // The maximum number of blocks that an async installation can use. -1 is | ||
| // unlimited. | ||
| int64 installation_deadline_blocks = 7 [ | ||
| (gogoproto.jsontag) = "installation_deadline_blocks", | ||
| (gogoproto.moretags) = "yaml:\"installation_deadline_blocks\"" | ||
| ]; | ||
|
|
||
| // The maximum number of seconds that an async installation can use. -1 is | ||
| // unlimited. | ||
| int64 installation_deadline_seconds = 8 [ | ||
| (gogoproto.jsontag) = "installation_deadline_seconds", | ||
| (gogoproto.moretags) = "yaml:\"installation_deadline_seconds\"" | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // The maximum number of blocks that an async installation can use. -1 is | |
| // unlimited. | |
| int64 installation_deadline_blocks = 7 [ | |
| (gogoproto.jsontag) = "installation_deadline_blocks", | |
| (gogoproto.moretags) = "yaml:\"installation_deadline_blocks\"" | |
| ]; | |
| // The maximum number of seconds that an async installation can use. -1 is | |
| // unlimited. | |
| int64 installation_deadline_seconds = 8 [ | |
| (gogoproto.jsontag) = "installation_deadline_seconds", | |
| (gogoproto.moretags) = "yaml:\"installation_deadline_seconds\"" | |
| ]; | |
| // The maximum number of blocks between creation of a chunked artifact and | |
| // receipt of its final chunk. -1 is unlimited. | |
| int64 chunked_artifact_deadline_blocks = 7 [ | |
| (gogoproto.jsontag) = "chunked_artifact_deadline_blocks", | |
| (gogoproto.moretags) = "yaml:\"chunked_artifact_deadline_blocks\"" | |
| ]; | |
| // The maximum number of seconds between creation of a chunked artifact and | |
| // receipt of its final chunk. -1 is unlimited. | |
| int64 chunked_artifact_deadline_seconds = 8 [ | |
| (gogoproto.jsontag) = "chunked_artifact_deadline_seconds", | |
| (gogoproto.moretags) = "yaml:\"chunked_artifact_deadline_seconds\"" | |
| ]; |
| // Doubly-linked list in order of start block and time. | ||
| uint64 first_pending_id = 2 [ | ||
| (gogoproto.jsontag) = "first_pending_id", | ||
| (gogoproto.moretags) = "yaml:\"first_pending_id\"" | ||
| ]; | ||
|
|
||
| // The last pending id that has not expired or completed. | ||
| uint64 last_pending_id = 3 [ | ||
| (gogoproto.jsontag) = "last_pending_id", | ||
| (gogoproto.moretags) = "yaml:\"last_pending_id\"" | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Doubly-linked list in order of start block and time. | |
| uint64 first_pending_id = 2 [ | |
| (gogoproto.jsontag) = "first_pending_id", | |
| (gogoproto.moretags) = "yaml:\"first_pending_id\"" | |
| ]; | |
| // The last pending id that has not expired or completed. | |
| uint64 last_pending_id = 3 [ | |
| (gogoproto.jsontag) = "last_pending_id", | |
| (gogoproto.moretags) = "yaml:\"last_pending_id\"" | |
| ]; | |
| // The head of a doubly-linked list of pending chunked artifacts ordered by | |
| // ascending start block and time (i.e., the ID of the oldest). | |
| uint64 first_chunked_artifact_id = 2 [ | |
| (gogoproto.jsontag) = "first_chunked_artifact_id", | |
| (gogoproto.moretags) = "yaml:\"first_chunked_artifact_id\"" | |
| ]; | |
| // The tail of a doubly-linked list of pending chunked artifacts ordered by | |
| // ascending start block and time (i.e., the ID of the youngest). | |
| uint64 last_chunked_artifact_id = 3 [ | |
| (gogoproto.jsontag) = "last_chunked_artifact_id", | |
| (gogoproto.moretags) = "yaml:\"last_chunked_artifact_id\"" | |
| ]; |
| // The chunk is still in-flight. | ||
| CHUNK_STATE_IN_FLIGHT = 1; | ||
|
|
||
| // The chunk has been received. | ||
| CHUNK_STATE_RECEIVED = 2; | ||
|
|
||
| // The chunk has been processed. | ||
| CHUNK_STATE_PROCESSED = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the definitions of these states?
| // The pending installation node in the doubly-linked list of pending | ||
| // installations in descending order of age. This is used to track the state of | ||
| // a pending installation and to expire them efficiently. | ||
| message PendingInstallNode{ | ||
| // The id of the pending bundle installation. | ||
| uint64 pending_id = 1 [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // The pending installation node in the doubly-linked list of pending | |
| // installations in descending order of age. This is used to track the state of | |
| // a pending installation and to expire them efficiently. | |
| message PendingInstallNode{ | |
| // The id of the pending bundle installation. | |
| uint64 pending_id = 1 [ | |
| // A node in the doubly-linked list of pending chunked artifacts ordered by | |
| // ascending start block and time. This is used to efficiently expire stale | |
| // chunked artifacts. | |
| message PendingInstallNode{ | |
| // The id of the incomplete chunked artifact. | |
| uint64 chunked_artifact_id = 1 [ |
42024a6 to
4f7ede2
Compare
c817898 to
2222ba8
Compare
b92e051 to
e5bbb06
Compare
closes: #11923, closes: #11982 ## Description Introduce `OrchestrationAccountCommon.transferWithMeta(...): Promise<{ result: Promise<any>, meta: Record<string, any> }>`. This uses a `Promise<{ result, meta }>` record to be fulfilled "soon" when a long-term operation is started, and the `result` is a Promise that fulfills "on completion" to indicate when the transfer has returned successfuly. The `meta` record has `{ traffic: [{ src: [...], dst: [...], seq: number | null }, ...] }` to surface the details needed for an off-chain service to be able to track progress of the transfer operation. A related service is `CosmosOrchestrationAccount.executeEncodedTxWithMeta([...msgs])`, also with `Promise<{result, meta }>` to track progress of the ICA operation from Agoric to the attached chain. Putting these together, when the `transferWithMeta` is performed on an ICA, it will return two traffic entries-- one for the IBC send of a MsgTransfer to the remote chain, followed by another anticipating the acknowledgement of the ICS-20 transfer packet. ### Security Considerations ### Scaling Considerations Increases the size of Orchestration contract bundles, which may prevent uploading them like in #11981. We hope that #11202 will be a solution. Adds a new `zone.weakStore('icaAccountToDetails')...`, which is roughly equivalent to adding a new internal state record to the IcaAccount (one per portfolio subaccount). `icaAccountToDetails` is used for memoizing address details for each ICA used by the `*WithMeta` methods, so that the cost of looking up those details is amortized over many calls to `transferWithMeta` from the same ICA. ### Documentation Considerations New features intended for orchestration, and documented in the API. `tryDecodeResponse` now takes a `Codec` so that it can validate `Codec.typeUrl` instead of just using `Codec.toProtoMsg` without any context. ### Testing Considerations Will be tested as a part of Ymax implementation. ### Upgrade Considerations Needs upgrade testing to ensure all exoClassKits upgrades provide required facets.
25644dd to
7f14085
Compare
chore(cosmos): `make proto-gen` chore(cosmic-proto): `yarn codegen` test(cosmic-proto): update golden snapshots feat(cosmos): support `InstallBundle` and `SendChunk`
7f14085 to
9e534bf
Compare
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Switching primary submitter and continuing review at #12211 |
refs: #11634
Description
This change introducers machinery to
agdsuch that chains can receive bundles in chunks. The Go side of the house stages these chunks until a number of blocks have elapsed, such that if all the chunks arrive before the deadline, the accumulated bundle will pass up to the SwingSet controller, just as it would if it were submitted in a single transaction. This allows us to receive large bundles despite immovable limitations on the maximum RPC message size.Security Considerations
This change uses content hashes to ensure that the assembled bundles match the manifest of the original submission. Consequently, chunks can theoretically be submitted by multiple parties, but because they are addressed by hash, an attack would have to find a coïncident hash for a chunk of the same exact size.
Scaling Considerations
This will cause some data to buffer before its integrity is verified. This should impose limited overhead. Chunks are staged in a linked list to avoid expensive seeking while collecting expired chunks.
Documentation Considerations
This change will necessarily be accompanied by changes to the documented processes for submitting bundles, albeit Cosgov or the Agoric CLI.
Testing Considerations
Upgrade Considerations
Should not impact upgrade. However, publishing clients can sense whether this feature is present on the connected chain by looking for the SwingSet params for chunk size limits.