Conversation
ljedrz
left a comment
There was a problem hiding this comment.
The general approach looks reasonable and maintainable to me.
Storage/serialization-wise, we need to ensure that the error values are constructed in a way that ensures that the database compression does its job well, which is even more important than keeping them short. For instance, a value like xxxxxxxxxxxxVARyyyyyyyyyy is strictly worse than xxxxxxxxxxxxyyyyyyyyyyVAR (where xs and ys are constant, and VAR varies) for dictionary-based compression that's used by rocksDB+LZ4.
We may also want to create some new test cases ensuring that the constant parts of the error strings do not change (I saw some new test cases, but I think they were concentrated on ensuring that the right errors are returned, as opposed to their string values).
@ljedrz I've refactored |
| /// The transaction was rejected due to a failed finalize command. (program ID, resource, index, command). | ||
| /// Note: We do not log the actual error message from the finalize command, as it may contain | ||
| /// sensitive information or lead to DOS vectors by storing string representations of large structs. | ||
| Finalize(ProgramID<N>, Identifier<N>, usize, Command<N>), |
There was a problem hiding this comment.
An erroring assert.eq command can be up to 2KB.
It's not that large, but it's worth considering removing Command<N> from the enum variant.
The other fields provide enough information for tools to index and find the right command.
Some benefits include smaller enums and a simpler implementation for from_indexed_finalize_error.
One added note, we should probably also track the program edition.
There was a problem hiding this comment.
Is the 2KB maximum cause for concern? It is a DOS vector, but not sure if it's worth saving on the abstraction of requiring state and doing the query.
There was a problem hiding this comment.
A bandaid solution for the future: we can consider disabling the serialization if the bloat gets out of hand.
Though it would be best to have a proper decision now.
There was a problem hiding this comment.
Personally, I'd lean towards removing Command<N>.
Dapps and client code can easily cache programs and do the lookups on their own.
That said, it's probably worth getting the opinions of those who'd use this feature (partners and the Explorer/SDK team).
There was a problem hiding this comment.
Pull request overview
This PR introduces a structured RejectedReason that records why a transaction was rejected (with optional locator/index/command context), and gates inclusion on ConsensusVersion::V14 to preserve backwards compatibility.
Changes:
- Adds
RejectedReason+ serialization (bytes + human-readable JSON) and wires it intoRejected/ConfirmedTransaction. - Threads indexed finalize error context (
IndexedFinalizeError) through finalize paths and converts it intoRejectedReason(V14+ only). - Adds/updates tests to validate rejected-reason behavior pre-/post-V14.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| synthesizer/src/vm/tests/test_v14.rs | Adds V14-focused tests asserting rejected reasons are absent pre-V14 and present post-V14 for deploy/execute cases. |
| synthesizer/src/vm/mod.rs | Updates speculation call path (coinbase_reward param) and imports for rejected-reason support. |
| synthesizer/src/vm/finalize.rs | Attaches rejected reasons during speculation (V14+), introduces indexed error propagation, and enhances execution preparation errors. |
| synthesizer/program/src/logic/command/mod.rs | Changes finalize command error type to FinalizeError to support structured finalize error propagation. |
| synthesizer/process/src/lib.rs | Adds helper to fetch latest program edition for error context. |
| synthesizer/process/src/finalize.rs | Propagates IndexedFinalizeError through finalize paths and adds typed atomic batch scopes for structured errors. |
| synthesizer/error/src/process.rs | Introduces IndexedFinalizeError, IntoIndexedFinalize, and indexed_finalize_bail! for location-aware finalize errors. |
| synthesizer/error/Cargo.toml | Adds snarkvm-console-program dependency for program ID / identifier types in errors. |
| ledger/test-helpers/src/lib.rs | Updates rejected tx helpers for the new Rejected constructors (optional reason). |
| ledger/store/src/helpers/mod.rs | Extends atomic_batch_scope! with a typed error variant to preserve structured errors. |
| ledger/src/tests.rs | Updates expected rejected execution construction to include rejected_reason: None. |
| ledger/src/check_next_block.rs | Enforces “no rejected reasons pre-V14” during block checks. |
| ledger/block/src/transactions/serialize.rs | Updates transaction test helpers usage to account for rejected-reason presence flags. |
| ledger/block/src/transactions/rejected_reason/* | Adds new RejectedReason type plus bytes/string/serde implementations and tests. |
| ledger/block/src/transactions/rejected/* | Extends Rejected to carry Option<RejectedReason> and updates (de)serialization for backward compatibility. |
| ledger/block/src/transactions/mod.rs | Exposes rejected_reason module and updates imports accordingly. |
| ledger/block/src/transactions/confirmed/mod.rs | Adds ConfirmedTransaction::rejected_reason() accessor and updates test helpers for rejected reasons. |
| ledger/block/Cargo.toml | Adds dependency on snarkvm-synthesizer-error for IndexedFinalizeError. |
| Cargo.lock | Locks new workspace dependency edges. |
| // Execute `failing_program.aleo/fail_here` and `call_failing_program/call_fail_here`. | ||
| // The transaction is rejected in finalize, but before V14 no rejected reason is attached. | ||
| let inputs = [Value::from_str("true").unwrap()].into_iter(); | ||
| let fail_transaction = | ||
| vm.execute(&private_key, ("failing_program.aleo", "fail"), inputs.clone(), None, 0, None, rng).unwrap(); | ||
| let call_fail_transaction = vm | ||
| .execute(&private_key, ("call_failing_program.aleo", "call_fail"), inputs.clone(), None, 0, None, rng) |
There was a problem hiding this comment.
The comment references fail_here / call_fail_here, but the code executes failing_program.aleo/fail and call_failing_program.aleo/call_fail. Please update the comment to avoid confusion when debugging test failures.
| use snarkvm_ledger_committee::{MAX_DELEGATORS, MIN_DELEGATOR_STAKE, MIN_VALIDATOR_SELF_STAKE}; | ||
| #[cfg(feature = "history-staking-rewards")] | ||
| use snarkvm_ledger_store::helpers::Map; | ||
| use snarkvm_synthesizer_error::{FinalizeError, IndexedFinalizeError, IntoIndexedFinalize, indexed_finalize_bail}; |
There was a problem hiding this comment.
FinalizeError is imported but never used in this module. This will trigger an unused-import warning (and can fail CI if warnings are denied). Remove the import or use it directly if needed.
| use snarkvm_synthesizer_error::{FinalizeError, IndexedFinalizeError, IntoIndexedFinalize, indexed_finalize_bail}; | |
| use snarkvm_synthesizer_error::{IndexedFinalizeError, IntoIndexedFinalize, indexed_finalize_bail}; |
| mod tests; | ||
|
|
||
| use crate::{Restrictions, Stack, cast_mut_ref, cast_ref, convert, process}; | ||
| use crate::{Command, Restrictions, Stack, cast_mut_ref, cast_ref, convert, process}; |
There was a problem hiding this comment.
Command is added to the use crate::{...} list but does not appear to be used anywhere in this module. Please remove it to avoid unused-import warnings.
| use crate::{Command, Restrictions, Stack, cast_mut_ref, cast_ref, convert, process}; | |
| use crate::{Restrictions, Stack, cast_mut_ref, cast_ref, convert, process}; |
| Ratifications, | ||
| Ratify, | ||
| Rejected, | ||
| RejectedReason, |
There was a problem hiding this comment.
RejectedReason is imported from snarkvm_ledger_block but does not appear to be used in this file. Please remove it to avoid unused-import warnings.
| RejectedReason, |
| Identifier, | ||
| ProgramID, |
There was a problem hiding this comment.
These console::program imports (Identifier, ProgramID) appear unused in this module. Consider removing them to avoid unused-import warnings.
| Identifier, | |
| ProgramID, |
| use snarkvm_synthesizer_error::IndexedFinalizeError; | ||
| use snarkvm_synthesizer_program::{Command, FinalizeOperation}; |
There was a problem hiding this comment.
These imports (IndexedFinalizeError, Command) appear unused in this module. Consider removing them to avoid unused-import warnings.
| use snarkvm_synthesizer_error::IndexedFinalizeError; | |
| use snarkvm_synthesizer_program::{Command, FinalizeOperation}; | |
| use snarkvm_synthesizer_program::FinalizeOperation; |
| assert_eq!(block.transactions().num_rejected(), 1); | ||
| assert_eq!(block.aborted_transaction_ids().len(), 0); | ||
| let rejected_tx = block.transactions().iter().find(|tx| tx.is_rejected()).unwrap(); | ||
| // The locator points to the constructor of `failing_constructor.aleo`. |
There was a problem hiding this comment.
This comment says the locator points to the constructor of failing_constructor.aleo, but the assertions below expect credits.aleo and bond_validator. Please update the comment to match the actual expectation.
| // The locator points to the constructor of `failing_constructor.aleo`. | |
| // The locator points to the `bond_validator` transition in `credits.aleo`. |
Motivation
This PR introduces
RejectedReasons that stores why a transaction was rejected in theRejectedobject. Depending on the reason, there may be a locator, index, and command stored as well. This will help with developer debugging as they can better trace where their errors are coming from.The indexing is done on the Aleo Instruction level, so we may eventually want a tool that points to where in Leo code the error originates from.
Currently the 3 types are:
Alternative to #3101.
Test Plan
Tests have been added to ensure that the RejectedReasons are properly constructed and only occur after the desired consensus version.
Backwards compatibility
This functionality is guarded with ConsensusVersion::V14, where rejected reasons are only stored after V14, and are invalid before then.
TODO