Closed rphmeier closed 6 years ago
(this is sort of a stream-of-consciousness write-up, just going through the things which seem the most fragile/hacky in the core code for now)
I couldn’t agree more! I would also add decoupling BlockChainClient
and database. Our tests, where it's not necessary, should not use io at all
@debris Good point, that's done in #4566.
Also relevant: #4591
What's the take on this?
I will close this pr as most post of the points have been addressed by independent pull requests.
Motivation:
Parity was originally written with purely the Ethereum chain in mind -- basic synchronization and mining, with an RPC layer over-top of it. We have now evolved numerous consensus engines, each with their own demands. The light client is on the way and warp sync complicates things further. We've grown a UI which adapts to the operating mode of the client. While developing these features, we have taken on a not-insignificant amount of technical debt. These are the key issues which I feel we currently face:
Common Transaction Pool Interface
Decouple Transaction Queue from Miner/Client
&Mutex<TransactionQueue>
as a parameter to updating sealing.Decouple Client and Miner
Client
andMiner
doesn’t represent their roles correctly.Miner
should hold a reference toClient
, but this isn't strictly necessary.ChainNotify
implementation which ties together anArc<TransactionPool>
,Arc<Miner>
, and anArc<BlockProvider>
to update the pool with retracted TXsDecouple Miner and Consensus
AwaitingSeal
,BlockProposal(proposal)
which would then be broadcast, rather than triggering broadcast immediately.Client Traits Refactoring
BlockChainClient: BlockProvider
to reduce duplication. RequiresBlockId
match withinBlockChain
but that’s fine.BlockChainClient
is to provide an inspectableState
at a givenBlockId
. ThisState
object should be a publicly-facing encapsulation ofethcore::state::State
.ChainImport
trait for queuing blocks, importing sealed blocks, queue management.nonce
,balance
,call
,estimate_gas
, etc. without duplication of code.Sealer
changes.BlockChainClient
trait will no longer be sufficient for syncing. But another IPC-enabled trait which wraps a Client, ChainImport, and TransactionQueue would be. This trait could reside entirely withinethsync
, allowing IPC codegen to run there exclusively.Generalize Client
SnapshotService
. For light nodes it might be the canonical cache (although this as well could reside within the client).Consensus Engines
EngineClient
: this has nothing to do with a block chain client.EngineSigner
is a good step forwards, but it should be set not viaSealer
but through the engine directly. Decouple Sealer from AccountProvider.Engine
abstraction reside belowClient
in the crate hierarchy, with implementations in their own crates.What does this enable?
cc @ethcore/core-devs for feedback.