[net] Add extra entropy to the version message
ClosedPublic
Actions

Authored by deadalnix on Jan 9 2021, 22:02.

Details

Reviewers

majcosta

Group Reviewers

Restricted Project

Commits

rABC9343802bc731: [net] Add extra entropy to the version message

Summary

This ensures we have 128 bits of entropy to play with, which can be used as a cryptographic challenge. 64bits is not enough for this.

Depends on D8860

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

entropyversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 14685
Build 29329: Build Diff	lint-circular-dependencies · build-without-wallet · build-clang · build-diff · build-clang-tidy · build-debug
Build 29328: arc lint + arc unit

Event Timeline

deadalnix created this revision.Jan 9 2021, 22:02

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 9 2021, 22:02

deadalnix requested review of this revision.Jan 9 2021, 22:02

deadalnix edited the summary of this revision. (Show Details)Jan 9 2021, 22:02

deadalnix added a parent revision: D8860: [test framework] Simplify version message handling.

Harbormaster completed remote builds in B14684: Diff 26825.Jan 9 2021, 22:14

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

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

------- Stdout: -------
2021-01-09T22:12:32.729000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20210109_220945/wallet_importdescriptors_511
2021-01-09T22:12:33.060000Z TestFramework (INFO): Setting up wallets
2021-01-09T22:12:33.116000Z TestFramework (INFO): Mining coins
2021-01-09T22:12:33.322000Z TestFramework (INFO): Import should fail if a descriptor is not provided
2021-01-09T22:12:33.325000Z TestFramework (INFO): Should import a p2pkh descriptor
2021-01-09T22:12:33.340000Z TestFramework (INFO): Internal addresses cannot have labels
2021-01-09T22:12:33.347000Z TestFramework (INFO): Should import a 1-of-2 bare multisig from descriptor
2021-01-09T22:12:33.360000Z TestFramework (INFO): Should not treat individual keys from the imported bare multisig as watchonly
2021-01-09T22:12:33.361000Z TestFramework (INFO): Ranged descriptors cannot have labels
2021-01-09T22:12:33.363000Z TestFramework (INFO): Private keys required for private keys enabled wallet
2021-01-09T22:12:33.365000Z TestFramework (INFO): Ranged descriptor import should warn without a specified range
2021-01-09T22:12:33.383000Z TestFramework (INFO): Should not import a ranged descriptor that includes xpriv into a watch-only wallet
2021-01-09T22:12:33.398000Z TestFramework (INFO): Key ranges should be imported in order
2021-01-09T22:12:33.506000Z TestFramework (INFO): Check imported descriptors are not active by default
2021-01-09T22:12:33.530000Z TestFramework (INFO): Should import a descriptor with a WIF private key as spendable
2021-01-09T22:12:34.613000Z TestFramework (INFO): Test that multisigs can be imported, signed for, and getnewaddress'd
2021-01-09T22:13:36.364000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/wallet_importdescriptors.py", line 331, in run_test
    self.sync_all()
  File "/work/test/functional/test_framework/test_framework.py", line 509, in sync_all
    self.sync_mempools(nodes, **kwargs)
  File "/work/test/functional/test_framework/test_framework.py", line 505, in sync_mempools
    sync_mempools(nodes or self.nodes, **kwargs)
  File "/work/test/functional/test_framework/util.py", line 493, in sync_mempools
    "".join("\n  {!r}".format(m) for m in pool)))
AssertionError: Mempool sync timed out:
  set()
  {'33f55827231ee85c37ee41bc6c43de907d9f2552bdb8fc00a022a83ad547b9f3'}
2021-01-09T22:13:36.415000Z TestFramework (INFO): Stopping nodes
2021-01-09T22:13:36.717000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20210109_220945/wallet_importdescriptors_511
2021-01-09T22:13:36.717000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20210109_220945/wallet_importdescriptors_511/test_framework.log
2021-01-09T22:13:36.718000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20210109_220945/wallet_importdescriptors_511' to consolidate all logs

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

Fix node version message (relay and extra entropy were reverted)

Harbormaster completed remote builds in B14685: Diff 26827.Jan 9 2021, 23:33

deadalnix added a child revision: D8862: [avalanche] Initiate avalanche when the service bit is present and we have a stake..Jan 9 2021, 23:44

majcosta requested changes to this revision.Jan 10 2021, 08:53

majcosta added a subscriber: majcosta.

majcosta added inline comments.

src/net.cpp
474	`const`
1160	`const`
3011	since it's a POD passed by value I don't see the need for the `In` suffix. seems like it shouldn't be mutated either so I'd make it `const` here so the compiler yells at whoever does it, but up to you
src/net.h
858	going by the devnotes.md, shouldn't this be `m_remoteHostNonce` or something?
860	ditto
861–866	won't die on this hill, but an empty line before and after will make this block even clearer
1034	ditto for hungarian notation and no `m_` prefix
src/net_processing.cpp
552	`const`

This revision now requires changes to proceed.Jan 10 2021, 08:53

The requested change differ from the existing code style for a benefit that is at best elusive.

src/net.cpp
3011	The in is there to avoid shadowing.
src/net.h
858	This doesn't match the style of the modified code.
1034	This is not consistent with the surrounding code.