rest: Support transaction broadcast in REST interface

danielabrozzoni commented at 2:36 pm on October 10, 2024: contributor

This adds a REST endpoint to allow broadcasting a transaction: POST /rest/broadcast.hex

This would make it possible to build electrum indexers that access the REST interface only, eliminating the need of accessing the RPC interface. This would improve UX (no need to authenticate) and security (only a small subset of methods are available from the REST interface). See also #31065 (comment)

The transaction hex must be passed in the body of the request; on success, the txid of the broadcasted transaction will be returned.

Fixes #31017

DrahtBot commented at 2:36 pm on October 10, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	1440000bytes
Concept NACK	stickies-v, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on Oct 10, 2024

danielabrozzoni force-pushed on Oct 10, 2024

danielabrozzoni marked this as ready for review on Oct 10, 2024

danielabrozzoni force-pushed on Oct 10, 2024

in src/rest.cpp:1033 in dff67a815c outdated

1028+        const CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
1029+        std::string err_string;
1030+        NodeContext* node = GetNodeContext(context, req);
1031+        if (!node) return false;
1032+
1033+        const node::TransactionError error = node::BroadcastTransaction(*node, tx, err_string, /*max_tx_fee=*/0, /*relay=*/true, /*wait_callback=*/true);

1440000bytes commented at 0:12 am on October 12, 2024:

Why zero max_tx_fee? sendrawtransaction RPC has a different default

danielabrozzoni commented at 7:43 pm on October 13, 2024:

I wanted to keep the API easier by passing only the transaction hex in the body, instead of asking for a whole JSON with optional maxfeerate/maxburnamount (same parameters as sendrawtransaction), and without the ability to set the maxfeerate, having zero (=suppress the check) is better than having the default max fee rate (otherwise you wouldn’t be able to broadcast a very high-paying transaction even if you really wanted to).

Additionally, I think most people that would use this API would use it on some external node and not their own, and so they wouldn’t delegate the feerate check to the REST interface, but they would do it themselves.

I’m also ok with adding a sentence to the documentation explaining that the maxfeerate is not checked here, although I fear that it might be confusing instead of helpful.

Anyways, I don’t really have strong opinions, so I’m totally ok with changing this :)

1440000bytes commented at 8:27 pm on October 13, 2024:

I don’t have a strong opinion either.

1440000bytes approved

1440000bytes commented at 8:27 pm on October 13, 2024: none

utACK https://github.com/bitcoin/bitcoin/pull/31065/commits/dff67a815c2780ce5328d65a77508d29d606c364

in test/functional/interface_rest.py:447 in dff67a815c outdated

439@@ -440,5 +440,20 @@ def run_test(self):
440         resp = self.test_rest_request(f"/deploymentinfo/{INVALID_PARAM}", ret_type=RetType.OBJ, status=400)
441         assert_equal(resp.read().decode('utf-8').rstrip(), f"Invalid hash: {INVALID_PARAM}")
442 
443+        self.log.info("Test the /broadcast URI")
444+        tx = self.wallet.create_self_transfer()
445+        resp_hex = self.test_rest_request("/broadcast", http_method='POST', req_type=ReqType.HEX, body=tx["hex"], ret_type=RetType.OBJ)
446+        assert_equal(resp_hex.read().decode('utf-8').rstrip(), tx["txid"])
447+        self.sync_all()

brunoerg commented at 8:44 am on October 14, 2024:

In dff67a815c2780ce5328d65a77508d29d606c364 “rest: Add “/broadcast” endpoint”: You’re using the node0 to create and broadcast the transaction. Then, you sync the nodes and check if the transaction is into node0’s mempool. Shouldn’t you check if the transaction is into node1’s mempool to ensure the broadcast worked?

danielabrozzoni commented at 10:11 am on October 14, 2024:

Oh yes, thank you for spotting this! Fixed in 1e07530836fad9899aecb45f4676e768f253fb25

danielabrozzoni force-pushed on Oct 14, 2024

brunoerg commented at 7:51 am on October 15, 2024: contributor

Worth adding a release note?

stickies-v commented at 10:47 am on October 16, 2024: contributor

I’d prefer not extending the REST interface if we don’t have to. This functionality is already well supported in the RPC, so this seems like purely a convenience endpoint?

I’ve asked for clarification in #31017, but until there’s a good reason to add this I’m Concept NACK.

danielabrozzoni commented at 3:17 pm on October 16, 2024: contributor

When building an electrum indexer, REST interface would be preferred over RPC as:

it’s easier to set up for users, since it doesn’t require any form of authentication
it’s safer, since from the REST interface only a small subset of methods can be accessed, none of which interact with the wallet or can be a security concern. While you can use rpcwhitelist to limit access to the RPC, this requires an extra step of configuration from users, who may forget to set it or misuse it. For example, the electrs documentation doesn’t even mention rpcwhitelist.

The only reason why indexers can’t use the REST interface right now is the missing broadcast method.

stickies-v commented at 4:13 pm on October 16, 2024: contributor

it’s easier to set up for users, since it doesn’t require any form of authentication

It seems electrs already uses the authentication cookie by default (as per their docs), so for most setups, this shouldn’t be a meaningful difference?

it’s safer…

It seems this new endpoint is aimed at less savvy users or users that don’t have an overly complex setup and e.g. just want to run an electrum server for personal use. In that scenario, I’m not sure the footguns of e.g forgetting to whitelist are super relevant, since they control both the bitcoind instance and the electrum server? I’m not implying that people shouldn’t always operate on a principle of least privilege, of course, just that practically I don’t see this meaningfully changing anything?

1440000bytes commented at 7:33 pm on October 16, 2024: none

By default, the RPC interface can only be accessed locally. However, a user may want to keep the RPC local and enable the REST interface with rest=1 to allow public access to available endpoints.

This pull request adds a basic endpoint to the REST interface for broadcasting transactions. As long as Bitcoin Core supports the REST interface and other endpoints exist, there shouldn’t be an issue with adding /rest/broadcast.hex or more endpoints to improve REST. Since the REST interface is disabled by default, it won’t affect users unless they add rest=1 in config.

Whether the REST interface should be supported or not seems off-topic for this pull request.

0xB10C commented at 9:15 am on October 17, 2024: contributor

One thing to consider is that this PR changes the REST interface from being read-only to also being able to “modify” node state (mempool in this case). This PR might introduce new considerations for someone exposing the REST interface to a local or even a public network: Someone who previously could only query information can now also publish transactions via the node.

1440000bytes commented at 9:58 am on October 17, 2024: none

One thing to consider is that this PR changes the REST interface from being read-only to also being able to “modify” node state (mempool in this case). This PR might introduce new considerations for someone exposing the REST interface to a local or even a public network: Someone who previously could only query information can now also publish transactions via the node.

This point could be added in docs so that users are careful about the security
Broadcasting transactions on behalf of others improves privacy
It is possible to modify the node state (mempool) using P2P messages

rest: Add "/broadcast" endpoint

This commit adds a REST endpoint to allow broadcasting a transaction:
`POST /rest/broadcast.hex`

The transaction hex must be passed in the body of the request;
on success, the txid of the broadcasted transaction will be returned.

Fixes #31017

6b32a886e3

danielabrozzoni force-pushed on Oct 17, 2024

danielabrozzoni commented at 2:40 pm on October 17, 2024: contributor

Worth adding a release note?

Thank you! Done in 6b32a886e351f0af3084ccab076a52e0eafc5aab @stickies-v:

It seems electrs already uses the authentication cookie by default (as per their docs), so for most setups, this shouldn’t be a meaningful difference?

I agree that adding this endpoint won’t make much difference in the short term, but I still believe that in the long term, if both existing and new indexers adopt the REST interface, it will improve UX and security.

It seems this new endpoint is aimed at less savvy users or users that don’t have an overly complex setup and e.g. just want to run an electrum server for personal use.

I think using REST could be useful even for more complex setups: if bitcoind and the indexer are on separate machines, being able to use an unauthenticated interface is slightly easier than having to set up rpcuser/rpcpassword.

In that scenario, I’m not sure the footguns of e.g forgetting to whitelist are super relevant, since they control both the bitcoind instance and the electrum server? I’m not implying that people shouldn’t always operate on a principle of least privilege, of course, just that practically I don’t see this meaningfully changing anything?

I’m not sure, but it seems to me that forgetting rpcwhitelist can be risky even in simpler setups. A bug in the indexer could potentially expose the RPC interface and allow wallet access without the entire machine being compromised. @0xB10C:

One thing to consider is that this PR changes the REST interface from being read-only to also being able to “modify” node state (mempool in this case). This PR might introduce new considerations for someone exposing the REST interface to a local or even a public network: Someone who previously could only query information can now also publish transactions via the node.

Yes, that’s a really good point, I can see how some users that already have the REST interface exposed wouldn’t want to allow transaction broadcast from their own node.

Do you think it would make sense to add an additional flag, like -allowrestbroadcast, defaulting to off?

1440000bytes approved

1440000bytes commented at 0:09 am on October 18, 2024: none

ACK https://github.com/bitcoin/bitcoin/pull/31065/commits/6b32a886e351f0af3084ccab076a52e0eafc5aab

Changes since last review:

Test fixed
Added in release notes

Do you think it would make sense to add an additional flag, like -allowrestbroadcast, defaulting to off?

I don’t think this needs to be done in bitcoin core. Users can return 403 for certain endpoints or configure security in their network accordingly. It could be mentioned in REST interface docs.

brunoerg commented at 10:01 am on October 22, 2024: contributor

One thing to consider is that this PR changes the REST interface from being read-only to also being able to “modify” node state (mempool in this case). This PR might introduce new considerations for someone exposing the REST interface to a local or even a public network: Someone who previously could only query information can now also publish transactions via the node.

Yes. Could it bring a security issue? Maybe someone could use this endpoint maliciously to spend resources?

laanwj commented at 4:55 pm on October 22, 2024: member

Concept NACK from me, sorry

i’d prefer to keep REST as read-only interface. As an unauthenticated interface, allowing any kind of posting exposes bitcoind to cross-site scripting attacks through web-browsers, for example.

There’s also a scope-creep issue. If we expose more and more of the RPC interface on REST, we end up having to maintain and test two interfaces for everything. And they’re not even so different.

This would improve UX (no need to authenticate) and security (only a small subset of methods are available from the REST interface).

i see two alternatives for accomplishing this:

Submit the transaction through P2P. This may be more of a hassle as the application has to speak bitcoin’s binary protocol, and there is less feedback than using RPC. But it always works.
Add a RPC user with only permission for submitblock. This can be accomplished by configuring multiple RPC users and setting a -rpcwhitelist=<whitelist> (as you already mention here ). This feature was added exactly for this use case! If it is too hard to configure, then we need to improve the documentation, not add things to the REST interface on a case-by-case basis.

1440000bytes commented at 8:42 pm on October 23, 2024: none

What is the use of REST interface that doesn’t have enough endpoints?

danielabrozzoni commented at 9:13 pm on October 23, 2024: contributor

Closing as it doesn’t seem this is the way to go, thanks everyone for the feedback! :)

danielabrozzoni closed this on Oct 23, 2024

stickies-v commented at 9:39 am on October 24, 2024: contributor

What is the use of REST interface that doesn’t have enough endpoints?

It allows for a binary response format, which is not possible with the (JSON-)RPC interface. For large queries, not having the JSON de/serialization round-trip can make a very meaningful performance difference. As such, I believe adding non-performance-critical REST endpoints is always going to be a tough sell, because they wouldn’t be meaningfully different from their RPC alternative, thus adding less benefit for the maintenance overhead.

laanwj commented at 11:49 am on October 24, 2024: member

It allows for a binary response format, which is not possible with the (JSON-)RPC interface. For large queries, not having the JSON de/serialization round-trip can make a very meaningful performance difference.

Exactly. Univalue was designed to be a simple and easy to review JSON library, it wasn’t designed for performance. The encoding/decoding overhead can matter.

What is the use of REST interface that doesn’t have enough endpoints?

That is a more existential question. There are people that want to deprecate, or at least spin off the REST interface as soon as an alternative high-performance interface is available. There will be one with the multiprocess work, which exposes a cap’n’proto based fast binary interprocess API.

Personally i think it’s fine to keep REST around for the forseeable future. Clearly people are using it, as an easy and relatively fast way to query public data from anything that has basic HTTP support. But that should imo be the entire scope.

rest: Support transaction broadcast in REST interface #31065

Code Coverage

Reviews