P2P: Inform peer if not accepting transactions

[heifner]

Optimize network traffic by informing peers when node is configured for p2p-accept-transactions=false. Since the transactions will be immediately dropped, no reason for the peer to send them.
If configured to not accept transactions, p2p-accept-transactions=false, then inform peer the node is a blocks only connection.

Resolves #767

[linh2931]

Can !my_impl->p2p_accept_transactions be folded inside is_blocks_only_connection()?

[heifner]

No, as other places is_blocks_only_connection() is called we don't want it to be true. For example is_blocks_only_connection() is called in the loop to send out trxs. If it was true we would not send out a trx to our peer. We want to send trxs out, just not allow transactions to flow in.

[linh2931]

Thanks. Maybe you can add a comment here because hello.p2p_address += ":blk"; makes the node like a block only node.

[greg7mdp]

I think this might be better because it also updates connection_type and logs the change.

       if(hello.sig == chain::signature_type())
          hello.token = sha256();
       hello.p2p_address = my_impl->p2p_address;
+      if (!my_impl->p2p_accept_transactions && !is_blocks_only_connection()) {
+         fc_dlog( logger, "Setting connection ${c} type for: ${peer} to transactions only", ("c", connection_id)("peer", peer_addr) );
+         connection_type = blocks_only;
+      }
       if( is_transactions_only_connection() ) hello.p2p_address += ":trx";
       if( is_blocks_only_connection() ) hello.p2p_address += ":blk";
       hello.p2p_address += " - " + hello.node_id.str().substr(0,7);

[heifner]

We do not want to set connection_type = blocks_only see my comment to Lin above.
Adding a log statement we are doing it is a good idea.

[greg7mdp]

@heifner I see in your comment above why we don't want to change connection_type (so we keep sending transactions).
Still, this seems a little bit hacky to me. I think I'd recommend changing connection_type to be an something like an uint32_t which can contain 4 non-exclusive flags:

• send_transactions
• receive_transactions
• send_blocks
• receive_blocks

[heifner]

Keeping track of receive_transactions is already tracked via p2p_accept_transaction. I don't think we want to track that state in two different places. net_plugin already has the concept blk block only connection (do not send transactions to the peer). All we are doing here is telling the peer that from its perspective we are blocks only.

[greg7mdp]

Right now, we are keeping state describing what the connection permits in two places, connection_type and p2p_accept_transaction.

I am suggesting keeping it only in one place: connection_type . I think it would be cleaner.

[heifner]

That is not how I see it. connection_type is the type of connection I have with my peer. That does not imply it is the same connection they have with me. p2p_accept_transaction is an indication of what I allow. A peer might not honor my request for what they send me, so I need to enforce it myself. In my mind these are two separate concepts.

[greg7mdp]

Well, I understand this is how it is setup today. However, if you just consider what you allow on your connection to your peer:

• send_transactions
• accept_transactions
• send_blocks
• accept_blocks

I don't see why these capabilities should not be stored together in one field. It is certainly less confusing than the current way imho.

[greg7mdp]

However I'm fine approving as-is.