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

add signet support #291

Merged
merged 1 commit into from Oct 26, 2020
Merged

Conversation

kallewoof
Copy link
Contributor

@kallewoof kallewoof commented Jul 10, 2019

This PR adds the "signet" network type to the list of networks.

See also:

@stevenroose
Copy link
Collaborator

FWIW, #255 removed our dependency on rust-bech32-bitcoin but is blocked on a bech32 PR, but that should land soon.

@stevenroose
Copy link
Collaborator

Getting bitcoin_constants in would make this a lot nicer. It would also solve the problem I have with having to duplicate a lot of code for Elements and Liquid.

@kallewoof
Copy link
Contributor Author

@stevenroose Yeah I noticed a lot of duplicate code all over the place. Wasn't sure what that was about but hopefully it can be reduced. :) I am gonna leave the bech32 PR for now, even if it is not going to relate to this repo specifically in the end.

@clarkmoody
Copy link
Member

Yeah #255 looks good and should allow us to narrow dependency scope for some projects. I believe we're closing in on rust-bitcoin/rest-bech32#35.

@kallewoof kallewoof force-pushed the 2019-07-signet branch 2 times, most recently from 5be1545 to 973936a Compare July 10, 2019 22:57
@kallewoof kallewoof changed the title [DONOTMERGE] add signet support add signet support Jul 10, 2019
@kallewoof kallewoof force-pushed the 2019-07-signet branch 2 times, most recently from 19b0295 to ed1fb0a Compare July 30, 2019 06:50
@codecov-io
Copy link

codecov-io commented Jul 30, 2019

Codecov Report

Merging #291 into master will decrease coverage by 0.11%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #291      +/-   ##
=========================================
- Coverage   85.81%   85.7%   -0.12%     
=========================================
  Files          40      40              
  Lines        7461    7752     +291     
=========================================
+ Hits         6403    6644     +241     
- Misses       1058    1108      +50
Impacted Files Coverage Δ
src/util/bip32.rs 87.14% <0%> (ø) ⬆️
src/consensus/params.rs 0% <0%> (ø) ⬆️
src/blockdata/constants.rs 90.9% <100%> (+4.7%) ⬆️
src/util/key.rs 73.62% <100%> (+0.84%) ⬆️
src/util/address.rs 88.01% <50%> (+1.7%) ⬆️
src/network/constants.rs 86.82% <91.66%> (+1.55%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cff794...505ebce. Read the comment docs.

@dongcarl
Copy link
Member

@kallewoof I'd be happy to review after rebase ☺️

@kallewoof
Copy link
Contributor Author

@dongcarl I didn't realize it was in need of a rebase until you nudged, thanks. I think I've rebased it and it works, but my account is flagged at Travis right now so I don't think that CI will ever finish..

Copy link
Member

@dongcarl dongcarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we only need to complete the TODO before this is usable?

merkle_root: txdata[0].txid(),
time: 1534313275,
bits: 0x1e2adc28,
nonce: 621297
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: you can verify the time, bits, and nonce parameters by checking:

https://github.com/kallewoof/bitcoin/blob/2b9db9ac0936cf278ad27c59497d9feec61f6440/src/chainparams.cpp#L512

and

https://github.com/kallewoof/bitcoin/blob/2b9db9ac0936cf278ad27c59497d9feec61f6440/src/chainparams.cpp#L270

We need to make sure that, before merging, there's some way of passing in the blockscript and creating the Coinbase signature like so:

https://github.com/kallewoof/bitcoin/blob/2b9db9ac0936cf278ad27c59497d9feec61f6440/src/chainparams.cpp#L505-L513

0x62a429192f9e8ce5u64,
0xc63b5f100aa5d8dfu64,
0x3c82faa55d83e40au64,
0x00002adc28cf53b6u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/consensus/params.rs Show resolved Hide resolved
@@ -94,6 +97,7 @@ impl Network {
match *self {
Network::Bitcoin => 0xD9B4BEF9,
Network::Testnet => 0x0709110B,
Network::Signet => 0x6A70C7F0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/util/bip32.rs Show resolved Hide resolved
src/util/key.rs Outdated
@@ -128,6 +128,7 @@ impl PrivateKey {
let mut ret = [0; 34];
ret[0] = match self.network {
Network::Bitcoin => 128,
Network::Signet => 217,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to also add the parsing logic for this in the from_wif method.

@@ -370,6 +370,7 @@ impl Display for Address {
let mut prefixed = [0; 21];
prefixed[0] = match self.network {
Network::Bitcoin => 0,
Network::Signet => 125,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dongcarl
Copy link
Member

Kicking Travis!

@dr-orlovsky
Copy link
Collaborator

Why is it closed?

@kallewoof
Copy link
Contributor Author

@dr-orlovsky Nobody has looked at it for 5 months. Maybe after signet is merged into core it will have more interest.

@dr-orlovsky
Copy link
Collaborator

Well, I do need it in rust-bitcoin... Probably I will integrate it into my develop branch directly at this stage.

@kallewoof
Copy link
Contributor Author

I actually didn't realize I can't reopen PRs I close on my own, but in either case, I do intend to make a PR, and I will maintain this branch so feel free to use it! If you need help don't hesitate to ping me btw. I'd love to see what you're working on.

@kallewoof
Copy link
Contributor Author

@stevenroose good catch! Added Signet case.

@kallewoof
Copy link
Contributor Author

kallewoof commented Jun 10, 2020

Edit: the travis error is unrelated to this PR. See #431.

stevenroose
stevenroose previously approved these changes Jun 28, 2020
Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sgeisler
sgeisler previously approved these changes Aug 12, 2020
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk if the changes are exhaustive, but the code that is there looks good. ACK from me.

Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One test fails + needs rebase with a small conflict in one comment

src/blockdata/constants.rs Outdated Show resolved Hide resolved
@kallewoof
Copy link
Contributor Author

Thanks for the nudge. Rebased and fixed genesis nonce/bits/timestamp.

@dr-orlovsky
Copy link
Collaborator

tACK a3d9899

@dr-orlovsky
Copy link
Collaborator

dr-orlovsky commented Sep 18, 2020

Not sure however how it matches information found in ElementsProject/lightning#2972 about that the new genesis signet nonce is 621297

@kallewoof
Copy link
Contributor Author

@dr-orlovsky Yeah, well noted. The nonce and such unfortunately changed when we nudged the POW limit awhile ago. I haven't done a re-request to the lightning project with an update yet.

@dr-orlovsky
Copy link
Collaborator

@kallewoof: c-Lightning version PR with fix: ElementsProject/lightning#4068

@dr-orlovsky
Copy link
Collaborator

Signet got merged into Bitcoin Core master: bitcoin/bitcoin#18267 (comment)

Let's merge this PR as well - and unblock downstream Electrs dependency: romanz/electrs#239

@kallewoof
Copy link
Contributor Author

Any ideas on how to push this forward? Should I nudge someone, or just sit tight? Who/where if the former?

Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified all the constants against bitcoin core, they all match except bip16_time which I couldn't figure out what is the equivalent in bitcoin core

@kallewoof
Copy link
Contributor Author

Thanks, @elichai! I think the bip16_time is only relevant during main net IBD, due to that one offending block, which does not exist in signet, so as long as it's not in the future, it should be fine with any value, I think.

@apoelstra apoelstra added the API break This PR requires a version bump for the next release label Oct 8, 2020
dr-orlovsky added a commit to LNP-BP/rust-lnpbp that referenced this pull request Oct 21, 2020
@apoelstra apoelstra merged commit 93df7cb into rust-bitcoin:master Oct 26, 2020
romanz referenced this pull request in romanz/electrs Nov 5, 2020
@kallewoof kallewoof deleted the 2019-07-signet branch November 6, 2020 00:51
Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API break This PR requires a version bump for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet