-
Notifications
You must be signed in to change notification settings - Fork 526
Add new test mempool accept method #1212
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
base: master
Are you sure you want to change the base?
Conversation
This commits adds a new rpc method (`blockchain.transaction.testmempoolaccept`) which proxies to Bitcoind's [testmempoolaccept](https://developer.bitcoin.org/reference/rpc/testmempoolaccept.html)
kwsantiago
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
Main issue is missing tests for this new RPC method. There should be basic test coverage for valid/invalid transaction scenarios at the minimum.
| .context("failed to broadcast transaction") | ||
| } | ||
|
|
||
| pub(crate) fn test_mempool_accept(&self, tx: &Transaction) -> Result<bool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider returning the full test result instead of just the allowed bool.
| let tx_bytes = Vec::from_hex(tx_hex).context("non-hex transaction")?; | ||
| let tx = deserialize(&tx_bytes).context("invalid transaction")?; | ||
| let accepted = self.daemon.test_mempool_accept(&tx)?; | ||
| Ok(json!(accepted)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to add something like:
Ok(json!({"allowed": accepted, "reject_reason": ...}))
Currently clients will get no diagnostic info when a tx is rejected.
| "blockchain.scripthash.subscribe" => Params::ScriptHashSubscribe(convert(params)?), | ||
| "blockchain.scripthash.unsubscribe" => Params::ScriptHashUnsubscribe(convert(params)?), | ||
| "blockchain.transaction.broadcast" => Params::TransactionBroadcast(convert(params)?), | ||
| "blockchain.transaction.testmempoolaccept" => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method name match Electrum protocol spec, or is this a custom extension? May want to document if the latter.
This commits adds a new rpc method
(
blockchain.transaction.testmempoolaccept) which proxies to Bitcoin'd testmempoolacceptSeeking a concept Ack/Nack here. My motivation for this PR comes from wanting to use Payjoin on wallets that use electrum as their primary block source. More info here.
Usage example