From 7bcffc0a8fab90e45a993ddabc4edcea04f99e9c Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 12 Dec 2019 17:03:49 +0100 Subject: [PATCH 1/6] tx-q: basic verification of local transactions --- ethcore/src/miner/pool_client.rs | 5 +++++ miner/src/pool/client.rs | 9 +++++++++ miner/src/pool/tests/client.rs | 6 ++++++ miner/src/pool/verifier.rs | 8 +++++++- 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/ethcore/src/miner/pool_client.rs b/ethcore/src/miner/pool_client.rs index 8b8740dff5d..5004773dd87 100644 --- a/ethcore/src/miner/pool_client.rs +++ b/ethcore/src/miner/pool_client.rs @@ -139,6 +139,11 @@ impl<'a, C: 'a> pool::client::Client for PoolClient<'a, C> where self.chain.transaction_block(TransactionId::Hash(*hash)).is_some() } + fn verify_transaction_basic(&self, tx: &UnverifiedTransaction)-> Result<(), transaction::Error> { + self.engine.verify_transaction_basic(tx, &self.best_block_header)?; + Ok(()) + } + fn verify_transaction(&self, tx: UnverifiedTransaction)-> Result { self.engine.verify_transaction_basic(&tx, &self.best_block_header)?; let tx = tx.verify_unordered()?; diff --git a/miner/src/pool/client.rs b/miner/src/pool/client.rs index bcd2b968954..1579ba40daa 100644 --- a/miner/src/pool/client.rs +++ b/miner/src/pool/client.rs @@ -50,6 +50,15 @@ pub trait Client: fmt::Debug + Sync { /// Is transaction with given hash already in the blockchain? fn transaction_already_included(&self, hash: &H256) -> bool; + /// Perform basic/cheap transaction verification. + /// + /// This should include all cheap checks that can be done before + /// actually checking the signature, like chain-replay protection. + /// + /// This method is currently used only for verifying local transactions. + fn verify_transaction_basic(&self, t: &transaction::UnverifiedTransaction) + -> Result<(), transaction::Error>; + /// Structurarily verify given transaction. fn verify_transaction(&self, tx: transaction::UnverifiedTransaction) -> Result; diff --git a/miner/src/pool/tests/client.rs b/miner/src/pool/tests/client.rs index 4735fbd64d4..ce927c8e983 100644 --- a/miner/src/pool/tests/client.rs +++ b/miner/src/pool/tests/client.rs @@ -103,6 +103,12 @@ impl pool::client::Client for TestClient { false } + fn verify_transaction_basic(&self, _tx: &UnverifiedTransaction) + -> Result<(), transaction::Error> + { + Ok(()) + } + fn verify_transaction(&self, tx: UnverifiedTransaction) -> Result { diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index 79d50d7138f..a6b57f4d9e3 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -250,7 +250,13 @@ impl txpool::Verifier for Verifier tx, + Transaction::Local(tx) => match self.client.verify_transaction_basic(&**tx) { + Ok(()) => tx, + Err(err) => { + debug!(target: "txqueue", "[{:?}] Rejected local tx {:?}", hash, err); + return Err(err) + } + }, }; // Verify RLP payload From 16c57787d10252ed11908bd4a5cbddb9bc2e8ce9 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 12 Dec 2019 17:58:11 +0100 Subject: [PATCH 2/6] miner: basic test for local import --- ethcore/src/miner/miner.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 997ceb1bccf..e48cf94dc2d 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -1695,6 +1695,30 @@ mod tests { assert_eq!(miner.prepare_pending_block(&client), BlockPreparationStatus::NotPrepared); } + #[test] + fn should_reject_local_transaction_with_invalid_chain_id() { + let spec = spec::new_test(); + let miner = Miner::new_for_tests(&spec, None); + let client = TestBlockChainClient::default(); + let chain_id = spec.chain_id(); + + // chain_id + 100500 is invalid + let import = miner.import_claimed_local_transaction( + &client, + PendingTransaction::new(transaction_with_chain_id(chain_id + 10500), None), + false, + ); + assert!(import.is_err()); + + // chain_id is valid + let import = miner.import_claimed_local_transaction( + &client, + PendingTransaction::new(transaction_with_chain_id(chain_id), None), + false, + ); + assert_eq!(import, Ok(())); + } + #[test] fn should_prioritize_locals() { let client = TestBlockChainClient::default(); From f32cf2f34788f8acf2c059f7f1b6241a8e585dd2 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 12 Dec 2019 22:39:20 +0100 Subject: [PATCH 3/6] miner: info log when rejecting local txn --- miner/src/pool/verifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index a6b57f4d9e3..6bbd23aa89d 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -253,7 +253,7 @@ impl txpool::Verifier for Verifier match self.client.verify_transaction_basic(&**tx) { Ok(()) => tx, Err(err) => { - debug!(target: "txqueue", "[{:?}] Rejected local tx {:?}", hash, err); + info!(target: "txqueue", "[{:?}] Rejected local tx {:?}", hash, err); return Err(err) } }, From e5eb8c3d8c181de5dc350b23727395416588426b Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 12 Dec 2019 22:40:36 +0100 Subject: [PATCH 4/6] Hernandofmt Co-Authored-By: Hernando Castano --- ethcore/src/miner/miner.rs | 4 ++-- ethcore/src/miner/pool_client.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index e48cf94dc2d..c9f515cb3b2 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -1705,7 +1705,7 @@ mod tests { // chain_id + 100500 is invalid let import = miner.import_claimed_local_transaction( &client, - PendingTransaction::new(transaction_with_chain_id(chain_id + 10500), None), + PendingTransaction::new(transaction_with_chain_id(chain_id + 10500), None), false, ); assert!(import.is_err()); @@ -1713,7 +1713,7 @@ mod tests { // chain_id is valid let import = miner.import_claimed_local_transaction( &client, - PendingTransaction::new(transaction_with_chain_id(chain_id), None), + PendingTransaction::new(transaction_with_chain_id(chain_id), None), false, ); assert_eq!(import, Ok(())); diff --git a/ethcore/src/miner/pool_client.rs b/ethcore/src/miner/pool_client.rs index 5004773dd87..61bdaad00c3 100644 --- a/ethcore/src/miner/pool_client.rs +++ b/ethcore/src/miner/pool_client.rs @@ -139,12 +139,12 @@ impl<'a, C: 'a> pool::client::Client for PoolClient<'a, C> where self.chain.transaction_block(TransactionId::Hash(*hash)).is_some() } - fn verify_transaction_basic(&self, tx: &UnverifiedTransaction)-> Result<(), transaction::Error> { + fn verify_transaction_basic(&self, tx: &UnverifiedTransaction) -> Result<(), transaction::Error> { self.engine.verify_transaction_basic(tx, &self.best_block_header)?; Ok(()) } - fn verify_transaction(&self, tx: UnverifiedTransaction)-> Result { + fn verify_transaction(&self, tx: UnverifiedTransaction) -> Result { self.engine.verify_transaction_basic(&tx, &self.best_block_header)?; let tx = tx.verify_unordered()?; From c8768ce6141a90b9eb648a3c2be5cfd7190342b2 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 13 Dec 2019 11:41:55 +0100 Subject: [PATCH 5/6] miner: assert in a test with the concrete error type --- ethcore/src/miner/miner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index c9f515cb3b2..e3d3e04425f 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -1708,7 +1708,7 @@ mod tests { PendingTransaction::new(transaction_with_chain_id(chain_id + 10500), None), false, ); - assert!(import.is_err()); + assert_eq!(import, Err(transaction::Error::InvalidChainId)); // chain_id is valid let import = miner.import_claimed_local_transaction( From 1d4d1b579066061778bebd6a892aa179d3578ebb Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 13 Dec 2019 18:56:10 +0100 Subject: [PATCH 6/6] tx-q: info! -> warn! on local tx rejection --- miner/src/pool/verifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index 6bbd23aa89d..ae64ac5223c 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -253,7 +253,7 @@ impl txpool::Verifier for Verifier match self.client.verify_transaction_basic(&**tx) { Ok(()) => tx, Err(err) => { - info!(target: "txqueue", "[{:?}] Rejected local tx {:?}", hash, err); + warn!(target: "txqueue", "[{:?}] Rejected local tx {:?}", hash, err); return Err(err) } },