Resolve #153: Make irrelevant sigs a soft check

Previously, all nodes rejected transactions with irrelevant signatures
always. Now, this is still true, except if a transaction with irrelevant
signatures is included in an otherwise valid block: then the transaction
is accepted even though it fails the soft check.

libraries/chain/chain_controller.cpp

@@ -318,8 +318,8 @@ signed_block chain_controller::_generate_block(
 
    uint64_t postponed_tx_count = 0;
    // pop pending state (reset to head block state)
-   for( const SignedTransaction& tx : _pending_transactions )
-   {
+   for (auto itr = _pending_transactions.begin(); itr != _pending_transactions.end(); ) {
+      auto& tx = *itr;
       size_t new_total_size = total_block_size + fc::raw::pack_size( tx );
 
       // postpone transaction if it would make block too big
@@ -344,12 +344,15 @@ signed_block chain_controller::_generate_block(
          }
          pending_block.cycles.back().back().user_input.emplace_back(tx);
 #warning TODO: Populate generated blocks with generated transactions
+
+         ++itr;
       }
       catch ( const fc::exception& e )
       {
-         // Do nothing, transaction will not be re-applied
-         wlog( "Transaction was not processed while generating block due to ${e}", ("e", e) );
+         // Do nothing, and discard transaction so it will not be re-applied
+         elog( "Transaction was not processed while generating block due to ${e}", ("e", e) );
          wlog( "The transaction was ${t}", ("t", tx) );
+         itr = _pending_transactions.erase(itr);
       }
    }
    if( postponed_tx_count > 0 )
@@ -450,7 +453,9 @@ void chain_controller::_apply_block(const signed_block& next_block)
       for (const auto& thread : cycle)
          for (const auto& trx : thread.user_input) {
             validate_referenced_accounts(trx);
-            check_transaction_authorization(trx);
+            // Check authorization, and allow irrelevant signatures.
+            // If the block producer let it slide, we'll roll with it.
+            check_transaction_authorization(trx, true);
          }
 
    /* We do not need to push the undo state for each transaction
@@ -484,7 +489,7 @@ void chain_controller::_apply_block(const signed_block& next_block)
 
 } FC_CAPTURE_AND_RETHROW( (next_block.block_num()) )  }
 
-void chain_controller::check_transaction_authorization(const SignedTransaction& trx)const {
+void chain_controller::check_transaction_authorization(const SignedTransaction& trx, bool allow_unused_signatures)const {
    if ((_skip_flags & skip_transaction_signatures) && (_skip_flags & skip_authority_check)) {
       ilog("Skipping auth and sigs checks");
       return;
@@ -519,7 +524,7 @@ void chain_controller::check_transaction_authorization(const SignedTransaction&
          }
       }
 
-   if ((_skip_flags & skip_transaction_signatures) == false)
+   if (!allow_unused_signatures && (_skip_flags & skip_transaction_signatures) == false)
       EOS_ASSERT(checker.all_keys_used(), tx_irrelevant_sig,
                  "Transaction bears irrelevant signatures from these keys: ${keys}", ("keys", checker.unused_keys()));
 }

libraries/chain/include/eos/chain/chain_controller.hpp

@@ -269,7 +269,7 @@ namespace eos { namespace chain {
             return f();
          }
 
-         void check_transaction_authorization(const SignedTransaction& trx)const;
+         void check_transaction_authorization(const SignedTransaction& trx, bool allow_unused_signatures = false)const;
 
          ProcessedTransaction apply_transaction(const SignedTransaction& trx, uint32_t skip = skip_nothing);
          ProcessedTransaction _apply_transaction(const SignedTransaction& trx);

tests/common/database_fixture.cpp

@@ -116,7 +116,8 @@ void testing_blockchain::produce_blocks(uint32_t count, uint32_t blocks_to_miss)
       auto slot = blocks_to_miss + 1;
       auto producer = get_producer(get_scheduled_producer(slot));
       auto private_key = fixture.get_private_key(producer.signing_key);
-      generate_block(get_slot_time(slot), producer.owner, private_key, chain_controller::skip_transaction_signatures);
+      generate_block(get_slot_time(slot), producer.owner, private_key,
+                     skip_trx_sigs? chain_controller::skip_transaction_signatures : 0);
    }
 }
 

tests/tests/block_tests.cpp

@@ -574,4 +574,36 @@ BOOST_FIXTURE_TEST_CASE(wipe, testing_fixture)
       }
 } FC_LOG_AND_RETHROW() }
 
+BOOST_FIXTURE_TEST_CASE(irrelevant_sig_soft_check, testing_fixture) {
+   try {
+      Make_Blockchain(chain);
+      chain.set_hold_transactions_for_review(true);
+      chain.set_skip_transaction_signature_checking(false);
+      chain.set_auto_sign_transactions(true);
+
+      // Make an account, but add an extra signature to the transaction
+      Make_Account(chain, alice);
+      // Check that it throws for irrelevant signatures
+      BOOST_CHECK_THROW(chain.review_transaction([](SignedTransaction& trx, auto) {
+                           trx.sign(fc::ecc::private_key::regenerate(fc::digest("an unknown key")), {});
+                           return true;
+                        }), tx_irrelevant_sig);
+      // Push it through with a skip flag
+      chain.review_transaction([](SignedTransaction& trx, uint32_t& skip) {
+         trx.sign(fc::ecc::private_key::regenerate(fc::digest("an unknown key")), {});
+         skip |= chain_controller::skip_transaction_signatures;
+         return true;
+      });
+
+      // Skip sig checks so we can produce a block with the oversigned transaction
+      chain.set_skip_transaction_signature_checking(true);
+      chain.produce_blocks();
+
+      // Now check that a second blockchain accepts the block with the oversigned transaction
+      Make_Blockchain(newchain);
+      Make_Network(net, (chain)(newchain));
+      BOOST_CHECK_NE((newchain_db.find<account_object, by_name>("alice")), nullptr);
+   } FC_LOG_AND_RETHROW()
+}
+
 BOOST_AUTO_TEST_SUITE_END()