[ETCM-1044] add best block hash into storage #1077
Conversation
| @@ -27,6 +38,16 @@ class AppStateStorage(val dataSource: DataSource) extends TransactionalKeyValueS | |||
| def getBestBlockNumber(): BigInt = | |||
| getBigInt(Keys.BestBlockNumber) | |||
|
|
|||
| def getBestBlockInfo(): BestBlockInfo = | |||
| BestBlockInfo( // FIXME default value for hash ? | |||
| get(Keys.BestBlockHash).map(v => ByteString(Hex.decode(v))).getOrElse(ByteString.empty), | |||
There was a problem hiding this comment.
I don't really know if there is a decent value to return when the hash is not there. This is a case of "should not happen" because we always have a genesis when running the application. But it does happen during tests.
There was a problem hiding this comment.
Is it because the tests setting are incomplete (missing genesis, empty storage, other ?), and is it considered ok ?
As for the default value, I would eventually go the Option way.
But it raises the issue of how to handle the default value (be it the actual empty ByteString or a None). If every piece of code handling the potential missing hash is unable to cope with it, it may be worth making it mandatory to have a proper genesis each time, and raise a hard-error at a single point during the getBestBlockData()
There was a problem hiding this comment.
Is it because the tests setting are incomplete (missing genesis, empty storage, other ?), and is it considered ok ?
I think the reason is that it is not strictly necessary to actually have a genesis in storage for some test because of the mocking, and setting up a real genesis / making it mandatory in the design was not done as a consequence. In this particular case as 0 was a pretty decent default value (it is the genesis number after all) it was not really visible. But now storing the genesis hash makes the problem more apparent.
This inconsistency does not really happen outside of tests because the application loads genesis on startup, so it's "mostly" ok.
As for the default value, I would eventually go the Option way.
I would personally prefer to have a design that makes this inconsistency impossible as you said to be sure that we have a genesis (probably having the Storage take some information about the genesis in its constructor). I'm not sure about the impact this would have though as for instance ETS requires us to change the genesis at runtime.
Returning an option would mean that we would have to handle the None case everywhere, which really only means the the application is in an inconsistent state (In which case the application should probably crash).
There was a problem hiding this comment.
Would it help to have to genesis block available by other means (eg. passed in as parameter) instead of from storage? That might actually be the more straightforward approach: genesis data is loaded from config when the node starts and is then written to the database. So loading it from the database is a bit of a detour.
There was a problem hiding this comment.
Yes, what I think about is having some information about genesis passed as a parameter. Not only for the app state storage, but also in other storage such as bodies and headers. This way we can ensure that the genesis is there and remove some options in the code (like in getBestBlock).
I think that doing it in this PR would be too big of a change even if we only do the AppStateStorage part. I created this followup ticket to detail what would need to be done : https://jira.iohk.io/browse/ETCM-1090.
2904794 to
4d3aa02
Compare
6e8e48b to
4f5f20a
Compare
src/main/scala/io/iohk/ethereum/db/storage/AppStateStorage.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/jsonrpc/JsonRpcControllerEthSpec.scala
Outdated
Show resolved
Hide resolved
src/test/scala/io/iohk/ethereum/domain/BlockchainReaderSpec.scala
Outdated
Show resolved
Hide resolved
4f5f20a to
0a5edb8
Compare
|
Do we still need to store best block number when we could fetch the header by hash and read the number from there? |
6ebd60a to
ab47a79
Compare
Yes, we should be able to get rid of the block number in the app state storage. It is used in a few places right now so it cannot be done in one step. I'm still not sure that we should do it though, but I don't see any reason to not favor consistency right now so I'll create a ticket for this. |
68e270b to
223df14
Compare
223df14 to
a9d4190
Compare
rename putBestBlockData to putBestBlockInfo avoid using magic number for block number in test
ec740a6 to
e6d128c
Compare
e6d128c to
beb908b
Compare
src/main/scala/io/iohk/ethereum/domain/appstate/BestBlockInfo.scala
Outdated
Show resolved
Hide resolved
f71d59a to
6b3202f
Compare
Description
This was originally from a spike (ETCM-1051), but I ended up implementing ETCM-1044 while poking at the code.
Make
getBestBlockusegetBlockByHash.For this I added the best block hash along with the bestBlockNumber, so that when we want to get the best block number we can just get it from its hash and not from the block number mapping.