chore(rpcserver): Match signature of getblock to its rpc #788

jaoleal · 2026-01-17T17:29:52Z

Description and Notes

#682 introduced changes that brought getblock rpc method closer to bitcoin core behavior. The changes presented here make the internal method signature to match the specification.

Davidson-Souza · 2026-01-17T18:23:09Z

Marking as draft since it depends on #682

jaoleal · 2026-01-21T14:18:01Z

Marking as ready to review

cc @Davidson-Souza @moisesPompilio

Davidson-Souza

What 781efaf achieves that tests/floresta-cli/getblock.py don't?

crates/floresta-node/src/json_rpc/blockchain.rs

moisesPompilio · 2026-01-21T15:16:43Z

crates/floresta-rpc/src/test_data/mod.rs

+
+use crate::rpc_types::GetBlockRes;
+
+pub fn getblock_data(verbose: bool) -> GetBlockRes {


verbose is not a bool but a numeric type, I think we need to fix this here.

This is just a internal helper to return the data, It does not need to be fallible yet neither needs to be api compatible. The bool can express both verbosity we support rn

moisesPompilio · 2026-01-21T15:26:49Z

crates/floresta-rpc/src/lib.rs

+    use crate::test_data::getblock_data;

-    struct Florestad {
+    pub struct Florestad {


Is there any reason to make this public?

jaoleal · 2026-01-21T16:22:16Z

Applied suggestions on 17161d2

jaoleal · 2026-01-21T18:48:04Z

What 781efaf achieves that tests/floresta-cli/getblock.py don't?

Its easy to run

Davidson-Souza · 2026-01-21T20:06:28Z

Its easy to run

That's not a reason to have two modules doing the same thing. Specially given that this doesn't test things like chain work and mtp calculation.

I like 17161d2, but NACK c733bbf

jaoleal · 2026-01-22T15:46:55Z

Really ? Honestly i see value in this, the idea is not to compete or to replace the python tests, as you said and I briefly tried to imply is that the python tests indeed deliver more assertion value but in the cost of having a lot more dependencies, including the environment setup needed.

The value i see in this lives in the case where we want to assert that we are not breaking any field and any other test, the older getblock test was doing this and my plan was just to expand it make a direct comparison with a bitcoin core output.

Im the "vitcim" of one of these cases, the floresta-nix packaging have these tests blacklisted exactly because its trying to get instances up and running... I cant do much in the nix part, thats how they achieved packaging determinism, but hear this propose.

Even with this i cant run the test, i had the idea to include this and to move it to be a functional test in the server without instantiating a florestad, we can make something similar as done here in extensions.rs... As you said, theres no reason to have two modules doing the same thing and we are investing a lot of time to make the python tests better, and i see these tests in floresta-rpc and indeed trying to compete against our python tests, specially because is a integration test between client and server.

If youre willing to accept #787 we wont need to check if we are parsing the arguments correctly because im literally just getting the args from FlorestaRpc and passing to rust-bitcoin`s jsonrpc... And they already tested their client

Davidson-Souza · 2026-01-22T16:25:34Z

Im the "vitcim" of one of these cases, the floresta-nix packaging have these tests blacklisted exactly because its trying to get instances up and running... I cant do much in the nix part, thats how they achieved packaging determinism, but hear this propose.

That could as well be in the PR description. I did not understand what you were doing here because it just says

It asserts that #682 is indeed working

so does getblock.py. Since the goal here is to make it a simple functional test like the others we already have (and have a use-case for it), things are different. Not a big fan of this json approach, tho. All other tests leave the values inside the test itself.

jaoleal · 2026-01-22T18:55:05Z

I did not understand what you were doing here because it just says

Im well aware of that, Im really focusing on being clear following your suggestion. Sincerely, I was frustrated to be spending more time writing and reviewing my writing than actually coding and contributing, as the time goes im improving that to be more efficient. Thanks for the patience.

so does getblock.py. Since the goal here is to make it a simple functional test like the others we already have (and have a use-case for it), things are different. Not a big fan of this json approach, tho. All other tests leave the values inside the test itself.

Well, I might change that, we can hardcode these same values directly in types(maybe implementing default ?), I used the raw json because its literally a copy-paste from what i got from bitcoin core... regarding having simple and lightweight functional tests server side, are there any cons ?

Also, I want to bring up the point that we are doing a lightweight node with the objective to run on constrained devices, I think its important to have tests that people can actually run without much trouble, and that, getblock.py does not achieve... And its fairly easy to have the tests that Im proposing

jaoleal · 2026-02-03T12:58:08Z

Okay, dropped the commit that was actually extending the tests.

Ill present the same changes with different approach later

Davidson-Souza · 2026-02-03T14:37:54Z

Ill present the same changes with different approach later

Are you planning to do this on this PR or another one? If you want to do it here, please mark it as draft

For new approach, I recommend just following the same approach taken by other tests in this file.

jaoleal · 2026-02-03T14:54:16Z

Yes, another PR

jaoleal · 2026-02-03T15:00:35Z

For new approach, I recommend just following the same approach taken by other tests in this file.

Im not sure if i got this correctly. What you mean by "the same approach" ?

crates/floresta-node/src/json_rpc/blockchain.rs

Davidson-Souza

ACK d36a64d

crates/floresta-node/src/json_rpc/server.rs

moisesPompilio · 2026-02-09T13:39:06Z

ACK b37772d

Davidson-Souza

ACK b37772d

Davidson-Souza · 2026-02-09T15:19:54Z

@jaoleal can you update the PR description, please?

jaoleal · 2026-02-09T18:41:02Z

Changed the pr description but im still unable to alter the name of the PR.

Maybe chore(rpcserver): Match signature of getblock to its rpc, same as the pr commit message, would be good.

jaoleal changed the title ~~Assert getblock corecompatible~~ test: Assert getblock corecompatible Jan 17, 2026

Davidson-Souza added the RPC Changes something with our JSON-RPC interface label Jan 17, 2026

Davidson-Souza changed the title ~~test: Assert getblock corecompatible~~ test: Assert getblock core compatible Jan 17, 2026

Davidson-Souza marked this pull request as draft January 17, 2026 18:22

jaoleal force-pushed the assert_getblock_corecompatible branch 6 times, most recently from 4467f93 to 0f0dd5f Compare January 21, 2026 14:16

jaoleal marked this pull request as ready for review January 21, 2026 14:17

Davidson-Souza requested changes Jan 21, 2026

View reviewed changes

crates/floresta-node/src/json_rpc/blockchain.rs Outdated Show resolved Hide resolved

moisesPompilio reviewed Jan 21, 2026

View reviewed changes

jaoleal force-pushed the assert_getblock_corecompatible branch from 0f0dd5f to 17161d2 Compare January 21, 2026 16:21

luisschwab assigned jaoleal Jan 28, 2026

jaoleal force-pushed the assert_getblock_corecompatible branch from 17161d2 to 6a7bcd4 Compare February 3, 2026 11:51

Davidson-Souza requested changes Feb 3, 2026

View reviewed changes

crates/floresta-node/src/json_rpc/blockchain.rs Show resolved Hide resolved

crates/floresta-node/src/json_rpc/blockchain.rs Show resolved Hide resolved

jaoleal force-pushed the assert_getblock_corecompatible branch from 6a7bcd4 to 1aa4c6a Compare February 4, 2026 15:13

jaoleal force-pushed the assert_getblock_corecompatible branch from 1aa4c6a to d36a64d Compare February 4, 2026 15:19

Davidson-Souza previously approved these changes Feb 4, 2026

View reviewed changes

moisesPompilio reviewed Feb 4, 2026

View reviewed changes

crates/floresta-node/src/json_rpc/server.rs Outdated Show resolved Hide resolved

chore(rpcserver): Match signature of getblock to its rpc

b37772d

jaoleal dismissed Davidson-Souza’s stale review via b37772d February 9, 2026 10:02

jaoleal force-pushed the assert_getblock_corecompatible branch from d36a64d to b37772d Compare February 9, 2026 10:02

Davidson-Souza approved these changes Feb 9, 2026

View reviewed changes

Davidson-Souza changed the title ~~test: Assert getblock core compatible~~ chore(rpcserver): Match signature of getblock to its rpc Feb 9, 2026

Davidson-Souza merged commit a323d74 into getfloresta:master Feb 9, 2026
10 of 13 checks passed


		use crate::rpc_types::GetBlockRes;

		pub fn getblock_data(verbose: bool) -> GetBlockRes {

chore(rpcserver): Match signature of getblock to its rpc #788

chore(rpcserver): Match signature of getblock to its rpc #788

Conversation

jaoleal commented Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and Notes

Uh oh!

Davidson-Souza commented Jan 17, 2026

Uh oh!

jaoleal commented Jan 21, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moisesPompilio Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

jaoleal Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

moisesPompilio Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

jaoleal commented Jan 21, 2026

Uh oh!

jaoleal commented Jan 21, 2026

Uh oh!

Davidson-Souza commented Jan 21, 2026

Uh oh!

jaoleal commented Jan 22, 2026

Uh oh!

Davidson-Souza commented Jan 22, 2026

Uh oh!

jaoleal commented Jan 22, 2026

Uh oh!

jaoleal commented Feb 3, 2026

Uh oh!

Davidson-Souza commented Feb 3, 2026

Uh oh!

jaoleal commented Feb 3, 2026

Uh oh!

jaoleal commented Feb 3, 2026

Uh oh!

Uh oh!

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moisesPompilio commented Feb 9, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Davidson-Souza commented Feb 9, 2026

Uh oh!

jaoleal commented Feb 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaoleal commented Jan 17, 2026 •

edited

Loading