Merge bitcoin#19474: doc: Use precise permission flags where possible

fab5586 doc: Use precise permission flags where possible (MarcoFalke) Pull request description: Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour. This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated. Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help. ACKs for top commit: jnewbery: reACK fab5586 fjahr: Code review ACK fab5586 jonatack: ACK fab5586 Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
PastaPastaPasta · Jul 11, 2020 · ca05588 · ca05588
2 parents 505b4ed + fab5586
commit ca05588
Show file tree

Hide file tree

Showing 10 changed files with 37 additions and 45 deletions.
diff --git a/doc/reduce-traffic.md b/doc/reduce-traffic.md
@@ -50,7 +50,7 @@ Be reminded of the effects of this setting.
   Doing so disables the automatic broadcasting of transactions from wallet. Not
   relaying other's transactions could hurt your privacy if used while a wallet
   is loaded or if you use the node to broadcast transactions.
-- If a peer is whitelisted and "-whitelistforcerelay" is set to "1" (which will
-  also set "whitelistrelay" to "1"), we will still receive and relay their transactions.
+- If a peer has the forcerelay permission, we will still receive and relay
+  their transactions.
 - It makes block propagation slower because compact block relay can only be
   used when transaction relay is enabled.
diff --git a/share/examples/bitcoin.conf b/share/examples/bitcoin.conf
@@ -20,8 +20,8 @@
 # Bind to given address and always listen on it. Use [host]:port notation for IPv6
 #bind=<addr>
 
-# Bind to given address and whitelist peers connecting to it. Use [host]:port notation for IPv6
-#whitebind=<addr>
+# Bind to given address and add permission flags to peers connecting to it. Use [host]:port notation for IPv6
+#whitebind=perm@<addr>
 
 ##############################################################
 ##            Quick Primer on addnode vs connect            ##

diff --git a/src/init.cpp b/src/init.cpp
@@ -396,7 +396,7 @@ void SetupServerArgs(NodeContext& node)
     gArgs.AddArg("-blocknotify=<cmd>", "Execute command when the best block changes (%s in cmd is replaced by block hash)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 #endif
     gArgs.AddArg("-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from inbound peers is disabled, unless '-whitelistforcerelay' is '1', in which case whitelisted peers' transactions will be relayed. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    gArgs.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from inbound peers is disabled, unless the peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     gArgs.AddArg("-conf=<file>", strprintf("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     gArgs.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     gArgs.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -117,7 +117,7 @@ static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24};
 /** Average delay between peer address broadcasts */
 static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30};
 /** Average delay between trickled inventory transmissions in seconds.
- *  Blocks and whitelisted receivers bypass this, outbound peers get half this delay. */
+ *  Blocks and peers with noban permission bypass this, outbound peers get half this delay. */
 static const unsigned int INVENTORY_BROADCAST_INTERVAL = 5;
 /** Maximum number of inventory items to send per transmission.
  *  Limits the impact of low-fee transaction floods. */
@@ -249,7 +249,7 @@ struct CNodeState {
     bool fCurrentlyConnected;
     //! Accumulated misbehaviour score for this peer.
     int nMisbehavior;
-    //! Whether this peer should be disconnected and marked as discouraged (unless whitelisted with noban).
+    //! Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission).
     bool m_should_discourage;
     //! String name of this peer (debugging/logging purposes).
     const std::string name;
@@ -1895,8 +1895,8 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman* connman, ChainstateMan
             // headers to fetch from this peer.
             if (nodestate->pindexBestKnownBlock && nodestate->pindexBestKnownBlock->nChainWork < nMinimumChainWork) {
                 // This peer has too little work on their headers chain to help
-                // us sync -- disconnect if using an outbound slot (unless
-                // whitelisted or addnode).
+                // us sync -- disconnect if it is an outbound disconnection
+                // candidate.
                 // Note: We compare their tip to nMinimumChainWork (rather than
                 // ::ChainActive().Tip()) because we won't start block download
                 // until we have a headers chain that has at least
@@ -2531,9 +2531,10 @@ void ProcessMessage(
         // block-relay-only peer
         bool fBlocksOnly = !g_relay_txes || (pfrom.m_tx_relay == nullptr);
 
-        // Allow whitelisted peers to send data other than blocks in blocks only mode if whitelistrelay is true
-        if (pfrom.HasPermission(PF_RELAY))
+        // Allow peers with relay permission to send data other than blocks in blocks only mode
+        if (pfrom.HasPermission(PF_RELAY)) {
             fBlocksOnly = false;
+        }
 
         LOCK(cs_main);
 
@@ -2887,14 +2888,14 @@ void ProcessMessage(
             }
 
             if (pfrom.HasPermission(PF_FORCERELAY)) {
-                // Always relay transactions received from whitelisted peers, even
+                // Always relay transactions received from peers with forcerelay permission, even
                 // if they were already in the mempool,
                 // allowing the node to function as a gateway for
                 // nodes hidden behind it.
                 if (!mempool.exists(tx.GetHash())) {
-                    LogPrintf("Not relaying non-mempool transaction %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
+                    LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
                 } else {
-                    LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
+                    LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
                     RelayTransaction(tx.GetHash(), *connman);
                 }
             }
@@ -3044,7 +3045,7 @@ void ProcessMessage(
                 PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock;
                 ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact);
                 if (status == READ_STATUS_INVALID) {
-                    MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case of whitelist
+                    MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect
                     Misbehaving(pfrom.GetId(), 100, strprintf("Peer %d sent us invalid compact block\n", pfrom.GetId()));
                     return;
                 } else if (status == READ_STATUS_FAILED) {
@@ -3177,7 +3178,7 @@ void ProcessMessage(
             PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock;
             ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn);
             if (status == READ_STATUS_INVALID) {
-                MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case of whitelist
+                MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect
                 Misbehaving(pfrom.GetId(), 100, strprintf("Peer %d sent us invalid compact block/non-matching block transactions\n", pfrom.GetId()));
                 return;
             } else if (status == READ_STATUS_FAILED) {
@@ -4258,9 +4259,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         // Check for headers sync timeouts
         if (state.fSyncStarted && state.nHeadersSyncTimeout < std::numeric_limits<int64_t>::max()) {
             // Detect whether this is a stalling initial-headers-sync peer
-            if (pindexBestHeader->GetBlockTime() <= GetAdjustedTime() - 24*60*60) {
+            if (pindexBestHeader->GetBlockTime() <= GetAdjustedTime() - 24 * 60 * 60) {
                 if (nNow > state.nHeadersSyncTimeout && nSyncStarted == 1 && (nPreferredDownload - state.fPreferredDownload >= 1)) {
-                    // Disconnect a (non-whitelisted) peer if it is our only sync peer,
+                    // Disconnect a peer (without the noban permission) if it is our only sync peer,
                     // and we have others we could be using instead.
                     // Note: If all our peers are inbound, then we won't
                     // disconnect our sync peer for stalling; we have bigger
@@ -4270,7 +4271,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
                         pto->fDisconnect = true;
                         return true;
                     } else {
-                        LogPrintf("Timeout downloading headers from whitelisted peer=%d, not disconnecting\n", pto->GetId());
+                        LogPrintf("Timeout downloading headers from noban peer=%d, not disconnecting\n", pto->GetId());
                         // Reset the headers sync state so that we have a
                         // chance to try downloading from a different peer.
                         // Note: this will also result in at least one more

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -798,10 +798,8 @@ static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
 
     if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) {
         // Block not found on disk. This could be because we have the block
-        // header in our index but don't have the block (for example if a
-        // non-whitelisted node sends us an unrequested long chain of valid
-        // blocks, we add the headers to our index, but don't accept the
-        // block).
+        // header in our index but not yet have the block or did not accept the
+        // block.
         throw JSONRPCError(RPC_MISC_ERROR, "Block not found on disk");
     }
 

diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
@@ -383,7 +383,7 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
     BOOST_CHECK(!NetWhitebindPermissions::TryParse("bloom,forcerelay,oopsie@1.2.3.4:32", whitebindPermissions, error));
     BOOST_CHECK(error.original.find("Invalid P2P permission") != std::string::npos);
 
-    // Check whitelist error
+    // Check netmask error
     BOOST_CHECK(!NetWhitelistPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitelistPermissions, error));
     BOOST_CHECK(error.original.find("Invalid netmask specified in -whitelist") != std::string::npos);
 

diff --git a/src/validation.h b/src/validation.h
@@ -845,7 +845,7 @@ class ChainstateManager
      * validationinterface callback.
      *
      * @param[in]   pblock  The block we want to process.
-     * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
+     * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources.
      * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call
      * @returns     If the block was processed, independently of block validity
      */

diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py
@@ -57,10 +57,10 @@ def run_test(self):
             self.nodes[0].p2p.wait_for_tx(txid)
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
 
-        self.log.info('Check that txs from whitelisted peers are not rejected and relayed to others')
-        self.log.info("Restarting node 0 with whitelist permission and blocksonly")
+        self.log.info('Check that txs from forcerelay peers are not rejected and relayed to others')
+        self.log.info("Restarting node 0 with forcerelay permission and blocksonly")
         self.restart_node(0, ["-persistmempool=0", "-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"])
-        assert_equal(self.nodes[0].getrawmempool(),[])
+        assert_equal(self.nodes[0].getrawmempool(), [])
         first_peer = self.nodes[0].add_p2p_connection(P2PInterface())
         second_peer = self.nodes[0].add_p2p_connection(P2PInterface())
         peer_1_info = self.nodes[0].getpeerinfo()[0]
@@ -72,14 +72,15 @@ def run_test(self):
         assert_equal(self.nodes[0].testmempoolaccept([sigtx])[0]['allowed'], True)
         txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid']
 
-        self.log.info('Check that the tx from whitelisted first_peer is relayed to others (ie.second_peer)')
+        self.log.info('Check that the tx from forcerelay first_peer is relayed to others (ie.second_peer)')
         with self.nodes[0].assert_debug_log(["received getdata"]):
             first_peer.send_message(msg_tx(FromHex(CTransaction(), sigtx)))
-            self.log.info('Check that the whitelisted peer is still connected after sending the transaction')
+            self.log.info('Check that the forcerelay peer is still connected after sending the transaction')
             assert_equal(first_peer.is_connected, True)
             second_peer.wait_for_tx(txid)
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
-        self.log.info("Whitelisted peer's transaction is accepted and relayed")
+        self.log.info("Forcerelay peer's transaction is accepted and relayed")
+
 
 if __name__ == '__main__':
     P2PBlocksOnly().main()
diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py
@@ -108,9 +108,9 @@ def check_tx_relay(self):
         block_op_true = self.nodes[0].getblock(self.nodes[0].generatetoaddress(100, ADDRESS_BCRT1_P2WSH_OP_TRUE)[0])
         self.sync_all()
 
-        self.log.debug("Create a connection from a whitelisted wallet that rebroadcasts raw txs")
+        self.log.debug("Create a connection from a forcerelay peer that rebroadcasts raw txs")
         # A python mininode is needed to send the raw transaction directly. If a full node was used, it could only
-        # rebroadcast via the inv-getdata mechanism. However, even for whitelisted connections, a full node would
+        # rebroadcast via the inv-getdata mechanism. However, even for forcerelay connections, a full node would
         # currently not request a txid that is already in the mempool.
         self.restart_node(1, extra_args=["-whitelist=forcerelay@127.0.0.1"])
         p2p_rebroadcast_wallet = self.nodes[1].add_p2p_connection(P2PDataStore())
@@ -135,7 +135,7 @@ def check_tx_relay(self):
 
         self.log.debug("Check that node[1] will send the tx to node[0] even though it is already in the mempool")
         connect_nodes(self.nodes[1], 0)
-        with self.nodes[1].assert_debug_log(["Force relaying tx {} from whitelisted peer=0".format(txid)]):
+        with self.nodes[1].assert_debug_log(["Force relaying tx {} from peer=0".format(txid)]):
             p2p_rebroadcast_wallet.send_txs_and_test([tx], self.nodes[1])
             wait_until(lambda: txid in self.nodes[0].getrawmempool())
 
@@ -146,7 +146,7 @@ def check_tx_relay(self):
             [tx],
             self.nodes[1],
             success=False,
-            reject_reason='Not relaying non-mempool transaction {} from whitelisted peer=0'.format(txid),
+            reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid),
         )
 
     def checkpermission(self, args, expectedPermissions, whitelisted):

diff --git a/test/functional/p2p_unrequested_blocks.py b/test/functional/p2p_unrequested_blocks.py
@@ -4,7 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test processing of unrequested blocks.
 
-Setup: two nodes, node0+node1, not connected to each other. Node1 will have
+Setup: two nodes, node0 + node1, not connected to each other. Node1 will have
 nMinimumChainWork set to 0x10, so it won't process low-work unrequested blocks.
 
 We have one P2PInterface connection to node0 called test_node, and one to node1
@@ -71,18 +71,10 @@ def set_test_params(self):
         self.extra_args = [[], ["-minimumchainwork=0x10"]]
 
     def setup_network(self):
-        # Node0 will be used to test behavior of processing unrequested blocks
-        # from peers which are not whitelisted, while Node1 will be used for
-        # the whitelisted case.
-        # Node2 will be used for non-whitelisted peers to test the interaction
-        # with nMinimumChainWork.
         self.setup_nodes()
 
     def run_test(self):
-        # Setup the p2p connections
-        # test_node connects to node0 (not whitelisted)
         test_node = self.nodes[0].add_p2p_connection(P2PInterface())
-        # min_work_node connects to node1 (whitelisted)
         min_work_node = self.nodes[1].add_p2p_connection(P2PInterface())
 
         # 1. Have nodes mine a block (leave IBD)
@@ -226,7 +218,7 @@ def run_test(self):
         self.nodes[0].getblock(all_blocks[286].hash)
         assert_equal(self.nodes[0].getbestblockhash(), all_blocks[286].hash)
         assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, all_blocks[287].hash)
-        self.log.info("Successfully reorged to longer chain from non-whitelisted peer")
+        self.log.info("Successfully reorged to longer chain")
 
         # 8. Create a chain which is invalid at a height longer than the
         # current chain, but which has more blocks on top of that