-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Draft EIP: Whisper packet codes spec #627
Conversation
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 think this is a good start, but needs fleshing out a lot more if it's to be useful to other implementers.
|
||
The Whisper sub-protocol should support the following packet codes: | ||
|
||
Status (0x00) |
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.
You should probably put these in a table with a column for EIP, so later EIPs can extend this set.
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.
where can i see an example?
|
||
This packet contains two objects: integer (0x01) followed by a single Whisper Envelope. | ||
|
||
This packet is used for sending the standard Whisper envelopes. |
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.
Which are defined where?
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.
at the moment they are defined only in the source code of v5 implementation
EIPS/eip-draft-whisper.md
Outdated
@@ -1,20 +1,21 @@ | |||
## Preamble | |||
|
|||
EIP: draft | |||
Title: Whsiper Packet Codes - Specification | |||
Title: Whsiper Specification |
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.
s/whsiper/whisper/
EIPS/eip-draft-whisper.md
Outdated
|
||
[ Version, Expiry, TTL, Topic, AESNonce, Data, EnvNonce ] | ||
|
||
`Version`: up to 4 bytes (currently one byte containing zero). Version indicates encryption method. If Version is higher than current, envelope could not be decrypted, and therefore only forwarded to the peers. |
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.
s/decrypted/decoded/
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.
Should messages be forwarded even if they can't be decoded? What if they contain forwarding rules that the node can't understand?
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.
All messages are sent to everybody else, if they are valid (good format, not expired, good PoW, etc.). Decryption is only possible for the node that holds the corresponding key.
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.
Are all messages always broadcast everywhere? I was under the impression that some messages could be point-to-point, depending on the payload.
EIPS/eip-draft-whisper.md
Outdated
|
||
`Version`: up to 4 bytes (currently one byte containing zero). Version indicates encryption method. If Version is higher than current, envelope could not be decrypted, and therefore only forwarded to the peers. | ||
|
||
`Expiry`: 4 bytes (UNIX time in seconds). |
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 have both an expiry and a TTL?
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 sure that message is not created in the future.
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 follow; could you not just set an expiry time, and ditch the TTL?
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.
actually a decent point. what's important to the receiver is how long the message has left to live, not the time that it was initially meant to be live for.
EIPS/eip-draft-whisper.md
Outdated
|
||
Those unable to decrypt the message data are also unable to access the signature. The signature, if provided, is the ECDSA signature of the Keccak-256 hash of the unencrypted data using the secret key of the originator identity. The signature is serialised as the concatenation of the `R`, `S` and `V` parameters of the SECP-256k1 ECDSA signature, in that order. `R` and `S` are both big-endian encoded, fixed-width 256-bit unsigned. `V` is an 8-bit big-endian encoded, non-normalised and should be either 27 or 28. | ||
|
||
The padding is introduced in order to align the message size, since message size alone might reveal important metainformation. The first several bytes of padding (up to four bytes) indicate the total size of padding. E.g. if padding is less than 256 bytes, then one byte is enough; if padding is less than 65536 bytes, then 2 bytes; and so on. |
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.
Does the length prefix include the length bytes? How do I know how many length bytes to read?
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.
Does the length prefix include the length bytes? -- fixed.
How do I know how many length bytes to read? -- explained in the next line.
EIPS/eip-draft-whisper.md
Outdated
|
||
This packet contains two objects: integer (0x02) followed by a single Whisper Envelope. | ||
|
||
This packet is used for sending the peer-to-peer messages, which are not supposed to be forwarded any further. E.g. it might be used by the Whisper Mail Server for delivery of old (expired) messages, which is otherwise not allowed. |
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.
Mail server and relay of non-live messages should happen at a layer above the message relaying protocol or within a separate subprotocol. I vote to keep whisper as simple as possible by including only messages relevant to live envelope propagation.
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 is not only mail. There might be some other cases when you can not afford to spend time on PoW, e.g. some kind of real-time data feeds, etc.
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.
Those are valid use-cases, and that's fine, but in the aim of keeping the whisper protocol simple it is better to make this an opt-in subprotocol or happen via some other mode of communication with the trusted peer.
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 disagree with the notion that adding an extra message type suddenly makes whisper complicated. especially because this type is optional. if you don't implement it -- fine, just gracefully ignore this message. actually, this is required behavior for any unknown type anyway. the reason that this type is in the EIP is because it is reserved for this purpose. but it it not mandatory.
EIPS/eip-draft-whisper.md
Outdated
|
||
`Version`: up to 4 bytes (currently one byte containing zero). Version indicates encryption method. If Version is higher than current, envelope could not be decoded, and therefore only forwarded to the peers. | ||
|
||
`Expiry`: 4 bytes (UNIX time in seconds). |
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.
Should be 64-bits for future-proofing
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.
can you elaborate? what is your concern?
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.
After 2038 this type will be too small to hold future timestamps. I think it is better to future-proof it from the beginning without requiring a protocol upgrade at some point.
EIPS/eip-draft-whisper.md
Outdated
|
||
Envelopes are RLP-encoded structures of the following format: | ||
|
||
[ Version, Expiry, TTL, Topic, AESNonce, Data, EnvNonce ] |
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 disagree with the inclusion of AESNonce
-- the relaying of whisper envelopes should be completely generic over the contents of the payload. As long as the application using it (e.g. RPC API) expects a certain format to payloads with specific topic, we can include such metadata in there.
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.
Whisper can not be encryption-agnostic because it implements the filters, decrypts messages and delivers only decrypted messages to the Dapps.
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.
If it expects that messages with matching filters will contain payloads encrypted with a certain key/scheme pair (which is likely, given that it's a probabilistic indicator), then I don't see why it wouldn't work. Topic collision to this degree is already possible even without making it encryption-agnostic.
EIPS/eip-draft-whisper.md
Outdated
|
||
[ Version, Expiry, TTL, Topic, AESNonce, Data, EnvNonce ] | ||
|
||
`Version`: up to 4 bytes (currently one byte containing zero). Version indicates encryption method. If Version is higher than current, envelope could not be decoded, and therefore only forwarded to the peers. |
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.
My previous comments about encryption make version number unnecessary.
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.
please see my comment above
EIPS/eip-draft-whisper.md
Outdated
|
||
Packet codes 0x00 and 0x01 are already used in all Whisper versions. | ||
|
||
Packet codes 0x02 and 0x03 are necessary to implement Whisper Mail Server and Client. Without P2P messages it would be impossible to deliver the old messages, since they will be recognized as expired, and the peer will be disconnected for violating the Whisper protocol. They might be useful for other purposes when it is not possible to spend time on PoW, e.g. if a stock exchange will want to provide live feed about the latest trades. |
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.
This can be done on another subprotocol
At the level of the core wire protocol, I think the only concerns should be
We can have additional messages to peers detailing things like PoW or topic requirements in order to restrict the incoming set. All the described checks for payload validity, encryption, and so forth can be done at the application level. This also leaves us open to using the whisper relay network for a larger variety of applications, including those which broadcast plaintext or use alternate encryption. A less complicated message format means doing less work for packets that don't match your local filter. I would expect the RPC layer to handle things like standard message format, extracting a metadata such as AES nonce, encryption, signing, and decryption. Having a more general wire protocol lets us upgrade these standards without diminishing the connectivity of the network. The |
EIPS/eip-draft-whisper.md
Outdated
|
||
**PoW Requirement** [`0x04`, `PoW`] | ||
|
||
This packet contains two objects: integer (0x04) followed by a single floating point value of PoW. |
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.
what does this floating point value indicate? PoW and PoW heuristic calculation should be specified, otherwise nobody will come to agreement over what the "requirement" means.
Here's a decent method:
fn short_rlp(envelope) = rlp of envelope, excluding env_nonce field.
fn pow_hash(envelope, env_nonce) = sha3(short_rlp(envelope) ++ env_nonce)
lower pow_hash
is better (treating as big-endian 32-byte integer)
To calculate PoW heuristic:
fn work_to_space_time_ratio(pow_hash, size, ttl)
= 2**leading_zeros(pow_hash) / (size * ttl)
where size is the size of the full RLP-encoded envelope.
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.
thanks! this is how it is implemented, i will update the spec accordingly.
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.
perhaps an even better metric would be to split the hash into two bit vectors: leading_zeros
and nonzero_part
and then define
fn work_to_space_time_ratio(pow_hash, size, ttl)
=
let partial_len = min(len(leading_zeros), len(nonzero_part));
(2**len(leading_zeros) + (nonzero_part[0..partial_len] as uint) / (size * ttl)
to avoid work ramp-up being purely exponential. Although then you would likely get more floating point drift in the division.
EIPS/eip-draft-whisper.md
Outdated
|
||
This packet contains two objects: integer (0x05) followed by a byte array of arbitrary size. | ||
|
||
This packet is used by Whisper nodes for sharing their interest in messages with specific topics. |
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.
bloom function needs to be specified to define protocol misbehavior
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.
good point, i will do it
EIPS/eip-draft-whisper.md
Outdated
|
||
`TTL`: 4 bytes (time-to-live in seconds). | ||
|
||
`Topic`: 4 bytes of arbitrary data. |
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 a list of topics? Issuer pays for the PoW of more topics anyway.
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.
Because the purpose of Topic is probabilistic indication of encryption key (should we try to decrypt the message?), and not to indicate anything about the content of the message. Every message can be encrypted with only a single key, hence single Topic.
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.
You can have other topics though. For something like a broadcast you may multicast to multiple topics rather than only to a single one. This opens up the ability to use one-time keys for convenient broadcast with a pretty simple scheme (using a chat room as a concrete example):
- Encrypt the message with a 32-byte one-time use key K (and globally agreed-upon nonce). Call the ciphertext C
- Topics: [sha32(chat_room_name_1), sha32(chat_room_name_2), ...]
- Payload: [sha3(chat_room_name_1) ^ K, sha3(chat_room_name_2) ^ K, ..., C]
The payload length is always (32 * n_topics) + len(C).
Now anybody who knows any one (not all!) of the chat room names can decrypt the messages by
a) checking for the topic
b) extracting the one-time key
c) decrypting the 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.
This is very narrow case, which can be done on Dapp level without further complicating the Whisper protocol. E.g. every time somebody is invited to join a chat room, he receives a single message with all the details (session key, possible topics of outgoing messages, possible topics of incoming messages, etc.).
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 is just one of many use cases made possible by generalizing Whisper over encryption type and giving the ability to provide many topics. Can you find a way to multicast as efficiently with the spec here?
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.
Would agree, multiple topics would enable multicasting with much less work done. This is an advantage in a lot of usecases.
EIPS/eip-draft-whisper.md
Outdated
|
||
**Messages** [`0x01`, `whisper_envelope`] | ||
|
||
This packet contains two objects: integer (0x01) followed by a single Whisper Envelope. |
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.
Relaying only a single message per packet? It should be more efficient to allow batching.
Previous version of whisper also used the packet as a form of keep-alive: both peers on a connection would take turns sending (potentially empty) Messages
packets, starting with the peer with lower node key (comparing big-endian) and always responding within 1 second.
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 is better to leave it to the Dapp level.
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.
For using it as keep-alive, that's fair. But relaying multiple messages from your pool at a time is strictly an efficiency concern and can only be addressed at the wire protocol level.
EIPS/eip-draft-whisper.md
Outdated
|
||
Those unable to decrypt the message data are also unable to access the signature. The signature, if provided, is the ECDSA signature of the Keccak-256 hash of the unencrypted data using the secret key of the originator identity. The signature is serialised as the concatenation of the `R`, `S` and `V` parameters of the SECP-256k1 ECDSA signature, in that order. `R` and `S` are both big-endian encoded, fixed-width 256-bit unsigned. `V` is an 8-bit big-endian encoded, non-normalised and should be either 27 or 28. | ||
|
||
The padding is introduced in order to align the message size, since message size alone might reveal important metainformation. The first several bytes of padding (up to four bytes) indicate the total size of padding (including these length bytes). E.g. if padding is less than 256 bytes, then one byte is enough; if padding is less than 65536 bytes, then 2 bytes; and so on. |
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 like the addition of padding, but as per my other comments it is not the concern of the wire protocol
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.
If we leave it to the Dapp level, very few people will implement it, and those who do will look very suspicious. This is the whole point of plausible deniability -- it should be implemented by default and used by everyone, especially by those who don't actually need it.
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.
Good point. It still can be done at the RPC level.
EIPS/eip-draft-whisper.md
Outdated
Data field contains encrypted message of the Envelope. Plaintext (unencrypted) payload is formed as a concatenation of a single byte for flags, additional metadata (as stipulated by the flags) and the actual payload. The message has the following structure: | ||
This section describes optional description of Data Field to set up an example. It is only relevant if you want to decrypt the incoming message, but any other format would be perfectly valid and must be forwarded to the peers. | ||
|
||
Data field contains encrypted message of the Envelope. In case of symmetric encryption, it also contains appended Salt (12 bytes). Plaintext (unencrypted) payload is formed as a concatenation of a single byte for flags, additional metadata (as stipulated by the flags) and the actual payload. The message has the following structure: |
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.
agreed on data field format, but I think it actually belongs in the upcoming whisper RPCs EIP under a "Common Whisper Payload Format" section
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.
As RPC EIP does not exist yet, we should at least mention the payload format here. Maybe I should say it is temporarily here?
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.
that makes sense to me. i can start to draft the RPCs EIP shortly as well
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.
fixed
EIPS/eip-draft-whisper.md
Outdated
LET D[*] = 0 | ||
FOREACH i IN { 0, 1, 2 } DO | ||
LET n = S[i] | ||
IF S[3] & (2 ** i) THEN n += 512 |
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.
wouldn't it be n += 256
, since it's meant to be treated as the 9th bit? adding 512 would overflow, but also be congruent mod 512 in the first place.
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.
indeed, thanks!
EIPS/eip-draft-whisper.md
Outdated
|
||
[ Version, Expiry, TTL, Topic, Data, Nonce ] | ||
|
||
`Version`: 1 byte. If Version is higher than current, envelope could not be decoded, and therefore only forwarded to the peers. |
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.
main problem here is that without Expiry
, TTL
, and Nonce
, we can't know when to evict a "future version" message from the pool. I would say it should be illegal to forward a message to a peer which isn't decodable by them, according to their whisper version.
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.
so new version nodes can continue to forward old version formats, but old version nodes can't forward new version formats. nodes should also have some idea of which version numbers actually change the format
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.
also: clarify what to set version
to.
is it always equal to the protocol version of the creating node? or equal to an alternate envelope format version? not all protocol version upgrades will change the envelope format.
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 disagree. all envelopes should be forwarded further, unless envelope is invalid. besides, when you start a couple of nodes with a new version they will not be able to send any messages because most likely they will be surrounded by the nodes with the old version.
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.
the purpose of version is to indicate the encryption algorithm, in case we will add a new one, or we will have multiple clients using different algorithms (e.g. geth vs. parity).
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.
hm, but since payload is general then version should be contained within there, rather than within the envelope. I was under the impression that version
here was to signify the envelope format version (if extra fields were added or something), and then we can't always know if there will be a TTL/Expiry.
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.
requiring all future versions to have these fields really just makes topic and data (and potentially new fields) the only ones that can change. For backwards compatibility we can say that
- all versions of the message must begin with
[expiry, ttl]
, and end withnonce
. - if we do need to change this for some reason, that's also fine. We just can't relay such messages to peers with older versions. The upgraded whisper nodes can choose to send old-style packets until they have enough peers who have upgraded as well.
It should still be against the protocol to send a peer a message they cannot evaluate expiry or PoW for. Otherwise, how can they relay it, or know when to stop relaying it?
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 see your point. so, do you want to remove Version?
EIPS/eip-draft-whisper.md
Outdated
|
||
**Messages** [`1`, `whisper_envelopes`] | ||
|
||
This packet contains two objects: integer (0x01) followed by a list of Whisper Envelopes. |
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.
clarify: may be empty. we need either that or a ping-pong mechanism
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.
ok
EIPS/eip-draft-whisper.md
Outdated
|
||
### Packet Format and Usage | ||
|
||
**Status** [`0`, `whisper_protocol_version`] |
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.
actually unnecessary: devp2p already handles protocol version negotiation. this only introduces further opportunity for mismatch
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.
so then do we actually need a status packet at all?
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.
this is a legacy from all the old versions. actually, this is the only thing they have in common. this should be the first packet. it might possibly contain other information in future versions. i suggest we leave it for backwards compatibility. i will remove the version info, though.
EIPS/eip-draft-whisper.md
Outdated
|
||
**PoW Requirement** [`2`, `PoW`] | ||
|
||
This packet contains two objects: integer (0x02) followed by a single floating point value of PoW. |
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.
floating point, 32 or 64 bit? You can serialize their bits directly, but you have to be careful when decoding that the bit pattern doesn't encode an sNAN
. Maybe specify that sending any of qNAN
, sNAN
, INF
, or -INF
is illegal?
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.
or perhaps even subnormal numbers shouldn't be allowed?
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 guess, it should be 32 bit, and meaningless stuff should not be allowed.
LGTM for now -- still pushing forward for an upgrade for multiple topics. Note that this can be done without breaking backwards compatibility or losing efficiency: if there is a single topic we RLP-encode as a single element, and otherwise as a list. |
The next steps would involve an EIP for the "standard whisper payload format", which will specify stuff like flags, padding, supported encryption schemes, etc, and an EIP for the RPC API. These could be combined. |
@gluk256 can you expand on the changes to the payload format and why they were necessary? |
EIPS/eip-draft-whisper.md
Outdated
|
||
## Motivation | ||
|
||
It is necessary to specify the standard for Whipser messages in order to ensure forward compatibility of different Whisper clients. |
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.
Spelling: should be Whisper
instead of Whipser
.
EIPS/eip-draft-whisper.md
Outdated
**Status** [`0`] | ||
|
||
This packet contains two objects: integer (0x00). Might be followed by some arbitrary data in future versions, which should be gracefully ignored for future compatibility. | ||
This message should be send after the initial handshake and prior to any other 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.
Grammar: should be be sent
instead of be send
.
EIPS/eip-draft-whisper.md
Outdated
|
||
**Status** [`0`] | ||
|
||
This packet contains two objects: integer (0x00). Might be followed by some arbitrary data in future versions, which should be gracefully ignored for future compatibility. |
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.
The first sentence says this packet contains two objects. The second sentence reads like the second object might not exist. Does this packet always contains two objects, or sometimes just one object?
EIPS/eip-draft-whisper.md
Outdated
|
||
Blooms are formed by the bitwise OR operation on a number of bloomed topics. The bloom function takes the topic and projects them onto a 512-bit slice. At most, three bits are marked for each bloomed topic. | ||
|
||
The projection function is defined as a mapping from a a 4-byte slice S to a 512-bit slice D; for ease of explanation, S will dereference to bytes, whereas D will dereference to bits. |
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.
a
is duplicated in a a 4-byte slice
.
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.
thanks!
EIPS/eip-draft-whisper.md
Outdated
|
||
### Packet Codes | ||
|
||
The message codes reserved for Whisper protocol: 0 - 127. |
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.
What's the rationale behind reserving 128 codes?
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.
we need to decide upon some arbitrary number.
128 is as good as any other.
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.
@gluk256 this choice leads to integer overflow issues in certain circumstances (see openethereum/parity-ethereum#7567), so it'd be really great to figure out the motivation for such a wide space and if it is possible to reduce it.
This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:
If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR. In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks. Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:
|
No description provided.