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

Common Node API #1027

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

Conversation

ansermino
Copy link

A work-in-progress to establish a common API for all node implementations.

Many todos remain at this point, most notably:

  • Further discussion on which methods are included and the associated rationale
  • Sufficient conformance testing to validate implementations are adherent to the specification

@jennijuju
Copy link
Member

@filecoin-project/lotus-maintainers 👀 plz

@Stebalien
Copy link
Member

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).

Comment on lines +100 to +101
- 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.
Copy link
Member

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).

Copy link
Author

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.

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:
Copy link
Member

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:

  1. Events (which currently use a different API).
  2. Messages (there's actually no great "native" API for watching for messages).

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.

Copy link
Member

@jennijuju jennijuju Jul 4, 2024

Choose a reason for hiding this comment

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

Copy link
Member

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

@jennijuju
Copy link
Member

I am supportive of the motivation and direction of this proposal. Will give a more thorough review once the methods list are proposed

@Stebalien
Copy link
Member

FWIW, I think this is a great idea.

Comment on lines 68 to 76
{ "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] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ "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] }

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 :)

@aatifsyed
Copy link

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).

@Stebalien We chose to be a technical FIP based on this guidance:

- **Interface** - includes improvements around client [API/RPC](https://spec.filecoin.io/#systems__filecoin_nodes__node_types__node-interface) specifications and standards, and also certain language-level standards like method names.

Happy to move to FRC if that fits better - maybe the above could be amended?

@ansermino
Copy link
Author

ansermino commented Jul 2, 2024

I am supportive of the motivation and direction of this proposal. Will give a more thorough review once the methods list are proposed

🙏 @jennijuju

We have a working doc with all the methods here:
https://docs.google.com/spreadsheets/d/1fFkQuEjvFAd2s1dGX5zGmhxsEMLMUZ4uQFnIXgSZA5w/edit?usp=sharing

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

aatifsyed and others added 5 commits July 5, 2024 02:46
* mark: aatifsyed/common-node-api

* feat: describe subscriptions as a bunch of OpenRPC fragments

* fix: link to lotus issue
Co-authored-by: Jiaying Wang <42981373+jennijuju@users.noreply.github.com>
@BigLep
Copy link
Member

BigLep commented Jul 12, 2024

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 |
Copy link
Member

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

Copy link
Author

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 |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| 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 |

Copy link
Member

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

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

Copy link
Member

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 |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| 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. |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| 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. |

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.
Copy link
Member

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?

Copy link
Member

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"?

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.

6 participants