Add rejection reason storage to finalize_store#3101
Add rejection reason storage to finalize_store#3101
Conversation
Co-authored-by: vicsn <24724627+vicsn@users.noreply.github.com>
Co-authored-by: vicsn <24724627+vicsn@users.noreply.github.com>
Co-authored-by: vicsn <24724627+vicsn@users.noreply.github.com>
ljedrz
left a comment
There was a problem hiding this comment.
I would recommend against a String rejection reason; the storage might be subject to compression, but I'm not confident a string like Program {id} has already been deployed in this block would compress reliably, despite both the ID already present in a key and a constant error context string.
If we want the tx rejection reasons to be both auditable and lightweight, we should isolate them into a dedicated enum.
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
|
@vicsn do we want to be specific about the nature of the finalization errors? |
Yes, you can get inspiration from these errors: https://github.com/ProvableHQ/snarkVM/pull/3081/files#diff-7d838a571bdcbb8174c371914f0794cc9881157d10a2115b374cbc6b0021618eR117 Which could in turn enable pretty-prenting something like: |
|
Blocked by #3122. |
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
| #[inline] | ||
| fn write_le<W: Write>(&self, mut writer: W) -> IoResult<()> { | ||
| match self { | ||
| Self::AlreadyDeployedInTheBlock => { |
There was a problem hiding this comment.
Can we have a more symmetric FailedToDeploy(reason)
| let rejected = Rejected::new_execution(*execution.clone()); | ||
| let rejected = Rejected::new_execution( | ||
| *execution.clone(), | ||
| RejectionReason::FailedToFinalize, |
There was a problem hiding this comment.
Perhaps this is still on your roadmap, but the primary usecase I had in mind was to fill this enum with the logged error present in this scope, so we learn why an Execution fails.
Now that you did all the work on StackInit errors already, I guess we can keep them, but where possible feel free to leave stuff as future work.
|
|
||
| /// Errors that may occur during program upgrade. | ||
| #[derive(Clone, Debug, PartialEq, Eq, Error, Serialize, Deserialize)] | ||
| pub enum ProgramUpgradeError { |
There was a problem hiding this comment.
I wonder how maintainable all of these enums are.
Perhaps this was what you were previously warning me about.
If we want to remove or change any of these errors, the serialization will change.
And while we may be able to prevent external indexers from relying on the format, a node's own database can end up misinterpreting it's own data.
If we want to stick with this approach, I guess we need versioning in some places? Perhaps a unit test can warn us if we change the wrong things?
And if that's not feasible, perhaps we just stick to untyped strings...
There was a problem hiding this comment.
Update: for deployments, we should just have a single high level enum DeploymentError, we don't have to go more granular. For executions, we should try and include the instruction line which failed. For other types of finalization errors, we should discuss whether it's worth to make enums.
I'd still be keen to know how to incorporate versioning in the enum serialization or types.
There was a problem hiding this comment.
We should annotate these with #[non_exhaustive], so adding a new error does not break the API.
Rejected transactions now store their rejection reasons in a new
rejection_reason_mapwithin the finalize storage, enabling API queries for why transactions were rejected.Changes
RejectionReasonMapDataMap to FinalizeStorage trait with implementations for both RocksDB and Memory backendsinsert_rejection_reason(transaction_id, reason)get_rejection_reason(transaction_id)contains_rejection_reason(transaction_id)VM::atomic_speculate_innerto store rejection reasons when creating rejected deployment or execution transactionsExample Usage
Rejection reasons include descriptive error context such as "Failed to finalize deployment: {error}" or "Program {id} has already been deployed in this block".
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.