Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

A fast-path for requesting AvailableData from backing validators #2453

Merged
merged 19 commits into from
Feb 17, 2021

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Feb 16, 2021

The user of AvailabilityRecoveryMessage::RecoverAvailableData can supply an optional backing group. The availability recovery subsystem will request from the backing validators, in random order, as an optimization. As at least a majority of the backing validators will have the data unless they are malicious, this is an optimization.

I also fixed some bugs and added new tests

  • We now check that validators are sending the right chunk index.
  • Garbage collection needed a condition inverted to work correctly

One concern I have is that we make the AvailableData requests sequentially, and the time-out we allow is pretty long. An adversary could stall us by refusing to answer requests and that might trigger no-shows. But it's disadvantageous for them to do that as it enlists more people to check their block. And once we recover it, we'd check the block as expected. So this isn't really a bad vector, it just lets an adversary either slightly delay finality or delay the pain of slashing.

@rphmeier rphmeier added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 16, 2021
@github-actions github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 16, 2021
@rphmeier rphmeier changed the title A fast-path requesting from backing validators A fast-path for requesting AvailableData from backing validators Feb 16, 2021
@rphmeier rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 16, 2021
@rphmeier rphmeier marked this pull request as ready for review February 16, 2021 22:22
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

I took the liberty of fixing two typos. Other then that, looks good to me. I am really worried about the chunk recovery to fail in practice, so I would suggest to move over to chunk recovery on any error, which makes it less likely to introduce bugs in that code, that would lead to chunk recovery not even be started.

node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
@rphmeier
Copy link
Contributor Author

rphmeier commented Feb 17, 2021

I am really worried about the chunk recovery to fail in practice, so I would suggest to move over to chunk recovery on any error

Understood. But the only error that can occur in the fast path is that the interaction task gets disconnected from the underlying subsystem. I've changed the code to better reflect that, and I think that in those cases, there is no point in moving on to chunk recovery.

@eskimor
Copy link
Member

eskimor commented Feb 17, 2021

Understood. But the only error that can occur in the fast path is that the interaction task gets disconnected from the underlying subsystem. I've changed the code to better reflect that, and I think that in those cases, there is no point in moving on to chunk recovery.

Looks indeed more fool-proof now :-)

@eskimor eskimor self-requested a review February 17, 2021 18:51
@rphmeier
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Feb 17, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Feb 17, 2021

Checks failed; merge aborted.

@rphmeier rphmeier merged commit f778e52 into master Feb 17, 2021
@rphmeier rphmeier deleted the rh-recovery-fast-path branch February 17, 2021 19:51
@rphmeier
Copy link
Contributor Author

(restarted transaction version job, which had a spurious failure, and it passed this time)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants