Implementing logic #7

ggleszczynski · 2024-10-31T18:37:44Z

No description provided.

ppolewicz · 2024-11-02T17:58:15Z

bt_ddos_shield/event_processor.py

+            print(f"MinerShieldEvent: {event.event_description}\nException happened:")
+            print(traceback.format_exc())
+        else:
+            print(f"MinerShieldEvent: {event.event_description}")


This prints, not logs, but I also think it is generally assembled wrong and won't work. No worries, you'll fix it when you start running it.

This is class mostly (and maybe only) for unit tests. But I change name to PrintingMinerShieldEventProcessor, because probably in the future there will be LoggingMinerShieldEventProcessor. Also it works - just run unit test.

And if we ever print that, we'd rather use stderr.
traceback.print_exc() can be a sufficient routine for that.

I've changed it already in future commits. But now I'm printing exception captured inside MinerShieldEvent. Probably not, but there is possibility, that it wouldn't be last exception thrown and print_exc calls sys.exception() inside to get the last one.

ppolewicz · 2024-11-02T18:00:58Z

bt_ddos_shield/miner_shield.py


        Args:
            validator_hotkey: The hotkey of the validator.
        """
+        self._add_task(MinerShieldBanValidatorTask(validator_hotkey))
+        pass


Suggested change

pass

ppolewicz · 2024-11-02T18:03:25Z

bt_ddos_shield/miner_shield.py

+    def _add_task(self, task):
+        """
+        Add task to task queue. It will be handled by _worker_function.
+        """
+        if not isinstance(task, MinerShieldTask):
+            raise Exception("Task is not instance of MinerShieldTask")


Suggested change

def _add_task(self, task):

"""

Add task to task queue. It will be handled by _worker_function.

"""

if not isinstance(task, MinerShieldTask):

raise Exception("Task is not instance of MinerShieldTask")

def _add_task(self, task: MinerShieldTask):

"""

Add task to task queue. It will be handled by _worker_function.

"""

Traditionally we don't implement type enforcement in the first lines of every method and function. Rather than running it in runtime, we have external tools for that which perform static analysis (whenever possible).

EncryptionManager was written using this check also in code - so I made this the same. And I forgot to specify type in parameter. Fixed - using trick with string literal, because there are no forward declarations in Python.

ppolewicz · 2024-11-02T18:08:45Z

bt_ddos_shield/miner_shield.py

+        if not isinstance(task, MinerShieldTask):
+            raise Exception("Task is not instance of MinerShieldTask")
+        if not self.run:
+            raise Exception("Shield is disabled")


In order to raise any exceptions you'll need to

class ShieldException(Exception): pass class ShieldIsDisabled(ShieldException): pass

and raise that - this way the user of the module will be able to catch the intended exceptions without catching every exception, which includes KeyError, ValueError etc

However, in this particular case you should raise a built-in ProgrammingError, which is a builtin for cases like that.

Exceptions added. And I added MinerShieldDisabledException, because _add_task is called at least from ban_validator and user can try to call it after stopping or before starting shield for some reason. Thats why I think MinerShieldDisabledException should be specified.

Except from above, I don't find anything like ProgrammingError (or ProgrammingException) - there is such class but in sqllite3 module.

ppolewicz · 2024-11-02T18:12:48Z

bt_ddos_shield/miner_shield.py

+            try_count = 1
+
+            while self.run:


Suggested change

try_count = 1

while self.run:

try_count: int = 0

while try_count <= self.options.max_retries:

I want tasks to be retried forever and I think this should be default behaviour - we can eventually talk about this. But I added new option to allow limited retrying.

ppolewicz · 2024-11-02T18:20:22Z

bt_ddos_shield/miner_shield.py

+        Initialize shield. Load state and initial validators set.
+        """
+        self.state_manager.get_state()
+        self.event_processor.add_event(MinerShieldEvent("State loaded"))


Rather than seeing

self.event_processor.add_event(MinerShieldEvent("State loaded"))

everywhere, I'd prefer to see

self._event("State loaded")

with

def _event(self, description: str, *args, **kwargs): return self.event_processor.add_event(self.event_class(description, *args, **kwargs))

and event_class=MinerShieldEvent in the constructor

Changed. But as for now I haven't added self.event_class, because it needs more changes - at least in AbstractMinerShieldEventProcessor, where type for event is hardcoded now. I think we can leave it for future.

ppolewicz · 2024-11-02T18:22:25Z

bt_ddos_shield/miner_shield.py

+
+        self.validators_manager.refresh_validators()
+        validators: dict[Hotkey, str] = self.validators_manager.get_validators()
+        self.event_processor.add_event(MinerShieldEvent(f"Validators refreshed, got {len(validators)} validators"))


Suggested change

self.event_processor.add_event(MinerShieldEvent(f"Validators refreshed, got {len(validators)} validators"))

self._event("Validators refreshed", validator_count=len(validators)))

the MinerShieldEvent class will need to get a little bit more complicated, but we'll be able to emit smart objects that can then be filtered, sorted etc

I will change this in future commits, because this needs more work, but I added TODO in my notes. I also think I need to talk how this should done the best way, because there is PrintingMinerShieldEventProcessor for example.

ppolewicz · 2024-11-02T18:34:10Z

bt_ddos_shield/miner_shield.py

+    def __init__(self, task_name: str):
+        """
+        Initialize task.
+
+        Args:
+            task_name: Short name of the task.
+        """
+        self.task_name = task_name
+
+    def handle(self, miner_shield: MinerShield):
+        """
+        Run task logic.
+
+        Args
+            miner_shield: Instance of MinerShield in which task is executed.
+        """
+        pass
+
+    def __repr__(self):
+        return self.task_name


Suggested change

def __init__(self, task_name: str):

"""

Initialize task.

Args:

task_name: Short name of the task.

"""

self.task_name = task_name

def handle(self, miner_shield: MinerShield):

"""

Run task logic.

Args

miner_shield: Instance of MinerShield in which task is executed.

"""

pass

def __repr__(self):

return self.task_name

NAME_DELETER = re.compile(r'^MinerShield(.*)Task$')

@abstractmethod

def run(self, miner_shield: MinerShield):

pass

def __repr__(self):

return self.NAME_DELETER.sub(r'\1', self.__class__.__name__)

ppolewicz · 2024-11-02T18:34:51Z

bt_ddos_shield/miner_shield.py

        pass
+
+
+class MinerShieldTask:


Suggested change

class MinerShieldTask:

class AbstractMinerShieldTask(ABC):

ppolewicz · 2024-11-02T18:43:25Z

bt_ddos_shield/miner_shield.py

+    def handle(self, miner_shield: MinerShield):
+        miner_shield._handle_initialize()


This could be refactored away into the abstract class too, but it's not worth it - we wouldn't save much anyway and it'd confuse static code analyzers

ppolewicz · 2024-11-08T10:12:04Z

README.md


 <!--
 @startuml ./assets/diagrams/CommunicationFlow
 participant Validator
 participant Miner
 participant GitHub
+participant AddressManager
 participant Storage
 participant Bittensor
 Validator -> Validator: Generate Validator key-pair
 Validator -> GitHub: Publish public key along with HotKey


Suggested change

Validator -> GitHub: Publish public key along with HotKey

Validator -> Bittensor: Publish public key along with HotKey

ppolewicz · 2024-11-08T10:12:33Z

README.md


 <!--
 @startuml ./assets/diagrams/CommunicationFlow
 participant Validator
 participant Miner
 participant GitHub
+participant AddressManager
 participant Storage


Suggested change

participant Storage

database Storage

ppolewicz · 2024-11-08T10:12:40Z

README.md


 <!--
 @startuml ./assets/diagrams/CommunicationFlow
 participant Validator
 participant Miner
 participant GitHub
+participant AddressManager
 participant Storage
 participant Bittensor


Suggested change

participant Bittensor

database Bittensor

ppolewicz · 2024-11-08T10:18:30Z

bt_ddos_shield/address.py

@@ -29,24 +29,27 @@ def __init__(self, address_id, address_type: AddressType, address: str, port: in
        self.address = address
        self.port = port

+    def __repr__(self):
+        return f"Address(id={self.address_id}, type={self.address_type}, address={self.address}:{self.port})"


Suggested change

return f"Address(id={self.address_id}, type={self.address_type}, address={self.address}:{self.port})"

return f"Address(id={self.address_id}, address={self.address}:{self.port})"

ppolewicz · 2024-11-08T10:43:52Z

bt_ddos_shield/miner_shield.py


            while self.run:
-                self.event_processor.add_event(MinerShieldEvent(f"Handling task {task}, try {try_count}"))
+                self._event(f"Handling task {task}, try {try_count}")


self._event("Handling task %{task}s, try %{try_count}s", task=task, try_count=try_count) def _event(self, template, exception=None, **kwargs): message = template.format(kwargs) message_type = template metadata = kwargs # WHERE event.metadata ->> "try_count": > 1 # context can be pulled from stacktrace to get Classname.method_name

ppolewicz · 2024-11-08T11:02:13Z

bt_ddos_shield/miner_shield.py

+        """
+        Publish info about current manifest file to blockchain.
+        """
+        current_address = Address.deserialize(self.blockchain_manager.get(self.miner_hotkey))


Suggested change

current_address = Address.deserialize(self.blockchain_manager.get(self.miner_hotkey))

current_address = self.ADDRESS_CLASS.deserialize(self.blockchain_manager.get(self.miner_hotkey))

class costam: ADDRESS_CLASS: type = Address class MyCostam: ADDRESS_CLASS: type = MyAddress

ClassVar[type[Address]] though?

I've done it other, better way with usage of Serializer class

ppolewicz · 2024-11-08T11:02:19Z

bt_ddos_shield/miner_shield.py

+        """
+        address = self.manifest_manager.add_mapping_file(self.state_manager.get_state().active_addresses)
+        self._event(f"Manifest updated, new address: {address}")
+        self._add_task(MinerShieldPublishManifestTask(address))


ppolewicz · 2024-11-08T11:05:10Z

bt_ddos_shield/miner_shield.py


-    def handle(self, miner_shield: MinerShield):
+class MinerShieldInitializeTask(AbstractMinerShieldTask):
+    def run(self, miner_shield: MinerShield):
        miner_shield._handle_initialize()



Suggested change

ppolewicz · 2024-11-08T11:12:58Z

bt_ddos_shield/utils.py

+Hotkey: TypeAlias = str
+PublicKey: TypeAlias = str


If you develop at head and plan to use 3.12+, which I pretty much think is true, according to

bt-ddos-shield/pyproject.toml

Lines 8 to 9 in c8e6b96

[tool.poetry.dependencies]

python = "^3.12"

...then I suggest you use type statements from PEP 695 instead:

Suggested change

Hotkey: TypeAlias = str

PublicKey: TypeAlias = str

type Hotkey = str

type PublicKey = str

Read more:

PEP 695

the type statement

After this change, such constructs used in my code stopped working:

if isinstance(obj, Hotkey):

decoded_mapping[Hotkey(hotkey)]

So I'm not changing this, because I think TypeAlias is what I want here. Maybe in future HotKey will be some class and not just alias for type.

EDIT:
I've checked it and I can change these lines to:

if isinstance(obj, Hotkey.__value__):

decoded_mapping[Hotkey.__value__(hotkey)]

But it looks ugly IMHO. Maybe there is some better way?

ppolewicz · 2024-11-08T11:13:20Z

tests/test_miner_shield.py

@@ -14,8 +14,8 @@ def test_start_stop(self):
        """
        Test if shield is properly starting and stopping.
        """
-        shield = MinerShield(None, None, None, None,
-                             None, LoggingMinerShieldEventProcessor(), MinerShieldOptions(retry_delay=1))
+        shield = MinerShield(None,None, None, None, None,


Suggested change

shield = MinerShield(None,None, None, None, None,

shield = MinerShield(None, None, None, None, None,

…shield flow test

…m with not stable encryption)

…tion and handling shielded port change

bswck · 2024-11-28T01:32:10Z

See also #9 and #10. GLHF working on this!

ggleszczynski · 2024-12-06T16:07:19Z

See also #9 and #10. GLHF working on this!

Checked it. And thanks :-)

bt_ddos_shield/manifest_manager.py

andreea-popescu-reef · 2024-12-18T15:59:01Z

bt_ddos_shield/miner_shield.py

+
+        self.task_queue.put(task)
+
+    def _worker_function(self):


why do we need threads? can't this be done with asyncio tasks?

for example def _add_task(self, task: 'AbstractMinerShieldTask'): could mostly be replaced with await task

I'm not sure if it can be done with asyncio tasks, because I don't wait here for the result and task is retried forever by worker until shield gets stopped (this behaviour is by design). I will learn more about asyncio and then think if it can be changed, because my knowledge about asyncio is very small now. I wrote it in my notes for future upgrade but as for now I think this can be left as it is.

I read something, thought about it, used AI and probably this can be refactored. But when current behavior is to be maintained, using asyncio won't change too much - I can use asyncio.to_thread to run _worker_function, use asyncio.Queue() instead of just Queue and make rest of needed changes, but logic and code structure will looks nearly the same (if I'm thinking correctly). So my question is, if it is worth time to change it, if it already works? I don't know the answer... Next time, when doing similar job, I'll try to use asyncio, but should I spend time for changing it here?

I'd say this is worth refactoring - there's no point in reimplementing the logic of a lib so common as asyncio. (https://docs.python.org/3/library/asyncio-task.html#awaitables)
The refactor wouldn't take long and it'll make the code much easier to follow for any py dev.

Ok, I'll do it. If my thoughts about how to do it sounds good?

see note in https://docs.python.org/3/library/asyncio-task.html#asyncio.to_thread about the GIL - I'd rather use asyncio.create_task instead of to_thread since there's no need for multiple threads. For the "io heavy" db work there is optimized native asyncio support in sqlalchemy https://docs.sqlalchemy.org/en/20/orm/extensions/asyncio.html.
the Queue is actually the event loop from asyncio - it shouldn't be needed

although you might want to keep the queue just for preventing gc of the task... see Important: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task

andreea-popescu-reef · 2024-12-18T16:02:25Z

bt_ddos_shield/miner_shield.py

+        miner_shield._handle_initialize()
+
+
+class MinerShieldDisableTask(AbstractMinerShieldTask):


insteaf of define these as classes - the functions could just be passed directly

Hmm, I'm not sure about this. Adding these tasks as class allows us to print their names in logs when handling - I know I can use method.name for this but getting class name looks nicer for me. Also some tasks have params, so I will need to pass method and params to queue, so anyway I need to wrap them in some tuple or something - having class for this also looks nicer for me.

This goes together with the asyncio refactor suggested above - in a standard asyncio app you'd expect this to be passed as function (The tuple you mentioned translates to a closure)
Having a class for this looks nice from an OOP pov but it's unnatural in python.

Ok, I'll change it too

bt_ddos_shield/state_manager.py

tests/test_address_manager.py

zyzniewski-reef · 2024-12-20T12:57:03Z

Bittensor BlockchainManager using commitments of knowledge

grzegorz-leszczynski-reef · 2025-01-03T15:55:13Z

Merging PR after consultations with Even. More changes will be made.

feat: implement enabling shield

0dbf21f

ggleszczynski force-pushed the features/implementation branch from 2275179 to 0dbf21f Compare November 2, 2024 11:14

ppolewicz reviewed Nov 2, 2024

View reviewed changes

Grzegorz Leszczyński added 2 commits November 5, 2024 19:02

fix: fixes after code review

9648ecb

feat: shield logic finished

6f321c7

ppolewicz reviewed Nov 8, 2024

View reviewed changes

Grzegorz Leszczyński added 19 commits November 8, 2024 18:14

fix: code review fixes

23fccc4

feat: add serialization logic

290fc3d

feat: logic finished

1c69b99

feat: encryption manager refactored

6b4abe1

feat: add memory implementation for some managers

34a9cf7

feat: finish memory implementations for all managers along with full …

168de86

…shield flow test

fix: finish tests for shield with needed fixes (there is still proble…

8f975a7

…m with not stable encryption)

feat: back to EncryptionManager using ECIES library

7f1873c

fix: added hash to manifest file to verify if content is the same

4d7fba3

feat: implemented Route53AddressManager

9802e2d

feat: implemented SQLAlchemyMinerShieldStateManager

16603b6

feat: implemented S3ManifestManager

6b24602

feat: added integration test

82e330c

feat: filling AwsAddressManager, work in progress

968f045

feat: added possibility of specyfing IP in AwsAddressManager

935c2e6

feat: filling AwsAddressManager - creating ELB is working

6ba1675

feat: filling AwsAddressManager - creating HostedZone during ELB crea…

2ac5dd7

…tion and handling shielded port change

feat: filling AwsAddressManager - creating alias DNS entry in Route53

097b2de

feat: finished AwsAddressManager - added creating firewall

dc62a30

feat: back to passing hosted_zone_id to AwsAddressManager

c9d3b75

fix: tiny code review fixes

845ec8a

andreea-popescu-reef reviewed Dec 18, 2024

View reviewed changes

andreea-popescu-reef reviewed Dec 19, 2024

View reviewed changes

tests/test_address_manager.py Outdated Show resolved Hide resolved

grzegorz-leszczynski-reef added 8 commits December 25, 2024 12:00

Add AWSClientFactory

44ebe33

Use pytest fixture in TestAddressManager

840941e

Simplify code after CR

54de75f

Upgrade handling AWS API errors

d2bc4a4

Add project dependencies

030cba1

Apply ruff linter fixes

5e27248

Add MinerShieldFactory

78ad8f2

Implement Validator

4af7d75

grzegorz-leszczynski-reef merged commit a50bff9 into bactensor:master Jan 3, 2025

	self.event_processor.add_event(MinerShieldEvent(f"Validators refreshed, got {len(validators)} validators"))
	self._event("Validators refreshed", validator_count=len(validators)))

		def handle(self, miner_shield: MinerShield):
		miner_shield._handle_initialize()

	Validator -> GitHub: Publish public key along with HotKey
	Validator -> Bittensor: Publish public key along with HotKey

	return f"Address(id={self.address_id}, type={self.address_type}, address={self.address}:{self.port})"
	return f"Address(id={self.address_id}, address={self.address}:{self.port})"

	current_address = Address.deserialize(self.blockchain_manager.get(self.miner_hotkey))
	current_address = self.ADDRESS_CLASS.deserialize(self.blockchain_manager.get(self.miner_hotkey))

	shield = MinerShield(None,None, None, None, None,
	shield = MinerShield(None, None, None, None, None,

		miner_shield._handle_initialize()


		class MinerShieldDisableTask(AbstractMinerShieldTask):

Implementing logic #7

Implementing logic #7

Conversation

ggleszczynski commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggleszczynski Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bswck commented Nov 28, 2024

ggleszczynski commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyzniewski-reef commented Dec 20, 2024

grzegorz-leszczynski-reef commented Jan 3, 2025

ggleszczynski Dec 6, 2024 •

edited

Loading