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.
Details
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…
ninja check-functional
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- useragent-conf
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 16068 Build 32025: Build Diff build-without-wallet · build-diff · lint-circular-dependencies · build-debug · build-clang-tidy · build-clang Build 32024: arc lint + arc unit
Event Timeline
Comment Actions
Changed to -uaclient, which sets the client name and version of the user agent, staying compatible with -uacomment
| 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. |
Comment Actions
Changed FormatSubVersion to FormatUserAgent and solely use FormatSubVersionUserAgent now
Comment Actions
Why not have two flags that can be set, -uaclientname, and -uaclientversion ?
Seems like that would be the simplest solution.
Comment Actions
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 ↗ | (On Diff #28980) | Merge this with FormatSubVersionUserAgent, these function don't make sense on their own. |
| src/init.cpp | ||
| 2322 ↗ | (On Diff #28980) | 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 ↗ | (On Diff #28980) | The only thing that shoudl change in there is the check for parameter's value and related code. Everything else is gratuitous complexity. |
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.
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