search

haskoin / haskoin-core

Haskoin Core is a Bitcoin and Bitcoin Cash library
MIT License
522 stars 94 forks source link

I have cutted your base58 out! #142

Closed s9gf4ult closed 9 years ago

s9gf4ult commented 9 years ago

Hello, I have cutted base58 implementation to separate package https://bitbucket.org/s9gf4ult/base58-bytestring

My question is: do you need a patch to this package adding dependency from base58-bytestring and removing it's implementation? Or you guys maybe want to add base58-bytestring to haskoin team on github?

plaprade commented 9 years ago

Hi, I like the idea of cutting out the base58 implementation into its own separate package.

I had a quick look at the code. I guess you didn't add the checksum functions as they were not strictly relevant to the base58 encoding and ripple/flickr? You do some property testing which is good as it guarantees internal consistency (decode . encode = id) but I'd also add some unit tests against known correct vectors to be sure that the library is not only internally consistent, but does the correct thing :)

If you want to provide a patch for Haskoin, that would be cool. As Base58.hs doesn't do base 58 really anymore, I was thinking of renaming it to Address.hs and link it properly to your new library. I would like to keep all the tests which are currently in Haskoin however just to avoid any regressions.

If you want to host the library in the Haskoin github page, it's all up to you. If you want to do that, I'll give you read/write access to the base58 package so you can continue maintaining it. Let me know.

Cheers,

s9gf4ult commented 9 years ago

You absolutely right about tests. Is there any golden files from bitcoin project or something?

What do you think about adding base58check inside base58-bytestring?

If yes, then base58-bytestring will not be such lightweight becuase of Crypto.Hash.SHA256 module from cryptohash as dependency (am i right?).

If no, then where base58check must be? It is strange to leave it in haskoin and too tiny to create another separate package like base58check-bytestring for it.

plaprade commented 9 years ago

I've been thinking about this for a while, and my opinion is that base58check is very specific to Bitcoin. It is only relevant for Bitcoin addresses and for a few other things like Bitcoin WIF encodings. I don't think it makes a lot of sense to include a very bitcoin-specific function into a generic base58 bytestring library. And as you pointed out, we would be adding an unnecessary dependency on the SHA256.

My suggestion would be to keep base58check functions in the Haskoin library together with the Address type, as they both work together. It makes sense to have them in the same place.

jprider63 commented 9 years ago

Huh, I just did this too. Maybe we can just work together?

Also, I'm using functionality similar to base58check even though I'm not using bitcoin, so maybe it's worth including base58check as well?

plaprade commented 9 years ago

Using the first 4 bytes of SHA256 as an error detection mechanism works, but it's not the smartest solution. There are much better ways of using codes to detect errors and even correct them. My opinion is that you should use another error detecting/correcting mechanism unless you really need backwards compatibility with bitcoin for some reason. I still believe that this somewhat weird checksum computation (base58check) should remain in bitcoin libraries and not in a generic base58 bytestring library.

jprider63 commented 9 years ago

Ok, that seems reasonable to me. Do you have suggestions for a better error detection mechanism?

sha49 commented 9 years ago

Base58check is a particular case of a checksum which is a very simple error-detecting code. It is fast and simple, that's it. Whenever you need a checksum, just use it. I do not think it worthwhile exposing this functionality as a general thing.

If you are interested in better ways to detect/correct errors, you should look in Coding Theory (http://en.wikipedia.org/wiki/Error_detection_and_correction). There are loots of schemes and I am sure you will find something satisfying your needs.

s9gf4ult commented 9 years ago

@jprider63 You are welcome to join. I have added some benchmarks https://bitbucket.org/s9gf4ult/base58-bytestring/src/638c1619bca7749f4a2a20079abf062533df8c10/benchmark-results/?at=master and performance is not very good for now. So we still have something to improve in base58-bytestring.

Maybe it is meaningful to generalize things with base58check to something like

data BChecker =
    BChecker
    { bCheck  :: ByteString  -> Bool
    , bEncode :: ByteString -> ByteString
    }

base58check :: BChecker
base58check =
    BChecker
    { bCheck = ...
    , bEncode = ...
    }

and implement several ways of checksuming including base58check and more reliable ways.

jprider63 commented 9 years ago

Ok, cool. Do you want to add me? I'm jprider63 on bitbucket too. I also have a few ideas of how to improve performance.

I'm starting to agree that the checksum functionality shouldn't be included in this package, but maybe another package dedicated to various checksums.

s9gf4ult commented 9 years ago

Added. But when me/we prepare patch for haskoin this repo will likely be moved to haskoin team on github.