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

change(log): Log a cute message for blocks that were mined by Zebra (off by default) #6098

Merged
merged 12 commits into from Feb 23, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 3, 2023

Motivation

It's nice to see when Zebra has mined some blocks:

2023-02-03T05:52:06.475443Z INFO zebra_state::service::write: looks like this block was mined by Zebra! text="z🦓" data="7af09fa693" height=2215022 hash=00531335a993d94aed848c76cceba5274e2913b14a286d6a485b44444eb3a9d6
2023-02-03T05:52:06.475598Z INFO zebra_rpc::methods::get_block_template_rpcs: submit block accepted block_hash=block::Hash("00531335a993d94aed848c76cceba5274e2913b14a286d6a485b44444eb3a9d6") block_height="2215022"

This is part of #5934, testing v5 transactions, extra coinbase data, and other minor changes allowed by the consensus rules or RPCs.

Specifications

Coinbase transactions can contain up to 100 bytes of data, but the consensus rules only specify the first 1-6 bytes.

Solution

  • Mark Zebra coinbase transactions with extra coinbase data if like_zcashd is false or if specifically configured
    • Make debug_like_zcashd into a config rather than a constant
    • Make a config for extra_coinbase_data
  • Log when we commit a block mined by Zebra to our state
    • Don't log control characters
    • Rate-limit to 1 in 1000 blocks

Related changes:

  • Reduce logging instrumentation during block writes
  • Add network and commit to write task logs

Review

This is a low-priority review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Low ❄️ A-diagnostics Area: Diagnosing issues or monitoring performance A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes labels Feb 3, 2023
@teor2345 teor2345 self-assigned this Feb 3, 2023
@teor2345 teor2345 changed the title change(log): Log a cute message when we verify a block that was mined by Zebra change(log): Log a cute message for blocks that were mined by Zebra Feb 3, 2023
@teor2345
Copy link
Contributor Author

teor2345 commented Feb 3, 2023

When I mine a block with this PR, here is what zebrad logs:
Screenshot 2023-02-03 at 16 08 25

And when it is loading blocks mined by other zebrads:
Screenshot 2023-02-03 at 16 08 47

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Merging #6098 (f85673c) into main (6adf2b5) will increase coverage by 0.03%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6098      +/-   ##
==========================================
+ Coverage   77.98%   78.02%   +0.03%     
==========================================
  Files         304      304              
  Lines       39087    39185      +98     
==========================================
+ Hits        30482    30574      +92     
- Misses       8605     8611       +6     

@teor2345 teor2345 marked this pull request as ready for review February 10, 2023 09:01
@teor2345 teor2345 requested review from a team as code owners February 10, 2023 09:01
@teor2345 teor2345 requested review from upbqdn and removed request for a team February 10, 2023 09:01
@arya2 arya2 added the no-review-reminders Turn off review reminders label Feb 15, 2023
@teor2345 teor2345 changed the title change(log): Log a cute message for blocks that were mined by Zebra change(log): Log a cute message for blocks that were mined by Zebra (off by default) Feb 20, 2023
@teor2345 teor2345 removed the no-review-reminders Turn off review reminders label Feb 22, 2023
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for adding this, it's nice to see Zebra committing blocks that were mined with Zebra.

zebra-state/src/service/write.rs Show resolved Hide resolved
zebra-state/src/service/write.rs Show resolved Hide resolved
zebra-chain/src/transaction/builder.rs Show resolved Hide resolved
zebra-state/src/service/write.rs Show resolved Hide resolved
Copy link
Contributor Author

@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.

Thanks for the review, I'll do the metrics in a follow-up PR.

zebra-state/src/service/write.rs Show resolved Hide resolved
zebra-state/src/service/write.rs Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Feb 22, 2023
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

Looks great, I left a few minor suggestions.

I also noticed this comment https://github.com/zcashfoundation/zebra/blob/f85673c9aa8794efff68f34741afe49abebce5c0/zebra-chain/src/transparent/serialize.rs#L184 says the "The height may produce 0..=5 initial bytes of coinbase data.", which was confusing since the consensus rules say "The length of heightBytes MUST be in the range {1 .. 5}." Should we refactor that comment?

zebra-chain/src/transparent.rs Show resolved Hide resolved
zebra-state/src/service/write.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

teor2345 commented Feb 22, 2023

mergify bot added a commit that referenced this pull request Feb 22, 2023
@mergify mergify bot merged commit ec43d63 into main Feb 23, 2023
@mergify mergify bot deleted the label-zebra-blocks branch February 23, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Diagnosing issues or monitoring performance A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants