Fixes cherry-picked from fix_keyring_tests. (Chia-Network#8766)

* Fixes cherry-picked from fix_keyring_tests.

* Logging for the failing test_using_legacy_cryptfilekeyring test

* See if reordering the tests makes a difference.

* Revert "See if reordering the tests makes a difference."

This reverts commit b538078.

* Log changes to the _configure_legacy_backend patch

* Fixed patching

* Checking if something isn't cleaned up properly

* Revert "Checking if something isn't cleaned up properly"

This reverts commit ce995ba.

* Revert "Fixed patching"

This reverts commit 66a70a1.

* Revert "Log changes to the _configure_legacy_backend patch"

This reverts commit 26791a4.

* Revert "Logging for the failing test_using_legacy_cryptfilekeyring test"

This reverts commit 4fd4873.

* Move test_keyring_wrapper.py into a standalone test

* Small refactorings/changes per feedback

.github/workflows/build-test-macos-core-util.yml

@@ -100,7 +100,8 @@ jobs:
     - name: Test core-util code with pytest
       run: |
         . ./activate
-        ./venv/bin/py.test tests/core/util/test_keychain.py tests/core/util/test_lru_cache.py tests/core/util/test_significant_bits.py tests/core/util/test_streamable.py tests/core/util/test_type_checking.py -s -v --durations 0
+        ./venv/bin/py.test tests/core/util/test_file_keyring_synchronization.py tests/core/util/test_keychain.py tests/core/util/test_lru_cache.py tests/core/util/test_significant_bits.py tests/core/util/test_streamable.py tests/core/util/test_type_checking.py -s -v --durations 0
+        ./venv/bin/py.test tests/core/util/test_keyring_wrapper.py -s -v --durations 0
 #
 # THIS FILE IS GENERATED. SEE https://github.com/Chia-Network/chia-blockchain/tree/main/tests#readme
 #

.github/workflows/build-test-ubuntu-core-util.yml

@@ -105,7 +105,8 @@ jobs:
     - name: Test core-util code with pytest
       run: |
         . ./activate
-        ./venv/bin/py.test tests/core/util/test_keychain.py tests/core/util/test_lru_cache.py tests/core/util/test_significant_bits.py tests/core/util/test_streamable.py tests/core/util/test_type_checking.py -s -v --durations 0
+        ./venv/bin/py.test tests/core/util/test_file_keyring_synchronization.py tests/core/util/test_keychain.py tests/core/util/test_lru_cache.py tests/core/util/test_significant_bits.py tests/core/util/test_streamable.py tests/core/util/test_type_checking.py -s -v --durations 0
+        ./venv/bin/py.test tests/core/util/test_keyring_wrapper.py -s -v --durations 0
 
 
 #

chia/util/dump_keyring.py

@@ -73,7 +73,7 @@ def dump(keyring_file, full_payload: bool, passphrase_file: Optional[TextIOWrapp
     if passphrase_file is not None:
         passphrase = read_passphrase_from_file(passphrase_file)
 
-    keyring = DumpKeyring(keyring_file)
+    keyring = DumpKeyring(Path(keyring_file))
 
     if full_payload:
         keyring.load_outer_payload()
@@ -99,6 +99,47 @@ def dump(keyring_file, full_payload: bool, passphrase_file: Optional[TextIOWrapp
             break
 
 
+def dump_to_string(
+    keyring_file, full_payload: bool, passphrase_file: Optional[TextIOWrapper], pretty_print: bool
+) -> str:
+    saved_passphrase: Optional[str] = KeyringWrapper.get_shared_instance().get_master_passphrase_from_credential_store()
+    passphrase: str = saved_passphrase or DEFAULT_PASSPHRASE_IF_NO_MASTER_PASSPHRASE
+    prompt: str = get_passphrase_prompt(str(keyring_file))
+    data: Dict[str, Any] = {}
+
+    print(f"Attempting to dump contents of keyring file: {keyring_file}\n")
+
+    if passphrase_file is not None:
+        passphrase = read_passphrase_from_file(passphrase_file)
+
+    keyring = DumpKeyring(Path(keyring_file))
+
+    if full_payload:
+        keyring.load_outer_payload()
+        data = keyring.outer_payload_cache
+
+    s: str = ""
+    for i in range(5):
+        try:
+            keyring.load_keyring(passphrase)
+            if len(data) > 0:
+                data["data"] = keyring.payload_cache
+            else:
+                data = keyring.payload_cache
+
+            if pretty_print:
+                s = yaml.dump(data)
+            else:
+                s = str(data)
+            break
+        except (ValueError, InvalidTag):
+            passphrase = prompt_for_passphrase(prompt)
+        except Exception as e:
+            print(f"Unhandled exception: {e}")
+            break
+    return s
+
+
 def main():
     colorama.init()
     dump()  # pylint: disable=no-value-for-parameter

chia/util/file_keyring.py

@@ -36,9 +36,7 @@ def loads_keyring(method):
 
     @wraps(method)
     def inner(self, *args, **kwargs):
-        # Watchdog's event dispatch timing is unreliable on macOS. Force a file modification time check for macOS.
-        if sys.platform == "darwin":
-            self.check_if_keyring_file_modified()
+        self.check_if_keyring_file_modified()
 
         # Check the outer payload for 'data', and check if we have a decrypted cache (payload_cache)
         with self.load_keyring_lock:

tests/core/util/test_file_keyring_synchronization.py

@@ -2,16 +2,16 @@
 import logging
 import os
 import pytest
-import random
-import unittest
 
 from chia.util.file_keyring import acquire_writer_lock, FileKeyring, FileKeyringLockTimeout
 from chia.util.keyring_wrapper import KeyringWrapper
+from chia.util.path import mkdir
 from multiprocessing import Pool, TimeoutError
 from pathlib import Path
 from sys import platform
 from tests.util.keyring import TempKeyring, using_temp_file_keyring
 from time import sleep
+from typing import List
 
 
 log = logging.getLogger(__name__)
@@ -20,18 +20,31 @@
 DUMMY_SLEEP_VALUE = 2
 
 
-def dummy_set_passphrase(service, user, passphrase, keyring_path):
+def dummy_set_passphrase(service, user, passphrase, keyring_path, index, num_workers):
     with TempKeyring(existing_keyring_path=keyring_path, delete_on_cleanup=False):
         if platform == "linux" or platform == "win32" or platform == "cygwin":
             # FileKeyring's setup_keyring_file_watcher needs to be called explicitly here,
             # otherwise file events won't be detected in the child process
             KeyringWrapper.get_shared_instance().keyring.setup_keyring_file_watcher()
 
-        KeyringWrapper.get_shared_instance().set_passphrase(service=service, user=user, passphrase=passphrase)
+        # Write out a file indicating this process is ready to begin
+        ready_file_path: Path = Path(keyring_path).parent / "ready" / f"{index}.ready"
+        with open(ready_file_path, "w") as f:
+            f.write(f"{os.getpid()}\n")
+
+        # Wait up to 30 seconds for all processes to indicate readiness
+        start_file_path: Path = Path(ready_file_path.parent) / "start"
+        remaining_attempts = 120
+        while remaining_attempts > 0:
+            if start_file_path.exists():
+                break
+            else:
+                sleep(0.25)
+                remaining_attempts -= 1
 
-        # Wait a short while between writing and reading. Without proper locking, this helps ensure
-        # the concurrent processes get into a bad state
-        sleep(random.random() * 10 % 3)
+        assert remaining_attempts >= 0
+
+        KeyringWrapper.get_shared_instance().set_passphrase(service=service, user=user, passphrase=passphrase)
 
         found_passphrase = KeyringWrapper.get_shared_instance().get_passphrase(service, user)
         if found_passphrase != passphrase:
@@ -40,6 +53,12 @@ def dummy_set_passphrase(service, user, passphrase, keyring_path):
                 f"get_passphrase: {found_passphrase}"  # lgtm [py/clear-text-logging-sensitive-data]
                 f", expected: {passphrase}"  # lgtm [py/clear-text-logging-sensitive-data]
             )
+
+        # Write out a file indicating this process has completed its work
+        finished_file_path: Path = Path(keyring_path).parent / "finished" / f"{index}.finished"
+        with open(finished_file_path, "w") as f:
+            f.write(f"{os.getpid()}\n")
+
         assert found_passphrase == passphrase
 
 
@@ -70,21 +89,63 @@ def child_writer_dispatch(func, lock_path: Path, timeout: int, max_iters: int):
         raise e
 
 
-class TestFileKeyringSynchronization(unittest.TestCase):
+def poll_directory(dir: Path, expected_entries: int, max_attempts: int, interval: float = 1.0) -> bool:
+    found_all: bool = False
+    remaining_attempts: int = 30
+    while remaining_attempts > 0:
+        entries: List[os.DirEntry] = list(os.scandir(dir))
+        if len(entries) < expected_entries:  # Expecting num_workers of dir entries
+            log.warning(f"Polling not complete: {len(entries)} of {expected_entries} entries found")
+            sleep(1)
+            remaining_attempts -= 1
+        else:
+            found_all = True
+            break
+    return found_all
+
+
+class TestFileKeyringSynchronization:
 
     # When: using a new empty keyring
     @using_temp_file_keyring()
     def test_multiple_writers(self):
         num_workers = 20
         keyring_path = str(KeyringWrapper.get_shared_instance().keyring.keyring_path)
         passphrase_list = list(
-            map(lambda x: ("test-service", f"test-user-{x}", f"passphrase {x}", keyring_path), range(num_workers))
+            map(
+                lambda x: ("test-service", f"test-user-{x}", f"passphrase {x}", keyring_path, x, num_workers),
+                range(num_workers),
+            )
         )
 
+        # Create a directory for each process to indicate readiness
+        ready_dir: Path = Path(keyring_path).parent / "ready"
+        mkdir(ready_dir)
+
+        finished_dir: Path = Path(keyring_path).parent / "finished"
+        mkdir(finished_dir)
+
         # When: spinning off children to each set a passphrase concurrently
         with Pool(processes=num_workers) as pool:
             res = pool.starmap_async(dummy_set_passphrase, passphrase_list)
-            res.get(timeout=40)  # 40 second timeout to prevent a bad test from spoiling the fun
+
+            # Wait up to 30 seconds for all processes to indicate readiness
+            assert poll_directory(ready_dir, num_workers, 30) is True
+
+            log.warning(f"Test setup complete: {num_workers} workers ready")
+
+            # Signal that testing should begin
+            start_file_path: Path = ready_dir / "start"
+            with open(start_file_path, "w") as f:
+                f.write(f"{os.getpid()}\n")
+
+            # Wait up to 30 seconds for all processes to indicate completion
+            assert poll_directory(finished_dir, num_workers, 30) is True
+
+            log.warning(f"Finished: {num_workers} workers finished")
+
+            # Collect results
+            res.get(timeout=10)  # 10 second timeout to prevent a bad test from spoiling the fun
 
         # Expect: parent process should be able to find all passphrases that were set by the child processes
         for item in passphrase_list:

tests/core/util/test_keyring_wrapper.py

@@ -1,5 +1,5 @@
 import logging
-import unittest
+import pytest
 
 from chia.util.keyring_wrapper import KeyringWrapper, DEFAULT_PASSPHRASE_IF_NO_MASTER_PASSPHRASE
 from pathlib import Path
@@ -9,14 +9,12 @@
 log = logging.getLogger(__name__)
 
 
-class TestKeyringWrapper(unittest.TestCase):
-    def setUp(self) -> None:
-        return super().setUp()
-
-    def tearDown(self) -> None:
+class TestKeyringWrapper:
+    @pytest.fixture(autouse=True, scope="function")
+    def setup_keyring_wrapper(self):
+        yield
         KeyringWrapper.cleanup_shared_instance()
         assert KeyringWrapper.get_shared_instance(create_if_necessary=False) is None
-        return super().tearDown()
 
     def test_shared_instance(self):
         """

tests/util/keyring.py

@@ -3,24 +3,34 @@
 import tempfile
 
 from chia.util.file_keyring import FileKeyring
-from chia.util.keychain import Keychain
+from chia.util.keychain import Keychain, default_keychain_service, default_keychain_user, get_private_key_user
 from chia.util.keyring_wrapper import KeyringWrapper
 from functools import wraps
 from keyring.util import platform_
 from keyrings.cryptfile.cryptfile import CryptFileKeyring  # pyright: reportMissingImports=false
 from pathlib import Path
-from typing import Optional
+from typing import Any, Optional
 from unittest.mock import patch
 
 
-def create_empty_cryptfilekeyring():
+def create_empty_cryptfilekeyring() -> CryptFileKeyring:
     """
     Create an empty legacy keyring
     """
     crypt_file_keyring = CryptFileKeyring()
     fd = os.open(crypt_file_keyring.file_path, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600)
     os.close(fd)
     assert Path(crypt_file_keyring.file_path).exists()
+    return crypt_file_keyring
+
+
+def add_dummy_key_to_cryptfilekeyring(crypt_file_keyring: CryptFileKeyring):
+    """
+    Add a fake key to the CryptFileKeyring
+    """
+    crypt_file_keyring.keyring_key = "your keyring password"  # type: ignore
+    user: str = get_private_key_user(default_keychain_user(), 0)
+    crypt_file_keyring.set_password(default_keychain_service(), user, "abc123")
 
 
 def setup_mock_file_keyring(mock_configure_backend, temp_file_keyring_dir, populate=False):
@@ -77,9 +87,7 @@ def using_temp_file_keyring_and_cryptfilekeyring(populate=False):
     def outer(method):
         @wraps(method)
         def inner(self, *args, **kwargs):
-            with TempKeyring(populate=populate):
-                # Create an empty legacy keyring
-                create_empty_cryptfilekeyring()
+            with TempKeyring(populate=populate, setup_cryptfilekeyring=True):
                 return method(self, *args, **kwargs)
 
         return inner
@@ -90,24 +98,33 @@ def inner(self, *args, **kwargs):
 class TempKeyring:
     def __init__(
         self,
+        *,
         user: str = "testing-1.8.0",
         service: str = "testing-chia-1.8.0",
         populate: bool = False,
+        setup_cryptfilekeyring: bool = False,
         existing_keyring_path: str = None,
         delete_on_cleanup: bool = True,
         use_os_credential_store: bool = False,
     ):
         self.keychain = self._patch_and_create_keychain(
-            user, service, populate, existing_keyring_path, use_os_credential_store
+            user=user,
+            service=service,
+            populate=populate,
+            existing_keyring_path=existing_keyring_path,
+            use_os_credential_store=use_os_credential_store,
+            setup_cryptfilekeyring=setup_cryptfilekeyring,
         )
         self.delete_on_cleanup = delete_on_cleanup
         self.cleaned_up = False
 
     def _patch_and_create_keychain(
         self,
+        *,
         user: str,
         service: str,
         populate: bool,
+        setup_cryptfilekeyring: bool,
         existing_keyring_path: Optional[str],
         use_os_credential_store: bool,
     ):
@@ -130,9 +147,11 @@ def _patch_and_create_keychain(
         mock_configure_backend = mock_configure_backend_patch.start()
         setup_mock_file_keyring(mock_configure_backend, temp_dir, populate=populate)
 
-        mock_configure_legacy_backend_patch = patch.object(KeyringWrapper, "_configure_legacy_backend")
-        mock_configure_legacy_backend = mock_configure_legacy_backend_patch.start()
-        mock_configure_legacy_backend.return_value = None
+        mock_configure_legacy_backend_patch: Any = None
+        if setup_cryptfilekeyring is False:
+            mock_configure_legacy_backend_patch = patch.object(KeyringWrapper, "_configure_legacy_backend")
+            mock_configure_legacy_backend = mock_configure_legacy_backend_patch.start()
+            mock_configure_legacy_backend.return_value = None
 
         mock_data_root_patch = patch.object(platform_, "data_root")
         mock_data_root = mock_data_root_patch.start()
@@ -141,6 +160,10 @@ def _patch_and_create_keychain(
         # We don't want CryptFileKeyring finding the real legacy keyring
         mock_data_root.return_value = temp_dir
 
+        if setup_cryptfilekeyring is True:
+            crypt_file_keyring = create_empty_cryptfilekeyring()
+            add_dummy_key_to_cryptfilekeyring(crypt_file_keyring)
+
         keychain = Keychain(user=user, service=service)
         keychain.keyring_wrapper = KeyringWrapper(keys_root_path=Path(temp_dir))
 
@@ -178,7 +201,8 @@ def cleanup(self):
         self.keychain._mock_supports_keyring_passphrase_patch.stop()
         self.keychain._mock_supports_os_passphrase_storage_patch.stop()
         self.keychain._mock_configure_backend_patch.stop()
-        self.keychain._mock_configure_legacy_backend_patch.stop()
+        if self.keychain._mock_configure_legacy_backend_patch is not None:
+            self.keychain._mock_configure_legacy_backend_patch.stop()
         self.keychain._mock_data_root_patch.stop()
 
         self.cleaned_up = True