Skip to content

[Master] Fix low hanging issues#1116

Merged
esemeniuc merged 2 commits intomaster_bamfrom
eric/master-tweak
Jan 9, 2026
Merged

[Master] Fix low hanging issues#1116
esemeniuc merged 2 commits intomaster_bamfrom
eric/master-tweak

Conversation

@esemeniuc
Copy link
Copy Markdown
Collaborator

@esemeniuc esemeniuc commented Jan 6, 2026

Bugs:

  • Remove the duplicate checkout in .github/workflows/cargo.yml.
  • Avoid storing empty bam url via admin-rpc
  • Running agave-validator --ledger /solana/ledger set-bam-config will stop bam (should have --bam-url to empty to stop bam), see stoppage at https://jitolabs.grafana.net/goto/ef9gr7lzl9dz4e?orgId=1
  • Running agave-validator --ledger /solana/ledger set-bam-config --bam-url "" will set url to empty string (not disable! we'd keep trying to connect to "")

Perf:

  • Rework BAM prevalidation/verify to reuse buffers, pass batch indexes instead of cloning in bam_receive_and_buffer.rs

Correctness:

  • bam_receive_and_buffer.rs: Blacklisted account rejection reports a TransactionError with DeserializationErrorReason::SanitizeError as the reason. That enum value doesn’t match TransactionErrorReason (it maps to INSUFFICIENT_FUNDS_FOR_FEE), so BAM will see the wrong error code.

@esemeniuc esemeniuc requested a review from dgski January 6, 2026 21:49
@esemeniuc esemeniuc changed the title add Reduce allocs Jan 6, 2026

- uses: actions/checkout@v6
with:
submodules: 'recursive'
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

duplicate?

let mut last_metrics_report = Instant::now();
let mut metrics = BamReceiveAndBufferMetrics::default();
let mut stats = ReceivingStats::default();
let mut prevalidated = Vec::new();
Copy link
Copy Markdown
Collaborator Author

@esemeniuc esemeniuc Jan 7, 2026

Choose a reason for hiding this comment

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

reuse across the containers instead of alloc'ing in the while loop


Ok((
atomic_txn_batch.clone(),
batch_index,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

don't clone

.zip(batch.sanitized_transactions().iter()),
processed_transactions.into_iter(),
);
self.seq_not_conflict_batch_reusables.set(reusables);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

don't lose the reusables, otherwise we keep the empty containers

processed_transactions
.into_iter()
.zip(batch.sanitized_transactions().iter()),
processed_transactions.into_iter(),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

already zipped above

type PrevalidationResult = Result<(AtomicTxnBatch, bool, u32, u64), (Reason, u32)>;
type PrevalidationOutput = (Vec<PrevalidationResult>, ReceivingStats);

type PrevalidationResult = Result<(usize, bool, u32, u64), (Reason, u32)>;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

avoid clones


for (transaction, transaction_info) in transactions {
let account_keys = transaction_info.account_keys();
let write_account_locks = account_keys
Copy link
Copy Markdown
Collaborator Author

@esemeniuc esemeniuc Jan 7, 2026

Choose a reason for hiding this comment

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

old code does 3 iterations over all account keys for the txn

.iter()
.enumerate()
.filter_map(|(index, key)| (!transaction_info.is_writable(index)).then_some(key));
let has_contention = write_account_locks.clone().any(|key| {
Copy link
Copy Markdown
Collaborator Author

@esemeniuc esemeniuc Jan 7, 2026

Choose a reason for hiding this comment

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

this is a full iteration over all account keys

transaction_read_locks.clear();
let mut has_contention = false;
for (key, writable) in
TransactionAccountLocksIterator::new(transaction_info).accounts_with_is_writable()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

single iteration over all account keys

processed_results: vec![
TransactionResult::NotCommitted(
NotCommittedReason::PohTimeout,
NotCommittedReason::PohTimeout, // Note: ChannelFull, ChannelDisconnected, MaxHeightReached are misreported as a PohTimeout
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

should fix this

@esemeniuc esemeniuc changed the title Reduce allocs [Master] Fix low hanging issues Jan 7, 2026
fn normalize_bam_url(url_str: &str) -> Result<String, BamUrlError> {
// If empty, return empty string to disable BAM
if url_str.trim().is_empty() {
return Ok(String::new());
Copy link
Copy Markdown
Collaborator Author

@esemeniuc esemeniuc Jan 7, 2026

Choose a reason for hiding this comment

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

this is dangerous

/// ```
pub fn extract_bam_url(matches: &ArgMatches) -> Result<Option<String>, BamUrlError> {
match matches.value_of("bam_url") {
Some(url) => normalize_bam_url(url).map(Some),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

since above always returns Ok(""), we will return Ok(Some("")) here

);
}

*meta.bam_url.lock().unwrap() = bam_url;
Copy link
Copy Markdown
Collaborator Author

@esemeniuc esemeniuc Jan 7, 2026

Choose a reason for hiding this comment

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

this would cause us to store new empty url here

@esemeniuc esemeniuc marked this pull request as ready for review January 7, 2026 03:51
@esemeniuc esemeniuc force-pushed the eric/master-tweak branch 2 times, most recently from 8b07d91 to 2453df2 Compare January 7, 2026 14:15
@@ -807,7 +814,7 @@ impl ReceiveAndBuffer for BamReceiveAndBuffer {

// If BAM is not enabled, drain the channel
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this would multiply num_dropped_without_parsing for each packet dropped

}

pub fn execute(subcommand_matches: &ArgMatches, ledger_path: &Path) -> crate::commands::Result<()> {
if !subcommand_matches.is_present("bam_url") {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

don't delete bam config if --bam-url not passed to agave-validator set-bam-config

jito_protos::proto::bam_types::TransactionError {
index: index as u32,
reason: DeserializationErrorReason::SanitizeError as i32,
reason: TransactionErrorReason::SanitizeFailure as i32,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

blacklisted account rejection reports a TransactionError with DeserializationErrorReason::SanitizeError as the reason. That enum doesn’t match TransactionErrorReason (it maps to INSUFFICIENT_FUNDS_FOR_FEE), so BAM will see the wrong error code

@esemeniuc esemeniuc merged commit 66b7160 into master_bam Jan 9, 2026
1 of 3 checks passed
@esemeniuc esemeniuc deleted the eric/master-tweak branch January 9, 2026 19:08
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.

2 participants