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

Add specification for Portal JSON-RPC #345

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add specification for Portal JSON-RPC #345

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e115cc
Add specification for Portal JSON-RPC
KolbyML Oct 11, 2024
794fc1e
Update specification.md
KolbyML Oct 11, 2024
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
49 changes: 49 additions & 0 deletions jsonrpc/specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Portal JSON-RPC specification

The `*` is meant to be a placeholder to substitute different Portal sub-networks `history`, `state`, `beacon`, etc
Copy link
Member

Choose a reason for hiding this comment

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

something like <network-identifier> or <identifier> or <namespace> might be more visually clear.


## portal_*RoutingTableInfo

## portal_*AddEnr
- Adds the Enr to the respective sub-networks routing table
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rather an attempt to add to the routing table. It might fail according to the rules of the routing table implementation/configuration (e.g. bucket already full, ip limit reached, etc.).

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 will make this more clear


## portal_*GetEnr

## portal_*DeleteEnr

## portal_*LookupEnr

## portal_*Ping

## portal_*FindNodes

## portal_*FindContent
- must validate content

## portal_*Offer
- must not validate content
- shouldn't store data on offering node

## portal_*RecursiveFindNodes

## portal_*GetContent
- must validate content
- must check local storage first before doing recursive find content
- if recursive find content is done must do poke mechanism
Copy link
Contributor

Choose a reason for hiding this comment

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

Poke is currently not part of the state network. Not sure if this should be mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Poke is currently not part of the state network. Not sure if this should be mentioned.

good point I will remove it 🚀

Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression that we could indeed poke the content, since we are effectively building up a proof by walking the trie... Couldn't the node that is doing the trie walk push the content into the nodes that should have had it but didn't after retrieval?

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 was under the impression that we could indeed poke the content, since we are effectively building up a proof by walking the trie... Couldn't the node that is doing the trie walk push the content into the nodes that should have had it but didn't after retrieval?

You are right, so I will keep poke listed here

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually poke isn't possible in portal_stateGetContent because we don't have the proof needed in order to build an offer in this context. The higher level rpc endpoints such as eth_getBalance will have this proof from walking the trie but portal_stateGetContent doesn't have a proof parameter and may get called/used in other contexts where we are not walking the trie. Sure, you could in theory walk the trie for every bit a state content being looked up but that would be very slow due to the many network requests potentially required.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add optional proof field to the portal_stateGetContent. If provided, it can be used together with content obtained from the network to generate content that can be poked.

This would technically make get_content for State different then the one from History and Beacon. But it actually is not that different, because for Beacon and History, you don't need any additional data in order to create poke content item.

I didn't think extensively about it, but it feels like we would have similar need for Transaction and Verkle networks (when added). So State wouldn't be the only network that will need this.

Side note: I'm on the fence if the field should be optional or not. I'm not sure in what situation one would have content key, but not have a proof for it. But on the other hand, I don't want to make API too strict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we could add that parameter for state network then the poke would be possible. My thoughts are that it should be optional so that state content can still be fetched for other purposes such as debugging if and when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should make API decisions with debugging/testing use case in mind. If we need something specific for debugging, those can be added separately (e.g. portalDebug_stateGetContent) and they can be:

  1. documented to behave the same as the original request but skipping validation
  2. enabled/disabled with a flag

But, if we have legitimate use case when we are requesting data but don't already have proof available, then field should of course be optional. I'm just not sure if such use case exists

Copy link
Member

Choose a reason for hiding this comment

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

Since we sometimes can poke the content, we should just specify this as a SHOULD with that context so that in the event that the call if poke-able it should be poked, otherwise, if it's unanchored content, it shouldn't.

- if recursive find content is done and content meets storage criteria store content

## portal_*TraceGetContent
- must validate content
- must check local storage first before doing recursive find content
- if recursive find content is done must do poke mechanism
- if recursive find content is done and content meets storage criteria store content

## portal_*Store
- must not validate content
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, but perhaps you could still make it validate the type encoding?

I can see however that for this call you would want to avoid additional network requests.

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'm not sure, but perhaps you could still make it validate the type encoding?

Of course the client should validate correct type encodings I will make this more clear

I can see however that for this call you would want to avoid additional network requests.

This call should make 0 network requests

Copy link
Contributor

Choose a reason for hiding this comment

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

For the state network, the only part of the validation that needs a network request is looking up the block header to get the state root. If we skip the state root validation we can still perform all other validations such as decoding, checking the nodeHash matches node in proof, checking structure of the proofs etc and we now do this in Fluffy.

I agree that we shouldn't make network requests but I would suggest applying as many validations as possible to reduce the risk of storing bad data in the db, anything that can be done without a network request.


## portal_*LocalContent
- content stored locally is assumed already validated

## portal_*Gossip
- must not validate content
- shouldn't store data on gossiping node
Comment on lines +47 to +49
Copy link
Collaborator

Choose a reason for hiding this comment

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

At Fluffy we were thinking of having validation + storage on this call but not on the portal_*Offer considering that one seems lower level. But we are not sure about it ourselves, so works either way for us.

Copy link
Member Author

Choose a reason for hiding this comment

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

At Fluffy we were thinking of having validation

If someone gossips invalid data the node gossiped to should temporarily ban the gossiper.

I don't think you can validate everything without making network requests

Copy link
Contributor

Choose a reason for hiding this comment

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

In Fluffy we do local validations without doing network requests (currently just the state network). Sure, the node receiving the offer will validate and reject the data if it is bad but if you can prevent the bad data before sending then why not.

Loading