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

chore: allow to call store api endpoints without a storenode (#1575) #1647

Merged

Conversation

Ivansete-status
Copy link
Collaborator

Chore

#1575

Comment

Separated change to always enable the store-rpc handler even though the 'storenode' param is not passed.

Additional info for new members

The storenode param is described ain:

storenode* {.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Yeah, makes sense to me!

@jm-clius
Copy link
Contributor

jm-clius commented Apr 4, 2023

Btw, if you use keywords such as "closes #1575" in your description, it will automatically close the related issue when this PR gets merged.

@LNSD
Copy link
Contributor

LNSD commented Apr 5, 2023

Separated change to always enable the store-rpc handler even though the 'storenode' param is not passed.

These changes, to work, require the WakuNode to mount the WakuStoreClient without passing the --storenode configuration flag. Otherwise, IIRC, the Waku Store JSON-RPC API will return a 500, internal error: "waku store client not mounted".

And another question: How would the Waku Store JSON-RPC behave if no nodes support the waku store protocol in the peer manager?

In my opinion, the two things above should be addressed before merging these changes.

Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

Please, check my comment 🙂

@Ivansete-status
Copy link
Collaborator Author

Thanks for your comments @LNSD. I will tackle them straight away.

And another question: How would the Waku Store JSON-RPC behave if no nodes support the waku store protocol in the peer manager?

In this case it should return a 500, internal error: "No peer store nodes found" (or sth like that) because the client doesn't have means to provide the peer store address in JSON-RPC.. therefore, 500 should be returned.

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

thanks for the pr! same concerns as @LNSD

  • not sure it will work without mounting the waku store client. once fixed mind including couple of commands (wakunode --flags...) and a curl command to test that it works? we could even add a unit test for this.
  • specify the error to be returned in case there are no peers supporting the store protocol. In this case selectPeer will return a none() and the request wont be completed.

@Ivansete-status Ivansete-status force-pushed the chore-1575-allow-store-api-without-storenode branch from b790608 to 87ace11 Compare April 6, 2023 15:44
@Ivansete-status
Copy link
Collaborator Author

Ivansete-status commented Apr 6, 2023

In case of missing a peer-store-node, when calling get_waku_v2_store_v1_messages from the client side, this raises an exception (because it is raised here)

I added a test that validates this behavior.

How to test this manually

  1. Start a solitaire node
    ./build/wakunode2 --log-level="DEBUG"
    
  2. Send 'get_waku_v2_store_v1_messages':
    curl -d '{"jsonrpc":"2.0","id":"id","method":"get_waku_v2_store_v1_messages"}' --header "Content-Type: application/json" http://localhost:8545
    
    The curl command will show:
    {"jsonrpc":"2.0","id":"id","error":{"code":-32000,"message":"get_waku_v2_store_v1_messages raised an exception","data":"no suitable remote store peers"}}
    

@Ivansete-status Ivansete-status force-pushed the chore-1575-allow-store-api-without-storenode branch from 87ace11 to bfe8fb8 Compare April 6, 2023 16:13
@Ivansete-status Ivansete-status self-assigned this Apr 6, 2023
* test_jsonrpc_store: testing when there is no peer-store-node available
@Ivansete-status Ivansete-status force-pushed the chore-1575-allow-store-api-without-storenode branch from bfe8fb8 to 3331a7b Compare April 6, 2023 18:12
Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@LNSD LNSD requested a review from alrevuelta April 7, 2023 07:25
Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

LGTM ✅

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm! thanks!

a clarification on this to ensure we are aligned.

In case of missing a peer-store-node, when calling get_waku_v2_store_v1_messages from the client side, this raises an exception (because it is raised here)

Would like to ellaborate on what In case of missing a peer-store-node means. A node has the following ways of having a peer-store-node:

  • Configured via cli flag, which sets it as the preferred one (calls addServicePeer under the hood)
  • Discovered and identified (either because we connect to it or it connected to us). This means that any node supporting the store protocol, can be a potential store node for us if no prefered one was configured.

I just confirmed this is the case with your fix, so no action required :)

./build/wakunode2 --rpc-admin=true --dns-discovery=true --dns-discovery-url=enrtree://AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM@prod.nodes.status.im --discv5-discovery=true

Now get_waku_v2_store_v1_messages works without provoding a store node. What happens under the hood is that an identified node supporting store is selected for the query.

curl -d '{"jsonrpc":"2.0","id":"id","method":"get_waku_v2_store_v1_messages"}' --header "Content-Type: application/json" http://localhost:8545

@Ivansete-status Ivansete-status merged commit 0b4a2e6 into master Apr 12, 2023
12 checks passed
@Ivansete-status Ivansete-status deleted the chore-1575-allow-store-api-without-storenode branch April 12, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants