bitcoin / bitcoin

Bitcoin Core integration/staging tree
https://bitcoincore.org/en/download
MIT License
78.98k stars 36.27k forks source link

Make all RPCs take a blockhash argument / allow rpc leasing? #24471

Closed JeremyRubin closed 1 week ago

JeremyRubin commented 2 years ago

One thing I heard interviewing some users is that it's very common to sequence RPCs with periodic tip hash checks and restart if a new block comes in.

Something that might be helpful would be if Core could support a few different things in RPCs:

1) a tip hash argument that causes failure if the passed_tip != actual_tip 2) a hash argument that causes failure if the passed_tip \not \in header_chain

another thing that might help these types of users could be node leases whereby:

1) the node is given a lease which expires e.g. N seconds after the lease is granted/no message has been sent by the leasor, and the node will not process new messages until the lease is free or let go

Optionally, we could allow a distinct non expiring lease.

One might use such an API like (made up RPCs)

finished = false
while not finished:
    try:
        h <- getbestblockhash
        b <- getwalletbalance h
        a <- getnewaddress h
        send b a
        finished = true

or

L <- get_lease
b <- getwalletbalance h
a <- getnewaddress h
send b a
clear_lease L

Leasing might be better than just a blockhash argument because you can guarantee e.g. repeated calls to mempool operations are consistent. This could be problematic e.g. if a txn is in the mempool and you're trying to coordinate wallet software to detect txns paying you so that you might fee bump them with CPFP, it might disappear while your flow of txns is occuring.

maflcko commented 2 years ago

See also #18567

JeremyRubin commented 2 years ago

@danielabrozzoni @afilini do you have thoughts on this?

afilini commented 2 years ago

The approach described in #18567 is very reasonable and probably much easier to implement than locks and whatnot.

Assuming the calling software can complete its task within the block time (which probably applies to most applications) it should work very well.

I could try working on that as soon as I'm done with other BDK stuff.

JeremyRubin commented 2 years ago

will that be sufficient though? I would assume you'd want to e.g. lock the mempool? Maybe we can allow locking to just lock mempool and not blocks?

danielabrozzoni commented 2 years ago

I agree that returning the hash might not be sufficient for some use cases, but it's better than nothing.

Ideally, being able to lock the mempool and/or the blocks would be great (even tho I can't really think of any use case where one wants to lock just the mempool and not the blocks) - but, as Alekos was saying, it seems to be a bit complicated to implement.

promag commented 2 years ago

I agree that returning the hash might not be sufficient for some use cases, but it's better than nothing.

Can you describe those use cases?

danielabrozzoni commented 2 years ago

The main issue would be: your application wants to sync with bitcoind, in doing so it does subsequent calls to the RPC, all the calls return the same blockhash, so you would assume the calls' results are consistent between each others, but they might not. This can happen if the calls depend on the mempool, and the mempool somehow changed between the calls.

The RPC calls that come to my mind that would have this problem are listunspent and listtransactions. (If there's a way to ask listunspent and listtransactions to ignore the mempool, I'm not aware of it).

promag commented 2 years ago

The RPC calls that come to my mind that would have this problem are listunspent and listtransactions. (If there's a way to ask listunspent and listtransactions to ignore the mempool, I'm not aware of it).

It's the wallet that would have to ignore the mempool, I think that could be done. A wallet could be loaded but be "disconnected" from the mempool and the chain. In this case, the wallet RPC's should return the latest block processed by the wallet. However, I think the wallet should be "readonly" (like watchonly) and all spend calls should fail. The GUI would have to be aware of this as well.

I understand leases/locks can simplify sync on the client-side. However, I don't think it's that great because:

The following are what I consider good tips for sync methods:

maflcko commented 2 years ago

What about (optionally) also returning the mempool sequence number?

JeremyRubin commented 2 years ago

i think it wouldn't be too useful because very likely things will change during your call and you'll end up spinning for a long time.

JeremyRubin commented 2 years ago

wild option:

add a small LISP with some special ops for handling JSON (i think we already have something like this in the Gui console?) and then allow sending a fully atomic sequence of operations.

ryanofsky commented 2 years ago

add a small LISP with some special ops for handling JSON (i think we already have something like this in the Gui console?) and then allow sending a fully atomic sequence of operations.

You may be interested in #20273 which ports the gui nested logic to the cli. Presumably it could ported to run atomically in the server, too.

Also has anybody suggested extending JSONRPCExecBatch to support atomic operations (or just letting multiple RPC calls share state)?

willcl-ark commented 1 week ago

There doesn't seem to be much action on implementing this feature.

Please feel free to submit a pull request if this is something that you still want. This could be done for specific RPCs.