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

R4R: Don't serialize Account Number and Sequence Number in signatures #2977

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Dec 2, 2018

This was not needed to be included within the tx body, as its in the
state.
Closes #2952

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests - existing tests suffoce

  • Updated relevant documentation (docs/) - auth spec doesn't exist yet

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

This was not needed to be included within the tx body, as its in the
state.
@ValarDragon ValarDragon force-pushed the dev/unserialize_seq_and_acc branch from 46387b6 to 4aa2730 Compare December 2, 2018 01:00
fmt.Println(len(cdc.MustMarshalBinaryBare([]sdk.Msg{msg1})))
fmt.Println(len(cdc.MustMarshalBinaryBare(tx)))
// output: 80
// 167
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the benchmark masked the true tx size, since it omitted account number, sequence number, and gas (as they were the default value). This increase is due to gas, but removal of accnum and sequence number will increase space savings.

@codecov
Copy link

codecov bot commented Dec 2, 2018

Codecov Report

Merging #2977 into develop will decrease coverage by 0.05%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           develop    #2977      +/-   ##
===========================================
- Coverage    56.29%   56.24%   -0.06%     
===========================================
  Files          120      120              
  Lines         8406     8387      -19     
===========================================
- Hits          4732     4717      -15     
+ Misses        3352     3348       -4     
  Partials       322      322

@ValarDragon ValarDragon requested review from alexanderbez and removed request for rigelrozanski December 2, 2018 01:14
@ValarDragon ValarDragon added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). ready-for-review labels Dec 2, 2018
@alexanderbez alexanderbez changed the title Don't serialize Account Number and Sequence Number in signatures R4R: Don't serialize Account Number and Sequence Number in signatures Dec 3, 2018
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the note ExampleTxSendSize, although I'm still not quite sure why it's a few bytes extra?

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add back the testcase? Otherwise LGTM.

x/bank/app_test.go Show resolved Hide resolved
x/auth/ante.go Show resolved Hide resolved
@cwgoes cwgoes merged commit 13e7816 into develop Dec 3, 2018
@cwgoes cwgoes deleted the dev/unserialize_seq_and_acc branch December 3, 2018 17:29
@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 3, 2018

Hope this clears up the situation bez: Before gas was 0, so it was being omitted as thats the protobuf default value. I went ahead and changed it to be non-zero so that it would show up in the total size. There were no bytes removed from that test, since that test didn't account for size of account number or sequence number to begin with. (They were both 0), I adapted the test locally to confirm that space was saved if it did account for them previously.

mircea-c pushed a commit that referenced this pull request Dec 5, 2018
… signatures

* Don't serialize Account Number and Sequence Number in signatures

This was not needed to be included within the tx body, as its in the
state.

* fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants