-
Notifications
You must be signed in to change notification settings - Fork 906
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
add signet support #2816
add signet support #2816
Conversation
d6af75a
to
ca06167
Compare
Awesome, thanks for the PR @kallewoof. I think this is a good addition once bitcoin/bitcoin#16411 is merged ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good, minor suggestion.
718a622
to
618bedc
Compare
618bedc
to
bdf7461
Compare
bdf7461
to
7eb368a
Compare
@ZmnSCPxj Thanks for the review, I believe I addressed everything you pointed at. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ad84897
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. suggestion to also update: jsonrpc.c:979
bitcoin/chainparams.c
Outdated
.when_lightning_became_cool = 1, | ||
.p2pkh_version = 125, | ||
.p2sh_version = 87, | ||
.testnet = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue that we should set this to true. see lightningd/bitcoin.c:380-387
and lightningd/options.c:685-688
for rationale.
7eb368a
to
d12c329
Compare
@niftynei Thanks! I switched |
ACK d12c329 |
Were we not supposed to hold off on this until bitcoin/bitcoin#16411 was merged? |
Ideally, but it's not harmful or causing weird dependency issues. Only thing is people will have to run a custom build of bitcoin core to use this, at the moment. |
I am more concerned that changes to the bitcoin PR might require changes to this PR, but I suppose the upstream PR is now mostly settled. |
Ahh, that is a good point. |
Yeah ideally this would have been held til the bitcoind PR was merged; I
wasn’t aware of the status of the upstream dependency til @cdecker pointed
it out (after it was merged).
It’s also missing a CHANGELOG entry 🙃
…On Tue, Jul 23, 2019 at 19:55 kallewoof ***@***.***> wrote:
Ahh, that is a good point.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2816?email_source=notifications&email_token=AAIMAKM2YF3SHWC3FKWFFETQA6R75A5CNFSM4IE4YKQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2U223Q#issuecomment-514436462>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIMAKP55PHDVX5QFISSWFDQA6R75ANCNFSM4IE4YKQA>
.
|
Sad, but if you wish, you can revert this PR and I will make a new one once the bitcoin core PR has been merged. If not, I will make sure that any changes done on bitcoin side are duplicated here, in a follow-up PR. |
This PR adds support for signet networks.
Blocked by: bitcoin/bitcoin#16411
See also: https://en.bitcoin.it/wiki/Signet