-
Notifications
You must be signed in to change notification settings - Fork 6
feat: integrate UTxOW rules checks with omnibus process
#495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
sandtreader
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work - I really like the referencing and the collecting the UTXOs needed for a transaction - that will be needed for the script context in phase 2 as well.
I'm worried about the switch from tx.consumes() to tx.inputs() though - I remember digging this out at the very beginning, consumes() handles the case of a phase 2 validation failure and consumption of the collateral. If this is only being used for validation and you handle collateral later, that's fine, but if it's used for the actual UTXO processing I think this will break things.
codec/src/tx.rs
Outdated
| let tx_hash = TxHash::from(*tx.hash()); | ||
|
|
||
| for input in tx.consumes() { | ||
| for input in tx.inputs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware! consumes() covers the case where the transaction phase 2 validation fails and the collateral is consumed instead of the input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks for pointing this out 👍🏼
| TxCertificateWithPos, Withdrawal, | ||
| }; | ||
|
|
||
| /// Get VKey Witnesses needed for transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice references and explanation!
sandtreader
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!
codec/src/tx.rs
Outdated
| parsed_inputs.push(utxo); | ||
| } | ||
|
|
||
| for (index, output) in tx.outputs().iter().enumerate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be tx.produces() for the same reason, but check my logic!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is right, I only changed the name 😓
a2a8a98
Description
Integrate
UTxOWrules check intoomnibusprocess and add test cases for eachUTxOWrules (expect transaction with native script)Also make
tx_unpacker,utxos_state,block_vrf_validator,block_kes_validatorto useValidationOutcomes.Related Issue(s)
Related to #424 #473
How was this tested?
Add unit tests to
utxos_stateandtx_unpackerforUTxOWrules andBadInputsUTxOcheck.Checklist
Impact / Side effects
In
utxos_state, we validate the transactions and this will make process run a bit slower.Reviewer notes / Areas to focus
utxos_state/src/utxo_state.rs- Where we validate transactions fromUTxODeltasMessage