fix: defer precompile claims in LocalTransactionProver - #3851
fix: defer precompile claims in LocalTransactionProver#3851PhilippGackstatter wants to merge 3 commits into
LocalTransactionProver#3851Conversation
PhilippGackstatter
added
the
pr-from-maintainers
78e84d1 to
c2b5508
Compare
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
Looks good overall, raised a test gap, happy to otherwise stamp once we discuss it a bit.
| /// | ||
| /// Transactions whose precompile claims are still outstanding are accepted: verification checks | ||
| /// that their deferred witness matches their VM proof, and the batch prover settles the claims | ||
| /// of all transactions in the batch with a single precompile proof. |
There was a problem hiding this comment.
nit: LocalBatchProver does not yet merge or prove transaction precompile wires, and the batch kernel still ignores transactions. Fine if we intend to merge before this goes in a release, otherwise we could describe this as planned settlement here and in the changelog.
There was a problem hiding this comment.
I'm working on a follow-up PR that I intend to stack on this one that implements merging the precompiles during batch building, so I would skip describing the planned settlement here.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I mostly focused on non-test code and left a couple of small comments inline.
| ### Changes | ||
|
|
||
| - Added type signatures where missing throughout the protocol and standards Miden Assembly libraries | ||
| - `LocalTransactionProver` now leaves precompile claims deferred for the batch prover to settle, instead of proving them per transaction ([#3851](https://github.com/0xMiden/protocol/pull/3851)). |
There was a problem hiding this comment.
nit: I'd put this into the "fixes" section.
| // Reconstruct structurally without verifying the transactions' proofs. | ||
| ProposedBatch::new_batch_inner( | ||
| transactions, | ||
| block_header, | ||
| partial_blockchain, | ||
| unauthenticated_note_proofs, | ||
| ) |
There was a problem hiding this comment.
Not from this PR, but I wonder if this is a good idea to have the deserialization path skip transaction proof verification as this technically allows constructing proposed batches from invalid transactions.
On the other hand, this serialization code is something we should be removing shortly anyway (in favor of the protobuf serialization) - so, maybe the important part is to check the protobuf serialization to make sure we uphold the invariants there.
4 participants
Changes
LocalTransactionProver::provecalledmiden_prover::prove_sync, which proves the VM execution and settles the transaction's precompile claims.TransactionVerifier::verifyrejects settled precompile work by design, so a locally proven ECDSA-authenticated transaction could never pass transaction verification, and the wasted precompile STARK dominated local prove time. Precompile claims now stay deferred: one proof per batch will cover the claims of all its transactions (in a follow-up).miden-tx: the prover runsFastProcessor::execute_for_proving_syncand thenProver::prove, the deferred counterpart ofprove_fullthatprove_syncuses.prove_partial_syncno longer exists in miden-prover 0.32.1; that free-function family collapsed ontoProver.miden-protocol:ProposedBatchno longer rejects transactions whose precompile claims are outstanding, innewor on deserialization. As discussed on the issue, settling those claims belongs to the batch prover, which merges the witnesses of all transactions in the batch into a single precompile proof.Closes #3835.
Performance
Measured locally with
make bench-txagainstorigin/next: ECDSA execute-and-prove drops from 1.24 s to 561 ms for the single-P2ID scenario and from 1.23 s to 560 ms for the two-P2ID scenario. Falcon is unchanged for single-P2ID and 0.6% slower for two-P2ID, which is the cost of losingprove_sync's overlapped execution/trace-build path:Prover::prove_full_traceis private upstream and has no partial counterpart. That overlap should be fine not to expose as it is planned to avoid re-execution during proving (issue to be created).Until batch-level settlement lands, a batch containing an ECDSA transaction is accepted by
ProposedBatchbut its precompile claims are never proven. The batch prover still rejects precompile work outright, and the batch kernel does not yet verify a merged precompile proof.