Changes based on style and lint checks. (final_code_quality_5) #1363
No reviewers
Labels
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
No Milestone
No project
No Assignees
1 Participants
No due date set.
Dependencies
No dependencies set.
Reference: Bitmessage/PyBitmessage-2024-12-03#1363
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "final_code_quality_5"
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?
Changes based on style and lint checks. (final_code_quality_5)
src/defaults.pyis made use of bysrc/bitmessagemain.py, though I've bumped that to a later PR since otherwise it was going to start pulling in the kivy files in that I haven't finished yet.Yea this is workaround for some Windows crap, it uses winsock which is kind of similar to unix' socket, but it can have its own error numbers, and these aren't defined when running python on other OSes. Maybe there is a nicer workaround but for the time being let's keep it as it is.
I'm not sure it's a FIXME, it could also be just a comment. However, it's not a big deal so I'm accepting this hunk.
This is a defense against the so called intersection attacks. It is called when you notice peer is requesting non-existing objects, or right after the connection is established. It will estimate how long an object will take to propagate across the network, and skip processing "getdata" requests until then. This means an attacker only has one shot per IP to perform the attack.
State after the bitmessage protocol handshake is completed (version/verack exchange, and if both side support TLS, the TLS handshake as well).
Initiate inventory synchronisation.
@ -101,3 +113,4 @@def set_connection_fully_established(self):"""Initiate inventory synchronisation."""if not self.isOutbound and not self.local:shared.clientHasReceivedIncomingConnections = TrueSend a partial list of known addresses to peer.
@ -101,3 +113,4 @@def set_connection_fully_established(self):"""Initiate inventory synchronisation."""if not self.isOutbound and not self.local:shared.clientHasReceivedIncomingConnections = TrueSend hashes of all inventory objects, chunked as the protocol has a per-command limit.
@ -101,3 +113,4 @@def set_connection_fully_established(self):"""Initiate inventory synchronisation."""if not self.isOutbound and not self.local:shared.clientHasReceivedIncomingConnections = TrueSend one chunk of inv entries in one command
@ -182,2 +192,4 @@for obj_hash, _ in bigInvList.items():payload += obj_hashobjectCount += 1Callback for TCP connection being established.
@ -182,2 +192,4 @@for obj_hash, _ in bigInvList.items():payload += obj_hashobjectCount += 1Callback for reading from a socket
@ -206,4 +223,4 @@self.connectedAt = time.time()receiveDataQueue.put(self.destination)def handle_read(self):Callback for writing to a socket
@ -206,4 +223,4 @@self.connectedAt = time.time()receiveDataQueue.put(self.destination)def handle_read(self):Callback for connection being closed.
@ -220,3 +238,4 @@def handle_write(self):"""Callback for writing to a socket"""TLSDispatcher.handle_write(self)SOCKS5 wrapper for TCP connections
@ -220,3 +238,4 @@def handle_write(self):"""Callback for writing to a socket"""TLSDispatcher.handle_write(self)State when SOCKS5 connection succeeds, we need to send a Bitmessage handshake to peer.
@ -220,3 +238,4 @@def handle_write(self):"""Callback for writing to a socket"""TLSDispatcher.handle_write(self)SOCKS4a wrapper for TCP connections
@ -220,3 +238,4 @@def handle_write(self):"""Callback for writing to a socket"""TLSDispatcher.handle_write(self)State when SOCKS4a connection succeeds, we need to send a Bitmessage handshake to peer.
@ -220,3 +238,4 @@def handle_write(self):"""Callback for writing to a socket"""TLSDispatcher.handle_write(self)TCP connection server for Bitmessage protocol
Is the socket bound?
Incoming connection callback
@ -286,3 +318,3 @@from debug import logger"""Add a port mapping"""for i in range(50):This doesn't do anything so it can be removed.
Make the small changes (docstrings, obsolete code) and it's ready to go.
The todo was added to consider the fixme. I consider it now well considered. I've formatted it as part of the docstring.
Updated in next commit...
@ -286,3 +318,3 @@from debug import logger"""Add a port mapping"""for i in range(50):OK, removed. Where I found unused variables involving unpack I left in the unpacks in case there was some state to do with unpacking lying around.
see
533df80@ -230,14 +262,11 @@ class uPnPThread(threading.Thread, StoppableThread):logger.debug("Found UPnP router at %s", ip)This statement can be probably removed. I cannot find where this
shared.alreadyAttemptedConnectionsListused.@ -230,14 +262,11 @@ class uPnPThread(threading.Thread, StoppableThread):logger.debug("Found UPnP router at %s", ip)OK, removed that context.
Edit: Actually, removed the whole list / lock, will also remove from shared.py when I commit the changed to that file.
@ -139,2 +149,2 @@filtered = {k: v for k, v in knownnodes.knownNodes[stream*2].items()if v["lastseen"] > (int(time.time()) - shared.maximumAgeOfNodesThatIAdvertiseToOthers)}if knownnodes.knownNodes[stream * 2] and stream not in self.streams:filtered = {k: v for k, v in knownnodes.knownNodes[stream * 2].items()It looks like
ObjectTrackeris used as a mixin, but it has an__init__. Other than suppressing the pylint warning, I'm not sure there's anything else to do. We could rename the__init__to_fake_init, but then we would have to suppressattribute-defined-outside-init. I would leave it as it is unless there's something I'm missing.Minor changes in docstrings, but I don't want to review it again so it's good to go. The docstrings can be adjusted later with a separate PR.
@ -139,2 +149,2 @@filtered = {k: v for k, v in knownnodes.knownNodes[stream*2].items()if v["lastseen"] > (int(time.time()) - shared.maximumAgeOfNodesThatIAdvertiseToOthers)}if knownnodes.knownNodes[stream * 2] and stream not in self.streams:filtered = {k: v for k, v in knownnodes.knownNodes[stream * 2].items()I would expect a mixin class to be defined as a parent:
class TCPConnection(BMProto, TLSDispatcher, ObjectTracker):and for the__init__s to be called in an appropriate order, each seeing*argsand**kwargsand each updating self as they go, passing off initialisation to the next parent class usingsuper(ThisClass, self).__init__(*args, **kwargs).However, if this is working as intended, perhaps re-initialisation is both happening and required? Either way, fixing this bad code smell will require some consideration, refactoring and testing.
@ -139,2 +149,2 @@filtered = {k: v for k, v in knownnodes.knownNodes[stream*2].items()if v["lastseen"] > (int(time.time()) - shared.maximumAgeOfNodesThatIAdvertiseToOthers)}if knownnodes.knownNodes[stream * 2] and stream not in self.streams:filtered = {k: v for k, v in knownnodes.knownNodes[stream * 2].items()super()wont work without refactoring, I tried.