Add /txs endpoint to submit tx packages #119
Conversation
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Related the hex vs tx thing, I just did exactly what the current
broadcast_rawdoes: 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?
There was a problem hiding this comment.
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 >.<)
|
I highly appreciate the effort to add See for example mempool/electrs#105 which seems to use another endpoint ( |
|
Hey, I agree with your proposed endpoints in mempool/electrs#105 and will happily adapt mine to that :) I might even adopt your implementation :) |
|
based on the comments by @stevenroose to adopt mempool/electrs#105 we will go ahead and merge that cc @moneyball |
Sounds like we're aligned on an API then. Thanks for resolving this! :) |
I'm not sure if those stability tests etc are ran in CI. I'll let CI run and please let me know if manual testing is advised. It won't work with versions before 28.0-rc anyway.
Is there a common practice here for what to do when an api endpoint requires a certain daemon version? I guess now the node will throw an error and it will just be propagated upwards to the user.