dagurval / electrum-cash-protocol

Electrum Cash protocol reference
MIT License
9 stars 6 forks source link

I added a new RPC method - `blockchain.utxo.get_info` #18

Open cculianu opened 4 years ago

cculianu commented 4 years ago

Hi Dagurval,

Cash Fusion and Stamp projects both kept complaining to me that there was no lightweight call for verifying if a UTXO exists and grabbing its value. I recently released a version of Fulcrum that implements this call:

https://electrum-cash-protocol.readthedocs.io/en/latest/protocol-methods.html#blockchain-utxo-get-info

How do you feel about adding it to the official spec? I didn't consult with you about it because I just started working on it last night spur of the moment and I finished it today and tested it and released a new Fulcrum that implements it. I wanted it out so it becomes part of the ecosystem. In particular Cash Fusion can benefit from this (whenever it gets finished).

dagurval commented 4 years ago

That sounds good to me. To be consistent with other RPC calls, I would suggest the following changes:

cculianu commented 4 years ago

I'm not sure how I feel about throwing an RPC error there. The reason I think that's not the same as transactions is -- transactions are generally "permanent" -- either they exist or they do not exist -- and once they exist it's rare for them to stop existing (with rare exceptions regarding mempool eviction). It's ok to throw RPC error there -- because it is a strong signal to the client that "they have the wrong idea about the world".

For UTXOs it's not quite the same given how ephemeral they are.

Let me be more specific: The usecase I'm thinking of is the Stamp wallet that Harry Barber is working on -- in his use case he will be pummelling the RPC with frequent blockchain.utxo.get_info requests to test if a utxo is "live" or is spent already. As such -- it just feels strange where the common-case will be to return "error" half the time. After all UTXOs are ephemeral. They exist for a while, then go away. 90%+ of all TXOs on the blockchain are spent!. This means at some point in the past, had this method existed.. they would have all returned non-error for some time, then all-of-a-sudden they return error. I don't know about you -- but when I see software do this I get annoyed. Return a result, not error. It's not my fault UTXO was spent. Error should be reserved for when it's the counterpary's fault. Not when it's an expected result.... It feels strange to throw RPC error in that regard. Error is YOUR fault as the client. A simple null result is nobody's fault. :)

Given the above considerations, I decided to go with null as a result. Note that other things in the API do return null rather than error, such as scripthash.subscribe ... Again in that case it's very common for a wallet to have a scripthash (because it generated addresses from an HD path) but for the blockchain to not have ever seen any such addresses. In fact, most of the addresses in a new wallet are like this -- they all just return null as the result. It's nobody's fault.

Additionally: I can think of 1 practical reason that error makes things harder: Error is that we want the client to use to differentiate between servers that have "accepted" this method and did some work, and they returned "nothing" versus servers that see this method and do not support it. It's the least burden on clients to return null and/or {} if the server supports the method, or test for "error" if the server does not.

dagurval commented 4 years ago

That rationale sounds solid to me

cculianu commented 4 years ago

Thanks. As for confirmed_height -- yeah you may be right. It might be too late to change it comfortably -- I already pushed out code live. Maybe next time I'll consult you first. In retrospect "height: 0" might have been clearer..

But to me .. that kind of bugged me.. "0" is a real height (there is only 1 tx on the blockchain with that height) -- so that ambiguity irked my need for perfection. And -1 already means "unconfirmed parent".. so .. omitting the key altogether seemed "right". LOL.

cculianu commented 4 years ago

Actually fulcrum supports asking about satoshi's first tx as a utxo! Look at this little badboy:

{"id":1,"method":"blockchain.utxo.get_info","params":["4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b", 0]}
{"id":1,"jsonrpc":"2.0","result":{"confirmed_height":0,"scripthash":"740485f380ff6379d11ef6fe7d7cdd68aea7f8bd0d953d9fdf3531fb7d531833","value":5000000000}}

There it's confimed in block 0! LOL!

This is why I don't like 0 to mean mempool.. it's ambiguous for 1 and only 1 tx on the blockchain.. making it.. imperfect.

cculianu commented 4 years ago

Just to beat a dead horse -- we have this ambiguity right now w.r.t. satoshi's first tx:

{"id":1,"method":"blockchain.scripthash.get_history","params":["740485f380ff6379d11ef6fe7d7cdd68aea7f8bd0d953d9fdf3531fb7d531833"]}
{"id":1,"jsonrpc":"2.0","result":[{"height":0,"tx_hash":"4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"},{"height":172165,"tx_hash":"08901b81e39bc61d632c93241c44ec3763366bd57444b01494481ed46079c898"}]}

Is that first tx in mempool? Is it in block 0? We know it's in block 0 -- but given the API spec -- it is misinterpreted as being in mempool. :(

dagurval commented 4 years ago

Satoshi will have to use another API for his wallet then 😂.

Can’t you just add “height” in next release? That would be backwards compatible and there is no rush

monsterbitar commented 4 years ago

in his use case he will be pummelling the RPC with frequent blockchain.utxo.get_info requests to test if a utxo is "live" or is spent already.

Wouldn't it make more sense to add a separate subscribe method then, so he can ask once, and learn when the change happens instead of polling?

monsterbitar commented 4 years ago

Just to beat a dead horse -- we have this ambiguity right now w.r.t. satoshi's first tx:

{"id":1,"method":"blockchain.scripthash.get_history","params":["740485f380ff6379d11ef6fe7d7cdd68aea7f8bd0d953d9fdf3531fb7d531833"]}
{"id":1,"jsonrpc":"2.0","result":[{"height":0,"tx_hash":"4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"},{"height":172165,"tx_hash":"08901b81e39bc61d632c93241c44ec3763366bd57444b01494481ed46079c898"}]}

Is that first tx in mempool? Is it in block 0? We know it's in block 0 -- but given the API spec -- it is misinterpreted as being in mempool. :(

For now, since this is how it already works, adding a note about it / documenting it might be easier than asking all (unknown) users who check for mempool by looking for 0 to change their behaviour.

monsterbitar commented 4 years ago

A thought about the 0=mempool setup, woudl it be possible to add non-numberic answers, like the string "mempool", or boolean "true"?

cculianu commented 4 years ago

Wouldn't it make more sense to add a separate subscribe method then, so he can ask once, and learn when the change happens instead of polling?

Yes, it would. But subscribe methods don't grow on trees! They have performance implications due to their persistence, etc.. so they require more engineering to implement right. :). T

That's certainly a good idea. If there's interest and if people actually use this method -- and if a subscribe method makes sense; that would be the next step.