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

feat(rpc): fill size field in getblock with verbosity=2 #9327

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

conradoplg
Copy link
Collaborator

Motivation

This fills the size field in the getblock RPC when verbosity=2.

To support verbosity=1 we would need to either take a performance hit (load all transactions from a block to get their sizes) or change the database to create a tx -> size index (which would be good to avoid if possible)

Might close #9020

Solution

Add a BlockAndSize request to the state which reads the raw block header and txs, gets their sizes, computes the block size from them, and finally parses it all to return the block.

Tests

I adjusted existing tests. I also tested it manually with zcash-rpc-diff.

The block size calculation logic was also tested manually with a throwaway program that computed block sizes from the state with both approaches and asserted that they are equal.

Specifications & References

Follow-up Work

We need to decide if this is enough for now or if we want to support verbosity=1. In that case, we will need to create a new issue for it.

PR Checklist

  • The PR name is suitable for the release notes.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.
  • If the PR shouldn't be in the release notes, it has the
    C-exclude-from-changelog label.

Sorry, something went wrong.

@conradoplg conradoplg requested a review from a team as a code owner March 14, 2025 02:09
@conradoplg conradoplg requested review from oxarbitrage and removed request for a team March 14, 2025 02:09
@github-actions github-actions bot added the C-feature Category: New features label Mar 14, 2025
oxarbitrage
oxarbitrage previously approved these changes Mar 17, 2025
Copy link
Contributor

@oxarbitrage oxarbitrage 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 to me, thank you.

Comment on lines 194 to 195
let tx_count = CompactSizeMessage::try_from(txs.len()).unwrap();
let tx_raw = tx_count.zcash_serialize_to_vec().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be better to return None instead of panicking ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Blocks deserialized from the database are guaranteed to be serializable without errors. I should have used expect though, I will change it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 826c6bf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getblock: return the size field
2 participants