[WIP_DO_NOT_MERGE] Add outpoint subscribe method

[Pantamis]

Solve #444

Do not merge before spesmilo/electrumx#90 is merged.

Not tested properly because I have problems with electrs logging.

I can't find how to increase electrs verbosity, I use a .conf file and I set "verbose=4 timestamp=true" and "rust_backtrack = true" but p2p banch doesn't seem to take it into account (it only shows me the very first line of log at launch with config parameters, and no logging parameters are displayed).

@romanz how can I test that the request is working in command line ? (assuming electrs listen to port 50001 on localhost)

[romanz]

Many thanks for the contribution, will review this week :)

I can't find how to increase electrs verbosity

Good catch - the code in p2p is using https://github.com/env-logger-rs/env_logger/.
You can use the following command to increase the verbosity to DEBUG level:

export RUST_LOG=electrs=DEBUG

[Pantamis]

I changed the search heuristic and fix errors I found after testing with logging.

There is a difficult tradeoff between searching the spending transaction by scripthash and searching by block height first. I had to use both for reasonnable speed. So the current code first looks at transactions with same scripthash and then filter them to match the block height of spending transaction we can found in the index.

This choice is optimal for addresses with small history but my be bad if the address' history is larger than a block.

[Kixunil]

The method is supposed to subscribe the caller but this change doesn't seem to subscribe anything. Am I missing something? Besides, there are several DoS vectors which I believe really should be fixed.

[Kixunil]

Lines 286-293 are not Rust-idiomatic and it looks incorrect. If fetching the transaction fails it will pretend everything is OK and not subscribe. Also the client could DoS the server by sending out-of-bounds vout. I think this would be more appropriate (notice question mark at the end of zeroth line):

let funding_tx = self.daemon.get_transaction(&funding.txid, funding_blockhash)?;

let funding_inputs = &funding_tx.input;
let outpoint_script = &funding_tx.output.get(vout as usize).ok_or_else(|| anyhow::anyhow!("vout out-of-bounds")).script_pubkey;

This suggestion reports the error to the client. Perhaps it'd be better to still subscribe the client and only send notifications when it works again.

[Pantamis]

I will not hide that I am terrible at Rust coding since I am litteraly doing it for the first time while working on this PR x')

I need to return an empty json if the funding transaction doesn't exist according to Electrum 1.5 specifications. Is it handled with your suggested code ?

Good catch for the possible out-of-bounds vout, thanks !

[Kixunil]

I find let scripthash = outpoint_script.script_hash(); a bit nicer.

[Pantamis]

I agree but new_status wants a ScriptHash struct defined in types.rs and not from rust-bitcoin (which is what you obtain when taking .script_hash() )

Is it a problem with import lib rust-bitcoin ?

[Kixunil]

This will crash if funding transaction i unconfirmed -another DoS vector. I suggest this:

let funding_height = match &funding_blockhash {
    Some(funding_blockhash) => self.tracker.chain().get_block_height(funding_blockhash)?,
    None => -1,
};

[Pantamis]

I have to return 0 in the None case or the compiler complain that Neg is not implemented for usize. This is fine :) thanks for the catch !

[Kixunil]

Another unidiomatic code. This one is a bit tricky but I think let mut iter = match spending_blockhash { ... }; Should do the trick. Either from crate either will need to be used. This can prevent allocation. You can then check for double spends:

let spender_txid = iter.next().ok_or_else(|| anyhow::anyhow!("WTF spending tx not found but exist ?"));
if let Some(double_spending_txid) = iter.next() {
    // This will probably never happen so allocating is OK.
    let mut txids = vec![spender_txid, double_spending_txid];
    txids.extend(iter);
    bail!("double spend of {}: {:?}", txid, txids);
}

[Pantamis]

I think I understand what your suggestion is doing.

Notice that it is totally normal if we don't find any spending txid. The method can be called on a outpoint that is unspend and we have to return a JSON with only the height of the funding transaction. So I guess I have to change the argument in ok_or_else method of your suggestion.

[Kixunil]

Ah, right then instead of ok_or_else probably match and None should return the height as success?

[Pantamis]

I end up refactoring most of the code, it should be more idiomatic now ! Feel free to tell me if there is something else ! :)

[Pantamis]

Many thanks for your review @Kixunil ! I will answer to your suggestions.

Yes my code doesn't implement the subscription. In fact I just tried to "merge" the work done in #444 in branch outpoint-RPC which didn't implement the subscription to notifications. I don't know how to do it for now so it only returns the expected outpoint, which is what I needed to display spending transaction in BTC-RPC-Explorer.

The current code is really terrible with outpoints for which the scriptPubKey has a big history. new_status load all the blocks where such scriptPubKey appears, this is not good when its history is big. I want to restrict the block loading to the only block where the outpoint is spend, I am currently working on this.

[Kixunil]

I think the right way to implement subscriptions is to send them together with client channel over a bounded channel (std::sync::mpsc or one from crossbeam; bound = 1000 - shouldn't be more than 50M at 1000 clients) to notification thread which stores them in outpoint_subscriptions: HashMap<OutPoint, HashSet<Sender>> where Sender is the client channel. For each new transaction, notifier then iterates over inputs and notifies all clients subscribed to the outpoint from outpoint_subscriptions. This way there shouldn't be ridiculous lock contention or deadlocks.

[Pantamis]

In the last commit I revert back to first getting the transactions of the block where the output is spend than searching which one is indeed spending the outpoint. My first try was too slow because I fetched transactions one by one with rpc call to Core, which is very slow. Now electrs makes only one call for the whole block where the spending transaction is and then search which one is spending the outpoint. This is a lot faster and electrs is not stalling anymore when the scriptPubKey of outpoint has a big history !

When the spending transaction is in the mempool, I had to use filter_by_spending method of the mempool part to avoid scanning it completly to find if the spending transaction exists.

@Kixunil code changed a lot since scripthash is not needed anymore. I still have one or two of your suggestions I will include soon. I really don't know how to implement subscriptions as you describe, I don't think I have the skill to do it from scratch, so it could be in another PR or you could edit mine to include your implementation of subscriptions.

[Pantamis]

I refactored the unidiomatic code to follow @Kixunil suggestions ! I learn a lot by doing it so thank you :)

The only case not covered in the current code is if a spending transaction is not in the block at the height given by the index. It returns the height of funding transaction as success while it means the electrs index of spending transactions is wrong (so it should panic instead...)

I will squash the commits once @romanz reviewed the PR !

I will soon publish a PR in BTC-RPC-Explorer repo to display the spending transaction of STXOs in the explorer using the outpoint.subscribe method this is really a great thing to have :P

[Kixunil]

Lol, was just writing reply.

I wonder if this could be sped up even more by optionally using txindex...

I will have to look at the code later. I may also try implement subscriptions, shouldn't be too hard, I hope.
Is there any client that can be used for testing them? (regtest, please)

index of spending transactions is wrong (so it should panic instead...)

I'm not sure if panicking in case of DB corruption is the right approach. Panics are intended for programmer errors (like index out of bounds), database could be corrupted due to disk problem, so not necessarily programmer bug.

[Pantamis]

I wonder if this could be sped up even more by optionally using txindex...

I don't think it is the case. The ElectRS DB only tell you which block you should look at. So you must always load all the transactions of the block where the outpoint is spent to find which one is indeed spending it. So the most optimal thing you can do is asking to Core the whole block to immediatly get all the transactions in it and start exhaustive search in it. txindex would only help if you can restrict even more the set of potential spending transactions before even looking at their content, which is not the case here since only block height is known (even the scripthash only help you to find the good block... that you already know with the spending index). Unless index schema is changed, you can't do better.

I will have to look at the code later. I may also try implement subscriptions, shouldn't be too hard, I hope.

I think we should wait for @romanz opinion on this PR before adding subscriptions anyway so no hurry :)

Is there any client that can be used for testing them? (regtest, please)

I don't know any :/. If I had to bet on the first client to use them, I would bet on Sparrow. Maybe @craigraw would quickly implement them on his client on regtest if you ask him.

index of spending transactions is wrong (so it should panic instead...)

I'm not sure if panicking in case of DB corruption is the right approach. Panics are intended for programmer errors (like index out of bounds), database could be corrupted due to disk problem, so not necessarily programmer bug.

Yeah you are right. If this case happens we would have much bigger issues somewhere else anyway, so I guess it is fine to not catch it.

[Kixunil]

Wanted to go AFK but got a few more ideas. 😆

[Kixunil]

We should somehow distinguish transaction not found from other errors so we can log them.

[Pantamis]

Here I return an empty JSON to follow the Electrum 1.5 spec.

get_transaction is defined here:

electrs/src/daemon.rs

Lines 139 to 147 in d153bfa

	pub(crate) fn get_transaction(
	&self,
	txid: &Txid,
	blockhash: Option<BlockHash>,
	) -> Result<Transaction> {
	self.rpc
	.get_raw_transaction(txid, blockhash.as_ref())
	.context("failed to get transaction")
	}

There is a context defined in case of error. Is it sufficient ?

[Kixunil]

Ah, anyhow is screwing us. I think this should work:

let funding_tx =  match self.daemon.get_raw_transaction(&funding.txid, funding_blockhash.as_ref()) {
    Ok(tx) => tx,
    Err(bitcoincore_rpc::Error::JsonRpc(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { code: -5, .. }))) => return Ok(json!({})),
    Err(error) => return Err(error),
};

[Pantamis]

I found anyhow::Context weird to handle. I had to keep the same match but I add a new match in error case.

In the error case, I downcast anyhow to a bitcoinrpc::Error to remove the context and match its type as you did. If it does, I return the empty JSON, else I just return the error.

Is it the right way to do it ?

[Kixunil]

I personally find it messy even though not wrong. That's why I suggested calling get_raw_transaction directly instead of via get_transaction which does the same thing except messing error type. If @romanz has a different opinion, I don't mind.

[Pantamis]

Would calling get_raw_transaction require the rpc field of daemon to be made public ? I don't know what it would imply...

It looks like a matter of preferences here, I don't mind either haha

[Kixunil]

I don't know what it would imply...

AFAIK nobody uses electrs as a library so nothing substantial. I think whole daemon struct may be just relic of the past when there was manual implementation.

Thinking about it, maybe the correct way to do it would be to change get_transaction to return Result<Option<Transaction>> instead of Result<Transaction> and then convert not found errors to None similarly to how e.g. HashMap works except fallible.

[Kixunil]

I think this:

let iter = block.txdata.into_iter().filter(|tx| is_spending(&tx, funding)).map(|tx| tx.txid());
txids.extend(iter);

may be more efficient. Anyway, I 'm not happy the whole thing can't be iterator, so maybe I'll look into it.

[Pantamis]

Indeed !

I tried to make it an iterator too but I had great trouble because of the match :/

[Kixunil]

The ElectRS DB only tell you which block you should look at.

Ah, I see, we don't know txid.

so I guess it is fine to not catch it.

I think we should handle it but log it and return Err instead of panicking.

[Pantamis]

Second round of @Kixunil suggestions included ! (Including error handling in case of bad indexing !)
ElectRS can return spending transactions of one thousand outpoints in about two minutes ! (benched by asking him to return all spending transaction of the 7cdaf30c42eac0271108fe0aa170a02a31f729fb10079156f06611f0489f8f67 outputs)

[Kixunil]

As far as this incomplete change goes it looks good. I would personally prefer not doing downcast for stylistic reasons, but logically it's sound.

Subscription is obviously missing, that will be reviewed separately or I will look into it eventually. :)

[romanz]

Sorry for the delay, hopefully will more available in the next week :(

[romanz]

Closing in favor of #454.
Many thanks for the contribution!