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

Disable merkle proofs for v2 get_block #3487

Merged

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Nov 9, 2020

This PR defaults to include_merkle_proof = false in the v2 api get_block jsonrpc endpoint.
These Merkle proofs are not actively used by anyone and were originally included (I believe) as a "proof of concept".
They are expensive to generate for historical blocks (specifically against a full archival node).

Related #3483.


Tested via -

curl -X POST \
     -H 'Content-Type: application/json' \
     -d '{"jsonrpc":"2.0","id":1,"method":"get_block","params":[952019, null, null]}' \
         http://localhost:3413/v2/foreign

This is equivalent behavior to the optional ?no_merkle_proof url param in the original v1 api endpoint -

curl "localhost:3413/v1/blocks/952022?no_merkle_proof"

Comment on lines 138 to 142
curl -X POST \
-H 'Content-Type: application/json' \
-d '{"jsonrpc":"2.0","id":1,"method":"get_block","params":[952019, null, null]}' \
http://localhost:3413/v2/foreign

Copy link
Member

Choose a reason for hiding this comment

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

What's that? The doctests (currently disabled) are just below.

Copy link
Member Author

Choose a reason for hiding this comment

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

lol 😭

{
"jsonrpc": "2.0",
"method": "get_block",
"params": [374274, null, null],
Copy link
Member

Choose a reason for hiding this comment

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

This is the value you want to change here I guess

@quentinlesceller
Copy link
Member

Other than that everything looks good to me.

@quentinlesceller quentinlesceller merged commit e6145db into mimblewimble:master Nov 9, 2020
@antiochp antiochp mentioned this pull request Nov 26, 2020
@yeastplume yeastplume mentioned this pull request Feb 23, 2022
26 tasks
bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants