Skip to content

Conversation

@leo-bogastry
Copy link
Contributor

Description

Calling appStateStorage.putBestBlockNumber that makes an update to the best block number without calling blockNumberMappingStorage.put to add the mapping between the block number and block hash can result on a None when calling blockchain.getBestBlock.
I wanted to add these two calls together in the blockchain or blockchainwriter class, but it is not that straight forward.

In order to be able to test this modification already I applied the fix in FastSync and created ETCM-1089 for a proper refactor

Testing

This has been tested against mainnet.

@leo-bogastry leo-bogastry force-pushed the bugfix/ETCM-987-fix-latest-block-not-found branch from 90f603a to 844b1ae Compare August 6, 2021 09:37
@leo-bogastry leo-bogastry force-pushed the bugfix/ETCM-987-fix-latest-block-not-found branch from 844b1ae to 1bd58c4 Compare August 9, 2021 07:14
Copy link
Contributor

@jvdp jvdp left a comment

Choose a reason for hiding this comment

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

I don't disagree with this change but I feel the main improvement it does is mention the TODO in the code that this logic needs to move to a blockchain-storage related component. It won't affect the actual bug because that doesn't have much to do with fast sync, as discussed.

@leo-bogastry
Copy link
Contributor Author

I don't disagree with this change but I feel the main improvement it does is mention the TODO in the code that this logic needs to move to a blockchain-storage related component. It won't affect the actual bug because that doesn't have much to do with fast sync, as discussed.

I think you are probably right. The modification shows that is was not just about having cache and storage in synchronization, different storages should also be always syncronized

@leo-bogastry leo-bogastry merged commit 1553ff5 into develop Aug 9, 2021
@leo-bogastry leo-bogastry deleted the bugfix/ETCM-987-fix-latest-block-not-found branch August 9, 2021 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants