since tpo gitweb and gitolite are going to be removed.

Closes team#307

Micah Elizabeth Scott authored Jan 18, 2023 and Damian Johnson committed Jan 26, 2023

The code to run __ainit__ constructors in the Synchronous mixin was
using the deprecated asyncio.coroutine() decorator, which was fully
removed in Python 3.11. Thankfully we can use the same approach for
ainit that this same class uses below for wrapping generators.

Fix from nyxnor...

  https://github.com/torproject/stem/pull/113

pragma31 made the good point that exit_used.py confusingly presents the
internal 1-hop circuits for fetching descriptors. Filtering those out.

  https://github.com/torproject/stem/pull/111

Honestly this is probably somewhat a step backward because if I return to Stem
I plan to substantially revert the asyncio migration, but for now simply fixing
the script. With the current codebase it fails with...

  % python exit_used.py
  Tracking requests for tor exits. Press 'enter' to end.

  stem/stem/control.py:4004: RuntimeWarning: coroutine 'Controller.get_circuit' was never awaited
    log.warn('Event listener raised an uncaught exception (%s): %s' % (exc, event))

Closes issue #109.  Long story short: a few names from collection are
now moved to collection.abc exclusively starting in Python 3.10. The
only name this app uses from there that was moved is
`collections.Iterable`.  Python versions starting from 3.3 support both
`collections.Iterable` and `collections.abc.Iterable` as the way to refer to
this class, which Python 3.10 being the first one to drop
`collections.Iterable`.  So.. we just work around this API quirk
and always refer ot it as `collections.abc.Iterable`.

Closes #105 

Since python 3.2: https://docs.python.org/3/library/stdtypes.html#int.from_bytes

proukornew authored Jun 13, 2021 and Damian Johnson committed Jun 12, 2021

Slightly modified from https://github.com/torproject/stem/pull/101

Mostly minor stylistic adjustments. The only functional bit is correcting a
unit test failure...

	======================================================================
	FAIL: test_identity_key_from_address
	----------------------------------------------------------------------
	ValueError: '55555555555555555555555555555555555555555555555555555555.onion' isn't a valid hidden service v3 address

	During handling of the above exception, another exception occurred:

	Traceback (most recent call last):
  	File "/home/atagar/Desktop/stem/test/unit/descriptor/hidden_service_v3.py", line 220, in test_identity_key_from_address
    	self.assertRaisesWith(ValueError, 'Bad checksum (expected def7 but was 842e)', HiddenServiceDescriptorV3.identity_key_from_address, '5' * 56)
  	File "/home/atagar/Desktop/stem/stem/util/test_tools.py", line 249, in assertRaisesWith
    	return self.assertRaisesRegexp(exc_type, '^%s$' % re.escape(exc_msg), *args, **kwargs)
	AssertionError: "^Bad\ checksum\ \(expected\ def7\ but\ was\ 842e\)$" does not match "'55555555555555555555555555555555555555555555555555555555.onion' isn't a valid hidden service v3 address"

----------------------------------------------------------------------

Tor now reports versions of libraries and the compilers use to build
it.  Only the first line has the Tor version.

First step to remove our asyncio metaprogramming...

  https://github.com/torproject/stem/issues/77

Our Query class now provides a run method for synchronous users, and run_async
for asyncio. This also adds a stop method that can cancel our download.

Python's unittest module added the first line of our docstrings to the test
output...

  https://docs.python.org/3/library/unittest.html#unittest.TestCase.shortDescription

This broke our code that provides a command to re-run test failures. That is to
say, rather than presenting...

  TESTING FAILED (9 seconds)
    [UNIT TEST] test_download (test.unit.descriptor.remote.TestDescriptorDownloader) ... FAIL

  You can re-run just these tests with:

    ./run_tests.py --unit --test descriptor.remote

... we lacked the last line.

Our Query class cached the bytes we download rather than parsed descriptors.
This could be advantagous if a user downloads descriptors without caring
about the results (unlikely), but otherwise it's all downside...

  * Slower: The Query class downloads asynchronously so we can parallelize.
    By parsing when the results are requested we serialize that part of the
    runtime.

  * Memory: Caching bytes reduced the upfront memory usage, but multiplies
    it upon retrieving the results because we create fresh Descriptor
    objects upon each invocation.

  * Duplication: Each invocation of our run method re-parsed the descriptors.
    For larger documents like the consensus this duplicates a lot of work.

  * Complexity: Caching bytes needlessly complicated the run method.

This class method initializes an asynchronous process. There's no benefit in
this method itself being asynchronous. This only was to ensure there's a loop
available, but our Synchronous parent provides that.

Reading our Query class I became worried that our 'start' argument fails to get
honored because the start() method is asynchronous (so its invocation returns a
coroutine rather than running it).

I was wrong. It works because as a Synchronous subclass our metaprogramming
converts the call. That said, this is none the less a good thing to check.

When we dropped python 2.x support this function became moot.

Oops. Actually, our last release allowed floats (and our 'decimal' argument
doesn't make terribly much sense otherwise). We *did* have a bug in that
remainder values that are less than 1 crashed us (corrected by commit c90e87ca),
but we shouldn't have addressed that by integer normalization.

Regression caught thanks to Nyx's tests...

  ======================================================================
  FAIL: test_draw_line (panel.connection.TestConnectionPanel)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/home/atagar/Desktop/nyx/test/__init__.py", line 59, in wrapped
      return func(self, *args, **kwargs)
    File "/usr/lib/python3.8/unittest/mock.py", line 1325, in patched
      return func(*newargs, **newkeywargs)
    File "/home/atagar/Desktop/nyx/test/panel/connection.py", line 228, in test_draw_line
      self.assertEqual(expected, rendered.content)
  AssertionError: ' 75.119.206.243:22 (de)      -->  82.121.9.9:3531              15.4s (INBOUND)' != ' 75.119.206.243:22 (de)      -->  82.121.9.9:3531              14.9s (INBOUND)'
  -  75.119.206.243:22 (de)      -->  82.121.9.9:3531              15.4s (INBOUND)
  ?                                                                 ^ ^
  +  75.119.206.243:22 (de)      -->  82.121.9.9:3531              14.9s (INBOUND)
  ?                                                                 ^ ^

When a CollecTor file is already within our cache we validate its hash against
CollecTor's index. However, files we wrote to disk are decompressed by default
so the hash naturally mismatches.

  https://github.com/torproject/stem/issues/76

Reproduced this issue with the following script...

  import stem.descriptor.collector

  collector = stem.descriptor.collector.get_instance()
  desc_file = collector.files(descriptor_type = 'server-descriptor')[0]  # pick any arbitrary file

  print('Number of descriptors (first read): %s' % len(list(desc_file.read(directory = '/tmp/collector_cache'))))
  print('Number of descriptors (second read): %s' % len(list(desc_file.read(directory = '/tmp/collector_cache'))))

Before...

  % python demo.py
  Number of descriptors (first read): 3112
  Traceback (most recent call last):
    File "scrap.py", line 8, in <module>
      print('Number of descriptors (second read): %s' % len(list(desc_file.read(directory = '/tmp/collector_cache'))))
    File "/home/atagar/Desktop/stem/stem/descriptor/collector.py", line 273, in read
      path = self.download(directory, True, timeout, retries)
    File "/home/atagar/Desktop/stem/stem/descriptor/collector.py", line 335, in download
      raise OSError("%s already exists but mismatches CollecTor's checksum (expected: %s, actual: %s)" % (path, expected_hash, actual_hash))
  OSError: /tmp/collector_cache/server-descriptors-2005-12.tar already exists but mismatches CollecTor's checksum (expected: bf700d8b6143e310219b2ce2810abd82f94bc295c7f08e9f1a88989562e33b2f, actual: 32a5ea8fd761e5967fbb8d399742f0da7cbb1c79c1539f2e58cad2e668462652)

After...

  % python demo.py
  Number of descriptors (first read): 3112
  Number of descriptors (second read): 3112

We can either solve this by dropping the hash check or caching compressed
archives. Initially I leaned toward the former to expedite cache reads, but on
reflection the later is conceptually simpler. Essentially, is this a network
cache or a read cache? A network cache is safer in that if CollecTor replaces a
file (but keeps the same filename) this will catch the change.

Building now requires the typehint plugin. Noting that and the pip command to
install it and sphinx.

The issue that prevented us from testing this was fixed...

  19:28 <+atagar> dgoulet: What is the status of
    tpo/core/tor#40089 ? The last
    comment discusses another review, but then the 'Needs Review' tag was
    removed.
  19:29 <+dgoulet> atagar: so that has been merged upstream it appears
    and waiting for backport down to 043
  19:31 <+atagar> Ahh, great - thanks. Per chance do you know what tor
    version has the fix? If so then I can check for it in our tests
    (https://gitweb.torproject.org/stem.git/tree/test/integ/control/controller.py#n1650).
  19:34 <+dgoulet> atagar: not sure exactly which one but I think 0.4.4.5 +
  19:36 <+atagar> Thanks, I'll conditional the assertion on that version.
    If it makes any test runs sad just let me know.
  19:36 <+dgoulet> ack thanks

Upgrading to pycodestyle 2.6.0 produced a few new warnings...

  STATIC CHECKS
  * /home/atagar/Desktop/stem/stem/descriptor/certificate.py
    line 257  - undefined name 'cryptography'            | def __init__(self, cert_type: Optional['stem.client.datatype.CertType'] = None, expiration: Optional[datetime.datetime] = None, key_type: Optional[int] = None, key: Optional[bytes] = None, extensions: Optional[Sequence['stem.descriptor.certificate.Ed25519Extension']] = None, signature: Optional[bytes] = None, signing_key: Optional['cryptography.hazmat.primitives.asymmetric.ed25519.Ed25519PrivateKey'] = None) -> None:  # type: ignore

  * /home/atagar/Desktop/stem/stem/descriptor/hidden_service.py
    line 288  - E741 ambiguous variable name 'l'         | link_specifiers = link_count + b''.join([l.pack() for l in self.link_specifiers])

  * /home/atagar/Desktop/stem/stem/interpreter/commands.py
    line 272  - E741 ambiguous variable name 'l'         | lines += [format(l, *STANDARD_OUTPUT) for l in str(desc).splitlines()]

  * /home/atagar/Desktop/stem/stem/util/__init__.py
    line 84   - undefined name 'cryptography'            | def _pubkey_bytes(key: Union['cryptography.hazmat.primitives.asymmetric.ed25519.Ed25519PrivateKey', 'cryptography.hazmat.primitives.asymmetric.ed25519.Ed25519PublicKey', 'cryptography.hazmat.primitives.asymmetric.x25519.X25519PrivateKey', 'cryptography.hazmat.primitives.asymmetric.x25519.X25519PublicKey']) -> bytes:  # type: ignore

  * /home/atagar/Desktop/stem/test/unit/response/events.py
    line 540  - 'stem.control.Controller' imported but unused | from stem.control import Controller, EventType

  * stem/client/__init__.py
    line 322  - unused 'type: ignore' comment

  * stem/control.py
    line 2493 - Unpacking a string is disallowed  [misc]
    line 2511 - Unpacking a string is disallowed  [misc]
    line 2516 - Unpacking a string is disallowed  [misc]

  * stem/directory.py
    line 270  - Unpacking a string is disallowed  [misc]
    line 271  - Unpacking a string is disallowed  [misc]
    line 436  - Unpacking a string is disallowed  [misc]

When running our tests with Python 3.8.5 I'm getting the following warnings...

	/home/atagar/Desktop/stem/test/unit/response/events.py:553: RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited
	  controller.authenticate()
	RuntimeWarning: Enable tracemalloc to get the object allocation traceback

Our mock spec doesn't provide anything here, so simply omitting it to silence
these.

Functions like size_label() and time_label() are documented as taking an int
argument, but getting a float shouldn't make them choke. Especially in such an
unhelpful way...

  ======================================================================
  ERROR: test_time_label
  Checks the time_label() function.
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/home/atagar/Desktop/stem/test/unit/util/str_tools.py", line 99, in test_time_label
      self.assertEqual('0s', str_tools.time_label(0.1))
    File "/home/atagar/Desktop/stem/stem/util/str_tools.py", line 366, in time_label
      return _get_label(TIME_UNITS, seconds, decimal, is_long)
    File "/home/atagar/Desktop/stem/stem/util/str_tools.py", line 595, in _get_label
      raise ValueError('BUG: %s should always be divisible by a unit (%s)' % (count, str(units)))
  ValueError: BUG: 0.1 should always be divisible by a unit (((86400.0, 'd', ' day'), (3600.0, 'h', ' hour'), (60.0, 'm', ' minute'), (1.0, 's', ' second')))

Oops, forgot to await one of our internal coroutine calls...

  stem/control.py:1236: RuntimeWarning: coroutine 'Controller.get_info' was never awaited

Most examples lacked test coverage. We tested some in 2012 (tutorial.py) and
2014 (tutorial_examples.py), but they weren't updated as new code got added.

Aside from testing and fixing just about everything, this branch asserts that
new examples have test coverage so we stay atop this.

Damn I need to rewrite our connection module. As indicated by the TODO comment,
this method copies some of Synchronous' init. No surprise then that when I
fixed Synchronous' iterators the fix didn't get propagated here.

Caught thanks to toralf noticing that our relay_connections.py example didn't
work...

  % python3.7 relay_connections.py
   0.4.5.0-alpha-dev   uptime: 14:27   flags: none

  Traceback (most recent call last):
    File "relay_connections.py", line 130, in <module>
      main()
    File "relay_connections.py", line 50, in main
      for desc in controller.get_network_statuses():
  TypeError: 'async_generator' object is not iterable

Our '--help' assertion pointlessly downloaded server descriptors (since we
negate sys.exit). This reduces this test's runtime from 211 to 1 ms.