-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
WIP [don't merge] Add BIP324 - Version 2 Peer-to-Peer Message Transport Protocol #1024
Conversation
Concept ACK |
Question: why not to stick to the same messaging structure used by the current LN and increase the number of encoding standards/protocol/library code? |
@dr-orlovsky The reason why I don't think using BOLT8 directly are the following:
|
|
||
== Motivation == | ||
|
||
The current peer-to-peer protocol is partially inefficient and in plaintext. |
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 don't like "current" for the same reason I don't like it on Wikipedia, it will be weird reading this document in 5 years. Better just write "the P2P protocol supported in Bitcoin 0.20.1".
"partially inefficient" in which way? Because of the double-SHA256? Surely the lack of encryption isn't inefficient, which is what most of the section talks about. I would at least just remove "partially", inefficiency isn't black/white in any case, so the word "partially" doesn't add anything.
|
||
== Abstract == | ||
|
||
This BIP describes a new Bitcoin peer to peer transport protocol with opportunistic encryption. |
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.
Inconsistent hyphenation in peer-to-peer
|
||
Adding opportunistic encryption introduces a high risk for attackers of being detected. Peer operators can compare encryption session IDs or use other form of authentication schemes <ref name="bip150">[https://github.com/bitcoin/bips/blob/master/bip-0150.mediawiki BIP150]</ref> to identify an attack. | ||
|
||
Each current version 1 Bitcoin peer-to-peer message uses a double-SHA256 checksum truncated to 4 bytes. Roughly the same amount of computation power would be required for encrypting and authenticating a peer-to-peer message with ChaCha20 & Poly1305. |
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.
It would be nice if there were some rough figures referenced here, maybe in seconds per byte?
|
||
Additionally, this BIP describes a way to identify data which has been manipulated by peers (intercepting, then blocking or tampering with messages). | ||
|
||
Encrypting traffic between peers is already possible with VPN, tor, stunnel, curveCP or any other encryption mechanism on a deeper OSI level, however, most of those solutions require significant technical experience in setting up a secure channel and are therefore not widely deployed. |
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.
Why invoke OSI? It is not clear what is deep in OSI. Apparently it is anything that can wrap the Bitcoin P2P protocol. But such a protocol could also leverage the application layer. I think it would be better to just say it can wrap Bitcoin.
|
||
A peer signaling <code>NODE_P2P_V2</code> MUST accept encrypted communication specified in this proposal. | ||
|
||
Peers MAY only make outbound connections to peers supporting <code>NODE_P2P_V2</code>. |
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.
Before you connect, you can't know whether the node supports V2. Why not say "maintain" instead of "make"?
Why is "peers" plural here, when it is singular above? An implementer of this BIP will write node software for a single peer, so I think singular works better. Better use plural for the other nodes.
|
||
Optimized implementations of ChaCha20-Poly1305@bitcoin are relatively fast, therefore it is very likely that encrypted messages will not require additional CPU cycles per byte when compared to the current unencrypted p2p message format (ChaCha20/Poly1305 versus double SHA256). | ||
|
||
The initial packet sequence numbers are 0. |
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.
Since there is only one initial element of anything, I suggest using singular.
|
||
Encrypted messages do not have the 4byte network magic. | ||
|
||
The maximum message size is 2^23 (8,388,608) bytes. Future communication MAY exceed this limit and thus MUST be split into different messages. |
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.
To make it clear that this is not xor, one could spell out "to the power of".
|
||
Peers calculate the counterparty limits and MUST disconnect immediately if a violation of the limits has been detected. | ||
|
||
=== Test Vectors === |
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.
why not put this in a CSV file so that it can easily be tested?
|
||
== Motivation == | ||
|
||
The current peer-to-peer protocol is partially inefficient and in plaintext. |
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.
Maybe link to the section "Length comparisons between v1 and v2 messages" as a reference for the inefficiency.
------------------------------------------------------------------------------------------ | ||
</pre> | ||
|
||
==== AEAD Test Vectors ==== |
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.
These could be in a CSV file.
@jonasschnelli Are you still working on this? |
@michaelfolkson @luke-jr I agree we can close this PR. The new new draft is very different and reviewing the diff will not make sense. I will link this closed PR in the new one. |
Discussions happend here:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/thread.html#16806
https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52#comments
Implementations is partially merged in Bitcoin Core (AEAD, KDF).