-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix transaction logs and inner ixs for leader nodes #18395
Conversation
6115d2c
to
8aa1fd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! @CriesofCarrots - can you please scroll through this PR too, I think you're more familiar with some parts of the changes here than I
ledger/src/blockstore_processor.rs
Outdated
rent_debits: Vec<RentDebits>, | ||
) { | ||
let slot = bank.slot(); | ||
let (inner_instructions, transaction_logs) = if !self.enable_cpi_and_log_storage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this removal intended? The presence/absence of --enable-cpi-and-log-storage
validator flag has no effect now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As context, using the Option to pass into the TransactionStatusService was a contortion to avoid having to collect empty Vecs for when --enable-cpi-and-log-storage
is not present: #14922 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! I thought this flag was redundant with the enable_cpi_recording
and enable_log_recording
flags. I added it back and I believe the option handling should match the previous behavior as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Pull request has been modified.
Codecov Report
@@ Coverage Diff @@
## master #18395 +/- ##
=========================================
- Coverage 82.5% 82.5% -0.1%
=========================================
Files 436 436
Lines 121916 121922 +6
=========================================
- Hits 100700 100678 -22
- Misses 21216 21244 +28 |
* Fix transaction logs and inner ixs for leader nodes * Fix cpi log storage flag (cherry picked from commit 5dd399d) # Conflicts: # runtime/src/bank.rs
…#18448) * Fix transaction logs and inner ixs for leader nodes (#18395) * Fix transaction logs and inner ixs for leader nodes * Fix cpi log storage flag (cherry picked from commit 5dd399d) # Conflicts: # runtime/src/bank.rs * resolve conflict Co-authored-by: Justin Starry <[email protected]>
Problem
Leader nodes with rpc transaction history enabled can return corrupted transaction logs and inner instructions when they create and process a transaction batch which contains some invalid transactions.
Notes:
Summary of Changes
Fixes #