Opened 7 days ago

Last modified 16 hours ago

#31684 needs_review task

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
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: 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

TicketStatusOwnerSummaryComponent
#31762newAdd GETINFO dir/status-vote/current/consensus-microdesc to the control specCore Tor/Tor

Change History (9)

comment:1 Changed 7 days ago by nickm

Status: newneeds_information

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

comment:2 Changed 7 days 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 29 hours ago by ltbringer

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

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

comment:4 Changed 28 hours ago by ltbringer

Status: newneeds_review

comment:5 in reply to:  3 Changed 24 hours 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 19 hours 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 17 hours 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 17 hours 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 17 hours ago by ltbringer (previous) (diff)

comment:9 Changed 16 hours ago by teor

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

Note: See TracTickets for help on using tickets.