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

Remove the assumption on BlockChain<T> has a block at least #750

Closed
dahlia opened this issue Jan 7, 2020 · 3 comments
Closed

Remove the assumption on BlockChain<T> has a block at least #750

dahlia opened this issue Jan 7, 2020 · 3 comments
Assignees
Labels
stale The issue is stale suggestion Suggestion or feature request

Comments

@dahlia
Copy link
Contributor

dahlia commented Jan 7, 2020

It's an idea which I came up with after reviewed the patch planetarium/libplanet-explorer#86.

After the patch #688 (and #746) was merged, BlockChain<T> became to guarantee it has a genesis block at least. This assumption purposes to prevent reorg/fork from the bottom by mistake.

However, IMHO the assumption that BlockChain<T>.Count is always greater than 0 is unnecessarily strong only for the purpose of preventing reorg from the bottom, and makes node apps (i.e., game clients) too complex to be deployed in reality, because each node has to bundle the whole data of the genesis block, which is hardly configurable by hand. For example, the patch planetarium/libplanet-explorer#86 adds an option -G/--genesis-block which takes a file, not a simple token/string, which is filled with the Bencodex-formatted genesis block. If you want to connect a Libplanet Explorer node to the existing network you need to extract a genesis block from the network first.

Instead of having such a strong assumption, to balance between the purpose and the complexity of deployment, I suggest BlockChain<T>() constructor to take only a HashDigest<SHA256> genesisHash instead of Block<T> genesisBlock. In this way, BlockChain<T> can be empty, but even if it has a genesis block it still guarantees what the genesis block is. We might be able to maintain the existing constructor taking Block<T> genesisBlock because it can be implemented by calling the basic constructor with genesisBlock.Hash and then immediately .Append(genesisBlock).

@planetarium/libplanet Opinions?

@dahlia dahlia added the suggestion Suggestion or feature request label Jan 7, 2020
@longfin
Copy link
Member

longfin commented Jan 10, 2020

I agree with the claim that maintaining the genesis block is too hard to configure in some situations. but I also worry that containing genesis hash instead of the block can complicate operations because it can also be seen as adding a mode. (we can generate genesis hash from a block, but it's too hard to guess for given genesis hash. so BlockChain<T> that contains only genesis hash can be sort of 'no-mining mode' until receiving genesis block from the network)

Therefore, if we accept BlockChain<T> with only genesis hash, we also should indicate in some way that this BlockChain<T>.MineBlock() can't be used without the network. (Of course, libplanet-explorer is okay because it can't mine for other reasons. 😅 )

@stale
Copy link

stale bot commented Mar 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale The issue is stale label Mar 10, 2020
@dahlia dahlia removed the stale The issue is stale label Mar 10, 2020
@stale
Copy link

stale bot commented May 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale The issue is stale label May 9, 2020
@dahlia dahlia closed this as completed Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale The issue is stale suggestion Suggestion or feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants