Don't apply subjective billing to read-only transactions

libraries/chain/controller.cpp

@@ -1592,14 +1592,16 @@ struct controller_impl {
                              std::move(trx_context.executed_action_receipt_digests) );
 
             // call the accept signal but only once for this transaction
-            if (!trx->accepted) {
-               trx->accepted = true;
-               emit( self.accepted_transaction, trx);
+            if (!trx->read_only) {
+                if (!trx->accepted) {
+                    trx->accepted = true;
+                    emit(self.accepted_transaction, trx);
+                }
+
+                dmlog_applied_transaction(trace);
+                emit(self.applied_transaction, std::tie(trace, trx->packed_trx()));
             }
 
-            dmlog_applied_transaction(trace);
-            emit(self.applied_transaction, std::tie(trace, trx->packed_trx()));
-
 
             if ( (read_mode != db_read_mode::SPECULATIVE && pending->_block_status == controller::block_status::incomplete) || trx->read_only ) {
                //this may happen automatically in destructor, but I prefer make it more explicit
@@ -1625,9 +1627,11 @@ struct controller_impl {
            handle_exception(wrapper);
          }
 
-         emit( self.accepted_transaction, trx );
-         dmlog_applied_transaction(trace);
-         emit( self.applied_transaction, std::tie(trace, trx->packed_trx()) );
+         if (!trx->read_only) {
+             emit(self.accepted_transaction, trx);
+             dmlog_applied_transaction(trace);
+             emit(self.applied_transaction, std::tie(trace, trx->packed_trx()));
+         }
 
          return trace;
       } FC_CAPTURE_AND_RETHROW((trace))

plugins/chain_plugin/chain_plugin.cpp

@@ -2813,7 +2813,7 @@ void read_only::compute_transaction(const fc::variant_object& params, next_funct
             abi_serializer::from_variant(params, *pretty_input, resolver, abi_serializer::create_yield_function( abi_serializer_max_time ));
         } EOS_RETHROW_EXCEPTIONS(chain::packed_transaction_type_exception, "Invalid packed transaction")
 
-        app().get_method<incoming::methods::transaction_async>()(pretty_input, true, true,
+        app().get_method<incoming::methods::transaction_async>()(pretty_input, false, true,
              [this, next](const std::variant<fc::exception_ptr, transaction_trace_ptr>& result) -> void {
                  if (std::holds_alternative<fc::exception_ptr>(result)) {
                      next(std::get<fc::exception_ptr>(result));

plugins/producer_plugin/producer_plugin.cpp

@@ -561,7 +561,8 @@ class producer_plugin_impl : public std::enable_shared_from_this<producer_plugin
 
             bool disable_subjective_billing = ( _pending_block_mode == pending_block_mode::producing )
                                               || ( persist_until_expired && _disable_subjective_api_billing )
-                                              || ( !persist_until_expired && _disable_subjective_p2p_billing );
+                                              || ( !persist_until_expired && _disable_subjective_p2p_billing )
+                                              || trx->read_only;
 
             auto first_auth = trx->packed_trx()->get_transaction().first_authorizer();
             uint32_t sub_bill = 0;
@@ -584,7 +585,9 @@ class producer_plugin_impl : public std::enable_shared_from_this<producer_plugin
                   }
                   exhausted = block_is_exhausted();
                } else {
-                  _subjective_billing.subjective_bill_failure( first_auth, trace->elapsed, fc::time_point::now() );
+                   if (!disable_subjective_billing)
+                      _subjective_billing.subjective_bill_failure( first_auth, trace->elapsed, fc::time_point::now() );
+
                   auto e_ptr = trace->except->dynamic_copy_exception();
                   send_response( e_ptr );
                }
@@ -596,7 +599,8 @@ class producer_plugin_impl : public std::enable_shared_from_this<producer_plugin
                   _unapplied_transactions.add_persisted( trx );
                } else {
                   // if db_read_mode SPECULATIVE then trx is in the pending block and not immediately reverted
-                  _subjective_billing.subjective_bill( trx->id(), expire, first_auth, trace->elapsed,
+                  if (!disable_subjective_billing)
+                     _subjective_billing.subjective_bill( trx->id(), expire, first_auth, trace->elapsed,
                                                        chain.get_read_mode() == chain::db_read_mode::SPECULATIVE );
                }
                send_response( trace );
@@ -1959,6 +1963,10 @@ bool producer_plugin_impl::process_unapplied_trxs( const fc::time_point& deadlin
             }
             // no subjective billing since we are producing or processing persisted trxs
             const uint32_t sub_bill = 0;
+            bool disable_subjective_billing = ( _pending_block_mode == pending_block_mode::producing )
+               || ( (itr->trx_type == trx_enum_type::persisted) && _disable_subjective_api_billing )
+               || ( !(itr->trx_type == trx_enum_type::persisted) && _disable_subjective_p2p_billing )
+               || trx->read_only;
 
             auto trace = chain.push_transaction( trx, trx_deadline, prev_billed_cpu_time_us, false, sub_bill );
             fc_dlog( _trx_failed_trace_log, "Subjective unapplied bill for ${a}: ${b} prev ${t}us", ("a",first_auth)("b",prev_billed_cpu_time_us)("t",trace->elapsed));
@@ -1978,7 +1986,8 @@ bool producer_plugin_impl::process_unapplied_trxs( const fc::time_point& deadlin
                               ("c", trace->except->code())("p", prev_billed_cpu_time_us)
                               ("r", fc::time_point::now() - start)("id", trx->id()) );
                      account_fails.add( first_auth, failure_code );
-                     _subjective_billing.subjective_bill_failure( first_auth, trace->elapsed, fc::time_point::now() );
+                     if (!disable_subjective_billing)
+                        _subjective_billing.subjective_bill_failure( first_auth, trace->elapsed, fc::time_point::now() );
                   }
                   ++num_failed;
                   itr = _unapplied_transactions.erase( itr );
@@ -1987,7 +1996,8 @@ bool producer_plugin_impl::process_unapplied_trxs( const fc::time_point& deadlin
             } else {
                fc_dlog( _trx_successful_trace_log, "Subjective unapplied bill for success ${a}: ${b} prev ${t}us", ("a",first_auth)("b",prev_billed_cpu_time_us)("t",trace->elapsed));
                // if db_read_mode SPECULATIVE then trx is in the pending block and not immediately reverted
-               _subjective_billing.subjective_bill( trx->id(), trx->packed_trx()->expiration(), first_auth, trace->elapsed,
+               if (!disable_subjective_billing)
+                  _subjective_billing.subjective_bill( trx->id(), trx->packed_trx()->expiration(), first_auth, trace->elapsed,
                                                     chain.get_read_mode() == chain::db_read_mode::SPECULATIVE );
                ++num_applied;
                itr = _unapplied_transactions.erase( itr );

tests/CMakeLists.txt

@@ -83,7 +83,7 @@ add_test(NAME producer-preactivate-feature-test COMMAND tests/prod_preactivation
 set_property(TEST producer-preactivate-feature-test PROPERTY LABELS nonparallelizable_tests)
 add_test(NAME nodeos_protocol_feature_test COMMAND tests/nodeos_protocol_feature_test.py -v --clean-run --dump-error-detail WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
 set_property(TEST nodeos_protocol_feature_test PROPERTY LABELS nonparallelizable_tests)
-add_test(NAME compute_transaction_test COMMAND tests/compute_transaction_test.py -v -p 2 --clean-run --dump-error-detail WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
+add_test(NAME compute_transaction_test COMMAND tests/compute_transaction_test.py -v -p 2 -n 3 --clean-run --dump-error-detail WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
 set_property(TEST compute_transaction_test PROPERTY LABELS nonparallelizable_tests)
 
 add_test(NAME distributed-transactions-test COMMAND tests/distributed-transactions-test.py -d 2 -p 4 -n 6 -v --clean-run --dump-error-detail WORKING_DIRECTORY ${CMAKE_BINARY_DIR})

tests/Node.py

@@ -851,6 +851,11 @@ def getEosBalances(self, accounts):
 
         return balances
 
+    # Gets subjective bill info for an account
+    def getAccountSubjectiveInfo(self, account):
+        acct = self.getEosAccount(account)
+        return acct["subjective_cpu_bill_limit"]
+
     # Gets accounts mapped to key. Returns json object
     def getAccountsByKey(self, key, exitOnError=False):
         cmdDesc = "get accounts"

tests/compute_transaction_test.py

@@ -73,7 +73,7 @@
     Print ("producing nodes: %s, non-producing nodes: %d, topology: %s, delay between nodes launch(seconds): %d" % (pnodes, total_nodes-pnodes, topo, delay))
 
     Print("Stand up cluster")
-    if cluster.launch(pnodes=pnodes, totalNodes=total_nodes, topo=topo, delay=delay) is False:
+    if cluster.launch(pnodes=pnodes, totalNodes=total_nodes, topo=topo, delay=delay,extraNodeosArgs=" --http-max-response-time-ms 990000 --disable-subjective-api-billing false" ) is False:
        errorExit("Failed to stand up eos cluster.")
 
     Print ("Wait for Cluster stabilization")
@@ -91,12 +91,17 @@
     account2 = Account('account2')
     account2.ownerPublicKey = EOSIO_ACCT_PUBLIC_DEFAULT_KEY
     account2.activePublicKey = EOSIO_ACCT_PUBLIC_DEFAULT_KEY
-    cluster.createAccountAndVerify(account2, cluster.eosioAccount, stakedDeposit=1000)
+    cluster.createAccountAndVerify(account2, cluster.eosioAccount, stakedDeposit=1000, stakeCPU=1)
 
     Print("Validating accounts after bootstrap")
     cluster.validateAccounts([account1, account2])
 
     node = cluster.getNode()
+
+    # non-producing node
+    npnode = cluster.nodes[-1]
+
+
     transferAmount="1000.0000 {0}".format(CORE_SYMBOL)
 
     node.transferFunds(cluster.eosioAccount, account1, transferAmount, "fund account")
@@ -125,8 +130,56 @@
 
     postBalances = node.getEosBalances([account1, account2])
 
-    testSuccessful = (postBalances == preBalances)
+    assert(postBalances == preBalances)
+
+#   Send through some failing *read only* transactions on a non-producer node
+    for x in range(5):
+        memo = 'tx-{}'.format(x)
+        trx2 = {
+
+            "actions": [{"account": "eosio.token","name": "transfer",
+                         "authorization": [{"actor": "account2","permission": "active"}],
+                         "data": {"from": "account2","to": "account1","quantity": "10.0001 SYS","memo": memo},
+                         "compression": "none"}]
+        }
+
+        results = npnode.pushTransaction(trx2, opts="--read-only")
+        assert(not results[0])
+
+# Verify that no subjective billing was charged
+    acct2 = npnode.getAccountSubjectiveInfo("account2")
+    assert(acct2["used"] == 0)
+
+    #   Send through some failing *non read-only* transactions on a non-producer node
+    for x in range(5):
+        memo = 'tx-{}'.format(x)
+        trx2 = {
+
+            "actions": [{"account": "eosio.token","name": "transfer",
+                         "authorization": [{"actor": "account2","permission": "active"}],
+                         "data": {"from": "account2","to": "account1","quantity": "10.0001 SYS","memo": memo},
+                         "compression": "none"}]
+        }
+
+        results = npnode.pushTransaction(trx2)
+        assert(not results[0])
+
+    # Verify that subjective billing *was* charged
+    acct2 = npnode.getAccountSubjectiveInfo("account2")
+    assert(acct2["used"] > 0)
+
+    # Test that irrelavent signature doesn't break read-only txn
+    trx3 = {
+
+        "actions": [{"account": "eosio.token","name": "transfer",
+                     "authorization": [{"actor": "account1","permission": "active"},{"actor": "account2","permission": "active"}],
+                     "data": {"from": "account1","to": "account2","quantity": "10.0001 SYS","memo": memo},
+                     "compression": "none"}]
+    }
+    results = npnode.pushTransaction(trx3, opts="--read-only")
+    assert(results[0])
 
+    testSuccessful = True
 finally:
     TestHelper.shutdown(cluster, walletMgr, testSuccessful, killEosInstances, killWallet, keepLogs, killAll, dumpErrorDetails)