Use `Txid`, `BlockHash`, `DescriptorId`, and `TxMerkleNode` where applicable by rustaceanrob · Pull Request #764 · bitcoindevkit/bdk-ffi

rustaceanrob · 2025-05-14T13:24:00Z

Explicitly leaving out BlockId to avoid merge conflicts. The refactor here is moslty uneventful, however it removes a handful of unwrap when parsing transaction IDs.

Changelog notice

Use new hash types across public APIs

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rustaceanrob · 2025-05-19T08:45:02Z

Rebased and closes #762

ItoroD · 2025-05-19T09:23:51Z

    pub server_version: String,
    /// Hash of the genesis block.
-    pub genesis_hash: String,
+    pub genesis_hash: Arc<BlockHash>,


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.

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

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

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

Ah! I see. 👍

Thanks for that catch!

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, } }

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

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!

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

Sign in to view

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


ItoroD · 2025-05-19T14:19:48Z

ACK d608592

…re applicable Explicitly leaving out `BlockId` to avoid merge conflicts. The refactor here is moslty uneventful, however it removes a handful of `unwrap` when parsing transaction IDs.

rustaceanrob · 2025-05-22T08:05:11Z

Rebased

thunderbiscuit

Looking good. I have one comment.

thunderbiscuit · 2025-05-22T15:20:35Z

+    pub fn descriptor_id(&self) -> Arc<DescriptorId> {
+        let d_id = self.extended_descriptor.descriptor_id();
+        Arc::new(DescriptorId(d_id.0))


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.

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

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...

thunderbiscuit · 2025-05-22T15:24:04Z

    pub server_version: String,
    /// Hash of the genesis block.
-    pub genesis_hash: String,
+    pub genesis_hash: Arc<BlockHash>,


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!

thunderbiscuit · 2025-05-22T15:28:43Z

    pub server_version: String,
    /// Hash of the genesis block.
-    pub genesis_hash: String,
+    pub genesis_hash: Arc<BlockHash>,


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

thunderbiscuit · 2025-05-22T15:42:59Z

It's not always clear what return values should be Arc'ed. I know we sometimes do return complex types as Arcs, and sometimes not... Maybe we should define guidelines for when to use which (maybe we should use Arcs everywhere on complex types?)

thunderbiscuit

ACK.

rustaceanrob force-pushed the use-hash-like-5-14 branch from 2ecb43c to 3953179 Compare May 14, 2025 13:35

rustaceanrob changed the title ~~Use Txid and BlockHash where applicable~~ Use Txid, BlockHash, DescriptorId, and TxMerkleNode where applicable May 14, 2025

rustaceanrob force-pushed the use-hash-like-5-14 branch 2 times, most recently from 0d80e13 to 9a52129 Compare May 19, 2025 08:44

ItoroD reviewed May 19, 2025

View reviewed changes

rustaceanrob force-pushed the use-hash-like-5-14 branch from 9a52129 to d608592 Compare May 19, 2025 13:31

rustaceanrob mentioned this pull request May 19, 2025

Express the wallet changeset over the FFI layer #756

Merged

8 tasks

refactor: use Txid, BlockHash, DescriptorId, TxMerkleNode whe…

23d14f0

…re applicable Explicitly leaving out `BlockId` to avoid merge conflicts. The refactor here is moslty uneventful, however it removes a handful of `unwrap` when parsing transaction IDs.

rustaceanrob force-pushed the use-hash-like-5-14 branch from d608592 to 23d14f0 Compare May 22, 2025 08:04

thunderbiscuit reviewed May 22, 2025

View reviewed changes

thunderbiscuit approved these changes May 22, 2025

View reviewed changes

thunderbiscuit merged commit 23d14f0 into bitcoindevkit:master May 22, 2025
26 checks passed

rustaceanrob deleted the use-hash-like-5-14 branch May 23, 2025 07:35

Conversation

rustaceanrob commented May 14, 2025

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

rustaceanrob commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ItoroD May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustaceanrob May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

ItoroD commented May 19, 2025

Uh oh!

rustaceanrob commented May 22, 2025

Uh oh!

thunderbiscuit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thunderbiscuit commented May 22, 2025

Uh oh!

thunderbiscuit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rustaceanrob commented May 19, 2025 •

edited

Loading

ItoroD May 19, 2025 •

edited

Loading

rustaceanrob May 22, 2025 •

edited

Loading