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 ability for ledger-tool to dump ledger transactions to CSV #10307

Closed
wants to merge 8 commits into from

Conversation

danpaul000
Copy link
Contributor

Problem

No tooling exists to create or index a database-like structure from the rocksDB history.

Summary of Changes

Add two subcommands slot-csv and print-csv to solana-ledger-tool, to print a single slot, or a range of slots (up to the entire contents of a rocksDB ledger), respectively.

Commands generate two related CSVs, the first "transactions file" contains txn information, accounts involved and any pre/post account balances. One txn per row. The second "instructions file" contains details of each instruction contained within any transaction. One instruction per row. Instruction rows contain the txn signature they came from to use as a key to index between the two files.

Related to #10149, but here using the ledger tool on a local rocksDB instead of querying whatever history the RPC node has. The output style is probably workable for the related issue.

@danpaul000
Copy link
Contributor Author

@mvines this is still WIP, but should suffice for our near-term accounting needs. Thoughts on my indexing split txn vs instruction?

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

@mvines mvines left a comment

Choose a reason for hiding this comment

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

The name of the commands, slot-csv and print-csv, are somewhat unintuitive to me. But I don't have a better suggestion yet, maybe after the next round of review something will strike me


#[derive(Serialize, Deserialize, Debug, Default, PartialEq)]
struct InstructionInfo {
txn_sig: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
txn_sig: Option<String>,
transaction_signature: Option<String>,

Please globally replace txn with transaction, sig with signature.

@@ -14,9 +14,17 @@ use solana_ledger::{
snapshot_utils,
};
use solana_sdk::{
clock::Slot, genesis_config::GenesisConfig, native_token::lamports_to_sol, pubkey::Pubkey,
clock::Slot,
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the implementation of these two new comments into a separate file? main.rs is getting way too big, and this will set a better example for the future

blockstore: &Blockstore,
slot: Slot,
txn_wtr: &mut Writer<File>,
instruction_wtr: &mut Writer<File>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
instruction_wtr: &mut Writer<File>,
instruction_writer: &mut Writer<File>,

don't make the reader guess what "wtr" means. water?

Comment on lines +1062 to +1063
let txn_csv_file = value_t_or_exit!(arg_matches, "txn_csv_file", String);
let instruction_csv_file = value_t_or_exit!(arg_matches, "instruction_csv_file", String);
Copy link
Member

Choose a reason for hiding this comment

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

These args aren't marked as .required(true) and yet there's value_t_or_exit!() here, which will cause the program to exit if they aren't provided. Should they be required?


let mut txn_info = TransactionInfo{
slot: Some(slot),
cluster_unix_timestamp: Some(genesis_creation_time + seconds_since_genesis),
Copy link
Member

Choose a reason for hiding this comment

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

This number is going to be inaccurate once we fix #9874
Not sure what do about that here, the value will likely change per-epoch in the bank and not actually be reflected in the ledger at all. I guess we can 🙈 here for now

transaction: &Transaction,
transaction_status: &Option<RpcTransactionStatusMeta>
) -> TransactionInfo {
let genesis_creation_time = open_genesis_config(&ledger_path).creation_time;
Copy link
Member

Choose a reason for hiding this comment

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

Calling open_genesis_config on every build_txn_info() is pretty expensive, load it once and pass in a &GenesisConfig?

txn_sig: Some(transaction.signatures[0].to_string()),
num_instructions: Some(transaction.message.instructions.len()),
num_accounts: Some(transaction.message.account_keys.len()),
..Default::default()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
..Default::default()
.. TransactionInfo::default()

let seconds_per_slot = DEFAULT_TICKS_PER_SLOT / DEFAULT_TICKS_PER_SECOND;
let seconds_since_genesis = (slot * seconds_per_slot) as i64;

let mut txn_info = TransactionInfo{
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this PR need some cargo fmt

@@ -602,6 +834,13 @@ fn main() {
.about("Print the ledger")
.arg(&starting_slot_arg)
)
.subcommand(
SubCommand::with_name("print-csv")
Copy link
Member

Choose a reason for hiding this comment

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

Could we get away with removing the "slot-csv" command if "print-csv" supported an --end-slot argument?

@stale
Copy link

stale bot commented Jun 5, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 5, 2020
@stale
Copy link

stale bot commented Jun 12, 2020

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants