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

wire: add bitcoin network magic for default SigNet #2276

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

starius
Copy link

@starius starius commented Nov 21, 2024

Added constant wire.SigNet, added it to stringer for BitcoinNet type.

Added a test in chaincfg to compare calculated magic value with the constant.

@Roasbeef
Copy link
Member

Approved CI.

@@ -183,6 +183,9 @@ const (
// TestNet3 represents the test network (version 3).
TestNet3 BitcoinNet = 0x0709110b

// SigNet represents the signet (default).
SigNet BitcoinNet = 0x40CF030A
Copy link
Member

Choose a reason for hiding this comment

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

Is this documented anywhere? Doesn't seem to be in the OG BIP.

Copy link
Author

Choose a reason for hiding this comment

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

I copied the constant from https://en.bitcoin.it/wiki/Protocol_documentation#Common_structures
The constant is equal to chaincfg.SigNetParams.Net, this is checked in new test TestSigNetMagic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should also be equal to CustomSignetParams(DefaultSignetChallenge, nil).Net, or:

	challengeLength := byte(len(DefaultSignetChallenge))
	hashDouble := chainhash.DoubleHashB(
		append([]byte{challengeLength}, challenge...),
	)
	net := binary.LittleEndian.Uint32(hashDouble[0:4])

Perhaps we can add a simple test case for it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I guess that's what we're doing with TestSigNetMagic, so never mind.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11956947932

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.008%) to 57.242%

Files with Coverage Reduction New Missed Lines %
peer/peer.go 5 74.19%
Totals Coverage Status
Change from base Build 11941199528: 0.008%
Covered Lines: 29886
Relevant Lines: 52210

💛 - Coveralls

@guggero guggero self-requested a review November 22, 2024 07:41
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Just one clarification request, otherwise LGTM 🎉

@@ -183,6 +183,9 @@ const (
// TestNet3 represents the test network (version 3).
TestNet3 BitcoinNet = 0x0709110b

// SigNet represents the signet (default).
SigNet BitcoinNet = 0x40CF030A
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should also be equal to CustomSignetParams(DefaultSignetChallenge, nil).Net, or:

	challengeLength := byte(len(DefaultSignetChallenge))
	hashDouble := chainhash.DoubleHashB(
		append([]byte{challengeLength}, challenge...),
	)
	net := binary.LittleEndian.Uint32(hashDouble[0:4])

Perhaps we can add a simple test case for it?

wire/protocol.go Outdated
@@ -183,6 +183,9 @@ const (
// TestNet3 represents the test network (version 3).
TestNet3 BitcoinNet = 0x0709110b

// SigNet represents the signet (default).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is probably the way it's called in most places (signet (default)), though I think we could make it a bit more obvious that this is only one of many possible signets, we could add something like: // SigNet represents the public default SigNet. For custom signets, see CustomSignetParams`.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@@ -183,6 +183,9 @@ const (
// TestNet3 represents the test network (version 3).
TestNet3 BitcoinNet = 0x0709110b

// SigNet represents the signet (default).
SigNet BitcoinNet = 0x40CF030A
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I guess that's what we're doing with TestSigNetMagic, so never mind.

Added constant wire.SigNet, added it to stringer for BitcoinNet type.
Added a test in chaincfg to compare calculated magic value with the constant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants