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

Jump to bottom

Add /txs endpoint to submit tx packages #119

Open
wants to merge 1 commit into
base: new-index
Choose a base branch
from
Open

Add /txs endpoint to submit tx packages #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions src/daemon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,19 @@ impl Daemon {
)
}

pub fn broadcast_pkg_raw(&self, txshex: &[String]) -> Result<Vec<Txid>> {
let res = self.request("submitpackage", json!([txshex]))?;
res.as_object().chain_err(|| "no object")?
.get("tx-results").chain_err(|| "missing key tx-results")?
.as_object().chain_err(|| "tx-results not an object")?
.into_iter().map(|(_, tx_ret)| tx_ret
.as_object().chain_err(|| "tx result not an object")?
.get("txid").chain_err(|| "missing txid field")?
.as_str().chain_err(|| "txid not a string")?
.parse().chain_err(|| "invalid txid")
).collect()
}

// Get estimated feerates for the provided confirmation targets using a batch RPC request
// Missing estimates are logged but do not cause a failure, whatever is available is returned
#[allow(clippy::float_cmp)]
Expand Down
8 changes: 8 additions & 0 deletions src/new_index/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ impl Query {
Ok(txid)
}

pub fn broadcast_pkg_raw(&self, txshex: &[String]) -> Result<Vec<Txid>> {
let txids = self.daemon.broadcast_pkg_raw(txshex)?;
for txid in &txids {
self.mempool.write().unwrap().add_by_txid(&self.daemon, &txid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding the transactions to the Mempool one-by-one will no longer work once #89 is merged. This is due to 5c8c785, which ensures that the electrs mempool store is always complete and consistent, with no possibility of missing parents. The previous behavior where children could be added to the store without their parents was wrong.

For this to work, you'll have to call Mempool::add() with a HashMap containing the entire package.

The transactions could be retrieved from bitcoind like broadcast_raw()/add_by_txid() are currently implemented, but it would be nicer to use the transactions we already have from txshex. Possibly with a sanity check that they match the txids returned from submitpackage. And it would probably be a good idea to attempt deserializing the transactions before sending them over them to bitcoind, as an additional sanity check.

Tx broadcast is the only API endpoint that results in a json-rpc call being made to bitcoind, all other queries are served directly from electrs' own db. So we should give some extra care to what we allow to be passed through. Probably a good idea to also limit the package size by tx count and/or bytes.

Apologies that this complicates your PR 😅 Pre-validation and limits were probably needed regardless, but supporting package submission makes them a higher priority.

Thanks for sending this! Package submission would be a great addition to the API.

Copy link
Author

Choose a reason for hiding this comment

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

Related the hex vs tx thing, I just did exactly what the current broadcast_raw does: it does the same thing.

Also, generally the packages are ordered so that the parents go before the child, but you're right that's probably not always the case. Adding by hashmap is not a big deal I think.

I agreed that limits on package size make a lot of sense. Any ideas for sensible limits?

Copy link
Author

@stevenroose stevenroose Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can put a bit more work in this, I'm currently actually in need for this API. I have no idea how to effectively test electrs, though. So I'll probably contact you about that :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related the hex vs tx thing, I just did exactly what the current broadcast_raw does: it does the same thing.

Right. It wouldn't be terrible if that remained that way but would be nice to improve it :)

Also, generally the packages are ordered so that the parents go before the child, but you're right that's probably not always the case.

Perhaps worth mentioning that even if bitcoind sends them back that way, the ordering might still be lost on the way because tx-results is returned as a JSON object that doesn't have a guaranteed order (like an array would).

I agreed that limits on package size make a lot of sense. Any ideas for sensible limits?

I'm not sure really, open to suggestions. but I guess we could start with a relatively low limit and increase it if needed, maybe something like 5 transactions and 2500 bytes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing to note is that Mempool::add() expects to be called with new transactions only, so you'd need to remove any transactions that are already known from the hashmap.

(The existing code shouldn't ever add known transactions, but Mempool::add() should still probably be fixed up to handle this more sanely - either skip known transactions or fail with an error. Right now it will mess up the indexes >.<)

}
Ok(txids)
}

pub fn utxo(&self, scripthash: &[u8]) -> Result<Vec<Utxo>> {
let mut utxos = self.chain.utxo(scripthash, self.config.utxos_limit)?;
let mempool = self.mempool();
Expand Down
8 changes: 8 additions & 0 deletions src/rest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,14 @@ fn handle_request(
.map_err(|err| HttpError::from(err.description().to_string()))?;
http_message(StatusCode::OK, txid.to_string(), 0)
}
(&Method::POST, Some(&"txs"), None, None, None, None) => {
let txshex = serde_json::from_slice::<Vec<String>>(&body[..]).map_err(|_| {
HttpError(StatusCode::BAD_REQUEST, "expected array of hexes".into())
})?;
let txids = query.broadcast_pkg_raw(&txshex)
.map_err(|err| HttpError::from(err.description().to_string()))?;
http_message(StatusCode::OK, serde_json::to_vec(&txids)?, 0)
}

(&Method::GET, Some(&"mempool"), None, None, None, None) => {
json_response(query.mempool().backlog_stats(), TTL_SHORT)
Expand Down
Loading