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

Add zcash_serialized_size() to ZcashSerialize trait #2824

Merged
merged 9 commits into from
Oct 6, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Oct 4, 2021

Motivation

I am not totally sure this is the way to do it but just a quick attempt to resolve #2778

I am open to just close the PR if this is not a good idea or make changes if needed.

Consensus Rules

The mempool of a node holds a set of transactions. Each transaction has a cost, which is an integer defined as:
max(serialized transaction size in bytes, 4000)

https://zips.z.cash/zip-0401#specification

Solution

Use the already in place zcash_serialize_to_vec() helper function to simplify the task of getting a serialized size of any object, in this case we are using it to get the size of a transaction.

Review

I think anyone can review.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think there's two ways we can be more efficient here:

  • stop allocating RAM and writing data to it - just sum the serialized sizes instead
  • calculate the serialized size once, then add it to UnminedTx as a new field

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few performance tweaks we could do.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

@teor2345 teor2345 enabled auto-merge (squash) October 6, 2021 21:10
@teor2345 teor2345 merged commit f1718f5 into ZcashFoundation:main Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the serialized transaction size to UnminedTx
2 participants