-
Notifications
You must be signed in to change notification settings - Fork 184
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
feature request: add litecoin support #142
Comments
In order to do this, we'd need to abstract away all PoW checking methods and a number of other |
Having had time to think & discuss this some more (thanks @losh11), I realised that Builder.DefaultP does not need to be changed, as 'p' is passed in to most Builder methods. So the handful of places Neutrino uses Builder could be changed to pass in an appropriate 'p' value dependent on whether Bitcoin or Litecoin is the active mode. @Roasbeef would it be acceptable to abstract&extend chainParams adding a coin identifier (Bitcoin/Litecoin) and a defaultP setting? Also a change to checkProofOfWork for Litecoin mode. I would like to submit a pull request to this effect. |
@Wowee0 why would P value need to different for litecoin? Further, why would the P value be modified without also modifying the value of M? |
I did a lot of debugging in Neutrino to find out why received Litecoin cFilterHeaders would not match the expected checkpoint, eventually tracked it down to the output of builder.BuildBasicFilter. Diffing gcs/builder.go between btcsuite/btcutil and ltcsuite/ltcutil/ indicates that the P value of 20 was used for LTCD (M was not changed). After making the change on the Neutrino side they matched. |
Thanks @Wowee0, yes that does appear to be what happened and makes sense why they wouldn’t match up. That being said, my recommendation would be to correct the value to 19 in ltcsuite/ltcutil. Users that have cfindex enabled will need to drop and reindex, but that only needs to happen once. Given that there hasn’t been an official deployment on litecoin, I think would be best to keep the implementation consistent with BIP158. Note that if litecoind were to pull in changes from upstream, it would have M=19, and the two implementations would differ for the same filter type. A significant amount of research was invested into the parameter choices, with the goal of minimizing the overall filter size. This write up by sipa explains the choices of the parameters in BIP158 https://gist.github.com/sipa/576d5f09c3b86c3b1b75598d799fc845 Above that, any future optimizations created for BIP158-parameterized filters may not backport as readily, and may require more code duplication if the parameters are distinct |
…has changed from 20 to 19. reference: lightninglabs/neutrino#142 (comment)
Thanks @cfromknecht your suggestion has been implemented in ltcutil & ltcd. Neutrino/blockmanager.go/CheckHeaderSanity : LND/config.go : The proposed code changes can be viewed here: |
NACK |
@benthecarman do you have a reason for the NACK? |
as @Roasbeef stated it will require more complexity around important code. Also there's no reason to support shitcoins |
This isn't the place to argue over this. That being said LND already supports the litecoin chain... and supports all features, excluding neutrino. We would just like to see feature parity between the chains. |
Neutrino currently does not support litecoin network, in either mainnet or testnet. It is frustrating to build litecoin software without even testnet support.
The text was updated successfully, but these errors were encountered: