Merge pull request #1094 from aludvik/validate-batches-last

Validate batches last

validator/sawtooth_validator/journal/chain.py

@@ -279,7 +279,7 @@ def _validate_on_chain_rules(self, blkw):
             return self._validation_rule_enforcer.validate(blkw, state_root)
         return True
 
-    def validate_block(self, blkw):
+    def validate_block(self, blkw, skip_batches=False):
         # pylint: disable=broad-except
         try:
             if blkw.status == BlockStatus.Valid:
@@ -291,20 +291,23 @@ def validate_block(self, blkw):
 
                 valid = self._validate_permissions(blkw)
 
-                consensus = self._consensus_module.\
-                    BlockVerifier(block_cache=self._block_cache,
-                                  state_view_factory=self._state_view_factory,
-                                  data_dir=self._data_dir,
-                                  config_dir=self._config_dir,
-                                  validator_id=self._identity_public_key)
                 if valid:
-                    valid = self._validate_on_chain_rules(blkw)
+                    consensus = self._consensus_module.BlockVerifier(
+                        block_cache=self._block_cache,
+                        state_view_factory=self._state_view_factory,
+                        data_dir=self._data_dir,
+                        config_dir=self._config_dir,
+                        validator_id=self._identity_public_key)
+                    valid = consensus.verify_block(blkw)
 
                 if valid:
-                    valid = self._verify_block_batches(blkw)
+                    valid = self._validate_on_chain_rules(blkw)
 
-                if valid:
-                    valid = consensus.verify_block(blkw)
+                # If skip_batches is true, the block is validated except for
+                # batches. In this case, the block is marked as invalid if any
+                # of the checks fail, but will never be marked as valid.
+                if valid and not skip_batches:
+                    valid = self._verify_block_batches(blkw)
 
                 # since changes to the chain-head can change the state of the
                 # blocks in BlockStore we have to revalidate this block.
@@ -314,8 +317,12 @@ def validate_block(self, blkw):
                         block_store.chain_head.identifier:
                     raise ChainHeadUpdated()
 
-                blkw.status = BlockStatus.Valid if\
-                    valid else BlockStatus.Invalid
+                if valid:
+                    if not skip_batches:
+                        blkw.status = BlockStatus.Valid
+                else:
+                    blkw.status = BlockStatus.Invalid
+
                 return valid
         except ChainHeadUpdated:
             raise
@@ -455,7 +462,17 @@ def run(self):
             self._find_common_ancestor(new_blkw, cur_blkw,
                                        new_chain, cur_chain)
 
-            # 3) Determine the validity of the new fork
+            # 3) Evaluate the 2 chains to see if the new chain should be
+            # committed
+            commit_new_chain = self._test_commit_new_chain()
+
+            # If we're not going to commit this fork and it is only a single
+            # block fork, we can skip validating the batches as an optimization
+            skip = (not commit_new_chain) and (
+                self._new_block.previous_block_id ==
+                self._chain_head.identifier)
+
+            # 4) Determine the validity of the new fork
             # build the transaction cache to simulate the state of the
             # chain at the common root.
             self._chain_commit_state = ChainCommitState(
@@ -464,7 +481,7 @@ def run(self):
             valid = True
             for block in reversed(new_chain):
                 if valid:
-                    if not self.validate_block(block):
+                    if not self.validate_block(block, skip_batches=skip):
                         LOGGER.info("Block validation failed: %s", block)
                         valid = False
                 else:
@@ -476,10 +493,6 @@ def run(self):
                 self._done_cb(False, self._result)
                 return
 
-            # 4) Evaluate the 2 chains to see if the new chain should be
-            # committed
-            commit_new_chain = self._test_commit_new_chain()
-
             # 5) Consensus to compute batch sets (only if we are switching).
             if commit_new_chain:
                 (self._result["committed_batches"],

validator/tests/test_journal/mock_consensus.py

@@ -118,7 +118,10 @@ def compare_forks(self, cur_fork_head, new_fork_head):
         new_num = new_fork_head.block_num
         new_weight = 0
         if new_fork_head.consensus:
-            new_weight = int(new_fork_head.consensus.decode().split(':')[1])
+            try:
+                new_weight = int(new_fork_head.consensus.decode().split(':')[1])
+            except IndexError:
+                return False
         cur_num = cur_fork_head.block_num
         cur_weight = 0
         if cur_fork_head.consensus:

validator/tests/test_journal/tests.py

@@ -588,7 +588,7 @@ def test_fork_invalid_predecessor(self):
 
     def test_block_bad_consensus(self):
         """
-        Test the case where the new block has a bad batch
+        Test the case where the new block has invalid consensus
         """
         chain, head = self.generate_chain_with_head(
             self.root, 5, {'add_to_store': True})