[WIP] P2P rudimentary scaling #34
Conversation
Note that I moved extractPeerInfo() down to near the bottom of peer.go (because it's not exported)
|
|
||
| // Broadcast sends message to all peers this peer is currently connected to | ||
| func (p *Peer) Broadcast(m msg.Message) error { | ||
| return errors.New("Function not implemented") | ||
| } | ||
|
|
||
| // ExtractPeerInfo extracts the peer ID and multiaddress from the | ||
| // given multiaddress. | ||
| // Returns peer ID (esentially 46 character hash created by the peer) | ||
| // and the peer's multiaddress in the form /ip4/<peer IP>/tcp/<CumulusPort>. |
There was a problem hiding this comment.
I'm not really sure that this function is necessary. I feel like it's doing much more work than it has to.
message/message.go
Outdated
| // Send the multiaddress of a peer to another peer | ||
| PeerInfo = iota | ||
| // Request addressess of peers in the remote peer's subnet | ||
| RequestPeerInfo = iota |
There was a problem hiding this comment.
What is the difference between these two? When would we ever need only one peer's info
message/message.go
Outdated
| // ability of your peer to communicate with others. | ||
| const ( | ||
| // Send the multiaddress of a peer to another peer | ||
| PeerInfo = iota |
There was a problem hiding this comment.
You only need to write iota the first time
Like:
const (
a = iota
b
c
)
// a = 0, b = 1, c = 2
message/message.go
Outdated
| // Request chunk of the blockchain from peer | ||
| RequestChunk = iota | ||
| // Advertise that we have a chunk of the blockchain | ||
| AdvertiseChunk = iota |
There was a problem hiding this comment.
I think we should assume that every node has the whole blockchain. That should be the first thing they do.
message/message.go
Outdated
| // FromString parses a message in the form of a string and returns a pointer | ||
| // to a new Message struct made from the contents of the string. Returns error | ||
| // if string is malformed. | ||
| func FromString(s string) (*Message, error) { |
There was a problem hiding this comment.
Should this be FromBytes/Marshal instead? Why is it a string?
There was a problem hiding this comment.
Yea I think this should be Unmarshal and should take a []byte as an argument.
message/message.go
Outdated
|
|
||
| // Bytes returns JSON representation of message as a byte array, or error if | ||
| // message cannot be marshalled. | ||
| func (m *Message) Bytes() ([]byte, error) { |
peer/peer.go
Outdated
| log.Debug("\t", mAddrString) | ||
| message, err := msg.New([]byte(mAddrString), msg.PeerInfo) | ||
| if err != nil { | ||
| log.Error("Failed to create message") |
There was a problem hiding this comment.
Same as above. Use log.WithError(....) Also two instances below
peer/peer.go
Outdated
| // makeMultiaddr creates a Multiaddress from the given Multiaddress (of the form | ||
| // /ip4/<ip address>/tcp/<TCP port>) and the peer id (a hash) and turn them | ||
| // into one Multiaddress. Will return error if Multiaddress is invalid. | ||
| func makeMultiaddr(iAddr ma.Multiaddr, pid lpeer.ID) (ma.Multiaddr, error) { |
There was a problem hiding this comment.
Use NewMultiAddr for consistency
peer/peer.go
Outdated
| func (p *Peer) handleMessage(m msg.Message, s net.Stream) { | ||
| switch m.Type() { | ||
| case msg.RequestPeerInfo: | ||
| p.advertisePeers(s) |
There was a problem hiding this comment.
Why advertise to everyone when only one peer requests?
subnet/subnet.go
Outdated
| func (sn *Subnet) RemovePeer(mAddr ma.Multiaddr) { | ||
| log.Debugf("Removing peer %s from subnet", mAddr.String()) | ||
| delete(sn.peers, mAddr) | ||
| sn.numPeers-- |
There was a problem hiding this comment.
Don't separately keep track of numPeers, just use len(sn.peers). It's O(1)
There was a problem hiding this comment.
Wow yea that is just disgusting.
subnet/subnet.go
Outdated
|
|
||
| // isFull returns true if the number of peers in the sunbet is at or over the | ||
| // limit set for that subnet, otherwise returns false. | ||
| func (sn *Subnet) isFull() bool { |
peer/peer.go
Outdated
| // DefaultIP is the IP address new hosts will use if none if provided | ||
| DefaultIP = "127.0.0.1" | ||
| ) | ||
|
|
||
| // Peer is a cumulus Peer composed of a host | ||
| type Peer struct { | ||
| host.Host | ||
| subnet sn.Subnet |
There was a problem hiding this comment.
I thiiiiink we're using capital letters, see https://github.com/ubclaunchpad/cumulus/pull/23/files Subnet
There was a problem hiding this comment.
If it's capitalized its public, so I decided against it.
What changed:
messagepackagesubnetpackageerrorspackagePeerpackage