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

Allow Serde-serialisation of Streamable #127

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

Conversation

xearl4
Copy link
Contributor

@xearl4 xearl4 commented Jan 24, 2023

This pull-request adds a "serde" build feature to chia-protocol. When this build feature is enabled, all chia-protocol types derive Serde's Serialize. This allows chia-protocol types to be serialised via Serde to different formats, most notably probably JSON, which can be very helpful for testing or glueing into other systems.

This PR only gets basic Serde serialisation working, without support for deserialisation. Serialisation follows Serde's defaults wherever possible. Fixed-length byte types (bytes48, bytes96, etc) are serialised to an array of bytes (numbers) for a start.

This PR is primarily to start discussion if there's interest in merging this into chia_rs at all. We are currently using this change to use chia_rs to convert from fullblock data to JSON for ad-hoc scripting/reporting. If there's interest in merging this, a next step would probably ensure that the JSON generated by chia_rs matches the JSON generated by chia-blockchain's Python.

@arvidn
Copy link
Contributor

arvidn commented Jan 25, 2023

one thing that worries me a bit about this is that it may create an expectation of stability of names of struct members. serializing classes into json based on reflection has bitten us a few times in python, and we have a few (pretty ugly) work-arounds to preserve some level of backwards compatibility.

Are the FullBlock JSON you produce compatible with what chia-blockchain produces?

@arvidn
Copy link
Contributor

arvidn commented Jan 25, 2023

that said, we already serialize to JSON on the python side, so we will most likely want to do the same on the rust side, in a compatible manner.

@xearl4
Copy link
Contributor Author

xearl4 commented Jan 25, 2023

Are the FullBlock JSON you produce compatible with what chia-blockchain produces?

Nope, not yet, at least not intentionally or verified. If there's interest in merging this, I'll look into getting chia_rs's Rust JSON to structurally match chia-blockchain's Python JSON.

Meanwhile, you could either merge this as is right away and I'll send a separate PR for JSON-compatibility with Python in the coming days/weeks. Or you leave this PR pending and I'll update it with necessary changes for Python compatibility.

@arvidn
Copy link
Contributor

arvidn commented Feb 1, 2023

the reason I hesitate is because we'll need caompatibility with the JSON protocol established by python already. In order to make it compatible, I imagine you'd need to "fork" the built-in json serializer that comes with serde to modify it, or possibly modify some other aspect to make it compatible.

I'm pretty confident that it would be simpler to add json support in the macros we use for Streamable support right now. Our first attempt to serialize and deserialize classes in Streamable format was via serde, and we needed a lot of code to get that to work. Partly because we were required to handle types that aren't supported by Streamable, but we also had to write more code just to deal with the serde API. using the macro greatly simplified the code.

@xearl4
Copy link
Contributor Author

xearl4 commented Feb 1, 2023

Ok, I'll have a quick look into establishing structural compatibility with the Python JSON to get an idea how much work that'll be. I don't think a serde fork is necessary for that, currently I believe it would boil down to adding a few more Serialize specializations. Just like the one already in place for BytesImpl that serialises [u8, N] into a hex-encoded string instead of an array of numbers.

JSON serialisation via the streamable macros would certainly also be a good option. I'm not sure, though, it would be much simpler. You'd basically need the same type-specific overrides and at least a basic JSON serialiser on top of that.

@xearl4 xearl4 force-pushed the serde branch 2 times, most recently from 5dbe0f0 to a14d307 Compare February 1, 2023 20:35
@xearl4
Copy link
Contributor Author

xearl4 commented Feb 1, 2023

This latest version now produces JSON identical in structure and content to chia-blockchain's Python JSON serialisation.

Two things were missing compared to my original version:

  • bytes serialised as hexstrings needed a 0x prefix.
  • Variable-length bytes also needed serde specialisation to be serialised as hexstrings.

I checked equality by dumping the FullBlocks for mainnet height 1519806 (~48KiB raw streamable-encoded data, after zstd decompression) and height 1592629 (~417KiB) as JSON from Python (json.dumps(block.to_json_dict())) and from Rust. I then ran both JSONs through jq --compact . to unify formatting and then diffed the results.

impl<const N: usize> Serialize for BytesImpl<N> {
fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
let s = format!("0x{}", hex::encode(&self.0));
serializer.serialize_str(&s)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit of a hack. it will make the JSON serializer do the right thing, but any other serde serializer will do the wrong thing. which may not be the end of the world, since we don't need any other one, but it might also be surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a bit of a hack. I'll have a look if I can limit the specialisation to just serde_json's Serializer and let every other serde serializer just serialize as bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, still no way, afaict.

@arvidn
Copy link
Contributor

arvidn commented Feb 1, 2023

I suspect the G1Element and G2Element may need some fixup too

@xearl4
Copy link
Contributor Author

xearl4 commented Feb 1, 2023

I suspect the G1Element and G2Element may need some fixup too

G1Element is just a Bytes48 and G2Element a Bytes96, at the moment. The specialisation for fixed-length BytesImpl takes care of those. Similarly, with variable-length Bytes and Program.

@arvidn
Copy link
Contributor

arvidn commented Feb 1, 2023

G1Element is just a Bytes48 and G2Element a Bytes96, at the moment. The specialisation for fixed-length BytesImpl takes care of those. Similarly, with variable-length Bytes and Program.

technically they are 1-tuples of Bytes48 and Bytes96. Perhaps the serde JSON serializer flattens those. I don't know.

@xearl4
Copy link
Contributor Author

xearl4 commented Feb 1, 2023

Yes, the JSON serialiser flattens those (https://serde.rs/json.html#structs-and-enums-in-json).

@arvidn
Copy link
Contributor

arvidn commented Mar 12, 2024

I believe @Rigidity might be interested in picking this up

Copy link

Pull Request Test Coverage Report for Build 9875600010

Details

  • 1 of 16 (6.25%) changed or added relevant lines in 4 files are covered.
  • 1157 unchanged lines in 32 files lost coverage.
  • Overall coverage decreased (-8.3%) to 74.364%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/chia-protocol/src/bytes.rs 0 3 0.0%
crates/chia-bls/src/public_key.rs 0 6 0.0%
crates/chia-bls/src/signature.rs 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
crates/chia-consensus/src/gen/run_puzzle.rs 1 98.31%
crates/chia-protocol/src/classgroup.rs 1 0.0%
crates/chia-protocol/src/chia_protocol.rs 2 0.0%
crates/chia-consensus/src/gen/run_block_generator.rs 2 94.83%
crates/chia-consensus/src/consensus_constants.rs 2 0.0%
crates/chia-bls/src/parse_hex.rs 3 60.0%
crates/chia-consensus/src/gen/validation_error.rs 4 24.19%
wheel/src/adapt_response.rs 4 0.0%
crates/chia-bls/src/error.rs 4 0.0%
crates/chia-traits/src/chia_error.rs 4 0.0%
Totals Coverage Status
Change from base Build 9866893150: -8.3%
Covered Lines: 10405
Relevant Lines: 13992

💛 - Coveralls

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