-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
ethcore/src/blockchain/best_block.rs
Outdated
@@ -19,7 +19,8 @@ use bigint::hash::H256; | |||
use bytes::Bytes; | |||
use header::BlockNumber; | |||
|
|||
/// Best block info. | |||
/// Best block info. Best block is the block with highest combined difficulty | |||
/// (usually the block with the highest block number). Sometimes refered as latest block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best block heuristic is really dependent on the consensus engine (although integrating that into the blockchain structure here is mostly a TODO). this new description is only applicable to the GHOST fork-choice rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I rollback the comment entirely, or provide some extra text like this:
/// Contains information on a best block that is specific to the consensus engine.
///
/// For GHOST fork-choice rule it would typically describe the block with highest
/// combined difficulty (usually the block with the highest block number).
///
/// Sometimes refered as 'latest block'.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text like that is fine. a couple nits, though: combined
-> cumulative
and the last line about sometimes being referred to as the latest block is correct, but those would be misuses of the term, so I'd recommend omitting that statement.
/// For example, auto-updater will disable client forever if there is a | ||
/// hard fork registered on-chain that we don't have capability for. | ||
/// When hard fork block rolls around, the client (if `update` is false) | ||
/// knows it can't proceed further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fields here are not public. comments are great, but they should be //
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason to enforce using //
. In multiple places in our codebase we use //
for private properties and structs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RLS provides hover hints for fields commented with ///
only.
ethcore/src/trace/db.rs
Outdated
/// Whenever a transaction is executed by evm it's execution trace is stored | ||
/// in trace database. Each trace has information, which contracts have been | ||
/// touched, which have been created during the execution of transaction, and | ||
/// which calls failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multi-line docs should have a single line first, then a gap, and then the rest. it formats much more nicely on the generated docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried, but failed to describe the whole thing in one compact line for a header. Or should I reformat the very same text but with an extra blank line?
On the other hand it would not be wise to write meaningless first line that Captain Obvious would be proud of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an obvious line like "Block and transaction execution trace database" gives more information than the struct name while allowing for more information to be included later.
b91ec0c
to
a76f404
Compare
OK, at this point most of import related data and some of the logic were moved to the Right now I'm trying to move Unfortunately, I do not see an easy way to resolve the issue. We need either to rework the whole trait hierarchy, or to edit the @debris, @rphmeier, do you have something in mind on this issue? |
I opt for reworking the trait hierarchy. Especially |
@debris You don't know what data will be needed by Miner beforehand - you need to recover the signatures of transactions to figure out what account balances you need for instance. Can you elaborate more on what's wrong with |
@tomusdrw you're right. It's not trivial.
Yes, I think that's the main issue now. |
Well, since |
Here's complete list of client methods that are used by From fn best_block_header(&self) -> encoded::Header;
fn block(&self, id: BlockId) -> Option<encoded::Block>;
fn latest_nonce(&self, address: &Address) -> U256;
fn latest_balance(&self, address: &Address) -> U256;
fn chain_info(&self) -> BlockChainInfo;
fn call_contract(&self, id: BlockId, address: Address, data: Bytes) -> Result<Bytes, String>;
fn transaction_block(&self, id: TransactionId) -> Option<H256>;
fn registry_address(&self, name: String) -> Option<Address>; From fn reopen_block(&self, block: ClosedBlock) -> OpenBlock;
fn prepare_open_block(&self, author: Address, gas_range_target: (U256, U256), extra_data: Bytes) -> OpenBlock; Maybe we should create special trait that |
Fair enough, I though that the trait was separate. IMHO we should duplicate methods in the A blanket implementation might also work, but that needs to be checked in code. |
Probably the whole enum should be renamed to `Strategy` or something alike.
Finally, trait hierarchy was modified according to @tomusdrw comment: #7038 (review) Currently, Now it looks like this: /// New chain head event. Restart mining operation.
fn update_sealing<C>(&self, chain: &C)
where C: AccountData + BlockChain + RegistryInfo
+ CallContract + BlockProducer + SealedBlockImporter;
/// Submit `seal` as a valid solution for the header of `pow_hash`.
/// Will check the seal, but not actually insert the block into the chain.
fn submit_seal<C: SealedBlockImporter>(&self, chain: &C, pow_hash: H256, seal: Vec<Bytes>) -> Result<(), Error>;
/// Get the sealing work package and if `Some`, apply some transform.
fn map_sealing_work<C, F, T>(&self, client: &C, f: F) -> Option<T>
where C: AccountData + BlockChain + BlockProducer + CallContract,
F: FnOnce(&ClosedBlock) -> T,
Self: Sized; In my opinion it's good that just by looking at a method declaration you already may have an idea what it does and what permissions it require. However, there are some issues remaining. And most of them are due to wide use of trait objects in the codebase:
Aside of that PR looks ready for me. All tests pass. |
# Conflicts: # ethcore/src/client/client.rs # ethcore/src/miner/miner.rs
Even if review would be successful I do not recommend merging it, before I run some tests on Aura and other chains. |
OK, I performed tests using Aura as well as the Foundation chains. Looks like all is working as expected, so we may probably consider this branch safe to merge when review would be completed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pr is huge, but generally, it looks good to me
Err(e) => { | ||
warn!(target: "client", "Failed to generate transition proof for block {}: {}", hash, e); | ||
warn!(target: "client", "Snapshots produced by this client may be incomplete"); | ||
Vec::new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if client
failed to generate proof
, why do we insert an invalid proof (Vec::new()
) into chain? line 644
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm. I see that this code hasn't been changed in this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to answer the original question, we could also bail and return an error. But this code-path indicates that either the validator contract or the local database are broken so whatever we do the chain is going to break at some point.
vec![], | ||
vec![], | ||
vec![], | ||
vec![block.rlp_bytes()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these values be empty vectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, I see that it hasn't been changed in this pr
ethcore/src/client/client.rs
Outdated
|
||
impl RegistryInfo for Client { | ||
fn registry_address(&self, name: String, block: BlockId) -> Option<Address> { | ||
let address = match self.registrar_address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest version of rust supports ?
for options, so you can write:
let address = self.registrar_address?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point.
@debris could you label it accordingly? Is this a "looksgood" or "mustngrumble"? Or does this require additional work? |
looksgood and it requires additional review |
ethcore/src/client/traits.rs
Outdated
} | ||
|
||
/// Provides various blockchain information, like block header, chain state etc. | ||
pub trait BlockChain: ChainInfo + BlockInfo + TransactionInfo /* + StateClient */ {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the StateClient
bound desired? Why is it commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it's an artifact. Previously I thought, that BlockChain
should bound on StateClient
but it wasn't the case.
ethcore/src/client/client.rs
Outdated
self.state_db.read().boxed_clone_canon(&header.hash()), | ||
header.state_root(), | ||
self.engine.account_start_nonce(header.number()), | ||
self.factories.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: close paren should go on following line, with one level less indentation
* Improves `BestBlock` comment * Improves `TraceDB` comment * Improves `journaldb::Algorithm` comment. Probably the whole enum should be renamed to `Strategy` or something alike. * Comments some of the `Client`'s fields * Deglobs client imports * Fixes comments * Extracts `import_lock` to `Importer` struct * Extracts `verifier` to `Importer` struct * Extracts `block_queue` to `Importer` struct * Extracts `miner` to `Importer` struct * Extracts `ancient_verifier` to `Importer` struct * Extracts `rng` to `Importer` struct * Extracts `import_old_block` to `Importer` struct * Adds `Nonce` trait * Adds `Balance` trait * Adds `ChainInfo` trait * Fixes imports for tests using `chain_info` method * Adds `BlockInfo` trait * Adds more `ChainInfo` imports * Adds `BlockInfo` imports * Adds `ReopenBlock` trait * Adds `PrepareOpenBlock` trait * Fixes import in tests * Adds `CallContract` trait * Fixes imports in tests using `call_contract` method * Adds `TransactionInfo` trait * Adds `RegistryInfo` trait * Fixes imports in tests using `registry_address` method * Adds `ScheduleInfo` trait * Adds `ImportSealedBlock` trait * Fixes imports in test using `import_sealed_block` method * Adds `BroadcastProposalBlock` trait * Migrates `Miner` to static dispatch * Fixes tests * Moves `calculate_enacted_retracted` to `Importer` * Moves import-related methods to `Importer` * Removes redundant `import_old_block` wrapper * Extracts `import_block*` into separate trait * Fixes tests * Handles `Pending` in `LightFetch` * Handles `Pending` in filters * Handles `Pending` in `ParityClient` * Handles `Pending` in `EthClient` * Removes `BlockId::Pending`, partly refactors dependent code * Adds `StateInfo` trait * Exports `StateOrBlock` and `BlockChain` types from `client` module * Refactors `balance` RPC using generic API * Refactors `storage_at` RPC using generic API * Makes `MinerService::pending_state`'s return type dynamic * Adds `StateOrBlock` and `BlockChain` types * Adds impl of `client::BlockChain` for `Client` * Exports `StateInfo` trait from `client` module * Missing `self` use To be fixed up to "Adds impl of `client::BlockChain` for `Client`" * Adds `number_to_id` and refactors dependent RPC methods * Refactors `code_at` using generic API * Adds `StateClient` trait * Refactors RPC to use `StateClient` trait * Reverts `client::BlockChain` trait stuff, refactors methods to accept `StateOrBlock` * Refactors TestClient * Adds helper function `block_number_to_id` * Uses `block_number_to_id` instead of local function * Handles `Pending` in `list_accounts` and `list_storage_keys` * Attempt to use associated types for state instead of trait objects * Simplifies `state_at_beginning` * Extracts `call` and `call_many` into separate trait * Refactors `build_last_hashes` to accept reference * Exports `Call` type from the module * Refactors `call` and `call_many` to accept state and header * Exports `state_at` in `StateClient` * Exports `pending_block_header` from `MinerService` * Refactors RPC `call` method using new API * Adds missing parentheses * Refactors `parity::call` to use new call API * Update .gitlab-ci.yml fix gitlab lint * Fixes error handling * Refactors `traces::call` and `call_many` to use new call API * Refactors `call_contract` * Refactors `block_header` * Refactors internal RPC method `block` * Moves `estimate_gas` to `Call` trait, refactors parameters * Refactors `estimate_gas` in RPC * Refactors `uncle` * Refactors RPC `transaction` * Covers missing branches * Makes it all compile, fixes compiler grumbles * Adds casts in `blockchain` module * Fixes `PendingBlock` tests, work on `MinerService` * Adds test stubs for StateClient and EngineInfo * Makes `state_db` public * Adds missing impls for `TestBlockChainClient` * Adds trait documentation * Adds missing docs to the `state_db` module * Fixes trivial compilation errors * Moves `code_hash` method to a `BlockInfo` trait * Refactors `Verifier` to be generic over client * Refactors `TransactionFilter` to be generic over client * Refactors `Miner` and `Client` to reflect changes in verifier and txfilter API * Moves `ServiceTransactionChecker` back to `ethcore` * Fixes trait bounds in `Miner` API * Fixes `Client` * Fixes lifetime bound in `FullFamilyParams` * Adds comments to `FullFamilyParams` * Fixes imports in `ethcore` * Fixes BlockNumber handling in `code_at` and `replay_block_transactions` * fix compile issues * First step to redundant trait merge * Fixes compilation error in RPC tests * Adds mock `State` as a stub for `TestClient` * Handles `StateOrBlock::State` in `TestBlockChainClient::balance` * Fixes `transaction_count` RPC * Fixes `transaction_count` * Moves `service_transaction.json` to the `contracts` subfolder * Fixes compilation errors in tests * Refactors client to use `AccountData` * Refactors client to use `BlockChain` * Refactors miner to use aggregate traits * Adds `SealedBlockImporter` trait * Refactors miner to use `SealedBlockImporter` trait * Removes unused imports * Simplifies `RegistryInfo::registry_address` * Fixes indentation * Removes commented out trait bound
* Echo back the message hash of a ping in the pong request * Fixed broken link in README (#8012) * Fixed broken link in README * Updated wiki link * [hardware wallet] sleeping -> pollling (#8018) * Use polling, enable missing doc warnings & docs * make try_connect_polling() a free function * `Client` refactoring (#7038) * Improves `BestBlock` comment * Improves `TraceDB` comment * Improves `journaldb::Algorithm` comment. Probably the whole enum should be renamed to `Strategy` or something alike. * Comments some of the `Client`'s fields * Deglobs client imports * Fixes comments * Extracts `import_lock` to `Importer` struct * Extracts `verifier` to `Importer` struct * Extracts `block_queue` to `Importer` struct * Extracts `miner` to `Importer` struct * Extracts `ancient_verifier` to `Importer` struct * Extracts `rng` to `Importer` struct * Extracts `import_old_block` to `Importer` struct * Adds `Nonce` trait * Adds `Balance` trait * Adds `ChainInfo` trait * Fixes imports for tests using `chain_info` method * Adds `BlockInfo` trait * Adds more `ChainInfo` imports * Adds `BlockInfo` imports * Adds `ReopenBlock` trait * Adds `PrepareOpenBlock` trait * Fixes import in tests * Adds `CallContract` trait * Fixes imports in tests using `call_contract` method * Adds `TransactionInfo` trait * Adds `RegistryInfo` trait * Fixes imports in tests using `registry_address` method * Adds `ScheduleInfo` trait * Adds `ImportSealedBlock` trait * Fixes imports in test using `import_sealed_block` method * Adds `BroadcastProposalBlock` trait * Migrates `Miner` to static dispatch * Fixes tests * Moves `calculate_enacted_retracted` to `Importer` * Moves import-related methods to `Importer` * Removes redundant `import_old_block` wrapper * Extracts `import_block*` into separate trait * Fixes tests * Handles `Pending` in `LightFetch` * Handles `Pending` in filters * Handles `Pending` in `ParityClient` * Handles `Pending` in `EthClient` * Removes `BlockId::Pending`, partly refactors dependent code * Adds `StateInfo` trait * Exports `StateOrBlock` and `BlockChain` types from `client` module * Refactors `balance` RPC using generic API * Refactors `storage_at` RPC using generic API * Makes `MinerService::pending_state`'s return type dynamic * Adds `StateOrBlock` and `BlockChain` types * Adds impl of `client::BlockChain` for `Client` * Exports `StateInfo` trait from `client` module * Missing `self` use To be fixed up to "Adds impl of `client::BlockChain` for `Client`" * Adds `number_to_id` and refactors dependent RPC methods * Refactors `code_at` using generic API * Adds `StateClient` trait * Refactors RPC to use `StateClient` trait * Reverts `client::BlockChain` trait stuff, refactors methods to accept `StateOrBlock` * Refactors TestClient * Adds helper function `block_number_to_id` * Uses `block_number_to_id` instead of local function * Handles `Pending` in `list_accounts` and `list_storage_keys` * Attempt to use associated types for state instead of trait objects * Simplifies `state_at_beginning` * Extracts `call` and `call_many` into separate trait * Refactors `build_last_hashes` to accept reference * Exports `Call` type from the module * Refactors `call` and `call_many` to accept state and header * Exports `state_at` in `StateClient` * Exports `pending_block_header` from `MinerService` * Refactors RPC `call` method using new API * Adds missing parentheses * Refactors `parity::call` to use new call API * Update .gitlab-ci.yml fix gitlab lint * Fixes error handling * Refactors `traces::call` and `call_many` to use new call API * Refactors `call_contract` * Refactors `block_header` * Refactors internal RPC method `block` * Moves `estimate_gas` to `Call` trait, refactors parameters * Refactors `estimate_gas` in RPC * Refactors `uncle` * Refactors RPC `transaction` * Covers missing branches * Makes it all compile, fixes compiler grumbles * Adds casts in `blockchain` module * Fixes `PendingBlock` tests, work on `MinerService` * Adds test stubs for StateClient and EngineInfo * Makes `state_db` public * Adds missing impls for `TestBlockChainClient` * Adds trait documentation * Adds missing docs to the `state_db` module * Fixes trivial compilation errors * Moves `code_hash` method to a `BlockInfo` trait * Refactors `Verifier` to be generic over client * Refactors `TransactionFilter` to be generic over client * Refactors `Miner` and `Client` to reflect changes in verifier and txfilter API * Moves `ServiceTransactionChecker` back to `ethcore` * Fixes trait bounds in `Miner` API * Fixes `Client` * Fixes lifetime bound in `FullFamilyParams` * Adds comments to `FullFamilyParams` * Fixes imports in `ethcore` * Fixes BlockNumber handling in `code_at` and `replay_block_transactions` * fix compile issues * First step to redundant trait merge * Fixes compilation error in RPC tests * Adds mock `State` as a stub for `TestClient` * Handles `StateOrBlock::State` in `TestBlockChainClient::balance` * Fixes `transaction_count` RPC * Fixes `transaction_count` * Moves `service_transaction.json` to the `contracts` subfolder * Fixes compilation errors in tests * Refactors client to use `AccountData` * Refactors client to use `BlockChain` * Refactors miner to use aggregate traits * Adds `SealedBlockImporter` trait * Refactors miner to use `SealedBlockImporter` trait * Removes unused imports * Simplifies `RegistryInfo::registry_address` * Fixes indentation * Removes commented out trait bound * Bump master to 1.11.0 (#8021) * Bump master to 1.11.0 * Bump price-info * Bump mac installer version * Fix gitlab builds * Add MCIP-6 Byzyantium transition to Musicoin spec (#7841) * Add test chain spec for musicoin byzantium testnet * Add MCIP-6 Byzyantium transition to Musicoin spec * Update mcip6_byz.json * ethcore: update musicoin byzantium block number * ethcore: update musicoin byzantium block number * ethcore: update musicoin bootnodes * Update musicoin.json * Update musicoin.json * More bootnodes. * prelude to the block module cleanup (#8025) * prelude to block cleanup * fixed tests * fix cache & snapcraft CI build (#8052) after successful testing it is necessary to port in a ```beta``` and ```stable``` * Update refs to shell (#8051) * Abstract devp2p (#8048) * Rename ethcore-network to ethcore-network-devp2p * Fix typo * Extract generic traits into util/network * Simplify util/network * Fix devp2p tests * Remove old feature * Fix RPC tests * Change port because testing environment didn't like those ports
Affects #6136.
The main idea is to decouple
Importer
from theClient
. Also minor tweaks are to be performed to enhance code readability.This PR is very much in progress.
This change is