Refactor(electrum): Remove unwraps and expects by vatsalkeshav · Pull Request #956 · getfloresta/Floresta

vatsalkeshav · 2026-04-14T06:55:04Z

Description and Notes

This pr removes all .unwrap() and non-infallible .expect() calls in non-test code of floresta-electrum

contributes to issue #463

note to reviewers : added self.addresses_to_scan.extend(addresses) to avoid silent address drop in at get_block_height failure in rescan_with_block_filters fn

Contributor Checklist

I've followed the contribution guidelines
I've verified one of the following:
- Ran just pcc (recommended but slower)
- Ran just lint-features '-- -D warnings' && cargo test --release
- Confirmed CI passed on my fork
I've linked any related issue(s) in the sections above

Finally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see How (and why) to sign Git commits and GitHub's guide to signing commits).

remove all .unwrap() and non-infallible .expect() calls in non-test code of /floresta-electrum/s/electrum_protocol.rs contribute to issue getfloresta#463

jaoleal · 2026-04-14T12:57:24Z

Did you tested this ? I dont think that the approach of just logging the error is efficient but i may be missing something about the electrum protocol...

Ill take a read about it and after that ill do a review...

Also, another strange thing are the unwrap_or calls, this default behavior is not desired, we need to know when this call fails.

Davidson-Souza

First pass through the code

Davidson-Souza · 2026-04-14T18:29:15Z

-
-                    let position = self.address_cache.get_position(&prevout.txid).unwrap();
+                for (utxo, prevout) in utxos.into_iter() {
+                    let height = self.address_cache.get_height(&prevout.txid).unwrap_or(0);


getting a None means we don't have it cached, so we should send and empty reply

done.
I initially left unwrap_or(0) as electrum treats 0 as sentinel for unconfirmed tx - sorry for that

Davidson-Souza · 2026-04-14T18:29:43Z

                    .chain
                    .get_block_hash(0)
-                    .expect("Genesis block should be present");
+                    .expect("genesis block is always in the chain store");


Suggested change

.expect("genesis block is always in the chain store");

.expect("Genesis block is always in the chain store");

Davidson-Souza · 2026-04-14T18:30:55Z

+        let unconfirmed = match self.address_cache.find_unconfirmed() {
+            Ok(txs) => txs,
+            Err(e) => {
+                error!("Could not fetch unconfirmed transactions for rebroadcast: {e}");
+                return;
+            }
+        };


This should be propagated back. Our convension is to not handle errors in utility functions like this

Davidson-Souza · 2026-04-14T18:31:33Z

        let Ok(blocks) =
            cfilters.match_any(_addresses, start_height, stop_height, self.chain.clone())
        else {
+            info!("Could not match block filters");


these are errors, not info

Davidson-Souza · 2026-04-14T18:32:54Z

+        match self.chain.get_height() {
+            Ok(chain_height) => {
+                if chain_height == height {
+                    for client in &mut self.clients.values() {
+                        let res = client.write(
+                            serde_json::to_string(&result)
+                                .expect("serde_json::Value is always serializable")
+                                .as_bytes(),
+                        );
+                        if res.is_err() {
+                            info!("Could not write to client {client:?}");
+                        }
+                    }
                }


please reduce nesting. I think you can use combinators here

Davidson-Souza · 2026-04-14T18:36:01Z

+                        client.write(
+                            serde_json::to_string(&res)
+                                .expect("serde_json::Value is always serializable")
+                                .as_bytes(),
+                        )?;


Suggested change

client.write(

serde_json::to_string(&res)

.expect("serde_json::Value is always serializable")

.as_bytes(),

)?;

let res = serde_json::to_string(&res)

.expect("serde_json::Value is always serializable");

client.write(

res.as_bytes(),

)?;

Davidson-Souza · 2026-04-14T18:37:50Z

Is this enough to forbid clippy::unwrap_used in floresta-electrum?

…ap_or(), flatten nesting

vatsalkeshav · 2026-04-15T11:18:40Z

Is this enough to forbid clippy::unwrap_used in floresta-electrum?

yes it is

Thanks for the review!

vatsalkeshav · 2026-04-15T11:19:52Z

Did you tested this ? I dont think that the approach of just logging the error is efficient but i may be missing something about the electrum protocol...

Ill take a read about it and after that ill do a review...

Also, another strange thing are the unwrap_or calls, this default behavior is not desired, we need to know when this call fails.

fixed unwrap_or calls after review

jaoleal · 2026-04-15T12:05:15Z

+                                    error!("main loop receiver dropped: {e:?}");
+                                    break;
+                                }
+                            }


prefer the a if let Err(e) = self.message_transmitter.send(Message::Message((self.client_id, line))) instead of a whole match.

Theres an example on line 80 in this same file

jaoleal · 2026-04-15T12:05:33Z

+                            match self.message_transmitter.send(Message::Disconnect(self.client_id)) {
+                                Ok(_) => {}
+                                Err(e) => error!("main loop receiver dropped: {e:?}"),
+                            }


https://github.com/getfloresta/Floresta/pull/956/changes#r3086286698

jaoleal · 2026-04-15T12:05:40Z

+                            match self.message_transmitter.send(Message::Disconnect(self.client_id)) {
+                                Ok(_) => {}
+                                Err(e) => error!("main loop receiver dropped: {e:?}"),
+                            }


https://github.com/getfloresta/Floresta/pull/956/changes#r3086286698

jaoleal · 2026-04-15T12:06:40Z

-
-                Err(super::error::Error::InvalidParams)
+                let Some(proof) = self.address_cache.get_merkle_proof(&tx_id) else {
+                    return Err(super::error::Error::InvalidParams);


Suggested change

return Err(super::error::Error::InvalidParams);

return Err(Error::InvalidParams);

jaoleal · 2026-04-15T12:07:08Z

+                let Some(height) = self.address_cache.get_height(&tx_id) else {
+                    return Err(super::error::Error::InvalidParams);
+                };


Suggested change

let Some(height) = self.address_cache.get_height(&tx_id) else {

return Err(super::error::Error::InvalidParams);

};

let Some(height) = self.address_cache.get_height(&tx_id) else {

return Err(Error::InvalidParams);

};

jaoleal · 2026-04-15T12:07:30Z

+        let unconfirmed = self
+            .address_cache
+            .find_unconfirmed()
+            .map_err(|e| super::error::Error::WatchOnly(Box::new(e)))?;


Suggested change

.map_err(|e| super::error::Error::WatchOnly(Box::new(e)))?;

.map_err(|e| Error::WatchOnly(Box::new(e)))?;

jaoleal · 2026-04-15T12:07:53Z


-    pub async fn rebroadcast_mempool_transactions(&self) {
-        let unconfirmed = self.address_cache.find_unconfirmed().unwrap();
+    pub async fn rebroadcast_mempool_transactions(&self) -> Result<(), super::error::Error> {


Suggested change

pub async fn rebroadcast_mempool_transactions(&self) -> Result<(), super::error::Error> {

pub async fn rebroadcast_mempool_transactions(&self) -> Result<(), Error> {

jaoleal · 2026-04-15T12:08:58Z

        let Ok(blocks) =
            cfilters.match_any(_addresses, start_height, stop_height, self.chain.clone())
        else {
+            error!("Could not match block filters");


Suggested change

error!("Could not match block filters");

error!("Could not find matching block filters");

jaoleal · 2026-04-15T12:11:39Z

+                        match message_transmitter
                            .send(Message::NewClient((client.client_id, client)))
-                            .expect("Main loop is broken");
-                        id_count += 1;
+                        {
+                            Ok(_) => {
+                                id_count += 1;
+                            }
+                            Err(e) => {
+                                error!("main loop receiver dropped: {e:?}");
+                                break;
+                            }
+                        }


https://github.com/getfloresta/Floresta/pull/956/changes#r3086286698

jaoleal · 2026-04-15T12:11:44Z

+                match message_transmitter.send(Message::NewClient((client.client_id, client))) {
+                    Ok(_) => {
+                        id_count += 1;
+                    }
+                    Err(e) => {
+                        error!("main loop receiver dropped: {e:?}");
+                        break;
+                    }
+                }


https://github.com/getfloresta/Floresta/pull/956/changes#r3086286698

… instead of pattern matching, shorter erro path

jaoleal

After reading a little about the Electrum protocol, it appears to leave to the server implementation to decide the error handling, similar to how the jsonrpc server should handle errors, following the spec of course.

Maybe we should follow another implementation as a standard ? Should we define our own standards for that as were doing on #831 ?

cc @Davidson-Souza @csgui

jaoleal · 2026-04-15T14:12:27Z

-                    Vec::from_hex(&tx).map_err(|_| super::error::Error::InvalidParams)?;
-                let tx: Transaction =
-                    deserialize(&hex).map_err(|_| super::error::Error::InvalidParams)?;
+                let hex: Vec<_> = Vec::from_hex(&tx).map_err(|_| Error::InvalidParams)?;


Suggested change

let hex: Vec<_> = Vec::from_hex(&tx).map_err(|_| Error::InvalidParams)?;

let hex = Vec::from_hex(&tx).map_err(|_| Error::InvalidParams)?;

jaoleal · 2026-04-15T14:12:58Z

+                    let Some(height) = self.address_cache.get_height(&prevout.txid) else {
+                        return json_rpc_res!(request, []);
+                    };
+                    let Some(position) = self.address_cache.get_position(&prevout.txid) else {
+                        return json_rpc_res!(request, []);
+                    };


You should be returning errors here too no ?

I know this isnt focused on error handling but it should replace unwraps with errors atleast right ?

makes sense
but None means not cached, so @davidsonsouza suggested sending []
I'll try forcing the error with electrum daemon to be sure though

Oh, if it was a recommendation from Davidson you can just forget this, thank you for telling me this.

csgui · 2026-04-16T19:52:25Z

+                    let Some(height) = self.address_cache.get_height(&prevout.txid) else {
+                        return json_rpc_res!(request, []);
+                    };


I'd like to double check what happens inside this for loop and, specially, with the return statement.

What's the expected behavior when a UTXO is found in the address cache but its height is missing? For example, utxos list has 5 UTXOs and one of them (the last one of 5) has a cache inconsistency (no height) what should the client receive? An empty list? A partial list of the 4 valid ones? An error? What is your code doing?

i looked into AddressCache - the tx gets saved before the utxo is added, and not removed later. so the none case shouldn't happen. [] seems to be right for the case when address is not yet cached as suggested by @davidsonsouza . An error could be added if something changes in future from that side

@csgui Would you prefer returning the ones we can get? Something like:

let final_utxos = utxos .into_iter() .flat_map(|(prev_output, outpoint)| { let height = self.address_cache.get_height(&outpoint.txid)?; let position = self.address_cache.get_position(&outpoint.txid)?; Some(json!({ "height": height, "tx_pos": position, "tx_hash": outpoint.1.txid, "value": prev_output.value })) }) .collect::<Vec<_>>();

yep, keep it simple returning only the consistent ones.

thanks for the suggest
Done!

csgui · 2026-04-16T19:53:44Z

+                        return json_rpc_res!(request, []);
+                    };
+                    let Some(position) = self.address_cache.get_position(&prevout.txid) else {
+                        return json_rpc_res!(request, []);


Same question above but for position https://github.com/getfloresta/Floresta/pull/956/changes#r3095960563

csgui · 2026-04-16T20:18:12Z

After reading a little about the Electrum protocol, it appears to leave to the server implementation to decide the error handling, similar to how the jsonrpc server should handle errors, following the spec of course.

Maybe we should follow another implementation as a standard ?

Yep, a protocol is designed around independent server implementations. So error handling is minimally specified, perhaps only as an envelope. Since Electrum is heavily based on JSON-RPC, I’d expect the error handling to be similar to a RPC server implementation.

vatsalkeshav · 2026-04-20T14:50:25Z

I guess an issue be opened for error handling as a separate pr
or do that with this one?

After reading a little about the Electrum protocol, it appears to leave to the server implementation to decide the error handling, similar to how the jsonrpc server should handle errors, following the spec of course.
Maybe we should follow another implementation as a standard ?

Yep, a protocol is designed around independent server implementations. So error handling is minimally specified, perhaps only as an envelope. Since Electrum is heavily based on JSON-RPC, I’d expect the error handling to be similar to a RPC server implementation.

jaoleal · 2026-04-22T15:53:39Z

I guess an issue be opened for error handling as a separate pr
or do that with this one?

Surely a follow up

csgui · 2026-05-08T11:14:15Z

@vatsalkeshav anything else holding this PR? Please refresh my mind :-)

vatsalkeshav · 2026-05-09T11:10:01Z

@vatsalkeshav anything else holding this PR? Please refresh my mind :-)

tested affected paths with electrum - good to go!

csgui · 2026-05-14T10:27:57Z

@vatsalkeshav, the error-handling work is now scoped as part of a SoB/2026 effort. Would you mind handing this PR over to the folks working on that? Thanks for your work here, we really appreciate it.

vatsalkeshav · 2026-05-14T15:38:20Z

@vatsalkeshav, the error-handling work is now scoped as part of a SoB/2026 effort. Would you mind handing this PR over to the folks working on that? Thanks for your work here, we really appreciate it.

Sure!

moisesPompilio · 2026-05-14T18:45:54Z

Need rebase

refactor(electrum): remove unwrap and expect

4d73fe1

remove all .unwrap() and non-infallible .expect() calls in non-test code of /floresta-electrum/s/electrum_protocol.rs contribute to issue getfloresta#463

jaoleal self-requested a review April 14, 2026 12:10

Davidson-Souza requested changes Apr 14, 2026

View reviewed changes

Davidson-Souza assigned vatsalkeshav Apr 14, 2026

Davidson-Souza added the reliability Related to runtime reliability, stability and production readiness label Apr 14, 2026

Davidson-Souza added this to Floresta Apr 14, 2026

github-project-automation Bot moved this to Backlog in Floresta Apr 14, 2026

Davidson-Souza moved this from Backlog to Needs review in Floresta Apr 14, 2026

refactor(electrum): fixes from review - propogate errors, remove unwr…

390cb74

…ap_or(), flatten nesting

refactor(electrum): run cargo fmt

8c8e54d

jaoleal reviewed Apr 15, 2026

View reviewed changes

refactor(electrum): run cargo fmt; fixes from review - use if-let-err…

ab159b6

… instead of pattern matching, shorter erro path

jaoleal reviewed Apr 15, 2026

View reviewed changes

refactor(electrum): remove unnecessary type hint

6db1b2a

csgui requested changes Apr 16, 2026

View reviewed changes

csgui added this to the Q2/2026 milestone May 8, 2026

refactor(electrum): listunspent returns partial list on cache miss

b1c1ba7

csgui removed this from the Q2/2026 milestone May 14, 2026

csgui added this to the SoB/2026 milestone May 14, 2026

csgui moved this from Needs review to Backlog in Floresta May 14, 2026

	.expect("genesis block is always in the chain store");
	.expect("Genesis block is always in the chain store");

	return Err(super::error::Error::InvalidParams);
	return Err(Error::InvalidParams);

	.map_err(\|e\| super::error::Error::WatchOnly(Box::new(e)))?;
	.map_err(\|e\| Error::WatchOnly(Box::new(e)))?;

	pub async fn rebroadcast_mempool_transactions(&self) -> Result<(), super::error::Error> {
	pub async fn rebroadcast_mempool_transactions(&self) -> Result<(), Error> {

	error!("Could not match block filters");
	error!("Could not find matching block filters");

	let hex: Vec<_> = Vec::from_hex(&tx).map_err(\|_\| Error::InvalidParams)?;
	let hex = Vec::from_hex(&tx).map_err(\|_\| Error::InvalidParams)?;

Conversation

vatsalkeshav commented Apr 14, 2026

Description and Notes

Contributor Checklist

Uh oh!

jaoleal commented Apr 14, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vatsalkeshav Apr 15, 2026 • 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!

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 Apr 14, 2026

Uh oh!

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

Uh oh!

Uh oh!

vatsalkeshav commented Apr 15, 2026

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!

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!

jaoleal 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!

vatsalkeshav Apr 15, 2026 •

edited

Loading

vatsalkeshav commented Apr 15, 2026 •

edited

Loading

Davidson-Souza Apr 30, 2026 •

edited

Loading