Opened 10 months ago

Closed 8 months ago

#23760 closed enhancement (implemented)

Use node_get_curve25519_onion_key() in extend_info_from_node()

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactor, easy, review-group-26
Cc: neel@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by teor)

In #23577, we are going to implement a node_get_curve25519_onion_key() function. We can use this in extend_info_from_node() if we want to.

Child Tickets

Attachments (3)

001-node_get_extend_info.patch (1.5 KB) - added by neel 8 months ago.
Patch to introduce node_get_curve25519_onion_key() in extend_info_from_node()
002-node_get_extend_info-r1.patch (1.6 KB) - added by neel 8 months ago.
Patch to introduce node_get_curve25519_onion_key() in extend_info_from_node() (Revision 1)
003-node_get_extend_info-r2.patch (2.1 KB) - added by neel 8 months ago.
Patch to introduce node_get_curve25519_onion_key() in extend_info_from_node() (Revision 2)

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 months ago by nickm

Maybe call it node_get_ntor_key() instead?

comment:2 Changed 10 months ago by teor

If we do this, we should rename the existing node_has_curve25519_onion_key() to node_has_ntor_key().

comment:3 Changed 10 months ago by nickm

ntor_key, ntor_onion_key, curve25519_ntor_key, and curve25519_onion_key are also good names for this key -- we just shouldn't call it curve25519_key (with no indication of its purpose).

comment:4 Changed 9 months ago by teor

Description: modified (diff)
Keywords: easy added
Summary: Use node_get_curve25519_key() in extend_info_from_node()Use node_get_curve25519_onion_key() in extend_info_from_node()

Modify the name for consistency with node_has_curve25519_onion_key().

comment:5 Changed 8 months ago by teor

Keywords: prop224 tor-hs single-onion ipv6 removed
Parent ID: #23577

Removing the parent and spurious keywords.

Changed 8 months ago by neel

Patch to introduce node_get_curve25519_onion_key() in extend_info_from_node()

comment:6 Changed 8 months ago by neel

I have a patch to resolve this. The filename is 001-node_get_extend_info.patch.

comment:7 Changed 8 months ago by teor

Status: newneeds_revision

Hi neel, thanks for the patch!

node_get_curve25519_onion_key() returns a const pointer, so the variable also has to be const. Otherwise, some compilers will complain.

The rest of the patch looks good!

Changed 8 months ago by neel

Patch to introduce node_get_curve25519_onion_key() in extend_info_from_node() (Revision 1)

comment:8 Changed 8 months ago by neel

I have an updated patch under the filename 002-node_get_extend_info-r1.patch which makes curve_pubkey a const.

comment:9 Changed 8 months ago by teor

Ok, looks good to me.

Do you want to write a changes file?
Any code changes get a changes file, even simple refactoring.

Also, we should check this compiles and passes the tests before we merge.

Changed 8 months ago by neel

Patch to introduce node_get_curve25519_onion_key() in extend_info_from_node() (Revision 2)

comment:10 Changed 8 months ago by neel

I have a new patch which includes a changes file. The filename is 003-node_get_extend_info-r2.patch.

comment:11 Changed 8 months ago by teor

Hi,

When I try to compile this, I get the following errors:

src/or/circuitbuild.c:2699:5: error: implicit declaration of function
      'node_get_curve25519_onion_key' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
    node_get_curve25519_onion_key(node);
    ^
src/or/circuitbuild.c:2699:5: note: did you mean
      'node_has_curve25519_onion_key'?
./src/or/nodelist.h:78:5: note: 'node_has_curve25519_onion_key' declared here
int node_has_curve25519_onion_key(const node_t *node);
    ^
src/or/circuitbuild.c:2698:34: error: incompatible integer to pointer conversion
      initializing 'const curve25519_public_key_t *' (aka 'const struct
      curve25519_public_key_t *') with an expression of type 'int'
      [-Werror,-Wint-conversion]
  const curve25519_public_key_t *curve_pubkey =
                                 ^

Please import the correct header for node_get_curve25519_onion_key(), or put a declaration in the correct header.

comment:12 Changed 8 months ago by teor

Status: needs_revisionmerge_ready

Oops, my master branch was about 1500 commits behind.
Not sure how that happened. Sorry for any confusion.

This is fine, it passes make check and make test-network.
Let's get it merged.

It's branch bug23760_nc in my github if you prefer branches.

comment:13 Changed 8 months ago by nickm

Keywords: review-group-26 added

Creating review-group-26.

comment:14 Changed 8 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging to master; thanks!

Note: See TracTickets for help on using tickets.