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

confidential: Don't reverse explicit values for serde #174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stevenroose
Copy link
Member

@stevenroose stevenroose commented Aug 21, 2023

So this is a breaking change. It might affect people actually using the serde serialization of confidential::Value::Explicit right now. Which is really unfortunate.

But IMO this is a bug. We for some miraculous reason (probably a mistake on some programmer's end) reverse the bytes when consensus-encoding explicit values. That's fine. But somehow we also ended up doing this for serde, which makes no sense.

I'm hitting this when trying to use this type in a Web setting through WASM and a small number of satoshis will (when reversed) cause a JS overflow (which only supports 53 bit integers).

@stevenroose
Copy link
Member Author

Maybe I should use the opportunity to change the serde for all confidential types to be a bit more JSON-friendly and use the lenght/type of the field to know whether it's confidential or not... Opinions welcome.

@apoelstra
Copy link
Member

Why doesn't it make sense for the serde encoding to match the on-wire encoding for explicit amounts, when it matches the encoding for confidential amounts?

@sanket1729
Copy link
Member

I agree that this is more of a bug, but I think that we should just live with it. This change will break a lot of things in many subtle ways. I think some implementations for liquid of electrum server also use serde format as a persistent store.

Can you work around this for your setting?

@sanket1729
Copy link
Member

See #96 and #105 where we broke serde format across versions.

@apoelstra
Copy link
Member

If it's a bug, it seems like it's a consensus bug in Elements. We can fix it during the bulletproof hardfork maybe.

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.

3 participants