[POC BDK] Introduce modular trait-based watch-only wallet architecture#950
[POC BDK] Introduce modular trait-based watch-only wallet architecture#950moisesPompilio wants to merge 5 commits into
Conversation
…anagement The provider layer establishes a new abstraction for managing all descriptor-related operations in the watch-only wallet. It centralizes responsibility for tracking wallet state across descriptors, including address derivation, transaction indexing, and balance calculations. The provider serves as the single source of truth for descriptor-scoped information, isolating this logic from higher-level wallet coordination. Key responsibilities: - Manage descriptor persistence and retrieval - Track generated and observed addresses per descriptor - Index transactions associated with each descriptor - Maintain UTXO sets and output tracking - Calculate balances on a per-descriptor basis - Process blockchain events (blocks, mempool) and emit descriptor-specific events - Added `WalletProvider` trait defining the provider interface - Introduced `WalletProviderEvent` enum for event-driven transaction notifications - Defined `WalletProviderError` for comprehensive error handling - Implemented feature-gated BDK provider backend via `bdk-provider` feature test(provider): add comprehensive provider unit tests Established test coverage for the provider interface, validating core descriptor and transaction management operations. Test scenarios: - Descriptor lifecycle (persist, retrieve, list, deduplicate) - Transaction indexing and querying by descriptor - Balance calculations with confirmation requirements - Address generation and management - UTXO tracking and spend status filtering - Mempool and block event processing - Edge cases (empty wallets, nonexistent descriptors, duplicate operations) - Script buffer management and local output tracking
…sistence The repository layer establishes a higher-level persistence abstraction for the watch-only wallet, centralizing all data storage operations. It manages wallet metadata, descriptor configurations, transaction indexing, and script tracking—providing a clean interface between the wallet service and the underlying database backend. Core responsibilities: - Persist wallet names and lifecycle management - Store descriptors associated with each wallet with their metadata (active status, change flag, labels) - Maintain descriptor configurations and derivation information - Index and retrieve transactions for Electrum protocol support - Track script buffers and derive addresses for transaction monitoring - Provide auxiliary transaction data structures needed for Electrum responses Implementation: - Added SQLiteRepository backed by rusqlite with migration-based schema initialization - Designed comprehensive WalletPersist trait defining the repository interface - Implemented wallet CRUD operations supporting multi-wallet environments - Created database schema with normalized tables for wallets, descriptors, transactions, and script buffers - Established foreign key constraints for data integrity Test coverage: - Wallet creation, listing, and deletion operations - Descriptor lifecycle (persist, retrieve, update, deduplication) - Multi-wallet descriptor management and isolation - Transaction indexing and querying - Script buffer operations and state tracking - Edge cases and error conditions Dependencies: - rusqlite: SQLite driver for Rust - refinery: Database migration management
…n and lifecycle management The wallet service establishes the coordination layer between the provider, repository, and metadata layers, orchestrating wallet operations and managing the complete wallet lifecycle. It serves as the primary interface for wallet clients (such as Electrum servers), translating high-level operations into coordinated calls across the underlying layers while maintaining consistent wallet state. Architecture & Responsibilities: *Provider Integration:* - Queries descriptor-specific transaction data and balance information - Retrieves address generation and UTXOs for each descriptor - Processes blockchain events and emits descriptor-scoped notifications *Repository Integration:* - Persists wallet metadata (name, creation, deletion) - Stores descriptor configurations with metadata (active flag, change flag, labels) - Maintains transaction index for Electrum protocol responses - Tracks script buffers and historical transaction data *Metadata Management:* - Maintains in-memory wallet state and descriptor registry - Administers descriptor lifecycle (add, activate, deactivate, replace) - Handles descriptor state transitions when adding new descriptors - Enforces single active descriptor per category (external/change) *Core Operations:* - Wallet creation and loading from persistent storage - Descriptor management with automatic deactivation of replaced descriptors - Block and mempool transaction processing with event propagation - Balance calculations aggregating across descriptors - Transaction history and proof retrieval for Electrum clients - Address generation delegated to active descriptors Implementation: - Implemented `Wallet` trait defining the complete service interface - Multi-layered error handling with service-specific error types - RwLock-based concurrency for thread-safe metadata access - Deterministic descriptor ID generation via SHA256 hashing
- Add architecture overview with three-layer design explanation - Include Mermaid class diagram showing trait relationships - Add sequence diagram for block processing data flow - Provide practical usage examples (wallet creation, descriptors, blocks, queries) - Document feature flags (bdk-provider, sqlite) and combinations - Detail error types, concurrency model, and development guidelines - All content in English with clear code examples
|
In this case, what is in service.rs will basically replace everything from lib.rs, however I didn't put it there so the code can still compile in case someone wants to test something |
Davidson-Souza
left a comment
There was a problem hiding this comment.
I pretty much like the direction proposed here. Just a couple questions, specially about the interfaces (I might drop a few ones as I think more about it 🙃).
@csgui @jaoleal could you guys give a look at this. I would like to see a decision on this ASAP, so @moisesPompilio can strip the BDK part for now and get this going.
@jaoleal specifically, there's some changes here that I think will make your lives way better on the RPC side, and will help us implementing most of the rawtransaction category from Core. Please double check my assumptions here.
| height: u32, | ||
| ) -> Result<Vec<WalletProviderEvent>, WalletProviderError>; | ||
|
|
||
| fn get_transaction(&self, txid: &Txid) -> Result<Transaction, WalletProviderError>; |
There was a problem hiding this comment.
On get_* methods I usually like to use Result<Option<T>> because in several cases, "we can't find it" and "we can't get it" are two different code paths. The former means everything is working, but we really don't have that info. While the latter means something isn't working, therefore we shouldn't continue doing what we are doing, and possibly even try to recover/HCF.
We may find ourselves matching against a TransactionNotFound variant in several places because we can't distinguish between the two.
There was a problem hiding this comment.
Sure, in my case, I was used to the idea that when you send a request and the element isn’t there, that should indicate NotFound. But I agree with this approach of returning an Option, and letting the higher layers decide what to do with that information if it isn’t found.
| fn create_transaction( | ||
| &self, | ||
| ids: HashSet<String>, | ||
| address: &str, | ||
| ) -> Result<(), WalletProviderError>; |
There was a problem hiding this comment.
Thinking out loud: perhaps we could break the createtransaction side into another trait? Maybe one trait for address keeping and another for creation, then Provider ties them up?
There was a problem hiding this comment.
I agree. In this case, I did it that way just to have something simple for the user to send to an address, but this transaction-creation part will need to be split up more so we can allow more customization, like coin selection, sending to multiple addresses, or even sending to hex, which would be the output script.
| fn block_process( | ||
| &self, | ||
| block: &Block, | ||
| height: u32, | ||
| ) -> Result<Vec<WalletProviderEvent>, WalletProviderError>; |
There was a problem hiding this comment.
Would there be any usefulness in giving proofs/inputs here? Do we want to keep utreexo proofs? Can inputs be useful for wallet somehow? If we support Silent Payments, they sure are.
There was a problem hiding this comment.
Yeah, we can add more events. In this case, the event could expose the important information, while the upper layer would be responsible for building and storing anything it needs, such as proofs or any other metadata that the provider itself does not persist.
| fn get_txo( | ||
| &self, | ||
| outpoint: &OutPoint, | ||
| is_spent: Option<bool>, | ||
| ) -> Result<Option<TxOut>, WalletProviderError>; |
There was a problem hiding this comment.
Hmm, should we really require our caller to know this?
There was a problem hiding this comment.
I think this can be removed. I only implemented it because there was a method like that in the wallet.
Sorry, since i had my setup ready here im dealing with my huge backlog... Ill dedicate this next saturday (may 9th) for this. |
RFC: Direction for the watch-only wallet architectureWhat I think #950 gets rightA few things worth calling out before digging in, because the questions below are about going further on the same direction, not reverting any of it:
Architectural questions surfaced by #950Six points where I think there's room to push the modularity further. Framing them as questions, not verdicts. 1. The Provider module imports BDK at its public surface
#[cfg(feature = "bdk-provider")]
use bdk_wallet::rusqlite::Connection;The Question: should the 2. Descriptor data is duplicated between Provider and Repository, with no atomicity
let existing_descriptor = self
.persister
.exists_descriptor(Some(&descriptor.id), None)?;
if !existing_descriptor {
self.get_provider_mut()?
.persist_descriptor(&descriptor.id, &descriptor.descriptor)?;
}
self.persister.insert_or_update_descriptor(&descriptor)?;There's no transaction crossing the two backends. The provider write is conditional on the descriptor being new ( Plus the data itself overlaps: Question: who owns descriptor strings? My intuition is the Provider owns them (since it has BDK's persister anyway), and the Repository owns only what's exclusively a Floresta concept — Merkle proofs, the 3.
|
In Rust’s structure, since the trait is inside the module, it already knows about BDK. In this case, I put this function that instantiates the BDK provider there so that, if we have other providers in the future, they can also be added in the same place. That way, this becomes the standard, centralized place to get the function that instantiates the provider, since it always returns something that implements the trait. In this way, the user doesn’t need to access the BDK module to get these things, and doesn’t even need to know it exists.
In this case, I only put the string in the repository to make requests to some endpoints easier, so it wouldn’t need to query the provider. But in this case, they can just stay in the provider; the only thing the rest of the system knows is the descriptor hash, which will be its ID, and that way it can link everything to it.
In this case, loadwallet is for loading a wallet, while createwallet creates a wallet in Floresta. When a wallet is loaded, it is placed in the metadata, which only contains wallet information such as which descriptors it has, which one is change, and so on. It should not interfere with block or mempool processing. As for block and mempool processing, that is done for all wallets simultaneously, because that is handled by the provider, and the provider doesn’t know which wallets exist. The only thing it knows is which descriptors it must track information for.
I think splitting the trait in the provider doesn’t make sense. However, it can exist in the service, which would be the higher-level API for floresta-watch-only. In that case, the provider trait would be available for export in case someone wants to implement their own provider, but the service would be the component that uses the provider. That way, the service becomes the public API for using floresta-watch-only. Then, if needed, we can split things into several traits later if that becomes necessary.
I don’t think this is necessary, because the descriptor ID should be generated only in the service layer. Everywhere else only needs to know that the unique descriptor ID is a string, and nothing more than that.
This comment is poorly written, because blocks are only processed if we already have a wallet that contains a descriptor. It doesn’t make sense for the wallet to process blocks if there’s nothing to process. |
|
Regarding multi_process and async in the wallet, this needs to be evaluated carefully, because in Bitcoin the wallet itself relies heavily on synchronous processing, especially for block processing. We can’t put block processing in parallel in the wallet, for example, because whenever we discover a TXO, we don’t know when an input spending it will appear. |
The wallet metadata keeps in memory which wallet the user is currently using, so it can’t be a list. Technically, it doesn’t persist data; it’s just a helper used when we need to ask something specific about a wallet. For example, if the user wants to generate an address, we need to know from which descriptor that address should be generated. |
Description and Notes
This PR introduces a proof of concept for integrating BDK into Floresta by restructuring the watch-only wallet architecture into a more modular and specialized design.
The new architecture splits responsibilities across three main layers:
Wallet): provides the high-level API and coordinates wallet operationsIt also adds shared models used for communication between layers, such as richer balance structures that expose more detailed information like
trusted,used, anduntrustedvalues.All layers communicate through traits, which makes the architecture more flexible and extensible. This allows multiple implementations for the same layer, for example:
For testing, the idea is to build an integration-test architecture around the traits themselves. I already started this approach in the provider layer, and in the service layer I left some usage examples to illustrate the intended behavior. The goal is to validate that the expected behavior of the
repository,provider, andservicetraits remains consistent regardless of the concrete implementation being used.With this approach, implementation-specific code should not need to duplicate unit tests for the core behavior already defined by the traits. Instead, concrete implementations would only need tests for auxiliary or internal helper functions that are specific to that implementation, similar to what is already done in the provider layer. This helps ensure that, no matter which Floresta implementation is plugged in, the same rules and expectations apply across the board.
In addition, this PR includes a short documentation update in
crates/floresta-watch-only/README.mdto explain how the new design works.Notes for reviewers:
How to verify the changes you have done?