Page MenuHomePhabricator

Merge #10408, #13291, and partial #13163
Needs RevisionPublic

Authored by nakihito on Mon, Jun 10, 21:27.

Details

Reviewers
jasonbcox
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Merge #10408: Net: Improvements to Tor control port parser
49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg)
0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg)
0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg)
d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg)
29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg)
d8e03c0 torcontrol: Improve comments (Jack Grigg)

Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae

Backport of Core PR10408
https://github.com/bitcoin/bitcoin/pull/10408/files

Merge #13291: test: Don't include torcontrol.cpp into the test file
97c112d4ca Declare TorReply parsing functions in torcontrol_tests (Ben Woosley)

Pull request description:

These methods are standalone string parsing methods which were included
into test via an include of torcontrol.cpp, which is bad practice.

~~Splitting them out reveals that they were the only torcontrol.cpp
methods under test, so the test file is renamed tor_reply_tests.cpp.~~

Introduced in #10408

Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac

Backport of Core PR13291
https://github.com/bitcoin/bitcoin/pull/13291/

(partial) Merge #13163: Make it clear which functions that are intended to be translation unit local
c3f34d06be Make it clear which functions that are intended to be translation unit local (practicalswift)

Pull request description:

Make it clear which functions that are intended to be translation unit local.

Do not share functions that are meant to be translation unit local with other translation units. Use internal linkage for those consistently.

Tree-SHA512: 05eebd233d5cfbf6116724eec3a99b465bf534ca220f2b6f5e56341a7da41387454d3cb6ceadd8ab6714a5df94069e4ad0dcab8801ccc7e8949be7199a19fb53

Backport of Core PR13163
https://github.com/bitcoin/bitcoin/pull/13163/

Completes T612

Introduces a memory leak fixed by PR10587
https://github.com/bitcoin/bitcoin/pull/10587

Memory leak fix: https://reviews.bitcoinabc.org/D3285

Includes a fix to the locale linter discussed with fabien

D3285 should be landed immediately after this one

Test Plan
make check
test_runner.py

arc lint should pass with no errors

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR10408
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6292
Build 10631: Bitcoin ABC Teamcity Staging
Build 10630: arc lint + arc unit

Event Timeline

nakihito created this revision.Mon, Jun 10, 21:27
Owners added a reviewer: Restricted Owners Package.Mon, Jun 10, 21:27
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Jun 10, 21:27
nakihito edited the test plan for this revision. (Show Details)Mon, Jun 10, 21:29
nakihito planned changes to this revision.Mon, Jun 10, 21:37
nakihito requested review of this revision.Mon, Jun 10, 22:46
nakihito edited the summary of this revision. (Show Details)Mon, Jun 10, 22:49
jasonbcox requested changes to this revision.Mon, Jun 10, 23:03
jasonbcox added inline comments.
src/Makefile.test.include
119 ↗(On Diff #9304)

Need associated CMake change as well.

src/torcontrol.cpp
322 ↗(On Diff #9304)

Comment doesn't match PR

347 ↗(On Diff #9304)

Both paragraphs auto-indented by linter need to be fixed.

430 ↗(On Diff #9304)

brackets

This revision now requires changes to proceed.Mon, Jun 10, 23:03
nakihito updated this revision to Diff 9308.Tue, Jun 11, 00:21

Added CMake change and fixed nits.

Fabien requested changes to this revision.Tue, Jun 11, 06:25
Fabien added inline comments.
arcanist/linter/LocaleDependenceLinter.php
34 ↗(On Diff #9308)

Note to reviewers:
This exception was here from the beginning when core added the locale linter.
It wasn't added to our exception list because there was no such case in our codebase (yet).

src/test/torcontrol_tests.cpp
5 ↗(On Diff #9308)

Use angle brackets

6 ↗(On Diff #9308)

Don't include cpp files. Look at what core did to this one, it should have been fixed.

12 ↗(On Diff #9308)

Can likely be static

49 ↗(On Diff #9308)

Dito

This revision now requires changes to proceed.Tue, Jun 11, 06:25

In the process of working on the mentioned backports.

src/test/torcontrol_tests.cpp
6 ↗(On Diff #9308)
49 ↗(On Diff #9308)

This and above are solved here: https://github.com/bitcoin/bitcoin/pull/13163

nakihito updated this revision to Diff 9326.Tue, Jun 11, 22:14

Changed to angled brackets in include statements.

nakihito edited the summary of this revision. (Show Details)Tue, Jun 11, 22:44
jasonbcox requested changes to this revision.Tue, Jun 11, 23:40
jasonbcox added inline comments.
src/test/torcontrol_tests.cpp
6 ↗(On Diff #9308)

Go ahead and include PR13291 in this backport. It's small enough that review shouldn't be negatively impacted.

49 ↗(On Diff #9308)

Include this in this backport as well.

This revision now requires changes to proceed.Tue, Jun 11, 23:40
nakihito updated this revision to Diff 9337.Wed, Jun 12, 00:23

Merged with PR13921 and the torcontrol_test.cpp parts of PR13163.

nakihito retitled this revision from Merge #10408: Net: Improvements to Tor control port parser to Merge #10408, #13291, and partial #13163.Wed, Jun 12, 00:31
nakihito edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Wed, Jun 12, 11:23

Please test running with TOR and add it to the test plan

src/test/torcontrol_tests.cpp
6 ↗(On Diff #9337)

Reorder:

#include <torcontrol.h>

#include <test/test_bitcoin.h>

Note the empty line to avoid the linter to re-sort them

This revision now requires changes to proceed.Wed, Jun 12, 11:23
nakihito updated this revision to Diff 9364.Wed, Jun 12, 17:32

Fixed includes ordering.

deadalnix added a comment.EditedWed, Jun 12, 20:07

Why are all of these together ?

deadalnix requested changes to this revision.Thu, Jun 13, 01:02
This revision now requires changes to proceed.Thu, Jun 13, 01:02

Why are all of these together ?

Originally, I had 3 different patches (https://reviews.bitcoinabc.org/D3293, https://reviews.bitcoinabc.org/D3294, https://reviews.bitcoinabc.org/D3285 ) that were to be landed immediately after this one (which originally only backported PR10408). I was told that these patches should be squashed together.

There are no explanation as to why the squash is necessary in the link you provided nor in your comment.

nakihito requested review of this revision.Tue, Jun 18, 18:15

There are no explanation as to why the squash is necessary in the link you provided nor in your comment.

Sorry, I'll explain why here:

PR10408 introduces a new test within which it includes a .cpp file. Rather than introducing the code as is, we decided it was better to include the small but related PR13291 which would fix the .cpp include line (see change request here: https://reviews.bitcoinabc.org/D3283#77581).

Additionally, there were a few functions introduced in PR10408 that would later be changed to static. Since these changes were part of the already rather large PR13163 (which was originally requested to be broken into smaller patches), we decided that it would be best to include in this patch (see change request here: https://reviews.bitcoinabc.org/D3283#77581).

Request to squash patches: https://reviews.bitcoinabc.org/D3283#77753

D3285 is the last remaining related patch related to this change which fixes a memory leak introduced by PR10408.

jasonbcox accepted this revision.Fri, Jun 21, 21:56
Fabien requested changes to this revision.Mon, Jun 24, 06:46

Still need to run tor as part of the test plan.

This revision now requires changes to proceed.Mon, Jun 24, 06:46