Implement Trusted Vector Preallocation #1920

preston-evans98 · 2021-03-18T22:58:04Z

Motivation

Resolve #1880.

Solution

Implement the SafePreallocate trait, setting sane limits on blind preallocation of vectors. Implement proptests (where applicable) and unit tests to verify that the limits set by SafePreallocate are loose enough that all structurally valid messages can be deserialized.

This implementation deviates from the specification in #1880 in order to remove the use of min_specialization. Instead, we tighten the trait bound from impl<T> zcash_deserialize for Vec<T>, to impl<T: SafePreallocate> zcash_deserialize for Vec<T>, and implement SafePreallocate for all necessary types. If we need to deserialize a Vector type which can't be safely PreAllocated in future, we'll just have to write a manual impl for it.

Also note that the bounds in this implementation differ slightly from those in #1880 in order to remove special cases. For example, we don't set the max_allocation() for MetaAddr to 1000 as described in the Bitcoin specification. Instead, we set the limit based on the MAX_PROTOCOL_MESSAGE_SIZE and the serialized length of a MetaAddr. This mitigates the risk of that future changes to the Zcash protocol will render the bounds in SafePreallocate obsolete, potentially causing Zebra to fall out of consensus.

The code in this pull request has:

Documentation Comments
Unit Tests and Property Tests

Review

This is low priority. @teor2345

Related Issues

#1880, #1917

Follow Up Work

Improve the efficiency of the proptests for SafePreallocate.

teor2345

Thanks for doing all this work, it looks great.

I'd like some tests to make sure that the maximum preallocation is within the maximum block or message size. Otherwise, the code is really good.

I also have a bunch of suggestions and nitpicks around documentation.

While reviewing this PR, I discovered that Zebra preallocates a few other types. So after the fix in #1925 merges, this PR will need a SafePreallocate impl for JoinSplitData, String, and Script.

zebra-chain/src/block.rs

zebra-chain/src/block/header.rs

zebra-chain/src/block.rs

zebra-chain/src/transaction/serialize.rs

zebra-network/src/meta_addr.rs

zebra-chain/src/block.rs

zebra-network/src/protocol/external/inv.rs

zebra-chain/src/block.rs

zebra-chain/src/transaction/serialize.rs

teor2345

I also discovered String and Script preallocations, which I fixed in #1925.

Note that equihash::Solution and transparent::Input::Coinbase also do manual allocations, but their checks are very strict.

Comments which did not include replacement code will be addressed in a follow-up commit. Co-authored-by: teor <teor@riseup.net>

zebra-chain/src/serialization/zcash_deserialize.rs

Add tests to show that the largest allowed vec under TrustedPreallocate is small enough to fit in a Zcash block/message (depending on type). Add doc comments to all TrustedPreallocate test cases. Tighten bounds on max_trusted_alloc for some types. Note - this commit does NOT include TrustedPreallocate impls for JoinSplitData, String, and Script. These impls will be added in a follow up commit

Comments which did not include replacement code will be addressed in a follow-up commit. Co-authored-by: teor <teor@riseup.net>

Add tests to show that the largest allowed vec under TrustedPreallocate is small enough to fit in a Zcash block/message (depending on type). Add doc comments to all TrustedPreallocate test cases. Tighten bounds on max_trusted_alloc for some types. Note - this commit does NOT include TrustedPreallocate impls for JoinSplitData, String, and Script. These impls will be added in a follow up commit

teor2345 · 2021-03-26T11:35:23Z

Looks like you're ready for another review here.

I should get to it next week, but we're prioritising the Orchard network upgrade right now, so it might slip to early the week after.

Fix imports in block.rs

…nchor>

teor2345 · 2021-04-03T02:01:19Z

Looks good so far, see preston-evans98#1 for a fix to the merge conflict with #1946.

We'll probably want to move the tests to their own modules inside each test submodule. But we can do that in a follow-up PR.

I'm just doing some testing now, then I'll do a detailed review early next week.

Resolve merge conflict and implement TrustedPreallocate for Spend<SharedAnchor>

teor2345 · 2021-04-05T23:33:25Z

The Google Cloud Build error is a network failure downloading the Rust docker image.

teor2345 · 2021-04-05T23:44:44Z

zebra-network/src/meta_addr.rs

+    #[test]
+    /// Confirm that each MetaAddr takes exactly META_ADDR_SIZE bytes when serialized.
+    /// This verifies that our calculated `TrustedPreallocate::max_allocation()` is indeed an upper bound.
+    fn meta_addr_size_is_correct() {


The MetaAddr tests should be proptests, but I can make that change in a follow-up PR.

(In case we add more address types in future, for example, Tor onion addresses.)

teor2345 · 2021-04-05T23:47:00Z

zebra-network/src/protocol/external/inv.rs

+    #[test]
+    /// Confirm that each InventoryHash takes exactly INV_HASH_SIZE bytes when serialized.
+    /// This verifies that our calculated `TrustedPreallocate::max_allocation()` is indeed an upper bound.
+    fn inv_hash_size_is_correct() {


Similarly, the inv tests should be proptests, in case we add more variants in future.

teor2345

Looks good!

Let's merge this now, then I'll submit 2 small cleanup PRs.

preston-evans98 added 2 commits March 18, 2021 11:47

Implement SafePreallocate. Resolves ZcashFoundation#1880

fac8c2e

Add proptests for SafePreallocate

3342a00

zfnd-bot bot assigned preston-evans98 Mar 18, 2021

Fix whitespace

e372ee7

teor2345 self-requested a review March 19, 2021 00:47

teor2345 added this to the 2021 Sprint 5 milestone Mar 19, 2021

teor2345 mentioned this pull request Mar 19, 2021

Stop untrusted preallocation during deserialization #1925

Merged

4 tasks

teor2345 suggested changes Mar 19, 2021

View reviewed changes

teor2345 reviewed Mar 19, 2021

View reviewed changes

zebra-chain/src/block.rs Outdated Show resolved Hide resolved

teor2345 mentioned this pull request Mar 19, 2021

Check Transaction cardinality at parse time, to limit memory usage #1917

Closed

2 tasks

teor2345 reviewed Mar 19, 2021

View reviewed changes

zebra-chain/src/transaction/serialize.rs Outdated Show resolved Hide resolved

teor2345 reviewed Mar 19, 2021

View reviewed changes

zebra-chain/src/transaction/serialize.rs Show resolved Hide resolved

teor2345 mentioned this pull request Mar 19, 2021

V5 transaction rfc #1886

Merged

3 tasks

teor2345 reviewed Mar 19, 2021

View reviewed changes

Apply suggestions from code review

a7a0810

Comments which did not include replacement code will be addressed in a follow-up commit. Co-authored-by: teor <teor@riseup.net>

teor2345 changed the title ~~Implement Safe Vector Preallocation~~ Implement Trusted Vector Preallocation Mar 22, 2021

teor2345 reviewed Mar 22, 2021

View reviewed changes

zebra-chain/src/serialization/zcash_deserialize.rs Outdated Show resolved Hide resolved

mpguerra removed this from the 2021 Sprint 5 milestone Mar 22, 2021

preston-evans98 and others added 5 commits March 22, 2021 19:36

Implement SafePreallocate. Resolves ZcashFoundation#1880

b8fd760

Add proptests for SafePreallocate

7caf4d6

Fix whitespace

4b53306

Apply suggestions from code review

4265337

Comments which did not include replacement code will be addressed in a follow-up commit. Co-authored-by: teor <teor@riseup.net>

preston-evans98 added 4 commits March 22, 2021 19:52

Impl TrustedPreallocate for Joinsplit

e67ea0f

Impl ZcashDeserialize for Vec<u8>

c258eae

Merge branch 'main' of github.com:preston-evans98/zebra

99e46ba

teor2345 mentioned this pull request Mar 25, 2021

Increase coverage CI timeout #1947

Merged

Fix comment

6ddbf21

teor2345 added this to the 2021 Sprint 6 milestone Mar 26, 2021

teor2345 added 2 commits April 3, 2021 11:49

Merge remote-tracking branch 'origin/main' into prealloc

d0cc399

Fix imports in block.rs

Arbitrary, TrustedPreallocate, Serialize, and tests for Spend<SharedA…

abd2632

…nchor>

teor2345 mentioned this pull request Apr 3, 2021

Resolve merge conflict and implement TrustedPreallocate for Spend<SharedAnchor> preston-evans98/zebra#1

Merged

Merge pull request #1 from teor2345/prealloc

2b376de

Resolve merge conflict and implement TrustedPreallocate for Spend<SharedAnchor>

teor2345 reviewed Apr 5, 2021

View reviewed changes

teor2345 approved these changes Apr 5, 2021

View reviewed changes

teor2345 merged commit 0daaf58 into ZcashFoundation:main Apr 5, 2021

This was referenced Apr 6, 2021

Move the preallocate tests into their own files #1977

Merged

Add InventoryHash and MetaAddr proptests #1985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Trusted Vector Preallocation #1920

Implement Trusted Vector Preallocation #1920

preston-evans98 commented Mar 18, 2021

teor2345 left a comment •

edited

Loading

teor2345 left a comment

teor2345 commented Mar 26, 2021

teor2345 commented Apr 3, 2021

teor2345 commented Apr 5, 2021

teor2345 Apr 5, 2021

teor2345 Apr 5, 2021

teor2345 Apr 5, 2021

teor2345 left a comment

Implement Trusted Vector Preallocation #1920

Implement Trusted Vector Preallocation #1920

Conversation

preston-evans98 commented Mar 18, 2021

Motivation

Solution

Review

Related Issues

Follow Up Work

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Mar 26, 2021

teor2345 commented Apr 3, 2021

teor2345 commented Apr 5, 2021

teor2345 Apr 5, 2021

Choose a reason for hiding this comment

teor2345 Apr 5, 2021

Choose a reason for hiding this comment

teor2345 Apr 5, 2021

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment •

edited

Loading