jwk #1: validator txn for publishing updates #11853
Conversation
zjma
force-pushed
the
zjma/jwk_txn_execution
branch
from
029e686 to
d0957db
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11853 +/- ##
=========================================
- Coverage 71.2% 71.2% -0.1%
=========================================
Files 795 795
Lines 182802 182968 +166
=========================================
+ Hits 130264 130282 +18
- Misses 52538 52686 +148 ☔ View full report in Codecov by Sentry. |
zjma
changed the title
zjma
changed the title
|
|
||
| enum ExpectedFailure { | ||
| // Move equivalent: `errors::invalid_argument(*)` | ||
| IncorrectVersion = 0x010103, |
There was a problem hiding this comment.
what do these 0x... values mean here? does it map to some values at other places?
There was a problem hiding this comment.
yes, see aptos-move/framework/aptos-framework/sources/jwks.move
There was a problem hiding this comment.
Didn't find it. Can you share a link?
There was a problem hiding this comment.
aptos-core/aptos-move/framework/aptos-framework/sources/jwks.move
Lines 32 to 36 in 3d5bfea
types/src/validator_txn.rs
Outdated
| match self { | ||
| ValidatorTransaction::DummyTopic1(_) => Topic::DUMMY1, | ||
| ValidatorTransaction::DKGResult(_) => Topic::DKG, | ||
| ValidatorTransaction::DummyTopic2(_) => Topic::DUMMY2, |
There was a problem hiding this comment.
why dummy is used outside out test?
| match self.process_jwk_update_inner(resolver, log_context, session_id, update) { | ||
| Ok((vm_status, vm_output)) => Ok((vm_status, vm_output)), | ||
| Err(Expected(failure)) => { | ||
| // Pretend we are inside Move, and expected failures are like Move aborts. |
There was a problem hiding this comment.
why do we need to simulate move abort? normal move abort is not discard too?
There was a problem hiding this comment.
We don't need to. But I had the impression that In the future we may want to move the checks into move.
Discarding or not is a separate question? And for vtxn we want to discard if the checks failed?
fixed, dummy has gone |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // Pretend we are inside Move, and expected failures are like Move aborts. | ||
| Ok(( | ||
| VMStatus::MoveAbort(AbortLocation::Script, failure as u64), | ||
| VMOutput::empty_with_status(TransactionStatus::Discard(StatusCode::ABORTED)), |
There was a problem hiding this comment.
Move aborts are kept, so we want a Keep here?
There was a problem hiding this comment.
I believe what we really want is to discard (otherwise we allow malicious validators to put things in)
There was a problem hiding this comment.
I'm just not sure what's the best value for the 1st element of the returning tuple.
There was a problem hiding this comment.
Some other vmstatus variants seem to be more serious and can cause the whole block to be rejected. That's undesired behavior
There was a problem hiding this comment.
In this case we do not need VMOutput, and can return a VMStatus? For example, for prologue which runs Move code we do it like this: https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/aptos-vm/src/errors.rs#L64 which rempas the error to status and returns just that. The caller can see Err(status) and create an empty output with a Discard?
There was a problem hiding this comment.
just to be clear, if the transaction is valid, we want to make changes; otherwise (expected failures), we want to discard.
Are you are suggesting change: from
fn process_jwk_update(...) -> Result<(VMStatus, VMOutput), VMStatus> {
match self.process_jwk_update_inner(...) {
//...
Err(Expected(failure)) => {
// Pretend we are inside Move, and expected failures are like Move aborts.
Ok((
VMStatus::MoveAbort(AbortLocation::Script, failure as u64),
VMOutput::empty_with_status(TransactionStatus::Discard(StatusCode::ABORTED)),
))
},
//...
}
}
to
fn process_jwk_update(...) -> Result<(VMStatus, VMOutput), VMStatus> {
match self.process_jwk_update_inner(...) {
//...
Err(Expected(failure)) => {
// Pretend we are inside Move, and expected failures are like Move aborts.
Err(VMStatus::SOMETHING)
},
//...
}
}
?
There was a problem hiding this comment.
we can't return errors here, that will kill the block execution. we can keep it as is but it's a bit ugly
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Description
Test Plan