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.

Sign up for GitHub

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
Closed

[WIP_DO_NOT_MERGE] Add outpoint subscribe method #446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
66fd923
Add outpoint subscribe method
Pantamis Jul 24, 2021
6f7a679
Change search heuristic for scripthash then block
Pantamis Jul 26, 2021
e882b36
Add outpoint subscribe method
Pantamis Jul 24, 2021
0751b34
remove parenthesis
Pantamis Jul 26, 2021
245390d
limiting search to spending block
Pantamis Jul 26, 2021
857c451
More idiomatic code
Pantamis Jul 28, 2021
4bdeed0
Rust idiomatic code
Pantamis Jul 28, 2021
6351ef7
Clean up unused stuff for lower diff
Pantamis Jul 28, 2021
f927f67
improve the great match
Pantamis Jul 28, 2021
9b63419
handle not existing transaction error
Pantamis Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 91 additions & 1 deletion src/electrum.rs
View file
Open in desktop
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,88 @@ 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 inputs_funding_confirmed = true;

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

if funding_tx.is_err() {
return Ok(json!({}));
}

let funding_inputs = &funding_tx.as_ref().unwrap().input;
let outpoint_script = &funding_tx.as_ref().unwrap().output.get(vout as usize).unwrap().script_pubkey;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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 !


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
inputs_funding_confirmed = false;
}
Pantamis marked this conversation as resolved.
Show resolved Hide resolved


let scripthash = ScriptHash::new(&outpoint_script);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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 ?

let outpoint_scripthash_status = self.new_status(scripthash);

let funding_height = self.tracker.chain().get_block_height(&funding_blockhash.unwrap()).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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 !


if spending_blockhash.is_none() {
let txids: Vec<Txid> = outpoint_scripthash_status.unwrap()
.get_mempool(&(self.tracker).mempool())
.iter()
.filter_map(|tx_entry| {
let mempool_tx = self.daemon.get_transaction(&tx_entry.txid, None);
if is_spending(&mempool_tx.unwrap(), funding) {
Some(tx_entry.txid)
} else {
None
}
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 :/

})
.collect();
if txids.len() > 1 {
return Ok(panic!("double spend of {}: {:?}", txid, txids));
}
if !txids.is_empty() {
let spending_tx = self.daemon.get_transaction(&txids[0], spending_blockhash);
if spending_tx.unwrap().input.iter().any(|txi| self.tracker.get_blockhash_by_txid(txi.previous_output.txid).is_none()) {
if inputs_funding_confirmed {
return Ok(json!({"height": 0, "spender_txhash": txids[0], "spender_height": -1}));
}
return Ok(json!({"height": -1, "spender_txhash": txids[0], "spender_height": -1}));
}
return Ok(json!({"height": funding_height, "spender_txhash": txids[0], "spender_height": 0}));
}
return Ok(json!({"height": funding_height}));
}

let spending_height = self.tracker.chain().get_block_height(&spending_blockhash.unwrap()).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 end up refactoring most of the code, it should be more idiomatic now ! Feel free to tell me if there is something else ! :)


let txids: Vec<Txid> = outpoint_scripthash_status.unwrap()
.get_confirmed(&self.tracker.chain())
.iter()
.filter(|confirmed_entry| {
confirmed_entry.height == spending_height
})
.filter_map(|script_block_entry| {
let script_block_tx = self.daemon.get_transaction(&script_block_entry.txid, spending_blockhash);
if is_spending(&script_block_tx.unwrap(), funding) {
Some(script_block_entry.txid)
} else {
None
}
})
.collect();

if txids.len() > 1 {
return Ok(panic!("double spend of {}: {:?}", txid, txids));
}
if txids.is_empty() { return Ok(json!({"WTF": "spending tx not found but exist ?"})); }
return Ok(json!({"height": funding_height, "spender_txhash": txids[0], "spender_height": spending_height}));

}

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 +466,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 +506,7 @@ enum Call {
EstimateFee((u16,)),
HeadersSubscribe,
MempoolFeeHistogram,
OutpointSubscribe((Txid, u32)),
PeersSubscribe,
Ping,
RelayFee,
Expand All @@ -441,6 +525,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 +569,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)
}

8 changes: 4 additions & 4 deletions src/status.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ impl TxEntry {
}

pub(crate) struct ConfirmedEntry {
txid: Txid,
height: usize,
pub txid: Txid,
pub height: usize,
}

impl ConfirmedEntry {
Expand All @@ -62,8 +62,8 @@ impl ConfirmedEntry {
}

pub(crate) struct MempoolEntry {
txid: Txid,
has_unconfirmed_inputs: bool,
pub txid: Txid,
pub has_unconfirmed_inputs: bool,
fee: Amount,
}

Expand Down
8 changes: 8 additions & 0 deletions src/tracker.rs
View file
Open in desktop
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 Expand Up @@ -101,4 +105,8 @@ impl Tracker {
// Note: there are two blocks with coinbase transactions having same txid (see BIP-30)
self.index.filter_by_txid(txid).next()
}

pub fn get_blockhash_spending_by_outpoint(&self, funding: OutPoint) -> Option<BlockHash> {
self.index.filter_by_spending(funding).next()
}
}