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.

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: address and proto books #590

Merged
merged 8 commits into from
Apr 9, 2020
Merged

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Mar 24, 2020

AddressBook and ProtoBook

This is the initial milestone regarding js-libp2p PeerStore improvements described on libp2p/js-libp2p#582

This PR restructures the initial version of peerStore, where we simply had a record of PeerInfos into data structures with different concerns and own logic.

We will start by introducing the AddressBook and ProtoBook since this was the content being used from the PeerInfos previously stored. In next PRs we will add other "books", namely the keybook and metadata book.

  • AddressBook implementation (initial version implementated)
  • ProtoBook implementation (initial version implementated)
  • PeerStore core
  • Change PeerStore usage to be compliant with the new implementation
  • Document PeerStore (initial version written)
  • Update docs/API.md
  • Remove peer-info from Ping (initial version implemented)

PeerStore usage

Change Peers data

  • identifyService

    • pushToPeerStore - identify push (on libp2p.handle, libp2p.unhandle)
      • iterate peerStore peers, can iterate on ProtoBook instead?
    • After identify, replace peer's data
    • After handle push message, replace peer's data
  • Upgrader: onConnection handler

    • Silent put to PeerStore
      • add to AddressBook
  • Discovery

    • add Peer with multiaddr to peerStore
  • DHT

    • Get/has operations
    • Put operations when receiving peers from other peers
      • DHT Message contains addresses to peers

Get Peers data

  • Registrar

    • topology needs to listen for protocol:change event and to iterate on all the known peers to check if they run the protocol
    • topology iterates on all the peers. if they support the protocol, they add it to the topology
  • dialProtocol

    • if received PeerInfo to dial (deprecate!)
  • Dialer

    • Uses peerStore.multiaddrsForPeer to get the multiaddrs for a PeerInfo
    • PeerInfo widely used internally, need to remove it and use peerId + addressBook content
  • getPeerInfo

    • Change with PeerInfo refactor?
  • ping

    • receives peerInfo and libp2p, but can simply receive peerId

Datastructure

The books datastructures where designed with consistency in mind among them and with their underlying data structure (Map). All should contain the main map methods set, get, has and delete, which should work in the same way as the map methods by default, but with some extra optional parameters to add additional super powers.

They were separated on their own instead of living on the top level peerStore for some reasons:

  • separation of concerns and logic
  • in the future some will need runtime pruning, such as multiaddr cleanning on ttls, scorings, ...

set(peerId, data, [options = { replace = true }]) : Array<data>

Set known data {Array<multiaddr>, Array<string>, ...} of a provided peer. By default, if previous data exists, it will replace it. If the replace options is explicitly defined to false, the data will be merged.

When the recorded data is inserted a new event should be triggered for the interested parties. If a record for the provided peer already exists and the new data is not relevant (i.e the final content will be the same) the operation just returns the final content, but no event is triggered for efficiency.

get(peerId) : Array<data>

Get known data of a provided peer. If the peer does not exist, it returns undefined.

has(peerId) : boolean

Do we have data for this peer?

delete(peerId, [data]) : boolean

Deletes the provided peer from the book. If data is provided, just remove the data

Returns true if the peer's data was fond and the data was deleted (or the partial data did not already exist). If data is modified or removed, the proper event change is triggered to the interest parties.

Concern: what if the array of data gets empty? should we remove the peer? I don't see any good reason to keep it!

General open questions

  • Who handles the emit peer?
    • When we known an address, so it should be when added to the AddressBook!

@vasco-santos
Copy link
Member Author

@jacobheun I made a first pass through the PeerStore improvements milestone 1.

This includes a brief explanation on the first post of the PR with PeerStore current usage and data structures decision. Moreover, it includes a README file with the PeerStore goals and a brief writing on its implementation and API.

I would say that for now, it would be great to have your feedback on the questions/decisions written on the PR and on the README (via PR review comments). You can also look into the structure and APIs for the AddressBook and ProtoBook but while we do not discuss what I previously mentioned things might need to be changed or completed.

src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/address-and-proto-books branch 9 times, most recently from 9f5f43a to 84a4c8a Compare March 31, 2020 11:53
@vasco-santos
Copy link
Member Author

