Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix parallel transactions race-condition #10995

Merged
merged 20 commits into from
Sep 11, 2019
Merged
Show file tree
Hide file tree
Changes from 12 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions ethcore/engines/instant-seal/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ ethjson = { path = "../../../json" }
ethereum-types = "0.6.0"
keccak-hash = "0.2.0"
machine = { path = "../../machine" }
parking_lot = "0.8"
ngotchac marked this conversation as resolved.
Show resolved Hide resolved
trace = { path = "../../trace" }

[dev-dependencies]
Expand Down
15 changes: 14 additions & 1 deletion ethcore/engines/instant-seal/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// You should have received a copy of the GNU General Public License
// along with Parity Ethereum. If not, see <http://www.gnu.org/licenses/>.

use std::collections::HashSet;

use common_types::{
header::Header,
engines::{
Expand All @@ -22,13 +24,15 @@ use common_types::{
params::CommonParams,
},
errors::EthcoreError as Error,
BlockNumber,
};
use engine::Engine;
use ethjson;
use machine::{
ExecutedBlock,
Machine
};
use parking_lot::Mutex;


/// `InstantSeal` params.
Expand All @@ -51,6 +55,7 @@ impl From<ethjson::spec::InstantSealParams> for InstantSealParams {
pub struct InstantSeal {
params: InstantSealParams,
machine: Machine,
sealed_blocks: Mutex<HashSet<BlockNumber>>,
ngotchac marked this conversation as resolved.
Show resolved Hide resolved
}

impl InstantSeal {
Expand All @@ -59,6 +64,7 @@ impl InstantSeal {
InstantSeal {
params,
machine,
sealed_blocks: Mutex::new(HashSet::new()),
}
}
}
Expand All @@ -74,7 +80,14 @@ impl Engine for InstantSeal {
if block.transactions.is_empty() {
Seal::None
} else {
Seal::Regular(Vec::new())
let mut sealed_blocks = self.sealed_blocks.lock();
ngotchac marked this conversation as resolved.
Show resolved Hide resolved
let block_number = block.header.number();
if !sealed_blocks.contains(&block_number) {
sealed_blocks.insert(block_number);
Seal::Regular(Vec::new())
} else {
Seal::None
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,11 @@ impl Client {
_ => self.block_header(id).and_then(|h| h.decode().ok())
}
}

/// Maybe call `update_sealing` method on the Miner
pub fn maybe_update_sealing(&self) {
self.importer.miner.maybe_update_sealing(self);
}
}

impl DatabaseRestore for Client {
Expand Down
17 changes: 16 additions & 1 deletion ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,7 @@ impl Miner {
false
}
}

/// Prepare pending block, check whether sealing is needed, and then update sealing.
fn prepare_and_update_sealing<C: miner::BlockChainClient>(&self, chain: &C) {
use miner::MinerService;
Expand All @@ -876,6 +877,13 @@ impl Miner {
SealingState::NotReady => { self.maybe_enable_sealing(); },
}
}

/// Call `update_sealing` if needed
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
pub fn maybe_update_sealing<C: miner::BlockChainClient>(&self, chain: &C) {
if self.transaction_queue.has_local_pending_transactions() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this. The implementation is very specific for NullEngine (i.e. checks local transactions), while we have plenty of other conditions that affect when to seal. Why can't we just call update_sealing instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is still valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I thought you were talking about the next line, where I first used prepare_and_update_sealing. I just updated it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess then there is no point in adding maybe_update_sealing to the traits and just call update_sealing directly?

self.prepare_and_update_sealing(chain);
}
}
}

impl miner::MinerService for Miner {
Expand Down Expand Up @@ -1242,7 +1250,7 @@ impl miner::MinerService for Miner {
/// Update sealing if required.
/// Prepare the block and work if the Engine does not seal internally.
fn update_sealing<C>(&self, chain: &C) where
C: BlockChain + CallContract + BlockProducer + SealedBlockImporter + Nonce + Sync,
C: miner::BlockChainClient,
{
trace!(target: "miner", "update_sealing");

Expand Down Expand Up @@ -1429,15 +1437,22 @@ impl miner::MinerService for Miner {
service_transaction_checker.as_ref(),
);
queue.cull(client);
if is_internal_import {
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
chain.maybe_update_sealing();
}
};

if let Err(e) = channel.send(ClientIoMessage::<Client>::execute(cull)) {
warn!(target: "miner", "Error queueing cull: {:?}", e);
}
} else {
self.transaction_queue.cull(client);
if is_internal_import {
self.maybe_update_sealing(chain);
}
}
}

if let Some(ref service_transaction_checker) = self.service_transaction_checker {
match service_transaction_checker.refresh_cache(chain) {
Ok(true) => {
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/miner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ pub trait MinerService : Send + Sync {

/// Update current pending block
fn update_sealing<C>(&self, chain: &C)
where C: BlockChain + CallContract + BlockProducer + SealedBlockImporter + Nonce + Sync;
where C: BlockChainClient;
ngotchac marked this conversation as resolved.
Show resolved Hide resolved

// Notifications

Expand Down
1 change: 1 addition & 0 deletions miner/src/pool/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ impl TransactionQueue {
let state_readiness = ready::State::new(client.clone(), stale_id, nonce_cap);
removed += self.pool.write().cull(Some(chunk), state_readiness);
}
self.cached_pending.write().clear();
ngotchac marked this conversation as resolved.
Show resolved Hide resolved
debug!(target: "txqueue", "Removed {} stalled transactions. {}", removed, self.status());
}

Expand Down