-
Notifications
You must be signed in to change notification settings - Fork 111
Conversation
…ield, add marshalling and unmarshalling functions for trojanMessage, add TestTrojanMessageSerialization func
…ed on new design, add TODOs
…ing-trojan-chunks
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.
There a couple of minor things I left, I think this I really good implementation.
Awesome job.
// Wrap creates a new trojan chunk for the given targets and trojan message | ||
// a trojan chunk is a content-addressed chunk made up of span, a nonce, and a payload | ||
// TODO: discuss if instead of receiving a trojan message, we should receive a byte slice as payload | ||
func (m *Message) Wrap(targets [][]byte) (chunk.Chunk, error) { |
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.
targets [][]byte
could be converted into a type ex. type Targets [][]byte
The same argument should be used for Send so exported type would be necessary.
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 honestly don't feel strongly either way.
if it's beneficial to us, then yeah, let's add a Targets
type. i will keep this in mind when we merge this with the API code
pss/trojan/message_test.go
Outdated
|
||
payload := c.Data() | ||
payloadSize := len(payload) | ||
expectedSize := chunk.DefaultSize + 8 // payload + span |
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.
Span could be a constant so it's parametrized in all references
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're starting to convince me...
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.
open to more opinions
pss/trojan/message_test.go
Outdated
t.Fatalf("trojan chunk payload has an unexpected size of %d rather than %d", payloadSize, expectedSize) | ||
} | ||
|
||
span := binary.BigEndian.Uint64(payload[:8]) |
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.
same here with span
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.
addressed in previous 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.
The changes I described are not critical, they can be address if needed later.
LGTM
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.
really just a few minor things
already approved
also https://travis-ci.org/github/ethersphere/swarm/jobs/672786246#L489
linter indicates another solution
pss/trojan/message.go
Outdated
} | ||
|
||
// toChunk finds a nonce so that when the given trojan chunk fields are hashed, the result will fall in the neighbourhood of one of the given targets | ||
// this is done by iterating the BMT hash of the serialization of the trojan chunk fields until the desired nonce is found |
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 done by iteratively enumerating different nonces until the BMT hash of the serialization of the trojan chunk fields results in chunk address that has one of the targets as its prefix
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.
done
thank you @zelig for the relentless reviewing ❤️ fixed the linter issues as well. just have a couple of very minor issues that i'd like to resolve before merging and we're good to go |
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.
<3
EXPERIMENTAL
(will not merge to
master
)This PR adds implements trojan chunks.
Related issue: #2153
Implementation overview
new package
trojan
new types
Topic
Message
, contains:length
(2 byte array)topic
(of theTopic
type)payload
andpadding
(both of them byte slices)main new functions
NewTopic
helper function constructs aTopic
by hashing an inputstring
newMessage
creates aMessage
based on the givenTopic
plus apayload
(byte slice)length
andpadding
fields based on the given paramsWrap
function takestargets
(slice of byte slices) plus aMessage
type, and returns achunk.Chunk
. this function is in charge of:targets
are validspan
nonce
so that when it is hashed along with thespan
and serializedMessage
, the resulting hash has a prefix which matches one of thetargets
Chunk
with the matchinghash
set as its address, plus the aforementioned serialized fields set as its payloadOpen questions
is it OK to callcrypto.Keccack256Hash
to hash a string to create a topic, or must we usecrypto.Keccack256
?please use the exact same hash as used everywhere else. no new variant to be introduced here.
is it OK to have aMessageg
var as a param for theWrap
func, rather than a byte slice (payload)? if a payload were received, this would mean that param is the serialization of the trojan message, which would then have to be de-serialized to find a nonceperfectly. ok imho
should thespan
field be encoded in little-endian or big-endian?let's try to be consistent with other parts of the codebase. so i guess little?done
why do we need to callReset
on astorage.SwarmHash
var?you do not need to call it if it is freshly created hasher
since we don't have a trojan chunk struct, we are relying on the helper funcserializeTrojanChunk
to serialize the trojan chunk fields (rather than a binary marshaller). don't we need adeserializeTrojanChunk
func too? this should be used on the receiving node to figure out whether the received chunk payload contains a serializedtrojanMsg
serializeTrojanChunk
function now removed as it was unnecessaryshouldMessage
'sUnmarshalBinary
function also:6.1 check if the received byte slice is longer than
4064
bytes? (and return an error if so)6.2 ignore any bytes in the slice past byte #4064?
6.3 check that for decoded
length
l
, it follows that0 ≤ l ≤ 4030
?none of them needed, whenever deserialise, you get the data from a chunk which is itself validated for payload length, in fact all restrictions enforced here just enforce the restrictions on content addressed chunks
TODOs
make this work withstorage.BMTHash
rather thanstorage.SHA3Hash
(currently, BMT hash does not change with each iteration loop; not sure why)test that if the random nonce overflows, the iteration still worksadd failure tests