test: Assert getblock core compatible by jaoleal · Pull Request #788 · getfloresta/Floresta

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

Description and Notes

This adds more extensive assert data which i retrieved from a local v30 Bitcoin Core...

It asserts that #682 is indeed working and also introduce a new /test_data dir for floresta-rpc.

~~I did change the type to use v30 instead of v29 but i expect that to be addressed on #682~~ Addressed.

Also i made GetBlockReq: PartialReq.

Depends on #682

I also added a second commit that present some chores related to match the ignature of the method to its accordingly rpc

How to verify the changes you have done?

check that the data Im including in test_data is indeed the return of

 bitcoin-cli -regtest  getblock 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206

 bitcoin-cli -regtest  getblock 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 0

after that you just need to run the test for floresta-rpc

cargo build

cargo test -p floresta-rpc

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).

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

moisesPompilio · 2026-02-04T20:18:47Z

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

+            let _ = state
+                .get_block(hash, 0)
                .await
-                .map(|v| serde_json::to_value(v).unwrap())
+                .map(|v| serde_json::to_value(v).unwrap());


Suggested change

let _ = state

.get_block(hash, 0)

.await

.map(|v| serde_json::to_value(v).unwrap())

.map(|v| serde_json::to_value(v).unwrap());

state.get_block(hash, 0).await.iter().for_each(|v| {

serde_json::to_value(v).unwrap();

});

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

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

d36a64d

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

Davidson-Souza approved these changes Feb 4, 2026

View reviewed changes

moisesPompilio reviewed Feb 4, 2026

View reviewed changes


		use crate::rpc_types::GetBlockRes;

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

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

How to verify the changes you have done?

Contributor Checklist

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!

moisesPompilio Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaoleal commented Jan 17, 2026 •

edited

Loading