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

BIP155: Mention SHA3-256 explicitly #1002

Merged
merged 2 commits into from
Oct 6, 2020
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 27, 2020

It seems better to clarify that CHECKSUM in Tor onion v3 address uses SHA3-256 hash function.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

@@ -166,10 +166,11 @@ onion_address = base32(PUBKEY | CHECKSUM | VERSION) + ".onion"
CHECKSUM = H(".onion checksum" | PUBKEY | VERSION)[:2]

where:
- PUBKEY is the 32 bytes ed25519 master pubkey of the hidden service.
- PUBKEY is the 32 bytes ed25519 master pubkey of the hidden service
- VERSION is an one byte version field (default value '\x03')
Copy link
Member

Choose a reason for hiding this comment

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

While here, if you are inclined to, could s/an one/a one/ and remove the EOL whitespace at line 155: The new message introduces a separate network ID for <code>TORV2</code>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@vasild
Copy link
Contributor

vasild commented Sep 28, 2020

ACK 6ef71b3

Duplicating the Tor spec into this BIP does not seem very useful to me, but, ok, since we already have some of it, why not clarify what is H().

@laanwj
Copy link
Member

laanwj commented Oct 6, 2020 via email

@laanwj laanwj merged commit ebeb28e into bitcoin:master Oct 6, 2020
@hebasto hebasto deleted the 200927-bip155-sha branch October 6, 2020 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants