Limited Deserialize isn't limiting anything#10952
Limited Deserialize isn't limiting anything#10952CriesofCarrots merged 2 commits intosolana-labs:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10952 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.1%
=========================================
Files 317 318 +1
Lines 72709 72724 +15
=========================================
- Hits 59752 59750 -2
- Misses 12957 12974 +17 |
CriesofCarrots
force-pushed
the
bincode-limit
branch
from
828237e to
17f7fe8
Compare
CriesofCarrots
force-pushed
the
bincode-limit
branch
from
17f7fe8 to
9034827
Compare
| bincode::config() | ||
| .limit(PACKET_DATA_SIZE as u64) | ||
| .deserialize(&wire_transaction) | ||
| .deserialize_from(&wire_transaction[..]) |
There was a problem hiding this comment.
I'm tempted to make this a plain-old bincode::deserialize, since we check the wire_transaction length and return a better error message immediately before this.
There was a problem hiding this comment.
If there isn't a performance impact I would say keep this consistent by using deserialize_from. Keeping it is also a good measure in case the other checks change or are removed for some reason.
There was a problem hiding this comment.
Sounds good. That's where I landed also
|
Good find @CriesofCarrots the changes in this PR look good to me. I wonder if there is any performance impact of this change. |
@jackcmay , I benchmarked the bincode methods, and I took a glance through the CI benches, and nothing stood out as significantly different. Are there any specific benches I should pay attention to? |
|
Not really, lgtm! |
* Add failing test * Use deserialize_from to enable limit
* Update spl-token to v2.0 (#11884) * Update account-decoder to spl-token v2.0 * Update transaction-status to spl-token v2.0 * Update rpc to spl-token v2.0 * Update getTokenSupply to pull from Mint directly * Fixup to spl-token v2.0.1 (cherry picked from commit 76be36c) # Conflicts: # Cargo.lock # account-decoder/Cargo.toml # core/Cargo.toml # core/src/rpc.rs # transaction-status/Cargo.toml * Fix non-Cargo.lock conflicts * Limited Deserialize isn't limiting anything (#10952) * Add failing test * Use deserialize_from to enable limit * Cargo.lock * chore(deps): bump bincode from 1.2.1 to 1.3.1 (#10867) * chore(deps): bump bincode from 1.2.1 to 1.3.1 Bumps [bincode](https://github.com/servo/bincode) from 1.2.1 to 1.3.1. - [Release notes](https://github.com/servo/bincode/releases) - [Commits](https://github.com/servo/bincode/commits) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * [auto-commit] Update all Cargo lock files * Switch from deprecated method * Add options to maintain behavior with bincode::options() Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <dependabot-buildkite@noreply.solana.com> Co-authored-by: Tyera Eulberg <tyera@solana.com> Co-authored-by: Tyera Eulberg <tyera@solana.com> Co-authored-by: Tyera Eulberg <teulberg@gmail.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <dependabot-buildkite@noreply.solana.com>
2 participants
Problem
We use this pattern several places.
However, it doesn't actually limit anything. See bincode issue: bincode-org/bincode#299
Also see failing test below; the 2nd assert fails because limited_deserialize does not return an error.
Summary of Changes
Use
deserialize_frominsteadShred uses this same pattern, but using
deserialize_fromwould require switching the trait bound to DeserializeOwnedsolana/ledger/src/shred.rs
Line 138 in 2fdbb97
Doing this seemed to build fine, but I am not sure what deeper implications the change might have.