@jacobheun this is now ready to review

It did not introduce any breaking change yet, as I left the older API of peerStore with adapters for the new addressBook and protoBook, since other modules (like DHT, ...) are still using them. Anyway, all the internal usage of the peerStore, including the tests now use the new API.

I will do a follow up PR with the breaking change for the peerStore API with the PRs of the modules using it. This will precede the breaking change PRs to remove peer-info usages from internal code and API

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Havent gone through everything yet, but sharing the review early as there is enough to go on while I go through the rest.

doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
src/identify/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/peer-store/README.md Outdated Show resolved Hide resolved
Libp2p's Peerstore is responsible for keeping an updated register with the relevant information of the known peers. It should gather environment changes, be able to take decisions and notice interested parties of relevant changes. The Peerstore comprises four main components: `addressBook`, `keyBook`, `protocolBook` and `metadataBook`. These book components have similar characteristics with the `Javascript Map` implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mention the Map piece. It's not compliant with a Map and we can't iterate on them, so I would avoid comparing the two.

It should gather environment changes, be able to take decisions and notice interested parties of relevant changes.

Can you clarify this? It's sounds like the PeerStore tries to be smart ("decisions"), it should not be smart. It should be the single source of truth for all peer data.

  • If a subsystem learns about peer data it needs to go directly to the peer store.
  • If any system/application/person wants to know about peers or listen for updates, they need to go to the peerstore.

The PeerStore manages the high level operations on its inner books. Moreover, the peerStore should be responsible for notifying interested parties of relevant events, through its Event Emitter.

(Future considerations: Peerstore should manage a job runner to trigger books runners for data trimming or computations)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't provide enough information for future devs (including us). We should make a dedicated Future considerations header and be a bit more descriptive about what the PeerStore might wish to do and why. What kind of data trimmings? What computations? These can be concise, but should be clear. For example:

## Future Considerations
- If multiaddr TTLs are added, the PeerStore may schedule jobs to delete all addresses that exceed the TTL to prevent AddressBook bloating

@vasco-santos vasco-santos force-pushed the feat/address-and-proto-books branch 2 times, most recently from d645b63 to b06040b Compare April 2, 2020 09:08
doc/API.md Outdated

Find the stored information of a given peer.

`peerStore.find(peerId)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just call this peerStore.get(peerId)? I'm worried users might misinterpret this as a network search for the peer.

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, I agree! The question is, should I do it now, or in the next PR? (this will break the return value)

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Some additional things.

I was thinking it might be helpful to add some tests that exercise some user scenarios. Such as:

  • I want to find all peers who support protocol x.
  • Given an address with no peer id, I want to find all peers that have this address.

This might help us catch any usability issues up front.

get (peerId) {
if (!PeerId.isPeerId(peerId)) {
throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can skip the type checks of peerId in these methods. We're documenting everything, so I feel like these are unnecessary restrictions.

Copy link
Member Author

@vasco-santos vasco-santos Apr 2, 2020

Choose a reason for hiding this comment

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

I personally don't like to see cannot access toString() of undefined as an error, as someone needs to go into the code/docs and see what is the issue. This way, I think the error is useful here. if we do not try to access properties of it, I would agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can table this discussion for outside of this PR as we're already doing similar checks elsewhere. In short, I'd rather see us move to support static type checking for users that want it instead of doing runtime checks. While they're not big, doing these type checks everywhere slowly adds to bundle size as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we open a general issue to discuss this?

throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
}

const rec = this.data.get(peerId.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

Use PeerId.toB58String everywhere, until we do a targeted switch to base32, otherwise we risk hitting unexpected behavior.

Suggested change
const rec = this.data.get(peerId.toString())
const rec = this.data.get(peerId.toB58String())

Comment on lines 27 to 28
* @property {number} validity NOT USED YET
* @property {number} confidence NOT USED YET
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just remove these until they're in place.

Suggested change
* @property {number} validity NOT USED YET
* @property {number} confidence NOT USED YET

* "peer" - emitted when a peer is discovered by the node.
* "change:multiaddrs" - emitted when the known multiaddrs of a peer change.
*/
this._ps = peerStore
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be set in the Book base class?

throw errcode(new Error('addresses must be provided'), ERR_INVALID_PARAMETERS)
}

if (!Array.isArray(addresses)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just simplify the API now and require set be provided an array? We could avoid these checks, and just let addresses.forEach throw the error if it's not an array.

// Add recorded uniquely to the new array (Union)
rec && rec.forEach((mi) => {
if (!multiaddrInfos.find(r => r.multiaddr.equals(mi.multiaddr))) {
multiaddrInfos.push(mi)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably avoid mutating multiaddrInfos and rec here. I'd suggest composing a new array for the union and setting that as the new record. This would help prevent accidental modification of the underlying records.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how this was before I moving code around to get set and add. However, now the multiaddrInfos is created by this._toMultiaddrInfos() function and it is "pushed" with data from the current record, and then set as the new data and not returned.

So, the only possible scenario of second effects here that I can see is if someone did get before and then changes the data after the add. So, I would say that for being safe we only need to destruct the record m?

Copy link
Member Author

Choose a reason for hiding this comment

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

Other scenario is if someone changes the multiaddr provided! So yes, it might be better to create a new array

Copy link
Member Author

Choose a reason for hiding this comment

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

but if we just create a new array, I think we will not solve any of these scenarios

@vasco-santos vasco-santos force-pushed the feat/address-and-proto-books branch 2 times, most recently from f521893 to 30b0ae9 Compare April 2, 2020 17:05
Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
@jacobheun jacobheun changed the base branch from master to 0.28.x April 9, 2020 10:22
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Just some minor things and API consistency changes. Also added some things to think about for followup PRs.

FYI: I updated the base branch of the PR to be 0.28.x instead of master

doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
src/peer-store/address-book.js Outdated Show resolved Hide resolved
src/peer-store/address-book.js Outdated Show resolved Hide resolved
src/peer-store/proto-book.js Outdated Show resolved Hide resolved
src/peer-store/proto-book.js Outdated Show resolved Hide resolved
if (!PeerInfo.isPeerInfo(peerInfo)) {
throw errcode(new Error('peerInfo must be an instance of peer-info'), ERR_INVALID_PARAMETERS)
}
// TODO: This is not a `peer-info` instance anymore, but an object with the data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps to avoid confusion, when we get rid of PeerInfo we should start calling variables peerData?

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, I was having a hard time with this naming confusion. Good suggestion! 👍

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jacobheun jacobheun merged commit c99a7a2 into 0.28.x Apr 9, 2020
@jacobheun jacobheun deleted the feat/address-and-proto-books branch April 9, 2020 14:07
@vasco-santos vasco-santos restored the feat/address-and-proto-books branch April 9, 2020 18:36
vasco-santos added a commit that referenced this pull request Apr 14, 2020
* feat: address and proto books

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: minor fixes and initial tests added

* chore: integrate new peer-store with code using adapters for other modules

* chore: do not use peerstore.put on get-peer-info

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: add new peer store tests

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
@vasco-santos vasco-santos mentioned this pull request Apr 27, 2020
2 tasks
vasco-santos added a commit that referenced this pull request May 1, 2020
* feat: address and proto books

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: minor fixes and initial tests added

* chore: integrate new peer-store with code using adapters for other modules

* chore: do not use peerstore.put on get-peer-info

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: add new peer store tests

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
@vasco-santos vasco-santos mentioned this pull request May 4, 2020
1 task
jacobheun added a commit that referenced this pull request May 28, 2020
* feat: address and proto books

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: minor fixes and initial tests added

* chore: integrate new peer-store with code using adapters for other modules

* chore: do not use peerstore.put on get-peer-info

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: add new peer store tests

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
jacobheun added a commit that referenced this pull request May 28, 2020
* feat: address and proto books

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: minor fixes and initial tests added

* chore: integrate new peer-store with code using adapters for other modules

* chore: do not use peerstore.put on get-peer-info

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* chore: add new peer store tests

* chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
@achingbrain achingbrain deleted the feat/address-and-proto-books branch May 18, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants