validate loaded accounts data size limit in transaction messages - #2005
Open
skyyycodes wants to merge 5 commits into
Open
validate loaded accounts data size limit in transaction messages#2005skyyycodes wants to merge 5 commits into
skyyycodes wants to merge 5 commits into
Conversation
The runtime accepts an integer in [1, 64 MiB]. Zero is rejected outright, and a value above the maximum is silently clamped down, so the transaction runs against a budget other than the one requested. Neither showed up until simulate or send. setTransactionMessageLoadedAccountsDataSizeLimit and setTransactionMessageConfig now throw at the point the value is set. Because zero is no longer a legal limit, the provisory limit that fillTransactionMessageProvisoryResourceLimits writes for version 1 messages moves from 0 to 1; the value occupies a fixed-width u32 either way, so the space reserved for the eventual estimate is unchanged.
Verified both halves of the claim against the Agave sanitizer rather than restating the issue: zero hits NonZeroU32::new(..).ok_or(InvalidLoadedAccounts- DataSizeLimit)?, and the top end is .min(MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES), so it clamps rather than fails. Link the expression that does both. Also call out in the changeset the break a consumer is most likely to hit: a buffered estimate that is no longer an integer, and passing 0 to mean "no limit".
The v1 sanitize_config in agave-sdk transaction-view checks heap size only, so the compute-budget-instruction error is not necessarily the operative path for a v1 transaction config. What does hold for every path is the budget type itself: loaded_accounts_data_size_limit is a NonZeroU32, so zero cannot be honored regardless of how it arrives. Cite both rather than generalising the one error.
Moving the v1 provisory value from 0 to 1 silently changed how estimateAndSet- ResourceLimits reads a message that still carries 0. It compared only against the new sentinel, so 0 started reading as an explicit, deliberate choice and was preserved rather than replaced. A message built by an earlier version of this package, or decoded from a transaction that was, would have been compiled and sent with a limit the runtime rejects outright, with nothing failing locally. Zero is never a legal limit, so it cannot be a deliberate choice. Treat it as provisory alongside 1. Also adds tests for behaviour that was agreed in review but never pinned: a decoded config carrying an illegal value stays editable field by field, the accepted range round trips through the config codec at a constant four bytes, and an out-of-range value from a custom estimator wrapper surfaces as a validation error rather than being encoded.
Adds tests from a differential sweep of the setter against both encodings. The accept boundary is exactly the integers in [1, 64 MiB] on the v1 config path and the legacy compute budget instruction path alike, and every accepted value survives compile, encode, decode and decompile unchanged while encoding to a constant size — which is what lets the provisory placeholder reserve the space its estimate will need. Non-numbers reaching the setter from untyped callers fail there rather than at the encoder; only undefined clears the limit. Also covers the documented workflow end to end: limits written by fill- TransactionMessageProvisoryResourceLimits are the ones estimateAndSetResource- Limits replaces.
🦋 Changeset detectedLatest commit: ddb8427 The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
BundleMonFiles updated (13)
Unchanged files (137)
Total files change +694B +0.12% Final result: ✅ View report in BundleMon website ➡️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Follow up to #2004. Same class of thing as #1961 / #1962 which #1972 handled for heap size and compute units, except this one has a wrinkle.
The loaded accounts data size limit has runtime bounds and Kit doesn't check them. Anything you hand to
setTransactionMessageLoadedAccountsDataSizeLimitgoes straight through and you find out at simulate/send time. The runtime takes an integer in[1, 64 * 1024 * 1024]. Unlike heap it doesn't have to be a multiple of anything, and unlike the other two fields0is not a legal value.Summary of Changes
Added
assertIsValidLoadedAccountsDataSizeLimittoresource-limit-validation.tsplus one more error code, and called it fromsetTransactionMessageLoadedAccountsDataSizeLimitandsetTransactionMessageConfig. Sits above the version switch like the other two so legacy and v0 get it as well, the limit applies whether the value travels as a compute budget instruction or a v1 config.I went and read agave for the exact behaviour this time instead of going off the issue text, since I got that wrong on the last PR.
sanitize_and_convert_to_compute_budget_limitsdoesNonZeroU32::new(requested).ok_or(TransactionError::InvalidLoadedAccountsDataSizeLimit)?and then.min(MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES), so zero genuinely errors and over-max genuinely clamps, exactly as you described. There's an agave test asserting the zero case too. The stronger thing is thatSVMTransactionExecutionAndFeeBudgetLimits.loaded_accounts_data_size_limitis itself aNonZeroU32, so zero isn't representable in the budget at all no matter which path it arrives by.One thing I couldn't pin down, and I'd rather ask than guess:
sanitize_configin agave-sdktransaction-viewonly checks heap size, it doesn't look at the loaded accounts data size limit. So for a v1 transaction specifically I couldn't find the code that rejects the zero. TheNonZeroU32means the semantics aren't in question, but the docblock currently links the compute budget instruction path and I'd like to link the right thing. If you know where a v1 config gets converted into budget limits, point me at it and I'll fix the link.The provisory limit
This is the bit the issue actually asks about. I went with
1.It's the smallest legal value, so nobody sets it deliberately and it works as a sentinel. The obvious alternative is 64 MiB, since that's what agave falls back to when the field is absent, but
estimateAndSetResourceLimitsalready documents that an explicitly set maximum is left alone on purpose, so using the max as the sentinel would collide with behaviour that's already there. The field is a fixed widthu32so0and1encode to the same bytes either way and the space reserved for the eventual estimate doesn't change. I checked that rather than assuming it, a v1 message compiles to the same 82 message bytes / 196 wire bytes with either value. Compute unit limit still uses0, that's still legal.There's a subtle thing that fell out of moving the sentinel and I want to flag it because it was green before I caught it.
estimateAndSetResourceLimitsdecides whether to re-estimate by comparing the existing value against the provisory one. If it only compares against the new1, then a message still carrying0— built by the current version of Kit, or decoded from a transaction that was — suddenly reads as an explicit deliberate choice and gets preserved. You'd then compile and send a transaction the runtime rejects outright, with nothing failing locally. Which is the exact failure this issue exists to prevent. So0is still treated as provisory alongside1. Zero is never a legal limit, so it can't be someone's deliberate choice.Happy to change the sentinel if you'd rather it were something else, it's a one line change plus a couple of test values.
Decoding is unaffected, same as last time. I extended the existing decompile test so a v1 message carrying a
0still decodes rather than throwing.Testing
Everything green on node and browser.
@solana/transaction-messages1397 unit tests, up from 1380@solana/kit165, up from 160@solana/errors263errorstransaction-messagestransactionssignersinstruction-planstransaction-introspectionrpcandkit, typecheck lint prettier and treeshakability includedmainto confirm I wasn't adding anyBeyond the usual boundary and
NaN/Infinity/ non-integer cases across all three versions, three things I added that felt worth it:A differential sweep of the setter over ~23 adversarial inputs against both encodings, to confirm the accept boundary is exactly the integers in
[1, 64 MiB]on the v1 config path and the legacy instruction path alike, and that they reject identically. Worth noting the rawu32encoder is perfectly happy with0and2^32-1, so this validation is the only thing constraining it. Non numbers coming from untyped callers (null,"60000",60000n) fail at the setter rather than reaching the encoder, onlyundefinedclears.A full wire round trip, message through compile, encode, decode and decompile, asserting the limit comes back unchanged at both ends of the range and that the encoded size is constant across it. That's the property the provisory placeholder actually depends on, so it seemed better to assert it than to claim it in the changeset.
And the workflow itself,
fillTransactionMessageProvisoryResourceLimitsfollowed byestimateAndSetResourceLimits. Both halves were tested separately but never composed, which is exactly the seam the sentinel bug above was hiding in.I also pinned the behaviour where a value from a custom estimator wrapper lands out of range.
Left out
The
estimateResourceLimitsu32 cap I mentioned on #1972 is still there, and it's slightly more live now. The docs invite custom estimator wrappers that buffer the returned values, and a wrapper doing* 1.1will now throw at the setter where it used to truncate at encode time. I added tests pinning that rather than changing it, since failing locally seems better than silently sending something the runtime rejects. If you'd rather that specific path clamped instead of threw, say the word and I'll do it here or in a follow up.Fixes
closes #2004