From 72a0954797f3d9553a72a3bb419a05b624ae1066 Mon Sep 17 00:00:00 2001 From: Benjamin Himes Date: Thu, 28 Nov 2024 13:10:50 +0200 Subject: [PATCH 1/9] Fixes timeout, adds unit test. --- bittensor/core/extrinsics/utils.py | 105 ++++++++++------------ tests/unit_tests/extrinsics/test_utils.py | 34 +++++++ 2 files changed, 83 insertions(+), 56 deletions(-) create mode 100644 tests/unit_tests/extrinsics/test_utils.py diff --git a/bittensor/core/extrinsics/utils.py b/bittensor/core/extrinsics/utils.py index 6af14b25fb..46b25e67ee 100644 --- a/bittensor/core/extrinsics/utils.py +++ b/bittensor/core/extrinsics/utils.py @@ -1,5 +1,7 @@ """Module with helper functions for extrinsics.""" +from concurrent.futures import ThreadPoolExecutor +import os import threading from typing import TYPE_CHECKING @@ -12,14 +14,7 @@ from substrateinterface import SubstrateInterface, ExtrinsicReceipt from scalecodec.types import GenericExtrinsic - -class _ThreadingTimeoutException(Exception): - """ - Exception raised for timeout. Different from TimeoutException because this also triggers - a websocket failure. This exception should only be used with `threading` timer.. - """ - - pass +EXTRINSIC_SUBMISSION_TIMEOUT = os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200) def submit_extrinsic( @@ -50,55 +45,53 @@ def submit_extrinsic( extrinsic_hash = extrinsic.extrinsic_hash starting_block = substrate.get_block() - def _handler(): - """ - Timeout handler for threading. Will raise a TimeoutError if timeout is exceeded. - """ - logging.error("Timed out waiting for extrinsic submission.") - raise _ThreadingTimeoutException - - # sets a timeout timer for the next call to 200 seconds - # will raise a _ThreadingTimeoutException if it reaches this point - timer = threading.Timer(200, _handler) - - try: - timer.start() - response = substrate.submit_extrinsic( - extrinsic, - wait_for_inclusion=wait_for_inclusion, - wait_for_finalization=wait_for_finalization, - ) - except SubstrateRequestException as e: - logging.error(format_error_message(e.args[0], substrate=substrate)) - # Re-rise the exception for retrying of the extrinsic call. If we remove the retry logic, the raise will need - # to be removed. - raise - - except _ThreadingTimeoutException: - after_timeout_block = substrate.get_block() - + timeout = EXTRINSIC_SUBMISSION_TIMEOUT + event = threading.Event() + + def submit(): + try: + response_ = substrate.submit_extrinsic( + extrinsic, + wait_for_inclusion=wait_for_inclusion, + wait_for_finalization=wait_for_finalization, + ) + except SubstrateRequestException as e: + logging.error(format_error_message(e.args[0], substrate=substrate)) + # Re-raise the exception for retrying of the extrinsic call. If we remove the retry logic, + # the raise will need to be removed. + raise + finally: + event.set() + return response_ + + with ThreadPoolExecutor(max_workers=1) as executor: response = None - for block_num in range( - starting_block["header"]["number"], - after_timeout_block["header"]["number"] + 1, - ): - block_hash = substrate.get_block_hash(block_num) - try: - response = substrate.retrieve_extrinsic_by_hash( - block_hash, f"0x{extrinsic_hash.hex()}" + future = executor.submit(submit) + if not event.wait(timeout): + logging.error("Timed out waiting for extrinsic submission.") + after_timeout_block = substrate.get_block() + + for block_num in range( + starting_block["header"]["number"], + after_timeout_block["header"]["number"] + 1, + ): + block_hash = substrate.get_block_hash(block_num) + try: + response = substrate.retrieve_extrinsic_by_hash( + block_hash, f"0x{extrinsic_hash.hex()}" + ) + except ExtrinsicNotFound: + continue + if response: + break + if response is None: + logging.error( + f"Extrinsic '0x{extrinsic_hash.hex()}' not submitted. " + f"Initially attempted to submit at block {starting_block['header']['number']}." ) - except ExtrinsicNotFound: - continue - if response: - break - finally: - timer.cancel() - - if response is None: - logging.error( - f"Extrinsic '0x{extrinsic_hash.hex()}' not submitted. " - f"Initially attempted to submit at block {starting_block['header']['number']}." - ) - raise SubstrateRequestException + raise SubstrateRequestException + + else: + response = future.result() return response diff --git a/tests/unit_tests/extrinsics/test_utils.py b/tests/unit_tests/extrinsics/test_utils.py new file mode 100644 index 0000000000..e70a6828e0 --- /dev/null +++ b/tests/unit_tests/extrinsics/test_utils.py @@ -0,0 +1,34 @@ +import time +from unittest.mock import MagicMock, patch + +import pytest +from substrateinterface.base import ( + SubstrateInterface, + GenericExtrinsic, + SubstrateRequestException, +) + +from bittensor.core.extrinsics import utils + + +def test_submit_extrinsic_timeout(): + timeout = 3 + + def wait(extrinsic, wait_for_inclusion, wait_for_finalization): + time.sleep(timeout + 1) + return True + + mock_substrate = MagicMock(autospec=SubstrateInterface) + mock_substrate.submit_extrinsic = wait + mock_extrinsic = MagicMock(autospec=GenericExtrinsic) + with patch.object(utils, "EXTRINSIC_SUBMISSION_TIMEOUT", timeout): + with pytest.raises(SubstrateRequestException): + utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) + + +def test_submit_extrinsic_success(): + mock_substrate = MagicMock(autospec=SubstrateInterface) + mock_substrate.submit_extrinsic.return_value = True + mock_extrinsic = MagicMock(autospec=GenericExtrinsic) + result = utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) + assert result is True From 7ac1829479fd66ae0d917ef5d14296f549efd956 Mon Sep 17 00:00:00 2001 From: Benjamin Himes Date: Thu, 28 Nov 2024 13:15:33 +0200 Subject: [PATCH 2/9] Add TODO --- bittensor/utils/networking.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bittensor/utils/networking.py b/bittensor/utils/networking.py index f47c88512e..21a9f1be7a 100644 --- a/bittensor/utils/networking.py +++ b/bittensor/utils/networking.py @@ -163,6 +163,8 @@ def get_formatted_ws_endpoint_url(endpoint_url: Optional[str]) -> Optional[str]: def ensure_connected(func): """Decorator ensuring the function executes with an active substrate connection.""" + # TODO we need to rethink the logic in this + def is_connected(substrate) -> bool: """Check if the substrate connection is active.""" sock = substrate.websocket.socket From 89b9c149455e7f561360bcce58e4be24713992e3 Mon Sep 17 00:00:00 2001 From: Benjamin Himes Date: Thu, 28 Nov 2024 13:34:20 +0200 Subject: [PATCH 3/9] Added more tests --- bittensor/core/extrinsics/utils.py | 2 +- tests/unit_tests/extrinsics/test_utils.py | 31 ++++++++++++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/bittensor/core/extrinsics/utils.py b/bittensor/core/extrinsics/utils.py index 46b25e67ee..b736c9e49d 100644 --- a/bittensor/core/extrinsics/utils.py +++ b/bittensor/core/extrinsics/utils.py @@ -14,7 +14,7 @@ from substrateinterface import SubstrateInterface, ExtrinsicReceipt from scalecodec.types import GenericExtrinsic -EXTRINSIC_SUBMISSION_TIMEOUT = os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200) +EXTRINSIC_SUBMISSION_TIMEOUT = int(os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200)) def submit_extrinsic( diff --git a/tests/unit_tests/extrinsics/test_utils.py b/tests/unit_tests/extrinsics/test_utils.py index e70a6828e0..cb5447ee06 100644 --- a/tests/unit_tests/extrinsics/test_utils.py +++ b/tests/unit_tests/extrinsics/test_utils.py @@ -1,6 +1,6 @@ import time from unittest.mock import MagicMock, patch - +import importlib import pytest from substrateinterface.base import ( SubstrateInterface, @@ -11,6 +11,11 @@ from bittensor.core.extrinsics import utils +@pytest.fixture +def set_extrinsics_timeout_env(monkeypatch): + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "3") + + def test_submit_extrinsic_timeout(): timeout = 3 @@ -32,3 +37,27 @@ def test_submit_extrinsic_success(): mock_extrinsic = MagicMock(autospec=GenericExtrinsic) result = utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) assert result is True + + +def test_submit_extrinsic_timeout_env(set_extrinsics_timeout_env): + importlib.reload(utils) + timeout = utils.EXTRINSIC_SUBMISSION_TIMEOUT + + def wait(extrinsic, wait_for_inclusion, wait_for_finalization): + time.sleep(timeout + 1) + return True + + mock_substrate = MagicMock(autospec=SubstrateInterface) + mock_substrate.submit_extrinsic = wait + mock_extrinsic = MagicMock(autospec=GenericExtrinsic) + with pytest.raises(SubstrateRequestException): + utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) + + +def test_submit_extrinsic_success_env(set_extrinsics_timeout_env): + importlib.reload(utils) + mock_substrate = MagicMock(autospec=SubstrateInterface) + mock_substrate.submit_extrinsic.return_value = True + mock_extrinsic = MagicMock(autospec=GenericExtrinsic) + result = utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) + assert result is True From fc714878356ff1047e5380bb5c9951193082dd61 Mon Sep 17 00:00:00 2001 From: Cameron Fairchild Date: Thu, 28 Nov 2024 09:35:55 -0500 Subject: [PATCH 4/9] catch value error and raise with nicer message --- bittensor/core/extrinsics/utils.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bittensor/core/extrinsics/utils.py b/bittensor/core/extrinsics/utils.py index b736c9e49d..cd10474e98 100644 --- a/bittensor/core/extrinsics/utils.py +++ b/bittensor/core/extrinsics/utils.py @@ -14,7 +14,12 @@ from substrateinterface import SubstrateInterface, ExtrinsicReceipt from scalecodec.types import GenericExtrinsic -EXTRINSIC_SUBMISSION_TIMEOUT = int(os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200)) +try: + EXTRINSIC_SUBMISSION_TIMEOUT = int(os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200)) +except ValueError: + raise ValueError( + "EXTRINSIC_SUBMISSION_TIMEOUT environment variable must be an integer." + ) def submit_extrinsic( From 1eb95174da1f1920f3ca33cc41b9d16c53765de8 Mon Sep 17 00:00:00 2001 From: Cameron Fairchild Date: Thu, 28 Nov 2024 09:52:06 -0500 Subject: [PATCH 5/9] parse as float and add check for negative numbers --- bittensor/core/extrinsics/utils.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bittensor/core/extrinsics/utils.py b/bittensor/core/extrinsics/utils.py index cd10474e98..967d8c5a6f 100644 --- a/bittensor/core/extrinsics/utils.py +++ b/bittensor/core/extrinsics/utils.py @@ -15,12 +15,15 @@ from scalecodec.types import GenericExtrinsic try: - EXTRINSIC_SUBMISSION_TIMEOUT = int(os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200)) + EXTRINSIC_SUBMISSION_TIMEOUT = float(os.getenv("EXTRINSIC_SUBMISSION_TIMEOUT", 200)) except ValueError: raise ValueError( - "EXTRINSIC_SUBMISSION_TIMEOUT environment variable must be an integer." + "EXTRINSIC_SUBMISSION_TIMEOUT environment variable must be a float." ) +if EXTRINSIC_SUBMISSION_TIMEOUT < 0: + raise ValueError("EXTRINSIC_SUBMISSION_TIMEOUT cannot be negative.") + def submit_extrinsic( substrate: "SubstrateInterface", From 7a2041b3b2d9d79ed35514a289b3e88ed6990bb8 Mon Sep 17 00:00:00 2001 From: Cameron Fairchild Date: Thu, 28 Nov 2024 09:52:23 -0500 Subject: [PATCH 6/9] add tests for new parsing, lower sleep time for test --- tests/unit_tests/extrinsics/test_utils.py | 49 +++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/extrinsics/test_utils.py b/tests/unit_tests/extrinsics/test_utils.py index cb5447ee06..424a1b9dc2 100644 --- a/tests/unit_tests/extrinsics/test_utils.py +++ b/tests/unit_tests/extrinsics/test_utils.py @@ -13,14 +13,14 @@ @pytest.fixture def set_extrinsics_timeout_env(monkeypatch): - monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "3") + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "1") def test_submit_extrinsic_timeout(): - timeout = 3 + timeout = 1 def wait(extrinsic, wait_for_inclusion, wait_for_finalization): - time.sleep(timeout + 1) + time.sleep(timeout + 0.01) return True mock_substrate = MagicMock(autospec=SubstrateInterface) @@ -61,3 +61,46 @@ def test_submit_extrinsic_success_env(set_extrinsics_timeout_env): mock_extrinsic = MagicMock(autospec=GenericExtrinsic) result = utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) assert result is True + + +def test_submit_extrinsic_timeout_env_float(monkeypatch): + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "1.45") # use float + + importlib.reload(utils) + timeout = utils.EXTRINSIC_SUBMISSION_TIMEOUT + + assert timeout == 1.45 # parsed correctly + + def wait(extrinsic, wait_for_inclusion, wait_for_finalization): + time.sleep(timeout + 0.3) # sleep longer by float + return True + + mock_substrate = MagicMock(autospec=SubstrateInterface) + mock_substrate.submit_extrinsic = wait + mock_extrinsic = MagicMock(autospec=GenericExtrinsic) + with pytest.raises(SubstrateRequestException): + utils.submit_extrinsic(mock_substrate, mock_extrinsic, True, True) + + +def test_import_timeout_env_parse(monkeypatch): + # int + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "1") + importlib.reload(utils) + assert utils.EXTRINSIC_SUBMISSION_TIMEOUT == 1 # parsed correctly + + # float + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "1.45") # use float + importlib.reload(utils) + assert utils.EXTRINSIC_SUBMISSION_TIMEOUT == 1.45 # parsed correctly + + # invalid + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "not_an_int") + with pytest.raises(ValueError) as e: + importlib.reload(utils) + assert "must be a float" in str(e.value) + + # negative + monkeypatch.setenv("EXTRINSIC_SUBMISSION_TIMEOUT", "-1") + with pytest.raises(ValueError) as e: + importlib.reload(utils) + assert "cannot be negative" in str(e.value) From 3461f36d26635b3ae2b897c22329b04fb62eedfb Mon Sep 17 00:00:00 2001 From: Cameron Fairchild Date: Thu, 28 Nov 2024 09:58:34 -0500 Subject: [PATCH 7/9] add default value test --- tests/unit_tests/extrinsics/test_utils.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit_tests/extrinsics/test_utils.py b/tests/unit_tests/extrinsics/test_utils.py index 424a1b9dc2..180296dfd7 100644 --- a/tests/unit_tests/extrinsics/test_utils.py +++ b/tests/unit_tests/extrinsics/test_utils.py @@ -104,3 +104,9 @@ def test_import_timeout_env_parse(monkeypatch): with pytest.raises(ValueError) as e: importlib.reload(utils) assert "cannot be negative" in str(e.value) + + # default (not checking exact value, just that it's a value) + monkeypatch.delenv("EXTRINSIC_SUBMISSION_TIMEOUT") + importlib.reload(utils) + assert isinstance(utils.EXTRINSIC_SUBMISSION_TIMEOUT, float) # has a default value + assert utils.EXTRINSIC_SUBMISSION_TIMEOUT > 0 # is positive From 9b7917694cd94c64c1dba7761e69a9947a0989f7 Mon Sep 17 00:00:00 2001 From: Cameron Fairchild Date: Thu, 28 Nov 2024 10:00:36 -0500 Subject: [PATCH 8/9] assert timeout low for test --- tests/unit_tests/extrinsics/test_utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit_tests/extrinsics/test_utils.py b/tests/unit_tests/extrinsics/test_utils.py index 180296dfd7..3cf0047b9e 100644 --- a/tests/unit_tests/extrinsics/test_utils.py +++ b/tests/unit_tests/extrinsics/test_utils.py @@ -42,6 +42,7 @@ def test_submit_extrinsic_success(): def test_submit_extrinsic_timeout_env(set_extrinsics_timeout_env): importlib.reload(utils) timeout = utils.EXTRINSIC_SUBMISSION_TIMEOUT + assert timeout < 5 # should be less than 5 seconds as taken from test env var def wait(extrinsic, wait_for_inclusion, wait_for_finalization): time.sleep(timeout + 1) From 1a2f2697393cd967a1213c708c4d3e8a4dde6bb6 Mon Sep 17 00:00:00 2001 From: Cameron Fairchild Date: Thu, 28 Nov 2024 10:00:43 -0500 Subject: [PATCH 9/9] chore: ruff --- tests/unit_tests/extrinsics/test_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/extrinsics/test_utils.py b/tests/unit_tests/extrinsics/test_utils.py index 3cf0047b9e..38a92e3dc2 100644 --- a/tests/unit_tests/extrinsics/test_utils.py +++ b/tests/unit_tests/extrinsics/test_utils.py @@ -109,5 +109,5 @@ def test_import_timeout_env_parse(monkeypatch): # default (not checking exact value, just that it's a value) monkeypatch.delenv("EXTRINSIC_SUBMISSION_TIMEOUT") importlib.reload(utils) - assert isinstance(utils.EXTRINSIC_SUBMISSION_TIMEOUT, float) # has a default value - assert utils.EXTRINSIC_SUBMISSION_TIMEOUT > 0 # is positive + assert isinstance(utils.EXTRINSIC_SUBMISSION_TIMEOUT, float) # has a default value + assert utils.EXTRINSIC_SUBMISSION_TIMEOUT > 0 # is positive