Page MenuHomePhabricator

Add -uaclientname and -uaclientversion config options to set the client name and version of the user agent in the version message.
ClosedPublic

Authored by tobias_ruck on Jun 4 2021, 02:37.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC038e6a22db0b: Add -uaclientname and -uaclientversion config options to set the client name…
Summary

Since BCHD unconditionally disconnects from nodes containing the "Bitcoin ABC" string, we cannot use it to connect to a Bitcoin ABC node, even if connected directly using -connect. This option allows to circumvent that feature.

Test Plan

ninja check-functional

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jun 4 2021, 02:37
deadalnix requested changes to this revision.Jun 4 2021, 09:59
deadalnix added a subscriber: deadalnix.

This breaks the -uacomment flag.

This revision now requires changes to proceed.Jun 4 2021, 09:59

Changed to -uaclient, which sets the client name and version of the user agent, staying compatible with -uacomment

tobias_ruck retitled this revision from Add -useragent config option to explicitly set the User Agent in the version message. to Add -uaclient config option to set the client name and version of the user agent in the version message..Jun 6 2021, 22:03
tobias_ruck edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Jun 7 2021, 09:50
deadalnix added inline comments.
src/init.cpp
2302 ↗(On Diff #28780)

Why? And if that is a requirement, why not ask for a client name and a version number rather than this?

2319 ↗(On Diff #28780)

here

src/net.cpp
3158 ↗(On Diff #28780)

You are branching twice on the same condition for pretty much the same reason (see here comment).

This is indicative of brittle design. For instance, if you take client name + version, then you can simply use the same function in both cases, but if you do not, then you can still create a default uaclient from CLIENT_NAME and CLIENT_VERSION. Either way, this current way of doing it doesn't make sense.

This revision now requires changes to proceed.Jun 7 2021, 09:50

Changed FormatSubVersion to FormatUserAgent and solely use FormatSubVersionUserAgent now

Why not have two flags that can be set, -uaclientname, and -uaclientversion ?

Seems like that would be the simplest solution.

deadalnix requested changes to this revision.Jun 8 2021, 15:29
deadalnix added inline comments.
src/init.cpp
2307 ↗(On Diff #28795)

It doesn't looks like this should be enforced.

src/net.cpp
3158 ↗(On Diff #28795)

That check happens within gArgs.GetArg already.

This revision now requires changes to proceed.Jun 8 2021, 15:29

Changed to -uaclientname and -uaclientversion as discussed.

tobias_ruck retitled this revision from Add -uaclient config option to set the client name and version of the user agent in the version message. to Add -uaclientname and -uaclientversion config options to set the client name and version of the user agent in the version message..Jun 25 2021, 05:26
deadalnix requested changes to this revision.Jun 28 2021, 13:02

It looks like to me this got a bit out of control. In D9637, you split FormatSubVersion in two, and in this, you do it again. At this rate, 8 more patches and we get 1024 variation of that function.

There are several function with only one callsite that do serve much of a purpose in this. You need to simplify.

src/clientversion.cpp
88

Merge this with FormatSubVersionUserAgent, these function don't make sense on their own.

src/init.cpp
2322

Every single call site contains both function calls, this is the kind of sign you must not ignore. You don't need me to tell you this, the code already told you this.

src/net.cpp
3219

The only thing that shoudl change in there is the check for parameter's value and related code. Everything else is gratuitous complexity.

This revision now requires changes to proceed.Jun 28 2021, 13:02

Merged FormatUserAgent and FormatSubVersionUserAgent

Failed tests logs:

====== Bitcoin ABC functional tests: abc_p2p_proof_inventory.py ======

------- Stdout: -------
2021-06-28T14:31:40.697000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210628_143131/abc_p2p_proof_inventory_624
2021-06-28T14:31:41.960000Z TestFramework (INFO): Test sending a proof to our peers
2021-06-28T14:31:46.612000Z TestFramework (INFO): Test that we don't send the same inv several times
2021-06-28T14:31:48.434000Z TestFramework (INFO): Test a peer is created on proof reception
2021-06-28T14:31:48.561000Z TestFramework (INFO): Test receiving a proof with missing utxo is orphaned
2021-06-28T14:31:54.029000Z TestFramework (INFO): Nodes should eventually get the proof from their peer
2021-06-28T14:31:56.107000Z TestFramework (INFO): Test broadcasting proofs
2021-06-28T14:32:01.825000Z TestFramework (INFO): Proofs that become invalid should no longer be broadcasted
2021-06-28T14:32:04.349000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 127, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_proof_inventory.py", line 295, in run_test
    self.test_unbroadcast()
  File "/work/test/functional/abc_p2p_proof_inventory.py", line 281, in test_unbroadcast
    assert_equal(raw_proof["orphan"], True)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(False == True)
2021-06-28T14:32:04.400000Z TestFramework (INFO): Stopping nodes
2021-06-28T14:32:04.754000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210628_143131/abc_p2p_proof_inventory_624
2021-06-28T14:32:04.754000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210628_143131/abc_p2p_proof_inventory_624/test_framework.log
2021-06-28T14:32:04.755000Z TestFramework (ERROR): 
2021-06-28T14:32:04.755000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210628_143131/abc_p2p_proof_inventory_624' to consolidate all logs
2021-06-28T14:32:04.755000Z TestFramework (ERROR): 
2021-06-28T14:32:04.755000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2021-06-28T14:32:04.755000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2021-06-28T14:32:04.755000Z TestFramework (ERROR):

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_p2p_proof_inventory.py

This revision is now accepted and ready to land.Jun 28 2021, 23:50