Page MenuHomePhabricator

[net] Add extra entropy to the version message
ClosedPublic

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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)

majcosta requested changes to this revision.Jan 10 2021, 08:53
majcosta added a subscriber: majcosta.
majcosta added inline comments.
src/net.cpp
474 ↗(On Diff #26827)

const

1160 ↗(On Diff #26827)

const

3011 ↗(On Diff #26827)

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 ↗(On Diff #26827)

going by the devnotes.md, shouldn't this be m_remoteHostNonce or something?

860 ↗(On Diff #26827)

ditto

861–866 ↗(On Diff #26827)

won't die on this hill, but an empty line before and after will make this block even clearer

1034 ↗(On Diff #26827)

ditto for hungarian notation and no m_ prefix

src/net_processing.cpp
552 ↗(On Diff #26827)

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 ↗(On Diff #26827)

The in is there to avoid shadowing.

src/net.h
858 ↗(On Diff #26827)

This doesn't match the style of the modified code.

1034 ↗(On Diff #26827)

This is not consistent with the surrounding code.

This revision is now accepted and ready to land.Jan 11 2021, 00:44