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

Cuts down serialization logic #38

Merged
merged 2 commits into from
Nov 27, 2022
Merged

Conversation

GambolingPangolin
Copy link
Contributor

@GambolingPangolin GambolingPangolin commented Oct 18, 2022

This PR removes the dependencies on bytes and cereal. Microbenchmarks show that binary is a little faster for encoding and decoding than cereal. Moreover, serializing to lazy ByteString is more composable.

Closes #2
Closes #20

@GambolingPangolin GambolingPangolin force-pushed the serialization-overhaul branch 2 times, most recently from deeb050 to ced96b2 Compare October 18, 2022 02:38
@GambolingPangolin
Copy link
Contributor Author

GambolingPangolin commented Oct 18, 2022

  • We can probably handle scripts a bit better. One option is to use strict encodings of keys directly instead of the Binary instance. Another option is use lazy ByteString as the representation for data carrying opcodes. We should probably do some exploration there.
  • There is a small performance regression after the patch. It's not terrible, but maybe we should take some time to address it.
  • I did not change the API to include specific encode and decode for each major type.
  • Should we change the type of WitnessStack to be a list of lazy ByteString values? That would improve serialization.

tochicool
tochicool previously approved these changes Oct 20, 2022
src/Bitcoin/Transaction/Common.hs Outdated Show resolved Hide resolved
@ProofOfKeags
Copy link
Collaborator

We can probably handle scripts a bit better. One option is to use strict encodings of keys directly instead of the Binary instance. Another option is use lazy ByteString as the representation for data carrying opcodes. We should probably do some exploration there.

One of the things I have in my notes is that we wanted to overhaul how scripts worked to begin with. When you say "handle scripts a bit better", are you meaning with respect to performance?

I believe that the data carrying opcodes are always less than 520 bytes long which is limited by protocol level consensus stuff. In addition to that we rarely manipulate these things except to send them to cryptographic functions which generally require ByteArrayAccess instances. A cursory search shows me that ShortByteStrings don't actually have a ByteArrayAccess instance but I can't tell why. It seems like they should be able to.

I did not change the API to include specific encode and decode for each major type.

I think we can defer this if we do it at all.

Should we change the type of WitnessStack to be a list of lazy ByteString values? That would improve serialization.

I don't have an issue with this.

@GambolingPangolin
Copy link
Contributor Author

Scripts. There are some tweaks that we can make to the representation in order to make serialization a bit more performant. However, since we do plan to rework that quite a lot over the next few months, we should not sweat any more changes now.

API. It would be nice to export direct serialization functions for the most important types. That helps downstream users (potentially most if we get it right) avoid depending on binary.

Witness stack. I'll go ahead and covert this to LazyByteString.

@ProofOfKeags
Copy link
Collaborator

wrt Scripts and API we don't need to tie that to this PR. So I'd say merge after the Witness stack change.

@GambolingPangolin
Copy link
Contributor Author

Unfortunately, changing the type of WitnessStack ended up being really hairy. I think that it would be better to do that as a separate change. So this patch is ready to merge.

@GambolingPangolin GambolingPangolin merged commit 1c43de8 into master Nov 27, 2022
@GambolingPangolin GambolingPangolin deleted the serialization-overhaul branch November 29, 2022 01:20
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.

Remove cereal as a dependency Remove cereal instances
3 participants