-
Notifications
You must be signed in to change notification settings - Fork 2k
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
New subscription and wallet sync protocol #17340
Conversation
412b7b4
to
9c51ce5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
d847732
to
61055ed
Compare
2730f47
to
fcff5ab
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
In general, there are opportunities to improve the protocol design as new messages are added. Subscriptions essentially open a "channel" multiplexed over the message stream, so we could introduce an explicit channel concept - just an incrementing nonce namespaced on each websocket. The request could allow specifying a start block height, so that we don't sync from genesis each time. Wallets can track a "current height" per puzzle hash. Perhaps something like:
This makes unsubscribe simpler:
Update messages might look something like:
The |
Generally speaking, having multiple potential responses to a particular request is annoying. It means when a client makes a request, it has multiple message IDs it needs to wait on to find a response. The serialization protocol doesn't support
we could merge them:
Here, exactly one of |
I don't understand the benefit of this, it seems like a more complicated API to do roughly the same thing. But maybe I'm misunderstanding something? Subscribing doesn't actually return any coin states, it just registers that you're interested in a given set of puzzle hashes going forward whenever coin states are updated. The |
We don't have any precedent for doing it this way, and this increases the size of the protocol messages and requires differentiating between the response types just the same. Message responses are identified by an id, so rather than listening for a given response type you just wait until you receive a message with the same nonce as was passed in the request. It's pretty simple to do, here is an example I have (a bit outdated, but it shows the idea): |
You will usually use |
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.
seems reasonable
I did notice a |
For request/responses, I see the signature conceptually as
For subscriptions, I see it as
Internal to this, you'd use a nonce to identify subscriptions. Remote messages containing subscription updates could use that subscription nonce to route it to the correct async iterator, and also use that nonce to inform the remote to cancel the subscription. This makes local use really easy. You get the async iterator with the subscription api, then simply [As I write this, I realize you probably actually want to return an async context manager so you can more affirmatively cancel the subscription with a remote message no matter how you exit locally.] I realize these comments aren't really about the code. But I've been trying to write code to connect to remote chia nodes and I've found it to be much more complicated than it should be, and how certain changes could allow for better abstractions and smaller code. But maybe you don't want to tackle this now. |
It should be the same value as the request id, if that's left as None the response will be too. If it's an update (ie CoinStateUpdate), it will also be None. Btw I wrote the |
my recollection of how our protocol driver works is that it's not waiting for message IDs, it's demultiplexing based on transaction IDs (or request IDs). So, any message can (in theory) be a response to any other. The current protocols usually have one success message and a separate failure message too. |
54350cf
to
d62b005
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
I think it's reasonable to not cover the checks for race condition |
|
Purpose:
The wallet protocol is lacking some essential functionality for syncing wallets and requesting coin data. This improves it with new messages for fetching coin state and subscribing.
Existing Limitations:
New Features
New Protocol
Remove Puzzle Subscriptions
Removes puzzle hashes from the subscription list (or all of them if
None
), returning the hashes that were actually removed.Remove Coin Subscriptions
Removes coin ids from the subscription list (or all of them if
None
), returning the ids that were actually removed.Request Puzzle State
Requests coin states that match the given puzzle hashes (or hints).
Unlike
RegisterForPhUpdates
, this does not add subscriptions for the puzzle hashes automatically.When
subscribe_when_finished
is set toTrue
, it will add subscriptions, but only once the last batch has been requested.As well as this, previously it was impossible to get all coin records if the number of items exceeded the limit.
This implementation allows you to continue where you left off with
previous_height
andheader_hash
.If a reorg of relevant blocks occurs while doing so,
previous_height
will no longer matchheader_hash
.This can be handled by a wallet by simply backtracking a bit, or restarting the sync from genesis. It could be inconvenient, but at least you can detect it.
In the event that a reorg is detected by a node,
RejectPuzzleState
will be returned. This is the only scenario it will be rejected directly like this.Additionally, it is now possible to filter out spent, unspent, or hinted coins, as well as coins below a minimum amount.
This can reduce the risk of spamming or DoS of a wallet in some cases, and improve performance.
If
previous_height
isNone
, you are syncing from genesis. Theheader_hash
should match the genesis challenge of the network you are connected to.Request Coin State
Request coin states that match the given coin ids.
Unlike
RegisterForCoinUpdates
, this does not add subscriptions for the coin ids automatically.When
subscribe
is set toTrue
, it will add and return as many coin ids to the subscriptions list as possible.Unlike the new
RequestPuzzleState
message, this does not implement batching for simplicity. The order is also not guaranteed.However, you can still specify the
previous_height
andheader_hash
to start from.If a reorg of relevant blocks has occurred,
previous_height
will no longer matchheader_hash
.This can be handled by a wallet depending on the use case (for example by restarting from zero). It could be inconvenient, but at least you can detect it.
In the event that a reorg is detected by a node,
RejectCoinState
will be returned. This is the only scenario it will be rejected directly like this.If
previous_height
isNone
, you are syncing from genesis. Theheader_hash
should match the genesis challenge of the network you are connected to.Reject State Reason
Concerns
The data which a full node gives you cannot be trusted unless it is your own node.
These additions to the protocol make no effort to make untrusted data any easier to validate.
The best method I am aware of is still just to cross reference with other peers and ban outliers.
Checklist