-
Notifications
You must be signed in to change notification settings - Fork 165
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
Common Node API #1027
base: master
Are you sure you want to change the base?
Common Node API #1027
Conversation
@filecoin-project/lotus-maintainers 👀 plz |
IMO, this should go under the FRC section instead of the FIP section as it's not a consensus critical change (and therefore doesn't have to go through the entire FIP process). |
- State Queries | ||
- These primarily concern the chain state, actor state or network (p2p) state. They are read-only methods that are necessary to expose all elements of global state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we have a pretty wide API here. Maybe a bit of a bikeshed, but I've always wanted to move some of the state-query logic into read-only methods on the actors themselves so state query operations simply become method implementations (simplifying client implementations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion!
Another thought we had is whether a generic state query method could be sufficient. Having a more direct relationship between what the actors return and the RPC responses would make this more feasible.
FIPS/fip-common_node_api.md
Outdated
Node implementers must include all specified methods in their API without any modifications. Implementers may choose to provide additional methods beyond what is included in this specification. The endpoint used to serve RPC requests must be versioned acccording the majo | ||
|
||
## Subscription methods | ||
The `ChainNotify` method is not a simple request/response, but rather a bidirectional exchange of JSON-RPC-like messages as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: F3 will also introduce some form of FinalizedChainNotify()
method which should be significantly simpler...
We should also consider:
- Events (which currently use a different API).
- Messages (there's actually no great "native" API for watching for messages).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment Steven.
We're currently rewriting this section if you'd like to sneak a preview: ChainSafe#2
Could you clarify what the Events
and Messages
APIs you're talking about are, or maybe point me to some docs? I've not heard of them before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/filecoin-project/lotus/blob/f8c5b737efa10b84c649ae2471030d691902e688/api/api_full.go#L197 This is basically Filecoin’s equivalent of eth_getlog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Messages are the ones started with Mpool
I am supportive of the motivation and direction of this proposal. Will give a more thorough review once the methods list are proposed |
FWIW, I think this is a great idea. |
FIPS/fip-common_node_api.md
Outdated
{ "jsonrpc": "2.0", "method": "xrpc.ch.val", params: [2, {}] } | ||
``` | ||
4. Caller sends a JSON-RPC request to cancel the subscription, with their Channel ID | ||
```json | ||
{ "jsonrpc": "2.0", "id": 1, "method": "xrpc.cancel", params: [2] } | ||
``` | ||
5. Callee responds with a JSON-RPC notification of cancellation | ||
```json | ||
{ "jsonrpc": "2.0", "id": 1, "method": "xrpc.ch.close", params: [2] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ "jsonrpc": "2.0", "method": "xrpc.ch.val", params: [2, {}] } | |
``` | |
4. Caller sends a JSON-RPC request to cancel the subscription, with their Channel ID | |
```json | |
{ "jsonrpc": "2.0", "id": 1, "method": "xrpc.cancel", params: [2] } | |
``` | |
5. Callee responds with a JSON-RPC notification of cancellation | |
```json | |
{ "jsonrpc": "2.0", "id": 1, "method": "xrpc.ch.close", params: [2] } | |
{ "jsonrpc": "2.0", "method": "xrpc.ch.val", "params": [2, {}] } | |
``` | |
4. Caller sends a JSON-RPC request to cancel the subscription, with their Channel ID | |
```json | |
{ "jsonrpc": "2.0", "id": 1, "method": "xrpc.cancel", "params": [2] } | |
``` | |
5. Callee responds with a JSON-RPC notification of cancellation | |
```json | |
{ "jsonrpc": "2.0", "id": 1, "method": "xrpc.ch.close", "params": [2] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! We're actually reewriting this section - you can peep ChainSafe#2 to see the current progress :)
@Stebalien We chose to be a technical FIP based on this guidance: Line 30 in 020bcb4
Happy to move to FRC if that fits better - maybe the above could be amended? |
We have a working doc with all the methods here: A few methods we're still unsure of, but we've taken a stance of most of them. Rationale is included for those that are excluded, and those that are included should fit into one of the categories we describe |
* mark: aatifsyed/common-node-api * feat: describe subscriptions as a bunch of OpenRPC fragments * fix: link to lotus issue
…/rpc-api # Conflicts: # README.md
Co-authored-by: Jiaying Wang <[email protected]>
Since GitHub didn't backlink it, the accompanying discussion for this PR is #1032 |
* doc: remove todos * doc: note on URLs * review: todos * review: v1 only
| StateMinerDeadlines | State query | Returns all the proving deadlines for the given miner | | ||
| StateMinerFaults | State query | Returns a bitfield indicating the faulty sectors of the given miner | | ||
| StateMinerInfo | State query | returns info about the specified miner | | ||
| StateMinerInitialPledgeCollateral | State query | returns the initial pledge collateral for the specified miner's sector | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI we're going to have to deprecate StateMinerInitialPledgeCollateral
and introduce a replacement because we can no longer use precommit to perform this calculation, so this is mostly useless now. Currently the new one is called StateMinerInitialPledgeForSector
in filecoin-project/lotus#12384
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up! WIll look into this and update the included methods 👍
| ChainGetTipSet | State query | Returns the tipset with the specified CID | | ||
| ChainGetTipSetAfterHeight | State query | Looks back for a tipset at the specified epoch. If there are no blocks at the specified epoch, the first non-nil tipset at a later epoch will be returned. A fork can be specified, or null for the heaviest chain | | ||
| ChainGetTipSetByHeight | State query | Returns the tipset at the specified height | | ||
| ChainHasObj | State query | checks if a given CID exists in the chain blockstore | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ChainHasObj | State query | checks if a given CID exists in the chain blockstore | | |
| ChainHasObj | Node operation | Checks if a given CID exists in the chain blockstore | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe call these "Chain IO"? something other than "state query" I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand this suggestion? To me HasObj
implies it does no writes, whereas Node operation
could suggest mutability.
cc @elmattic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, currently these operations read from the blockstore, not some special segment of it that's for state, it'll read anything with a CID, messages, events, whatever. I'm mainly taking issue with it being "state" query. Not a big deal, just seems to be a slight misscategorisation. ("Chain" in the name is also slightly misleading, but less so than "state")
| ChainHasObj | State query | checks if a given CID exists in the chain blockstore | | ||
| ChainHead | State query | Returns the chain head (ie. the heaviest tipset) | | ||
| ChainNotify | State query | Subscribe to changes to the chain head | | ||
| ChainPutObj | | Puts a given object into the block store | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ChainPutObj | | Puts a given object into the block store | | |
| ChainPutObj | Node operation | Puts a given object into the block store | |
| ChainHead | State query | Returns the chain head (ie. the heaviest tipset) | | ||
| ChainNotify | State query | Subscribe to changes to the chain head | | ||
| ChainPutObj | | Puts a given object into the block store | | ||
| ChainReadObj | State query | Reads IPLD nodes referenced by the specified CID from chain blockstore and returns raw bytes. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ChainReadObj | State query | Reads IPLD nodes referenced by the specified CID from chain blockstore and returns raw bytes. | | |
| ChainReadObj | Node operation | Reads IPLD nodes referenced by the specified CID from chain blockstore and returns raw bytes. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as #1027 (comment)
cc: @elmattic
- These primarily concern the chain state, actor state or network (p2p) state. They are read-only methods that are necessary to expose all elements of global state. | ||
|
||
- Miner Operations | ||
- A small set of specialized methods miner applications require to participate in consensus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but arguably mpoolpush and wallet* operations are useful for "client" operations too; maybe a broader category name would be good here, or split it into two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although, MpoolPushMessage is "miner" and the other mpool* are "transactions"
maybe all mpool and all wallet should be "transactions" or make a new "client" category to replace "transactions"?
A work-in-progress to establish a common API for all node implementations.
Many todos remain at this point, most notably: