Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP_DO_NOT_MERGE] Add outpoint subscribe method #446

Closed
wants to merge 10 commits into from
75 changes: 74 additions & 1 deletion src/electrum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use anyhow::{bail, Context, Result};
use bitcoin::{
consensus::{deserialize, serialize},
hashes::hex::{FromHex, ToHex},
BlockHash, Txid,
BlockHash, OutPoint, Transaction, Txid,
};
use rayon::prelude::*;
use serde_derive::Deserialize;
Expand Down Expand Up @@ -275,6 +275,71 @@ impl Rpc {
Ok(status)
}

fn outpoint_subscribe(&self, (txid, vout): (Txid, u32)) -> Result<Value> {
let funding = OutPoint { txid, vout };

let funding_blockhash = self.tracker.get_blockhash_by_txid(funding.txid);
let spending_blockhash = self.tracker.get_blockhash_spending_by_outpoint(funding);

let mut funding_inputs_confirmed = true;

let funding_tx = match self.daemon.get_transaction(&funding.txid, funding_blockhash) {
Ok(tx) => tx,
Err(_) => return Ok(json!({})),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@Kixunil Kixunil Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

let funding_inputs = &funding_tx.input;

if funding_blockhash.is_none() && funding_inputs.iter().any(|txi| self.tracker.get_blockhash_by_txid(txi.previous_output.txid).is_none()) {
Pantamis marked this conversation as resolved.
Show resolved Hide resolved
funding_inputs_confirmed = false;
}
Pantamis marked this conversation as resolved.
Show resolved Hide resolved

let funding_height = match &funding_blockhash {
Some(funding_blockhash) => self.tracker.chain().get_block_height(funding_blockhash).ok_or_else(|| anyhow::anyhow!("Blockhash not found"))?,
None => 0,
};

let tx_candidates: Vec<Txid> = match spending_blockhash {
None => self.tracker.mempool().filter_by_spending(&funding).iter().map(|e| e.txid).collect(),
Some(spending_blockhash) => {
let mut txids: Vec<Txid> = Vec::new();
self.daemon.for_blocks(Some(spending_blockhash).into_iter(), |_, block| {
for tx in block.txdata.into_iter() {
if is_spending(&tx, funding) {
txids.push(tx.txid())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed !

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

})?;
txids
},
};

let mut spender_txids = tx_candidates.iter();

match spender_txids.next() {
Some(spender_txid) => if let Some(double_spending_txid) = spender_txids.next() {
return bail!("double spend of {}: {}", spender_txid, double_spending_txid)
} else {
match spending_blockhash {
Some(spending_blockhash) => {
let spending_height = self.tracker.chain().get_block_height(&spending_blockhash).ok_or_else(|| anyhow::anyhow!("Blockhash not found"))?;
return Ok(json!({"height": funding_height, "spender_txhash": spender_txid, "spender_height": spending_height}))
}
None => {
let spending_tx = self.daemon.get_transaction(&spender_txid, None)?;
if spending_tx.input.iter().any(|txi| self.tracker.get_blockhash_by_txid(txi.previous_output.txid).is_none()) {
if funding_inputs_confirmed {
return Ok(json!({"height": funding_height, "spender_txhash": spender_txid, "spender_height": -1}));
}
return Ok(json!({"height": -1, "spender_txhash": spender_txid, "spender_height": -1}));
} else {return Ok(json!({"height": funding_height, "spender_txhash": spender_txid, "spender_height": 0}));}
}
}
},
None => return Ok(json!({"height": funding_height})),
};
Pantamis marked this conversation as resolved.
Show resolved Hide resolved
}

fn transaction_broadcast(&self, (tx_hex,): (String,)) -> Result<Value> {
let tx_bytes = Vec::from_hex(&tx_hex).context("non-hex transaction")?;
let tx = deserialize(&tx_bytes).context("invalid transaction")?;
Expand Down Expand Up @@ -384,6 +449,7 @@ impl Rpc {
Call::Donation => Ok(Value::Null),
Call::EstimateFee(args) => self.estimate_fee(args),
Call::HeadersSubscribe => self.headers_subscribe(client),
Call::OutpointSubscribe(args) => self.outpoint_subscribe(args),
Call::MempoolFeeHistogram => self.get_fee_histogram(),
Call::PeersSubscribe => Ok(json!([])),
Call::Ping => Ok(Value::Null),
Expand Down Expand Up @@ -423,6 +489,7 @@ enum Call {
EstimateFee((u16,)),
HeadersSubscribe,
MempoolFeeHistogram,
OutpointSubscribe((Txid, u32)),
PeersSubscribe,
Ping,
RelayFee,
Expand All @@ -441,6 +508,7 @@ impl Call {
"blockchain.block.headers" => Call::BlockHeaders(convert(params)?),
"blockchain.estimatefee" => Call::EstimateFee(convert(params)?),
"blockchain.headers.subscribe" => Call::HeadersSubscribe,
"blockchain.outpoint.subscribe" => Call::OutpointSubscribe(convert(params)?),
"blockchain.relayfee" => Call::RelayFee,
"blockchain.scripthash.get_balance" => Call::ScriptHashGetBalance(convert(params)?),
"blockchain.scripthash.get_history" => Call::ScriptHashGetHistory(convert(params)?),
Expand Down Expand Up @@ -484,3 +552,8 @@ fn result_msg(id: Value, result: Value) -> Value {
fn error_msg(id: Value, error: RpcError) -> Value {
json!({"jsonrpc": "2.0", "id": id, "error": error.to_value()})
}

fn is_spending(tx: &Transaction, funding: OutPoint) -> bool {
tx.input.iter().any(|txi| txi.previous_output == funding)
}

4 changes: 4 additions & 0 deletions src/tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ impl Tracker {
self.index.chain()
}

pub(crate) fn mempool(&self) -> &Mempool {
&self.mempool
}

pub(crate) fn fees_histogram(&self) -> &Histogram {
self.mempool.fees_histogram()
}
Expand Down