Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

enhancement/mailserver-api #116

Closed
wants to merge 10 commits into from
Closed

Conversation

decanus
Copy link
Contributor

@decanus decanus commented Apr 27, 2020

This PR introduces a new REST API for interacting with mailservers as documented in vacp2p/research#24.

This PR mainly serves for discussion and is no where near final. There are certain things that still need to be discussed.

  1. Do we fully deprecate the RLP api?
  2. How would a node advertise on which port the API is available, or should it be either define that it must be accessible on 80, 8080 or 443?
  3. Should we add more endpoints, eg. retrieving a single message?

Todos

  • Define Protobuf RPC interface
  • Update pagination to cursor

- **from [int]** - `required` - UNIX time in seconds; oldest requested envelope's creation time.
- **to [int]** - `required` - UNIX time in seconds; newest requested envelope's creation time.
- **bloom [string]** - `optional` - array of Waku topics encoded in a bloom filter to filter envelopes.
- **limit [int]** - `required default: 100` - The maximum amount of envelopes to return.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we lift the required if we have a default?

- **to [int]** - `required` - UNIX time in seconds; newest requested envelope's creation time.
- **bloom [string]** - `optional` - array of Waku topics encoded in a bloom filter to filter envelopes.
- **limit [int]** - `required default: 100` - The maximum amount of envelopes to return.
- **page [int]** - `optional` `default: 0` - The page to return.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we want likely to use cursor based pagination instead of page based, as page based is not stable.

Essentially mailserver will return a cursor(string) which is returned in the response, and used to fetch the next "page"

Copy link
Contributor

Choose a reason for hiding this comment

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

https://jsonapi.org/profiles/ethanresnick/cursor-pagination/ for reference, the current mailserver implementation already uses a cursor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, thanks @cammellos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed @cammellos

"nonce": "0x0123",
"data": "0x129232138ae123",
}],
"page": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably we would want it to be a cursor instead. Other api's offer a next and previous URL, which is also neat, but we currently only fetch using previous, so maybe not needed for now.

"data": "0x129232138ae123",
}],
"page": 0,
"count": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need count (although it does not harm as there's no performance issue), nor pages (pages might have to be calculated, and it might be expensive depending on implementation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@oskarth
Copy link
Contributor

oskarth commented Apr 28, 2020

Just thinking out loud here - does it have to be a REST API? Not clear to me that resources abstraction map neatly. E.g. we only do GET here, no POST or UPDATE afaict. It also restricts us to HTTP.

Would gRPC or JSON RPC (over HTTPs to start with, but extensible to e.g. WS and some direct kad p2p in future) be interesting?

The main thing we want here AFAIK is request-response.

@decanus
Copy link
Contributor Author

decanus commented Apr 28, 2020

The problem with gRPC, from what I hear is that its browser support is trash. I like the idea of thinking whether it needs to be rest @oskarth, my rationale for it was that it seems like a good radical change for getting away from json rpc. I would not be opposed to moving entirely away from JSON RPC in the distant future.

What I will define is a protobuf service, that way we can abstract away the transport and serialization too and maybe add support for multiple?

@oskarth
Copy link
Contributor

oskarth commented Apr 28, 2020

Relevant discussion ethereum/consensus-specs#1012

@decanus
Copy link
Contributor Author

decanus commented Apr 28, 2020

So as discussed with @oskarth, the play here seems to be that we should firstly define domain objects using protocol buffers along with the service. Once that is done we can start a discussion on the transport looking at various trade-offs, such as privacy, interaction with current tech, ease of implementation.

Copy link
Contributor

@kdeme kdeme left a comment

Choose a reason for hiding this comment

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

The way that I understood this issue is that the main problem is that the current mail server specification does not make use of the request/response mechanism (following packet ids) in the waku protocol.

We can extend it with an HTTP interface also if there is a need for this, but I'd like to hear the reasoning of why there is the need. Sorry if I missed this in earlier convo.

If we add this HTTP (REST) interface, I agree that ideally the definition of the objects is (or becomes) the same for this HTTP REST interface, and the existing Waku one (which will then requires changes probably). Serialisation will/can of course be different.

@cammellos
Copy link
Contributor

@kdeme status-im/specs#54 (comment) specifically for HTTPS.
I think https helps with a few things for sure, but as mentioned I don't feel strongly about whether we should use it or not, RLP is probably fine.
Req/Response is what will help us the most from a client perspective (we don't have to keep track of which envelopes are being sent and when they arrive, etc so the client can immediately act on the response and show informations to the user etc).

@oskarth
Copy link
Contributor

oskarth commented Apr 29, 2020

What would be the minimal change for us to get request-response? I'm a bit worried that if we start to get too much into the comfy REST/HTTP world it'll be hard to get rid of it, and we'll start to rely increasingly on cluster, DNS, load balancing, etc.

@cammellos
Copy link
Contributor

What would be the minimal change for us to get request-response?
It depends :)

Essentially the main problem is the maximum message size, which is the reason why they are sending them separate from the response (we split envelopes in batches, and we fit as many in the batch as we can).

The problem boils down to, "how do you send an envelope where size(envelope) == maxMessagesize?". The only way to do that is to send the envelope without any metadata, if you want to respect the maxMessageSize (which is checked before doing any parsing, rightly so as it might choke the parser an oversized message).
Accidentally I don't think our code actually solves this issue, but that needs testing. If an envelope is as big as the maximum allowed packet I don't think it will ever be able to leave the mailserver as we attach some metadata (we could fix this by allowing some extra allowance for p2p peers, to include metadata).

Without increasing the max message size (which is probably not a good idea) we won't be able to have proper req/response, and you will either have to piece things up on one end (which we don't do currently, but we could at the expenses of complexity), or get into something of a more concerted way to send envelopes when syncing between two peers (so one peer will have expectations about what the next message should look like once a request is made) .

HTTP does not have this problem as it's polling rather than pushing, so lots of complications go away, but it's true that it's quite a change.

From purely a core perspective, this only matters once we have bandwidth to implement it, and currently mailserver is probably not the highest priority as there's not much appetite in changing things as it mostly works. This would give us some time to sit on the decision for a bit longer.
Hard to say.

@decanus
Copy link
Contributor Author

decanus commented Apr 29, 2020

@oskarth, I see your point. But the way I say it the mailserver is a construction we want to eliminate later anyway. We might as well make this a REST based api and ensure that later storage protocols etc interact nicely without the need for such an API. Does that make sense?

@kdeme
Copy link
Contributor

kdeme commented Apr 29, 2020

What would be the minimal change for us to get request-response? I'm a bit worried that if we start to get too much into the comfy REST/HTTP world it'll be hard to get rid of it, and we'll start to rely increasingly on cluster, DNS, load balancing, etc.

Yes, I have the same concern which is why I asked why we really need to go this route. Sure, a lot of complications go away with HTTP, but that is not the point.

Regarding the maxMessageSize issue. We actually don't have a maxMessageSize defined in the specification, although both code bases do have it. In the nim-eth code it is used to limit the size (rlp encoded size) of the envelope, not the total size of the packet send. So there would be room for meta-data.
It does depend however also on the maximum size of rlpx, but in nim-eth this is set 10 times bigger currently. If it would be set the same, you could have issues.

Regarding the specification here, can't we just go with ABNF specification? And then we can keep RLP in the current RLPx solution. And move to something else, e.g. protobuf, whenever we decide to move to libp2p or any other tech.

@kdeme
Copy link
Contributor

kdeme commented Apr 29, 2020

But the way I say it the mailserver is a construction we want to eliminate later anyway. We might as well make this a REST based api and ensure that later storage protocols etc interact nicely without the need for such an API. Does that make sense?

You have a point. The mailserver is already a centralized concept so you could say, lets go all the way then.
I guess it boils down to the work/effort this change would take and whether it is worth the benefits.

From the perspective of the Nim implementation, I don't think we will be able to do the effort to support the HTTP solution, as we don't have a proper HTTP(S) server implementation. But I guess that is fine as we don't have the current mail server now anyhow, and no real reason to need one. Building the Nim mailserver as it is defined now is actually also rather low in priority list (if it is even on it :) ).

So, I'll leave it to the status-go team to decide the best way forward.

@decanus
Copy link
Contributor Author

decanus commented May 14, 2020

10 times bigger currently

@kdeme it seems like it may make sense that a client can determine their message says and send to a peer what message size they want?

@decanus decanus mentioned this pull request May 15, 2020
@oskarth
Copy link
Contributor

oskarth commented Jul 9, 2020

Seems outdated and better taking lessons from here and applying it to v2. Close?

@decanus
Copy link
Contributor Author

decanus commented Jul 9, 2020

@oskarth yea

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants