Skip to content
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

Use CanonicalSerialization of RawTransaction between client and validator #606

Closed
wants to merge 1 commit into from

Conversation

@davidiw
Copy link

commented Aug 16, 2019

Motivation

RawTransactions are signed by clients and transmitted to all validators. Validators verify their authenticity by comparing the bundled signature to the RawTransaction. RawTransaction uses protobuf requiring that the validator and client keep both a copy of the original client serialized bytes and the deserialized object, due to RawTransaction's use of protobuf.

This diff eliminates the reliance on protobuf but at the same time introduce breaking changes for current clients and validators.

Test Plan

cargo test in libra/
verified that all code paths in core libra workflow adopt the changes correctly in the various integration and unit tests included there in

Related PRs

#724

@davidiw davidiw force-pushed the davidiw:raw_txn branch 3 times, most recently from 86726ff to f70bdd7 Aug 16, 2019
@davidiw davidiw force-pushed the davidiw:raw_txn branch from f70bdd7 to 1239204 Aug 19, 2019
libra_swarm/src/swarm.rs Outdated Show resolved Hide resolved
@kphfb

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

It looks like you have some conflicts, can you take a look at those?

@davidiw davidiw force-pushed the davidiw:raw_txn branch from 7814d59 to b089893 Aug 20, 2019
@davidiw

This comment has been minimized.

Copy link
Author

commented Aug 20, 2019

It looks like you have some conflicts, can you take a look at those?

Hmm... it is disappointing that github doesn't leverage the commit from master that the diff was based upon...

Fixed!

Copy link
Contributor

left a comment

A couple suggestions here and there. Pinging @msmouse @kphfb @wqfish for review

common/canonical_serialization/README.md Outdated Show resolved Hide resolved
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
@huitseeker

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

This PR is getting relatively unwieldy, so that reviewing #622 is becoming hard for size reasons. Can we focus on this one first, rebasing it an merging it before doing further work ?

@davidiw

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

This PR is getting relatively unwieldy, so that reviewing #622 is becoming hard for size reasons. Can we focus on this one first, rebasing it an merging it before doing further work ?

I think the goal is to get #622 completed first as when we were working on this effort we noticed that there were many deficiencies in place that need to be resolved before we take on rolling out any breaking changes.

With respect to #622 or LCS in general, the plan is:

  • Refactor LCS to modernize it (taking on a lot of your suggestions from an early PR and standardizing the API)
  • Improving test coverage via proptest (again taking on your suggestion)
  • Eliminating access to raw bytes -- all bytes must be serialized with a prefixed length
  • Adding a version field prior to all structs

The only feature currently missing is the version field prior to structs. I'm pondering a low cost fashion to do so to support our current structs as they are all effectively version 0.

All those changes should only break the protocol between validators, so I'm happy to do whatever to make reviewers live's easy to expedite reviews (i.e., multiple PRs). Suggestions / feedback is appreciated.

Once those changes are in place, this diff can be rebased and landed sometime Tuesday or later, so we give time for partners to fix their transaction generation logic in their clients.

I wish the UI gave me the ability to specify that this PR is not ready for review.

types/src/transaction.rs Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

Just the one comment about raw_txn_bytes

@davidiw davidiw force-pushed the davidiw:raw_txn branch 2 times, most recently from 0ad3d68 to 68e603c Aug 24, 2019
@davidiw davidiw force-pushed the davidiw:raw_txn branch 7 times, most recently from 5c1de83 to 0193ef7 Aug 24, 2019
@davidiw davidiw force-pushed the davidiw:raw_txn branch from 0193ef7 to 4d45319 Aug 28, 2019
@davidiw davidiw force-pushed the davidiw:raw_txn branch from 4d45319 to c2b7bf2 Aug 28, 2019
@davidiw davidiw added the breaking label Aug 28, 2019
@@ -1,2 +1,2 @@
[seed_peers]
8deeeaed65f0cd7484a9e4e5ac51fbac548f2f71299a05e000156031ca78fb9f = ["/ip4/SEED_IP/tcp/6180"]
57ff83747054695f2228042c26eb6a243ac73de1b9038aea103999480b076d45 = ["/ip6/::1/tcp/60535"]

This comment has been minimized.

Copy link
@huitseeker

huitseeker Aug 28, 2019

Contributor
  • localhost?
  • ip6? (not this is not the most frequent stack in public)

This comment has been minimized.

Copy link
@davidiw

davidiw Aug 29, 2019

Author

Hmm... I think there was some confusion on my behalf on how to update the validator-sets, Sonia and I chatted and I think this needs to be reverted.

Copy link
Contributor

left a comment

Just nits

raw_txn.set_expiration_time(expiration_time);
raw_txn.set_max_gas_amount(max_gas_amount.unwrap_or(MAX_GAS_AMOUNT));
raw_txn.set_gas_unit_price(gas_unit_price);
let raw_txn = RawTransaction::new(

This comment has been minimized.

Copy link
@huitseeker

huitseeker Aug 28, 2019

Contributor

There's a relative amount of alignment in favor of the builder pattern previously used here. It would be great to have one for RawTransaction as well.

raw_txn.set_expiration_time(expiration_time);
raw_txn.set_max_gas_amount(max_gas_amount.unwrap_or(MAX_GAS_AMOUNT));
raw_txn.set_gas_unit_price(gas_unit_price);
let raw_txn = RawTransaction::new(

This comment has been minimized.

Copy link
@huitseeker

huitseeker Aug 28, 2019

Contributor

Same as above

RawTransactions must be serialized in a deterministic fashion across
platforms so that signatures can be verified given given only an object
representation. Without deterministic serialization, two different
platforms may serialize the same RawTransaction into two different byte
representations invalidating a signature. Validators have thus far
passed around the RawTransaction object and the client protobuf byte
representation thus far.

This diff swaps the protobuf implementation for CanonicalSerialization
which guarantees deterministic ordering regardless of platform or
programming language.

genesis.blob needed to be regenerated as a result of the change.That was
done by running cargo run in the language/vm/vm_genesis directory.

The test in admission_control that was deleted tests for UnknownFields
in protobufs, a feature not present in CanonicalSerialization, hence the
test has no equivalent after this change and was removed.

Testing verified by running cargo test in root.
@davidiw davidiw force-pushed the davidiw:raw_txn branch from c2b7bf2 to c6b396e Aug 29, 2019
@davidiw davidiw closed this Aug 30, 2019
@davidiw davidiw deleted the davidiw:raw_txn branch Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.