Skip to content

ETCM-938 Refactor Blockchain and extract methods getEvmCodeByHash and mptStateSavedKeys #1017

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

Merged
merged 1 commit into from
Jun 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/it/scala/io/iohk/ethereum/sync/util/CommonFakePeer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ abstract class CommonFakePeer(peerName: String, fakePeerCustomConfig: FakePeerCu
)

val bl = BlockchainImpl(storagesInstance.storages)
val evmCodeStorage = storagesInstance.storages.evmCodeStorage

val genesis = Block(
Fixtures.Blocks.Genesis.header.copy(stateRoot = ByteString(MerklePatriciaTrie.EmptyRootHash)),
Expand Down Expand Up @@ -203,7 +204,10 @@ abstract class CommonFakePeer(peerName: String, fakePeerCustomConfig: FakePeerCu
)

val blockchainHost: ActorRef =
system.actorOf(BlockchainHostActor.props(bl, peerConf, peerEventBus, etcPeerManager), "blockchain-host")
system.actorOf(
BlockchainHostActor.props(bl, storagesInstance.storages.evmCodeStorage, peerConf, peerEventBus, etcPeerManager),
"blockchain-host"
)

lazy val server: ActorRef = system.actorOf(ServerActor.props(nodeStatusHolder, peerManager), "server")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ object FastSyncItSpecUtils {
storagesInstance.storages.fastSyncStateStorage,
storagesInstance.storages.appStateStorage,
bl,
storagesInstance.storages.evmCodeStorage,
storagesInstance.storages.nodeStorage,
validators,
peerEventBus,
etcPeerManager,
Expand Down Expand Up @@ -72,7 +74,7 @@ object FastSyncItSpecUtils {
val codeHash = kec256(accountExpectedCode)
val accountExpectedStorageAddresses = (i until i + 20).toList
val account = bl.getAccount(accountAddress, blockNumber).get
val code = bl.getEvmCodeByHash(codeHash).get
val code = evmCodeStorage.get(codeHash).get
val storedData = accountExpectedStorageAddresses.map { addr =>
ByteUtils.toBigInt(bl.getAccountStorageAt(account.storageRoot, addr, ethCompatibleStorage = true))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,6 @@ class BlockchainMock(genesisHash: ByteString) extends Blockchain {

override def getChainWeightByHash(blockhash: ByteString): Option[ChainWeight] = ???

override def getEvmCodeByHash(hash: ByteString): Option[ByteString] = ???

override def getReceiptsByHash(blockhash: ByteString): Option[Seq[Receipt]] = ???

def getAccount(address: Address, blockNumber: BigInt): Option[Account] = ???
Expand Down Expand Up @@ -219,6 +217,4 @@ class BlockchainMock(genesisHash: ByteString) extends Blockchain {
override def save(block: Block, receipts: Seq[Receipt], weight: ChainWeight, saveAsBestBlock: Boolean): Unit = ???

override def getLatestCheckpointBlockNumber(): BigInt = ???

override def mptStateSavedKeys(): Observable[Either[RocksDbDataSource.IterationError, NodeHash]] = ???
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ object FixtureProvider {
override val blockBodiesStorage: BlockBodiesStorage = new BlockBodiesStorage(dataSource)
override val chainWeightStorage: ChainWeightStorage = new ChainWeightStorage(dataSource)
override val transactionMappingStorage: TransactionMappingStorage = new TransactionMappingStorage(dataSource)
override val nodeStorage: NodeStorage = new NodeStorage(dataSource)
override val cachedNodeStorage: CachedNodeStorage = new CachedNodeStorage(nodeStorage, caches.nodeCache)
override val pruningMode: PruningMode = ArchivePruning
override val appStateStorage: AppStateStorage = new AppStateStorage(dataSource)
val nodeStorage: NodeStorage = new NodeStorage(dataSource)
val cachedNodeStorage: CachedNodeStorage = new CachedNodeStorage(nodeStorage, caches.nodeCache)
val pruningMode: PruningMode = ArchivePruning
override val stateStorage: StateStorage =
StateStorage(
pruningMode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package io.iohk.ethereum.blockchain.sync

import akka.actor.{Actor, ActorLogging, ActorRef, Props}
import akka.util.ByteString
import io.iohk.ethereum.db.storage.EvmCodeStorage
import io.iohk.ethereum.domain.{BlockHeader, Blockchain}
import io.iohk.ethereum.network.PeerEventBusActor.PeerEvent.MessageFromPeer
import io.iohk.ethereum.network.PeerEventBusActor.SubscriptionClassifier.MessageClassifier
Expand All @@ -20,6 +21,7 @@ import io.iohk.ethereum.network.p2p.messages.Codes
*/
class BlockchainHostActor(
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect it will happen more during the refactoring that actors will have access to the storages directly? (To clarify: I don't really see a problem with it.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think might happen for state storage also as the Blockchain class does not really bring anything in term of read access (I think it's ok for read access).

We will probably want write access to be through a single service though, or redefined what we call a storage to be a class that manage several rocksdb namespace internally, because we have some writes that span several storages (for instance when adding a block we want to store the block itself but also update the transaction mapping and potentially some other metadata).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

peerConfiguration: PeerConfiguration,
peerEventBusActor: ActorRef,
etcPeerManagerActor: ActorRef
Expand Down Expand Up @@ -54,7 +56,7 @@ class BlockchainHostActor(
val maybeMptNodeData = blockchain.getMptNodeByHash(hash).map(e => e.toBytes: ByteString)

//If no mpt node was found, fetch evm by hash
maybeMptNodeData.orElse(blockchain.getEvmCodeByHash(hash))
maybeMptNodeData.orElse(evmCodeStorage.get(hash))
}

Some(NodeData(nodeData))
Expand Down Expand Up @@ -116,10 +118,13 @@ object BlockchainHostActor {

def props(
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
peerConfiguration: PeerConfiguration,
peerEventBusActor: ActorRef,
etcPeerManagerActor: ActorRef
): Props =
Props(new BlockchainHostActor(blockchain, peerConfiguration, peerEventBusActor, etcPeerManagerActor))
Props(
new BlockchainHostActor(blockchain, evmCodeStorage, peerConfiguration, peerEventBusActor, etcPeerManagerActor)
)

}
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@ import akka.actor.{Actor, ActorLogging, ActorRef, PoisonPill, Props, Scheduler}
import io.iohk.ethereum.blockchain.sync.fast.FastSync
import io.iohk.ethereum.blockchain.sync.regular.RegularSync
import io.iohk.ethereum.consensus.validators.Validators
import io.iohk.ethereum.db.storage.{AppStateStorage, FastSyncStateStorage}
import io.iohk.ethereum.db.storage.{AppStateStorage, EvmCodeStorage, FastSyncStateStorage, NodeStorage}
import io.iohk.ethereum.domain.Blockchain
import io.iohk.ethereum.ledger.Ledger
import io.iohk.ethereum.utils.Config.SyncConfig

class SyncController(
appStateStorage: AppStateStorage,
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
nodeStorage: NodeStorage,
fastSyncStateStorage: FastSyncStateStorage,
ledger: Ledger,
validators: Validators,
Expand Down Expand Up @@ -77,6 +79,8 @@ class SyncController(
fastSyncStateStorage,
appStateStorage,
blockchain,
evmCodeStorage,
nodeStorage,
validators,
peerEventBus,
etcPeerManager,
Expand Down Expand Up @@ -120,6 +124,8 @@ object SyncController {
def props(
appStateStorage: AppStateStorage,
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
nodeStorage: NodeStorage,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder at what point we'll create a case class MantisEnv or something that has all these instances in it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have something similar BlockchainStorages which is extended by Storages in the trait StoragesComponent, and with a concrete implementation in DefaultStorages.

It is a big bag with every storage in it so we will probably want to separate them to have storages that work together in the same container, and identify storages that are a "view" of other storages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

syncStateStorage: FastSyncStateStorage,
ledger: Ledger,
validators: Validators,
Expand All @@ -134,6 +140,8 @@ object SyncController {
new SyncController(
appStateStorage,
blockchain,
evmCodeStorage,
nodeStorage,
syncStateStorage,
ledger,
validators,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import io.iohk.ethereum.blockchain.sync.fast.SyncStateSchedulerActor.{
WaitingForNewTargetBlock
}
import io.iohk.ethereum.consensus.validators.Validators
import io.iohk.ethereum.db.storage.{AppStateStorage, FastSyncStateStorage}
import io.iohk.ethereum.db.storage.{AppStateStorage, EvmCodeStorage, FastSyncStateStorage, NodeStorage}
import io.iohk.ethereum.domain._
import io.iohk.ethereum.mpt.MerklePatriciaTrie
import io.iohk.ethereum.network.EtcPeerManagerActor.PeerInfo
Expand Down Expand Up @@ -48,6 +48,8 @@ class FastSync(
val fastSyncStateStorage: FastSyncStateStorage,
val appStateStorage: AppStateStorage,
val blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
nodeStorage: NodeStorage,
val validators: Validators,
val peerEventBus: ActorRef,
val etcPeerManager: ActorRef,
Expand Down Expand Up @@ -153,7 +155,7 @@ class FastSync(
private val syncStateScheduler = context.actorOf(
SyncStateSchedulerActor
.props(
SyncStateScheduler(blockchain, syncConfig.stateSyncBloomFilterSize),
SyncStateScheduler(blockchain, evmCodeStorage, nodeStorage, syncConfig.stateSyncBloomFilterSize),
syncConfig,
etcPeerManager,
peerEventBus,
Expand Down Expand Up @@ -1134,6 +1136,8 @@ object FastSync {
fastSyncStateStorage: FastSyncStateStorage,
appStateStorage: AppStateStorage,
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
nodeStorage: NodeStorage,
validators: Validators,
peerEventBus: ActorRef,
etcPeerManager: ActorRef,
Expand All @@ -1146,6 +1150,8 @@ object FastSync {
fastSyncStateStorage,
appStateStorage,
blockchain,
evmCodeStorage,
nodeStorage,
validators,
peerEventBus,
etcPeerManager,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
package io.iohk.ethereum.blockchain.sync.fast

import java.util.Comparator

import akka.util.ByteString
import com.google.common.hash.{BloomFilter, Funnel, PrimitiveSink}
import io.iohk.ethereum.blockchain.sync.fast.LoadableBloomFilter.BloomFilterLoadingResult
import io.iohk.ethereum.blockchain.sync.fast.SyncStateScheduler._
import io.iohk.ethereum.db.dataSource.RocksDbDataSource.IterationError
import io.iohk.ethereum.db.storage.{EvmCodeStorage, NodeStorage}
import io.iohk.ethereum.domain.{Account, Blockchain}
import io.iohk.ethereum.mpt.{BranchNode, ExtensionNode, HashNode, LeafNode, MerklePatriciaTrie, MptNode}
import io.iohk.ethereum.network.p2p.messages.ETH63.MptNodeEncoders.MptNodeDec
import io.vavr.collection.PriorityQueue
import monix.eval.Task
import monix.reactive.Observable

import scala.annotation.tailrec
import scala.collection.immutable.ArraySeq
Expand Down Expand Up @@ -43,7 +45,11 @@ import scala.util.Try
*
* Important part is that nodes retrieved by getMissingNodes, must eventually be provided for scheduler to make progress
*/
class SyncStateScheduler(blockchain: Blockchain, bloomFilter: LoadableBloomFilter[ByteString]) {
class SyncStateScheduler(
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
bloomFilter: LoadableBloomFilter[ByteString]
) {

val loadFilterFromBlockchain: Task[BloomFilterLoadingResult] = bloomFilter.loadFromSource

Expand Down Expand Up @@ -242,9 +248,9 @@ class SyncStateScheduler(blockchain: Blockchain, bloomFilter: LoadableBloomFilte

private def isInDatabase(req: StateNodeRequest): Boolean = {
req.requestType match {
case request: CodeRequest =>
blockchain.getEvmCodeByHash(req.nodeHash).isDefined
case request: NodeRequest =>
case _: CodeRequest =>
evmCodeStorage.get(req.nodeHash).isDefined
case _: NodeRequest =>
blockchain.getMptNodeByHash(req.nodeHash).isDefined
}
}
Expand Down Expand Up @@ -284,12 +290,22 @@ object SyncStateScheduler {
BloomFilter.create[ByteString](ByteStringFunnel, expectedFilterSize)
}

def apply(blockchain: Blockchain, expectedBloomFilterSize: Int): SyncStateScheduler = {
def apply(
blockchain: Blockchain,
evmCodeStorage: EvmCodeStorage,
nodeStorage: NodeStorage,
expectedBloomFilterSize: Int
): SyncStateScheduler = {
// provided source i.e mptStateSavedKeys() is guaranteed to finish on first `Left` element which means that returned
// error is the reason why loading has stopped
val mptStateSavedKeys: Observable[Either[IterationError, ByteString]] =
(nodeStorage.storageContent.map(c => c.map(_._1)) ++ evmCodeStorage.storageContent.map(c => c.map(_._1)))
.takeWhileInclusive(_.isRight)

new SyncStateScheduler(
blockchain,
LoadableBloomFilter[ByteString](expectedBloomFilterSize, blockchain.mptStateSavedKeys())
evmCodeStorage,
LoadableBloomFilter[ByteString](expectedBloomFilterSize, mptStateSavedKeys)
)
}

Expand Down
20 changes: 0 additions & 20 deletions src/main/scala/io/iohk/ethereum/domain/Blockchain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,6 @@ trait Blockchain {
*/
def getReceiptsByHash(blockhash: ByteString): Option[Seq[Receipt]]

/**
* Returns EVM code searched by it's hash
* @param hash Code Hash
* @return EVM code if found
*/
def getEvmCodeByHash(hash: ByteString): Option[ByteString]

/**
* Returns MPT node searched by it's hash
* @param hash Node Hash
Expand Down Expand Up @@ -207,8 +200,6 @@ trait Blockchain {
ethCompatibleStorage: Boolean
): WS

def mptStateSavedKeys(): Observable[Either[IterationError, ByteString]]

/**
* Strict check if given block hash is in chain
* Using any of getXXXByHash is not always accurate - after restart the best block is often lower than before restart
Expand All @@ -230,7 +221,6 @@ class BlockchainImpl(
protected val blockNumberMappingStorage: BlockNumberMappingStorage,
protected val receiptStorage: ReceiptStorage,
protected val evmCodeStorage: EvmCodeStorage,
protected val nodeStorage: NodeStorage,
protected val chainWeightStorage: ChainWeightStorage,
protected val transactionMappingStorage: TransactionMappingStorage,
protected val appStateStorage: AppStateStorage,
Expand All @@ -257,8 +247,6 @@ class BlockchainImpl(

override def getReceiptsByHash(blockhash: ByteString): Option[Seq[Receipt]] = receiptStorage.get(blockhash)

override def getEvmCodeByHash(hash: ByteString): Option[ByteString] = evmCodeStorage.get(hash)

override def getChainWeightByHash(blockhash: ByteString): Option[ChainWeight] = chainWeightStorage.get(blockhash)

override def getBestBlockNumber(): BigInt = {
Expand Down Expand Up @@ -509,11 +497,6 @@ class BlockchainImpl(
}
// scalastyle:on method.length

def mptStateSavedKeys(): Observable[Either[IterationError, ByteString]] = {
(nodeStorage.storageContent.map(c => c.map(_._1)) ++ evmCodeStorage.storageContent.map(c => c.map(_._1)))
.takeWhileInclusive(_.isRight)
}

/**
* Recursive function which try to find the previous checkpoint by traversing blocks from top to the bottom.
* In case of finding the checkpoint block number, the function will finish the job and return result
Expand Down Expand Up @@ -596,8 +579,6 @@ trait BlockchainStorages {
val evmCodeStorage: EvmCodeStorage
val chainWeightStorage: ChainWeightStorage
val transactionMappingStorage: TransactionMappingStorage
val nodeStorage: NodeStorage
val pruningMode: PruningMode
val appStateStorage: AppStateStorage
val cachedNodeStorage: CachedNodeStorage
val stateStorage: StateStorage
Expand All @@ -611,7 +592,6 @@ object BlockchainImpl {
blockNumberMappingStorage = storages.blockNumberMappingStorage,
receiptStorage = storages.receiptStorage,
evmCodeStorage = storages.evmCodeStorage,
nodeStorage = storages.nodeStorage,
chainWeightStorage = storages.chainWeightStorage,
transactionMappingStorage = storages.transactionMappingStorage,
appStateStorage = storages.appStateStorage,
Expand Down
8 changes: 6 additions & 2 deletions src/main/scala/io/iohk/ethereum/nodebuilder/NodeBuilder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import io.iohk.ethereum.blockchain.sync.{Blacklist, BlockchainHostActor, CacheBa
import io.iohk.ethereum.consensus._
import io.iohk.ethereum.db.components.Storages.PruningModeComponent
import io.iohk.ethereum.db.components._
import io.iohk.ethereum.db.storage.AppStateStorage
import io.iohk.ethereum.db.storage.{AppStateStorage, EvmCodeStorage}
import io.iohk.ethereum.db.storage.pruning.PruningMode
import io.iohk.ethereum.domain._
import io.iohk.ethereum.jsonrpc.NetService.NetServiceConfig
Expand Down Expand Up @@ -270,12 +270,14 @@ trait EtcPeerManagerActorBuilder {
trait BlockchainHostBuilder {
self: ActorSystemBuilder
with BlockchainBuilder
with StorageBuilder
with PeerManagerActorBuilder
with EtcPeerManagerActorBuilder
with PeerEventBusBuilder =>

val blockchainHost: ActorRef = system.actorOf(
BlockchainHostActor.props(blockchain, peerConfiguration, peerEventBus, etcPeerManager),
BlockchainHostActor
.props(blockchain, storagesInstance.storages.evmCodeStorage, peerConfiguration, peerEventBus, etcPeerManager),
"blockchain-host"
)

Expand Down Expand Up @@ -715,6 +717,8 @@ trait SyncControllerBuilder {
SyncController.props(
storagesInstance.storages.appStateStorage,
blockchain,
storagesInstance.storages.evmCodeStorage,
storagesInstance.storages.nodeStorage,
storagesInstance.storages.fastSyncStateStorage,
ledger,
consensus.validators,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ trait TestBlockchainBuilder extends BlockchainBuilder {
blockNumberMappingStorage = storages.blockNumberMappingStorage,
receiptStorage = storages.receiptStorage,
evmCodeStorage = storages.evmCodeStorage,
nodeStorage = storages.nodeStorage,
chainWeightStorage = storages.chainWeightStorage,
transactionMappingStorage = storages.transactionMappingStorage,
appStateStorage = storages.appStateStorage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,15 @@ class BlockchainHostActorSpec extends AnyFlatSpec with Matchers {
val etcPeerManager = TestProbe()

val blockchainHost = TestActorRef(
Props(new BlockchainHostActor(blockchain, peerConf, peerEventBus.ref, etcPeerManager.ref))
Props(
new BlockchainHostActor(
blockchain,
storagesInstance.storages.evmCodeStorage,
peerConf,
peerEventBus.ref,
etcPeerManager.ref
)
)
)
}

Expand Down
Loading