is_valid addition to transaction#208
Conversation
|
Yeah this was missing from the binary spec originally. We got IOHK to push a fix for this but haven't re-run the codegen to deploy the fix yet. Did you get this PR from re-running the codegen on the latest cddl? |
Oh I actually made these changes manually. |
|
One concern we have about this is that serialization-lib needs to still be able to parse transactions that happened before Alonzo (and therefore don't have this field). We're talking with Jared about the best way to represent this in the binary spec and we'll have to update serialization-lib accordingly |
What about making the bool optional and the serialize-lib can (de)serialize both tx with or without the bool. |
|
Just making it optional wouldn't work either since it's mandatory in the current alonzo spec. My guess is that we would want to it to be mandatory within the lib but maybe be permissive (mark it as true if it's not there) with it in deserialization. We'll have to wait on Jared's response to see what is the correct solution. We should know by the start of next week hopefully. |
|
From what I understood from Jared, nodes are currently accepting and fixing the old txs format for compatibility, presumably setting the flag to So I'm wondering if we need this flag here at all? Prolly critical for parsing, tho, if someone would use the library for that. |
rooooooooob
left a comment
rooooooooob
left a comment
There was a problem hiding this comment.
@alessandrokonrad Jared got back to me and it seems that we should probably always have it be set as true on our end for safety sake (not losing collateral). We do also want to support Mary era parsing so we should have permissive parsing (e.g. handling the old Transaction format which is 3 long and without is_valid) while also handling the new case. Since we could parse ones that have it set to false I think yes we should have this as a field, but we could have it default to true and only handle setting it via a setter, while having the deserializer also allow the old
transaction =
[ transaction_body
, transaction_witness_set
, auxiliary_data / null
]
format and in that case set is_valid to true.
| let witness_set = (|| -> Result<_, DeserializeError> { | ||
| Ok(TransactionWitnessSet::deserialize(raw)?) | ||
| })().map_err(|e| e.annotate("witness_set"))?; | ||
| let is_valid = (|| -> Result<_, DeserializeError> { |
There was a problem hiding this comment.
We want to be permissive here in parsing. Be careful how you do it though since if it's CBORType::Special it could be either case (null in the auxiliary_data / null) or the bool is_valid.
| pub fn new( | ||
| body: &TransactionBody, | ||
| witness_set: &TransactionWitnessSet, | ||
| is_valid: bool, |
There was a problem hiding this comment.
I think we shouldn't have this as a parameter and instead if advances users for some reason need to have it as false we can instead add in a pub fn set_is_valid(&mut self, is_valid: bool).
There was a problem hiding this comment.
Yes👍🏻, made all changes accordingly.
|
@alessandrokonrad Sorry for the hassle, but can you please update this branch? It's out of date. @vsubhuman can we get this merged after please? I approved it 3 weeks ago. |
|
@vsubhuman which release will this get put into? It has no breaking API changes whatsoever and is extremely blocking as it essentially locks out using any Alonzo functionality in the lib to be sent to the node. edit: I guess if you're using it from rust we do technically by cargo standards break by adding an error variant. |
@rooooooooob , published now as 9.1.2 here: #251 (comment) |
4 participants
is_valid bool was now introduced in transaction: https://github.com/input-output-hk/cardano-ledger-specs/blob/37322df14d44d1370e8a72b677815d64a92baa00/alonzo/test/cddl-files/alonzo.cddl#L16