haskell-bitcoin / bitcoin

Bitcoin Library for Haskell
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Cuts down serialization logic #38

Closed GambolingPangolin closed 1 year ago

GambolingPangolin commented 1 year ago

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 commented 1 year ago
ProofOfKeags commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.