I172 service transactions garbage collector + more

CHANGELOG.md

@@ -1,14 +1,21 @@
 ## Diamond Node Software 4.0.2
 
-OPTIONAL Update
 
-New behavior for validator nodes as main feature
+### New behavior for validator nodes
 - [Autoshutdown if a Node becomes a regular Node](https://github.com/DMDcoin/diamond-node/issues/322)
+- [Remove empty blocks during key gen phases behaviour](https://github.com/DMDcoin/diamond-node/issues/327)
+- [Service Transaction cleanup (garbage collect)](https://github.com/DMDcoin/diamond-node/issues/172) 
 
-Further stability improvements
+### RPC
+- [Gas price from contracts](https://github.com/DMDcoin/diamond-node/issues/159)
+
+### Further stability improvements
 - [FIXED: received transactions are getting pooled, if announced by another peer](https://github.com/DMDcoin/diamond-node/issues/304)
 - [FIXED: dropped transactions are getting pooled](https://github.com/DMDcoin/diamond-node/issues/303)
 - [FIXED: key generation can panic if faulty validators write malicious parts](https://github.com/DMDcoin/diamond-node/issues/100)
+- [FIXED: already included transactions are refretched from peers](https://github.com/DMDcoin/diamond-node/issues/196)
+- [Gracefull Node Shutdown: increase to 15 seconds](https://github.com/DMDcoin/diamond-node/issues/321)
+
 
 ## Diamond Node Software 4.0.1
 

crates/concensus/miner/src/pool/queue.rs

@@ -41,6 +41,8 @@ use crate::pool::{
     verifier, PendingOrdering, PendingSettings, PrioritizationStrategy,
 };
 
+use super::VerifiedTransaction;
+
 type Listener = (
     LocalTransactionsList,
     (listener::Notifier, listener::Logger),
@@ -413,6 +415,17 @@ impl TransactionQueue {
             .collect()
     }
 
+    /// Performs garbage collection of the pool of this transactionqueue for free service transactions.
+    /// Removes transaction that are not valid anymore.
+    /// The process executes listener calls.
+    pub fn garbage_collect<F: Fn(&VerifiedTransaction) -> bool>(
+        &self,
+        service_transaction_check: F,
+    ) {
+        let mut pool = self.pool.write();
+        pool.garbage_collect(service_transaction_check);
+    }
+
     /// Computes unordered set of pending hashes.
     ///
     /// Since strict nonce-checking is not required, you may get some false positive future transactions as well.

crates/ethcore/service/src/service.rs

@@ -233,7 +233,9 @@ impl IoHandler<ClientIoMessage> for ClientIoHandler {
             ClientIoMessage::Execute(ref exec) => {
                 (*exec.0)(&self.client);
             }
-            _ => {} // ignore other messages
+            ClientIoMessage::NewChainHead => {
+                self.client.garbage_collect_in_queue();
+            }
         }
     }
 }

crates/ethcore/src/client/client.rs

@@ -306,6 +306,10 @@ pub struct Client {
 
     shutdown: Arc<ShutdownManager>,
 
+    /// block number and block has of latest gc.
+    /// this information is used to avoid double garbage collection.
+    garbage_collect_latest_block: Mutex<(u64, H256)>,
+
     statistics: ClientStatistics,
 }
 
@@ -842,6 +846,13 @@ impl Importer {
             warn!("Failed to prune ancient state data: {}", e);
         }
 
+        client.schedule_garbage_collect_in_queue();
+
+        //self.on_block_commit_finalized();
+
+        //client.miner().g
+        //client.check_garbage();.garbage_collect(Duration::from_secs(1));
+
         route
     }
 
@@ -1107,6 +1118,7 @@ impl Client {
             importer,
             config,
             shutdown,
+            garbage_collect_latest_block: Mutex::new((0, H256::zero())),
             statistics,
         });
 
@@ -1540,6 +1552,52 @@ impl Client {
         }
     }
 
+    /// Schedule garbage collection of invalid service transactions from the transaction queue based on the given block hash.
+    pub fn schedule_garbage_collect_in_queue(&self) {
+        let m = ClientIoMessage::execute(|c| c.garbage_collect_in_queue());
+        if let Err(e) = self.io_channel.read().send(m) {
+            error!(target: "client", "Failed to schedule garbage collection in transaction queue for block {:?}", e);
+        }
+    }
+
+    /// Garbage collect invalid servive transactions from the transaction queue based on the given block header.
+    pub fn garbage_collect_in_queue(&self) {
+        let machine = self.engine().machine();
+
+        match &self.block_header_decoded(BlockId::Latest) {
+            Some(block_header) => {
+                {
+                    // scope for mutex.
+                    let mut last_gc = self.garbage_collect_latest_block.lock();
+
+                    if block_header.number() == last_gc.0 && block_header.hash() == last_gc.1 {
+                        // already gced for this block, or gc is ongoing.
+                        // we can return here.
+                        return;
+                    }
+
+                    // we treat ongoing gc as DONE, to avoid blocking of the message channel
+                    last_gc.0 = block_header.number();
+                    last_gc.1 = block_header.hash();
+                }
+
+                // here hides an accepted race condition.
+                // latest block could change during loing ongoing GCs.
+                // this could be avoided developing a more complex GC logic.
+                // but the GC blocks the tx queue, so it has to be placing fast.
+                self.importer.miner.collect_garbage(|tx|
+                    match machine.verify_transaction(tx.signed(), block_header, self) {
+                        Ok(_) => true,
+                        Err(e) => {
+                            info!(target: "client", "collected garbage transaction from {:?}: {:?} reason: {:?}", tx.signed().sender(), tx.signed().hash, e);                   
+                            false
+                        },
+                    });
+            }
+            None => {}
+        }
+    }
+
     fn check_garbage(&self) {
         self.chain.read().collect_garbage();
         self.importer.block_queue.collect_garbage();

crates/ethcore/src/engines/hbbft/contracts/staking.rs

@@ -35,11 +35,6 @@ pub fn get_posdao_epoch_start(
     call_const_staking!(c, staking_epoch_start_block)
 }
 
-pub fn start_time_of_next_phase_transition(client: &dyn EngineClient) -> Result<U256, CallError> {
-    let c = BoundContract::bind(client, BlockId::Latest, *STAKING_CONTRACT_ADDRESS);
-    call_const_staking!(c, start_time_of_next_phase_transition)
-}
-
 pub fn candidate_min_stake(client: &dyn EngineClient) -> Result<U256, CallError> {
     let c = BoundContract::bind(client, BlockId::Latest, *STAKING_CONTRACT_ADDRESS);
     call_const_staking!(c, candidate_min_stake)

crates/ethcore/src/engines/hbbft/hbbft_early_epoch_end_manager.rs

@@ -112,7 +112,7 @@ impl HbbftEarlyEpochEndManager {
             signing_address: signing_address.clone(),
         };
 
-        info!(target: "engine", "early-epoch-end: HbbftEarlyEpochEndManager created. start_time {now:?}, start_block: {epoch_start_block}");
+        trace!(target: "engine", "early-epoch-end: HbbftEarlyEpochEndManager created. start_time {now:?}, start_block: {epoch_start_block}");
 
         return Some(result);
     }

crates/ethcore/src/engines/hbbft/hbbft_engine.rs

@@ -51,7 +51,6 @@ use super::{
     NodeId,
     contracts::{
         keygen_history::{all_parts_acks_available, initialize_synckeygen},
-        staking::start_time_of_next_phase_transition,
         validator_set::{ValidatorType, get_pending_validators, is_pending_validator},
     },
     contribution::{unix_now_millis, unix_now_secs},
@@ -329,9 +328,6 @@ impl TransitionHandler {
 
         // If the minimum block time has passed we are ready to trigger new blocks.
         if timer_duration == Duration::from_secs(0) {
-            // Always create blocks if we are in the keygen phase.
-            self.engine.start_hbbft_epoch_if_next_phase();
-
             // If the maximum block time has been reached we trigger a new block in any case.
             if self.max_block_time_remaining(client.clone()) == Duration::from_secs(0) {
                 self.engine.start_hbbft_epoch(client);
@@ -1104,27 +1100,6 @@ impl HoneyBadgerBFT {
         self.client.read().as_ref().and_then(Weak::upgrade)
     }
 
-    fn start_hbbft_epoch_if_next_phase(&self) {
-        // experimental deactivation of empty blocks.
-        // see: https://github.com/DMDcoin/diamond-node/issues/160
-
-        match self.client_arc() {
-            None => return,
-            Some(client) => {
-                // Get the next phase start time
-                let genesis_transition_time = match start_time_of_next_phase_transition(&*client) {
-                    Ok(time) => time,
-                    Err(_) => return,
-                };
-
-                // If current time larger than phase start time, start a new block.
-                if genesis_transition_time.as_u64() < unix_now_secs() {
-                    self.start_hbbft_epoch(client);
-                }
-            }
-        }
-    }
-
     fn replay_cached_messages(&self) -> Option<()> {
         let client = self.client_arc()?;
 

crates/ethcore/src/engines/hbbft/hbbft_state.rs

@@ -209,10 +209,7 @@ impl HbbftState {
         // apply DAO updates here.
         // update the current minimum gas price.
 
-        match get_minimum_gas_from_permission_contract(
-            client.as_ref(),
-            BlockId::Number(self.current_posdao_epoch_start_block),
-        ) {
+        match get_minimum_gas_from_permission_contract(client.as_ref(), BlockId::Latest) {
             Ok(min_gas) => {
                 *current_minimum_gas_price.lock() = Some(min_gas);
             }

crates/ethcore/src/engines/hbbft/test/mod.rs

@@ -1,12 +1,11 @@
 use super::{
     contracts::{
         staking::{
-            get_posdao_epoch, start_time_of_next_phase_transition,
+            get_posdao_epoch,
             tests::{create_staker, is_pool_active},
         },
         validator_set::{is_pending_validator, mining_by_staking_address},
     },
-    contribution::unix_now_secs,
     test::hbbft_test_client::{HbbftTestClient, create_hbbft_client, create_hbbft_clients},
 };
 use crate::{client::traits::BlockInfo, types::ids::BlockId};
@@ -129,12 +128,6 @@ fn test_epoch_transition() {
     // To avoid performing external transactions with the MoC we create and fund a random address.
     let transactor: KeyPair = Random.generate();
 
-    let genesis_transition_time = start_time_of_next_phase_transition(moc.client.as_ref())
-        .expect("start_time_of_next_phase_transition call must succeed");
-
-    // Genesis block is at time 0, current unix time must be much larger.
-    assert!(genesis_transition_time.as_u64() < unix_now_secs());
-
     // We should not be in the pending validator set at the genesis block.
     assert!(
         !is_pending_validator(moc.client.as_ref(), &moc.address())

crates/ethcore/src/miner/miner.rs

@@ -420,6 +420,17 @@ impl Miner {
         self.service_transaction_checker.clone()
     }
 
+    /// Performs garbage collection of the pool for free service transactions.
+    /// Removes transaction that are not valid anymore.
+    /// The process executes listener calls.
+    pub fn collect_garbage<F: Fn(&VerifiedTransaction) -> bool>(
+        &self,
+        service_transaction_filter: F,
+    ) {
+        self.transaction_queue
+            .garbage_collect(service_transaction_filter);
+    }
+
     /// Retrieves an existing pending block iff it's not older than given block number.
     ///
     /// NOTE: This will not prepare a new pending block if it's not existing.

crates/transaction-pool/Cargo.toml

@@ -8,7 +8,6 @@ authors = ["Dragan Rakita <dragan0rakita@gmail.com","Karim Agha <karim.agha@gnos
 edition = "2018"
 description = "Generic transaction pool."
 
-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 [dependencies.log]
 version = "0.4"
 

crates/transaction-pool/src/pool.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-use log::{trace, warn};
+use log::{info, trace, warn};
 use std::{
     collections::{hash_map, BTreeSet, HashMap},
     slice,
@@ -236,6 +236,44 @@ where
         }
     }
 
+    /// Performs garbage collection of the pool for free service transactions.
+    /// Removes transaction that are not valid anymore.
+    /// The process executes listener calls.
+    pub fn garbage_collect<F: Fn(&T) -> bool>(&mut self, service_transaction_check: F) {
+        if self.best_transactions.is_empty() {
+            return;
+        }
+
+        let mut txs_to_remove = Vec::<T::Hash>::new();
+
+        for sender in self.transactions.iter() {
+            for tx in sender.1.iter_transactions() {
+                if tx.transaction.has_zero_gas_price() {
+                    if service_transaction_check(&tx.transaction) {
+                        txs_to_remove.push(tx.hash().clone());
+                    }
+                } else {
+                    // if the next transaction has not zero gas price,
+                    // we are not continuing.
+                    break;
+                };
+            }
+        }
+
+        if txs_to_remove.is_empty() {
+            return;
+        }
+
+        info!(
+            "Garbage collection: removing invalid {} service transactions from pool.",
+            txs_to_remove.len()
+        );
+
+        for tx in txs_to_remove.iter() {
+            self.remove(tx, true);
+        }
+    }
+
     /// Updates state of the pool statistics if the transaction was added to a set.
     fn finalize_insert(&mut self, new: &Transaction<T>, old: Option<&Transaction<T>>) {
         self.mem_usage += new.mem_usage();