search

TokTok / website

The TokTok website
https://toktok.ltd/
GNU General Public License v3.0
10 stars 54 forks source link

Create Tox_Quick_Name_Lookup_Specification.md #15

Closed GrayHatter closed 7 years ago

GrayHatter commented 7 years ago

This change is Reviewable

GrayHatter commented 7 years ago

@iphydf @robinlinden @cebe

cebe commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

toktok/tqnl.md, line 30 at r2 (raw file):

4. Relayed name resolution 

### Design

I suggest to rename this to "Technical Design" and provide a "Design" section that describes how this would look from a user point of view.

toktok/tqnl.md, line 33 at r2 (raw file):


- Tox-named will sit on top of the DHT API, and will use DHT.c functions along with the corresponding net_crypto.c and crypto_core.c to connect to servers provided by the user.  
- Users will call the tox_function() with the server lookup information, and the string to be queried.

what is "server lookup information"? what does it contain?

Comments from Reviewable

cebe commented 7 years ago

Reviewed 1 of 3 files at r2, 2 of 2 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

GrayHatter commented 7 years ago

@cebe assigned you

Zer0-One commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.

toktok/design/tqnl.md, line 10 at r4 (raw file):

## Goals

Currently toxcore supports ToxDNS. This is a service that sits on top of the historically insecure DNS system. With the idea in mind that toxcore should be a ‘security first’ project; using DNS as a backend/platform/service, is problematic. That said, while it’s true that knowing, using, and understanding the intent behind a ToxID will make your use of tox more secure, the primary connection to a friend being a ToxID is a non-starter for most users. An easy and human-readable name to ToxID resolution system/service is clearly required from the Tox Messenger. The primary goal of Tox-Named is replace ToxDNS with a more secure, and simpler to maintain/implement system. Secondly, we aim to provide an API that allows clients to quickly, simply, and securely (without the need to create or manage the security themselves) interface with servers of their choosing, without the need to use a 3rd party system/api.

toktok/design/tqnl.md, line 16 at r4 (raw file):

2. It must be able to connect to, and resolve any ‘name’ without leaking information (who’s the real info requester, the name of the ToxID searched, or the real ToxID itself).
3. Under expected/default configuration it must not expose the Long Term Key (LTK) of the user requesting information.
## Scope

Line break in-between the current paragraph and next heading

toktok/design/tqnl.md, line 24 at r4 (raw file):

In scope

1. Create and expose an API clients can use for toxcore to make and respond to string-to-ToxID queries.

should be "an API that clients can use"

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 30 at r2 (raw file):

Previously, cebe (Carsten Brandt) wrote…
I suggest to rename this to "Technical Design" and provide a "Design" section that describes how this would look from a user point of view. - How are name servers organized? - What happens if two nameservers provide resolution for the same name?

half done.

They're not organized yet Can't happen, only one server is queried

toktok/design/tqnl.md, line 33 at r2 (raw file):

Previously, cebe (Carsten Brandt) wrote…
what is "server lookup information"? what does it contain?

Done.

toktok/design/tqnl.md, line 10 at r4 (raw file):

Previously, Zer0-One (David Zero) wrote…
- Change "name to ToxID" to "name-to-ToxID" - Is it appropriate to use "Tox Messenger"? Maybe that should just be "Tox". - "Tox-Named" should probably be all lower-case, like the actual daemon name will be. If you want emphasis, italicize. - Choose whether "API" should be all caps or lower-case, don't mix.

done.

make this 4 comments next time pls?

toktok/design/tqnl.md, line 16 at r4 (raw file):

Previously, Zer0-One (David Zero) wrote…
Line break in-between the current paragraph and next heading

Done.

toktok/design/tqnl.md, line 24 at r4 (raw file):

Previously, Zer0-One (David Zero) wrote…
should be "an API that clients can use"

Done.

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r6. Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 10 at r4 (raw file):

make this 4 comments next time pls?

That's what you get for putting the whole paragraph on a single line.

toktok/design/tqnl.md, line 8 at r6 (raw file):

---

# Tox Quick Name Lookup

Add empty line after each heading.

toktok/design/tqnl.md, line 38 at r6 (raw file):

But the majority is out of the scope of this revision.

###In scope

Add space between ### and the heading text.

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 10 at r4 (raw file):

Previously, iphydf wrote…
> make this 4 comments next time pls? That's what you get for putting the whole paragraph on a single line.

yeah, yeah, yeah.... I know -_- that's fixed too

toktok/design/tqnl.md, line 8 at r6 (raw file):

Previously, iphydf wrote…
Add empty line after each heading.

Done.

toktok/design/tqnl.md, line 38 at r6 (raw file):

Previously, iphydf wrote…
Add space between `###` and the heading text.

Done.

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r7. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Comments from Reviewable

robinlinden commented 7 years ago

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 8 at r6 (raw file):

---

# Tox Quick Name Lookup

Remove this line. The title in the front matter will be added as a first heading.

toktok/design/tqnl.md, line 5 at r7 (raw file):

title: Tox Quick Name Lookup Spec
revision: 0
permalink: design/tqnl.html

Can you add a link to this file in the list of designs in design.md?

toktok/design/tqnl.md, line 49 at r7 (raw file):

### Not in scope

1. Distributed name resolution

We're using a markdown style checker that requires you to have 3 spaces after the list marker in ordered lists.

toktok/design/tqnl.md, line 57 at r7 (raw file):

## Technical Design

- TQNL will sit on top of the DHT API, and will use DHT.c functions along with

We're using a markdown style checker that requires you to have 3 spaces after the list marker in unordered lists.

Comments from Reviewable

robinlinden commented 7 years ago

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 49 at r7 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
We're using a markdown style checker that requires you to have 3 spaces after the list marker in ordered lists.

Per IRC, I shouldn't have copy-pasted this. It's 2 spaces after ordered list markers. 3 after unordered ones.

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 8 at r6 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Remove this line. The title in the front matter will be added as a first heading.

Done.

toktok/design/tqnl.md, line 5 at r7 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Can you add a link to this file in the list of designs in design.md?

Done.

toktok/design/tqnl.md, line 49 at r7 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Per IRC, I shouldn't have copy-pasted this. It's 2 spaces after ordered list markers. 3 after unordered ones.

Done.

toktok/design/tqnl.md, line 57 at r7 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
We're using a markdown style checker that requires you to have 3 spaces after the list marker in unordered lists.

Done.

Comments from Reviewable

robinlinden commented 7 years ago

The linter is still complaining, but it can be ignored. #26 fixes the linter settings so that it works correctly with our headings and this PR will pass after that's merged.

iphydf commented 7 years ago
:lgtm_strong:

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 2 of 2 files at r8. Review status: 1 of 2 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 32 at r8 (raw file):

        searched, or the real ToxID itself).
3.  Under expected/default configuration it must not expose the Long Term Key
        (LTK) of the user requesting information.

You're not using this abbreviation anywhere. No need to declare it. Also, it should probably be "long term public key".

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r9. Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: 1 of 2 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 32 at r8 (raw file):

Previously, iphydf wrote…
You're not using this abbreviation anywhere. No need to declare it. Also, it should probably be "long term public key".

Done.

Comments from Reviewable

GrayHatter commented 7 years ago

@Zer0-One you're blocking

Zer0-One commented 7 years ago
:lgtm:

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r10. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

cebe commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 2 files at r8, 1 of 1 files at r10. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r11. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

toktok/design/tqnl.md, line 6 at r11 (raw file):

authors: grayhatter
first proposed: 2016-10-07
last revision: 2017-01-10

I'm not sure last revision is necessary or useful, but perhaps it doesn't harm either. It's just something we need to remember to update.

@robinlinden can we use this metadata and display it on the page itself? Also, are spaces allowed in metadata keys? Do we want spaces in metadata keys?

Comments from Reviewable

iphydf commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 4 at r11 (raw file):

layout: default
title: Tox Quick Name Lookup Spec
authors: grayhatter

Perhaps also add a "state" key that says one of DRAFT/REVIEW/ACCEPTED or something.

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 2 of 2 files at r8, 1 of 1 files at r11. Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 6 at r11 (raw file):

Previously, iphydf wrote…
I'm not sure last revision is necessary or useful, but perhaps it doesn't harm either. It's just something we need to remember to update. @robinlinden can we use this metadata and display it on the page itself? Also, are spaces allowed in metadata keys? Do we want spaces in metadata keys?

We can use this metadata by using {{ page.key }}. The L#11 could for example currently be replaced with Revision {{ page.revision }}. and it would look the same as it does now, but change together with the metadata.

