Bitmessage/PyBitmessage-2024-12-12
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

moved dandelion_enabled from state to dandelion instance as enabled attribute #2246

Merged
anand-skss merged 1 commits from test3 into v0.6 2024-06-11 11:35:05 +02:00
Conversation 0 Commits 1 Files Changed 13 +76 -68
anand-skss commented 2024-05-29 08:01:15 +02:00 (Migrated from github.com)
Copy Link
  • moved dandelion_enabled from state to Dandelion class as enabled attribute
  • Move dandelion enabled checks from main into a function init_dandelion_enabled under Dandelion class
* moved dandelion_enabled from state to Dandelion class as enabled attribute * Move dandelion enabled checks from main into a function init_dandelion_enabled under Dandelion class
PeterSurda (Migrated from github.com) requested changes 2024-05-29 09:23:00 +02:00
src/bitmessagemain.py
PeterSurda (Migrated from github.com) commented 2024-05-29 09:17:55 +02:00

how about making this into a static method that returns True / False? Then the instance and the config are decoupled, you pass the result of is_dandelion_enabled do the constructor.

how about making this into a static method that returns `True` / `False`? Then the instance and the config are decoupled, you pass the result of `is_dandelion_enabled` do the constructor.
src/network/bmproto.py
@ -351,14 +350,14 @@ class BMProto(AdvancedDispatcher, ObjectTracker):
raise BMProtoExcessiveDataError()
PeterSurda (Migrated from github.com) commented 2024-05-29 09:19:04 +02:00

what we can do here is to move this check into the Dandelion.hasHash method. Then bmproto doesn't need to check.

what we can do here is to move this check into the `Dandelion.hasHash` method. Then bmproto doesn't need to check.
src/network/invthread.py
@ -40,10 +40,10 @@ class InvThread(StoppableThread):
@staticmethod
def handleLocallyGenerated(stream, hashId):
PeterSurda (Migrated from github.com) commented 2024-05-29 09:20:43 +02:00

here, addHash could return True / False depending on whether dandelion is on/off, and we could use this for further checks.

here, `addHash` could return `True` / `False` depending on whether dandelion is on/off, and we could use this for further checks.
PeterSurda (Migrated from github.com) commented 2024-05-29 09:21:13 +02:00

here, objectChildStem could return value depending on whether dandelion is enabled or not

here, `objectChildStem` could return value depending on whether dandelion is enabled or not
src/network/invthread.py
@ -75,10 +75,10 @@ class InvThread(StoppableThread):
except KeyError:
continue
try:
PeterSurda (Migrated from github.com) commented 2024-05-29 09:22:04 +02:00

Hmm, I forgot, the value is actually an int between 0 and 100. We need to rethink.

Hmm, I forgot, the value is actually an int between 0 and 100. We need to rethink.
src/network/invthread.py
@ -105,7 +105,6 @@ class InvThread(StoppableThread):
for _ in range(len(chunk)):
PeterSurda (Migrated from github.com) commented 2024-05-29 09:22:40 +02:00

this check could also be moved down (i.e. reRandomiseStems could check check the time.

this check could also be moved down (i.e. `reRandomiseStems` could check check the time.
anand-skss (Migrated from github.com) reviewed 2024-05-30 03:45:24 +02:00
src/bitmessagemain.py
anand-skss (Migrated from github.com) commented 2024-05-30 03:45:24 +02:00

but the problem is we need a variable where we can store the return value. currently after checking we are storing in class attr.

but the problem is we need a variable where we can store the return value. currently after checking we are storing in class attr.
anand-skss (Migrated from github.com) reviewed 2024-05-30 04:01:30 +02:00
src/network/bmproto.py
@ -351,14 +350,14 @@ class BMProto(AdvancedDispatcher, ObjectTracker):
raise BMProtoExcessiveDataError()
anand-skss (Migrated from github.com) commented 2024-05-30 04:01:30 +02:00

we can move into Dandelion.hasHash. but hasHash here used in some checks in iteration. i think that could not work same as here, like return direct True.

we can move into Dandelion.hasHash. but `hasHash` here used in some checks in iteration. i think that could not work same as here, like return direct True.
PeterSurda (Migrated from github.com) reviewed 2024-05-30 05:37:22 +02:00
src/bitmessagemain.py
PeterSurda (Migrated from github.com) commented 2024-05-30 05:37:22 +02:00

Hmm, now I see the problem. We then probably could create a Placeholder instance inside dandelion.py and create a proper instance here in bitmessagemain.py. Then we can pass the value to the constructor.

Hmm, now I see the problem. We then probably could create a `Placeholder` instance inside `dandelion.py` and create a proper instance here in `bitmessagemain.py`. Then we can pass the value to the constructor.
PeterSurda (Migrated from github.com) reviewed 2024-05-30 05:40:21 +02:00
src/network/bmproto.py
@ -351,14 +350,14 @@ class BMProto(AdvancedDispatcher, ObjectTracker):
raise BMProtoExcessiveDataError()
PeterSurda (Migrated from github.com) commented 2024-05-30 05:40:20 +02:00

The easiest solution is just to use the same approach in if as in for. It would result in dummy loops but my guess is that it's not a big performance hit. It could be optimised for performance later if needed.

The easiest solution is just to use the same approach in `if` as in `for`. It would result in dummy loops but my guess is that it's not a big performance hit. It could be optimised for performance later if needed.
anand-skss (Migrated from github.com) reviewed 2024-05-30 08:22:38 +02:00
src/bitmessagemain.py
anand-skss (Migrated from github.com) commented 2024-05-30 08:22:38 +02:00

invthread.py", line 55, in run
handleExpiredDandelion(dandelion.instance.expire())

  • ivthread initialized before dandelion instance created and checking for dandelion expire. real dandelion instance is required at the starting point of thread.
invthread.py", line 55, in run handleExpiredDandelion(dandelion.instance.expire()) - ivthread initialized before dandelion instance created and checking for dandelion expire. real dandelion instance is required at the starting point of thread.
PeterSurda (Migrated from github.com) requested changes 2024-06-03 08:05:05 +02:00
src/network/dandelion.py
@ -179,19 +191,18 @@ class Dandelion: # pylint: disable=old-style-class
def reRandomiseStems(self):
"""Re-shuffle stem mapping (parent <-> child pairs)"""
assert self.pool is not None
PeterSurda (Migrated from github.com) commented 2024-06-03 08:04:55 +02:00

We could use lazy initialisation on pool too and keep it as an attribute, then we don't need to pass it later.

We could use lazy initialisation on `pool` too and keep it as an attribute, then we don't need to pass it later.
src/protocol.py
@ -336,8 +336,8 @@ def assembleAddrMessage(peerList):
return retval
PeterSurda (Migrated from github.com) commented 2024-06-03 08:01:00 +02:00

We can have dandelion_enabled default to True. Then we can put it at the end of arguments as well.

We can have `dandelion_enabled` default to `True`. Then we can put it at the end of arguments as well.
src/tests/core.py
@ -319,16 +319,17 @@ class TestCore(unittest.TestCase):
def test_version(self):
PeterSurda (Migrated from github.com) commented 2024-06-03 08:00:14 +02:00

we don't really need an instance of dandelion here. We just need a variable and we pass the same variable to assembleVersionMessage and assertEqual

we don't really need an instance of dandelion here. We just need a variable and we pass the same variable to `assembleVersionMessage` and `assertEqual`
PeterSurda (Migrated from github.com) requested changes 2024-06-04 02:32:36 +02:00
src/network/__init__.py
PeterSurda (Migrated from github.com) commented 2024-06-04 02:24:42 +02:00

It should maybe say "pass instance", I don't think "lazy init" is a good comment. We're not lazy initialising is_enabled or pool, we're lazy initialising dandelion.

It should maybe say "pass instance", I don't think "lazy init" is a good comment. We're not lazy initialising is_enabled or pool, we're lazy initialising dandelion.
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2024-06-04 02:22:57 +02:00

Why in bmobject it does from . import dandelion_ins and here it does from network import dandelion_ins? Why is it inconsistent?

Why in `bmobject` it does `from . import dandelion_ins` and here it does `from network import dandelion_ins`? Why is it inconsistent?
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
PeterSurda (Migrated from github.com) commented 2024-06-04 02:26:00 +02:00

Maybe "pass pool instance"

Maybe "pass pool instance"
PeterSurda (Migrated from github.com) commented 2024-06-04 02:28:03 +02:00

Do we need a separate init_is_enabled and is_dandelion_enabled?

Do we need a separate `init_is_enabled` and `is_dandelion_enabled`?
src/network/dandelion.py
@ -93,3 +104,4 @@
def maybeAddStem(self, connection, invQueue):
"""
If we had too few outbound connections, add the current one to the
current stem list. Dandelion as designed by the authors should
PeterSurda (Migrated from github.com) commented 2024-06-04 02:30:20 +02:00

this isn't necessary, you can retrieve enabled directly as an object attribute

this isn't necessary, you can retrieve enabled directly as an object attribute
src/network/dandelion.py
@ -182,4 +198,4 @@
with self.lock:
try:
# random two connections
self.stem = sample(
PeterSurda (Migrated from github.com) commented 2024-06-04 02:30:34 +02:00

duplicate

duplicate
src/network/invthread.py
PeterSurda (Migrated from github.com) commented 2024-06-04 02:31:31 +02:00

we can just use dandelion_ins.enabled

we can just use `dandelion_ins.enabled`
anand-skss (Migrated from github.com) reviewed 2024-06-04 03:53:22 +02:00
src/network/invthread.py
anand-skss (Migrated from github.com) commented 2024-06-04 03:53:22 +02:00

As per this (https://github.com/Bitmessage/PyBitmessage/pull/2246#discussion_r1618334811) , here objectChildStem is returning value whether dandelion is enabled or not and that is used here. I've modified it now as mentioned here.

As per this (https://github.com/Bitmessage/PyBitmessage/pull/2246#discussion_r1618334811) , here `objectChildStem` is returning value whether dandelion is enabled or not and that is used here. I've modified it now as mentioned here.
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:09:21 +02:00
src/network/dandelion.py
@ -182,4 +198,4 @@
with self.lock:
try:
# random two connections
self.stem = sample(
anand-skss (Migrated from github.com) commented 2024-06-04 04:09:20 +02:00

Fixed

Fixed
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:09:26 +02:00
src/network/dandelion.py
@ -93,3 +104,4 @@
def maybeAddStem(self, connection, invQueue):
"""
If we had too few outbound connections, add the current one to the
current stem list. Dandelion as designed by the authors should
anand-skss (Migrated from github.com) commented 2024-06-04 04:09:26 +02:00

https://github.com/Bitmessage/PyBitmessage/pull/2246#discussion_r1618334811 , I've modified it now as mentioned here.

https://github.com/Bitmessage/PyBitmessage/pull/2246#discussion_r1618334811 , I've modified it now as mentioned here.
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:17:03 +02:00
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
anand-skss (Migrated from github.com) commented 2024-06-04 04:17:03 +02:00

merged it into is_dandelion_enabled by changing it to classmethod from staticmethod. and removed init_is_enabled.

merged it into `is_dandelion_enabled` by changing it to classmethod from staticmethod. and removed `init_is_enabled`.
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:19:15 +02:00
src/network/__init__.py
anand-skss (Migrated from github.com) commented 2024-06-04 04:19:15 +02:00

changed comment.

changed comment.
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:19:23 +02:00
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
anand-skss (Migrated from github.com) commented 2024-06-04 04:19:23 +02:00

Done

Done
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:24:10 +02:00
src/network/bmproto.py
anand-skss (Migrated from github.com) commented 2024-06-04 04:24:09 +02:00

Chenged it to from network import dandelion_ins everywhere.

Chenged it to `from network import dandelion_ins` everywhere.
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:28:50 +02:00
src/network/dandelion.py
@ -179,19 +191,18 @@ class Dandelion: # pylint: disable=old-style-class
def reRandomiseStems(self):
"""Re-shuffle stem mapping (parent <-> child pairs)"""
assert self.pool is not None
anand-skss (Migrated from github.com) commented 2024-06-04 04:28:50 +02:00

Done

Done
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:28:58 +02:00
src/tests/core.py
@ -319,16 +319,17 @@ class TestCore(unittest.TestCase):
def test_version(self):
anand-skss (Migrated from github.com) commented 2024-06-04 04:28:58 +02:00

Done

Done
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:30:41 +02:00
src/bitmessagemain.py
anand-skss (Migrated from github.com) commented 2024-06-04 04:30:41 +02:00

Resolved now. moved Dandelion instance into network/__init__.py

Resolved now. moved Dandelion instance into `network/__init__.py`
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:37:23 +02:00
src/network/invthread.py
@ -75,10 +75,10 @@ class InvThread(StoppableThread):
except KeyError:
continue
try:
anand-skss (Migrated from github.com) commented 2024-06-04 04:37:23 +02:00

when dandelion is on what would be the probable return value. that help us resolve the fixing comparison between int and bool

when dandelion is on what would be the probable return value. that help us resolve the fixing comparison between `int` and `bool`
anand-skss (Migrated from github.com) reviewed 2024-06-04 04:50:54 +02:00
src/network/invthread.py
anand-skss (Migrated from github.com) commented 2024-06-04 04:50:54 +02:00

As per https://github.com/Bitmessage/PyBitmessage/pull/2246#discussion_r1618334222. i have returned dandelion enabled value here as we. do i need to use dandelion_ins.enabled here as well?

As per https://github.com/Bitmessage/PyBitmessage/pull/2246#discussion_r1618334222. i have returned dandelion enabled value here as we. do i need to use `dandelion_ins.enabled` here as well?
PeterSurda (Migrated from github.com) requested changes 2024-06-10 07:50:01 +02:00
src/network/dandelion.py
PeterSurda (Migrated from github.com) commented 2024-06-10 07:45:12 +02:00

I would also pass invQueue using dependency injection. Then we can simplify import.

I would also pass `invQueue` using dependency injection. Then we can simplify import.
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
PeterSurda (Migrated from github.com) commented 2024-06-10 07:43:43 +02:00

I would call this init_dandelion_enabled

I would call this `init_dandelion_enabled`
PeterSurda (Migrated from github.com) commented 2024-06-10 07:46:27 +02:00

We probably don't need to check for dandelion_enabled here.

We probably don't need to check for `dandelion_enabled` here.
src/network/invthread.py
PeterSurda (Migrated from github.com) commented 2024-06-10 07:48:33 +02:00

I think using dandelion_ins.enabled makes more sense.

I think using `dandelion_ins.enabled` makes more sense.
anand-skss (Migrated from github.com) reviewed 2024-06-11 04:33:06 +02:00
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
anand-skss (Migrated from github.com) commented 2024-06-11 04:33:06 +02:00

Value is coming from config at startup. so i think it is required.

Value is coming from config at startup. so i think it is required.
anand-skss (Migrated from github.com) reviewed 2024-06-11 04:39:55 +02:00
src/network/invthread.py
anand-skss (Migrated from github.com) commented 2024-06-11 04:39:55 +02:00

Done

Done
anand-skss (Migrated from github.com) reviewed 2024-06-11 04:40:23 +02:00
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
anand-skss (Migrated from github.com) commented 2024-06-11 04:40:22 +02:00

done

done
PeterSurda (Migrated from github.com) reviewed 2024-06-11 04:40:46 +02:00
src/network/dandelion.py
@ -47,10 +46,23 @@ class Dandelion: # pylint: disable=old-style-class
average = FLUFF_TRIGGER_MEAN_DELAY
return start + expovariate(1.0 / average) + FLUFF_TRIGGER_FIXED_DELAY
PeterSurda (Migrated from github.com) commented 2024-06-11 04:40:46 +02:00

I mean we're checking twice, should work ok if we just check once.

I mean we're checking twice, should work ok if we just check once.
anand-skss (Migrated from github.com) reviewed 2024-06-11 05:52:47 +02:00
src/network/dandelion.py
anand-skss (Migrated from github.com) commented 2024-06-11 05:52:47 +02:00

At Constructor level, dependency injection is not working, having same issue. Dandelion class have two fns having invQueue dependency. from pybitmessage.queues import invQueue is working but for portable it fails.

At Constructor level, dependency injection is not working, having same issue. Dandelion class have two fns having invQueue dependency. `from pybitmessage.queues import invQueue` is working but for portable it fails.
anand-skss (Migrated from github.com) reviewed 2024-06-11 05:54:28 +02:00
src/network/dandelion.py
anand-skss (Migrated from github.com) commented 2024-06-11 05:54:28 +02:00

i think try except option would be better here until project fully migrated to py3

i think try except option would be better here until project fully migrated to py3
anand-skss (Migrated from github.com) reviewed 2024-06-11 10:56:21 +02:00
src/network/dandelion.py
anand-skss (Migrated from github.com) commented 2024-06-11 10:56:21 +02:00

Used dependency injection at function level.

Used dependency injection at function level.
PeterSurda (Migrated from github.com) approved these changes 2024-06-11 11:28:11 +02:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
PeterSurda
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-12-12#2246
No description provided.
Delete Branch "test3"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?