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

Rename the block sync service #2612

Closed
schomatis opened this issue Jul 27, 2020 · 4 comments · Fixed by #3624
Closed

Rename the block sync service #2612

schomatis opened this issue Jul 27, 2020 · 4 comments · Fixed by #3624
Assignees
Labels
kind/enhancement Kind: Enhancement

Comments

@schomatis
Copy link
Contributor

Currently BlockSyncService and BlockSync (client), to something like ChainExchange (which regrettably is currently used for IPFS block exchange in the bitswap setup, that should probably also be changed).

  • Block is an ambiguous term that gets confused with IPFS blocks. We need something more representative, ideally chain, but at least tipset (because that is in fact what we fetch, we don't request particular blocks).
  • Sync is irrelevant, it's the consumer of this service but has no role here.
  • We need to emphasize that we exchange contiguous tipsets in this service (a request here is just a tipset and a length).

This is important to distinguish it from our many similar "block" (whether IPFS or Filecoin) exchanges/services, and justify why we need this one: when syncing we can't fetch the Filecoin blocks independently because we don't know upfront which ones we need, we have no CIDs to drive the bitswap exchange, we just have a new (potential) head and we need to start from there, but we also don't want to fetch one block at a time, rather we fetch chain segments at the discretion of our peers that will assemble the segment for us based on the requested starting tipset.

@daviddias daviddias added the kind/enhancement Kind: Enhancement label Jul 30, 2020
@yiannisbot
Copy link

I would probably vote for TipsetSync, as this is the most accurate term IMO. Some justification:

  • ChainSync already exists, so we don't want to have conflicting terms.
  • The BlockSync process is rather similar to ChainSync, which is syncing of the chain when a new node joins the network. This fact should be mentioned in both parts of the spec and link to each other in order to make it clear and avoid misunderstandings.
  • I would probably avoid the term "exchange", as BlockSync is not really about random exchange, but rather actual syncing when a node has gone offline for a bit (but has sync'ed the chain already when they first joined).

@schomatis
Copy link
Contributor Author

Thanks for engaging. The spec is one of the main beneficiaries of this name change and should weigh in.

I'd stress again this part (but open to more feedback about it):

Sync is irrelevant, it's the consumer of this service but has no role here.


I would probably avoid the term "exchange", as BlockSync is not really about random exchange, but rather actual syncing when a node has gone offline for a bit (but has sync'ed the chain already when they first joined).

I think the name needs to reflect what the protocol provides, not how you use it. You can very much exchange any (call it random if you want) segment of a chain, it can even be a chain from a different network (different genesis block) than the one you're syncing to (as long as the peer has it).

@yiannisbot
Copy link

I see - I agree with your points. ChainExchange, BlockExchange, or TipsetExchange all sound reasonable to me. For the purposes of this protocol, I don't think we should be discouraged from using "block" due to its IPFS usage - if something needs to change in this case is the IPFS term, which in practice is a content "chunk" :)

If this is a general purpose protocol that can be used by any blockchain, then perhaps exclude the Tipset* option, as the tipset is specific to the Filecoin blockchain?

Another idea is to use *swap, as this protocol sounds similar to Bitswap, but for tipsets/blocks :-D

@raulk
Copy link
Member

raulk commented Aug 28, 2020

I'm looking to add an interface for *blocksync.BlockSync, as we need to mock that out in the Syncer so we can test it in isolation. I came across this thread.

I'm happy to do this refactor, if you give me the go. I think ChainExchange is descriptive and generic enough so it can be extended in the future.

What I'm thinking of:

  • directory rename: chain/{blocksync=>exchange}.
  • package name: exchange.
    • I'm generally a fan of naming package and directory the same, and allowing the consumer to do any aliasing it wants, if it needs to and it's sensible in its local context.
    • It's very counter-intuitive to import github.com/filecoin-project/lotus/chain/exchange just to find out that exchange. doesn't work. It forces me to dive into the source to find out what the developer decided to name the package, in the the happy hour they they invented it in.
    • I'm happy to follow whatever convention you prefer, though.
  • interfaces: exchange.Client and exchange.Server, with implementations being private and the constructors returning the interfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Kind: Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants