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

v1.17: Output BankHashDetails file when leader drops its' own block (backport of #34256) #34275

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Nov 29, 2023

This is an automatic backport of pull request #34256 done by Mergify.


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

Currently, the file is generated when a node drops a block that was
produced by another node. However, it would also be beneficial to see
the account state when a node drops its' own block.

Output the file in this additional failure codepath

(cherry picked from commit 935e06f)
@steviez
Copy link
Contributor

steviez commented Nov 29, 2023

Going to let CI run - will then tag Ashwin / Will / Trent as reviewers

  • Ashwin for content of PR
  • Will for whether it is suitable to BP
  • Trent for both as he reviewed the initial PR when we output these files as non-leader

I know we're being stingy with backports at the moment for both branches; however, this commit is very beneficial in debugging, both in time savings as well as giving us the potential to view state in a divergent node that may not recreate itself in attempts to reproduce.

I thing we should definitely BP this for v1.17, I'd argue for v1.16 too but that is dependent on #34257 getting approval

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #34275 (a0b3e93) into v1.17 (a55711d) will increase coverage by 0.0%.
The diff coverage is 75.0%.

Additional details and impacted files
@@           Coverage Diff           @@
##            v1.17   #34275   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         803      803           
  Lines      218021   218030    +9     
=======================================
+ Hits       178427   178466   +39     
+ Misses      39594    39564   -30     

Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

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

The change looks good to me.

My personal opinion is that we should backport this to 1.16 as well in case there are anymore mismatches before the upgrade to 1.17 happens in January. The bank hash details are invaluable to debugging consensus failures that are not easily reproducible.

@steviez steviez merged commit ff1d9a6 into v1.17 Nov 30, 2023
31 checks passed
@steviez steviez deleted the mergify/bp/v1.17/pr-34256 branch November 30, 2023 22:36
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.

None yet

4 participants