ElementsProject / rust-elements

Rust support for Elements transaction/block deserialization
Creative Commons Zero v1.0 Universal
52 stars 33 forks source link

Pset roundtrip fix #184

Closed RCasatta closed 1 year ago

RCasatta commented 1 year ago

@LeoComandini noticed some data from the PSET disappeared when going to_str/from_str

This draft contains a failing test for the issue.

The issue is due to the reuse in the pset serialization of the TxOut::Encodable logic which is skipping the serialization of witness, which is handled separately in Transaction::Encodable but this is not done in the pset logic.

To fix I thik I am going to create a wrapper type TxOutAll which wraps TxOut and has From/To methods and in which Encodable serialize also the witness. This type will be the used in the pset Input.witness_utxo.witness field

RCasatta commented 1 year ago

Need to fix the CI for serde errors, however, I would like to have feedback on the approach

RCasatta commented 1 year ago

The pset from the added example test_txout_in_input_roundtrip:

cHNldP8BAgQCAAAAAQQBAQEFAQAB+wQCAAAAAAEBSQAAAAAAQwEAAUX0/DRkm5IlKGRYA9OcC8R0ixkGYK9kp7uNfCQNEtFBRngT4zJhFq2M+7RwgiVkF1UhB85xybOccBz0wvSILbgBDiAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEPBAAAAAAA

Seems to fail analyzepsbt on elements core, @delta1 could you confirm this would be breaking for elements-core? If it is I am not sure we want to go this way

Another non-breaking but technical-debt-making approach would be to have a new key

const PSET_IN_WITNESS_UTXO_WITNESS: u8 = 0x19;

with another field in pset Input : pub witness: TxOutWitness

delta1 commented 1 year ago

Indeed this fails in Elements

elements-cli analyzepsbt "cHNldP8BAgQCAAAAAQQBAQEFAQAB+wQCAAAAAAEBSQAAAAAAQwEAAUX0/DRkm5IlKGRYA9OcC8R0ixkGYK9kp7uNfCQNEtFBRngT4zJhFq2M+7RwgiVkF1UhB85xybOccBz0wvSILbgBDiAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEPBAAAAAAA"
error code: -22
error message:
TX decode failed Size of value was not the stated size: iostream error
RCasatta commented 1 year ago

The value is very likely to be searched in PSBT_ELEMENTS_IN_UTXO_RANGEPROOF so this is not needed.

We are validating this and will close if confirmed

RCasatta commented 1 year ago

I confirm PSBT_ELEMENTS_IN_UTXO_RANGEPROOF is what we need. So I am going to close this.

@apoelstra in hindsight I think it's a bad design to have struct with fields that are not initialized when decoded even if they are logically part of the struct. With this I don't mean we have to change it now, but if we were designing from scratch I would.