feat(client): add async and blocking clients to submit txs package #114

acidbunny21 · 2025-01-22T08:58:15Z

Counterpart PR for Blockstream/electrs#119 to enable submitpackage API

src/api.rs

src/async.rs

src/blocking.rs

stevenroose

looks good, let just some nits. can confirm that the calls match the endpoint added in my pr on esplora (Blockstream/electrs#119)

src/blocking.rs

coveralls · 2025-01-27T14:07:43Z

Pull Request Test Coverage Report for Build 13591205851

Details

34 of 89 (38.2%) changed or added relevant lines in 2 files are covered.
8 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-3.6%) to 84.553%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/async.rs	14	41	34.15%
src/blocking.rs	20	48	41.67%

Files with Coverage Reduction	New Missed Lines	%
src/async.rs	1	72.66%
src/blocking.rs	7	72.47%

Totals
Change from base Build 13552496536:	-3.6%
Covered Lines:	1155
Relevant Lines:	1366

💛 - Coveralls

tnull · 2025-01-28T08:31:35Z

Cargo.toml

@@ -18,6 +18,7 @@ path = "src/lib.rs"

 [dependencies]
 serde = { version = "1.0", features = ["derive"] }
+serde_json = "1.0"


Can we disable default features here and only enable the features we actually need?

tnull · 2025-01-28T08:33:04Z

src/api.rs

+
+#[derive(Deserialize, Debug)]
+pub struct TxResult {
+    pub txid: String,


Let's deserialize these into actual types, not String everywhere (see https://github.com/rust-bitcoin/corepc/blob/b62e8c84e3271e57b10849d27191c0357711660b/types/src/model/raw_transactions.rs#L17-L58 for examples).

updated. I also added the same comments on the fields as the ones in rust-bitcoin

tnull · 2025-01-28T08:35:53Z

src/async.rs

+    /// if  `maxburnamount` is provided, any transaction
+    /// with higher provably unspendable outputs amount
+    /// will be rejected
+    pub async fn broadcast_package(


Lets call this submit_package to avoid introducing a new separate terminology?

tnull · 2025-01-29T11:26:02Z

src/api.rs

+pub struct SubmitPackageResult {
+    /// The transaction package result message. "success" indicates all transactions were accepted
+    /// into or are already in the mempool.
+    pub package_msg: String,


Do we know if the variants here are finite? Do we see a chance to parse this into an enum?

Agreed, that'd be best.

i agree that would be best, but as I can see here, there is no enum defined for that field. I'd be happy to update that part as soon as it is upgraded to one there

oleonardolima

It could use a test with mempool/electrs implementation, while it's not available in blockstream/electrs and electrsd, as I've looked on the other PR and looks like it's following the same API.

oleonardolima · 2025-02-03T03:09:40Z

src/api.rs

+    /// Transaction results keyed by [`Wtxid`].
+    #[serde(rename = "tx-results")]
+    pub tx_results: HashMap<Wtxid, TxResult>,
+    /// List of txids of replaced transactions.
+    #[serde(rename = "replaced-transactions")]
+    pub replaced_transactions: Option<Vec<Txid>>,


Any reason not to keep the snake_case standard to this endpoint in electrs implementation, as the other APIs do?

Bitcoin Core's RPC API is inconsistent in that it mixes snake and dash-case. I assume Esplora just re-exposes the identical field names.

oleonardolima · 2025-02-03T03:13:21Z

src/api.rs

+pub struct SubmitPackageResult {
+    /// The transaction package result message. "success" indicates all transactions were accepted
+    /// into or are already in the mempool.
+    pub package_msg: String,


Agreed, that'd be best.

acidbunny21 · 2025-02-03T15:23:03Z

It could use a test with mempool/electrs implementation, while it's not available in blockstream/electrs and electrsd, as I've looked on the other PR and looks like it's following the same API.

I'm okay with adding a test, but then we need to bump electrsd to 0.30 which introduce some breaking changes on bitcoind. Might be good to first bump it in a separated PR

src/async.rs

oleonardolima · 2025-02-03T16:53:47Z

src/async.rs

+            serde_json::to_string(&serialized_txs)
+                .unwrap()
+                .as_bytes()
+                .to_vec(),


Can't we just send the hex as we do for broadcast tx, it looks like the parsing in blockstream/electrs PR is the same as broadcast tx one.

mmmh i don't think so, the parsing in blockstream/electrs PR expects a json array so we cannot simply send a single hex

oleonardolima · 2025-02-03T16:54:40Z

src/async.rs

+        if let Some(maxfeerate) = maxfeerate {
+            request = request.query(&[("maxfeerate", maxfeerate.to_string())])
+        }
+
+        if let Some(maxburnamount) = maxburnamount {
+            request = request.query(&[("maxburnamount", maxburnamount.to_string())])
+        }
+
+        let response = request.send().await?;
+        if !response.status().is_success() {
+            return Err(Error::HttpResponse {
+                status: response.status().as_u16(),
+                message: response.text().await?,
+            });
+        }
+
+        response
+            .json::<SubmitPackageResult>()
+            .await
+            .map_err(Error::Reqwest)


I'm quite divided if we should repurpose post_request_hex to handle this too, instead of having the logic here, or maybe have another post_request_bytes if we stick to it.

~~mmmh i couldn't find a good way to factorize them, mainly because submitpackage request takes some query params~~

EDIT: i turned post_request_hex into post_request_bytes that accepts queryparams in addition to body

Great, thanks! I'll do another round of review.

oleonardolima · 2025-02-10T16:28:20Z

It could use a test with mempool/electrs implementation, while it's not available in blockstream/electrs and electrsd, as I've looked on the other PR and looks like it's following the same API.

I'm okay with adding a test, but then we need to bump electrsd to 0.30 which introduce some breaking changes on bitcoind. Might be good to first bump it in a separated PR

Yes, agree that it can be done in another PR. Another way that I thought was adding a test (under #[ignore]) that uses live mempool.space to test it, so we could run locally and in another step on CI while we don't have a new release on electrsd with the blockstream's esplora changes. However I think it's better to just wait until Blockstream/electrs#119 is release and just bump everything.

acidbunny21 · 2025-02-21T17:36:10Z

@oleonardolima it looks like the pipeline failures are caused by something unrelated with the PR, could you help please? :)

oleonardolima · 2025-02-26T18:42:12Z

@oleonardolima it looks like the pipeline failures are caused by something unrelated with the PR, could you help please? :)

It's caused due to recent native-tls release which bumped their MSRV to 1.80.0, it should be fine after #116 lands and a rebase should fix it.

ErikDeSmedt suggested changes Jan 22, 2025

View reviewed changes

src/api.rs Outdated Show resolved Hide resolved

src/api.rs Outdated Show resolved Hide resolved

src/async.rs Outdated Show resolved Hide resolved

src/blocking.rs Outdated Show resolved Hide resolved

stevenroose reviewed Jan 22, 2025

View reviewed changes

src/blocking.rs Outdated Show resolved Hide resolved

src/blocking.rs Outdated Show resolved Hide resolved

src/blocking.rs Outdated Show resolved Hide resolved

acidbunny21 force-pushed the submit-tx-pkg-clients branch 2 times, most recently from 6a22216 to 01fb9c9 Compare January 23, 2025 13:41

oleonardolima assigned acidbunny21 Jan 27, 2025

oleonardolima added the enhancement New feature or request label Jan 27, 2025

tnull reviewed Jan 28, 2025

View reviewed changes

acidbunny21 force-pushed the submit-tx-pkg-clients branch from 01fb9c9 to a5322c6 Compare January 29, 2025 08:34

tnull reviewed Jan 29, 2025

View reviewed changes

oleonardolima reviewed Feb 3, 2025

View reviewed changes

oleonardolima mentioned this pull request Feb 4, 2025

deps(blockstream/electrs): bump blockstream/electrs to latest under a new feature RCasatta/electrsd#93

Open

acidbunny21 force-pushed the submit-tx-pkg-clients branch from a5322c6 to 6147e74 Compare February 7, 2025 04:40

acidbunny21 force-pushed the submit-tx-pkg-clients branch from 6147e74 to c50be44 Compare February 28, 2025 15:25

feat(client): add async and blocking clients to submit txs package

7886297

acidbunny21 force-pushed the submit-tx-pkg-clients branch from c50be44 to 7886297 Compare March 1, 2025 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): add async and blocking clients to submit txs package #114

feat(client): add async and blocking clients to submit txs package #114

acidbunny21 commented Jan 22, 2025 •

edited

Loading

stevenroose left a comment

coveralls commented Jan 27, 2025 •

edited

Loading

tnull Jan 28, 2025

acidbunny21 Jan 29, 2025

tnull Jan 28, 2025

acidbunny21 Jan 29, 2025

tnull Jan 28, 2025

acidbunny21 Jan 29, 2025

tnull Jan 29, 2025 •

edited

Loading

oleonardolima Feb 3, 2025

acidbunny21 Feb 3, 2025

oleonardolima left a comment

oleonardolima Feb 3, 2025

tnull Feb 3, 2025 •

edited

Loading

oleonardolima Feb 3, 2025

acidbunny21 commented Feb 3, 2025

oleonardolima Feb 3, 2025

acidbunny21 Feb 7, 2025

oleonardolima Feb 3, 2025

acidbunny21 Feb 7, 2025 •

edited

Loading

oleonardolima Feb 10, 2025

oleonardolima commented Feb 10, 2025

acidbunny21 commented Feb 21, 2025

oleonardolima commented Feb 26, 2025

feat(client): add async and blocking clients to submit txs package #114

Are you sure you want to change the base?

feat(client): add async and blocking clients to submit txs package #114

Conversation

acidbunny21 commented Jan 22, 2025 • edited Loading

stevenroose left a comment

Choose a reason for hiding this comment

coveralls commented Jan 27, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13591205851

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acidbunny21 commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acidbunny21 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima commented Feb 10, 2025

acidbunny21 commented Feb 21, 2025

oleonardolima commented Feb 26, 2025

acidbunny21 commented Jan 22, 2025 •

edited

Loading

coveralls commented Jan 27, 2025 •

edited

Loading

tnull Jan 29, 2025 •

edited

Loading

tnull Feb 3, 2025 •

edited

Loading

acidbunny21 Feb 7, 2025 •

edited

Loading