Opened 14 months ago

Closed 5 months ago

#23107 closed defect (fixed)

prop224: Optimize hs_circ_service_get_intro_circ() digest calculation

Reported by: asn Owned by: neel
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, prop224-extra, tor-hs, optimization, 032-unreached
Cc: neel@… Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

Our prop224 function for getting an intro circ given its intro object re-calculates the relay identity fpr all the time:

  if (ip->base.is_only_legacy) {
    uint8_t digest[DIGEST_LEN];
    if (BUG(crypto_pk_get_digest(ip->legacy_key, (char *) digest) < 0)) {
      goto end;
    }
    circ = hs_circuitmap_get_intro_circ_v2_service_side(digest);

We could shove that in the hs_service_intro_point_t object as well to cut some digest calculations.

Child Tickets

Attachments (2)

b23107-r001.patch (3.4 KB) - added by neel 5 months ago.
Patch (Revision 1)
b23107-002.patch (3.4 KB) - added by neel 5 months ago.
Patch (Revision 2)

Download all attachments as: .zip

Change History (14)

comment:1 Changed 12 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

comment:2 Changed 5 months ago by neel

Cc: neel@… added

comment:3 Changed 5 months ago by neel

Owner: set to neel
Status: newassigned

Changed 5 months ago by neel

Attachment: b23107-r001.patch added

Patch (Revision 1)

comment:4 Changed 5 months ago by neel

I have created a patch for this patch under the filename b23107-r001.patch. I have also used the cached digest in register_intro_circ().

comment:5 Changed 5 months ago by dgoulet

Status: assignedneeds_review

comment:6 Changed 5 months ago by dgoulet

Points: 0.2
Reviewer: dgoulet
Sponsor: SponsorR-can
Status: needs_reviewneeds_revision

Thanks neel!

Couple of things.

  1. We should set the digest right away when creating the legacy key instead of this:

memset(ip->digest, 0, sizeof(ip->digest));

  1. We don't need to check for mem is zero when using the legacy IP because if is_only_legacy is set, then digest + legacy_key MUST be set. Actually, considering that we set the digest when an IP is allocated, we don't need this whole if().
  1. I would rename the digest to maybe something like legacy_key_digest because there are so many keys in that object that we should just be precise about what digest that is.

Huge thanks!

Changed 5 months ago by neel

Attachment: b23107-002.patch added

Patch (Revision 2)

comment:7 Changed 5 months ago by neel

I have a new patch. The filename is b23107-002.patch.

comment:8 Changed 5 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Status: needs_revisionneeds_review

comment:9 Changed 5 months ago by dgoulet

Reviewer: dgouletasn

comment:10 Changed 5 months ago by asn

Patch looks good to me.

Please see my branch bug23107 which is neel's patch, with a smaller commit msg, and a tiny amendment to the changes file so that make check-changes does not complain. Here is a PR too: https://github.com/torproject/tor/pull/76

Neel thanks for all the code! BTW, since you are a regular contributor now, you should consider forking the torproject tor repo on github and sending us branch names (or PRs) on the tickets instead of attaching .patch files. It's a smoother development procedure :) Thanks again! :)

comment:11 Changed 5 months ago by asn

Status: needs_reviewmerge_ready

comment:12 Changed 5 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

lgtm; I pushed an extra commit to fix an extra white-space left in the header. No biggy!

Merged. Thanks Neel/asn!

Note: See TracTickets for help on using tickets.