Centralize server-side defaults and align `addnode` behavior with Bitcoin Core by moisesPompilio · Pull Request #986 · getfloresta/Floresta

moisesPompilio · 2026-04-23T02:51:33Z

Description and Notes

Transfers the responsibility for deciding default values from the CLI and RPC clients to the server, so the logic is centralized in one place instead of being duplicated across multiple layers.

In floresta-cli and floresta-rpc, client-side defaults for Option parameters were removed. When a parameter is optional, the client now omits it entirely instead of forcing a default value with unwrap_or(), allowing the server to apply its own configured behavior.

The addnode v2transport default was also adjusted to match the node configuration, following Bitcoin Core behavior:

true by default, unless --allow-v1-fallback is enabled
false when --allow-v1-fallback is configured

How to verify the changes you have done?

Check that optional RPC and CLI parameters are no longer defaulted on the client side and are instead handled by the server.

For addnode, verify that the v2transport behavior follows the node configuration, matching Bitcoin Core defaults.

Without --allow-v1-fallback, the default should be true.
With --allow-v1-fallback, the default should be false.
Confirm that omitted optional parameters are no longer sent with forced defaults from the client.

jaoleal · 2026-04-23T11:01:05Z

+        command: AddNodeCommand,
+        v2transport: Option<bool>,
+    ) -> Result<Value> {
+        let mut params = vec![Value::String(node), Value::String(command.to_string())];


I think that we could work out some helpers for this, an example is the arg helpers on jsonrpc

I introduced RpcArg and rpc_params(), which already handle these parameter treatments automatically, so when it’s an option that should not be displayed, it automatically hides that option.

Micah-Shallom · 2026-04-23T23:19:59Z

cACK @moisesPompilio ... i think a small test for the new behaviour is worth introducing...at minimum addnode with and without v2transport hitting the proper defaults of the server

Micah-Shallom · 2026-04-23T23:40:10Z

        node_address: String,
        command: String,
-        v2transport: bool,
+        v2transport: Option<bool>,


is this change worth documenting as a minor bump for downstream users of this RPC? or maybe that doesnt matter much right now since the commmunity building on floresta is not large yet?

more of a check question for myself too since i have recently worked on an RPC that modified fieldnames

This is already documented in the RPC docs; this field is optional, so nothing changes for the user.

I think its a fair point, i think its fine to break things now because we didnt made the 1.0 release yet. The user count is not something we can be much sure of, AFAIK...

jaoleal · 2026-04-24T12:48:32Z

Also @moisesPompilio, do a check if the defaults changes arent being solved on #831 I changed a lot about how we do defaults there

… defaults The v2transport parameter in addnode is now optional. When omitted, the server applies its configured default value: - true (unless --allow-v1-fallback is set) - false (if --allow-v1-fallback is configured) This removes the requirement for explicit specification in the CLI while allowing the server's configuration to determine the connection behavior.

Remove unwrap_or() calls that force default values on the client side. Optional parameters are now omitted when None, letting the server determine appropriate defaults instead of the CLI forcing them. Introduce RpcArg enum and rpc_params() helper function to enable this: - RpcArg::Value wraps required parameters - RpcArg::Optional wraps optional parameters and filters None values - rpc_params() automatically collects parameters and handles filtering Implement From trait conversions for String, bool, u32, Txid, BlockHash, Vec<T>, and Option<T> types to provide seamless parameter construction across the RPC layer without manual error handling. Add comprehensive unit tests validating RpcArg conversions, parameter filtering behavior.

Add unit tests validating the behavior and correctness of all FlorestaRPC trait methods. Tests verify that method calls are properly forwarded to the JSON-RPC client, parameters are correctly serialized and passed, and returned values match expected results. Includes tests for all RPC methods including parameter handling, optional arguments filtering, and deserialization of responses.

moisesPompilio · 2026-04-24T21:24:59Z

e077196 I introduced the RpcArg enum and rpc_params(), which automatically handle parameter processing, and I added unit tests for this RPC part, which were missing, to verify whether the parameters and method are being passed correctly, and whether the response is being returned correctly.

moisesPompilio · 2026-04-24T21:33:55Z

Also @moisesPompilio, do a check if the defaults changes arent being solved on #831 I changed a lot about how we do defaults there

This PR is specifically to remove the default handling from floresta-rpc and floresta-cli and move that responsibility entirely to floresta-node. So #831 doesn’t solve this problem, since it only handles floresta-node. This PR does have two small conflicts with #831, but that’s just because I had to adjust the defaults for findtxo and addnode

jaoleal · 2026-04-27T14:55:32Z

    }
 }
+
+enum RpcArg {


Perhaps this can be an extension just like done in HeaderExtension ?

The Value type itself have enough capacity to handle this without the RpcArg enum or any middle structure.

Perhaps this can be an extension just like done in HeaderExtension ?

In this case, it can’t be an extension because it needs to be a separate enum. It checks, when it is an Option, whether the element should be added to the Vec stack. So I can’t really make it an extension the Value, for example. Also, I don’t think any other part of the code will need this, because it is directly tied only to when we make a request to the server, since it exists solely to hide information when it is null.

The Value type itself have enough capacity to handle this without the RpcArg enum or any middle structure.

Value doesn’t handle that case, because if a null value is pushed onto the stack, it preserves the null entry. If it removed it, the Vec would lose consistency with the items that were inserted

jaoleal · 2026-04-27T14:57:22Z

+        let params = rpc_params([tx.into()]);
+        self.call("sendrawtransaction", &params)


Really liked how we consume this.

jaoleal · 2026-04-27T14:58:16Z

-                vec![txids]
-            }
-        };
+        let params = rpc_params([txids.into(), blockhash.into()]);


I think you can get rid of those into somehow

It could be hidden using a macro, but I don’t think removing it is possible because it’s being converted into another object. However, I’m not sure whether using a macro here would be a good idea

jaoleal · 2026-04-27T15:07:49Z

+    impl MockRpcClient {
+        fn set_result(&self, result: Value) {
+            *self.result.borrow_mut() = Some(result);
+        }
+    }


I dont think this test is actually testing something besides the rpc args parsing which is also very shallow

Yeah, the tests are simple because these functions are simple, but they verify that the function is calling the method correctly, passing the correct information, and returning it properly as well.

moisesPompilio self-assigned this Apr 23, 2026

moisesPompilio added the reliability Related to runtime reliability, stability and production readiness label Apr 23, 2026

moisesPompilio added this to Floresta Apr 23, 2026

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

moisesPompilio moved this from Backlog to Needs review in Floresta Apr 23, 2026

moisesPompilio added this to the Q2/2026 milestone Apr 23, 2026

jaoleal self-requested a review April 23, 2026 10:48

jaoleal reviewed Apr 23, 2026

View reviewed changes

Micah-Shallom reviewed Apr 23, 2026

View reviewed changes

moisesPompilio added 3 commits April 24, 2026 17:59

moisesPompilio force-pushed the chore-default-cli branch from 8c62280 to e077196 Compare April 24, 2026 21:15

jaoleal reviewed Apr 27, 2026

View reviewed changes

		let params = rpc_params([tx.into()]);
		self.call("sendrawtransaction", &params)

Conversation

moisesPompilio commented Apr 23, 2026

Description and Notes

How to verify the changes you have done?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Micah-Shallom commented Apr 23, 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!

jaoleal commented Apr 24, 2026

Uh oh!

moisesPompilio commented Apr 24, 2026

Uh oh!

moisesPompilio commented Apr 24, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants