Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket20218 rebased #1670

Closed
wants to merge 8 commits into from
Closed

Conversation

Labels
None yet
Projects
None yet
4 participants
@nmathewson
Copy link
Contributor

@nmathewson nmathewson commented Jan 16, 2020

No description provided.

vnepveu and others added 3 commits Jan 16, 2020
- Check all fields that might change in a routerstatus
- Document the refactoring

Signed-off-by: Victor Nepveu <victor.nepveu@imt-atlantique.net>
@coveralls
Copy link

@coveralls coveralls commented Jan 16, 2020

Pull Request Test Coverage Report for Build 7807

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 63.335%

Totals Coverage Status
Change from base Build 7806: 0.02%
Covered Lines: 49966
Relevant Lines: 78892

💛 - Coveralls

rs.is_flagged_running = 1;
ASSERT_CHANGED();

// Isn't this obsolete?
Copy link
Contributor

@teor2345 teor2345 Jan 17, 2020

Named and Unnamed are no longer listed in the consensus (proposal 235).
But some of the client and authority code is still present:
https://gitweb.torproject.org/torspec.git/tree/proposals/282-remove-named-from-consensus.txt

If we want to remove this flag from client code, let's remove all instances, in another ticket?
And let's add a consensus method that removes the authority code? In another ticket?

And if we're looking to remove unused consensus code in a new consensus method, "package" lines can go too:
https://gitweb.torproject.org/torspec.git/tree/proposals/301-dont-vote-on-package-fingerprints.txt

rs.has_bandwidth = 1;
ASSERT_CHANGED();

// Does not actually matter unless exitsummary changes.
Copy link
Contributor

@teor2345 teor2345 Jan 17, 2020

It actually doesn't matter at all, exitsummary is not output via the control port routerstatus format. (See exitsummary below for details.)

routerstatus_has_changed() Is only checking for changes that get output over the control port:

It only checks for fields that are output by control port.
This should be kept in sync with the struct routerstatus_t
and the printing function routerstatus_format_entry in
NS_CONTROL_PORT mode.

Let's rename routerstatus_has_changed() to a name that reflects its purpose as a control-port only function.

Copy link
Contributor

@teor2345 teor2345 Jan 17, 2020

In fact, has_exitsummary is not used in any routerstatus format. Perhaps it's used in microdescriptors?


// Does not actually matter? Shouldn't it? XXXX
rs.bw_is_unmeasured = 1;
ASSERT_SAME();
Copy link
Contributor

@teor2345 teor2345 Jan 17, 2020

This is a NS_CONTROL_PORT format change function, and this field is only output in NS_V3_VOTE format:

if (format == NS_V3_VOTE && vrs && vrs->has_measured_bw) {

In fact, bw_is_unmeasured is not used in any routerstatus format. Perhaps it's used by other code?

// XXX Shoudn't this count as a change?
rs.has_guardfraction = 1;
rs.guardfraction_percentage = 22;
ASSERT_SAME();
Copy link
Contributor

@teor2345 teor2345 Jan 17, 2020

This is a NS_CONTROL_PORT format change function, and this field is only output in NS_V3_VOTE format:

if (format == NS_V3_VOTE && vrs && vrs->status.has_guardfraction) {


// Shouldn't this count as a change?
rs.exitsummary = (char*)"accept 1-2";
ASSERT_SAME();
Copy link
Contributor

@teor2345 teor2345 Jan 17, 2020

exitsummary is not used by in any routerstatus format. Perhaps it's used by other code?

nmathewson added 5 commits Jan 17, 2020
Now they also check whether output of routerstatus_format_entry()
has changed.
This is an automated commit, generated by this command:

./scripts/maint/rename_c_identifier.py \
        routerstatus_has_changed routerstatus_has_visibly_changed

It was generated with --no-verify, since it introduces a wide line.
I'll fix it in a subsequent commit.
@teor2345 teor2345 mentioned this pull request Jan 20, 2020
@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jan 20, 2020

Resolved merge conflict in #1673.

@teor2345 teor2345 closed this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment