Skip to content

BM-736: Targeted benchmark proving #507

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

willemolding
Copy link
Contributor

Closes #485

Copy link

linear bot commented Apr 11, 2025

@willemolding willemolding marked this pull request as draft April 11, 2025 02:20
@willemolding willemolding self-assigned this Apr 11, 2025
@willemolding willemolding marked this pull request as ready for review April 11, 2025 04:27
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Skimming this, it's unclear to me if trying to target specific provers with orders, why we would not just selectively send orders to specific addresses? Feels like it should be handled at the order stream server level? If you really want to skip preflight, you just have the prover add the benchmark program ID to skip_preflight_ids?

Also perhaps I'm misreading the impl, but this means for brokers that are not being benchmarked will try to process the order and fail because they don't have the secret prefix? This seems unideal.

Comment on lines +436 to +449
Some(v) => Helper(v).serialize(serializer),
None => serializer.serialize_none(),
}
}

pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<Vec<u8>>, D::Error>
where
D: serde::Deserializer<'de>,
{
#[derive(Deserialize)]
struct Helper(#[serde(with = "hex_serde")] Vec<u8>);

let helper = Option::deserialize(deserializer)?;
Ok(helper.map(|Helper(v)| v))
Copy link
Contributor

Choose a reason for hiding this comment

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

This may work for toml/json, just noting that this will not work for some serialization protocols. Probably won't ever matter in our case, but wondering why handling Option case implicitly. serde_with crate would remove the need for all of this.

@nategraf nategraf marked this pull request as draft April 23, 2025 22:08
@nategraf
Copy link
Member

Moved this to draft to indicate we do not intend to merge as is, after discussion on the approach here.

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.

Targeted proving tasks to specific provers for measuring capacity without trust
3 participants