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

RPC method for clearing the engine signer #10920

Merged
dvdplm merged 8 commits into from
Oct 9, 2019
Merged

RPC method for clearing the engine signer #10920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0f89768
RPC method parity_clearEngineSigner
vkomenda Apr 2, 2019
a155fc7
corrected the return type of clear_author
vkomenda Jul 25, 2019
b6105be
review comment responses and a rebase fix
vkomenda Aug 27, 2019
538334d
removed a spurrious warning
vkomenda Aug 27, 2019
c513359
moved clear_signer functionality to set_signer
vkomenda Sep 12, 2019
e798a09
Merge clear_author into MinerService::set_author.
afck Oct 8, 2019
869c3b3
Add trace logs to Clique::set_signer.
afck Oct 8, 2019
2249974
Clique: Don't lock signer multiple times.
afck Oct 8, 2019
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
2 changes: 1 addition & 1 deletion ethcore/engine/src/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ pub trait Engine: Sync + Send {
fn handle_message(&self, _message: &[u8]) -> Result<(), EngineError> { Err(EngineError::UnexpectedMessage) }

/// Register a component which signs consensus messages.
fn set_signer(&self, _signer: Box<dyn EngineSigner>) {}
fn set_signer(&self, _signer: Option<Box<dyn EngineSigner>>) {}

/// Sign using the EngineSigner, to be used for consensus tx signing.
fn sign(&self, _hash: H256) -> Result<Signature, Error> { unimplemented!() }
Expand Down
105 changes: 73 additions & 32 deletions ethcore/engines/authority-round/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1622,8 +1622,8 @@ impl Engine for AuthorityRound {
self.validators.register_client(client);
}

fn set_signer(&self, signer: Box<dyn EngineSigner>) {
*self.signer.write() = Some(signer);
fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
*self.signer.write() = signer;
}

fn sign(&self, hash: H256) -> Result<Signature, Error> {
Expand Down Expand Up @@ -1751,31 +1751,72 @@ mod tests {
fn generates_seal_and_does_not_double_propose() {
let tap = Arc::new(AccountProvider::transient_provider());
let addr1 = tap.insert_account(keccak("1").into(), &"1".into()).unwrap();
let addr2 = tap.insert_account(keccak("2").into(), &"2".into()).unwrap();

let spec = spec::new_test_round();
let engine = &*spec.engine;
let genesis_header = spec.genesis_header();
let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock().unwrap();

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
if let Seal::Regular(seal) = engine.generate_seal(&b1, &genesis_header) {
assert!(b1.clone().try_seal(engine, seal).is_ok());
// Second proposal is forbidden.
assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
} else {
panic!("block 1 not sealed");
Comment thread
niklasad1 marked this conversation as resolved.
}
}

engine.set_signer(Box::new((tap, addr2, "2".into())));
if let Seal::Regular(seal) = engine.generate_seal(&b2, &genesis_header) {
#[test]
fn generates_seal_iff_sealer_is_set() {
let tap = Arc::new(AccountProvider::transient_provider());
let addr1 = tap.insert_account(keccak("1").into(), &"1".into()).unwrap();
let spec = spec::new_test_round();
let engine = &*spec.engine;
let genesis_header = spec.genesis_header();
let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header,
last_hashes.clone(), addr1, (3141562.into(), 31415620.into()),
vec![], false)
.unwrap().close_and_lock().unwrap();
// Not a signer. A seal cannot be generated.
assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
// Become a signer.
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
if let Seal::Regular(seal) = engine.generate_seal(&b1, &genesis_header) {
assert!(b1.clone().try_seal(engine, seal).is_ok());
// Second proposal is forbidden.
assert!(engine.generate_seal(&b1, &genesis_header) == Seal::None);
} else {
panic!("block 1 not sealed");
}
// Stop being a signer.
engine.set_signer(None);
// Make a step first and then create a new block in that new step.
engine.step();
let addr2 = tap.insert_account(keccak("0").into(), &"0".into()).unwrap();
let mut header2 = genesis_header.clone();
header2.set_number(2);
header2.set_author(addr2);
header2.set_parent_hash(header2.hash());
let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &header2,
last_hashes, addr2, (3141562.into(), 31415620.into()),
vec![], false)
.unwrap().close_and_lock().unwrap();
// Not a signer. A seal cannot be generated.
assert!(engine.generate_seal(&b2, &header2) == Seal::None);
// Become a signer once more.
engine.set_signer(Some(Box::new((tap, addr2, "0".into()))));
if let Seal::Regular(seal) = engine.generate_seal(&b2, &header2) {
assert!(b2.clone().try_seal(engine, seal).is_ok());
// Second proposal is forbidden.
assert!(engine.generate_seal(&b2, &genesis_header) == Seal::None);
assert!(engine.generate_seal(&b2, &header2) == Seal::None);
} else {
panic!("block 2 not sealed");
}
}

Expand All @@ -1798,13 +1839,13 @@ mod tests {
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock().unwrap();

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
match engine.generate_seal(&b1, &genesis_header) {
Seal::None => panic!("wrong seal"),
Seal::Regular(_) => {
engine.step();

engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
match engine.generate_seal(&b2, &genesis_header) {
Seal::Regular(_) => panic!("sealed despite wrong difficulty"),
Seal::None => {}
Expand Down Expand Up @@ -1922,11 +1963,11 @@ mod tests {
assert!(aura.verify_block_family(&header, &parent_header).is_ok());
assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0);

aura.set_signer(Box::new((
aura.set_signer(Some(Box::new((
Arc::new(AccountProvider::transient_provider()),
validator2,
"".into(),
)));
))));

// Do not report on steps skipped between genesis and first block.
header.set_number(1);
Expand Down Expand Up @@ -2041,7 +2082,7 @@ mod tests {
client.add_notify(notify.clone());
engine.register_client(Arc::downgrade(&client) as _);

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));

let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();
Expand Down Expand Up @@ -2085,7 +2126,7 @@ mod tests {
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
engine.step();

Expand All @@ -2102,9 +2143,9 @@ mod tests {
let b2 = b2.close_and_lock().unwrap();

// we will now seal a block with 1tx and include the accumulated empty step message
engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
if let Seal::Regular(seal) = engine.generate_seal(&b2, &genesis_header) {
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
let empty_step2 = sealed_empty_step(engine, 2, &genesis_header.hash());
let empty_steps = ::rlp::encode_list(&vec![empty_step2]);

Expand Down Expand Up @@ -2138,14 +2179,14 @@ mod tests {
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
engine.step();

// step 3
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock().unwrap();
engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
assert_eq!(engine.generate_seal(&b2, &genesis_header), Seal::None);
engine.step();

Expand All @@ -2154,10 +2195,10 @@ mod tests {
let b3 = OpenBlock::new(engine, Default::default(), false, db3, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b3 = b3.close_and_lock().unwrap();

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
if let Seal::Regular(seal) = engine.generate_seal(&b3, &genesis_header) {
let empty_step2 = sealed_empty_step(engine, 2, &genesis_header.hash());
engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
let empty_step3 = sealed_empty_step(engine, 3, &genesis_header.hash());

let empty_steps = ::rlp::encode_list(&vec![empty_step2, empty_step3]);
Expand Down Expand Up @@ -2188,7 +2229,7 @@ mod tests {
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
engine.step();

Expand Down Expand Up @@ -2242,7 +2283,7 @@ mod tests {
);

// empty step with valid signature from incorrect proposer for step
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
let empty_steps = vec![sealed_empty_step(engine, 1, &parent_header.hash())];
set_empty_steps_seal(&mut header, 2, &signature, &empty_steps);

Expand All @@ -2252,9 +2293,9 @@ mod tests {
);

// valid empty steps
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
let empty_step2 = sealed_empty_step(engine, 2, &parent_header.hash());
engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr2, "0".into()))));
let empty_step3 = sealed_empty_step(engine, 3, &parent_header.hash());

let empty_steps = vec![empty_step2, empty_step3];
Expand Down Expand Up @@ -2298,7 +2339,7 @@ mod tests {
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));
assert_eq!(engine.generate_seal(&b1, &genesis_header), Seal::None);
engine.step();

Expand Down Expand Up @@ -2335,7 +2376,7 @@ mod tests {
let engine = &*spec.engine;

let addr1 = accounts[0];
engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), addr1, "1".into()))));

let mut header: Header = Header::default();
let empty_step = empty_step(engine, 1, &header.parent_hash());
Expand Down Expand Up @@ -2416,7 +2457,7 @@ mod tests {
header.set_author(accounts[0]);

// when
engine.set_signer(Box::new((tap.clone(), accounts[1], "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), accounts[1], "0".into()))));
let empty_steps = vec![
sealed_empty_step(&*engine, 1, &parent.hash()),
sealed_empty_step(&*engine, 1, &parent.hash()),
Expand Down Expand Up @@ -2453,9 +2494,9 @@ mod tests {
header.set_author(accounts[0]);

// when
engine.set_signer(Box::new((tap.clone(), accounts[1], "0".into())));
engine.set_signer(Some(Box::new((tap.clone(), accounts[1], "0".into()))));
let es1 = sealed_empty_step(&*engine, 1, &parent.hash());
engine.set_signer(Box::new((tap.clone(), accounts[0], "1".into())));
engine.set_signer(Some(Box::new((tap.clone(), accounts[0], "1".into()))));
let es2 = sealed_empty_step(&*engine, 2, &parent.hash());

let mut empty_steps = vec![es2, es1];
Expand Down
10 changes: 6 additions & 4 deletions ethcore/engines/basic-authority/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ impl Engine for BasicAuthority {
}
}

fn set_signer(&self, signer: Box<dyn EngineSigner>) {
*self.signer.write() = Some(signer);
fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
*self.signer.write() = signer;
}

fn sign(&self, hash: H256) -> Result<Signature, Error> {
Expand Down Expand Up @@ -269,7 +269,7 @@ mod tests {

let spec = new_test_authority();
let engine = &*spec.engine;
engine.set_signer(Box::new((Arc::new(tap), addr, "".into())));
engine.set_signer(Some(Box::new((Arc::new(tap), addr, "".into()))));
let genesis_header = spec.genesis_header();
let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
Expand All @@ -287,7 +287,9 @@ mod tests {

let engine = new_test_authority().engine;
assert_eq!(SealingState::NotReady, engine.sealing_state());
engine.set_signer(Box::new((Arc::new(tap), authority, "".into())));
engine.set_signer(Some(Box::new((Arc::new(tap), authority, "".into()))));
assert_eq!(SealingState::Ready, engine.sealing_state());
engine.set_signer(None);
assert_eq!(SealingState::NotReady, engine.sealing_state());
}
}
11 changes: 8 additions & 3 deletions ethcore/engines/clique/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,9 +762,14 @@ impl Engine for Clique {
}
}

fn set_signer(&self, signer: Box<dyn EngineSigner>) {
trace!(target: "engine", "set_signer: {}", signer.address());
Comment thread
niklasad1 marked this conversation as resolved.
*self.signer.write() = Some(signer);
fn set_signer(&self, signer: Option<Box<dyn EngineSigner>>) {
let mut current_signer = self.signer.write();
if let Some(signer) = signer.as_ref() {
trace!(target: "engine", "set_signer: {:?}", signer.address());
} else if let Some(signer) = &*current_signer {
trace!(target: "engine", "set_signer: cleared; previous signer: {:?})", signer.address());
}
*current_signer = signer;
}

fn register_client(&self, client: Weak<dyn EngineClient>) {
Expand Down
47 changes: 32 additions & 15 deletions ethcore/src/miner/miner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,21 +893,38 @@ impl miner::MinerService for Miner {
self.params.write().extra_data = extra_data;
}

fn set_author(&self, author: Author) {
self.params.write().author = author.address();

if let Author::Sealer(signer) = author {
if self.engine.sealing_state() != SealingState::External {
// Enable sealing
self.sealing.lock().enabled = true;
// --------------------------------------------------------------------------
// | NOTE Code below may require author and sealing locks |
// | (some `Engine`s call `EngineClient.update_sealing()`) |
// | Make sure to release the locks before calling that method. |
// --------------------------------------------------------------------------
self.engine.set_signer(signer);
} else {
warn!("Setting an EngineSigner while Engine does not require one.");
fn set_author<T: Into<Option<Author>>>(&self, author: T) {
let author_opt = author.into();
self.params.write().author = author_opt.as_ref().map(Author::address).unwrap_or_default();

match author_opt {
Some(Author::Sealer(signer)) => {
if self.engine.sealing_state() != SealingState::External {
// Enable sealing
self.sealing.lock().enabled = true;
// --------------------------------------------------------------------------
// | NOTE Code below may require author and sealing locks |
// | (some `Engine`s call `EngineClient.update_sealing()`) |
// | Make sure to release the locks before calling that method. |
// --------------------------------------------------------------------------
self.engine.set_signer(Some(signer));
} else {
warn!("Setting an EngineSigner while Engine does not require one.");
}
}
Some(Author::External(_address)) => (),
None => {
// Clear the author.
if self.engine.sealing_state() != SealingState::External {
// Disable sealing.
self.sealing.lock().enabled = false;
// --------------------------------------------------------------------------
// | NOTE Code below may require author and sealing locks |
// | (some `Engine`s call `EngineClient.update_sealing()`) |
// | Make sure to release the locks before calling that method. |
// --------------------------------------------------------------------------
self.engine.set_signer(None);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/miner/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ pub trait MinerService : Send + Sync {
/// Set info necessary to sign consensus messages and block authoring.
///
/// On chains where sealing is done externally (e.g. PoW) we provide only reward beneficiary.
fn set_author(&self, author: Author);
fn set_author<T: Into<Option<Author>>>(&self, author: T);

// Transaction Pool

Expand Down
4 changes: 4 additions & 0 deletions rpc/src/v1/impls/light/parity_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ impl<F: Fetch> ParitySet for ParitySetClient<F> {
Err(errors::light_unimplemented(None))
}

fn clear_engine_signer(&self) -> Result<bool> {
Err(errors::light_unimplemented(None))
}

fn set_transactions_limit(&self, _limit: usize) -> Result<bool> {
Err(errors::light_unimplemented(None))
}
Expand Down
5 changes: 5 additions & 0 deletions rpc/src/v1/impls/parity_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,11 @@ impl<C, M, U, F> ParitySet for ParitySetClient<C, M, U, F> where
Ok(true)
}

fn clear_engine_signer(&self) -> Result<bool> {
self.miner.set_author(None);
Ok(true)
}

fn add_reserved_peer(&self, peer: String) -> Result<bool> {
match self.net.add_reserved_peer(peer) {
Ok(()) => Ok(true),
Expand Down
Loading