Conversation
andresilva
added
A3-in_progress
| set_id: request.set_id, | ||
| message: catch_up, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Maybe we should update the view of the peer after sending the catch up message. In case of receiving the same catch-up request, do we respond again?
There was a problem hiding this comment.
Good point! I think we should add a validation here that the peer doesn't request a catch up for a round earlier than whatever his view is. If we update their view when we reply to a catch-up then this would prevent DoS but I'm not sure we should do it. I was thinking of adding a cache of last requests in a follow-up PR, and this cache would be used to prevent DoS.
There was a problem hiding this comment.
Updating the view here could be a bit weird, since they might have a neighbor packet in transit that is from before the view we expect them to have upon receiving the catch-up message.
I'd rather simply rate-limit the amount of catch-up messages a peer can send us. ideally we could inform peers of the rate-limiting as well...maybe with a CatchUp::NotUntil(duration_millis) variant. Regular CatchUp responses could also hold that kind of timeout parameter.
There was a problem hiding this comment.
Well, that's probably not the right approach considering the PSM...what we should be doing is actually charging the peer a cost for every catch-up request. The hope being that if they spam us, they get disconnected fairly quickly.
There was a problem hiding this comment.
Added a static cost to answering a catch up request which is inverse of the benefit the other party gets.
Co-Authored-By: Robert Habermeier <rphmeier@gmail.com>
andresilva
force-pushed
the
andre/grandpa-catchup-messages
branch
from
61b8650 to
4bf6dab
Compare
andresilva
added
A0-please_review
|
I think I've addressed all grumbles. Need to release finality-grandpa 0.8.1 but this should be ready for review. |
rphmeier
added
A6-mustntgrumble
and removed
A0-please_review
|
I'd like docs on the historical votes to ensure that the property of being a minimal set is maintained. And then the finality-grandpa update as well. |
|
on ice until the new finality-grandpa is released. |
|
@andresilva please merge once finality-grandpa is updated. |
|
finality-grandpa v0.8 released, merging. |
* grandpa: initial structure for catch up messages * grandpa: answer catch up requests * grandpa: inject catch up messages into global stream * grandpa: keep track of pending catch up request * grandpa: block catchup until all referenced blocks are imported * grandpa: unify catch up and commit streams * grandpa: simplify communication stream/sink types * grandpa: note gossip validator on catch up message import * grandpa: fix cost on catch up message validation * grandpa: check signatures on catch up messages * grandpa: clean up catch up request handling state * grandpa: adjust costs on invalid catch up requests * grandpa: release lock before pushing catch up message * grandpa: validate catch up request against peer view * grandpa: catch up docs * grandpa: fix tests * grandpa: until_imported: add tests for catch up messages * grandpa: add tests for catch up message gossip validation * grandpa: integrate HistoricalVotes changes * grandpa: add test for neighbor packet triggering catch up * grandpa: add test for full voter catch up * grandpa: depend on finality-grandpa 0.8 from crates * granda: use finality-grandpa test helpers * grandpa: add PSM cost for answering catch up requests * grandpa: code style fixes Co-Authored-By: Robert Habermeier <rphmeier@gmail.com> * grandpa: more trailing commas * grandpa: lower cost of invalid catch up requests near set change * grandpa: process catch up sending on import of neighbor message * grandpa: add comments on HistoricalVotes * grandpa: use finality-grandpa v0.8.1 from crates.io * grandpa: fix test compilation
* grandpa: initial structure for catch up messages * grandpa: answer catch up requests * grandpa: inject catch up messages into global stream * grandpa: keep track of pending catch up request * grandpa: block catchup until all referenced blocks are imported * grandpa: unify catch up and commit streams * grandpa: simplify communication stream/sink types * grandpa: note gossip validator on catch up message import * grandpa: fix cost on catch up message validation * grandpa: check signatures on catch up messages * grandpa: clean up catch up request handling state * grandpa: adjust costs on invalid catch up requests * grandpa: release lock before pushing catch up message * grandpa: validate catch up request against peer view * grandpa: catch up docs * grandpa: fix tests * grandpa: until_imported: add tests for catch up messages * grandpa: add tests for catch up message gossip validation * grandpa: integrate HistoricalVotes changes * grandpa: add test for neighbor packet triggering catch up * grandpa: add test for full voter catch up * grandpa: depend on finality-grandpa 0.8 from crates * granda: use finality-grandpa test helpers * grandpa: add PSM cost for answering catch up requests * grandpa: code style fixes Co-Authored-By: Robert Habermeier <rphmeier@gmail.com> * grandpa: more trailing commas * grandpa: lower cost of invalid catch up requests near set change * grandpa: process catch up sending on import of neighbor message * grandpa: add comments on HistoricalVotes * grandpa: use finality-grandpa v0.8.1 from crates.io * grandpa: fix test compilation
* grandpa: initial structure for catch up messages * grandpa: answer catch up requests * grandpa: inject catch up messages into global stream * grandpa: keep track of pending catch up request * grandpa: block catchup until all referenced blocks are imported * grandpa: unify catch up and commit streams * grandpa: simplify communication stream/sink types * grandpa: note gossip validator on catch up message import * grandpa: fix cost on catch up message validation * grandpa: check signatures on catch up messages * grandpa: clean up catch up request handling state * grandpa: adjust costs on invalid catch up requests * grandpa: release lock before pushing catch up message * grandpa: validate catch up request against peer view * grandpa: catch up docs * grandpa: fix tests * grandpa: until_imported: add tests for catch up messages * grandpa: add tests for catch up message gossip validation * grandpa: integrate HistoricalVotes changes * grandpa: add test for neighbor packet triggering catch up * grandpa: add test for full voter catch up * grandpa: depend on finality-grandpa 0.8 from crates * granda: use finality-grandpa test helpers * grandpa: add PSM cost for answering catch up requests * grandpa: code style fixes Co-Authored-By: Robert Habermeier <rphmeier@gmail.com> * grandpa: more trailing commas * grandpa: lower cost of invalid catch up requests near set change * grandpa: process catch up sending on import of neighbor message * grandpa: add comments on HistoricalVotes * grandpa: use finality-grandpa v0.8.1 from crates.io * grandpa: fix test compilation
5 participants
Currently after syncing to the latest authority set, GRANDPA voters will observe new commit messages and start a prospective round based on that (e.g. they're at round 1, they observe a commit for round 10, so they start a prospective round 11). This PR replaces this logic by creating an explicit request mechanism for catching up to the latest round. When we see a neighbor packet from a peer that's at a round
Nhigher thanlocal_round + 2we issue a catch up request targeted to that node. The receiving node replies back with a catch up message that includes all round data needed (base, prevotes, precommits) to safely jump to that round. Currently the node may reply with a valid catch up message that finalizes any round higher than requested.Depends on paritytech/finality-grandpa#53.
TODO: