Skip to content
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

[Rust/Bitcoin/Utxo] Great Migration towards tw_bitcoin and tw_utxo #3382

Merged
merged 371 commits into from
Sep 19, 2023

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Aug 17, 2023

Alright, major PR incoming 🥳 This undertaking was quite extensive - almost more so than BRC20 was - but we have reached a promising milestone. I have iteratively refined the protobuf structures until I discovered a design pattern that aligns seamlessly with the Bitcoin/UTXO workflow.

This PR includes:

  • Modular Separation: Isolate the Bitcoin and UTXO specific logic into separate tw_bitcoin and tw_utxo modules. This clean separation allows us to repurpose tw_utxo for other chains/forks that employ the same UTXO model.

  • Enhanced Protobuf Structures: Introduce refined Protobuf structures to facilitate more efficient and less error-prone transaction creation, thereby minimizing the need for frequent back-and-forth adjustments.

  • Integration of tw_coin_entry: Incorporate Sergey's work on tw_coin_entry into tw_bitcoin. This integration is engineered to:

    • Generate the "preimages" for signing, enabling users to avoid exposing their private keys to our library,
    • Include a "compile" function to assemble all components into a final, network-ready transaction,
    • Offer a "sign" function that executes all necessary signing steps in one efficient call.
  • Backwards Compatibility Maintenance: We are committed to maintaining support for the current SigningInput and TransactionPlan, ensuring that existing protobuf structures can continue to leverage this library. This compatibility is achieved by wrapping the legacy SigningInput and TransactionPlan over the new structures. While substantial progress has been made on this front, some final touches are still pending.

The Current State

UPDATE 2023-08-28: There are two things that still need to be tested that are unfortunately not really straightforward to test, meaning I'll need to use another library (in Python probably) and reconstruct it there. The current implementation here is probably okay considering that the underlying rust-bitcoin library handles that part, but still. Most likely a separate PR should be opened for that:

  • Test P2SH/P2SWH (rarely used transaction types)
    • WIP: kind of stuck here, signature fails to verify in test environment
    • FINALLY DONE, it works
  • Test non-default Sighash types (SignSingle, AllPlusAnyoneCanPay, etc.)

UPDATE 2023-08-31: Added explicit tests for:

  • Custom P2TR script-path (complex scripts) input/output builders.
  • Automatically derive the appropriate transaction type from an address (already tested for P2PKH, 2WPKH and P2TR)
    • P2SH
    • P2WSH

State of tw_bitcoin

  • Integrate CoinEntry
    • parse_address
      • Expected to be straightforward to implement Done
    • derive_address
      • Expected to be straightforward to implement Done
    • preimage_hashes
      • Constructs all inputs/outputs and generates the necessary sighashes required for signing.
    • compile
      • Merges the constructed inputs/outputs with the signatures to assemble the final transaction.
    • sign
      • Essentially executes preimage_hashes -> signs sighashes -> compile
    • json_signer
      • Not supported, yet?
    • plan_builder
      • Not supported, but should it?
  • Add Protobuf builders for transaction types.
    • P2SH
      • Expected to be straightforward to implement Done
    • P2PKH
    • P2WSH
      • Expected to be straightforward to implement Done
    • P2WPKH
    • P2TR key-path (simple balance transfers)
    • P2TR script-path
    • BRC20 transfers
    • Ordinal NFTs
      • Expected to be straightforward to implement, similar to BRC20 transfers Done
  • Add Protobuf structures for custom scripts (scriptPubkey, scriptSig, and Witness).
  • Add Protobuf structure for automatically deriving transaction type (P2PKH, P2WPKH, etc.) from a given address.
    • Yet to be tested Done
  • Add mechanism for calculating weight/fee estimation without requiring actual signatures/claim scripts.
    • Yet to be tested Done
  • Calculate the final fee/weight
  • Existing FFI functions ("legacy") wrap over the new structures:
    • tw_build_p2pkh_script
    • tw_build_p2wpkh_script
    • tw_build_p2tr_key_path_script
    • tw_build_brc20_transfer_inscription
    • tw_bitcoin_build_nft_inscription
      • Straightforward, analogous to the BRC20 implementation Done
    • tw_taproot_build_and_sign_transaction
      • Near completion Done

Regarding tests, I have integrated the existing tests we already have for tw_bitcoin, and all of them function correctly. However, since our goal is to use this crate for all Bitcoin transactions, and not just BRC20, we need to expand our testing to be more general and thorough.

What is done:

  • Test P2PKH builders.
  • Test P2WPKH builders.
  • Test P2TR key-path builders.
  • Test BRC20 (P2TR script-path) builders.
  • Test existing FFI functions for building scripts.

Precise tests required for:

  • Thoroughly test SigningInput and TransactionPlan for legacy protobuf.
    • NOTE: In this PR, we'll just maintain backwards compatibility for our existing BRC20/Ordinal API. We will not handle the full API from Bitcoin.proto (legacy) as of yet
  • Test Sequence features (replace-by-fee, etc.)
    • Likely to be part of tw_utxo
    • NOTE: Sequence is correctly included, but we should probably have builder functions for those instead of raw u32(?)
  • Test weight/fee estimation without the presence of signatures/claim scripts.
  • Test automatic construction of return output.
  • Test automatic detection of transaction types based on provided address.

State of tw_utxo

  • Create tw_utxo with interface analogous to tw_coin_entry.
    • preimage_hashes
      • Constructs sighashes containing inputs/outputs and other necessary transaction details for signing.
      • Support Legacy (for P2SH, P2PKH)
      • Support Segwit (for P2WSH, P2WPKH)
      • Support Taproot (for P2TR)
    • compile
      • Accepts all inputs/outputs, along with claim scripts (scriptSig & Witness), and constructs and serializes the final transaction.
  • Add mechanism for automatic input selection.
  • Enable automatic creation of change/return output, if configured.

From the perspective of tw_utxo, it has no concepts of transaction types (P2PKH, P2WPKH, etc.) but only works with scriptSig, Witness, etc., in an opaque form.

Regarding tests:

  • Test P2PKH transactions (Legacy)
  • Test P2WPKH transactions (Segwit)
  • Test P2TR transactions (Taproot)
  • Test automatic input selections
    • Testing more cases still needed, however.
  • Test automatic change output creation.

src/proto/BitcoinV2.proto Outdated Show resolved Hide resolved
src/proto/BitcoinV2.proto Show resolved Hide resolved
src/proto/BitcoinV2.proto Show resolved Hide resolved
src/proto/BitcoinV2.proto Show resolved Hide resolved
src/proto/BitcoinV2.proto Show resolved Hide resolved
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Only minor comments

rust/tw_bitcoin/src/modules/plan_builder.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/plan_builder.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
rust/tw_bitcoin/src/modules/utils.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@Milerius Milerius merged commit f521ce6 into dev Sep 19, 2023
12 checks passed
@Milerius Milerius deleted the integrate-tw-utxo-restruct branch September 19, 2023 13:44
dfinity-ryancroote pushed a commit to dfinity-ryancroote/wallet-core that referenced this pull request Oct 6, 2023
@satoshiotomakan satoshiotomakan mentioned this pull request Oct 9, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants