Opened 3 months ago

Closed 7 weeks ago

#31684 closed enhancement (fixed)

Add control port GETINFO support for dumping the local consensus

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: control-port easy extra-review
Cc: Actual Points: 0.5
Parent ID: Points: 1
Reviewer: asn Sponsor:

Description

We have GETINFO ns/all but that only dumps the routerstatus documents from the consensus and not the entire consensus. There is lots of good information in the consensus headers/footers that people are interested in and it should be exposed to the control port.

(This will also need a spec patch)

Child Tickets

TicketTypeStatusOwnerSummary
#31762taskclosedAdd GETINFO dir/status-vote/current/consensus-microdesc to the control spec

Change History (30)

comment:1 Changed 3 months ago by nickm

Status: newneeds_information

I think this is already implemented as GETINFO dir/status-vote/current/consensus?

comment:2 Changed 3 months ago by nickm

Status: needs_informationnew

Whoops! dir/status-vote/current/consensus works for an NS consensus, but not for a microdesc consensus.

comment:3 Changed 3 months ago by ltbringer

https://github.com/torproject/tor/pull/1328

This adds GETINFO dir/status-vote/microdesc/consensus

comment:4 Changed 3 months ago by ltbringer

Status: newneeds_review

comment:5 in reply to:  3 Changed 3 months ago by teor

Hi, thanks for this pull request!

Replying to ltbringer:

https://github.com/torproject/tor/pull/1328

This adds GETINFO dir/status-vote/microdesc/consensus

How did you come up with this GETINFO name?
If we want the control spec to match the directory spec, it should be:

GETINFO dir/status-vote/current/consensus-microdesc

See https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n3895

This ticket creates a small reduction in code quality, because it turns a 300-line function into a 320-line function. If you would like to improve code quality, please add a commit that splits the two dir/status-vote/* GETINFOs into their own static function. Otherwise, our reviewer can do that for you, before they approve your pull request.

We also need to update the control-spec, I'll open a child ticket.

(We could do #28982 instead of this ticket, but that's a much more complex refactor.)

comment:6 Changed 3 months ago by ltbringer

This ticket creates a small reduction in code quality, because it turns a 300-line function into a 320-line function. If you would like to improve code quality, please add a commit that splits the two dir/status-vote/* GETINFOs into their own static function. Otherwise, our reviewer can do that for you, before they approve your pull request.

I will create the static function. I feel the entire function could use some help as I see the if-else blocks having some degree of similarity (my commit creates almost identical blocks, which differ only by a string argument).

Also, I was unaware of the spec. I'll fix that too.

comment:7 Changed 3 months ago by teor

Yes, the whole function could be turned in to a lookup table.

Hopefully we'll do that in #28982.

comment:8 Changed 3 months ago by ltbringer

https://github.com/torproject/tor/pull/1328 is updated with the function that switches between the two dir/status-vote/*

and the name follows the convention
GETINFO dir/status-vote/current/microdesc/consensus
to
GETINFO dir/status-vote/current/consensus-microdesc

Last edited 3 months ago by ltbringer (previous) (diff)

comment:9 Changed 3 months ago by teor

Thanks! We'll get someone to review this change in the next week or so.

comment:10 Changed 3 months ago by asn

Reviewer: asn

comment:11 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

OK I finished a code review here. And left a few comments on the PR. Marking as needs_revision for now!

Also can you tell me how you tested this new feature?

Thanks for all the code!

Last edited 2 months ago by asn (previous) (diff)

comment:12 Changed 2 months ago by ltbringer

You are welcome :)
I'll work on the comments in the PR.

I used the python stem client to test this feature.

comment:13 Changed 2 months ago by ltbringer

Status: needs_revisionneeds_review

comment:14 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

Closer! Some more work on the unittest would be great!

comment:15 Changed 2 months ago by ltbringer

I created a mock of networkstatus_map_cached_consensus and make it return a simulated tor_mmap_t object. Please review and let me know if that works.

comment:16 Changed 2 months ago by ltbringer

Status: needs_revisionneeds_review

comment:17 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

Yes the unittest looks much better now! There are still a few things that need to be ironed out, and also the Appveyor CI fails. But we are close! :)

comment:18 Changed 2 months ago by ltbringer

Status: needs_revisionneeds_review

comment:19 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

Thanks for all the work. We are moving closer!

comment:20 Changed 2 months ago by ltbringer

Status: needs_revisionneeds_review
  1. function declaration and definition are not separated within tests.
  2. unmocked networkstatus_get_cache_fname and we_want_to_fetch_flavor.
Last edited 2 months ago by ltbringer (previous) (diff)

comment:21 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

This seems pretty much ready, but you should use networkstatus_get_flavor_name() instead of making a new function for that.

comment:22 Changed 2 months ago by ltbringer

Status: needs_revisionneeds_review

comment:23 Changed 2 months ago by asn

Keywords: extra-review added

Thanks. Two more minor issues and we are done.

I also tested that this works fine with a stem script that looks like this:

 controller = Controller.from_port(port=9090)
 controller.authenticate()
 ns = controller.get_info("dir/status-vote/current/consensus-microdesc")
 print("microdesc: %s" % ns)
 ns = controller.get_info("dir/status-vote/current/consensus")
 print("ns: %s" % ns)

After you change these two minor issues please mark it as merge_ready.

Marking as extra review so that the merger checks that the issues have been fixed before merging (I will be away for two weeks)

Thanks again for the code!

comment:24 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

comment:25 Changed 2 months ago by ltbringer

Status: needs_revisionmerge_ready

comment:26 Changed 7 weeks ago by teor

Keywords: nickm-merge dgoulet-merge added

comment:27 Changed 7 weeks ago by teor

Actual Points: 0.5
Keywords: nickm-merge dgoulet-merge removed
Type: taskenhancement

I did some fixes:

  • standard whitespace
  • typos
  • standard error handling
  • remove unnecessary test code

Here is the branch with all the fixes:
https://github.com/teor2345/tor/commits/ticket31684
There is a description of each fix in my review on PR 1328.

Here is the squashed PR for merging:

I squashed the whitespace fixes with the rest of the code, to minimise the diff.

Since these are obvious fixes on already reviewed code, any maintainer can merge after CI passes.

comment:28 Changed 7 weeks ago by teor

I also tested the new GETINFO with tor-prompt.

comment:29 Changed 7 weeks ago by teor

The torspec PR is https://github.com/torproject/torspec/pull/92
They should merge at the same time.

comment:30 Changed 7 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to master.

Note: See TracTickets for help on using tickets.