rpc: make getprivatebroadcastinfo fail if privatebroadcast is not enabled #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

I think it would be better to use JSON's native true / false (and maybe change the key to "privatebroadcast_enabled").

It was like this in the first approach, but was changed to a more "upgradability" option. See #35267 (review) and #35267 (review) for context

Such an output in getnetworkinfo would make it more difficult to change the config option -privatebroadcast= in the future. For example - to change it to tri-state, or to string, or to add a new -privateboadcastwallet=. I am not saying we want to do these, but its good to keep the option to be able to and with this new addition in getnetworkinfo it will be harder.

To me it makes more sense to throw from the getprivatebroadcastinfo RPC if the config option -privatebroadcast= is set to 0. Like "do something with Foo" -> throws "Foo is not available".

That would resolve the original problem more elegantly - from Sparrow, query getprivatebroadcastinfo and if it throws then assume no private broadcast. Must be because either there is no such RPC (old Bitcoin Core) or because -privatebroadcast=0.

The first approach of this PR was to return True or False in getprivatebroadcastinfo, but the approach was changed after comments in #35267 (review).

Throwing in getprivatebroadcastinfo is a breaking change. I don't think it is critical as it has only been there for 1 release and probably there are no many applications depending on it, but I wanted to avoid it just in case.

Why is it a breaking change? getprivatebroadcastinfo RPC would throw an exception in e.g. Bitcoin Core 30.0 (because no such RPC exists). Do you think that some application (e.g. Sparrow or another) conditionally uses getprivatebroadcastinfo based on the version of Bitcoin Core?

It is a breaking change because in v31.0 it does not throw if it is not enabled. Idk if there's anyone currently using it (I don't think so):

I don't think it is critical as it has only been there for 1 release and probably there are no many applications depending on it

But, if possible, I always try to avoid breaking changes.

Ok, my take is that a new field in getnetworkinfo seems a bit like a clutter and it might make future changes more difficult because once exposed as "privatebroadcast": "enabled/disabled" we would have to keep it that way. It is kind of "frozen".

Throwing from getprivatebroadcastinfo to me seems like a better solution because any robust client program has to expect an exception anyway. And that combines well with checking whether this is an old Bitcoin Core which does not support the feature.

Concept~0 on it, tagging other reviewers to get more opinions, if we get consensus on just throwing I will change the approach to it happily. cc. @andrewtoth @stickies-v

To me it makes more sense to throw from the getprivatebroadcastinfo RPC if the config option -privatebroadcast= is set to 0

I agree, that's a nice solution. Keeps things self-contained. Not sure if I'd prefer using the existing RPC_METHOD_NOT_FOUND or adding a new RPC_METHOD_DISABLED. I think the former suffices, but could be open to the latter too.

If we go with this approach a new RPC_METHOD_DISABLED makes more sense imho. So we can clearly inform the user how to use it in the error message, and wallets can rely on error codes to know how to act.

"Private broadcast is not enabled. Ensure you're running Bitcoin Core v31 or higher, and start it with -privatebroadcast=1. "

Simple is better when it works well enough.

Throwing from getprivatebroadcastinfo to me seems like a better solution because any robust client

+1 on this being a better approach

I’ve seen RPC_MISC_ERROR used a couple of times when a feature isn’t enabled

 git grep -niE 'RPC_MISC_ERROR.*(not enabled|not configured|restart bitcoind|is unavailable)' -- 'src/**/*.cpp'

src/rpc/blockchain.cpp:2627:            throw JSONRPCError(RPC_MISC_ERROR, tfm::format("Index is not enabled for filtertype %s", filtertype_name));
src/rpc/blockchain.cpp:3001:        throw JSONRPCError(RPC_MISC_ERROR, tfm::format("Index is not enabled for filtertype %s", filtertype_name));
src/rpc/external_signer.cpp:46:            if (command == "") throw JSONRPCError(RPC_MISC_ERROR, "Error: restart bitcoind with -signer=<cmd>");
src/rpc/mempool.cpp:1025:                throw JSONRPCError(RPC_MISC_ERROR, "Mempool lacks a relevant spend, and txospenderindex is unavailable.");

To me, this might also be appropriate here.

That said, I think RPC_METHOD_DISABLED is also okay and could also be used for other options in the future.

Given that there are other RPCs using RPC_MISC_ERROR I decided to follow that.

I prefer RPC_METHOD_DISABLED, but using it implies or being inconsistent with other RPCs or changing all of them (braking change). So for the scope of this PR I kept RPC_MISC_ERROR. If a new error code is desired this can easily be updated together with all the other RPCs.

Given that there are other RPCs using RPC_MISC_ERROR I decided to follow that.

I don't think there's meaningful benefit for consumers to be consistent here. RPC_MISC_ERROR is the wrong error code, and I don't think we should push ourselves further into a wrong direction just to be consistent when we're already making a breaking change.

I don't think we should push ourselves further into a wrong direction just to be consistent when we're already making a breaking change

~Fair enough, I've added a previous commit adding the new error code. And updated the followings according.~

Fair enough, I've changed the error code.

I also think RPC_MISC_ERROR is the correct think to throw here. RPC_METHOD_NOT_FOUND is reserved for calls with a malformed method string, not calls that show up in the help, but are disabled for some reason.

The JSON-RPC specification states:

code message meaning -32601 Method not found The method does not exist / is not available.

"is not available" seems perfectly applicable here. Whether we implement getprivatebroadcast to throw a RPC_METHOD_NOT_FOUND or we implement conditional registration of the getprivatebroadcastinfo method is an implementation detail and should not leak into the interface. From a user's perspective, all that matters is whether the method is available or not.

not calls that show up in the help

I agree that the method showing up in help when it is not available is not ideal. I'm not sure if conditional registration is worth the overhead here, but I could supportive of that too. Regardless, the method should document that it is only available when -privatebroadcast is enabled.

Conditional registration seems pretty straightforward here (when relying on gArgs):

<details> <summary>git diff on e10f3ed76f</summary>

diff --git a/doc/release-notes-35267.md b/doc/release-notes-35267.md
index 07a36d4d13..d5d54c762f 100644
--- a/doc/release-notes-35267.md
+++ b/doc/release-notes-35267.md
@@ -1,5 +1,7 @@
 RPC
 ---
 
-The `getprivatebroadcastinfo` RPC now throws with error code `-32601` if the node is not running with the `-privatebroadcast` startup option enabled.
+The `getprivatebroadcastinfo` and `abortprivatebroadcast` RPCs are now only available
+when the node is running with the `-privatebroadcast` startup option enabled. When it is
+disabled, calling them returns the `-32601` ("Method not found") error.
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0ca56e7041..50ea62ff05 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1852,7 +1852,6 @@ PeerManagerInfo PeerManagerImpl::GetInfo() const
     return PeerManagerInfo{
         .median_outbound_time_offset = m_outbound_time_offsets.Median(),
         .ignores_incoming_txs = m_opts.ignore_incoming_txs,
-        .private_broadcast = m_opts.private_broadcast,
     };
 }
 
diff --git a/src/net_processing.h b/src/net_processing.h
index 630656e2af..4ae48f84e4 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -71,7 +71,6 @@ struct CNodeStateStats {
 struct PeerManagerInfo {
     std::chrono::seconds median_outbound_time_offset{0s};
     bool ignores_incoming_txs{false};
-    bool private_broadcast{DEFAULT_PRIVATE_BROADCAST};
 };
 
 class PeerManager : public CValidationInterface, public NetEventsInterface
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 6747458a12..acd5fa6e2f 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -178,9 +178,6 @@ static RPCMethod getprivatebroadcastinfo()
             const PeerManager& peerman{EnsurePeerman(node)};
             const auto txs{peerman.GetPrivateBroadcastInfo()};
 
-            if (!peerman.GetInfo().private_broadcast) {
-                throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Private broadcast is not enabled. Ensure you're running Bitcoin Core with -privatebroadcast=1.");
-            }
             UniValue transactions(UniValue::VARR);
             for (const auto& tx_info : txs) {
                 UniValue o(UniValue::VOBJ);
@@ -1534,8 +1531,6 @@ void RegisterMempoolRPCCommands(CRPCTable& t)
 {
     static const CRPCCommand commands[]{
         {"rawtransactions", &sendrawtransaction},
-        {"rawtransactions", &getprivatebroadcastinfo},
-        {"rawtransactions", &abortprivatebroadcast},
         {"rawtransactions", &testmempoolaccept},
         {"blockchain", &getmempoolancestors},
         {"blockchain", &getmempooldescendants},
@@ -1553,4 +1548,14 @@ void RegisterMempoolRPCCommands(CRPCTable& t)
     for (const auto& c : commands) {
         t.appendCommand(c.name, &c);
     }
+
+    if (gArgs.GetBoolArg("-privatebroadcast", DEFAULT_PRIVATE_BROADCAST)) {
+        static const CRPCCommand private_broadcast_commands[]{
+            {"rawtransactions", &getprivatebroadcastinfo},
+            {"rawtransactions", &abortprivatebroadcast},
+        };
+        for (const auto& c : private_broadcast_commands) {
+            t.appendCommand(c.name, &c);
+        }
+    }
 }
diff --git a/test/functional/p2p_private_broadcast.py b/test/functional/p2p_private_broadcast.py
index 7afedea476..dfea0ce979 100755
--- a/test/functional/p2p_private_broadcast.py
+++ b/test/functional/p2p_private_broadcast.py
@@ -223,11 +223,13 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
         tx_receiver = self.nodes[1]
         far_observer = tx_receiver.add_p2p_connection(P2PInterface())
 
-        self.log.info("Test getprivatebroadcastinfo fails if the node is running without -privatebroadcast set")
-        assert_raises_rpc_error(-32601, "Private broadcast is not enabled. Ensure you're running Bitcoin Core with -privatebroadcast=1.",
-            tx_receiver.getprivatebroadcastinfo)
-
-        wallet = MiniWallet(tx_originator)
+        self.log.info("Test the private broadcast RPCs are only registered when -privatebroadcast is set")
+        assert "getprivatebroadcastinfo" not in tx_receiver.help()
+        assert "abortprivatebroadcast" not in tx_receiver.help()
+        assert_raises_rpc_error(-32601, "Method not found", tx_receiver.getprivatebroadcastinfo)
+        assert_raises_rpc_error(-32601, "Method not found", tx_receiver.abortprivatebroadcast, "00" * 32)
+        assert "getprivatebroadcastinfo" in tx_originator.help()
+        assert "abortprivatebroadcast" in tx_originator.help()
 
         self.fill_node_addrman(node_index=0, address_types_to_add=[CAddress.NET_IPV4, CAddress.NET_IPV6, CAddress.NET_TORV3, CAddress.NET_I2P, CAddress.NET_CJDNS])
 

</details>

        # receiver is not running with -privatebroadcast set.

also, this is self-documented in the next log-line

dropped the comment

~Idk what is the number assignment process, just used 37 as is the next non-used number.~

Just curious, 21 was never assigned as a joke "21 is not an error" or something like that? :)

Can you explain the rationale for adding this new error, because I don't see how it's better than using the existing RPC_METHOD_NOT_FOUND. All the client needs to know is that they can't call this method on this server, and RPC_METHOD_NOT_FOUND handles that perfectly.

This really feels like an unnecessary addition to me, and adds a small extra burden on users because they now have to handle both the RPC_METHOD_NOT_FOUND (for <v31) and the RPC_METHOD_DISABLED errors (for v31 with this patch).

I have totally misread the error codes suggested in the previous thread. At some point I was thinking "why are they suggesting opposite stuff in each comment?" lol.

My bad, sorry. Fixed in the next push