-
-
Notifications
You must be signed in to change notification settings - Fork 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
New net interface #69
Conversation
warning: not yet wired. |
return false | ||
} | ||
return bool(r[0]&0x80 == 0x80) | ||
} |
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.
@perfmode @whyrusleeping heads up -- Request
uses the per-msg RequestID
nonce to also identify whether it's a request or a response (based on the first bit). (lmk if you think this is too sketchy)
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.
When can r == nil
?
My concern would be around future-proofing. Will this representation be resistant to errors introduced by updates and versioning incompatibilities?
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.
Ehhh... im going to lean towards 'sketchy'. Like, yeah it works, but it should be commented to hell and back if were going to keep 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.
@perfmode r == nil
when messages do not want a RequestID associated -- i.e. they're not requests.
@whyrusleeping Yeah, it's sketchy. It feels so wrong to use a whole protobuf for a bit thought >.< -- i guess it is technically TRTTD.
// speaking to (i.e. peer.ID). In terms of moving around simple addresses | ||
// -- instead of an (ID, Addr) pair -- we can use: | ||
// | ||
// /ip4/10.20.30.40/tcp/1234/ipfs/Qxhxxchxzcncxnzcnxzcxzm |
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 we have some sort of extra functionality added to multiaddr? Maybe add pluggable protocols so ipfs can specify the 'ipfs peer' protocol which denotes an id that is expeced from the connection?
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.
yeah, sounds good. we can allocate a multiaddr number and all.
think b58 is good enough to have multiaddr default to for protocols whose string representations it doesn't know? could do hex. Oh damnit b58 will run into trouble on case sensitive OSes....
609a71b
to
5fafaa4
Compare
|
||
// add all the peers we got first. | ||
for _, p := range peers { | ||
r.addPeerToQuery(p, nil) // don't have access to self 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.
Hrm... having a reference to outself here might be useful. I had issues without it before.
@jbenet Recall services register handlers on the network. These handlers are called when inbound messages arrive.
What does it mean for the handler to return an error to the network layer? |
@perfmode good point, that doesn't make much sense. See https://github.com/jbenet/go-ipfs/blob/net/net/service/service.go#L183-L188 Think it should just return a message, and handle/log/whatever its own error? |
@jbenet @whyrusleeping merged this into net already, but still want to make sure you're aware. |
true -> always send to peer false -> use ledger-based strategy described in IPFS paper draft 3
failing due to nil pointer dereference
AHHHHHHHHHHHHHHH!!!! |
The smile on my face right now. |
|
@jbenet o.o that changelist.... |
update libp2p to 4.4.3
No description provided.