Skip to content
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

Pset roundtrip fix #184

Closed
wants to merge 7 commits into from

Conversation

RCasatta
Copy link
Collaborator

@RCasatta RCasatta commented Nov 24, 2023

@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
Copy link
Collaborator Author

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

TxOut is encoded/decoded without the witness, it is handled separetely during
transaction encoding/decoding (segwit).
For PSET the TxOut encoding is reused but in this case the witness is not
handled separately.
This commit introduce TxOutWithWitness which is logically a TxOut with the
exception that also the witness is encoded/decoded.
4 Existing tests breaks with this change because they are using string test
vectors not containing the witness. To avoid too many changes this tests
are temporary ignored, they will be re-added in following commit in the same PR.
pset_swap_tutorial test vector is updated with the updated serialization of
TxOutWithWitness.

Note the old test vector doesn't contain a input.witness_utxo.witness but the
changed encoding require to find an empty value there.

To achieve so, decode of TxOutWitness has been temporary changed to skip
the witness decoding, the parsed PSET is then encoded with the updated encoding.
The result is then used as `pset_swap_tutorial.hex`
Copy link
Member

@delta1 delta1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@RCasatta
Copy link
Collaborator Author

RCasatta commented Nov 25, 2023

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
Copy link
Member

delta1 commented Nov 27, 2023

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@RCasatta RCasatta closed this Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants