refactor(wire): make node interface a trait by Davidson-Souza · Pull Request #1035 · getfloresta/Floresta

Davidson-Souza · 2026-05-06T22:24:15Z

Description and Notes

While working on my filters PR, I've reached a roadblock on how to test it. I would need to create a utreexo node with mocked peers to reply with the data I've needed. This would be very convoluted and very out of scope. I've noticed that traiting out the node interface would make it way easier, since I can simply mock the node interface to return my test data. No UtreexoNode would be used here.

This also aligns with out roadmap, and can easily be ported to floresta-domain.

Changelog

 - Created four new traits: MempoolOperations, Networking, BlockData and NodeConfig that contains all methods for `NodeHandle`. Consumers can now cherry-pick which capabilities they wish to use.
 - Created a `NodeInterface` trait that has all API-level methods for the interface, tying up all four traits
 - Renamed the existing `NodeInterface` struct to `NodeHandle`
 - Moved `NodeHandle` to its own file
 - Changed all the usage of `NodeHandle` to include the interface and update the paths

Note to reviewers

I know it's pretty disgusting to look at a function signature like:

fn broadcast_transaction(
    &self,
    transaction: Transaction,
 ) -> impl Future<Output = Result<Result<Txid, MempoolError>, RecvError>>;

but we can't use async syntax sugar in trait declarations. This is basically the de-sugar of async done manually.

lorenzolfm · 2026-05-07T00:01:33Z

You already did the heavy lifting, but if you only need to mock one method I find it sometimes useful to impl a trait only for the method.

Example:

struct Carlos {}

impl Carlos {
    fn say_hi() {
        println!("hi");
    }
}

trait SayBye {
    fn say_bye(&self);
}

impl SayBye for Carlos {
    fn say_bye(&self) {
        println!("bye");
    }
}

fn example(c: &impl SayBye) {
    c.say_bye();
}

#[cfg(test)]
mod tests {
    struct MockCarlos {}

    impl crate::SayBye for MockCarlos {
        fn say_bye(&self) {
            println!("mock bye");
        }
    }
}

Davidson-Souza · 2026-05-07T00:40:33Z

You already did the heavy lifting, but if you only need to mock one method I find it sometimes useful to impl a trait only for the method.

Example:

...

That's a fair approach, but we need a few functions for each test, but they need different functions themselves. Overall they fit almost the whole API. I thought about breaking this trait up into several traits and tying them up with NodeInterface.

Something like Network, Blocks and Filters. Then

pub trait NodeInterface: Network + Blocks + Filters {}

luisschwab · 2026-05-07T02:16:58Z

Something like Network, Blocks and Filters. Then

This makes sense. Me and @oleonardolima are planning something similar for rust-esplora-client

jaoleal · 2026-05-07T13:01:45Z

but we can't use async syntax sugar in trait declarations.

Looks like were going to need https://github.com/Davidson-Souza/maybe-async2

jaoleal · 2026-05-07T13:10:38Z

+    Onetry((IpAddr, u16)),
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]


derive statements should be before or after docs ?

jaoleal · 2026-05-07T13:17:16Z

+}
+
+#[derive(Debug)]
+pub struct RequestData {


docs for this one ?

jaoleal · 2026-05-07T13:44:43Z

+//! under `node_handle.rs`. We do this to make our testing easier, since we can mock a node while
+//! testing other modules, and to allow people to reuse other crates without wire: simply
+//! re-implement the relevant parts of node interface and you are fine!


To provide better ergonomics for mocking we could offer default implementation for our main traits. partially addressing #776, which avoids garbage code trough the codebase such as

Floresta/crates/floresta-chain/src/pruned_utreexo/partial_chain.rs

Line 293 in 736f832

fn accept_header(&self, _header: BlockHeader) -> Result<(), BlockchainError> {

(the unimplemented call which could be avoid by defining it as a default)

But how wold a default implementation look like here? Returning hardcoded data? I think this is pretty error-prone. I woud prefer adding a test-only implementation that assists mocking (I will open a new PR with this soon).

just unimplemented! as a default would reduce a lot the code, I see returning hardcoded data and no-ops like

Floresta/crates/floresta-chain/src/pruned_utreexo/partial_chain.rs

Line 284 in 736f832

Ok(())

as being more rare than the need to write unimplemented! on every trait definition.

Also, the trait splits that we are working on will mitigate this already but these cases where defaults would reduce code will still exist i think

Davidson-Souza · 2026-05-07T14:48:58Z

This makes sense. Me and @oleonardolima are planning something similar for rust-esplora-client

So, how about:

MempoolOps: broadcast_transaction, get_mempool_transaction, get_mempool_transaction
Meta: get_config
Networking: add_peer, remove_peer, disconnect_peer, onetry_peer, get_peer_info, get_connection_count, ping
Block Data: get_block
CFilters: get_cfheaders, get_cfilters

(Open for naming suggestion).

On my filters one I would use Block Data and CFilters only. Which is nice, since I only need those for testing. Furthermore, if anyone wants to use floresta-compact-filters without floresta-wire, they only need to reimplement those two.

@moisesPompilio is doing the same for RPC, following Core docs's categories.

Looks like were going to need https://github.com/Davidson-Souza/maybe-async2

Not really, it's a different problem. There is the async_trait crate for that, but not sure we need a dependency, since the problem is just a mild inconvenience when writing trait methods.

Node interface is currently a concrete type. While it make adding new methods and passing it as parameter easier, it has the major drawback of making testing for modules using it incredibly hard, due to requiring a full `UtreexoNode` to request messages. By traiting it out, we can build minimal mocks that only reply to what we need, with pre-filled data. This commit introduces several traits, named: MempoolOperations, Networking, BlockData and NodeConfig. Each trait gives the handle a subset of functions. They are all tied-up by the meta-trait `NodeInterface`. If you don't need all methods, you may only use the cathegories that makes sense to you. For example: `floresta-electrum` only needs `MempoolOperations` and `BlockData`. Finally, `NodeHandle` was moved to its own module, and left `NodeInterface` as just the interface and types.

Davidson-Souza · 2026-05-12T17:39:28Z

Alright, created four new traits: MempoolOperations, Networking, BlockData and NodeConfig (not attached to the names, btw). NodeInterface now just ties them up if you don't want to cherry-pick capabilities.

If I get a green light for this approach, I can refactor existing core. I believe floresta-electrum can use just MempoolOperations and BlockData, that would simplify its tests. I think this should be a follow up tho.

luisschwab · 2026-05-12T17:43:15Z

I'll review this in a few hours.

lorenzolfm

ACK 38485e8

luisschwab · 2026-05-13T18:27:19Z

+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+/// A request to addnode that can be made to the node.
+///
+/// This enum represents all the possible requests that can be made to the node to add, remove
+/// or just try to connect to a peer, following the same pattern as the `addnode` command in [Bitcoin Core].
+///
+/// [Bitcoin Core]: (https://bitcoincore.org/en/doc/29.0.0/rpc/network/addnode/)
+pub enum AddNode {
+    /// The `Add` variant is used to add a peer to the node's peer list
+    Add((IpAddr, u16)),
+
+    /// The `Remove` variant is used to remove a peer from the node's peer list
+    Remove((IpAddr, u16)),
+
+    /// The `Onetry` variant is used to try a connection to the peer once, but not add it to the peer list.
+    Onetry((IpAddr, u16)),
+}


Use SocketAddr instead of IpAddr + u16.

Or maybe leave it to a follow up, as this change would be out of scope for this PR. But there's no reason to use IpAddr and u16 instead of a SocketAddr.

luisschwab · 2026-05-13T18:28:36Z

+    /// Add a peer to the node's peer list.
+    ///
+    /// This function will add this peer to a special list of peers such that, if we lose the
+    /// connection, we will keep trying to connect to it until we succeed.
+    Add((IpAddr, u16, bool)),
+
+    /// Removes a node from the node's peer list.
+    ///
+    /// This function will remove a node that was added with [`AddNode::Add`]. This will **not**
+    /// disconnect the peer, but if it disconnects, it will not be reconnected again.
+    Remove((IpAddr, u16)),
+
+    /// Attempts to connect to a peer once.
+    ///
+    /// Different from [`AddNode::Add`], this function will try to connect to the peer once, but
+    /// will not add it to the node's added peers list.
+    Onetry((IpAddr, u16, bool)),
+
+    /// Attempt to disconnect from a peer.
+    Disconnect((IpAddr, u16)),


Use SocketAddr instead of IpAddr + u16.

This will be fixed by #984

luisschwab · 2026-05-13T18:37:48Z

I would rename the traits to:

Chain

Network

Mempool

Filters

Wallet

I didn't want to give it names that will clash with other structs, like Mempool

You could add Methods as a suffix.

Davidson-Souza added this to the Q2/2026 milestone May 6, 2026

Davidson-Souza self-assigned this May 6, 2026

Davidson-Souza added the architecture label May 6, 2026

Davidson-Souza added this to Floresta May 6, 2026

github-project-automation Bot moved this to Backlog in Floresta May 6, 2026

luisschwab self-requested a review May 6, 2026 22:29

jaoleal reviewed May 7, 2026

View reviewed changes

Davidson-Souza moved this from Backlog to Needs review in Floresta May 7, 2026

jaoleal mentioned this pull request May 10, 2026

[POC BDK] Introduce modular trait-based watch-only wallet architecture #950

Draft

Davidson-Souza force-pushed the feat/node-interface-trait branch 2 times, most recently from 36e9bc7 to f2c2d01 Compare May 12, 2026 17:31

Davidson-Souza force-pushed the feat/node-interface-trait branch from f2c2d01 to 38485e8 Compare May 12, 2026 17:36

lorenzolfm reviewed May 13, 2026

View reviewed changes

luisschwab suggested changes May 13, 2026

View reviewed changes

Conversation

Davidson-Souza commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and Notes

Changelog

Note to reviewers

Uh oh!

lorenzolfm commented May 7, 2026

Uh oh!

Davidson-Souza commented May 7, 2026

Uh oh!

luisschwab commented May 7, 2026

Uh oh!

jaoleal commented May 7, 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!

Davidson-Souza commented May 7, 2026

Uh oh!

Davidson-Souza commented May 12, 2026

Uh oh!

luisschwab commented May 12, 2026

Uh oh!

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

luisschwab May 13, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Davidson-Souza commented May 6, 2026 •

edited

Loading

luisschwab May 13, 2026 •

edited

Loading