-
Notifications
You must be signed in to change notification settings - Fork 111
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
Added the first draft of Route Broadcasting Protocol #455
Added the first draft of Route Broadcasting Protocol #455
Conversation
#### Procedure | ||
Route Update is done in the following procedure: | ||
|
||
- A node requests an update to the corresponded node with information of route update logs. |
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.
One thing that's unclear here is which node does what, and how this relates to the different relationships defined in Node Requirements (draft). Perhaps we need to define the Requestor
and the Counterpart
more formally?
If my understanding is correct, NodeA will send a RouteControlRequest
to NodeB, and then NodeB will enter the SYNC
mode. Next, NodeA will send a RouteUpdateRequest
with added or withdrawn routes.
I suppose that if NodeA is a child of NodeB, however, then the original RouteControlRequest
would fail?
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.
Added Operation Model
section to make the roles clear.
Does this help?
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.
It seems that if a child node is configured to receive route updates, and the parent is not configured to send routes, it fails.
(works only when the child is configured to receive route updates, and the parent is configured to send route updtes also)
I'll add some statements for it.
|
||
- A node requests an update to the corresponded node with information of route update logs. | ||
- The corresponded node updates its routing table, and respond that the request is done. | ||
- If the request cannot be deserialized or interpreted as appropriate, the corresponded node responds error. |
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.
It would be good to also add summary notes about what should happen between Child, Peer, and Parent nodes with regard to the overall protocol.
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.
Added Link Relations and Routing
section and modified Procedure
sections a bit. What do you think?
Auth ::= OCTET STRING (SIZE (32)) | ||
|
||
Property ::= SEQUENCE { | ||
meta UInt8, |
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.
It might be worth defining the meaning of meta
. See ilp-protocol-ccp for some examples of the data encoded into this byte field.
From what I can tell, the following fields exist: optional
, transitive
, partial
, isUtf8
. It's unclear, however, from the source code in ilp-protocol-ccp
what each of these mean.
@adrianhopebailie, @justmoon for more details?
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.
I actually knew the options but I hesitated writing it because it seemed that the options were not used. Should I write it nevertheless?
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.
I think it would be fine if these definitions were written later (e.g., after merging this PR), although the terms are used in the source-code (though undocumented), and so it would be good to outline them in the event some implementation wanted to start using them.
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.
I added some statements about meta
.
I can imagine what these flags mean, but those are not certain, so I didn't write the exact meaning of the flags at this point.
routingTableId RoutingTableId, -- the routing table ID that the requesting node knows at that point | ||
currentEpochIndex UInt32, -- current epoch index of the node that requests the update for the other nodes | ||
fromEpochIndex UInt32, -- the epoch index that the log of this update request starts from | ||
toEpochIndex UInt32, -- the epoch index that the log of this update request ends at |
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.
I assume this is inclusive
(as opposed to exclusive
), but might be worth stating directly. For example, if fromEpoch=1
, and toEpoch=3
, then I would expect 3 routes (1, 2, and 3).
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.
I referred to this code:
https://github.com/interledgerjs/ilp-connector/blob/master/src/routing/ccp-sender.ts#L182-L186
and this specification:
https://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.10
As a conclusion, in my opinion, 3
is not included in your case. See the picture below.
Am I wrong?
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, I think you're right. In that case, we should use the term exclusive
, like this:
the epoch index that the log of this update request ends at, exclusive
currentEpochIndex UInt32, -- current epoch index of the node that requests the update for the other nodes | ||
fromEpochIndex UInt32, -- the epoch index that the log of this update request starts from | ||
toEpochIndex UInt32, -- the epoch index that the log of this update request ends at | ||
holdDownTime UInt32, -- reserved for the future, currently not used |
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.
Despite not being used, can we document what this means?
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.
I wanted to specify, but I could not find any reference to determine the function.
@justmoon Could you give me some reference?
toEpochIndex UInt32, -- the epoch index that the log of this update request ends at | ||
holdDownTime UInt32, -- reserved for the future, currently not used | ||
speaker Address, -- the ILP address of the requesting node | ||
addedRoutes SEQUENCE OF NewRoute, -- the added routes |
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.
Consider changing to New Routes that the speaker is advertising to the counterpart node
holdDownTime UInt32, -- reserved for the future, currently not used | ||
speaker Address, -- the ILP address of the requesting node | ||
addedRoutes SEQUENCE OF NewRoute, -- the added routes | ||
withdrawnRoutes SEQUENCE OF Address -- the withdrawn routes |
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.
Consider changing to: Withdrawn Routes that the speaker is advertising as being no longer valid or useful.
…oute-broadcasting-protocol
@sappenin Thank you so much for the input. |
@sappenin I fixed some of your inputs as far as I could at this point. |
On the other hand, in a `Peer` and `Peer` relation, the two nodes have different addresses respectively, and the further connected addresses are unclear. So in this case, it is generally helpful to configure to exchange routing information each other. | ||
|
||
Conclusively, the following is the general configuration of nodes depending on the relations. | ||
- `Peer` and `Peer` |
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.
Consider using to
instead of and
in these cases, so it would read like this:
Peer
to Peer
.
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.
BTP connections could be opened from each side, and to
makes us imagine it is directional, so I thought and
was more appropriate in this case. Though I don't have the native sense of English, I'll follow your opinion. Amended the lines.
@@ -162,6 +188,7 @@ Route Update is done in the following procedure: | |||
|
|||
- A node sends a route update request to the corresponded node with information of route update logs. | |||
- The receiver node updates its routing table, and responds that the request is done. | |||
- If the receiving node is not configured to receive route updates, the receiving node responds error. |
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.
Minor nit, should read: ...the receiving node responds with an error
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.
Fixed to use prepositions.
bumping to keep this from getting closed by Stalebot. |
We have to define how |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. If this issue is important, please feel free to bring it up on the next Interledger Community Group Call or in the Gitter chat. |
I'm finally trying to dig into CCP. Thanks @dora-gt for putting this together -- it's super helpful! Much easier to understand than the code in |
Fixes Issue #83 with the following changes: * Refactor the CCPSender to track epochs and incoming routes on a per-peer basis (see interledger/rfcs#455) * Refactor Routing services into `RouteBroadcaster` and `ExternalRoutingService` * Refactor RoutingTables to properly store incoming/outgoing routes separately from the Log of routes. * Fix epoch tracking for all components. * Properly load all route-types at startup. Signed-off-by: David Fuelling <[email protected]>
* Fixes Issue #83 with the following changes: * Refactor the CCPSender to track epochs and incoming routes on a per-peer basis (see interledger/rfcs#455) * Refactor Routing services into `RouteBroadcaster` and `ExternalRoutingService` * Refactor RoutingTables to properly store incoming/outgoing routes separately from the Log of routes. * Fix epoch tracking for all components. * Properly load all route-types at startup. * Remove unused Local AccountRelationship * Javadoc updates * Remove PostProcessor * This existed as an experiment for performance optimization purposes, but was overall a terrible idea. Plus, it turned out to be unnecessary. Signed-off-by: David Fuelling <[email protected]>
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. If this issue is important, please feel free to bring it up on the next Interledger Community Group Call or in the Gitter chat. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. If this issue is important, please feel free to bring it up on the next Interledger Community Group Call or in the Gitter chat. |
Added the first draft of Route Broadcasting Protocol based on this issue: #432, including ASN.1 definition.
Please check the description.
It seems that there are some unused fields, so either of removing or implementing may be required.