ledger-tool: Make blockstore slot functional with no tx metadata

[steviez]

Problem

A previous command consolidated code between the bigtable block and blockstore slot commands. In doing so, support for running the slot command on partial blocks was removed. However, support for running the command when tx metadata is absent was also accidentally removed.

Summary of Changes

This change re-adds the ability for the slot command to print transaction information even when the the blockstore does not contain transaction metadata.

[steviez]

@CriesofCarrots - This PR is marked draft as I have a couple TODO's, but I wanted to get your general thoughts / see if you hate the approach I took.

As the problem stated, a regular validator without --enable-rpc-transaction-data can't run ledger-tool blockstore slot because of #1255. When I did that PR, I was testing on a node with --enable-rpc-transaction-history so completely missed that I broke things for the no-tx-metadata case

[CriesofCarrots]

The one part I do rather dislike is pulling the shreds from blockstore twice in the Err case. It would be nice if we could write a Blockstore method that would return a block populated with something like TransactionWithStatusMetas, but I guess the problem with that type specifically is that it does not support VersionedTransactions for the MissingMetadata variant. Hmm, will ponder more.

[steviez]

The one part I do rather dislike is pulling the shreds from blockstore twice in the Err case. It would be nice if we could write a Blockstore method that would return a block populated with something like TransactionWithStatusMetas,

Yeah, that's a fair point. We might be able to shift some logic around / create intermediate functions to avoid code duplication. But ....

but I guess the problem with that type specifically is that it does not support VersionedTransactions for the MissingMetadata variant. Hmm, will ponder more.

Yeah exactly; as I was trying to figure out how to make this work, I re-read this comment several times. It makes sense for the context it was designed for, just made things a little more difficult for this case

agave/transaction-status/src/lib.rs

Lines 655 to 658 in a2a849f

	// Confirmed block with type guarantees that transaction metadata
	// is always present. Used for uploading to BigTable.
	#[derive(Clone, Debug, PartialEq)]
	pub struct VersionedConfirmedBlock {

[steviez]

Hmm, will ponder more.

Bumping this - did you have any more thoughts / how you feeling about the direction of this one ? Granted the remaining cleanup isn't much, but figure no point in doing it if we don't think this is a good direction

[CriesofCarrots]

Bumping this - did you have any more thoughts / how you feeling about the direction of this one ?

Sorry for the delay! I think I'm satisfied with the direction for now. If we decide later that the performance of pulling shreds twice isn't livable (and/or have a second use-case), we can new structs and a Blockstore method to do the efficient thing.

[steviez]

Ok, this one is finally ready for review. Some sample outputs from a blockstore without tx metadata columns:

verbose level == 0

Slot 288660608
  num_shreds: 2073, parent_slot: Some(288660607), next_slots: [288660609], num_entries: 188, is_full: true

verbose level == 1

Slot 288660608
  SlotMeta { slot: 288660608, consumed: 2073, received: 2073, first_shred_timestamp: 1724168324918, last_index: Some(2072), parent_slot: Some(288660607), next_slots: [288660609], connected_flags: ConnectedFlags(0x0), completed_data_indexes: {41, 84, 126, 167, 210, 252, 294, 336, 378, 420, 466, 509, 551, 593, 634, 676, 718, 760, 803, 845, 887, 929, 971, 1010, 1050, 1087, 1129, 1172, 1215, 1258, 1300, 1343, 1386, 1429, 1471, 1513, 1556, 1599, 1642, 1685, 1724, 1767, 1812, 1864, 1906, 1955, 2004, 2040, 2072} } is_full: true
  Transactions: 6127, hashes: 4000000, block_hash: AnHayKHKbfbzEt7UvLd27LcKHAPbUNLWxczSpqgPym9n
  Programs:
Vote111111111111111111111111111111111111111 : 6118
8tfDNiaEyrV6Q1U4DEXrEigs9DoDtkugzFbybENEbCDz: 26
ComputeBudget111111111111111111111111111111 : 16
cookr8CThnfEQZvvrB6zhh5K4X8XNkPjJi4uUDtkBuG : 12
11111111111111111111111111111111            : 4

verbose level == 2

Slot: 288660608
Parent Slot: 288660607
Blockhash: AnHayKHKbfbzEt7UvLd27LcKHAPbUNLWxczSpqgPym9n
Previous Blockhash: 11111111111111111111111111111111
Entry 0 - num_hashes: 60161, hash: Mp8GHigRYE9bChbk9kUTgzukHJE4fVYZ5TzakmA8e9M, transactions: 63, starting_transaction_index: 0
  Transaction 0:
    Version: legacy
    Recent Blockhash: EgN8xYrm3rqaK9wJeCiv1idT3zkgEDoAfrre4LTvv9rQ
    Signature 0: 3qbL5L1MUoJGmKukUL4XzJdaSW5kkjRJmhQvYwForN79kPnJqmryG85FPdLunzWYdJeia2trcwmDQwzayr4GtaBM
    Account 0: srw- 5ChLLcTk1tfJ9rzQKfsNjnEtEcvyr5KiEJJ4of9Zhqoj (fee payer)
    ...
    Status: Unavailable
  ...

[steviez]

With moving the print of slot into output_slot(), this became redundant

[CriesofCarrots]

This looks good! I don't have any comments of substance, but the test failure looks legitimate. Maybe better to say "legitimate": looks like it's failing because of the (root) addition, but I'm not concerned about that kind of breakage.

[CriesofCarrots]

Are there any ramifications of removing this error mapping? I guess only if/when we add transaction version 1 and don't update the Options here?

[steviez]

Are there any ramifications of removing this error mapping?

I don't think so; LedgerToolError has a corresponding variant

agave/ledger-tool/src/error.rs

Lines 13 to 14 in 0880cb6

	#[error("{0}")]
	TransactionEncode(#[from] solana_transaction_status::EncodeError),

and the underlying error type displays essentially the same information with different wording:

agave/transaction-status/src/lib.rs

Lines 58 to 62 in 0880cb6

	#[derive(Error, Debug, PartialEq, Eq, Clone)]
	pub enum EncodeError {
	#[error("Encoding does not support transaction version {0}")]
	UnsupportedTransactionVersion(u8),
	}

I think the mapping even being there in the first place was an oversight on my end in #1255

guess only if/when we add transaction version 1 and don't update the Options here?

That sounds right, altho we'd hit the error regardless of how we map and bubble the error up

[steviez]

Maybe better to say "legitimate": looks like it's failing because of the (root) addition

Yep, that's exactly the failure. The nitty gritty is:

• The macro to create a new ledger marks slot 0 as a root
• When you copy from one ledger to another, shreds are copied but the other columns like "roots" / "dead_slots" / etc are not.

agave/ledger/src/blockstore.rs

Line 4821 in 0880cb6

blockstore.set_roots(std::iter::once(&0))?;

but I'm not concerned about that kind of breakage.

Agreed, I have stripped it out

[steviez]

Are there any ramifications of removing this error mapping?

I don't think so; LedgerToolError has a corresponding variant

agave/ledger-tool/src/error.rs

Lines 13 to 14 in 0880cb6

	#[error("{0}")]
	TransactionEncode(#[from] solana_transaction_status::EncodeError),

and the underlying error type displays essentially the same information with different wording:

agave/transaction-status/src/lib.rs

Lines 58 to 62 in 0880cb6

	#[derive(Error, Debug, PartialEq, Eq, Clone)]
	pub enum EncodeError {
	#[error("Encoding does not support transaction version {0}")]
	UnsupportedTransactionVersion(u8),
	}

I think the mapping even being there in the first place was an oversight on my end in #1255

guess only if/when we add transaction version 1 and don't update the Options here?

That sounds right, altho we'd hit the error regardless of how we map and bubble the error up

[mergify]

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.