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

fix(rpc): fix commands handling #2515

Merged
merged 6 commits into from
Aug 9, 2023

Conversation

vgonkivs
Copy link
Member

Overview

Fixes all found rpc issues

Fixes #2503 and #2500

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs added area:rpc kind:fix Attached to bug-fixing PRs labels Jul 27, 2023
@vgonkivs vgonkivs requested a review from renaynay as a code owner July 27, 2023 10:47
@vgonkivs vgonkivs self-assigned this Jul 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

Codecov Report

Merging #2515 (4c83925) into main (a86ddca) will decrease coverage by 0.43%.
Report is 8 commits behind head on main.
The diff coverage is 19.35%.

@@            Coverage Diff             @@
##             main    #2515      +/-   ##
==========================================
- Coverage   51.39%   50.97%   -0.43%     
==========================================
  Files         158      158              
  Lines       10304    10395      +91     
==========================================
+ Hits         5296     5299       +3     
- Misses       4556     4632      +76     
- Partials      452      464      +12     
Files Changed Coverage Δ
cmd/celestia/rpc.go 17.56% <0.00%> (-1.07%) ⬇️
share/getters/cascade.go 71.62% <0.00%> (-6.64%) ⬇️
cmd/celestia/blob.go 9.80% <85.71%> (ø)

... and 19 files with indirect coverage changes

@jcstein
Copy link
Member

jcstein commented Jul 27, 2023

testing out https://based64.xyz (so testing retrieval of submitted plain-text) on this branch and it is not, functioning as expected, so this doesn't resolve the issue #2503 of plain text data that is expected to be returned in base64, being returned as base64

Screenshot 2023-07-27 at 14 05 01

my version matches last commit on this PR:

Semantic version: v0.11.0-rc8-13-ga9ac16a2
Commit: a9ac16a24181094162b54b2ce5e3616037038fdc
Build Date: Thu Jul 27 14:01:39 CEST 2023
System version: arm64/darwin
Golang version: go1.20.2

as an example, i will try to submit "gm" and retrieve base64 that converts to string as "gm". alternatively, as explained in this comment: #2503 (comment) the only difference is i am using the version above

  1. celestia light start --core.ip consensus-full-arabica-9.celestia-arabica.com --p2p.network arabica
  2. export CELESTIA_NODE_AUTH_TOKEN=$(celestia light auth admin --p2p.network arabica)
  3. celestia rpc blob Submit 0x42690c204d39600fddd3 '"gm"'
  4. celestia rpc blob GetAll [blockheightfrom#3] 0x42690c204d39600fddd3
  5. it should convert to "gm" with some metadata, it doesn't
Screenshot 2023-07-27 at 14 10 00
  1. oddly, converting from base64 -> hex and then from hex -> text returns the same data as converting base64 -> text as shown above:
Screenshot 2023-07-27 at 14 11 43

@jcstein
Copy link
Member

jcstein commented Jul 27, 2023

however, i believe GetSharesByNamespace is working as expected:

celestia rpc share GetSharesByNamespace "$(celestia rpc header GetByHeight 103692 | jq '.result.dah' -r)" 0x42690c204d39600fddd3
{
  "jsonrpc": "2.0",
  "result": [
    {
      "Shares": [
        "AAAAAAAAAAAAAAAAAAAAAAAAAEJpDCBNOWAP3dMBAAAACEltZHRJZz09AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
      ],
      "Proof": {
        "start": 1,
        "end": 2,
        "nodes": [
          "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABHBoPeolnLUCIgDXv2uScYcpg3V/gJIIwil8j33b33eH",
          "/////////////////////////////////////////////////////////////////////////////4S2andMgMf77TuFT8NmQm5qoIqmmDsUmUHKwGCZ+wg0"
        ],
        "leaf_hash": null,
        "is_max_namespace_id_ignored": true
      }
    }
  ],
  "id": 1
}

@jcstein
Copy link
Member

jcstein commented Jul 27, 2023

linking docs context celestiaorg/docs#915 (comment)

@jcstein jcstein closed this Jul 27, 2023
@jcstein
Copy link
Member

jcstein commented Jul 27, 2023

oops 😅

@jcstein jcstein reopened this Jul 27, 2023
@vgonkivs
Copy link
Member Author

celestia light start --core.ip consensus-full-arabica-9.celestia-arabica.com --p2p.network arabica
export CELESTIA_NODE_AUTH_TOKEN=$(celestia light auth admin --p2p.network arabica)
celestia rpc blob Submit 0x42690c204d39600fddd3 '"gm"'
celestia rpc blob GetAll [blockheightfrom#3] 0x42690c204d39600fddd3
it should convert to "gm" with some metadata, it doesn't

I missed the thing with converting from one type to another and fixed errors in our code.

@vgonkivs
Copy link
Member Author

I feel like #2503 has two potential issues inside.

  1. Issue with converting(that @jcstein mentioned in Actual Result)
  2. Issue with incorrect handling of rpc commands(see Relevant Output).

This PR is now fixing only issue #2. I need to look deeper what is wrong with the serialisation.

blob/blob.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs marked this pull request as draft July 31, 2023 11:26
@vgonkivs vgonkivs marked this pull request as ready for review August 7, 2023 14:36
@vgonkivs vgonkivs requested a review from Wondertan August 7, 2023 14:36
cmd/celestia/rpc.go Outdated Show resolved Hide resolved
share/getters/cascade.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 8, 2023

FYI @jcstein , we changed the plaintext flag to base64 in order to convert data to the human readable format by default. To get the data as base64 string, you have to pass --base64=true

@vgonkivs vgonkivs enabled auto-merge (squash) August 8, 2023 10:03
@vgonkivs vgonkivs merged commit 94a6b63 into celestiaorg:main Aug 9, 2023
13 of 15 checks passed
@jcstein
Copy link
Member

jcstein commented Aug 9, 2023

what happens if no --plaintext or --base64 flag?

@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 9, 2023

Data will be converted to the plain text by default

renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Aug 9, 2023
jcstein added a commit to celestiaorg/docs that referenced this pull request Aug 9, 2023
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Aug 23, 2023
walldiss pushed a commit to walldiss/celestia-node that referenced this pull request Sep 22, 2023
walldiss pushed a commit that referenced this pull request Sep 22, 2023
walldiss pushed a commit to walldiss/celestia-node that referenced this pull request Sep 22, 2023
walldiss pushed a commit to walldiss/celestia-node that referenced this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rpc kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blob.GetAll not returning correct base64 data, GetShare and GetSharesByNamespace also don't work
6 participants