[ETCM-1042] Switch to a case class implementation for blockchain branch #1073
Conversation
d907735 to
30caf6b
Compare
|
|
||
| /** An interface to manipulate blockchain branches */ | ||
| trait Branch { | ||
| case class BestBranchSubset(tipBlockHash: ByteString, tipBlockNumber: BigInt) extends Branch |
There was a problem hiding this comment.
Why did you add the suffix Subset here?
There was a problem hiding this comment.
It works with any subset on the best branch so I wanted to make it clear and kept the name in from my initial draft of the case class representation. The idea was that we could use it later to define a fork as a tip and a best branch subset which is common to the fork and our current best branch.
But I agree that it might be confusing and not really relevant to our use case for now. I'll just name it BestBranch as before
15b4be9 to
7881d76
Compare
| } | ||
|
|
||
| /** Returns a block hash for the block at the given height if any */ | ||
| def getHashByBlockNumber(branch: Branch, number: BigInt): Option[ByteString] = branch match { |
There was a problem hiding this comment.
what do you think of using a type alias for ByteString when we are talking about a hash?
There was a problem hiding this comment.
I would prefer a value class or a tagged type, because the type alias would just be "cosmetic". But it would probably be out of scope to introduce the new type in this PR.
|
|
||
| /** An interface to manipulate blockchain branches */ | ||
| trait Branch { | ||
| case class BestBranch(tipBlockHash: ByteString, tipBlockNumber: BigInt) extends Branch |
There was a problem hiding this comment.
I can imagine this case class being used for all the branches, not only the best one
There was a problem hiding this comment.
It does not contain data specific to the best branch, yes. Right now it is more a marker type to tell the code that we can use the block number -> hash index.
1691ef1 to
3a61349
Compare
| val number = getBestBlockNumber() | ||
| blockNumberMappingStorage | ||
| .get(number) | ||
| .orElse(blockNumberMappingStorage.get(appStateStorage.getBestBlockNumber())) |
There was a problem hiding this comment.
What's this for? This is a work-around not related to this change, right?
There was a problem hiding this comment.
Yes it is not related to this change.
It's related to anastasiia's comment. It allows to be consistent with getBestBlock which fallbacks to what is in the storage if the block from memory does not exists.
| case BestBranch(_, tipBlockNumber) => | ||
| (for { | ||
| header <- getBlockHeaderByHash(hash) if header.number <= tipBlockNumber | ||
| hash <- getHashByBlockNumber(branch, header.number) |
There was a problem hiding this comment.
You are shadowing hash here; perhaps you can specify the names a bit more?
Also, this relies on using the "hash by block number" index as an authority of "this chain". How do you envision that to work in the new architecture? Will this index be kept up to date eg. when reorganising?
There was a problem hiding this comment.
Good catch. Fixed
For now we have to keep the index updated indeed. Right now isInChain is used by BranchResolution.resolveBranch to check if a sequence of blocks has a parent inside the canonical chain (i.e. if the sequence is rooted). In the end, we would like for mantis to be able to synchronize without maintaining that index. It would still be there to help json rpc calls, but mantis would not have to wait for it to be rebuilt to continue its work.
That said, in the new design, we won't really care that a parent is part of the chain, but we will care that a block is rooted so that we can compare its weight to the current best block.
If we want to do that, I think it depend on how we store incoming blocks:
- if only rooted block are stored (i.e. we store block once they form a branch), then if it is in storage, all is good and we can compare it with the best branch.
- if we store every block it gets trickier. I think in that case we will have to add some block metadata which tells us if a block is rooted or not so that we can easily tell "the parent is rooted so this block is also rooted", probably with different types of blocks.
| /** Checks if given block hash is in this chain. (i.e. is an ancestor of the tip block) */ | ||
| def isInChain(hash: ByteString): Boolean | ||
| } | ||
| case object EmptyBranch extends Branch |
There was a problem hiding this comment.
So we'll have multiple types of branch later on, right? For the purposes of the code you're changing now (branch as produced by getBestBranch) you could also name the trait BestBranch and rename empty branch to Genesis, which resolves to a block number of 1. (Or is it 0?)
There was a problem hiding this comment.
I think we will have several types yes (well, best branch and "other" branch). I don't really know what to do with the EmptyBranch as I return it when even the genesis is not found (normally that can happen only during tests). I wonder if we should not add the genesis as a parameter to the block storage, to ensure that it is always there.
b98e2c9 to
6130c05
Compare
Description
Instead of having a interface which tries to contains every method for a branch, we change it to be a simple case class. It will be more flexible for future changes ; e.g. implement some function for the canonical branch but not for other kinds of branch (or the other way around), without changing a central interface.
This PR