Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add receive_hash option for the blocks_info RPC #3702

Merged

Conversation

Exxenoz
Copy link
Contributor

@Exxenoz Exxenoz commented Jan 31, 2022

Closes #3701

@zhyatt zhyatt added enhancement rpc Changes related to Remote Procedure Calls labels Jan 31, 2022
@zhyatt zhyatt added this to the V24.0 milestone Jan 31, 2022
@Exxenoz Exxenoz force-pushed the rpc_add_receive_hash_to_blocks_info branch from 0174324 to 25c565a Compare February 3, 2022 12:50
@Exxenoz
Copy link
Contributor Author

Exxenoz commented Feb 3, 2022

@dsiganos * Fixed clang format

@Exxenoz Exxenoz force-pushed the rpc_add_receive_hash_to_blocks_info branch from 25c565a to 4ca8e76 Compare February 5, 2022 18:06
@Exxenoz
Copy link
Contributor Author

Exxenoz commented Feb 5, 2022

Small consistency update: In case no receive block is found the full zero hash is now returned instead of just "0" (similar to the "successor" field).

nano/node/json_handler.cpp Outdated Show resolved Hide resolved
nano/node/json_handler.cpp Outdated Show resolved Hide resolved
@dsiganos dsiganos requested a review from clemahieu March 15, 2022 23:30
@dsiganos
Copy link
Contributor

@Exxenoz I made some improvements. PLease review, if possible.

@Exxenoz
Copy link
Contributor Author

Exxenoz commented Mar 16, 2022

@Exxenoz I made some improvements. PLease review, if possible.

Looks good to me. 👍

@dsiganos dsiganos requested review from theohax and thsfs March 27, 2022 13:37
clemahieu
clemahieu previously approved these changes Mar 28, 2022
@@ -1774,6 +1774,54 @@ void nano::node::populate_backlog ()
}
}

/** Given the block hash of a send block, find the associated receive block that receives that send.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this function should be packed into the ledger.cpp file, or in another more specific file, if there is a better one. This is more related to the ledger logic and the node is a more general class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. But I do not yet understand how the database classes are structured and I don't know how to do that.
So I have taken the easy and slightly less accurate route of implementing in the node class.
We can work on this together to improve it, if you like.

Copy link
Contributor

@thsfs thsfs Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to send the function to json_handler.cpp as it isn't used anywhere else, but as you @dsiganos foresee other usages, so let's keep the function in the ledger class, as it is also better for unit test access.

thsfs
thsfs previously approved these changes Mar 28, 2022
@dsiganos dsiganos force-pushed the rpc_add_receive_hash_to_blocks_info branch from 434045f to 48b8966 Compare March 28, 2022 20:05
@dsiganos dsiganos requested review from clemahieu and thsfs March 28, 2022 20:05
@dsiganos
Copy link
Contributor

dsiganos commented Mar 28, 2022

This PR couldn't be merged because it was conflicting.
Merging would be too messy because this Pr has been alive for too long.
So I squashed and rebased and then resolved the conflicts.
Please recheck.

Added the function nano::node::find_receive_block_by_send_hash to
find the receive block linked with a send block for better code reuse.

Co-authored-by: Dimitrios Siganos <[email protected]>
Co-authored-by: Thiago Silva <[email protected]>
@dsiganos dsiganos force-pushed the rpc_add_receive_hash_to_blocks_info branch from 48b8966 to 77c9fda Compare March 28, 2022 20:46
@dsiganos dsiganos self-requested a review March 28, 2022 21:02
@dsiganos dsiganos merged commit 4752c66 into nanocurrency:develop Mar 28, 2022
@thsfs thsfs added the documentation This item indicates the need for or supplies updated or expanded documentation label Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This item indicates the need for or supplies updated or expanded documentation enhancement rpc Changes related to Remote Procedure Calls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add receive_hash option for the blocks_info RPC
5 participants