[GraphQL] Add a mutation payload size

[stefan-mysten]

Description

The mutation payload can be a lot higher than a query payload, due to the possibility of passing a large transaction data (e.g., publishing a package). This PR adds a different check for when a mutation is requested, and adds a max_tx_payload_size variable that holds the max bytes that can be sent through a GraphQL mutation request.
The total sum of txBytes + signatures of all GraphQL mutation or txBytes in a dryRunTransactionBlock query have to be below the max_tx_payload_size.

The max_tx_payload_size is computed based on the protocol_version -> max_tx_bytes and a Base64 overhead as follows:
max_tx_bytes * 4 / 3

Test plan

Added several tests.
cd crates/sui-graphql-rpc
cargo nextest run --features pg_integration -- test_query test_mutation test_dry_run_transaction test_transaction_dry_run

---

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

• Protocol:
• Nodes (Validators and Full nodes):
• Indexer:
• JSON-RPC:
• GraphQL: Adds a max_tx_payload_size variable to protect against large transaction queries. The sum of txBytes + signatures in all GraphQL mutation executeTransactionBlock nodes, or txBytes in dryRunTransactionBlock nodes from a query must be less than the max_tx_payload_size.

The max_tx_payload_size is computed based on the protocol_version -> max_tx_bytes and a Base64 overhead as follows: max_tx_bytes * 4 / 3.

Also adds a check that the overall query size is not larger than max_tx_payload_size + max_query_payload_size, where max_query_payload_size is the read part of the query.

• CLI:
• Rust SDK:

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 30, 2024 10:47pm

3 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Aug 30, 2024 10:47pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Aug 30, 2024 10:47pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview		Aug 30, 2024 10:47pm

[vercel]

@stefan-mysten is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

[stefan-mysten]

@amnn not quite sure how to make the default function to work correctly when a max_query_payload_size is set in the config toml file, as this will change the mutation payload size value.

Use cases:
🟢 no config toml, everything is loaded from default, all good
🔴 pass query payload size in config toml, the max mutation will not be correct. We need a way to update it if payload size is set and not default, but we cannot sum up different values in the impl function

    async fn max_mutation_payload_size(&self) -> u32 {
        self.limits.max_mutation_payload_size (+ self.limits.max_query_payload_size + signature_payload_size)
    }

because of use case below (if one sets mutation payload size in config toml, its value should include all payloads, thus we might double count).
🟡 pass query payload size and mutation payload size, all good as the mutation payload should contain both. However, I see how this field might be a bit confusing, as it should include all 3 payload sizes: max allowed tx bytes + overhead + query payload size + signature size.
For the previous case, it should only include the maximum tx bytes allowed + base64 overhead, so yeah, it might have hiccups.

[amnn]

This looks mostly good, but one consideration for us is whether people can abuse the more generous mutation limit to provide a much bigger query -- I've left a couple of questions and suggestions related to that.

Re: your question about interaction between mutation size and query payload size. I think your current approach (to treat the two as "independent" variables) is best (least confusing).

[stefan-mysten]

Re: your question about interaction between mutation size and query payload size. I think your current approach (to treat the two as "independent" variables) is best (least confusing).

Yes, but the current code will not be correct if a configuration has the max_query_payload_size variable set, but not the mutation payload variable. The mutation payload variable needs to take into account that value, only if that value is set manually. This should happen either in the default function or somewhere else after config is read and initialized to ensure its correct value.

[amnn]

Yes, but the current code will not be correct if a configuration has the max_query_payload_size variable set, but not the mutation payload variable. The mutation payload variable needs to take into account that value, only if that value is set manually, but not if it is not set....

You are saying that if only the max_query_payload_size is set then we aren't bumping up the default max_mutation_payload_size accordingly? If so, my position is that it's okay, and that's part of understanding the limits being changed.

[stefan-mysten]

Yes, but the current code will not be correct if a configuration has the max_query_payload_size variable set, but not the mutation payload variable. The mutation payload variable needs to take into account that value, only if that value is set manually, but not if it is not set....

You are saying that if only the max_query_payload_size is set then we aren't bumping up the default max_mutation_payload_size accordingly? If so, my position is that it's okay, and that's part of understanding the limits being changed.

Correct. We don't have a very good docs on this config limits yet, maybe that will alleviate create problems. I hope though most RPCs use the default ones or > than defaults, as otherwise might create some issues if folks switch (often) between RPCs.

[stefan-mysten]

Found out that the protocol config max_tx_bytes actually includes the signature, so we don't need to check for the signatures. So we have two options:

• separate tx bytes and signatures, and consider having signatures as part of the max query payload size?
• separate tx bytes and signatures, and have some contraints on how many signatures are allowed?

[stefan-mysten]

@amnn some more development here, I think it's almost there. I need to figure out one more thing (how to match variables' names with txBytes and signatures, and ignore the rest of the defined variables, for counting the mutation payload bytes).

I think I found a bug: query.len() does not resolve the variables in the query - shouldn't this read query payload be evaluated after the variables are resolved in the query? Basically, the fix is simple: just call stringify on the doc + variables. Not sure how it works with fragments, I'll have to poke at it tomorrow.

[amnn]

You keep refreshing this PR, so let me put these comments out first and then keep going :)

[amnn]

It's good that things are getting simpler, here are some things to keep pushing on:

• The potential stack overflow risk needs to be fixed.
• The handling of the variables map does not seem right (wrong thing being used as the key, and we shouldn't be summing its values at the end to figure out what the overall tx usage is -- use the tx_payload_size - available_budget for that, like you were doing before).
• Many, many, many more tests -- the ones I listed in the inline comments, and then also a regression test for each of these issues.

[amnn]

Strictly greater

[stefan-mysten]

So if a dryRun has a txBytes of 1000 and available budget is 1000, this should not be allowed? Tried to understand why it wouldn't be allowed?

[amnn]

So if a dryRun has a txBytes of 1000 and available budget is 1000, this should not be allowed?

No -- that dry run should be allowed, and if you use >= here you would not allow it, because you are checking whether the node_len has exceeded the budget here.

[stefan-mysten]

@amnn I addressed all comments except the more tests one. Will try to get to it in the morning, but I might not be able to.

[amnn]

Ended up making quite a few changes/refactors. Detailed in the commit message from me, but I'll copy it here for reference:

• Move the overall (initial) payload size check to happen before the document is parsed. Doing it after defeats the point of the check, because we end up doing work proportional to the size we're checking before we check it.
• Similarly, avoid copies as another example of work that is proportional to the size that we are checking, before we complete the check.
• Perform the transaction payload check after the inputs test. The former is implemented recursively, which means if it runs before the input nodes check (which limits query depth), it could stack overflow.
• Incorporate this check into LimitsTraversal, rather than as a separate recursive function. This allows it to take advantage of the context that it carries around, rather than passing multiple parameters down recursively.
• Removed the query_payload_too_large_size metric, because it uses the same histogram buckets as the query_payload_size metric which we are already logging to, so we don't gain any fidelity by tracking it separately.
• Fixed handling of variables, which was not taking into account variables that could show up embedded within other GraphQL expressions.
• Standardise names of tests so they are easier to search, and to selectively run in the test runner.
• Implement tests that use variables but are not expected to pass limits checks in a way that does not require running the test cluster. This way they are faster to run, and avoid issues related to hanging indefinitely.

The additional tests are to come, still.

[amnn]

The additional tests are to come, still.

Tests now added, this PR should be good to go now.

[wlmyng]

looking good!

[wlmyng]

Suggested change

	// Next, with the tranaction payloads accounted for, ensure the remaining query is within
	// Next, with the transaction payloads accounted for, ensure the remaining query is within

[wlmyng]

Suggested change

	"{message}. Requests must spend {max_tx_payload} bytes or fewer on transaction \
	"{message}. Requests are limited to {max_tx_payload} bytes or fewer on transaction \

Looking through the test examples in builder.rs I was a bit confused by the "must spend" language

[stefan-mysten]

Thanks @amnn and @wlmyng for finishing this PR! 🙇