Skip to content

Add a GetTxStatus trait in blockchain.#697

Closed
tnull wants to merge 5 commits intobitcoindevkit:masterfrom
tnull:2022-08-trait-gettxstatus
Closed

Add a GetTxStatus trait in blockchain.#697
tnull wants to merge 5 commits intobitcoindevkit:masterfrom
tnull:2022-08-trait-gettxstatus

Conversation

@tnull
Copy link
Copy Markdown
Contributor

@tnull tnull commented Aug 2, 2022

Description

This adds a GetTxStatus trait that, when implemented, allows to query for the confirmation status of a transaction. I now also added a preliminary implementation for Esplora and will also update ElectrumClient as soon as it allows to query the necessary data.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@tnull tnull marked this pull request as draft August 2, 2022 16:02
@tnull tnull changed the title Add a GetTxStatus trait in bdk::blockchain. Add a GetTxStatus trait in blockchain. Aug 2, 2022
@notmandatory
Copy link
Copy Markdown
Member

@vladimirfomene this is one to check out and see if you can incorporate into your rust-esplora-client crate.

@tnull
Copy link
Copy Markdown
Contributor Author

tnull commented Aug 2, 2022

@vladimirfomene this is one to check out and see if you can incorporate into your rust-esplora-client crate.

I'm also happy to remove the client-specific behavior here and open another PR once the separate crate repo goes up.

@tnull
Copy link
Copy Markdown
Contributor Author

tnull commented Aug 2, 2022

Ah, I just realized on the EsploraBlockchain side of things there already is blockchain::esplora::api::TxStatus which I probably should use to deserialize the response. However, there seems to be a discrepancy: the last field of TxStatus currently is block_time: Option<u64>, but it seems to be missing the optional block_hash field described in the API.

Is this intentional? Otherwise I'd just add it?

@notmandatory
Copy link
Copy Markdown
Member

... However, there seems to be a discrepancy: the last field of TxStatus currently is block_time: Option<u64>, but it seems to be missing the optional block_hash field described in the API.

Is this intentional? Otherwise I'd just add it?

It probably wasn't needed by BDK so wasn't added, so for completeness sake yes please add it. Thanks!

@tnull
Copy link
Copy Markdown
Contributor Author

tnull commented Aug 4, 2022

It probably wasn't needed by BDK so wasn't added, so for completeness sake yes please add it. Thanks!

Alright, thanks!

I now added rudimentary support of GetTxStatus for ElectrumBlockchain, EsploraBlockchain, and the RpcBlockchain.

For the time being I based this on this fork of the Electrum client, which is a simple rebase of bitcoindevkit/rust-electrum-client#58.

There are some issues I found while implementing:

  1. Unsure how I should implement the trait for CompactFiltersBlockchain, since this one has no access to the chain. Might be arguable whether it should have the Blockchain trait at all, or if it would suffice for it to have WalletSync?
  2. In the implementations for Electrum and RPC I had to resort to computing the confirmation block height as cur_height + 1 - confirmations. This is unfortunately race-y since we need two requests for that and by the time the second one is answered the block height might just have changed. Not entirely sure how to solve this since the solutions I currently come up with are really not performant (e.g., checking the block height a second time after the fact and looping until it matches the first).
  3. In the RpcBlockchain case get_tx_status might actually return inaccurate results if no txindex is set in Bitcoin Core. As this wasn't considered by the GetTx trait so far, I now also didn't handle that/return an error in that case. Would be great to get some feedback how BDK's design philosophy is here: should we check in both cases whether Capability::GetAnyTx is set and return an error otherwise to alert the user?

@tnull tnull force-pushed the 2022-08-trait-gettxstatus branch from 9d2477d to 3dddbfa Compare August 4, 2022 15:36
@tnull
Copy link
Copy Markdown
Contributor Author

tnull commented Aug 4, 2022

Btw. I'm aware that there are diverging opinions on whether such a general trait should be part of the BDK's Blockchain interface itself or whether it is enough to expose it through the underlying clients (see for example this comment by @LLFourn #490 (comment)).

Even though I think a more general interface would be nice, I'm happy to go either way, especially since currently neither is supported. If we only want to implement functionalities such as transaction status on individual blockchain clients, then bdk::blockchain would at least need to allow us to access the underlying client in some way.

@tnull
Copy link
Copy Markdown
Contributor Author

tnull commented Aug 8, 2022

After speaking to @afilini, I'm closing in favor of opening individual PRs for missing client features in the rust-electrum-client and rust-esplora-client repositories.

@tnull tnull closed this Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants