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

Preliminary Work #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MercuricChloride
Copy link
Collaborator

I am creating this PR to serve as an area to get feedback. Right now it's not clean and will evolve, but I reached some basic structure with primitive multi-address parsing, and host connectivity.

@vyzo vyzo self-requested a review August 3, 2024 13:59
@vyzo
Copy link
Contributor

vyzo commented Aug 3, 2024

can you do a rebase on master?

@vyzo
Copy link
Contributor

vyzo commented Aug 4, 2024

Also, you can push your branch directly in the repo! This is actually preferable as other gerbils can make edits etc.

(import :std/build-script
:std/misc/process)

(invoke "gxtags" ["."])
Copy link
Contributor

Choose a reason for hiding this comment

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

this is generally unnecessary with gerbil pkg; you can just tag the package once you've linked it.

libp2p/host.ss Outdated
Comment on lines 23 to 30
(defmethod {init! p2p-host}
(lambda (self (key (keygen/ed25519)) (multi-addr "ip4/127.0.0.1/tcp/4444"))
(set! (@ self multi-address) (multi-address multi-addr))
(set! (@ self peer-store) (make-hash-table))
(set! (@ self handlers) (make-hash-table))
(set! (@ self connections) (make-hash-table))
;; TODO Make this a proper implementation
(set! (@ self id) (make-peer-id key))))
Copy link
Contributor

Choose a reason for hiding this comment

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

so with typed gerbil (master), you get an implicit (self ::- YourClass) annotation and you an use dotted access for slots; not only this results in significantly faster code, it is also statically checked at expansion time.

So:

(set! self.multi-address (multi-address multi-addr))
(set! self.peer-store ...)
...

libp2p/host.ss Outdated
;; TODO Make this a proper implementation
(set! (@ self id) (make-peer-id key))))

(def (tcp-listener address (proc 123))
Copy link
Contributor

Choose a reason for hiding this comment

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

use type annotations here for the arguments, as the can be checked at runtime for you at the call boundary, and often times be eliminated (the checks) when the compiler knows the type.

libp2p/host.ss Outdated
Comment on lines 33 to 45
(let* ((addr (get-tcp-addr address))
(sock (tcp-listen addr)))
(while #t
(try
(let (cli (ServerSocket-accept sock))
(when cli
(println (format "~a" cli))
(println (format "Accepted connection from ~a" (StreamSocket-peer-address cli)))
;;(spawn proc cli)
))
(catch (e)
(errorf "Error accepting connection: ~a" e))))))

Copy link
Contributor

Choose a reason for hiding this comment

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

use the dots, luke!

(using (sock (tcp-listen addr) :- Serverocket)
   (while #t
      (using (cli (sock.accept) :- StreamSoscket)
         ...
         ... cli.peer-address ...)))

Copy link
Contributor

Choose a reason for hiding this comment

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

Also of note, you need to allow an exception to exit the loop if the socket is closed by another thread -- in fact you should make the socket closable by putting it in the host -- so that you can cleanly exit.

libp2p/host.ss Outdated
Comment on lines 46 to 52
(defmethod {listen! p2p-host}
(lambda (self)
(def addr (@ self multi-address))
(match addr
((? tcp-ip?) (set! (@ self listener)
(spawn tcp-listener addr))))))

Copy link
Contributor

Choose a reason for hiding this comment

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

again here, the implicit type declaration for self:

... self.multi-address 
... self.listener

libp2p/host.ss Outdated
Comment on lines 53 to 55
(defmethod {set-protocol-handler! p2p-host}
(lambda (self protocol handler)
(hash-put! (@ self handlers) protocol handler)))
Copy link
Contributor

Choose a reason for hiding this comment

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

self.handlers

Copy link
Contributor

Choose a reason for hiding this comment

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

and if you declare in the struct/class def that handlers is a HashTable, then you can have faster hash-put that ellides the type check!

(lambda (self protocol handler)
(hash-put! (@ self handlers) protocol handler)))

(defmethod {connect! p2p-host}
Copy link
Contributor

Choose a reason for hiding this comment

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

here too

(interface (Host Closer)
(ID) => PeerID

(PeerStore) => PeerStore
Copy link
Contributor

Choose a reason for hiding this comment

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

lets keep this small caps -- peer-store or something?

(and (list? obj)
(andmap string? obj))))
;; Removal of protocol handlers
(remove-protocol-handler! (pid :~ string?))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is general something that is very rarely done -- I don't think we even allow it in go-libp2p, so probably not worth the trouble.

(defstruct multi-address (protocols str)
constructor: init!)

(defmethod {init! multi-address}
Copy link
Contributor

Choose a reason for hiding this comment

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

it is also very useful to initialize from binary input (a u8vector), this is very common pattern that we need to support.

Copy link
Contributor

Choose a reason for hiding this comment

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

My general recommendation would be to store the binary representation (as a u8vector) and a parsed version that is a alist/plist of protocols and protocol values.
We can also cache the string rep for printing...

Actually I think it is better to have the binary rep, string rep, and list rep. The primary one is the list rep, that's what we should work with. And we can initialize either from string or binary rep, and generate the other one when needed (say to print or to send over the network).

;;; -*- Gerbil -*-
;;; -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the modline intact!

Comment on lines +56 to +57
(defgeneric connect
(lambda args #f))
Copy link
Contributor

Choose a reason for hiding this comment

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

i am not convinced this should be a generic, all you take is a multiaddr, and dispatch based on this.

Much better to implement as a method in the host, which maintains a mapping of (multiaddr) proto -> dialer

@@ -0,0 +1,10 @@
#lang :std/protobuf/proto
Copy link
Contributor

Choose a reason for hiding this comment

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

let's call the libp2p/protobuf package libp2p/pb? This is closer to go conventions and I'd like to follow them when it makes sense to do so.

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