rpc: inform if privatebroadcast is enabled in getnetworkinfo #35267

We already call getprivatebroadcastinfo() here for the tx_originator, so we can assert it's True here instead of duplicating the call for it below.

done

I've reverted this change and moved back where it was inspired by your other comment. That's a function called multiple times, there is no need to perform that check each time it is called.

It doesn't seem straightforward to verify that this change to NetTest doesn't potentially cause issues to other tests that use this class. I think it'd be better to keep this self contained?

The only approach that I can imagine while containing the test inside test_getnetworkinfo() is this, but I don't really like it:

$ git diff
diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 1a4d1b44ff..9acfe8ef83 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -24,6 +24,7 @@ from test_framework.util import (
     assert_greater_than,
     assert_raises_rpc_error,
     p2p_port,
+    write_config,
 )
 from test_framework.wallet import MiniWallet
 
@@ -64,16 +65,12 @@ class NetTest(BitcoinTestFramework):
         self.num_nodes = 2
         self.extra_args = [
             ["-minrelaytxfee=0.00001000"],
-            [
-                "-minrelaytxfee=0.00000500",
-                "-privatebroadcast",
-            ],
+            ["-minrelaytxfee=0.00000500"],
         ]
         # Specify a non-working proxy to make sure no actual connections to public IPs are attempted
         for args in self.extra_args:
             args.append("-proxy=127.0.0.1:1")
         self.supports_cli = False
-        self.disable_autoconnect = False
 
     def run_test(self):
         # We need miniwallet to make a transaction
@@ -218,7 +215,17 @@ class NetTest(BitcoinTestFramework):
         assert_equal(info['connections_in'], 1)
         assert_equal(info['connections_out'], 1)
         assert_equal(info['privatebroadcast'], "disabled")
+
+        # Temporarily rewrite node 1's config without connect=0 so that
+        # -privatebroadcast (which is incompatible with -connect) can be tested.
+        write_config(self.nodes[1].datadir_path / "bitcoin.conf", n=1, chain=self.chain, disable_autoconnect=False)
+        self.restart_node(1, extra_args=["-minrelaytxfee=0.00000500", "-proxy=127.0.0.1:1", "-privatebroadcast"])
         assert_equal(self.nodes[1].getnetworkinfo()['privatebroadcast'], "enabled")
+        write_config(self.nodes[1].datadir_path / "bitcoin.conf", n=1, chain=self.chain, extra_config="bind=127.0.0.1\n")
+        self.restart_node(1, extra_args=self.extra_args[1])
+        self.connect_nodes(0, 1)
+        self.sync_all()
 
         with self.nodes[0].assert_debug_log(expected_msgs=['SetNetworkActive: false\n']):
             self.nodes[0].setnetworkactive(state=False)

Why not just enabled? The RPC is already getprivatebroadcast?

                {RPCResult::Type::BOOL, "enabled", "If -privatebroadcast is set"},

done

Needs a trailing newline to get CI to pass.

The `getprivatebroadcastinfo` RPC now exposes whether the `-privatebroadcast` startup option is enabled with a new response key `enabled`.

woops, fixed thanks

                {RPCResult::Type::BOOL, "enabled", "If -privatebroadcast is enabled"},

done

nit: we don't need to move this line. We could just push the enabled value below where we push transactions.

done

        # originator should report that privatebroadcast was set while the receiver should not

done

I think this line can still be moved to the beginning of the test. We don't need to check it for each broadcast.

done

Perhaps a "status" of Type::STR would be more expressive and extensible?

What other expressions could we need?

It's hard to predict how APIs get extended in the future. Booleans make backwards-compatible upgrades harder. See https://ariya.io/2011/08/hall-of-api-shame-boolean-trap/ . For boolean traps, I don't think the onus should be on the reviewer to think of what other situations might arise, but one that came to mind is: "status": "err-no-tor". I believe we currently don't allow starting up when no private network is found, but that may change in the future.

Fair enough, makes sense.

Using startup args seems brittle. Perhaps a better approach is to expose this in PeerManager::GetInfo(), and then surface it in getnetworkinfo() alongside the other peerman details?

Using startup args seems brittle

Did it this way to keep the format from sendrawtransaction

surface it in getnetworkinfo()

Concept~0 here, I am fine with both approaches. @andrewtoth what do you think?

In 30ba2d3eabfe31559604c641ff4c89b424583298

There's already a similar boolean present in PeerManager, would prefer for it to be used.

https://github.com/bitcoin/bitcoin/blob/10ca73c02cbff59f2134c0c7da3b8d0a7e727475/src/net_processing.h#L95-L96

then surface it in getnetworkinfo()

I'm leaning towards this based on the descriptions of both these RPCs.

getnetworkinfo - Returns an object containing various state info regarding P2P networking getprivatebroadcastinfo - Returns information about transactions that are currently being privately broadcast

getnetworkinfo's description is already sufficient if this new key is added there, but if added in getprivatebroadcastinfo then its description would need to be updated as well.

Question for reviewers, do you prefer status of private broadcast ? Following #35267 (review) idea of extension?

"private broadcast status (one of \"enabled\", \"disabled\")" has my preference