We can't use spaces in the keys. We can use things like arrays if, for example, we have a design with multiple authors in the future.

toktok/design/tqnl.md, line 11 at r11 (raw file):

---

Revision 0.

Revision {{ page.revision }}.

toktok/design/tqnl.md, line 13 at r11 (raw file):

Revision 0.

## Goals

A change made in #26 means that you'll have to change all headings to have one fewer # symbol. Sorry. :(

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 13 at r11 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
A change made in #26 means that you'll have to change all headings to have one fewer `#` symbol. Sorry. :(

lol no... you guys are iterating too quickly for something I don't care about.... you guys are in charge now -_-

Comments from Reviewable

iphydf commented 7 years ago

Review status: 1 of 2 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 13 at r11 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
lol no... you guys are iterating too quickly for something I don't care about.... you guys are in charge now -_-

Done.

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r12. Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r12. Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

zetok commented 7 years ago

Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 3 at r12 (raw file):

---
layout: default
title: Tox Quick Name Lookup Spec

s/Spec/Specification/ ?

toktok/design/tqnl.md, line 3 at r12 (raw file):

---
layout: default
title: Tox Quick Name Lookup Spec

Why is the word Quick needed there?

Wouldn't going through DHT make it actually slow? And if so, wouldn't Slow be more suited there?

toktok/design/tqnl.md, line 23 at r12 (raw file):

and human-readable name-to-ToxID resolution system/service is clearly required
from the Tox Messenger. The primary goal of Tox Quick Name Lookup is replace
ToxDNS with a more secure, and simpler to maintain/implement system. Secondly,

There is already a replacement system in the place, and the solution doesn't depend on toxcore, and is used by every client except for µTox.

TQNL should aim to replace something that is currently widely used and agreed on, rather than trying to deprecate something that was deprecated by ToxMe long time ago.

toktok/design/tqnl.md, line 30 at r12 (raw file):

# Requirements

1.  The system must be able to resolve any byte-string to a ToxID.

What is a byte-string ? Definition should be included.

toktok/design/tqnl.md, line 39 at r12 (raw file):

# Scope

The scope of this document is only to cover the replacement for ToxDNS. Many

Why is the already existing toxdns replacement not evaluated?

toktok/design/tqnl.md, line 45 at r12 (raw file):

## In scope

1.  Create and expose an API that clients can use for Toxcore to make and

What is Toxcore here? toxcore lib and all the associated

toktok/design/tqnl.md, line 59 at r12 (raw file):


-   TQNL will sit on top of the DHT API, and will use DHT.c functions along
    with the corresponding `net_crypto.c` and `crypto_core.c` to connect to

Which functions exactly, and what they are supposed to do?

toktok/design/tqnl.md, line 64 at r12 (raw file):

    (IP or hostname, Port, Public Key), and the string to be queried.
-   Servers will be specified by a Domain name, or IP address, a port, and a
    public key.

Inconsistent naming – above the keywords start with upper-case?

Same goes for all the other occurrences.

toktok/design/tqnl.md, line 65 at r12 (raw file):

-   Servers will be specified by a Domain name, or IP address, a port, and a
    public key.
-   TQNL will connect to the server, deliver the query packet, and then store

What is in the packet?

toktok/design/tqnl.md, line 66 at r12 (raw file):

    public key.
-   TQNL will connect to the server, deliver the query packet, and then store
    the server + query information in an array.

Why does it have to be stored in an array?

toktok/design/tqnl.md, line 67 at r12 (raw file):

-   TQNL will connect to the server, deliver the query packet, and then store
    the server + query information in an array.
-   The query packet to the name-server will be encrypted with the DHT key,

Why does the DHT key need to be used?

Why can't a separate, temporary for name requests key be used?

toktok/design/tqnl.md, line 70 at r12 (raw file):

    and the server key.
-   The query packet to the name-server includes the DHT public key, the
    string to be queried, and a nonce.

How is that serialised? Or is that supposed to just be concatenated?

toktok/design/tqnl.md, line 72 at r12 (raw file):

    string to be queried, and a nonce.

The nonce exists to identify which packet the server is responding to, as well

What is a nonce? It should be defined.

toktok/design/tqnl.md, line 75 at r12 (raw file):

as prohibit replays, or pre-generation.

-   The server may or may not respond to the query.

Under what circumstances server wouldn't respond?

toktok/design/tqnl.md, line 77 at r12 (raw file):

-   The server may or may not respond to the query.
-   If the name-server does respond to a query, it’ll respond to the sending
    peer with the nonce, followed by either: a failure code, or a valid ToxID

What is the failure code?

Why is the server responding in plaintext?

toktok/design/tqnl.md, line 78 at r12 (raw file):

-   If the name-server does respond to a query, it’ll respond to the sending
    peer with the nonce, followed by either: a failure code, or a valid ToxID
-   The sending peer will select a timeout to resend query packets, and will

Since it is toxcore that is supposed to manage all things other than the address, port, PK, "selecting" a timeout also is its job. How it is selected, and from what values?

toktok/design/tqnl.md, line 79 at r12 (raw file):

    peer with the nonce, followed by either: a failure code, or a valid ToxID
-   The sending peer will select a timeout to resend query packets, and will
    try to resend the same query with a new nonce after the timeout.

What is the timeout?

toktok/design/tqnl.md, line 80 at r12 (raw file):

-   The sending peer will select a timeout to resend query packets, and will
    try to resend the same query with a new nonce after the timeout.
-   The sending peer will select a number of retries, and will send the query

Same as the timeout – what number, selected from which values?

toktok/design/tqnl.md, line 81 at r12 (raw file):

    try to resend the same query with a new nonce after the timeout.
-   The sending peer will select a number of retries, and will send the query
    packet that number of times plus 1 additional for the first attempt.

Why is adding 1 necessary after selecting number of retries?

toktok/design/tqnl.md, line 94 at r12 (raw file):


-   Expand API to allow for registrations, editing existing registrations, and
    deleting IDs

NB: already doable with ToxMe

toktok/design/tqnl.md, line 102 at r12 (raw file):

-   Support for restricted domain lookups. (Allow clients to restrict the
    servers queried for certain names. As of r0 the client MUST handle this
    internally)

missing:

Comments from Reviewable

fcore117 commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 2 files at r8, 1 of 1 files at r12. Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 4 at r11 (raw file):

Previously, iphydf wrote…
Perhaps also add a "state" key that says one of DRAFT/REVIEW/ACCEPTED or something.

Done.

toktok/design/tqnl.md, line 6 at r11 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
We can use this metadata by using `{{ page.key }}`. The L#11 could for example currently be replaced with `Revision {{ page.revision }}.` and it would look the same as it does now, but change together with the metadata. We can't use spaces in the keys. We can use things like arrays if, for example, we have a design with multiple authors in the future.

do I need to make a change here?

toktok/design/tqnl.md, line 11 at r11 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Revision {{ page.revision }}.

Done.

toktok/design/tqnl.md, line 3 at r12 (raw file):

Previously, zetok wrote…
s/Spec/Specification/ ?

Done.

toktok/design/tqnl.md, line 3 at r12 (raw file):

Previously, zetok wrote…
Why is the word `Quick` needed there? Wouldn't going through DHT make it actually slow? And if so, wouldn't `Slow` be more suited there?

No, I'm using quick, because first, it's faster than any other options, and 2nd because it's simple. There's no extra overhead, send request string, get response string.

toktok/design/tqnl.md, line 23 at r12 (raw file):

Previously, zetok wrote…
There is already a replacement system in the place, and the solution doesn't depend on toxcore, and is used by every client except for µTox. TQNL should aim to replace something that is currently widely used and agreed on, rather than trying to deprecate something that was deprecated by ToxMe long time ago.

I don't agree, do you have another different or better argument?

toktok/design/tqnl.md, line 30 at r12 (raw file):

Previously, zetok wrote…
What is a `byte-string` ? Definition should be included.

byte-string is self explanatory, It's a string of bytes.

toktok/design/tqnl.md, line 39 at r12 (raw file):

Previously, zetok wrote…
Why is the already existing toxdns replacement not evaluated?

Because that's not a part of toxcore, and doesn't interface in any way with toxcore.

E.g., I'm also not evaluating replacing libfilteraudio.

toktok/design/tqnl.md, line 45 at r12 (raw file):

Previously, zetok wrote…
What is `Toxcore` here? `toxcore` lib and all the associated

fixed

toktok/design/tqnl.md, line 59 at r12 (raw file):

Previously, zetok wrote…
Which functions exactly, and what they are supposed to do?

fixed

toktok/design/tqnl.md, line 64 at r12 (raw file):

Previously, zetok wrote…
Inconsistent naming – above the keywords start with upper-case? Same goes for all the other occurrences.

fixed on this line, and nearby

toktok/design/tqnl.md, line 65 at r12 (raw file):

Previously, zetok wrote…
What is in the packet?

added

toktok/design/tqnl.md, line 66 at r12 (raw file):

Previously, zetok wrote…
Why does it have to be stored in an array?

for re-queries, this is answered below too BTW

toktok/design/tqnl.md, line 67 at r12 (raw file):

Previously, zetok wrote…
Why does the DHT key need to be used? Why can't a separate, temporary for name requests key be used?

It can be, I chose this design because my original intent was to make the DHT do all of the name lookups, but because the reason for that is out of scope for this revision. There's no limiting reason we have to use the DHT key. In this case it does because it's more resource efficient to not generate a new key.

A have later plans to do lookups through the onion as well, in which case, using the DHT key will make it harder for metadata collection to differentiate from other DHT traffic.

toktok/design/tqnl.md, line 70 at r12 (raw file):

Previously, zetok wrote…
How is that serialised? Or is that supposed to just be concatenated?

Currently undefined.

toktok/design/tqnl.md, line 72 at r12 (raw file):

Previously, zetok wrote…
What is a nonce? It should be defined.

like byte-string, it's self evident what a nonce is https://www.google.com/search?q=nonce&ie=utf-8&oe=utf-8

toktok/design/tqnl.md, line 75 at r12 (raw file):

Previously, zetok wrote…
Under what circumstances server wouldn't respond?

Many, packet loss, overloaded server, if time() % 11777; then don't respond to queries

toktok/design/tqnl.md, line 77 at r12 (raw file):

Previously, zetok wrote…
What is the failure code? Why is the server responding in plaintext?

No failure code is implemented at this time.

Why do you think it's responding in plaintext?

toktok/design/tqnl.md, line 78 at r12 (raw file):

Previously, zetok wrote…
Since it is toxcore that is supposed to manage all things other than the `address, port, PK`, "selecting" a timeout also is its job. How it is selected, and from what values?

fixed.

toktok/design/tqnl.md, line 79 at r12 (raw file):

Previously, zetok wrote…
What is the timeout?

I haven't picked a number yet, but I plan to select it by fair dice roll. http://xkcd.com/221/

toktok/design/tqnl.md, line 80 at r12 (raw file):

Previously, zetok wrote…
Same as the timeout – what number, selected from which values?

fixed

toktok/design/tqnl.md, line 81 at r12 (raw file):

Previously, zetok wrote…
Why is adding 1 necessary after selecting number of retries?

This is just to clarify the total of packets sent, from the number selected for retries.

toktok/design/tqnl.md, line 94 at r12 (raw file):

Previously, zetok wrote…
NB: already doable with ToxMe

I'm aware

toktok/design/tqnl.md, line 102 at r12 (raw file):

Previously, zetok wrote…
missing: - server-side info regarding, but not limited to storage of data

why?

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r13. Review status: all files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 6 at r11 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
do I need to make a change here?

You can't use spaces in the keys. Replace them with underscores.

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 6 at r11 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
You can't use spaces in the keys. Replace them with underscores.

Done.

Comments from Reviewable

GrayHatter commented 7 years ago

push isn't working

diff --git a/toktok/design/tqnl.md b/toktok/design/tqnl.md
index d736721..ec47d35 100644
--- a/toktok/design/tqnl.md
+++ b/toktok/design/tqnl.md
@@ -2,8 +2,8 @@
 layout: default
 title: Tox Quick Name Lookup Specification
 authors: grayhatter
-first proposed: 2016-10-07
-last revision: 2017-01-13
+first_proposed: 2016-10-07
+last_revision: 2017-01-13
 state: REVIEW
 revision: 1
 permalink: design/tqnl.html
iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r13. Review status: 1 of 2 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

Comments from Reviewable

Zer0-One commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 2 files at r8, 1 of 1 files at r14. Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

Comments from Reviewable

zetok commented 7 years ago

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 3 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
No, I'm using quick, because first, it's faster than any other options, and 2nd because it's simple. There's no extra overhead, send request string, get response string.

Oh, do you have any benchmark data?

Regarding it being simple, Tox Simple Name Lookup ? TSNL → THE SNAIL :D

toktok/design/tqnl.md, line 30 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
`byte-string` is self explanatory, It's a string of bytes.

So using UTF-16 is perfectly fine?

toktok/design/tqnl.md, line 39 at r12 (raw file):

Because that's not a part of toxcore, and doesn't interface in any way with toxcore.

No, name lookup is something that clients use and interact with. Why would toxcore be needed for it?

E.g., I'm also not evaluating replacing libfilteraudio.

What is this example supposed to show?

toktok/design/tqnl.md, line 67 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
It can be, I chose this design because my original intent was to make the DHT do all of the name lookups, but because the reason for that is out of scope for this revision. There's no limiting reason we have to use the DHT key. In this case it does because it's more resource efficient to not generate a new key. A have later plans to do lookups through the onion as well, in which case, using the DHT key will make it harder for metadata collection to differentiate from other DHT traffic.

That info should be in the spec.

toktok/design/tqnl.md, line 70 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
Currently undefined.

Should be defined.

toktok/design/tqnl.md, line 72 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
like byte-string, it's self evident what a nonce is https://www.google.com/search?q=nonce&ie=utf-8&oe=utf-8

So one can use 2-bytes long nonce for it?

toktok/design/tqnl.md, line 77 at r12 (raw file):

No failure code is implemented at this time.

This is spec, not the implementation.

Why do you think it's responding in plaintext?

That's what is specified.

toktok/design/tqnl.md, line 80 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
fixed

How is network quality evaluated?

toktok/design/tqnl.md, line 81 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
This is just to clarify the total of packets sent, from the number selected for retries.

It doesn't clarify it.

toktok/design/tqnl.md, line 94 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
I'm aware

Spec is not aware.

toktok/design/tqnl.md, line 69 at r14 (raw file):

    the server + query information in an array.
-   The packet will consist of the nonce, and the string the client wishes to
    search for.

This being concatenated in order to work?

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 30 at r12 (raw file):

Previously, zetok wrote…
So using UTF-16 is perfectly fine?

Sure, it's a string of bytes. Per the IRC discussion, it'll use the same def as the toxcore spec. An array of octets

toktok/design/tqnl.md, line 39 at r12 (raw file):

What is this example supposed to show?

it's another 3rd party plugin that's not relevant.

toktok/design/tqnl.md, line 66 at r12 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
for re-queries, this is answered below too BTW

The reason for this is answered below

toktok/design/tqnl.md, line 67 at r12 (raw file):

Previously, zetok wrote…
That info should be in the spec.

Will add a rational section in the next revision

toktok/design/tqnl.md, line 70 at r12 (raw file):

Previously, zetok wrote…
Should be defined.

will be in the next revision

toktok/design/tqnl.md, line 72 at r12 (raw file):

Previously, zetok wrote…
So one can use 2-bytes long nonce for it?

sure

toktok/design/tqnl.md, line 77 at r12 (raw file):

Previously, zetok wrote…
>No failure code is implemented at this time. This is spec, not the implementation. >Why do you think it's responding in plaintext? That's what is specified.

I don't think I said it was plaintext.

Either way, tqnl responds the same way tox does

toktok/design/tqnl.md, line 80 at r12 (raw file):

Previously, zetok wrote…
How is network quality evaluated?

That would be up to toxcore, it'll need tox api support

toktok/design/tqnl.md, line 94 at r12 (raw file):

Previously, zetok wrote…
Spec is not aware.

that's because this spec has nothing to do with toxme

Comments from Reviewable

GrayHatter commented 7 years ago

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

toktok/design/tqnl.md, line 3 at r12 (raw file):

Previously, zetok wrote…
Oh, do you have any benchmark data? Regarding it being simple, `Tox Simple Name Lookup` ? `TSNL → THE SNAIL` :D

Nope, not yet.

toktok/design/tqnl.md, line 69 at r14 (raw file):

Previously, zetok wrote…
This being concatenated in order to work?

yes

Comments from Reviewable

cebe commented 7 years ago
:lgtm_strong:

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r14. Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 3 files at r2, 1 of 2 files at r3, 1 of 1 files at r14. Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

Comments from Reviewable