Skip to content

blocksync/v2: remove unsupported reactor#7046

Merged
mergify[bot] merged 11 commits intotendermint:masterfrom
tychoish:blocksync-v2-remove
Oct 4, 2021
Merged

blocksync/v2: remove unsupported reactor#7046
mergify[bot] merged 11 commits intotendermint:masterfrom
tychoish:blocksync-v2-remove

Conversation

@tychoish
Copy link
Contributor

@tychoish tychoish commented Oct 4, 2021

This commit should be one of the first to land as part of the v0.36
cycle after cutting the 0.35 branch.

The blocksync/v2 reactor was originally implemented as an experiement
to produce an implementation of the blockstack protocol that would be
easier to test and validate, but it was never appropriately
operationalized and this implementation was never fully debugged. When
the p2p layer was refactored as part of the 0.35 cycle, the v2
implementation was not refactored and it was left in the codebase but
not removed. This commit just removes all references to it.

Copy link

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

Only one request, otherwise I am excited about this change.

@tychoish tychoish added the S:automerge Automatically merge PR when requirements pass label Oct 4, 2021
tychoish and others added 3 commits October 4, 2021 16:55
This script is referenced from the release documentation, we should make sure it's functional. This is helpful in generating the "Special Thanks" section of the changelog.
@mergify mergify bot merged commit cb69ed8 into tendermint:master Oct 4, 2021
@cmwaters
Copy link
Contributor

cmwaters commented Oct 5, 2021

This should probably have a changelog entry.

Also a question for the team (@tychoish, @creachadair, @williambanfield): do you think it's clearer if we were to structure the repo as if there never were multiple versions (same can be said for mempool or PEX). I.e. can we just have blocksync/ instead of blocksync/v0?

Comment on lines +17 to +18
that is battle-tested, but whose test coverage is lacking and is not
formally verified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an outstanding issue for this just so we don't forget to increase test coverage at some latter point. It would be great if we could have a more confident message for our users :)

@tychoish
Copy link
Contributor Author

tychoish commented Oct 5, 2021

It's my intention, heretofore unstated, to flatten the versioned paths as we remove the legacy components.

@creachadair
Copy link

It's my intention, heretofore unstated, to flatten the versioned paths as we remove the legacy components.

Agreed, this is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S:automerge Automatically merge PR when requirements pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants