Skip to content
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.

Sign up for GitHub

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

feat: custom and store self agent version + store self protocol version #800

Merged
merged 2 commits into from
Nov 20, 2020
Merged

feat: custom and store self agent version + store self protocol version #800

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bc7c81
feat: custom and store self protocol and agent version
vasco-santos Nov 13, 2020
93d804e
fix: do not enable custom protocolVersion
vasco-santos Nov 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ Creates an instance of Libp2p.
| options.modules | [`Array<object>`](./CONFIGURATION.md#modules) | libp2p [modules](./CONFIGURATION.md#modules) to use |
| [options.addresses] | `{ listen: Array<string>, announce: Array<string>, noAnnounce: Array<string> }` | Addresses for transport listening and to advertise to the network |
| [options.config] | `object` | libp2p modules configuration and core configuration |
| [options.host] | `{agentVersion: string, protocolVersion: string}` | libp2p host options |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not inclined to allow setting protocolVersion. agentVersion is fine it has no impact, but any change to protocolVersion warrants a connection closure in the spec (although we dont enforce that here). We could allow it, but we'd need to be very clear about not changing it unless you're intentionally creating an isolated network. ie: "DO NOT change unless you know what you're doing"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense.

So, js-ipfs has supposedly protocolVersion assigned to 9000. But in theory, this is not used anywhere I think. I am not sure about the reasoning behind that number, but it would need to be changed to libp2p's default value. If so, we can drop the setting of protocolVersion unless a new use case appears

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go-IPFS reports protocolVersion as 'ipfs/0.1.0' so it should probably change to that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure why js-ipfs is trying to use that, but you are correct, it won't change anything we're actually sending over the wire. That should be corrected in js-ipfs.

Let's leave out being able to configure protocolVersion for now. We may add it in the future, but that's a can of worms I'd rather not open right now.

| [options.connectionManager] | [`object`](./CONFIGURATION.md#configuring-connection-manager) | libp2p Connection Manager [configuration](./CONFIGURATION.md#configuring-connection-manager) |
| [options.transportManager] | [`object`](./CONFIGURATION.md#configuring-transport-manager) | libp2p transport manager [configuration](./CONFIGURATION.md#configuring-transport-manager) |
| [options.datastore] | `object` | must implement [ipfs/interface-datastore](https://github.com/ipfs/interface-datastore) (in memory datastore will be used if not provided) |
Expand Down
8 changes: 8 additions & 0 deletions src/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ const mergeOptions = require('merge-options')
const { dnsaddrResolver } = require('multiaddr/src/resolvers')

const Constants = require('./constants')
const {
AGENT_VERSION,
PROTOCOL_VERSION
} = require('./identify/consts')

const { FaultTolerance } = require('./transport-manager')

Expand All @@ -27,6 +31,10 @@ const DefaultConfig = {
dnsaddr: dnsaddrResolver
}
},
host: {
agentVersion: AGENT_VERSION,
protocolVersion: PROTOCOL_VERSION
},
metrics: {
enabled: false
},
Expand Down
14 changes: 12 additions & 2 deletions src/identify/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,16 @@ class IdentifyService {
this._protocols = protocols

this.handleMessage = this.handleMessage.bind(this)

// Store self host metadata
this._host = {
agentVersion: AGENT_VERSION,
protocolVersion: PROTOCOL_VERSION,
...libp2p._options.host
}

this.peerStore.metadataBook.set(this.peerId, 'AgentVersion', uint8ArrayFromString(this._host.agentVersion))
this.peerStore.metadataBook.set(this.peerId, 'ProtocolVersion', uint8ArrayFromString(this._host.protocolVersion))
}

/**
Expand Down Expand Up @@ -246,8 +256,8 @@ class IdentifyService {
const signedPeerRecord = await this._getSelfPeerRecord()

const message = Message.encode({
protocolVersion: PROTOCOL_VERSION,
agentVersion: AGENT_VERSION,
protocolVersion: this._host.protocolVersion,
agentVersion: this._host.agentVersion,
publicKey,
listenAddrs: this._libp2p.multiaddrs.map((ma) => ma.bytes),
signedPeerRecord,
Expand Down
82 changes: 72 additions & 10 deletions test/identify/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ describe('Identify', () => {
peerId: localPeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: localPeer }),
multiaddrs: listenMaddrs
multiaddrs: listenMaddrs,
_options: { host: {} }
},
protocols
})
Expand All @@ -63,7 +64,8 @@ describe('Identify', () => {
peerId: remotePeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: remotePeer }),
multiaddrs: listenMaddrs
multiaddrs: listenMaddrs,
_options: { host: {} }
},
protocols
})
Expand Down Expand Up @@ -106,7 +108,8 @@ describe('Identify', () => {
peerId: localPeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: localPeer }),
multiaddrs: listenMaddrs
multiaddrs: listenMaddrs,
_options: { host: {} }
},
protocols
})
Expand All @@ -116,7 +119,8 @@ describe('Identify', () => {
peerId: remotePeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: remotePeer }),
multiaddrs: listenMaddrs
multiaddrs: listenMaddrs,
_options: { host: {} }
},
protocols
})
Expand Down Expand Up @@ -165,7 +169,8 @@ describe('Identify', () => {
peerId: localPeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: localPeer }),
multiaddrs: []
multiaddrs: [],
_options: { host: {} }
},
protocols
})
Expand All @@ -174,7 +179,8 @@ describe('Identify', () => {
peerId: remotePeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: remotePeer }),
multiaddrs: []
multiaddrs: [],
_options: { host: {} }
},
protocols
})
Expand All @@ -201,6 +207,38 @@ describe('Identify', () => {
.and.to.have.property('code', Errors.ERR_INVALID_PEER)
})

it('should store host data into metadataBook', () => {
const agentVersion = 'js-project/1.0.0'
const protocolVersion = '1000'
const peerStore = new PeerStore({ peerId: localPeer })

sinon.spy(peerStore.metadataBook, 'set')

new IdentifyService({ // eslint-disable-line no-new
libp2p: {
peerId: localPeer,
connectionManager: new EventEmitter(),
peerStore,
multiaddrs: listenMaddrs,
_options: {
host: {
agentVersion,
protocolVersion
}
}
},
protocols
})

expect(peerStore.metadataBook.set.callCount).to.eql(2)

const storedAgentVersion = peerStore.metadataBook.getValue(localPeer, 'AgentVersion')
const storedProtocolVersion = peerStore.metadataBook.getValue(localPeer, 'ProtocolVersion')

expect(agentVersion).to.eql(unit8ArrayToString(storedAgentVersion))
expect(protocolVersion).to.eql(unit8ArrayToString(storedProtocolVersion))
})

describe('push', () => {
it('should be able to push identify updates to another peer', async () => {
const connectionManager = new EventEmitter()
Expand All @@ -211,7 +249,8 @@ describe('Identify', () => {
peerId: localPeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: localPeer }),
multiaddrs: listenMaddrs
multiaddrs: listenMaddrs,
_options: { host: {} }
},
protocols: new Map([
[multicodecs.IDENTIFY],
Expand All @@ -224,7 +263,8 @@ describe('Identify', () => {
peerId: remotePeer,
connectionManager,
peerStore: new PeerStore({ peerId: remotePeer }),
multiaddrs: []
multiaddrs: [],
_options: { host: {} }
}
})

Expand Down Expand Up @@ -272,7 +312,8 @@ describe('Identify', () => {
peerId: localPeer,
connectionManager: new EventEmitter(),
peerStore: new PeerStore({ peerId: localPeer }),
multiaddrs: listenMaddrs
multiaddrs: listenMaddrs,
_options: { host: {} }
},
protocols: new Map([
[multicodecs.IDENTIFY],
Expand All @@ -285,7 +326,8 @@ describe('Identify', () => {
peerId: remotePeer,
connectionManager,
peerStore: new PeerStore({ peerId: remotePeer }),
multiaddrs: []
multiaddrs: [],
_options: { host: {} }
}
})

Expand Down Expand Up @@ -404,5 +446,25 @@ describe('Identify', () => {
// Verify the streams close
await pWaitFor(() => connection.streams.length === 0)
})

it('should store host data into metadataBook', () => {
const agentVersion = 'js-project/1.0.0'
const protocolVersion = '1000'

libp2p = new Libp2p({
...baseOptions,
peerId,
host: {
agentVersion,
protocolVersion
}
})

const storedAgentVersion = libp2p.peerStore.metadataBook.getValue(localPeer, 'AgentVersion')
const storedProtocolVersion = libp2p.peerStore.metadataBook.getValue(localPeer, 'ProtocolVersion')

expect(agentVersion).to.eql(unit8ArrayToString(storedAgentVersion))
expect(protocolVersion).to.eql(unit8ArrayToString(storedProtocolVersion))
})
})
})