Skip to content

Use Txid, BlockHash, DescriptorId, and TxMerkleNode where applicable #764

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 10 additions & 10 deletions bdk-ffi/src/bitcoin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ use std::sync::{Arc, Mutex};
#[derive(Debug, Clone, Eq, PartialEq, uniffi:: Record)]
pub struct OutPoint {
/// The transaction.
pub txid: String,
pub txid: Arc<Txid>,
/// The index of the output in the transaction.
pub vout: u32,
}

impl From<&BdkOutPoint> for OutPoint {
fn from(outpoint: &BdkOutPoint) -> Self {
OutPoint {
txid: outpoint.txid.to_string(),
txid: Arc::new(Txid(outpoint.txid)),
vout: outpoint.vout,
}
}
Expand All @@ -57,7 +57,7 @@ impl From<&BdkOutPoint> for OutPoint {
impl From<OutPoint> for BdkOutPoint {
fn from(outpoint: OutPoint) -> Self {
BdkOutPoint {
txid: BitcoinTxid::from_str(&outpoint.txid).unwrap(),
txid: BitcoinTxid::from_raw_hash(outpoint.txid.0.to_raw_hash()),
vout: outpoint.vout,
}
}
Expand Down Expand Up @@ -169,9 +169,9 @@ pub struct Header {
/// Block version, now repurposed for soft fork signalling.
pub version: i32,
/// Reference to the previous block in the chain.
pub prev_blockhash: String,
pub prev_blockhash: Arc<BlockHash>,
/// The root hash of the merkle tree of transactions in the block.
pub merkle_root: String,
pub merkle_root: Arc<TxMerkleNode>,
/// The timestamp of the block, as claimed by the miner.
pub time: u32,
/// The target value below which the blockhash must lie.
Expand All @@ -184,8 +184,8 @@ impl From<BdkHeader> for Header {
fn from(bdk_header: BdkHeader) -> Self {
Header {
version: bdk_header.version.to_consensus(),
prev_blockhash: bdk_header.prev_blockhash.to_string(),
merkle_root: bdk_header.merkle_root.to_string(),
prev_blockhash: Arc::new(BlockHash(bdk_header.prev_blockhash)),
merkle_root: Arc::new(TxMerkleNode(bdk_header.merkle_root.to_raw_hash())),
time: bdk_header.time,
bits: bdk_header.bits.to_consensus(),
nonce: bdk_header.nonce,
Expand Down Expand Up @@ -304,8 +304,8 @@ impl Transaction {

/// Computes the Txid.
/// Hashes the transaction excluding the segwit data (i.e. the marker, flag bytes, and the witness fields themselves).
pub fn compute_txid(&self) -> String {
self.0.compute_txid().to_string()
pub fn compute_txid(&self) -> Arc<Txid> {
Arc::new(Txid(self.0.compute_txid()))
}

/// Returns the weight of this transaction, as defined by BIP-141.
Expand Down Expand Up @@ -549,7 +549,7 @@ impl From<&BdkTxIn> for TxIn {
fn from(tx_in: &BdkTxIn) -> Self {
TxIn {
previous_output: OutPoint {
txid: tx_in.previous_output.txid.to_string(),
txid: Arc::new(Txid(tx_in.previous_output.txid)),
vout: tx_in.previous_output.vout,
},
script_sig: Arc::new(Script(tx_in.script_sig.clone())),
Expand Down
8 changes: 8 additions & 0 deletions bdk-ffi/src/descriptor.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::bitcoin::DescriptorId;
use crate::error::DescriptorError;
use crate::keys::DescriptorPublicKey;
use crate::keys::DescriptorSecretKey;

use bdk_wallet::bitcoin::bip32::Fingerprint;
use bdk_wallet::bitcoin::key::Secp256k1;
use bdk_wallet::bitcoin::Network;
use bdk_wallet::chain::DescriptorExt;
use bdk_wallet::descriptor::{ExtendedDescriptor, IntoWalletDescriptor};
use bdk_wallet::keys::DescriptorPublicKey as BdkDescriptorPublicKey;
use bdk_wallet::keys::{DescriptorSecretKey as BdkDescriptorSecretKey, KeyMap};
Expand Down Expand Up @@ -296,6 +298,12 @@ impl Descriptor {
self.extended_descriptor.is_multipath()
}

/// A unique identifier for the descriptor.
pub fn descriptor_id(&self) -> Arc<DescriptorId> {
let d_id = self.extended_descriptor.descriptor_id();
Arc::new(DescriptorId(d_id.0))
Comment on lines +302 to +304
Copy link
Member

Choose a reason for hiding this comment

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

I think here we could return a straight DescriptorId instead of an Arc. Let me know if you had a reason to prefer the Arc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Everything will be reference counted in the target language anyway, so it doesn't seem like a big deal to go with the Arc. The only thing that shouldn't use Arc is uniffi::Record imo

Copy link
Member

Choose a reason for hiding this comment

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

I hate that this is not clear (or maybe it's just not clear for me?) and loosey goosey from the point of view of library builders.

There are places where removing an Arc will actually throw at compile time, others where the Arc gets added for you even if you don't have it there...

}

/// Return descriptors for all valid paths.
pub fn to_single_descriptors(&self) -> Result<Vec<Arc<Descriptor>>, MiniscriptError> {
self.extended_descriptor
Expand Down
12 changes: 7 additions & 5 deletions bdk-ffi/src/electrum.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::bitcoin::{Header, Transaction};
use crate::bitcoin::{BlockHash, Header, Transaction, Txid};
use crate::error::ElectrumError;
use crate::types::Update;
use crate::types::{FullScanRequest, SyncRequest};
Expand Down Expand Up @@ -131,12 +131,12 @@ impl ElectrumClient {
}

/// Broadcasts a transaction to the network.
pub fn transaction_broadcast(&self, tx: &Transaction) -> Result<String, ElectrumError> {
pub fn transaction_broadcast(&self, tx: &Transaction) -> Result<Arc<Txid>, ElectrumError> {
let bdk_transaction: BdkTransaction = tx.into();
self.0
.transaction_broadcast(&bdk_transaction)
.map_err(ElectrumError::from)
.map(|txid| txid.to_string())
.map(|txid| Arc::new(Txid(txid)))
}

/// Returns the capabilities of the server.
Expand Down Expand Up @@ -177,7 +177,7 @@ pub struct ServerFeaturesRes {
/// Server version reported.
pub server_version: String,
/// Hash of the genesis block.
pub genesis_hash: String,
pub genesis_hash: Arc<BlockHash>,
/// Minimum supported version of the protocol.
Copy link
Collaborator

@ItoroD ItoroD May 19, 2025

Choose a reason for hiding this comment

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

There is a kotlin test that fails when I run locally here Its seems the hash type is returning the reverse byte order (little-endian)

Expected :000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943

Actual :43497fd7f826957108f4a30fd9cec3aeba79972084e90ead01ea330900000000

This will happen for any other tests like these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will investigate this, as I am not sure if this is an Electrum protocol quark or the result of deserialize from bitcoin

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright 👍

On my test from here I did notice deserializing does return the reverse (little endian) which I think is normal for bitcoin. Same for transaction.computeTxid() if you serialize and deserialize with the hashlike type, you get reverse (little endian) of what transaction.computeTxid() actually gives

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would expect that to be the case, since it is the consensus encoding. That is fine when going to and from bytes. What I am skeptical of is the Display implementation of bitcoin using little endian. Any time I have seen a block hash printed to the console it is in the expected enidanness. It sounds like the [u8; 32] is not returned in the endian-ness expected by deserialze, so the BlockHash in ServerFeatureRes is just completely incorrect. I am almost certain Display will return the expected result at all other callsites and this was just incorrect for parsing ServerFeatureRes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah! I see. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for that catch!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of what the code was before the fix I now see, but when I parse the bytes into a BlockHash I get the correct hash and the tests work well. For example:

fn from(value: BdkServerFeaturesRes) -> ServerFeaturesRes {
    let blockhash: BlockHash = BlockHash::from_bytes(value.genesis_hash.into_vec()).unwrap();
    ServerFeaturesRes {
        server_version: value.server_version,
        genesis_hash: Arc::new(blockhash),
        protocol_min: value.protocol_min,
        protocol_max: value.protocol_max,
        hash_function: value.hash_function,
        pruning: value.pruning,
    }
}

Copy link
Collaborator Author

@rustaceanrob rustaceanrob May 22, 2025

Choose a reason for hiding this comment

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

The old state of the code was that. You should run LiveElectrumClientTest.kt to verify.

The [u8; 32] reported by Electrum is not in Bitcoin consensus format, so deserializewithin BlockHash::from_bytes should produce a backwards blockhash. Parsing [u8; 32] as a string does it in the canonical format, not consensus format, so the string correct. That string is used to parse into a blockhash

This is the rare (and only?) case we will be parsing bytes that have not been consensus serialized

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry I ran my tests locally but didn't recompile, so they were passing but it's because I wasn't on my fixed code haha. Yes I see them fail now. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Really odd. I'm trying to find out where this behavior is defined. The docs for Electrum are not clear: https://electrumx.readthedocs.io/en/latest/protocol-methods.html#server-features

pub protocol_min: String,
/// Maximum supported version of the protocol.
Expand All @@ -190,9 +190,11 @@ pub struct ServerFeaturesRes {

impl From<BdkServerFeaturesRes> for ServerFeaturesRes {
fn from(value: BdkServerFeaturesRes) -> ServerFeaturesRes {
let hash_str = value.genesis_hash.to_hex_string(Case::Lower);
let blockhash = hash_str.parse::<bdk_core::bitcoin::BlockHash>().unwrap();
ServerFeaturesRes {
server_version: value.server_version,
genesis_hash: value.genesis_hash.to_hex_string(Case::Lower),
genesis_hash: Arc::new(BlockHash(blockhash)),
protocol_min: value.protocol_min,
protocol_max: value.protocol_max,
hash_function: value.hash_function,
Expand Down
23 changes: 10 additions & 13 deletions bdk-ffi/src/esplora.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::bitcoin::BlockHash;
use crate::bitcoin::Transaction;
use crate::bitcoin::Txid;
use crate::error::EsploraError;
use crate::types::Tx;
use crate::types::TxStatus;
Expand All @@ -8,7 +10,6 @@ use crate::types::{FullScanRequest, SyncRequest};
use bdk_esplora::esplora_client::{BlockingClient, Builder};
use bdk_esplora::EsploraExt;
use bdk_wallet::bitcoin::Transaction as BdkTransaction;
use bdk_wallet::bitcoin::Txid;
use bdk_wallet::chain::spk_client::FullScanRequest as BdkFullScanRequest;
use bdk_wallet::chain::spk_client::FullScanResponse as BdkFullScanResponse;
use bdk_wallet::chain::spk_client::SyncRequest as BdkSyncRequest;
Expand All @@ -17,7 +18,6 @@ use bdk_wallet::KeychainKind;
use bdk_wallet::Update as BdkUpdate;

use std::collections::{BTreeMap, HashMap};
use std::str::FromStr;
use std::sync::Arc;

/// Wrapper around an esplora_client::BlockingClient which includes an internal in-memory transaction
Expand Down Expand Up @@ -110,9 +110,8 @@ impl EsploraClient {
}

/// Get a [`Transaction`] option given its [`Txid`].
pub fn get_tx(&self, txid: String) -> Result<Option<Arc<Transaction>>, EsploraError> {
let txid = Txid::from_str(&txid)?;
let tx_opt = self.0.get_tx(&txid)?;
pub fn get_tx(&self, txid: Arc<Txid>) -> Result<Option<Arc<Transaction>>, EsploraError> {
let tx_opt = self.0.get_tx(&txid.0)?;
Ok(tx_opt.map(|inner| Arc::new(Transaction::from(inner))))
}

Expand All @@ -128,27 +127,25 @@ impl EsploraClient {
}

/// Get the [`BlockHash`] of a specific block height.
pub fn get_block_hash(&self, block_height: u32) -> Result<String, EsploraError> {
pub fn get_block_hash(&self, block_height: u32) -> Result<Arc<BlockHash>, EsploraError> {
self.0
.get_block_hash(block_height)
.map(|hash| hash.to_string())
.map(|hash| Arc::new(BlockHash(hash)))
.map_err(EsploraError::from)
}

/// Get the status of a [`Transaction`] given its [`Txid`].
pub fn get_tx_status(&self, txid: String) -> Result<TxStatus, EsploraError> {
let txid = Txid::from_str(&txid)?;
pub fn get_tx_status(&self, txid: Arc<Txid>) -> Result<TxStatus, EsploraError> {
self.0
.get_tx_status(&txid)
.get_tx_status(&txid.0)
.map(TxStatus::from)
.map_err(EsploraError::from)
}

/// Get transaction info given its [`Txid`].
pub fn get_tx_info(&self, txid: String) -> Result<Option<Tx>, EsploraError> {
let txid = Txid::from_str(&txid)?;
pub fn get_tx_info(&self, txid: Arc<Txid>) -> Result<Option<Tx>, EsploraError> {
self.0
.get_tx_info(&txid)
.get_tx_info(&txid.0)
.map(|tx| tx.map(Tx::from))
.map_err(EsploraError::from)
}
Expand Down
16 changes: 7 additions & 9 deletions bdk-ffi/src/tx_builder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::bitcoin::{Amount, FeeRate, OutPoint, Psbt, Script};
use crate::bitcoin::{Amount, FeeRate, OutPoint, Psbt, Script, Txid};
use crate::error::CreateTxError;
use crate::types::{LockTime, ScriptAmount};
use crate::wallet::Wallet;
Expand All @@ -8,13 +8,12 @@ use bdk_wallet::bitcoin::amount::Amount as BdkAmount;
use bdk_wallet::bitcoin::script::PushBytesBuf;
use bdk_wallet::bitcoin::Psbt as BdkPsbt;
use bdk_wallet::bitcoin::ScriptBuf as BdkScriptBuf;
use bdk_wallet::bitcoin::{OutPoint as BdkOutPoint, Sequence, Txid};
use bdk_wallet::bitcoin::{OutPoint as BdkOutPoint, Sequence};
use bdk_wallet::KeychainKind;

use std::collections::BTreeMap;
use std::collections::HashMap;
use std::convert::{TryFrom, TryInto};
use std::str::FromStr;
use std::sync::Arc;

type ChangeSpendPolicy = bdk_wallet::ChangeSpendPolicy;
Expand Down Expand Up @@ -413,7 +412,7 @@ impl TxBuilder {
/// until finally calling `finish` to consume the builder and generate the transaction.
#[derive(Clone, uniffi::Object)]
pub struct BumpFeeTxBuilder {
txid: String,
txid: Arc<Txid>,
fee_rate: Arc<FeeRate>,
sequence: Option<u32>,
current_height: Option<u32>,
Expand All @@ -425,7 +424,7 @@ pub struct BumpFeeTxBuilder {
#[uniffi::export]
impl BumpFeeTxBuilder {
#[uniffi::constructor]
pub fn new(txid: String, fee_rate: Arc<FeeRate>) -> Self {
pub fn new(txid: Arc<Txid>, fee_rate: Arc<FeeRate>) -> Self {
BumpFeeTxBuilder {
txid,
fee_rate,
Expand Down Expand Up @@ -506,11 +505,10 @@ impl BumpFeeTxBuilder {
/// WARNING: To avoid change address reuse you must persist the changes resulting from one or more calls to this
/// method before closing the wallet. See `Wallet::reveal_next_address`.
pub fn finish(&self, wallet: &Arc<Wallet>) -> Result<Arc<Psbt>, CreateTxError> {
let txid = Txid::from_str(self.txid.as_str()).map_err(|_| CreateTxError::UnknownUtxo {
outpoint: self.txid.clone(),
})?;
let mut wallet = wallet.get_wallet();
let mut tx_builder = wallet.build_fee_bump(txid).map_err(CreateTxError::from)?;
let mut tx_builder = wallet
.build_fee_bump(self.txid.0)
.map_err(CreateTxError::from)?;
tx_builder.fee_rate(self.fee_rate.0);
if let Some(sequence) = self.sequence {
tx_builder.set_exact_sequence(Sequence(sequence));
Expand Down
16 changes: 8 additions & 8 deletions bdk-ffi/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::bitcoin::{Address, Amount, BlockHash, OutPoint, Script, Transaction, TxOut};
use crate::bitcoin::{Address, Amount, BlockHash, OutPoint, Script, Transaction, TxOut, Txid};
use crate::error::{CreateTxError, RequestBuilderError};

use bdk_core::bitcoin::absolute::LockTime as BdkLockTime;
Expand Down Expand Up @@ -52,7 +52,7 @@ pub enum ChainPosition {
/// A child transaction that has been confirmed. Due to incomplete information,
/// it is only known that this transaction is confirmed at a chain height less than
/// or equal to this child TXID.
transitively: Option<String>,
transitively: Option<Arc<Txid>>,
},
/// The transaction was last seen in the mempool at this timestamp.
Unconfirmed { timestamp: Option<u64> },
Expand All @@ -74,7 +74,7 @@ impl From<BdkChainPosition<BdkConfirmationBlockTime>> for ChainPosition {
block_id,
confirmation_time: anchor.confirmation_time,
},
transitively: transitively.map(|t| t.to_string()),
transitively: transitively.map(|t| Arc::new(Txid(t))),
}
}
BdkChainPosition::Unconfirmed { last_seen } => ChainPosition::Unconfirmed {
Expand Down Expand Up @@ -213,7 +213,7 @@ impl From<BdkLocalOutput> for LocalOutput {
fn from(local_utxo: BdkLocalOutput) -> Self {
LocalOutput {
outpoint: OutPoint {
txid: local_utxo.outpoint.txid.to_string(),
txid: Arc::new(Txid(local_utxo.outpoint.txid)),
vout: local_utxo.outpoint.vout,
},
txout: TxOut {
Expand Down Expand Up @@ -668,7 +668,7 @@ pub struct TxStatus {
/// Height of the block this transaction was included.
pub block_height: Option<u32>,
/// Hash of the block.
pub block_hash: Option<String>,
pub block_hash: Option<Arc<BlockHash>>,
/// The time shown in the block, not necessarily the same time as when the block was found.
pub block_time: Option<u64>,
}
Expand All @@ -678,7 +678,7 @@ impl From<BdkTxStatus> for TxStatus {
TxStatus {
confirmed: status.confirmed,
block_height: status.block_height,
block_hash: status.block_hash.map(|h| h.to_string()),
block_hash: status.block_hash.map(|h| Arc::new(BlockHash(h))),
block_time: status.block_time,
}
}
Expand All @@ -688,7 +688,7 @@ impl From<BdkTxStatus> for TxStatus {
#[derive(Debug, uniffi::Record)]
pub struct Tx {
/// The transaction identifier.
pub txid: String,
pub txid: Arc<Txid>,
/// The transaction version, of which 0, 1, 2 are standard.
pub version: i32,
/// The block height or time restriction on the transaction.
Expand All @@ -706,7 +706,7 @@ pub struct Tx {
impl From<BdkTx> for Tx {
fn from(tx: BdkTx) -> Self {
Self {
txid: tx.txid.to_string(),
txid: Arc::new(Txid(tx.txid)),
version: tx.version,
locktime: tx.locktime,
size: tx.size as u64,
Expand Down
11 changes: 4 additions & 7 deletions bdk-ffi/src/wallet.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::bitcoin::{Amount, FeeRate, OutPoint, Psbt, Script, Transaction};
use crate::bitcoin::{Amount, FeeRate, OutPoint, Psbt, Script, Transaction, Txid};
use crate::descriptor::Descriptor;
use crate::error::{
CalculateFeeError, CannotConnectError, CreateWithPersistError, DescriptorError,
Expand All @@ -11,13 +11,12 @@ use crate::types::{
Update,
};

use bdk_wallet::bitcoin::{Network, Txid};
use bdk_wallet::bitcoin::Network;
use bdk_wallet::rusqlite::Connection as BdkConnection;
use bdk_wallet::signer::SignOptions as BdkSignOptions;
use bdk_wallet::{KeychainKind, PersistedWallet, Wallet as BdkWallet};

use std::borrow::BorrowMut;
use std::str::FromStr;
use std::sync::{Arc, Mutex, MutexGuard};

/// A Bitcoin wallet.
Expand Down Expand Up @@ -335,10 +334,8 @@ impl Wallet {
/// confirmed or unconfirmed. If the transaction is confirmed, the anchor which proves the
/// confirmation is provided. If the transaction is unconfirmed, the unix timestamp of when
/// the transaction was last seen in the mempool is provided.
pub fn get_tx(&self, txid: String) -> Result<Option<CanonicalTx>, TxidParseError> {
let txid =
Txid::from_str(txid.as_str()).map_err(|_| TxidParseError::InvalidTxid { txid })?;
Ok(self.get_wallet().get_tx(txid).map(|tx| tx.into()))
pub fn get_tx(&self, txid: Arc<Txid>) -> Result<Option<CanonicalTx>, TxidParseError> {
Ok(self.get_wallet().get_tx(txid.0).map(|tx| tx.into()))
}

/// Calculates the fee of a given transaction. Returns [`Amount::ZERO`] if `tx` is a coinbase transaction.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class LiveElectrumClientTest {

assertEquals(
expected = "000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943",
actual = features.genesisHash
actual = features.genesisHash.toString()

This comment was marked as resolved.

)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ final class LiveElectrumClientTests: XCTestCase {
print("Server Features:\n\(features)")

XCTAssertEqual(
features.genesisHash,
features.genesisHash.description,
"000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943"
)
}
Expand Down
Loading