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 option to record transactions to ledger-tool #181

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

seanyoung
Copy link

@seanyoung seanyoung commented Mar 11, 2024

Problem

When diagnosing a consensus failure, ledger-tool has the --record-slots and --verify-slots option to find the diverging slot. This PR add a --record-slots-config=tx option, which writes all transactions to the recording. This can be be used to find the exact transaction where the failure was.

Short workflow:

  • ledger-tool verify --records-slots --record-slots-config=tx
  • (make change to the runtime)
  • ledger-tool verify --record-slots slots-before.json --record-slots-config=tx
  • diff -u slots.json slots-before.json

Longer Workflow if the files get too large:

  • ledger-tool verify --records-slots
  • (make change to the runtime)
  • ledger-tool verify --verify-slots => slot 102 diverges
  • ledger-tool verify --record-slots-config=tx --record-slots=after.json --halt-at-slot 102
  • (revert runtime change)
  • ledger-tool verify --record-slots-config=tx --record-slots=before.json --halt-at-slot 102
  • diff -u after.json before.json

Now the diff will show exactly where things went wrong.

Summary of Changes

Fixes #

@seanyoung seanyoung marked this pull request as ready for review March 11, 2024 18:02
@seanyoung
Copy link
Author

@alessandrod I found this helpful for debugging consensus failures, wdyt

ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@seanyoung seanyoung force-pushed the records-txs branch 4 times, most recently from 3634918 to 75ea1fa Compare March 16, 2024 10:39
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2024

Codecov Report

Attention: Patch coverage is 59.09091% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (025ed45) to head (214cc9b).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #181   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         846      846           
  Lines      228889   228900   +11     
=======================================
+ Hits       187419   187441   +22     
+ Misses      41470    41459   -11     

@HaoranYi
Copy link

HaoranYi commented Mar 29, 2024

@alessandrod I found this helpful for debugging consensus failures, wdyt

I used this PR when working on my features. The transaction log is very helpful. I would say let's merge it.

Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

This is a good idea overall, and piggybacking off of the TransactionStatusSender is a seemingly cleaner solution than the initial crack I took at adding this a while back. No code changes in /ledger/src 🚀 .

A few comments / questions right now, will take a second pass at this later today again

ledger-tool/src/ledger_utils.rs Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
runtime/src/bank/bank_hash_details.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@seanyoung seanyoung force-pushed the records-txs branch 2 times, most recently from 214cc9b to 63f5475 Compare April 3, 2024 15:13
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Apologies on the delayed review again. I think we've previously discussed that it would be nice to stream the details as they come in. I had initially been hoping/planning to make the BankHashDetails structure support streaming, and then land your PR (which would hopefully be simplified by doing so). But, that clearly hasn't happened as other things have come up and I haven't found the time.

So, I'll stop letting perfect get in the way of good as this is definitely a useful change. Things look fairly good as-is, just one nit right now + looks like you need to rebase.

ledger-tool/src/main.rs Outdated Show resolved Hide resolved
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

I can try to follow up with some of the enhancements we've discussed offline (ie streaming)

@seanyoung seanyoung merged commit 873808c into anza-xyz:master Jun 12, 2024
52 checks passed
@seanyoung seanyoung deleted the records-txs branch June 12, 2024 10:43
samkim-crypto pushed a commit to samkim-crypto/agave that referenced this pull request Jul 31, 2024
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.

5 participants