Page MenuHomePhabricator

Merge #10408, #13291, and partial #13163
ClosedPublic

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

Details

Reviewers
jasonbcox
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGINGbe756d0c7f94: Merge #10408, #13291, and partial #13163
rABCbe756d0c7f94: Merge #10408, #13291, and partial #13163
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
sudo apt install tor # if not already installed
sudo etc/init.d/tor status # to check if tor is running
sudo etc/init.d/tor start # if it is not already running
./src/bitcoind -proxy=127.0.0.1:<tor port>
./bitcoin-cli getpeerinfo

tor defaults to port 9050.
getpeerinfo should be populated.
arc lint should pass with no errors.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR10408
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6257
Build 10561: Bitcoin ABC Buildbot (legacy)
Build 10560: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Owners added a reviewer: Restricted Owners Package.Jun 10 2019, 21:27
jasonbcox requested changes to this revision.Jun 10 2019, 23:03
jasonbcox added inline comments.
src/Makefile.test.include
119

Need associated CMake change as well.

src/torcontrol.cpp
322

Comment doesn't match PR

347

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

430

brackets

This revision now requires changes to proceed.Jun 10 2019, 23:03

Added CMake change and fixed nits.

Fabien requested changes to this revision.Jun 11 2019, 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.Jun 11 2019, 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

Changed to angled brackets in include statements.

jasonbcox requested changes to this revision.Jun 11 2019, 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.Jun 11 2019, 23:40

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.Jun 12 2019, 00:31
nakihito edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Jun 12 2019, 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.Jun 12 2019, 11:23

Why are all of these together ?

deadalnix requested changes to this revision.Jun 13 2019, 01:02
This revision now requires changes to proceed.Jun 13 2019, 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.

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.

Fabien requested changes to this revision.Jun 24 2019, 06:46

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

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

Ran bitcoind under tor and updated test plan.

nakihito edited the test plan for this revision. (Show Details)

Clarified test plan concerning tor.

OK I got back to it. I don't get the way things are split up. Why do this, which introduce a memory leak, and then do PR10587 to fix it? If the goal was to merge things so that we don't land bugs then what is it about? On the other hand, PR13291 was merged here, but clearly it is not bugfix. The logic of the decoupage do not seems to follow any principle.

This revision is now accepted and ready to land.Jun 25 2019, 20:24