Add Wallet Integration Tests and Fix Descriptor Persistence Issues by moisesPompilio · Pull Request #821 · getfloresta/Floresta

moisesPompilio · 2026-02-02T18:59:50Z

Description and Notes

Adds comprehensive integration tests for wallet functionality and fixes several issues related to descriptor persistence and handling.

Key fixes:

Duplicate descriptor prevention: Added centralized validation to prevent the same descriptor from being stored multiple times in the database. The push_descriptor method now checks if a descriptor already exists before adding it.
Improved xpub parsing and descriptor generation:
- Xpub parsing now rejects xprivs and multisig-related xpubs
- Fixed descriptor generation to dynamically create wpkh, pkh, or sh-wpkh descriptors based on the provided xpub (previously only wpkh was generated)
- Added network validation to ensure xpubs match the node's network

key improvements:

Centralized address derivation: Address derivation logic has been moved to floresta-watch-only. When a descriptor is added, addresses are automatically derived and cached, simplifying wallet initialization and management.
Consolidated descriptor parsing: Moved parse_descriptors, parse_xpubs, and slip132 logic to the floresta-watch-only module, removing scattered code from floresta-node and floresta-common. Removed the miniscript dependency from floresta-common.
Documentation clarity: Updated wallet_descriptor option documentation to accurately describe descriptor behavior (previously incorrectly referred to xpubs).
Typos configuration: Added _typos.toml to prevent the typos checker from analyzing strings between 32-150 characters (hashes, keys, addresses, descriptors).

New integration tests:

wallet_conf.py: Tests wallet loading via config.toml, verifying correct descriptor/xpub loading and rejection of xprivs and private key descriptors
wallet_flag.py: Tests wallet loading via command-line flags with the same validations
RPC method tests for loaddescriptor (including duplicate detection and private key rejection) and listdescriptors
Constants centralization: Created constants.py to centralize all constants used across integration tests, improving maintainability.

How to verify the changes you have done?

Run the integration tests using the standard command:
```
./tests/run.sh
```
Verify that all new wallet tests (wallet_conf.py, wallet_flag.py, loaddescriptor, listdescriptors) pass successfully.
Test that duplicate descriptors are properly rejected when using loaddescriptor RPC method.
Confirm that xprivs and descriptors with private keys are rejected during wallet initialization.
Verify that xpubs are correctly validated against the node's network and generate appropriate descriptor types (wpkh, pkh, sh-wpkh).

jaoleal · 2026-02-02T22:50:55Z

The first commit seems unrelated

moisesPompilio · 2026-02-02T22:59:28Z

The first commit seems unrelated

The first commit introduces constants.py which centralizes the constants used in the code that are later used in the wallet tests, to store the xpubs and descriptors.

jaoleal

Concept ACK.

Heres some initial review

crates/floresta-node/src/error.rs

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/lib.rs

jaoleal · 2026-02-02T23:26:47Z

tests/floresta-cli/listdescriptors.py

+        self.run_node(self.florestad)
+
+        self.log("Checking initial descriptors...")
+        descriptors = self.florestad.rpc.list_descriptors()


One thing to add, both listdescriptors.py and loaddescriptor.oy is fine but they are dependent on each other, if one fails they will both fail...

that will be a problem if breaking one raises a false alarm about the other.

In integration tests it's common to have some interdependencies, such as between wallet_conf and wallet_flag. They also depend on listdescriptors, because that's the single method we use to check the descriptors Floresta has stored.

Yeah, fine to depend on listdescriptors, but listdescriptors test dont need to depend on loaddescriptor you can instantiate and insert a descriptor from the Config.toml. It can help for this case.

But in this case, if there is a problem with listdescriptors, the wallet_conf and wallet_flag tests will also fail, because they also depend on that RPC.

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/descriptor/mod.rs

jaoleal · 2026-02-02T23:30:49Z

crates/floresta-watch-only/src/descriptor/mod.rs

+/// Parses each descriptor, validates it, and derives the specified number of addresses
+/// starting from the given index.
+pub fn derive_addresses_from_list_descriptors(
+    descriptors: &[String],


Same thing from: https://github.com/getfloresta/Floresta/pull/821/changes/146e2ffb3060c633a40904a84de29bbe39c56363..27e55ad8c35c6d9d3aadb9dd3d20560b6d397a42#r2756460303

The descriptor is already a String, so using a &[&str] will make us waste resources converting it to &str. It's better to use it as a String directly; this method is used inside the wallet internals which already obtain the descriptor as a String.

Theres no cost casting to a &str. but i understand thats how our codebase is written right now, fine.

IIUC theres no cost at all to cast Strings to &strs, but theres some when casting &str to String, basically the same relation between [B] and Vec<B>... Being picky with this may lead us to less runtime allocs.

The ideal is to declare functions that only read and compare strings with a generic S where S: AsRef<str>

The problem here is that we can have multiple database implementations; so if we pass only pointers (&) to objects retrieved from the database it will be wrong, because the objects live shorter than the pointers. It's better to pass the object in this case, for example we pass a Vec<String> for descriptors, because if we passed Vec<&str> the &str would need to live longer than the Vec<String> that only exists inside the descriptor-retrieval function.

That's why when we receive a Vec<String> and it needs to be used by another function, that function takes a &[String] to avoid allocating memory by creating a new array of strings.

Davidson-Souza · 2026-02-03T19:09:45Z

Needs rebase

moisesPompilio · 2026-02-04T19:34:07Z

c2bff7d Performed a rebase and added some review requests.

Davidson-Souza · 2026-02-04T22:36:55Z

In ae433c1 there's still a REGTEST_RPC_SERVER constant in rpc/{floresta, utreexod}.py. I think it should also be moved?

moisesPompilio · 2026-02-05T15:12:24Z

In ae433c1 there's still a REGTEST_RPC_SERVER constant in rpc/{floresta, utreexod}.py. I think it should also be moved?

I didn't add them because they will be removed in this PR #733 .

moisesPompilio · 2026-02-06T21:52:51Z

5ce80a0 I did the rebase, adjusted some parts of the code that were using String where they could use &str, and removed the descriptors-no-std feature from CI (introduced by PR #825).

moisesPompilio · 2026-02-09T19:40:35Z

7f9b79b I did the rebase.

crates/floresta-watch-only/src/lib.rs

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/lib.rs

jaoleal · 2026-02-09T21:56:15Z

crates/floresta-node/src/wallet_input.rs

-fn parse_xpubs(xpubs: &[String]) -> Result<Vec<Descriptor<DescriptorPublicKey>>, FlorestadError> {
+fn parse_xpubs(
+    xpubs: &[String],
+    network: Network,


I think you dont need to take ownership to read enums

I couldn't understand the comment here

You can read the network enum by reference

Copy types should never be passed by reference

crates/floresta-node/src/wallet_input.rs

tests/test_framework/rpc/floresta.py

crates/floresta-common/src/lib.rs

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/descriptor/mod.rs

moisesPompilio · 2026-02-15T19:12:21Z

21eca0d I've applied the review suggestions from @jaoleal

moisesPompilio · 2026-03-03T15:38:21Z

3c66053 Implemented the changes requested by @JoseSK999

Davidson-Souza

This PR centralizes so many things, we should call it ~~REDACTED~~ 🤣

crates/floresta-watch-only/src/descriptor/slip132.rs

crates/floresta-node/src/slip132.rs

crates/floresta-node/src/wallet_input.rs

tests/florestad/wallet_conf.py

crates/floresta-watch-only/src/descriptor/mod.rs

moisesPompilio · 2026-03-06T00:41:05Z

285993f Implemented the changes requested by @Davidson-Souza

jaoleal · 2026-03-09T22:45:07Z

crates/floresta-node/src/florestad.rs

@@ -365,7 +368,7 @@ impl Florestad {
            .map_err(FlorestadError::CouldNotInitializeWallet)?;


Cant these go inside Florestad::setup_wallet() ?

I think it's better to keep it as it is

I think its confusing the way it is.

It calls two times a method that supposed to make the wallet setup, since theyre separate is clear that theyre different. Also, the setup_wallet() doesnt need more to take the &mut wallet since it could be instantiated inside there.

I suggest for the code between lines 365 and 368 to be moved inside the setup_wallet method, turning it into a single entry for the node to load and instantiate the wallet instead of two

Done! I took the opportunity and combined the load_wallet method with wallet_setup.

jaoleal · 2026-03-09T22:50:46Z

crates/floresta-node/src/florestad.rs

-            let is_cached = wallet.is_cached(&descriptor)?;
+        for descriptor in self.get_descriptors() {
+            match wallet.push_descriptor(&descriptor) {
+                Ok(_) => info!("Added descriptor to wallet: {descriptor}"),


Suggested change

Ok(_) => info!("Added descriptor to wallet: {descriptor}"),

Ok(()) => info!("Added descriptor to wallet: {descriptor}"),

In this case you can't use it like that, because a Vec of addresses is returned.

jaoleal · 2026-03-09T22:51:04Z

crates/floresta-node/src/florestad.rs

-                wallet.push_descriptor(&descriptor)?;
+        for xpub in self.get_xpubs() {
+            match wallet.push_xpub(&xpub, self.config.network) {
+                Ok(_) => info!("Added xpubs to wallet: {xpub}"),


Suggested change

Ok(_) => info!("Added xpubs to wallet: {xpub}"),

Ok(()) => info!("Added xpubs to wallet: {xpub}"),

jaoleal · 2026-03-09T23:42:52Z

crates/floresta-watch-only/src/descriptor/slip132.rs

+use bitcoin::bip32::Xpub;
+
+/// Magical version bytes for xpub: bitcoin mainnet public key for P2PKH or P2SH
+const VERSION_MAGIC_XPUB: [u8; 4] = [0x04, 0x88, 0xB2, 0x1E];


Space between these constants ?

jaoleal · 2026-03-09T23:43:36Z

crates/floresta-watch-only/src/descriptor/slip132.rs

+    /// Extended private keys are unsupported
+    XprivUnsupported,
+
+    /// Extended public keys for multisig are unsupported


Forgot to delete this line ?

No, this line is fine. The line below is just an explanation showing how a multisig should be imported.

Can you add a dot by the end ? I think itll help

jaoleal · 2026-03-09T23:55:51Z

crates/floresta-watch-only/src/descriptor/slip132.rs

+        let mut data = base58::decode_check(s)?;
+
+        let prefix: [u8; 4] = extract_slip132_prefix(s)?;
+        let slice = match prefix {


Suggested change

let slice = match prefix {

let prefix = match prefix {

"I changed it to bip44_prefix, because it's from this BIP that we use the version.

jaoleal · 2026-03-16T20:00:32Z

I added just two suggestions more on ongoing comments, besides them this LGTM

moisesPompilio · 2026-03-17T19:53:11Z

ba83c7d Applied @jaoleal review requests

jaoleal · 2026-03-17T20:01:06Z

crates/floresta-node/src/florestad.rs

    }

-    fn load_wallet(data_dir: &String) -> Result<AddressCache<KvDatabase>, FlorestadError> {
+    fn setup_wallet(&self, data_dir: &String) -> Result<AddressCache<KvDatabase>, FlorestadError> {


Nit: datadir can be accessed from self, no need for it to be a input

Created a `constants.py` file to centralize all constants used in the test framework. This change improves maintainability by providing a single location for managing constants and makes it easier to understand their purpose and usage across the tests.

Updated the documentation for the `wallet_descriptor` option to explicitly describe its behavior with descriptors. The previous text incorrectly referred to xpubs, which could cause confusion. Add a short help-note to the `wallet_descriptor` and `wallet_xpub` CLI flags: if `descriptors`/`xpubs` are added after IBD finishes or when IBD was performed with `assume-valid`, related transactions may not be discovered. In that case use the JSON-RPC `rescanblockchain` to find those transactions.

Previously, it was possible to save the same descriptor multiple times, leading to redundancy and potential inconsistencies. Adds a verification step in the `push_descriptor` method to ensure that a descriptor is only added if it does not already exist.

- Updated xpub parsing to reject xpriv and xpubs related to multisig, ensuring only standard public keys are accepted for descriptor generation. - Fixed an issue in descriptor generation by xpub where only `wpkh` descriptors were being created. Descriptors are now generated dynamically as `wpkh`, `pkh`, or `sh-wpkh`, depending on the xpub provided. - Added a network validation step to ensure the xpub matches the network the node is running on. - Added \_typos\.toml configuration to prevent https://github.com/crate-ci/typos from analyzing words with lengths between 32 and 150 characters, as these correspond to hashes, public keys, private keys, XPUBs, descriptors, and Bitcoin addresses.

- Added `wallet_conf.py` to test wallet loading via `config.toml`. This test verifies: - Descriptors and XPUBs are correctly loaded from the configuration file. - XPRIVs and descriptors with private keys are rejected. - Added `wallet_flag.py` to test wallet loading via command-line flags. This test performs the same checks as `wallet_conf.py`, but uses flags to pass wallet information during node initialization.

- Added tests to verify that the `loaddescriptor` RPC method can successfully load a descriptor. - Ensured that duplicate descriptors cannot be loaded. - Verified that descriptors containing private keys are rejected.

…thod - Added tests to verify that the `listdescriptors` RPC method correctly displays the descriptors loaded in the node. - Ensured that the method returns all loaded descriptors in the expected format.

…-only` module Improved code organization by consolidating descriptor handling in a single module - Moved `parse_descriptors`, `parse_xpubs`, and `slip132` to the `floresta-watch-only` module. - Centralized descriptor and XPUB parsing logic, previously scattered across `floresta-node` and `floresta-common`. - Removed descriptor-related features and the `miniscript` dependency from `floresta-common`.

… and refactor wallet handling - Centralized address derivation in `floresta-watch-only`: - Address derivation is now automatically performed when pushing a descriptor. - Added `push_xpub` method to convert `XPUBs` into `descriptors`, push them, and derive addresses for the wallet. - Removed `walletInput` file from floresta-node: - This file was no longer necessary as its functionality (`XPUB`, `descriptor`, and address derivation) is now handled automatically by `floresta-watch-only`.

Prefer an explicit `config_file` when present, otherwise build `{data_dir}/config.toml` inside `get_config_file`.

Combine setup_wallet and load_wallet operations into one unified method that handles both initialization and loading, eliminating redundant calls

jaoleal · 2026-03-20T15:57:21Z

ACK c9b6795

moisesPompilio self-assigned this Feb 2, 2026

jaoleal reviewed Feb 2, 2026

View reviewed changes

moisesPompilio force-pushed the add-test-wallet branch from 27e55ad to c2bff7d Compare February 4, 2026 19:32

moisesPompilio mentioned this pull request Feb 4, 2026

ci: add no-std job to check floresta-common #825

Merged

moisesPompilio force-pushed the add-test-wallet branch from c2bff7d to 5ce80a0 Compare February 6, 2026 21:47

jaoleal self-requested a review February 9, 2026 10:07

moisesPompilio force-pushed the add-test-wallet branch 2 times, most recently from 7b9921f to 7f9b79b Compare February 9, 2026 19:31

jaoleal reviewed Feb 10, 2026

View reviewed changes

moisesPompilio force-pushed the add-test-wallet branch 3 times, most recently from e18de15 to 1c97cb5 Compare February 15, 2026 19:06

moisesPompilio force-pushed the add-test-wallet branch from 1c97cb5 to 21eca0d Compare February 15, 2026 20:04

Davidson-Souza mentioned this pull request Feb 17, 2026

0.9.0 priorities #850

Closed

20 tasks

Davidson-Souza requested changes Mar 4, 2026

View reviewed changes

moisesPompilio force-pushed the add-test-wallet branch from 3c66053 to 285993f Compare March 6, 2026 00:33

moisesPompilio requested a review from Davidson-Souza March 6, 2026 02:11

moisesPompilio force-pushed the add-test-wallet branch from 285993f to 16ecc70 Compare March 6, 2026 14:41

jaoleal reviewed Mar 9, 2026

View reviewed changes

jaoleal reviewed Mar 10, 2026

View reviewed changes

Davidson-Souza removed this from the v0.9.0 milestone Mar 10, 2026

moisesPompilio force-pushed the add-test-wallet branch from 16ecc70 to 138c79c Compare March 15, 2026 18:27

moisesPompilio requested a review from jaoleal March 16, 2026 13:23

moisesPompilio force-pushed the add-test-wallet branch from 138c79c to ba83c7d Compare March 17, 2026 19:50

jaoleal reviewed Mar 17, 2026

View reviewed changes

moisesPompilio force-pushed the add-test-wallet branch from ba83c7d to 48bc23f Compare March 17, 2026 20:33

moisesPompilio added 11 commits March 17, 2026 17:33

test(integration): add integration tests for listdescriptors RPC me…

c4a751e

…thod - Added tests to verify that the `listdescriptors` RPC method correctly displays the descriptors loaded in the node. - Ensured that the method returns all loaded descriptors in the expected format.

chore(florestad): centralize config_file resolution in get_config_file

91992bd

Prefer an explicit `config_file` when present, otherwise build `{data_dir}/config.toml` inside `get_config_file`.

chore(florestad): merge wallet setup and load into single method

c9b6795

Combine setup_wallet and load_wallet operations into one unified method that handles both initialization and loading, eliminating redundant calls

moisesPompilio force-pushed the add-test-wallet branch from 48bc23f to c9b6795 Compare March 17, 2026 20:35

moisesPompilio requested a review from jaoleal March 17, 2026 20:36

		@@ -365,7 +368,7 @@ impl Florestad {
		.map_err(FlorestadError::CouldNotInitializeWallet)?;

	Ok(_) => info!("Added descriptor to wallet: {descriptor}"),
	Ok(()) => info!("Added descriptor to wallet: {descriptor}"),

	Ok(_) => info!("Added xpubs to wallet: {xpub}"),
	Ok(()) => info!("Added xpubs to wallet: {xpub}"),

Conversation

moisesPompilio commented Feb 2, 2026

Description and Notes

How to verify the changes you have done?

Uh oh!

jaoleal commented Feb 2, 2026

Uh oh!

moisesPompilio commented Feb 2, 2026

Uh oh!

jaoleal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Davidson-Souza commented Feb 3, 2026

Uh oh!

moisesPompilio commented Feb 4, 2026

Uh oh!

Davidson-Souza commented Feb 4, 2026

Uh oh!

moisesPompilio commented Feb 5, 2026

Uh oh!

moisesPompilio commented Feb 6, 2026

Uh oh!

moisesPompilio commented Feb 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moisesPompilio commented Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moisesPompilio commented Mar 3, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moisesPompilio commented Feb 15, 2026 •

edited

Loading

jaoleal Mar 9, 2026 •

edited

Loading