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

Implement multi-hop routing of consensus messages #472

Merged
merged 44 commits into from
Sep 6, 2023
Merged

Implement multi-hop routing of consensus messages #472

merged 44 commits into from
Sep 6, 2023

Conversation

swatanabe
Copy link
Collaborator

No description provided.

- reqwest doesn't support local sockets, so I've hacked around it by
  creating an in-process proxy
…a new consensus. This was causing premature commitment when the old consensus was a single producer
- Reorganize to use unittest
- Add control for node logging
- Wrong index caused incorrect behavior of is_sole_producer and could
  also lead to dereferencing a null pointer
- set_producers can now enter the default implementation in more cases,
  because we call it with the actual producers of each block
- View change now includes a base block
- Fork restrictions are now based on the view change rather than the
  best prepared block
- Ensure that view changes are synchronized network-wide
- Propagage and use joint producer view changes, including when the new
  producers are not BFT
- Fix missing connection state change to ready when we have no blocks
@James-Mart James-Mart linked an issue Aug 29, 2023 that may be closed by this pull request
@swatanabe swatanabe marked this pull request as ready for review August 29, 2023 16:05
- Y has been committed by 2f+1 producers, because that is the requirement for it to be considered irreversible
- let Z be the earliest block that is an ancestor of X (inclusive of X), conflicts with Y, is better than Y, and is prepared by 2f+1 producers. Such a block is guaranteed to exist because X itself must be prepared by 2f+1 producers in order to be committed by any honest producer.

The existence of Z implies that at least f+1 producers have violated the protocol
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the existence of X and Y already imply that at least f+1 producers have violated the protocol?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does. This list is the breakdown of exactly how they violated the protocol.

{
if (route.second.metric == RouteMetric::infinite)
{
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't infinite metric return !feasible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://datatracker.ietf.org/doc/html/rfc8966#feasibility-condition
"Note further that retractions (updates with infinite metric) are always feasible, since they cannot possibly cause a routing loop"

Copy link
Member

Choose a reason for hiding this comment

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

Based on the name, I expected this to test that the routing would follow the shortest path, but the tests really seem only to verify that multi-hop routing works (and will only route messages to producers).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not exactly sure how I would test that. The exact path followed is kind of an implementation detail.

@swatanabe swatanabe merged commit 8f3e72f into main Sep 6, 2023
3 checks passed
@swatanabe swatanabe deleted the routing branch September 6, 2023 18:57
@James-Mart James-Mart added the Node infra Related to the necessary infrastructure provided by all psibase infrastructure providers label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Node infra Related to the necessary infrastructure provided by all psibase infrastructure providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacement for direct_routing
2 participants