Skip to content

Conversation

@dan-da
Copy link
Collaborator

@dan-da dan-da commented May 24, 2025

note to reviewers: the primary changes are in files: vm_proving_capability.rs, cli_args.rs, transaction_proof_builder.rs, prover_job.rs, and triton_vm_job_queue/mod.rs. Other changes are mainly dictated by the modifications in these files.


edit: reviewers, please review and double check the two logging modes for vm_state (proof inputs). These are described fully in src/util_types/log_vm_state.rs. They are used in prover_job.rs (logs all proofs) and transaction_proof_builder.rs (logs only single-proofs without primitive-witness claims). In particular, it is important that we not leak secrets in the NoWitness mode that could result in loss-of-funds if the resulting json file is shared. Files of both types can be generated via:

mkdir /tmp/vm_state
mkdir /tmp/vm_state_no_witness

NEPTUNE_VM_STATE_DIR=/tmp/vm_state cargo test
NEPTUNE_VM_STATE_NO_WITNESS_DIR=/tmp/vm_state_no_witness cargo test

(note that each mode can be independently enabled)

The /tmp/vm_state directory will contain many files and is around 5.8Gb for me, while the other dir is much smaller.


also, in testing I found that some ProofCollection proofs were requiring log2-padded-height of 15, so I changed TransactionProofType::ProofCollection variant to specify 15 instead of 11.

commit text follows and describes the changes well.


refactor: make proving-capability correct for proving

closes #576.

TxProvingCapability was being used by ProofBuilder and by ProvingJob but its
variants were logically incorrect for these usages because these types only care
about NeptuneProof, not about TransactionProof variants, which
TxProvingCapability mirrored.

This commit fixes that by renaming enum TxProvingCapability to struct
VmProvingCapability which is a newtype for log2(padded_height).

Some impl are added to TransactionProofType that make it easy to convert
to a VmProvingCapability and vice-versa.

capability detection is moved from cli_args::Args into
VmProvingCapability::auto_detect(). Proof-collection is now log2-padded-height
of 15, rather than 11. This is because some tests were failing and required 15
so it is experimentally determined.

changes --tx-proving-capability to --vm-proving-capability .

removes --max_log2_padded_height_for_proofs as --vm-proving-capability has the
same meaning.

removes fields proof_type and max_log2_padded_height from ProverJobSettings, as
they are no longer relevant. proof_type makes no sense there and
max_log2_padded_height is present inside vm_proving_capability.

adds method transaction_proof_type() to TransactionProofBuilder, where it
actually does make sense.

updates tests, adds docs and examples.

Squashed:

  • removed proof_type and max_log2_padded_height from ProverJobSettings.
    Added transaction_proof_type to TransactionProofBuilder.
  • re-implemented TxProvingCapability as struct, not enum.
  • rename TxProvingCapability VmProvingCapability
  • default tx_proof_type to best proof capable of. other misc
  • move capability auto-detection from Args into VmProvingCapability
  • ProofCollection now 15 instead of 11

feat: add env var VM_JOB_QUEUE_SHARED for tests

This adds a kind of turbo-mode for running unit tests.

Normally the job-queue is a shared singleton instance.

When running unit tests, a shared instance has these characteristics:

  1. tests must wait for each other's jobs to complete. CPU usage tends to
    be low for much of the testing duration.
  2. it is possible to run all tests in parallel and generate proofs when
    proofs do not exist in local cache or on proof-server.
  3. total time for running all tests increases substantially compared to
    scenario where each test using its own job-queue instance.

A non-shared instance has these characteristics:

  1. tests run independently and use up all CPU cores.
  2. it is not possible to run all tests in parallel and generate proofs
    as it would exhaust device's resources, especially RAM. This mode
    only works well when proofs are already cached. A workaround is to
    run with --test-threads 1 to generate proofs.
  3. total time for running all tests decrease substantially (assuming
    proofs are cached) vs the shared-instance scenario.

When running unit tests, shared mode is the default. The mode can now
be selected at runtime via:

VM_JOB_QUEUE_SHARED=false cargo test <args>

VM_JOB_QUEUE_SHARED=true cargo test <args>

feat: env LOG2_PADDED_HEIGHT_METHOD for proving capability

A previous commit made calculation of log2(padded-height) about 4x slower for
ProofBuilder and ProverJob because it used a different calculation method. This
commit returns to the baseline method and speed, but enables runtime selection
via environment variable.

By default the log2(padded-height) is calculated using VmState::run().

A more accurate but slower way is to use VM::trace_execution_of_state().
This is typically about 4x slower.

And the fastest method is to skip running the program entirely. But
that option is only available when running unit tests.

These methods can be selected at runtime:

LOG2_PADDED_HEIGHT_METHOD=trace neptune-core <args>
LOG2_PADDED_HEIGHT_METHOD=run neptune-core <args>

LOG2_PADDED_HEIGHT_METHOD=skip cargo test <args>

chore: improvements to VmProvingCapability

addresses #603 review comments.  In particular:

+ suggest padded-height value in --vm-proving-capability help text for each
  TransactionProofType variant.
+ add dep on const_format crate, for formatting const strings
+ handle error for VMState::run() instead of unwrap()
+ writes vm state to disk if NEPTUNE_WRITE_VM_STATE_DIR is set
+ warn instead of panic if total memory is reported as zero.
+ adds back some asserts
+ make some enums non_exhaustive
+ improve some doc-comments

Also:
+ renames LOG2_PADDED_HEIGHT_METHOD to NEPTUNE_LOG2_PADDED_HEIGHT_METHOD
+ removes unwrap() when calling spawn_blocking()

chore: make vm_state logging more flexible

Logging proof inputs can be a security risk if the claims contain
primitive-witness data (secrets).

This commit creates two logging modes: Proof and NoWitness.

Proof mode logs all proofs processed by the ProverJob.
It is enabled by setting NEPTUNE_VM_STATE_DIR=<dir>

NoWitness mode logs only SingleProof that are produced from non
primitive-witness claims.  These are logged from TransactionProofBuilder.
It is enabled by setting NEPTUNE_VM_STATE_NO_WITNESS_DIR=<dir>

A new module, log_vm_state, is introduced that holds the logging code.

@dan-da dan-da force-pushed the 576_proving_capability_pr branch 4 times, most recently from 2923bce to 8f1ef00 Compare May 24, 2025 20:22
dan-da added 3 commits May 24, 2025 22:29
closes Neptune-Crypto#576

TxProvingCapability was being used by ProofBuilder and by ProvingJob but its
variants were logically incorrect for these usages because these types only care
about NeptuneProof, not about TransactionProof variants, which
TxProvingCapability mirrored.

This commit fixes that by renaming enum TxProvingCapability to struct
VmProvingCapability which is a newtype for log2(padded_height).

Some impl<From> are added to TransactionProofType that make it easy to convert
to a VmProvingCapability and vice-versa.

capability detection is moved from cli_args::Args into
VmProvingCapability::auto_detect().  Proof-collection is now log2-padded-height
of 15, rather than 11.  This is because some tests were failing and required 15
so it is experimentally determined.

changes --tx-proving-capability to --vm-proving-capability <int>.

removes --max_log2_padded_height_for_proofs as --vm-proving-capability has the
same meaning.

removes fields proof_type and max_log2_padded_height from ProverJobSettings, as
they are no longer relevant.  proof_type makes no sense there and
max_log2_padded_height is present inside vm_proving_capability.

adds method transaction_proof_type() to TransactionProofBuilder, where it
actually does make sense.

updates tests, adds docs and examples.

Squashed:

+ removed proof_type and max_log2_padded_height from ProverJobSettings.
  Added transaction_proof_type to TransactionProofBuilder.
+ re-implemented TxProvingCapability as struct, not enum.
+ rename TxProvingCapability VmProvingCapability
+ default tx_proof_type to best proof capable of.  other misc
+ move capability auto-detection from Args into VmProvingCapability
+ ProofCollection now 15 instead of 11
This adds a kind of turbo-mode for running unit tests.

Normally the job-queue is a shared singleton instance.

When running unit tests, a shared instance has these characteristics:

1. tests must wait for each other's jobs to complete. CPU usage tends to
   be low for much of the testing duration.
2. it is possible to run all tests in parallel and generate proofs when
   proofs do not exist in local cache or on proof-server.
3. total time for running all tests increases substantially compared to
   scenario where each test using its own job-queue instance.

A non-shared instance has these characteristics:

1. tests run independently and use up all CPU cores.
2. it is not possible to run all tests in parallel and generate proofs
   as it would exhaust device's resources, especially RAM.  This mode
   only works well when proofs are already cached.  A workaround is to
   run with --test-threads 1 to generate proofs.
3. total time for running all tests decrease substantially (assuming
   proofs are cached) vs the shared-instance scenario.

When running unit tests, shared mode is the default. The mode can now
be selected at runtime via:

```
VM_JOB_QUEUE_SHARED=false cargo test <args>

VM_JOB_QUEUE_SHARED=true cargo test <args>
```
A previous commit made calculation of log2(padded-height) about 4x slower for
ProofBuilder and ProverJob because it used a different calculation method.  This
commit returns to the baseline method and speed, but enables runtime selection
via environment variable.

By default the log2(padded-height) is calculated using VmState::run().

A more accurate but slower way is to use VM::trace_execution_of_state().
This is typically about 4x slower.

And the fastest method is to skip running the program entirely.  But
that option is only available when running unit tests.

These methods can be selected at runtime:

```
LOG2_PADDED_HEIGHT_METHOD=trace neptune-core <args>
LOG2_PADDED_HEIGHT_METHOD=run neptune-core <args>

LOG2_PADDED_HEIGHT_METHOD=skip cargo test <args>
```
@dan-da dan-da force-pushed the 576_proving_capability_pr branch from 8f1ef00 to 52f680c Compare May 24, 2025 22:36
Copy link
Member

@jan-ferdinand jan-ferdinand left a comment

Choose a reason for hiding this comment

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

I've focused my attention on the files you have pointed out. Seems like a solid change in general. I've left a few questions, comments, and nits in line.

dan-da added 2 commits May 29, 2025 03:50
addresses Neptune-Crypto#603 review comments.  In particular:

+ suggest padded-height value in --vm-proving-capability help text for each
  TransactionProofType variant.
+ add dep on const_format crate, for formatting const strings
+ handle error for VMState::run() instead of unwrap()
+ writes vm state to disk if NEPTUNE_WRITE_VM_STATE_DIR is set
+ warn instead of panic if total memory is reported as zero.
+ adds back some asserts
+ make some enums non_exhaustive
+ improve some doc-comments

Also:
+ renames LOG2_PADDED_HEIGHT_METHOD to NEPTUNE_LOG2_PADDED_HEIGHT_METHOD
+ removes unwrap() when calling spawn_blocking()
Logging proof inputs can be a security risk if the claims contain
primitive-witness data (secrets).

This commit creates two logging modes: Proof and NoWitness.

Proof mode logs all proofs processed by the ProverJob.
It is enabled by setting NEPTUNE_VM_STATE_DIR=<dir>

NoWitness mode logs only SingleProof that are produced from non
primitive-witness claims.  These are logged from TransactionProofBuilder.
It is enabled by setting NEPTUNE_VM_STATE_NO_WITNESS_DIR=<dir>

A new module, log_vm_state, is introduced that holds the logging code.
Copy link
Member

@jan-ferdinand jan-ferdinand left a comment

Choose a reason for hiding this comment

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

Thanks for adding vm-state logging!

I've left mostly nits inline, and one somewhat bigger question. In particular, I'm surprised that the logging seems to pertain only to the SingleProof program.

In case you want to land the tx-proving-capability PR without spending time discussing logging things, I think the logging could be a separate PR. If you don't feel like such a discussion is holding things back, I'm happy to progress as is.

Comment on lines 50 to 51
Self::Proof => "vm_state.claim",
Self::NoWitness => "vm_state.no_witness_claim",
Copy link
Member

Choose a reason for hiding this comment

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

I like the no_witness_claim to indicate also in the filename that the claim does not contain a primitive witness. I'm less sure about vm_state.claim: the state file does not only contain the claim but also the non-determinism and the proof (neither of which is part of the claim). Maybe just drop the .claim suffix of the prefix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the state file does not only contain the claim but also the non-determinism and the proof

I'm confused about how the file can contain the proof.

My understanding has been that VMState::run() just runs the program and does not generate a proof. As such it can run in seconds while the real proof generation with triton_vm::prove() takes sometimes minutes for the same program, claim/input, nondeterminism triple.

Can you clarify about that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, I noticed that the proof caching mechanism used for neptune-core testing identifies the cached data by the hash of the claim. Here is the proof_filename() fn from program.rs.

    fn proof_filename(claim: &Claim) -> String {
        let base_name = Hash::hash(claim).to_hex();

        format!("{base_name}.proof")
    }

So I was essentially working from this precedent (which I did not author).

It does seem logical to me that hash(program, claim, nondeterminism) would be a better identifier, but from above the claim seems to have been deemed sufficient.

Copy link
Collaborator Author

@dan-da dan-da Jun 3, 2025

Choose a reason for hiding this comment

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

Self::Proof => "vm_state.claim",
Self::NoWitness => "vm_state.no_witness_claim",

I also was considering that perhaps the Self::Proof case should somehow warn not to share, eg: "vm_state.do_not_share" or "vm_state.unsafe_to_share". what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about how the file can contain the proof.

And rightfully so. I was trying to say that the VM state also contains the non-determinism and the program (not proof).

My understanding has been […]. Can you clarify about that?

What you describe here is accurate.

also, I noticed that the proof caching mechanism used for neptune-core testing identifies the cached data by the hash of the claim.

I concur that a hash of the claim is sufficient for identifying a proof.1 I think I left the comment more because I think that the filename vm_state.claim suggests that the file contains only the claim, while in truth, it's the entire VM state.

I also was considering that perhaps the Self::Proof case should somehow warn not to share

I like that idea and your suggestion. How about commenting on the share-safety in both cases?

Self::Proof => "vm_state.do_not_share",
Self::NoWitness => "vm_state.safe_to_share",

Footnotes

  1. In case you're interested in the “why”: A claim contains the hash of the program, so the program is pinned down that way. The exact non-determinism is irrelevant for the proofs validity. In general, there can be many different non-determinisms with which it's possible to generate a proof. The point is that the non-determinism doesn't have to be pinned down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

vm_state.claim suggests that the file contains only the claim, while in truth, it's the entire VM state.

The problem with abbreviating things is it can have different readings. I was intending it to mean "vm_state for claim X" where X is included in the filename.

In terms of symmetry, I like this better/best:

Self::Proof => "vm_state.unsafe_to_share",
Self::NoWitness => "vm_state.safe_to_share"

I'm hesitant to label it "safe_to_share" though. Seems like we are then making some kind of guarantee. If there's consensus amongst team members it is "safe" and we should name it that I'm ok with it, I just don't want to make the claim on my own.

Copy link
Member

Choose a reason for hiding this comment

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

Let's hear from people with more knowledge of the architecture whether these VM states are actually safe to share. 👍 (@aszepieniec, @Sword-Smith)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

20705dd changes it to:

            Self::MayContainWalletSecrets => "vm_state.unsafe_to_share",
            Self::DoesNotContainWalletSecrets => "vm_state.safe_to_share",

@dan-da
Copy link
Collaborator Author

dan-da commented Jun 3, 2025

In case you want to land the tx-proving-capability PR without spending time discussing logging things, I think the logging could be a separate PR. If you don't feel like such a discussion is holding things back, I'm happy to progress as is.

I think it makes sense to continue in this PR since this PR removed the original logging (which you flagged in review). So if we were to merge the PR without it, then we have a kind of regression in master until a subsequent PR is merged. Also too, I think it will not take much effort to address your latest comments.

address Neptune-Crypto#603 review comments:

fixes logging of proof program.  previously it was always logging
SingleProof.program() regardless of the Proof type.

rename LogProofInputsType variants to:
 - MayContainWalletSecrets
 - DoesNotContainWalletSecrets

rename corresponding env vars to:
 - NEPTUNE_VM_STATE_WITH_SECRETS_DIR
 - NEPTUNE_VM_STATE_NO_SECRETS_DIR

rename correspoing file prefixes to:
 - vm_state.unsafe_to_share
 - vm_state.safe_to_share
@dan-da dan-da requested a review from jan-ferdinand June 9, 2025 06:30
@dan-da
Copy link
Collaborator Author

dan-da commented Jun 9, 2025

I've addressed the remaining review comments. I think it should be about ready to merge.

@dan-da
Copy link
Collaborator Author

dan-da commented Jun 10, 2025

ok, we have one approving review. @aszepieniec or @Sword-Smith, can one of you please review and merge if you agree. thanks! (In particular, @aszepieniec you might want to check it since this PR came out of our lengthy discussions.)

@aszepieniec
Copy link
Contributor

ACK. Will do.

Copy link
Contributor

@aszepieniec aszepieniec left a comment

Choose a reason for hiding this comment

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

Conceptually, I approve of these changes. However, I would like to push back against the improper use (in my eyes) of automatic conversion traits (From/into specifically), which I think are confusing and error-prone. I left more arguments and suggestions in specific in-line comments. In summary:

  • TransactionProofType --> VmProvingCapability. This can and should be From because it's quite obvious what's going on and quite simple too. So no changes requested for this one.
  • VmProvingCapability --> TransactionProofType. This mapping does have to be computed in some locations and I think you chose the right concrete map. However, the logic is non-trivial. Please use a dedicated function with a descriptive name.
  • {TransactionProofType, VmProvingCapability} --> {u8, u32} These maps are confusing and they only make sense if you rely on the contextual knowledge that in the end the thing being compared is $\log_2$-padded-heights. This reliance raises the bar for newcomers who do not have that context. These maps resist change because they effectively pin down internal implementation details. And lastly, I do not think they are necessary. Please drop them.

/// assert!(capability.can_prove(TransactionProofType::SingleProof).is_err());
///
/// let single_proof_capability: VmProvingCapability = TransactionProofType::SingleProof.into();
/// assert!(single_proof_capability.can_prove(TransactionProofType::SingleProof).is_ok());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// assert!(single_proof_capability.can_prove(TransactionProofType::SingleProof).is_ok());
/// assert!(single_proof_capability.can_prove(TransactionProofType::SingleProof).is_ok());
/// ```

///
/// let single_proof_capability: VmProvingCapability = TransactionProofType::SingleProof.into();
/// assert!(single_proof_capability.can_prove(TransactionProofType::SingleProof).is_ok());
pub fn can_prove(&self, other: impl Into<u32>) -> Result<(), VmProvingCapabilityError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function signature is confusing. Syntactically, the type of other is, if I read correctly, anything that can be cast via .into() into a u32, which includes i8, u16, i16, u32. But semantically, I am not sure if any of those options make sense. And then the docstring suggests that you can pass TransactionProofType variants, which is unexpected because it is not obvious how they map to u32s -- or more pertinently, to whatever the benchmark for comparing proving task complexity is. Lastly, why would u32 even be the right primitive type to capture that benchmark? I am reading this with the context that VmProvingCapability is a wrapper around u8 which certainly suffices to capture all $\log_2$-padded-heights that we are ever going to need.

I would suggest:

  • Drop the genericity in favor of explicit function names, such as can_prove_log2_padded_height(log2_padded_height) and can_prove_transaction(transaction_proof_type).
  • Drop the Into or From conversions from TransactionProofType to u32s.

Comment on lines +31 to +47
impl From<u8> for VmProvingCapability {
fn from(log2_padded_height: u8) -> Self {
Self { log2_padded_height }
}
}

impl From<VmProvingCapability> for u8 {
fn from(capability: VmProvingCapability) -> Self {
capability.log2_padded_height
}
}

impl From<VmProvingCapability> for u32 {
fn from(capability: VmProvingCapability) -> Self {
capability.log2_padded_height.into()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like these From implementations. There is no natural mapping from u8 to VM proving capability or vice versa. The fact that VmProvingCapability is a wrapper around u8 is an implementation detail which these From fail to make abstraction of. They only make sense if you already know that VmProvingCapability is a wrapper around u8 (thus raising the bar for newcomers), and even then only as long as that is true. In the future we might expand VmProvingCapability in ways that do not change the semantic captured by the struct, which is the information necessary to determine whether you can produce a particular proof. For instance, we might add available parallelism or available AVX instructions or GPU acceleration, etc. In such an event the From conversions certainly make no sense, but will leave the person applying the refactor scratching their head about what to replace them with.

I favor more explicit conversion functions:

  • from_log2_padded_height
  • log2_padded_height (accessor).

Comment on lines +223 to +249
pub(crate) fn auto_detect() -> Self {
const SINGLE_PROOF_CORE_REQ: usize = 19;
// see https://github.com/Neptune-Crypto/neptune-core/issues/426
const SINGLE_PROOF_MEMORY_USAGE: u64 = (1u64 << 30) * 120;

const PROOF_COLLECTION_CORE_REQ: usize = 2;
const PROOF_COLLECTION_MEMORY_USAGE: u64 = (1u64 << 30) * 16;

let s = System::new_all();
let total_memory = s.total_memory();
if total_memory.is_zero() {
tracing::warn!("Total memory reported illegal value of 0");
}

let physical_core_count = s.physical_core_count().unwrap_or(1);

if total_memory > SINGLE_PROOF_MEMORY_USAGE && physical_core_count > SINGLE_PROOF_CORE_REQ {
TransactionProofType::SingleProof.into()
} else if total_memory > PROOF_COLLECTION_MEMORY_USAGE
&& physical_core_count > PROOF_COLLECTION_CORE_REQ
{
TransactionProofType::ProofCollection.into()
} else {
TransactionProofType::PrimitiveWitness.into()
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic in here seems rather roundabout to me. We have a number capturing the total available RAM. We compare it against memory required for {ProofCollection, SingleProof}, and map the correct one of these into a $\log_2$-padded-height ... which represents the biggest table that, accounting for arithmetic, can fit in RAM.

Why not go straight from RAM to RAM? Or more specifically, why not go straight from total RAM available to max $\log_2$-padded-height and bypass {ProofCollection, SingleProof}?

Comment on lines +259 to +260
let transaction_proof_type = transaction_proof_type
.unwrap_or_else(|| proof_job_options.job_settings.vm_proving_capability.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

This .into() hides a lot of logic. If I understand correctly, it is mapping a VmProvingCapability to a TransactionProofType by selecting the TransactionProofType whose corresponding VmProvingCapability is the largest subject to being smaller than the given vm_proving_capability. A mapping as complex and non-trivial as that warrants an explicit, descriptive name. For instance: most_complex_transaction_proof_possible.

In contrast, .into() should be reserved for cases where target type and concrete mapping are so obvious that the extra explicit indication of a dedicated function can be omitted.

Comment on lines +72 to +82
impl From<TransactionProofType> for u8 {
fn from(proof_type: TransactionProofType) -> Self {
proof_type.log2_padded_height()
}
}

impl From<TransactionProofType> for u32 {
fn from(proof_type: TransactionProofType) -> Self {
proof_type.log2_padded_height().into()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Mapping TransactionProofTypes to u8s or u32s seems like it is exposing the implementation detail that "proving capability" is represented as a $\log_2$-padded-height. These conversion methods only make sense if you take that context into account. To readers unfamiliar with that context, they are confusing.

I would suggest dropping these From-implementations and relying instead on From<TransactionProofType> for VmProvingCapability, which I have no objection to.

/// and logging padded-height values in the ProverJob.
///
/// They might need to be adjusted in the future.
pub(crate) const fn log2_padded_height(&self) -> u8 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to change the function name to reflect the fact that it returns an estimate or heuristic? Right now it reads like an accessor returning either a field verbatim or a straightforward function of the encapsulated data.

(The feature that disqualifies this function from being straightforward is that its accuracy is up in the air.)

Comment on lines 646 to 654
new_block: &Block,
predecessor_block: &Block,
tx_proving_capability: TxProvingCapability,
vm_proving_capability: impl Into<VmProvingCapability>,
composing: bool,
) -> (Vec<MempoolEvent>, Vec<UpdateMutatorSetDataJob>) {
let vm_proving_capability = vm_proving_capability.into();

// If the mempool is empty, there is nothing to do.
if self.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing anything that can be cast into a VmProvingCapability as opposed to a VmProvingCapability directly, is more permissive and also more error-prone. (Why) do we need the permissiveness?

let alice_wallet = WalletEntropy::devnet_wallet();
let alice_key = alice_wallet.nth_generation_spending_key_for_tests(0);
let proving_capability = TxProvingCapability::SingleProof;
let proving_capability = TransactionProofType::SingleProof;
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable is called proving_capability but its type is TransactionProofType. Discrepant.

Please modify the variable name to reflect the data or move the .into() up so that the data matches with the variable name.

Comment on lines 2006 to 2008
async fn tx_with_proof_type(
proof_type: TxProvingCapability,
proof_type: impl Into<TransactionProofType>,
network: Network,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of this genericity? If none, drop.

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.

Rename and refactor TxProvingCapability to use correct abstraction

3 participants