Opened 2 years ago

Closed 2 months ago

#21377 closed enhancement (fixed)

DirAuths should expose bwauth bandwidth files

Reported by: tom Owned by: juga
Priority: High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: authority-test-done, tor-dirauth, metrics-needs, tor-bwauth, 035-removed-20180711, 040-roadmap-proposed, 040-backport
Cc: starlight@… Actual Points: 0.3
Parent ID: #25925 Points: 0.2
Reviewer: ahf Sponsor:

Description

Currently, DirAuths that vote on bwauth files do not expose the raw voting file they used. This data would be good to archive for debugging and transparency purposes.

Child Tickets

TicketTypeStatusOwnerSummary
#26694enhancementclosedjugadir-spec: DirAuths should expose bwauth bandwidth files
#28815defectclosedRefactor similar compression buffer code in dircache.c
#28816defectclosedrl1987Call a correct connection_buf_add* function based on compress_state of dir_connection_t
#29896enhancementclosedteorBackport connection_dir_buf_add() to 0.3.4 and later
#29897defectclosedteorRefactor handle_get_next_bandwidth() to use connection_dir_buf_add()

Change History (77)

comment:1 Changed 2 years ago by tom

The ticket blocks archiving the data in collector, #21378

comment:2 Changed 2 years ago by asn

Keywords: tor-dirauth metrics added
Milestone: Tor: unspecified

comment:3 Changed 2 years ago by nickm

Priority: LowHigh

comment:4 Changed 12 months ago by juga

This is now fixed in sbws, though is still not being use by any DirAuth.

It is not planned to add this to Torflow.

comment:5 Changed 12 months ago by juga

Keywords: tor-bwauth added

comment:6 Changed 12 months ago by juga

Cc: teor added

comment:7 in reply to:  4 Changed 12 months ago by teor

Summary: DirAuths should expose bwauth votesDirAuths should expose bwauth bandwidth files

Replying to juga:

This is now fixed in sbws, though is still not being use by any DirAuth.

It is not planned to add this to Torflow.

This ticket is about archiving the entire v3bw file, not just the headers.

Can you explain how this is fixed in sbws?

Version 0, edited 12 months ago by teor (next)

comment:8 Changed 12 months ago by juga

My mistake, didn't re-read the title of the ticket.
No, sbws does not fix this.

comment:9 Changed 11 months ago by juga

Changes to dir-spec commented https://trac.torproject.org/projects/tor/ticket/21378#comment:7 implemented in my branch.
Please let me know if i should create a different ticket for the spec.

Edit: correct url to the comment

Last edited 11 months ago by juga (previous) (diff)

comment:10 Changed 11 months ago by juga

Status: newneeds_review

comment:11 in reply to:  9 Changed 11 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: needs_reviewneeds_revision

Replying to juga:

Changes to dir-spec commented https://trac.torproject.org/projects/tor/ticket/21378#comment:7 implemented in my branch.

I think the pull request is:
https://github.com/torproject/torspec/pull/21

In future, please link to the branch or pull request, so I know which one you want me to review.

Please let me know if i should create a different ticket for the spec.

Yes, please open a different ticket, otherwise this ticket will close when we merge the spec.

Please see my comments on the pull request.

comment:12 Changed 11 months ago by juga

Created #26694 for the spec changes.

comment:13 Changed 11 months ago by teor

Parent ID: #25925

comment:14 Changed 11 months ago by juga

Owner: set to juga
Status: needs_revisionaccepted

Changing status since the ticket for the spec is #26694

comment:15 Changed 11 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

These tickets are being triaged out of 0.3.5. The ones marked "035-roadmap-proposed" may return.

comment:16 Changed 11 months ago by teor

Keywords: 035-roadmap-proposed added

We would like to implement this ticket in 0.3.5, but we want a working bandwidth scanner first.

comment:17 Changed 11 months ago by teor

The refactor in #26797 will involve storing a copy of the bandwidth file in RAM or on disk, so let's do it first.

comment:18 Changed 8 months ago by teor

Keywords: 036-roadmap-proposed added; 035-roadmap-proposed removed

0.3.5 is closed to new features, moving to 0.3.6 proposed.

comment:19 Changed 8 months ago by juga

Cc: teor removed
Status: acceptedneeds_review

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

I'm not sure i've implemented in the right way, maybe it should be call from handle_get_next_vote, or somehow check that the bandwidth file that used in the vote is the same being read from options->V3BandwidthsFile.
Maybe the hash of the file should be stored somewhere?.
Should some maximum size be check?.
Would be the timeline of the file working as expected?.

comment:20 Changed 7 months ago by teor

Note for the review assigner:

ahf wants to review tor bandwidth authority patches.

Note for the reviewer:

Let's get a simple version of this patch working first. The simplest version sends the bandwidth file that is on disk right now.

Then we can follow up with:

  • #27047: authorities should keep recent consensuses, votes, and bandwidth files
  • #26797: read the file once per vote, and use the cached file for votes and bandwidth file requests
  • #26698: put a hash of the bandwidth file in the vote

comment:21 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

All the CI on this branch failed.

juga, I'm not sure if you get emails from appveyor or travis when builds fail? Or if you're in the #tor-ci channel?

comment:22 in reply to:  21 Changed 7 months ago by juga

Replying to teor:

juga, I'm not sure if you get emails from appveyor or travis when builds fail? Or if you're in the #tor-ci channel?

yes i do. I just was not going to work on it at the moment i saw it, but i should have removed "need_review".

comment:23 Changed 7 months ago by juga

Status: needs_revisionneeds_review

Fixed CI

comment:24 Changed 7 months ago by dgoulet

Status: needs_reviewneeds_revision

comment:25 Changed 7 months ago by juga

Status: needs_revisionneeds_review

New commits

comment:26 Changed 7 months ago by starlight

Cc: starlight@… added

comment:27 Changed 7 months ago by teor

Keywords: 040-roadmap-proposed added; 036-roadmap-proposed removed

0.3.6 is now 0.4.0: changing roadmap keywords

comment:28 Changed 7 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.0.x-final

This is a new feature with code: it belongs in the next feature release.

comment:29 Changed 7 months ago by asn

Reviewer: ahf

comment:30 Changed 7 months ago by ahf

Status: needs_reviewneeds_revision

Left some minor stylistic comments on the PR, but code looks overall very good.

comment:31 Changed 7 months ago by juga

Status: needs_revisionneeds_review

comment:32 Changed 7 months ago by ahf

Status: needs_reviewmerge_ready

LGTM now.

comment:33 Changed 7 months ago by nickm

Status: merge_readyneeds_revision

This looks solid, but I have some requests and questions:

  • Could we please have a changes file?
  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here. Maybe we're supposed to send the compressed length if compression is in use?

comment:34 in reply to:  33 ; Changed 7 months ago by teor

Replying to nickm:

This looks solid, but I have some requests and questions:

  • Could we please have a changes file?
  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here.

If it is present, Content-Length must be the length of the encoded data.

Maybe we're supposed to send the compressed length if compression is in use?

When we deliver compressed replies in other code, we don't provide Content-Length:
https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/feature/dircache/dircache.c#L1293

comment:35 in reply to:  34 ; Changed 7 months ago by juga

Replying to teor:

  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here.

If it is present, Content-Length must be the length of the encoded data.

Maybe we're supposed to send the compressed length if compression is in use?

When we deliver compressed replies in other code, we don't provide Content-Length:
https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/feature/dircache/dircache.c#L1293

I verified that there's not Content-Length in the header doing that.

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

comment:36 in reply to:  35 ; Changed 7 months ago by teor

Replying to juga:

Replying to teor:

  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here.

If it is present, Content-Length must be the length of the encoded data.

Maybe we're supposed to send the compressed length if compression is in use?

When we deliver compressed replies in other code, we don't provide Content-Length:
https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/feature/dircache/dircache.c#L1293

I verified that there's not Content-Length in the header doing that.

I don't understand what you mean.
If you pass a zero or positive length, then the code sets Content-Length.
I added a suggested fix on the pull request.

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

I don't understand what you mean here.
If you're calling the new code in the test, you have to add headers to the request.
Otherwise, the new code should return NO_METHOD.

I don't understand how you are getting UNKNOWN_METHOD.
Please show me the code you are using, and the logs of the results.

comment:37 in reply to:  36 ; Changed 7 months ago by juga

Replying to teor:

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

I don't understand what you mean here.
If you're calling the new code in the test, you have to add headers to the request.
Otherwise, the new code should return NO_METHOD.

I don't understand how you are getting UNKNOWN_METHOD.
Please show me the code you are using, and the logs of the results.

https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554

comment:38 in reply to:  37 Changed 7 months ago by teor

Replying to juga:

Replying to teor:

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

I don't understand what you mean here.
If you're calling the new code in the test, you have to add headers to the request.
Otherwise, the new code should return NO_METHOD.

I don't understand how you are getting UNKNOWN_METHOD.
Please show me the code you are using, and the logs of the results.

https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554

I can't find the logs of the results. Can you put them in a pastebin?

comment:40 Changed 7 months ago by juga

I put them in code comments. Created also a pastebin with logs only for that test and log level debug: https://paste.debian.net/1054163/

comment:41 in reply to:  40 Changed 6 months ago by juga

Replying to juga:

I put them in code comments. Created also a pastebin with logs only for that test and log level debug: https://paste.debian.net/1054163/

Now in https://paste.debian.net/hidden/23b3c78e/

comment:42 Changed 6 months ago by teor

It looks like you're setting the compression state, then adding the data uncompressed.

You didn't open a pull request, so I commented on the commit:
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629291
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629096

I also opened:

  • #28815 to clean up the copied code,
  • #28816 to log a bug warning when uncompressed data is added to a compressed connection

You don't have to do these tickets: they are not on the roadmap.

comment:43 in reply to:  42 ; Changed 6 months ago by juga

Replying to teor:

It looks like you're setting the compression state, then adding the data uncompressed.

You didn't open a pull request, so I commented on the commit:
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629291
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629096

I didn't create a PR, cause i knew that code fails, but wanted to ask about it.
i've left new comments after setting the compression state.

comment:44 in reply to:  43 ; Changed 6 months ago by teor

Replying to juga:

Replying to teor:

It looks like you're setting the compression state, then adding the data uncompressed.

You didn't open a pull request, so I commented on the commit:
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629291
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629096

I didn't create a PR, cause i knew that code fails, but wanted to ask about it.

Please open pull requests for code questions and CI, even if the code doesn't work yet.

When you open a pull request:

  • comments are easier to make and easier to find
  • new commits get handled better
  • the CI is done on a merge with master, so any bugs fixed in master are fixed in the CI

If you don't want a pull request merged, put the ticket in needs_review, and say that the code doesn't work.

Some people also put "WIP" or "work in progress" in the pull request title:
https://stackoverflow.com/a/39741877

i've left new comments after setting the compression state.

I'm not sure how to find the new code, or the CI for that code.

I tried looking at your branches, but they haven't changed:
https://github.com/juga0/tor/branches

When you ask questions, please open a pull request.
(Or link to a branch, or a commit. But a pull request is better.)

comment:45 in reply to:  44 ; Changed 6 months ago by juga

Replying to teor:
[...]

Please open pull requests for code questions and CI, even if the code doesn't work yet.

is this a net-team preference or your personal preference?.
I'm sorry that i learn about preferred working methods by try/error. I don't think this is documented anywhere and IRC questions don't work well with our timezones.

i've left new comments after setting the compression state.

I'm not sure how to find the new code, or the CI for that code.

Sorry, there was not new code pushed to the branch, i thought it was possible to follow the comments without the code.

When you ask questions, please open a pull request.

https://github.com/juga0/tor/commits/ticket21377_035_01_work
https://github.com/torproject/tor/pull/610

Because it's the same branch, the previous comments/questions can be seen in the PR.

Edit: link to the PR, not the branch

Last edited 6 months ago by juga (previous) (diff)

comment:46 Changed 6 months ago by juga

Status: needs_revisionneeds_review

comment:47 in reply to:  45 ; Changed 6 months ago by teor

Replying to juga:

Replying to teor:
[...]

Please open pull requests for code questions and CI, even if the code doesn't work yet.

is this a net-team preference or your personal preference?.

It's common practice within the team.

I'm sorry that i learn about preferred working methods by try/error. I don't think this is documented anywhere and IRC questions don't work well with our timezones.

That's ok, we are all still working out how we want to use GitHub.

i've left new comments after setting the compression state.

I'm not sure how to find the new code, or the CI for that code.

Sorry, there was not new code pushed to the branch, i thought it was possible to follow the comments without the code.

When you ask questions, please open a pull request.

https://github.com/juga0/tor/commits/ticket21377_035_01_work
https://github.com/torproject/tor/pull/610

Because it's the same branch, the previous comments/questions can be seen in the PR.

Edit: link to the PR, not the branch

Thanks!

I think nickm's comments might be the cause of your issue: is the test actually running the compression code?

comment:48 in reply to:  47 Changed 6 months ago by juga

Replying to teor:

I think nickm's comments might be the cause of your issue:

yes, that makes sense

is the test actually running the compression code?

the test is using the compression code implemented in directory_handle_command_get, but because the file is wrote with connection_write_to_buf_mock (connection_buf_add_compress is not mocked), fetch_from_buf_http gets the content uncompressed (the headers are fine).

I'm not sure how difficult would be to mock connection_buf_add_compress. For now i added fixups to the non-wip PR (https://github.com/torproject/tor/pull/468) with the mistakes in the implementation. Then run a test network and retrieved the bandwidth file from an authority using stem:

>>> 
... import stem.descriptor
... import stem.descriptor.remote
... import stem.directory
>>> downloader = stem.descriptor.remote.DescriptorDownloader(
...   document_handler = stem.descriptor.DocumentHandler.DOCUMENT,
... )
>>> resource = '/tor/status-vote/next/bandwidth'
>>> query_args = {'endpoints': [('127.10.0.1', 2003)]}
>>> bw = downloader.query(resource, **query_args)
>>> bw.content
b'1544811454\nversion=1.2.0\nearliest_bandwidth=2018-12-12T15:02:35\nfile_created=2018-12-15T08:09:48\ngenerator_started=2018-12-15T08:06:50\nlatest_bandwidth=2018-12-14T18:17:34\nminimum_number_eligible_relays=3820\nminimum_percent_eligible_relays=60\nnumber_consensus_relays=6366\nnumber_eligible_relays=0\npercent_eligible_relays=0\nsoftware=sbws\nsoftware_version=1.0.3-dev0\n====='
>>> bw.compression
['identity']
>>> resource = '/tor/status-vote/next/bandwidth.z'
>>> bw = downloader.query(resource, **query_args)
>>> bw.content
b'1544811454\nversion=1.2.0\nearliest_bandwidth=2018-12-12T15:02:35\nfile_created=2018-12-15T08:09:48\ngenerator_started=2018-12-15T08:06:50\nlatest_bandwidth=2018-12-14T18:17:34\nminimum_number_eligible_relays=3820\nminimum_percent_eligible_relays=60\nnumber_consensus_relays=6366\nnumber_eligible_relays=0\npercent_eligible_relays=0\nsoftware=sbws\nsoftware_version=1.0.3-dev0\n====='
>>> bw.compression
['gzip']

You think this is valid test and this can be merged?.

comment:49 Changed 5 months ago by teor

You think this is valid test and this can be merged?.

As long as we can get stem 1.7, it seems like a good test.

comment:50 in reply to:  49 ; Changed 5 months ago by juga

Replying to teor:

You think this is valid test and this can be merged?.

As long as we can get stem 1.7, it seems like a good test.

Hmm, i didn't mean to include the python code as a test in tor code, since the test in C already checks the header (https://github.com/torproject/tor/pull/468/files#diff-a63a60330b655aa123b096768848f5bfR2552), but to manually extra check that i can actually get the bandwidth file in a test network.
I currently don't know how it'd be possible to run a test network when running tor tests and include this code as an stem test.

So the question is then, is it fine the PR as it is?.

comment:51 in reply to:  50 ; Changed 5 months ago by teor

Replying to juga:

Replying to teor:

You think this is valid test and this can be merged?.

As long as we can get stem 1.7, it seems like a good test.

Hmm, i didn't mean to include the python code as a test in tor code, since the test in C already checks the header (https://github.com/torproject/tor/pull/468/files#diff-a63a60330b655aa123b096768848f5bfR2552), but to manually extra check that i can actually get the bandwidth file in a test network.
I currently don't know how it'd be possible to run a test network when running tor tests and include this code as an stem test.

We don't need a test network, we just need a single tor instance.
We have some tor integration tests already:
https://gitweb.torproject.org/tor.git/tree/src/test/test_rebind.sh

But we would need to configure the tor instance as a directory authority. So we would need to run tor-gencert to generate the authority keys. And require stem.

Maybe the best place for this test is in stem?
We could open a ticket for a stem integration test that tests bandwidth files.
Do you think this test is worth adding? Will it catch future bugs?

So the question is then, is it fine the PR as it is?.

I think the PR is ok without the test.
But ahf still needs to review your changes.

comment:52 Changed 5 months ago by ahf

Status: needs_reviewneeds_revision

Two minor comments added, but other than that I think it looks good.

comment:53 Changed 5 months ago by juga

Status: needs_revisionneeds_review

I added the comment and change the log message.

comment:54 Changed 5 months ago by ahf

Status: needs_reviewmerge_ready

LGTM, let's get it in :-)

comment:55 Changed 5 months ago by nickm

Fine by me -- only, which PR do I merge here? I'm looking for links, but this is a looong ticket :)

comment:56 in reply to:  55 Changed 5 months ago by juga

Replying to nickm:

Fine by me -- only, which PR do I merge here? I'm looking for links, but this is a looong ticket :)

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

comment:57 in reply to:  51 ; Changed 5 months ago by juga

Replying to teor:

Maybe the best place for this test is in stem?
We could open a ticket for a stem integration test that tests bandwidth files.
Do you think this test is worth adding? Will it catch future bugs?

I talked with atagar in #tor-dev, and there'd be the same problem in stem, it does not run a test network. Maybe it can be mocked all that dirauth needs before serving the bandwidth file, but i didn't see and easy way to do it.

comment:58 Changed 5 months ago by nickm

Could someone please prepare a squashed branch for me to merge? When I try to use "git-rebase" to squash all the fixup commits, I get conflicts.

comment:59 in reply to:  57 Changed 5 months ago by teor

Replying to juga:

Replying to teor:

Maybe the best place for this test is in stem?
We could open a ticket for a stem integration test that tests bandwidth files.
Do you think this test is worth adding? Will it catch future bugs?

I talked with atagar in #tor-dev, and there'd be the same problem in stem, it does not run a test network. Maybe it can be mocked all that dirauth needs before serving the bandwidth file, but i didn't see and easy way to do it.

Why do you think we need a test network?

Replying to teor:

We don't need a test network, we just need a single tor instance.
We have some tor integration tests already:
https://gitweb.torproject.org/tor.git/tree/src/test/test_rebind.sh

But we would need to configure the tor instance as a directory authority. So we would need to run tor-gencert to generate the authority keys. And require stem.

comment:60 in reply to:  58 Changed 5 months ago by juga

Replying to nickm:

Could someone please prepare a squashed branch for me to merge? When I try to use "git-rebase" to squash all the fixup commits, I get conflicts.

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

Probably it's better you also squash all the commits in one, since most of them are small fixes to the previous commits.

comment:61 Changed 5 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.4.1.x-final

Okay, looks reasonable to me. Let's take it first thing in 0.4.1.

comment:62 Changed 4 months ago by dgoulet

Status: merge_readyneeds_revision

See comments on the PR. Don't hesitate to rebase on master here to use the new function as I pointed out.

comment:63 Changed 4 months ago by juga

Oh, do the children tickets need to be closed before?.
(In this case i thought that they were improvements that could be solved later and would need to be unparented to close this ticket first.)
In that case i would wait for #28815 before adapting the code to #28816, unless there's a better strategy.

comment:64 in reply to:  63 Changed 3 months ago by teor

Replying to juga:

Oh, do the children tickets need to be closed before?.
(In this case i thought that they were improvements that could be solved later and would need to be unparented to close this ticket first.)
In that case i would wait for #28815 before adapting the code to #28816, unless there's a better strategy.

We can do #28815 later, and #26694 after we merge this ticket.

comment:65 Changed 3 months ago by teor

Keywords: metrics-needs added; metrics removed

We need to get this code in 0.4.1, so that we can archive bandwidth files from authorities.

comment:66 Changed 3 months ago by juga

Status: needs_revisionneeds_review

I was asked in https://github.com/torproject/tor/pull/654/files#r258582166 to use the #28816 refactor that was just introduced.
I realized that it was not backported to 0.3.5, so i don't know how to do in this case:

  • should i cherry pick that in 0.3.5?
  • should this ticket code not use #28816 patch and create a ticket to refactor that in 0.4?

comment:67 Changed 3 months ago by juga

I also don't know why with #28816 patch it seems there is no need anymore to flush the compression state with connection_buf_add_compress("", 0, conn, 1).

comment:68 in reply to:  67 Changed 3 months ago by teor

Keywords: consider-backport-after-authority-test consider-backport-after-040-stable 034-backport-maybe 035-backport-maybe 040-backport-maybe added
Status: needs_reviewneeds_revision

Replying to juga:

I was asked in https://github.com/torproject/tor/pull/654/files#r258582166 to use the #28816 refactor that was just introduced.
I realized that it was not backported to 0.3.5, so i don't know how to do in this case:

  • should i cherry pick that in 0.3.5?
  • should this ticket code not use #28816 patch and create a ticket to refactor that in 0.4?

I will backport connection_dir_buf_add() to 0.3.4 in #29896, without the other refactoring.
Then you can rebase this branch on #29896.

I don't know if we will backport this change to 0.3.4 yet.
It is a new feature, and we usually do not backport features.
But it is well-contained, so it is low-risk.

Replying to juga:

I also don't know why with #28816 patch it seems there is no need anymore to flush the compression state with connection_buf_add_compress("", 0, conn, 1).

connection_dir_buf_add() takes a Boolean "done" parameter, and passes it to connection_buf_add_compress().
Since you are only adding one item to the buffer, done should be 1.

comment:69 Changed 3 months ago by teor

Keywords: 034-backport-maybe removed
Status: needs_revisionmerge_ready

I think we should merge the code in this branch to 0.4.0 and master, then do the connection_dir_buf_add() refactor and backports in #29896 and #29897.

Here are the pull requests, any merger can merge after CI passes:
0.3.5: https://github.com/torproject/tor/pull/852
master: https://github.com/torproject/tor/pull/853 (clean merge, testing only, I wonder if practracker will error)

I tried rebasing to 0.3.4, and it wasn't clean, so we won't backport to 0.3.4.
(By the time we backport this feature, most authorities should be on 0.3.5, or the new 0.4.0 stable.)

comment:70 Changed 3 months ago by teor

The refactor is in #29897, it's one simple commit, if we want to review and merge that PR instead.

comment:71 Changed 3 months ago by teor

comment:72 Changed 3 months ago by teor

Actual Points: 0.2
Keywords: 040-backport added; 040-backport-maybe removed
Milestone: Tor: 0.4.1.x-finalTor: 0.3.5.x-final
Points: 0.2

The CI passed. Practracker is ok.

Merged to 0.4.0 and later, marking for backport to 0.3.5, to be considered after 0.4.0 stable and an authority test.

comment:73 Changed 3 months ago by teor

Once #29897 is reviewed and merged, we should backport:
https://github.com/torproject/tor/pull/858

comment:74 Changed 2 months ago by teor

We also need to backport #30001 if we backport this feature.

comment:75 Changed 2 months ago by teor

Actually, we decided not to backport #29897.

So we need to backport https://github.com/torproject/tor/pull/852 and #30001.

comment:76 Changed 2 months ago by teor

Keywords: authority-test-done added; consider-backport-after-authority-test removed

We have tested this code on moria1 and longclaw.

comment:77 Changed 2 months ago by teor

Actual Points: 0.20.3
Keywords: consider-backport-after-040-stable 035-backport-maybe removed
Milestone: Tor: 0.3.5.x-finalTor: 0.4.0.x-final
Resolution: fixed
Status: merge_readyclosed

This feature is too risky to backport: it caused #30001. And it looks like 0.4.0 will be stable before metrics has finished their bandwidth file archiving and analysis code.

Directory authorities that want their bandwidth files archived should upgrade to 0.4.0 when it is stable.

Note: See TracTickets for help on using tickets.