Consider changing the processing behavior of transactions that lack rc to avoid sharing on p2p network

When a node tries to apply a transaction for an account with insufficient rc, the rc processor doesn't outright reject the transaction unless it is_producing. This means that most nodes will keep the transaction in their mempool and share it to other nodes. Such transactions linger in the mempool until the transaction expires or gets added to a block.

In my opinion, nodes should just reject such transactions outright and not distribute them further in the network. This would lower network traffic and cpu overhead associated with these transactions. In most cases, such transactions would immediately be killed when the broadcasting node sees there is not enough rc to include it.

added optimization label

assigned to @dan

mentioned in issue #199

After reviewing the code, we believe we understand the true nature of the strange behavior we saw when reviewing the hived logs. The code is basically sound and the problem is relatively minor, but the log message is incorrect. To understand this issue, it's best to first describe how the rcplugin code functions:

The function use_account_rc computes rc costs, deducts it from an account's mana supply, and in some cases throws an exception when the mana supply goes below 0 in order to reject the transaction being analyzed.

There's three conceptual paths that call this function: a) when a transaction is received via the p2p network (this is the only path that rejects the transaction) b) when a block is received via the p2p network (in this case, even if mana is less than 0, the block with the underpowered transaction is accepted, but the node will warn that it is "Accepting transaction" that it shouldn't because of lack of RC). c) when the nodes is generating the block. In this case, there is no need to potentially reject the transaction, because it would have already been rejected earlier when it was received via the p2p network (path a).

So, at first it appears that the only time the node could warn it is accepting a transaction in a block that has insufficient RC would be when that was actually what was happening.

But we observed a case where this message was being reported for transactions from the same account in a consecutive set of blocks on a node that wasn't configured as a witness, yet despite the message, none of those blocks contained the transaction claimed by the log message. The log pattern conceptually looked like this:

1. received a block x contained a transaction by low_account
2. received block x+1 and reported that it accepted a transaction with insufficient rc from low_account. But actual block received did not contain this transaction (proved by checking block explorer)!
3. same message as 2 for several consecutive blocks
4. received a block x that contained a transaction from low_account
5. normal behavior afterwards on subsequent blocks received

Our first clue as to what was happening was that we saw there was an unexpected code path that could generate this log message: before a received block is processed, transactions that were applied to the local node's state after being received from the p2p network were popped, the block's transactions were applied, then the p2p-received transactions were reapplied (when they weren't already applied as part of processing the block's transactions). In this final step, when the popped p2p transactions were reapplied, the is_producing flag is enabled (the only other place it is set is when a p2p transaction is received), abd this is the flag that potentially allows the troublesome "Accepting transaction" log message to be generated.

Despite this flag being set, at first it didn't seem possible that the log message could be generated during this re-apply of the transaction, because we are only reapplying transactions that have previously been applied when we first received them from the p2p network, and if the account lacked RC now, it should have lacked RC then, so it should have rejected the transaction in the past and hence never had a chance to be re-applied.

But there seemed no other way than this code path that could trigger this message, so after some thought, we finally found a scenario under which the log message could be erroneously triggered:

1. an account with low rc generates two transactions (t1 and t2) during the same block (the account only has enough rc to pay for one of these two transactions).
2. these transactions arrive in opposite order in the local node (t1, then t2) vs the node that generates the next block (t2, then t1).
3. The local node adds t1 to its local state and rejects t2. The block-generating node adds t2 to its local state (and ultimately to its block) and rejects t1.
4. The local node receives the new block from the generating node, so it pops its state, which leaves it not yet counting the cost for t1 that it has already accepted into set of transactions with sufficient rc. Then it pushes the new block (quietly accepting t2 the transaction that it previously rejected because the rc consumed by t1 was restored), then reapplies its pending transactions to its local state including t1 (but since t2 consumed the RC that was previously allocated for t1, the mana for the account is too low now but this code path doesn't allow the transaction to be rejected a this point)
5. From that block onwards, the node will keep reapplying this t1 transaction to its local state after it applies each newly received block, erroneously warning each time that it is accepting a transaction in the block that has low rc. But the transaction is not in the block being pushed, it is the reapplied local transaction that is generating this message.
6. This will continue until one of two things happen: 1) t1 gets included into a block by a block-generating node (in which case it gets really accepted into the blockchain and removed from the locally applied transactions on the complaining node causing the complaining log message to go away) or 2) t1 expires and doesn't get into the blockchain at all.

For posterity, here are some notes I made when analyzing this code:

rc plugin:


use_account_rc is called on_post_apply_transaction:
1. count_resources is called on transaction 
2. exception is thrown if !has_mana but only if node is_producing

is_producing is set by set_producing call only

 write_queue_visitor.() (signed_block)
  db->push_block
    without_pending_transactions
      _push_block
        handle potential forking which can call apply_block many times on branches
        apply_block(new_block)
          _apply_block( next_block)
            validate_block_header
            identify who created the block
            reject the block if they are running wrong hardfork
            for block.transactions 
              apply_transaction(signed_transaction_
                _apply_transaction(signed_transaction)
          
          
 write_queue_visitor.() (signed_transaction) returns true if transaction doesn't throw
  db->push_transaction
   set_producing(true)
   set_pending_trx(true)
   _push_transaction
    start_undo_session
    _apply_transaction
      notify_post_apply_transaction
        on_post_apply_transaction of each plugin including rc_plugin

 write_queue_visitor.() (generate_block)
   block_generator->generate_block()
     apply_pending_transactions
       for pending_tx do db.apply_transaction if fits in block
     db.push_block

is producing set when push_transaction, is not set when apply_transaction!!!!

is_producing is not true when processing received block
is_producing is not true when generating block. But transaction was is_producing when transaction first received.

is_producing is true when receiving a transaction (so we should always reject ntransaction received via p2p layer when no mana)

rc is computed on_post_apply_transaction, so it is computed for generated and received blocks and received transactions
but it is only checked for received transactions, because only then is_producing is set

Ideally, the t1 transaction should just be rejected when the attempt to reapply it is made after applying the new block. But this code is fairly ugly (changes are made in state that can generate side-effect behavior in code scattered all over the place), so it is probably not worth analyzing the code enough to determine what steps would need to be taken to safely remove this transaction from local pending transactions.

I'm going to close this issue, since I think it's not worth the trouble to fix at this time what is mostly just a deceptive log message that shouldn't occur too often. If it becomes more of an issue later, we can revisit the decision at that point in time.

closed

mentioned in issue #223

mentioned in merge request !348 (merged)