[Docs] AAT related comments & documentation improvements #1598

Olshansk · 2024-02-07T05:08:38Z

This PR is the result of reviewing the codebase to answer AAT-related questions
for the backend & infra teams at grove. As a result, the following changes were made:

Reformat (automatically) a handful of files using goimports-reviser
Update comments in some parts of the source code related to AATs
Update the documentation related to generating and using AATs
Build a reference upon which the foundation will update the official documentation at pokt-network/docs-v2

General quality-of-life improvements for documentation, comments and code health.

app/util.go

adshmh

Thank you for sharing the PR, it is very helpful in showing how AATs work.
One extra suggestion: would be great if we could reference several of the files that are touched here in a list of docs/code to look at to learn how the relays and proofs work.

doc/specs/application-auth-token.md

x/pocketcore/keeper/aat.go

commoddity

Agree with @adshmh here; this is great improvement in clarity and providing context. Thanks a lot.

Olshansk · 2024-02-13T00:42:40Z

@adshmh

One extra suggestion: would be great if we could reference several of the files that are touched here in a list of docs/code to look at to learn how the relays and proofs work.

Would you be open to tackle this one yourself?

msmania

I think the main purpose of this PR is to add description about AAT rather than general documentation improvement. Can you update the PR description?

This PR contains redundant descriptions. Can you put the most detailed description in .md file under doc/specs or in https://github.com/pokt-network/docs-v2, and make other places refer to it? Code comments should be kept simple.
I see some comments are not necessary because they're just rephrasing a code.
Regarding AAT, I know PNF or Grove are introducing a new term "Gateway" to call what is used to be called "Client". Having a comment like "(a.k.a gatewayPubKey)" everywhere is confusing. In v0 code, can we just keep using clientPubKey? Since it's defined as clientPubKey in the code, it's just clientPubKey.
You emphasize "appPubKey and clientPubKey may or may not be the same", but I think it's not that important. It's ok to say it in application-auth-token.md as a note, but not as code comments.

app/cmd/cli/app.go

app/cmd/rpc/client.go

app/query.go

x/pocketcore/types/crypto.go

x/pocketcore/types/proof.go

doc/specs/application-auth-token.md

app/cmd/cli/app.go

x/pocketcore/types/proof.go

x/pocketcore/types/crypto.go

x/pocketcore/types/aat.go

x/pocketcore/keeper/aat.go

app/cmd/cli/app.go

POKT-Discourse · 2024-02-22T20:12:20Z

This pull request has been mentioned on Pocket Network Forum. There might be relevant details there:

https://forum.pokt.network/t/block-sizes-claims-and-proofs-in-the-multi-gateway-era/5060/9

Olshansk · 2024-03-04T22:36:25Z

@msmania @kutoft @ezeike @nodiesBlade @commoddity @adshmh Wanted to bump this PR. PTAL when you have a chance.

app/cmd/cli/app.go

app/util.go

doc/specs/application-auth-token.md

doc/specs/cli/apps.md

x/pocketcore/keeper/proof.go

app/util.go

x/apps/keeper/querier_test.go

doc/specs/application-auth-token.md

msmania · 2024-03-09T21:23:15Z

doc/specs/application-auth-token.md

+decomposed into two options:
+
+1. **Application Key != Client Key ** - Recommended for all Gateways.
+2. **Application Key == Client Key** - Recommended for independent Applications that are not Gateways.


This part is still confusing to me. Having "Recommended for all Gateways" and "Recommended for independent Applications that are not Gateways" in the same list doesn't sound right. The first item says "for gateways" while the second item says "for Applications", but these are two different concepts. Application is not Gateway.

I think it's fair to say we create AAT for gateways because a gateway (aka client, relayer) is a program to use AAT. Then the second case confuses me. Do you mean there is a situation where we can use AAT without any gateways? Can you elaborate that case a little bit more?

You should watch my talk! https://www.youtube.com/watch?v=jVW-3lRzVT0&t=1s

Docs also updated.

I added this:

_Note: Please note that option 2 is only shown for theoretical purposes. Due to the scaling limitations of Morse (v0) implemented in `pocket-core`, Gateways have to be used. The original design / intention of Pocket, and how Shannon (v1) will operate is support both modes._

If this is not enough and you want me to write out the whole story, I'd rather just delete these updates ot the docs altogether.

Lmk what you prefer.

Hmm, looks like we're not yet on the same page. Before just stepping back, let's discuss it more. I think it's important. If I miss something, I want to know it.

First of all, I watched https://www.youtube.com/watch?v=jVW-3lRzVT0. That was a great talk!

I watched "Sovereign Application" part at around 15:00 carefully and if I understand that part correctly, the Sovereign Application scenario still requires a gateway, right? As you said "they talk to the suppliers themselves" in the talk, they have their own program to submit relays, which is a gateway.

I guess you're calling only a delegated client "gateway". If so, "gateway" is just one usecase of "client" and we can't say "client, aka gateway". Whether it's delegated or not, a client is a program to submit relays. Actually I personally prefer calling it "relayer".

Anyway, the question is if "Sovereign Application" operator doesn't delegate the relayer part to anyone and opereates it themselves, is it recommended to use the same key for the Gateway and App? My answer is no.

To me, this question is almost the same as "can we use the same password for multiple services, like Amazon.com account and Google account?". The benefit from doing that is we can reduce the number of passwords to be managed, but of course the answer is "we can, but it's not recommended because it sacrifices security".

I think Application/Gateway has the same story.

In whatever scenario, a gateway program needs to access client private key to send a request. It needs to load the key in a raw format at some point. Access to application private key, on the other hand, is not required if they precreate an AAT. And if they do so, even if their server is hacked, application private key is safe. Good. If they use the same key for Client and Application, application private key is leaked when their client is hacked, and the staked tokens will be at stake really.

Maybe I'm getting paranoid to this. Probably the server will not be hacked. Managing less private keys is more beneficial. But if you recommend something, you need to clarify pros and cons, and explain why. Otherwise people cannot choose which fits their system "Application Key != Client Key" or "Application Key == Client Key" .

The goal is to deprecate pocket-core and focus on poktroll, so I don't want to make a big deal out of this. This was a "side quest".

How about I just delete this part of the documentation to avoid confusion and paranoia?
👍?

We discussed in Discord and agreed:

We should not bring Shannon concepts to this repo

Client in v0 is not Gateway in Shannon

Client pubkey may be the same as App pubkey, but it's not recommended in v0

msmania · 2024-03-09T21:23:54Z

doc/specs/cli/apps.md

@@ -61,38 +61,31 @@ Transaction submitted with hash: <Transaction Hash>
 pocket apps create-aat <appAddr> <clientPubKey>
 ```

-Creates a signed application authentication token \(version `0.0.1` of the AAT spec\), that can be embedded into
-application software for Relay servicing. Will prompt the user for the `<appAddr>` account passphrase.
+This CLI is hard-coded to generate an AAT with spec version 0.0.1.


Please update this to match the actual command output.

This was the output. How should I update the docs?

pocket apps create-aat 0551ce49b10bd603c85b8de89434686efdbb9c7a aaa 2024/03/11 19:47:48 Initializing Pocket Datadir 2024/03/11 19:47:48 datadir = /Users/olshansky/.pocket Enter passphrase: { "version": "0.0.1", "app_pub_key": "1789ebc8b3a37ebb07b6a9bd8716f34a67004c61f2120a8c1f76560474d8e4f0", "client_pub_key": "aaa", "signature": "8f07d21bc90fe571fcc326d0ba281eb71f61ddc781abe8f125ebe27ea74a3a55c09f7e4c19744dc7488b362a022e767b0d4e57965c0461dfc97f11c0996d2101" }

I mean the output of pocket apps create-aat --help. We updated the text in app/cmd/cli/app.go, so this needs to match it.

## Description `Uncompressed Inline` section's `tar` command had a flag for gzip files even though that archive is uncompressed. This changes it to `tar -xv` instead so it doesn't error out. reviewpad:summary

msmania · 2024-03-12T05:57:58Z

doc/specs/cli/apps.md

@@ -61,38 +61,31 @@ Transaction submitted with hash: <Transaction Hash>
 pocket apps create-aat <appAddr> <clientPubKey>
 ```

-Creates a signed application authentication token \(version `0.0.1` of the AAT spec\), that can be embedded into
-application software for Relay servicing. Will prompt the user for the `<appAddr>` account passphrase.
+This CLI is hard-coded to generate an AAT with spec version 0.0.1.


I mean the output of pocket apps create-aat --help. We updated the text in app/cmd/cli/app.go, so this needs to match it.

msmania · 2024-03-12T07:03:19Z

doc/specs/application-auth-token.md

+decomposed into two options:
+
+1. **Application Key != Client Key ** - Recommended for all Gateways.
+2. **Application Key == Client Key** - Recommended for independent Applications that are not Gateways.


Hmm, looks like we're not yet on the same page. Before just stepping back, let's discuss it more. I think it's important. If I miss something, I want to know it.

First of all, I watched https://www.youtube.com/watch?v=jVW-3lRzVT0. That was a great talk!

I watched "Sovereign Application" part at around 15:00 carefully and if I understand that part correctly, the Sovereign Application scenario still requires a gateway, right? As you said "they talk to the suppliers themselves" in the talk, they have their own program to submit relays, which is a gateway.

I guess you're calling only a delegated client "gateway". If so, "gateway" is just one usecase of "client" and we can't say "client, aka gateway". Whether it's delegated or not, a client is a program to submit relays. Actually I personally prefer calling it "relayer".

Anyway, the question is if "Sovereign Application" operator doesn't delegate the relayer part to anyone and opereates it themselves, is it recommended to use the same key for the Gateway and App? My answer is no.

To me, this question is almost the same as "can we use the same password for multiple services, like Amazon.com account and Google account?". The benefit from doing that is we can reduce the number of passwords to be managed, but of course the answer is "we can, but it's not recommended because it sacrifices security".

I think Application/Gateway has the same story.

In whatever scenario, a gateway program needs to access client private key to send a request. It needs to load the key in a raw format at some point. Access to application private key, on the other hand, is not required if they precreate an AAT. And if they do so, even if their server is hacked, application private key is safe. Good. If they use the same key for Client and Application, application private key is leaked when their client is hacked, and the staked tokens will be at stake really.

Maybe I'm getting paranoid to this. Probably the server will not be hacked. Managing less private keys is more beneficial. But if you recommend something, you need to clarify pros and cons, and explain why. Otherwise people cannot choose which fits their system "Application Key != Client Key" or "Application Key == Client Key" .

x/apps/keeper/querier_test.go

## Description reviewpad:summary

msmania

Sorry for taking long. Thanks!

WIP - Improve comments & naming

dc232f1

Olshansk requested review from msmania, kutoft and adshmh February 8, 2024 21:19

Olshansk changed the title ~~[DRAFT] Improve comments & naming~~ [Documentation] Improve comments & naming Feb 8, 2024

Olshansk self-assigned this Feb 8, 2024

Olshansk added the documentation label Feb 8, 2024

Olshansk marked this pull request as ready for review February 8, 2024 21:20

Olshansk requested review from ezeike and nodiesBlade February 9, 2024 18:13

adshmh reviewed Feb 12, 2024

View reviewed changes

app/util.go Outdated Show resolved Hide resolved

adshmh requested changes Feb 12, 2024

View reviewed changes

commoddity approved these changes Feb 12, 2024

View reviewed changes

Reply to a few review comments

4f13ce1

msmania requested changes Feb 14, 2024

View reviewed changes

Olshansk changed the title ~~[Documentation] Improve comments & naming~~ [Docs] AAT related comments & documentation improvements Feb 16, 2024

Olshansk added this to the Network Quality of Life milestone Feb 16, 2024

Olshansk added 2 commits February 16, 2024 14:25

Reply to some of the review comments

37d245b

Reply to all review comments

a22eb56

Olshansk requested a review from msmania February 16, 2024 22:36

Revert code changes

c5965fe

msmania requested changes Feb 21, 2024

View reviewed changes

Olshansk mentioned this pull request Feb 24, 2024

[Code Health] Remove sessionNodeCount from ValidateLocal in pprofs.go along with up/down stream related code paths #1599

Open

Olshansk requested review from commoddity, adshmh and msmania February 24, 2024 22:29

msmania requested changes Mar 6, 2024

View reviewed changes

Reply to PR review comments

0054870

Olshansk force-pushed the comment_and_name_improvements branch from 5c7c13b to 0054870 Compare March 7, 2024 22:18

Olshansk requested a review from msmania March 7, 2024 22:18

msmania requested changes Mar 9, 2024

View reviewed changes

0xThresh and others added 3 commits March 11, 2024 19:42

Fix Uncompressed Inline tar cmd (#1597)

ee408ab

## Description `Uncompressed Inline` section's `tar` command had a flag for gzip files even though that archive is uncompressed. This changes it to `tar -xv` instead so it doesn't error out. reviewpad:summary

applicaitons typo fix

5fcb117

Reply to PR review comments

b0f23ab

Olshansk requested a review from msmania March 12, 2024 02:53

msmania requested changes Mar 12, 2024

View reviewed changes

Remove "gateway" terms because it's a concept in Shannon (#1602)

37dd286

## Description reviewpad:summary

msmania approved these changes Mar 12, 2024

View reviewed changes

Olshansk merged commit 93592fa into staging Mar 13, 2024
4 checks passed

Olshansk deleted the comment_and_name_improvements branch March 13, 2024 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Docs] AAT related comments & documentation improvements #1598

[Docs] AAT related comments & documentation improvements #1598

Olshansk commented Feb 7, 2024 •

edited

Loading

adshmh left a comment

commoddity left a comment

Olshansk commented Feb 13, 2024 •

edited

Loading

msmania left a comment

POKT-Discourse commented Feb 22, 2024

Olshansk commented Mar 4, 2024

msmania Mar 9, 2024

Olshansk Mar 12, 2024

Olshansk Mar 12, 2024

msmania Mar 12, 2024

Olshansk Mar 12, 2024

msmania Mar 12, 2024

msmania Mar 9, 2024

Olshansk Mar 12, 2024

msmania Mar 12, 2024

msmania Mar 12, 2024

msmania Mar 12, 2024

msmania left a comment

[Docs] AAT related comments & documentation improvements #1598

[Docs] AAT related comments & documentation improvements #1598

Conversation

Olshansk commented Feb 7, 2024 • edited Loading

adshmh left a comment

Choose a reason for hiding this comment

commoddity left a comment

Choose a reason for hiding this comment

Olshansk commented Feb 13, 2024 • edited Loading

msmania left a comment

Choose a reason for hiding this comment

POKT-Discourse commented Feb 22, 2024

Olshansk commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msmania left a comment

Choose a reason for hiding this comment

Olshansk commented Feb 7, 2024 •

edited

Loading

Olshansk commented Feb 13, 2024 •

edited

Loading