[RPC] Remove waitforblock and waitfornewblock #10519

Removed because these RPCs are for tests only and are now unused

@theuni These RPCs were added in #8680, and never used (only waitforblockatheight was ever used). Is there a plan or expectation that they'll ever be needed?

I did not know it existed, but I think it is very useful, I would use it if I am sure it stays.

Easier to use than configuring blocknotify or ZMQ.

Would be nice to have a test for them, if we keep them.

@sipa If ever used, I figured it'd only be to make some python tests less cumbersome. @jnewbery Do you have any use for these? Otherwise, I'd be fine with removing.

@jnewbery Do you have any use for these? Otherwise, I'd be fine with removing.

Not currently, although they may be useful for some tests. On the other hand, it's just as easy to call getbestblockhash or getblock in a loop until the desired block has been accepted.

I have no issue with these being removed or staying (as long as they remain hidden and not part of the public API).

I am ok with removing or keeping and adding tests. Though, it seems the functionality is redundant:

• waitfornewblock() ---> waitforblockheight(current_height + 1)
• waitforblock(hash) ---> waitforblockheight(hash_height) && getblockhash(hash_height) == hash

utACK 5fc1253bb8b95d34880e565cb2aa193caab06d71

@MarcoFalke They're intended to be atomic operations as we were dealing with some test race issues a while back. They're indeed redundant if atomicity isn't needed.

well, I think they are useful. Instead of polling for new block I would definitively use that. I just did not know it existed.

utACK b69194916c5fc8ebdaebb04e3cc6c4aab37e1c49

rebased

utACK b691949.

IMO blocking or long pool RPC/REST calls should be avoided.

I think this should not be removed, but documented. Right now the only way to get block notification is to connect to to the P2P layer or use ZeroMQ. Both requires additional configuration. (also, I have never managed to make ZeroMQ notifications works with a .NET Client) I end up connecting to my node by P2P to get new blocks.

I would have known that such RPC method existed, I would have definitively used it in place of my current solution.

I think a WaitWalletTransaction would also be higly helpful, as right now, I either end up with polling listtransactions or even implement my own tracking not relying on Core, just because polling listtransaction is not reactive enough.

@NicolasDorier FYI there is also blocknotify

Same problem as ZeroMQ. It requires additional configuration on the bitcoin node, but it is even worse, because you need additional plumbing code so the blocknotify program can notify the program interested by the event (by pipe, other queueing mechanism, or HTTP callback which add a dependency to curl, which is not easy to install on all environments).... It add one more layer to the problem than ZeroMQ, this is a pain.

WaitBlock/WaitWalletTransaction makes things so much easy, it introduces zero dependency and require zero conf. Please do not remove it. :/

@NicolasDorier - Be careful about using these RPCs in product code. They were introduced as test tools only and there is no guarantee that they will work as you expect. For example, I just managed to break bitcoind's RPC server by running 4 waitfornewblock requests in parallel. Any request after that fails, and I don't think there's any way to cancel the waitfornewblock requests.

There are also no tests for them and they are hidden debug RPCs, so it's likely you'd be the only one using them, which increases the risk of regressions.

I'm slightly averse to long-polling RPCs in general, but I'm also not too concerned if these stay (with the provisos above).

Agree with @jnewbery. You can have a small utility to listen zmq messages and forward in the most suitable transport for your .NET app.

And I think doing that to some kind of queue would benefit both parts, specially if the queue has means to easily notify your app.

@jnewbery I know there is a maximum number of concurrent requests, this can be notified to the user when it happens. @promag I enumerated the problems of using ZMQ above. This just make it harder to build stuff on top of Core by making it require more configuration and dependency. It is easier to connect to the P2P layer and listen for new block than it is to setup ZMQ, and listening through it with a cross plateform library.

making it require more configuration @NicolasDorier IMO that´s fine. Even if there was websocket support, it should have/require configuration before it could be used. You also configure RPC right?

and dependency.

I agree that there should be pure HTTP notifications but for the moment adding some zmq client to the playground doesn't seem like a real issue.

Another option is to allow only one waitforblock and waitfornewblock consumer?

@promag no I do not configure RPC. If bitcoin is running with default parameters, I can connect to it through the wellknown port and wellknown cookiefile.

I do not use either blocknotify and ZMQ because it makes my users config stuff and add dependencies. This requires me to document and to know how to install those dependencies on the plateforms I support (as well as testing them).

At one point it will be easier to develop a wallet connected via P2P to a trusted node from scratch than using RPC, which is a shame.

Rebased

If #7949 would be merged, I would see no problem removing this rpc method.

Needs rebase, if still relevant. (seems to have been dorment for a long time)

Seems like nobody is using it, and on my side I am not at ease using something that might disappear tomorrow, so feel free to delete it.

But I still think it is a shame, this is the most useful RPC call ever! :(

Rebased

utACK c26ffdb5adcbab58ab12509d22123b96ae2ae4a8, but I'd prefer if we removed all of their kind.

NACK.

waitfor... RPC calls are very useful. They should be documented rather than removed. If they ruin some coding best practices, then remove it, otherwise don't.

Imagine a wallet daemon that gives you p2pkh only through RPC, it gives you p2wpkh only through ZMQ and it gives you p2wpkh over p2sh only through websockets. Sure that works, but you must have 3 different communication channels properly implemented and configured in order to get all three of them. The more communication protocols a software uses the less reliable it is. Networking may be the trickiest coding challenge, where so many little things can go wrong.

https://github.com/MetacoSA/NBitcoin/pull/363 https://github.com/nopara73/MagicalCryptoWallet/issues/32 https://github.com/nopara73/MagicalCryptoWallet/issues/40

As both @NicolasDorier and @nopara73 are apparently using these calls (or would if they aren't goin to be removed) and think they are useful, I don't think they should be removed, then. Closing.

Personally we were using but removed them, due to this change, so it's fine by me. Although I'd still not be happy if the user space breaks (like ever.) But that's more an API design philosophical question.

Although I'd still not be happy if the user space breaks (like ever.)

These RPC methods were introduced as hidden calls for testing only. They shouldn't be used in production systems. They should either be removed (since they're unused in testing now), or made unhidden and fully supported. I'd vote for removing them.

Marked up for grabs for someone to address my feedback: #10519 (review)