[RPCSAGA] New internal descriptor module.

[jaoleal]

What is the purpose of this pull request?

• Bug fix
• Documentation update
• New feature
• Test
• Other:

Which crates are being modified?

• floresta-chain
• floresta-common
• floresta-compact-filters
• floresta-electrum
• floresta-node
• floresta-rpc
• floresta-watch-only
• floresta-wire
• bin/florestad
• bin/floresta-cli
• Other:

Description and Notes

Part of #670

The star of the show guys, this is the main descriptor module that should satisfy all the next steps to #670 as well everything related to descriptors in floresta.

The point of this PR.

Solely introducing this new descriptor module, as well
defining a strong API and extensive test module.

Im afraid these tests arent that great so ill still work a little bit more on them.

I worked a lot on the docs so it should be easy to understand the whys and the objective of each piece.

How to verify the changes you have done?

Read the module docs

Read and execute the tests

Share your thougts about the API and if you found a test case that should be covered.

[jaoleal]

Dropped #666

[jaoleal]

Sorry, i messed with cargo lock

[Davidson-Souza]

Preliminary comments, haven't looked at the tests yet

[Davidson-Souza]

Suggested change

	pub fn arr_sum_index(to_sum: &mut Vec<DescriptorUnit>, idx: u32) {
	pub fn batch_sum_index(to_sum: &mut [DescriptorUnit], idx: u32) {

[jaoleal]

using batch instead of arr

[Davidson-Souza]

Why?

[jaoleal]

They are "validated" inside DescrpitorRequest::into_unit which should be the only way to make a DescriptorUnit.

Updating the string with new naming

[jaoleal]

ecd83af updated with lattest suggestions and new tests, pcc looks fine

[moisesPompilio]

In the DescriptorRequest struct, optional fields use skip_serializing_if so that None values are omitted from the JSON output. These fields do not make sense when empty, and Bitcoin Core follows the same behavior.

[moisesPompilio]

Suggested change

	#[serde(default)]
	pub range: DerivationRange,
	#[serde(default, skip_serializing_if = "Option::is_none")]
	pub range: Optional<DerivationRange>,

The ranger must be optional because not every descriptor supports it. During descriptor validation we need to know whether a ranger is required. For example, pkh([d6043800/0'/0'/18']03efdee34c0009fd175f3b20b5e5a5517fd5d16746f2e635b44617adafeaebc388)#4ahsl9pk does not support a ranger since it represents a single address. But tr(xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHi/86h/0h/0h/0/*) does support a ranger because the * indicates derivation.

[moisesPompilio]

The texts ended up very long. Adding some line breaks would improve readability

[moisesPompilio]

Suggested change

	impl DescriptorRequest {
	/// Interpret [`DescriptorRequest`]s returning the extracted and already derived
	/// descriptors.
	///
	/// The returned [`RescanRequest`] is the more embracing in order to satisfy the users request.
	/// You can read further on [`RescanRequest::check_override`].
	pub fn handle_requests(
	requests: Vec<DescriptorRequest>,
	) -> Result<(Vec<DescriptorUnit>, RescanRequest), DescriptorError> {
	let mut rescan_request: RescanRequest = RescanRequest::SpecifiedTime(u32::MAX);
	let mut descriptors: Vec<DescriptorUnit> = Vec::new();
	for request in requests {
	rescan_request = rescan_request.check_override(&request.timestamp);
	descriptors.append(&mut request.into_unit()?);
	}
	Ok((descriptors, rescan_request))
	}
	pub struct ImportDescriptorsRequest (pub Vec<DescriptorRequest>);
	impl ImportDescriptorsRequest {
	pub fn process(
	self,
	) -> Vec<DescriptorResponse> {
	for request in self.0 {
	match request.handle_requests() {
	Ok((descriptors, rescan)) => {
	DescriptorResponse::Success(DescriptorResult { descriptors, rescan })
	}
	Err(e) => {
	DescriptorResponse::Failure(e)
	}
	}
	}
	}
	}
	pub enum DescriptorResponse{
	Success(DescriptorResult),
	Failure(DescriptorError),
	}
	impl DescriptorRequest {
	/// Interpret [`DescriptorRequest`]s returning the extracted and already derived
	/// descriptors.
	///
	/// The returned [`RescanRequest`] is the more embracing in order to satisfy the users request.
	/// You can read further on [`RescanRequest::check_override`].
	pub fn handle_requests(
	Self
	) -> Result<(DescriptorUnit, RescanRequest), DescriptorError> {
	let mut rescan_request: RescanRequest = RescanRequest::SpecifiedTime(u32::MAX);
	let mut descriptors: Vec<DescriptorUnit> = Vec::new();
	rescan_request = rescan_request.check_override(&Self.timestamp);
	descriptors.append(&mut Self.into_unit()?);
	Ok((descriptors, rescan_request))
	}

I think this function is not ideal because processing all descriptors at once means a single error stops the whole import and only returns that error. This is not compliant with Bitcoin Core and makes it hard to know which descriptor failed.

It would be better to have a struct like pub struct ImportDescriptorsRequest(pub Vec<DescriptorRequest>); with a process method that handles each descriptor individually. This way, one failing descriptor doesn’t affect the others, and it’s easy to identify which failed by its position in the input vector. The method could return a Vec of enums indicating success or failure, matching the input vector’s order. Then handle_request of DescriptorRequest would only deal with a single descriptor at a time.