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

getConfirmedBlock: add encoding optional parameter #7756

Merged
merged 3 commits into from
Jan 13, 2020

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Jan 10, 2020

Problem

Currently, the Transactions and Hashes returned by the getConfirmedBlock method are only nominally in JSON format... anything serialized with short_vec appears as an array of bytes, with the short_vec length as an artifact. This is not friendly for humans or machines.

Summary of Changes

  • Add an optional encoding parameter to specify JSON or binary format ("json", "binary"). If this parameter is not provided, the method returns JSON

Before:

{"jsonrpc":"2.0","result":{"blockhash":[15,174,10,25,54,47,135,83,10,197,138,74,168,179,16,94,149,240,6,101,61,94,177,217,182,235,250,220,34,165,235,231],"parentSlot":5050,"previousBlockhash":[109,153,35,131,54,24,180,169,120,2,250,229,13,100,94,14,73,32,90,229,192,154,104,174,168,10,76,135,44,160,222,255],"transactions":[[{"message":{"accountKeys":[[5],[104,199,171,32,70,11,114,246,73,255,240,206,227,228,40,237,155,126,165,127,62,228,232,46,139,246,29,153,118,123,255,158],[22,127,165,61,250,121,95,180,243,23,123,26,125,245,158,110,40,52,159,178,80,98,129,95,175,247,232,158,252,142,100,236],[6,167,213,23,25,47,10,175,198,242,101,227,251,119,204,122,218,130,197,41,208,190,59,19,110,45,0,85,32,0,0,0],[6,167,213,23,24,199,116,201,40,86,99,152,105,29,94,182,139,94,184,163,155,75,109,92,115,85,91,33,0,0,0,0],[7,97,72,29,53,116,116,187,124,77,118,36,235,211,189,179,216,53,94,115,209,16,67,252,13,163,83,128,0,0,0,0]],"header":{"numReadonlySignedAccounts":0,"numReadonlyUnsignedAccounts":3,"numRequiredSignatures":2},"instructions":[[1],{"accounts":[[3],1,2,3],"data":[[53],2,0,0,0,1,0,0,0,0,0,0,0,186,19,0,0,0,0,0,0,254,248,120,160,6,198,155,125,91,237,67,5,174,193,8,211,87,44,56,2,228,212,94,11,35,46,35,93,223,238,114,73,0],"programIdIndex":4}],"recentBlockhash":[109,153,35,131,54,24,180,169,120,2,250,229,13,100,94,14,73,32,90,229,192,154,104,174,168,10,76,135,44,160,222,255]},"signatures":[[2],[128,197,204,227,151,23,164,209,192,104,225,55,86,183,64,136,43,175,39,153,145,216,6,60,247,172,178,13,126,56,192,158,193,88,6,206,221,34,92,59,18,126,186,207,94,211,86,180,73,139,197,244,85,237,24,161,107,55,202,231,179,237,19,6],[143,240,162,250,92,248,173,247,240,152,204,59,216,220,78,79,229,245,49,72,124,63,20,242,56,146,234,93,109,222,253,106,170,41,189,79,145,55,155,68,157,227,235,170,37,200,195,37,197,168,15,95,32,160,62,229,117,153,240,7,108,47,34,7]]},{"fee":10000,"postBalances":[499974522500,27881760,1,1,1],"preBalances":[499974532500,27881760,1,1,1],"status":{"Ok":null}}]]},"id":1}

After: (edited after 265803d)
Json:

{"jsonrpc":"2.0","result":{"blockhash":"Gp3t5bfDsJv1ovP8cB1SuRhXVuoTqDv7p3tymyubYg5","parentSlot":1,"previousBlockhash":"EFejToxii1L5aUF2NrK9dsbAEmZSNyN5nsipmZHQR1eA","transactions":[[{"message":{"accountKeys":["6H94zdiaYfRfPfKjYLjyr2VFBg6JHXygy84r3qhc3NsC","39UAy8hsoYPywGPGdmun747omSr79zLSjqvPJN3zetoH","SysvarS1otHashes111111111111111111111111111","SysvarC1ock11111111111111111111111111111111","Vote111111111111111111111111111111111111111"],"header":{"numReadonlySignedAccounts":0,"numReadonlyUnsignedAccounts":3,"numRequiredSignatures":2},"instructions":[{"accounts":[1,2,3],"data":"29z5mr1JoRmJYQ6ynmk3pf31cGFRziAF1M3mT3L6sFXf5cKLdkEaMXMT8AqLpD4CpcupHmuMEmtZHpomrwfdZetSomNy3d","programIdIndex":4}],"recentBlockhash":"EFejToxii1L5aUF2NrK9dsbAEmZSNyN5nsipmZHQR1eA"},"signatures":["35YGay1Lwjwgxe9zaH6APSHbt9gYQUCtBWTNL3aVwVGn9xTFw2fgds7qK5AL29mP63A9j3rh8KpN1TgSR62XCaby","4vANMjSKiwEchGSXwVrQkwHnmsbKQmy9vdrsYxWdCup1bLsFzX8gKrFTSVDCZCae2dbxJB9mPNhqB2sD1vvr4sAD"]},{"fee":18000,"postBalances":[499999972500,15298080,1,1,1],"preBalances":[499999990500,15298080,1,1,1],"status":{"Ok":null}}]]},"id":1}

Binary:

{"jsonrpc":"2.0","result":{"blockhash":"Gp3t5bfDsJv1ovP8cB1SuRhXVuoTqDv7p3tymyubYg5","parentSlot":1,"previousBlockhash":"EFejToxii1L5aUF2NrK9dsbAEmZSNyN5nsipmZHQR1eA","transactions":[["81UZJt4dh4Do66jDhrgkQudS8J2N6iG3jaVav7gJrqJSFY4Ug53iA9JFJZh2gxKWcaFdLJwhHx9mRdg9JwDAWB4ywiu5154CRwXV4FMdnPLg7bhxRLwhhYaLsVgMF5AyNRcTzjCVoBvqFgDU7P8VEKDEiMvD3qxzm1pLZVxDG1LTQpT3Dz4Uviv4KQbFQNuC22KupBoyHFB7Zh6KFdMqux4M9PvhoqcoJsJKwXjWpKu7xmEKnnrSbfLadkgjBmmjhW3fdTrFvnhQdTkhtdJxUL1xS9GMuJQer8YgSKNtUXB1eXZQwXU8bU2BjYkZE6Q5Xww8hu9Z4E4Mo4QsooVtHoP6BM3NKw8zjVbWfoCQqxTrwuSzrNCWCWt58C24LHecH67CTt2uXbYSviixvrYkK7A3t68BxTJcF1dXJitEPTFe2ceTkauLJqrJgnER4iUrsjr26T8YgWvpY9wkkWFSviQW6wV5RASTCUasVEcrDiaKj8EQMkgyDoe9HyKitSVg67vMWJFpUXpQobseWJUs5FTWWzmfHmFp8FZ",{"fee":18000,"postBalances":[499999972500,15298080,1,1,1],"preBalances":[499999990500,15298080,1,1,1],"status":{"Ok":null}}]]},"id":1}

@CriesofCarrots CriesofCarrots requested a review from mvines January 10, 2020 20:42
@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #7756 into master will increase coverage by <.1%.
The diff coverage is 91.4%.

@@           Coverage Diff            @@
##           master   #7756     +/-   ##
========================================
+ Coverage    81.8%   81.8%   +<.1%     
========================================
  Files         241     241             
  Lines       50903   50988     +85     
========================================
+ Hits        41652   41726     +74     
- Misses       9251    9262     +11

@mvines
Copy link
Member

mvines commented Jan 10, 2020

Better!

The instruction data for the JSON output is still yucky:
2,0,0,0,2,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,41,53,227,177,134,29,112,126,6,84,65,211,71,229,173,93,204,20,54,218,220,22,110,122,71,130,231,169,168,227,113,131,1,234,209,23,94,0,0,0,0

base58 encoding this would be super too (not attached to base58 but it's the tool we have at hand).

For the "binary" output, all those byte arrays could be base58ed too. wdyt?

@CriesofCarrots
Copy link
Contributor Author

base58 encoding this would be super too (not attached to base58 but it's the tool we have at hand).

For the "binary" output, all those byte arrays could be base58ed too. wdyt?

Base-58 encode all the things!
Yeah, that's a good thought, as it decreases the size even further, and it makes handling the Hashes a little cleaner.

You okay with this approach of lots of Rpc formatting-specific structs?

@mvines
Copy link
Member

mvines commented Jan 10, 2020

You okay with this approach of lots of Rpc formatting-specific structs?

I don't see a better alternative, so yep! I guess it helps us keep the RPC interface more stable, it won't just change if some other random part of the tree changes

@CriesofCarrots
Copy link
Contributor Author

CriesofCarrots commented Jan 10, 2020

I guess it helps us keep the RPC interface more stable, it won't just change if some other random part of the tree changes

...at least this part of the RPC interface. Perhaps I'll consider whether other methods would benefit from being normalized this way.

@mvines
Copy link
Member

mvines commented Jan 10, 2020

Once we start locking down the ABI (#7738), we'll see much less RPC churn as well

@CriesofCarrots CriesofCarrots force-pushed the gcb-encoding branch 2 times, most recently from 37032a2 to 4d81d66 Compare January 10, 2020 23:12
@CriesofCarrots
Copy link
Contributor Author

@mvines , fixed up with all the base 58 (updated examples above). Look okay?

mvines
mvines previously approved these changes Jan 13, 2020
Copy link
Member

@mvines mvines left a comment

Choose a reason for hiding this comment

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

lgtm, with json rpc api doc updates

@mergify mergify bot dismissed mvines’s stale review January 13, 2020 04:56

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Jan 13, 2020
@solana-grimes solana-grimes merged commit a17d579 into solana-labs:master Jan 13, 2020
mergify bot pushed a commit that referenced this pull request Jan 13, 2020
sakridge pushed a commit to sakridge/solana that referenced this pull request Jan 14, 2020
@CriesofCarrots CriesofCarrots deleted the gcb-encoding branch February 21, 2020 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants