Minimale Viable Tx Pool #898

cburgdorf · 2018-06-12T14:27:04Z

What was wrong?

As specified in #866 we need a minimal viable tx pool that may not work as a proper pool at first but at least relay transactions to peers. This PR serves as a stub to signal that I'm on it.

How was it fixed?

Implemented minimal viable transaction pool that simply relays transactions among connected peers.

Cute Animal Picture

carver · 2018-06-12T21:35:46Z

trinity/nodes/full.py

@@ -23,6 +26,9 @@ def __init__(self, chain_config: ChainConfig) -> None:
        self._node_key = chain_config.nodekey
        self._node_port = chain_config.port
        self._max_peers = chain_config.max_peers
+        self._tx_pool = TxPool()
+        self.add_service(self._tx_pool)


Shouldn't the light node also need to propagate the transaction pool? If so, this can go in Node instead of FullNode.

That was infact, one of the questions I had in mind. I wasn't sure if a light node relays transactions similar to a full node. Thanks for jumping in to clarify!

pipermerriam · 2018-06-14T19:14:02Z

trinity/tx_pool/base.py

+            receiving_peer = cast(ETHPeer, receiving_peer)
+            if receiving_peer is not peer:
+                receiving_peer.sub_proto.send_transactions(
+                    self._filter_tx_for_peer(receiving_peer, msg)


The filtering should probably happen first, and then only send if the result of filtering is not an empty list.

Yes, good catch. I plan to cover all of that with test as well.

pipermerriam · 2018-06-18T15:55:35Z

evm/rlp/transactions.py

    @property
    def hash(self) -> bytes:
        return keccak(rlp.encode(self))

+
+class BaseTransaction(BaseTransactionCommonFields, BaseTransactionCommonMethods):


Not for this PR but I think we could drop the Common from these names.

pipermerriam · 2018-06-18T15:56:33Z

p2p/rlp.py

-from evm.rlp.transactions import BaseTransaction
-
+# from eth_typing import (
+#     Address


why commented out?

Refactoring leftover. Will fix.

pipermerriam · 2018-06-18T16:00:19Z

p2p/eth.py

@@ -40,7 +41,7 @@ class NewBlockHashes(Command):

 class Transactions(Command):
    _cmd_id = 2
-    structure = sedes.CountableList(P2PTransaction)
+    structure = sedes.CountableList(BaseTransactionCommonFields)


Would you be willing to extract these changes to their own PR? They seem like they can stand on their own.

They are related (because I need hash() on the P2PTransaction and getting rid of that P2PTransaction alltogether seems like a good thing to me. But I'm happy to move that into a seperate commit to make the core of the tx pool change stand out better.

pipermerriam · 2018-06-18T16:07:19Z

trinity/tx_pool/pool.py

+        return [val for val in txs if self._construct_bloom_entry(peer, val) not in self._bloom]
+
+    def _construct_bloom_entry(self, peer: BasePeer, tx: BaseTransactionCommonFields) -> bytes:
+        return "{!r}-{}".format(peer.remote, tx.hash).encode()


This should have a localized salt of some sort, otherwise, it will be possible for peers to craft bloom filter collisions. Not a big deal in terms of security, but still something I think we should mitigate. The pool can just generate a singe random value on instantiation that it appends to this message.

pipermerriam · 2018-06-18T17:46:58Z

trinity/tx_pool/pool.py

+    def __init__(self, peer_pool: PeerPool) -> None:
+        super().__init__()
+        self._peer_pool = peer_pool
+        self._bloom = BloomFilter()


Can we be explicit about the bloom filter parameters as well as having a comment here which documents what our collision rates are likely to be with the given parameters.

what our collision rates are likely to be with the given parameters

¯_(ツ)_/¯

cburgdorf · 2018-06-18T19:51:52Z

tests/p2p/test_tx_pool.py

+    MockPeerPoolWithConnectedPeers,
+)
+
+# TODO: This whole file should live in the trinity tests but can't


@pipermerriam any advice how to deal with that? My limited Python fu tells me the problem is that most of the directories within tests aren't actually Python modules (no __init__.py) and that is the reason why I can't cross import it (..relative.lookup also doesn't work).

But I read somewhere that NOT making the test directories proper python modules is actually the right way to handle test directories because of some pytest madness. Any advice how to handle this?

Maybe relocating peer_helpers to something like p2p.utils.testing.peer_helpers. That will cause it to be shipped with the actual p2p module but I don't see that as a big deal as long as the overhead remains small (like not including large fixture files or something like that).

Mmh...not terribly excited about that 😅 I digged a bit further and noticed that tests already had a __init__.py but tests/p2p did not. So, I tried what would happen if we make that a module and this seems to work: 8827809

Still has some things failing but I think that could be worked out. But maybe we leave this out of this PR and I address that in a follow up?

I slightly changed that comment and plan to address that test refactoring in a follow up PR to not grow this PR with more unrelated changes.

carver · 2018-06-18T22:00:02Z

p2p/server.py

-        self.logger.info("Running server...")
-        upnp_dev = await self._discover_upnp_device()
+    async def _make_discovery(self,
+                              upnp_device: Awaitable[upnpclient.Device]) -> DiscoveryProtocol:


Passing in a single upnp device will not be sufficient for discovery. Take a look at the proposed refactor here, which loops over multiple devices: #905

Yep, that commit basically just tries to solve the problem of not having the PeerPool available with the instantiation of the Server. It's a small refactor which doesn't change the rest of the process. I can rebase that in case #905 lands first.

pipermerriam · 2018-06-19T21:42:34Z

p2p/server.py

            await self._add_nat_portmap(upnp_dev)
+            external_ip = upnp_dev.WANIPConn1.GetExternalIPAddress()['NewExternalIPAddress']
+
+        addr = Address(external_ip, self.port, self.port)


Just realized this will collide with our eth_typing.Address. Just pointing it out.

Address is a pretty generic name so I expect it to exist in many different modules. That said, in this module only kademlia.Address is used. Do you want me to change that to kademlia.Address to make that more explicit?

No, just wanted to point it out as we'll probably make the mistake eventually.

pipermerriam · 2018-06-19T21:54:53Z

trinity/tx_pool/pool.py

+    def __init__(self, peer_pool: PeerPool) -> None:
+        super().__init__()
+        self._peer_pool = peer_pool
+        self._bloom = BloomFilter(max_elements=10000)


some quick checks suggest 10,000 transactions is equivalent to about 90 blocks at current mainnet volume.

Lets bump this to something like 1 million, which looks like it will consume about 1mb of memory. This also suggests a rolling bloom filter (maybe per peer) solution is going to be a priority very quickly.

Will bump it up to 1m. I'd also like to check what geth and Parity do. If they do a rolling bloom filter or something else and how exactly. But yeah, can make that a high priority.

cburgdorf · 2018-06-20T20:28:11Z

@pipermerriam @gsalgado This is now rebased on top of the refactored PeerPool. I also bumped up the bloom filter size as suggested.

pipermerriam · 2018-06-21T21:16:04Z

trinity/tx_pool/pool.py

+
+        for receiving_peer in self._peer_pool.peers:
+            receiving_peer = cast(ETHPeer, receiving_peer)
+            if receiving_peer is not peer:


I've found that in loops like this the following is easier to read. Thoughts?

for thing in many_things: if not some_required_condition(thing): continue do_for_loop_body_logic(thing) # instead of for thing in many_things: if some_required_condition(thing): do_for_loop_body_logic(thing)

Basically, rather than making the context deeper, trying to keep the context as shallow as possible by putting the early exit conditions first and then maintaining the shallow context level for the main function body.

For bodies with a single conditional this isn't a huge improvement, but when we get to having two conditional checks:

for thing in many_things: if not some_required_condition(thing): continue if some_forbidden_condition(thing): continue do_for_loop_body_logic(thing) # instead of for thing in many_things: if some_required_condition(thing): if not some_forbidden_condition(thing): do_for_loop_body_logic(thing)

I can live with that. I'm personally more used to something like this (python-pseudocode):

things.filter(lambda thing: return thing.is_cool()) .filter(lambda thing: return thing.rocks()) .filter(lambda thing: return thing.doesn_it_kitten()) .for_each(lamda thing: do_stuff(thing))

I find this far more readable but I'm not sure if something similarish exists. I guess with lambdas being restricted to single lines in Python this probably doesn't work so well in this ecosystem.

Oh and I do agree that rightwards drift is ugly and should be avoided. I'm just not used that much to that loop skipping style.

You can accomplish this pretty easily with toolz/cytoolz but the result is far less readable.

tuple(pipe( things, functools.partial(filter, operator.methodcaller('is_cool')), functools.partial(filter, operator.methodcaller('rocks')), functools.partial(filter, operator.methodcaller('does_it_kitten')), functools.partial(map, do_stuff) ))

I suspect there is a data pipeline library out there somewhere which allows for a smoother api.

but the result is far less readable

Well...yes...that is.....GOD DAMN UGLY hahaha!

It's fine, it's the ecosystem I signed up for. If that's the price I pay for working on exciting stuff, I'm happy to pay it ;)

pipermerriam · 2018-06-21T21:19:19Z

trinity/tx_pool/pool.py

+            peer: BasePeer,
+            txs: List[BaseTransactionFields]) -> List[BaseTransactionFields]:
+
+        return [val for val in txs if self._construct_bloom_entry(peer, val) not in self._bloom]


nitpick

I find these easier to read as

return [ val for val in txs if self._construct_bloom_entry(peer, val) not in self._bloom ] # or if you're more into brevity return [ val for val in txs if self._construct_bloom_entry(peer, val) not in self._bloom ]

yep, same here, I'm more used to

txs.filter(lambda tx: return self._construct_bloom_entry(peer, tx) not in self._bloom)

I like the second of your suggestions more than the first.

Fixes ethereum#866

carver reviewed Jun 12, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/tx-pool branch 2 times, most recently from 6dac181 to 82efd80 Compare June 14, 2018 12:45

pipermerriam reviewed Jun 14, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/tx-pool branch 5 times, most recently from 3b14e4e to 9c7b43d Compare June 18, 2018 15:51

pipermerriam reviewed Jun 18, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/tx-pool branch from 377ba2e to d2c48f9 Compare June 18, 2018 19:47

cburgdorf commented Jun 18, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/tx-pool branch 2 times, most recently from e55cf13 to 8fd4cfc Compare June 18, 2018 20:58

cburgdorf changed the title ~~WIP: Minimale Viable Tx Pool~~ Minimale Viable Tx Pool Jun 18, 2018

carver reviewed Jun 18, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/tx-pool branch from 8827809 to 2ea31e8 Compare June 19, 2018 07:38

cburgdorf requested a review from gsalgado June 19, 2018 15:49

pipermerriam reviewed Jun 19, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/tx-pool branch from 2ea31e8 to 3a7697a Compare June 20, 2018 09:02

cburgdorf mentioned this pull request Jun 20, 2018

Make PeerPool available earlier #886

Closed

cburgdorf force-pushed the christoph/feat/tx-pool branch 3 times, most recently from a14972c to ebad10f Compare June 20, 2018 20:26

cburgdorf force-pushed the christoph/feat/tx-pool branch from ebad10f to 7fc226a Compare June 20, 2018 22:39

cburgdorf requested a review from pipermerriam June 21, 2018 07:11

cburgdorf mentioned this pull request Jun 21, 2018

Prevent relaying of invalid transactions (possible DDOS attacks) #945

Closed

pipermerriam approved these changes Jun 21, 2018

View reviewed changes

cburgdorf added 2 commits June 22, 2018 00:06

Implement minimal viable transaction pool

0283e42

Fixes ethereum#866

Refactor P2pTransaction to BaseTransactionFields

45542d9

cburgdorf force-pushed the christoph/feat/tx-pool branch from 7fc226a to ed74275 Compare June 21, 2018 22:07

Rename BaseTransactionCommonMethods -> BaseTransactionMethods

7c290df

cburgdorf force-pushed the christoph/feat/tx-pool branch from ed74275 to 7c290df Compare June 21, 2018 22:14

cburgdorf merged commit 82022a7 into ethereum:master Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimale Viable Tx Pool #898

Minimale Viable Tx Pool #898

cburgdorf commented Jun 12, 2018 •

edited

carver Jun 12, 2018

cburgdorf Jun 13, 2018 •

edited

pipermerriam Jun 14, 2018

cburgdorf Jun 14, 2018

pipermerriam Jun 18, 2018

pipermerriam Jun 18, 2018

cburgdorf Jun 18, 2018

pipermerriam Jun 18, 2018

cburgdorf Jun 18, 2018

pipermerriam Jun 18, 2018

pipermerriam Jun 18, 2018

cburgdorf Jun 18, 2018

cburgdorf Jun 18, 2018

pipermerriam Jun 18, 2018

cburgdorf Jun 18, 2018

cburgdorf Jun 19, 2018

carver Jun 18, 2018

cburgdorf Jun 19, 2018 •

edited

pipermerriam Jun 19, 2018

cburgdorf Jun 19, 2018

pipermerriam Jun 20, 2018

pipermerriam Jun 19, 2018

cburgdorf Jun 19, 2018

cburgdorf commented Jun 20, 2018

pipermerriam Jun 21, 2018

cburgdorf Jun 21, 2018 •

edited

cburgdorf Jun 21, 2018

pipermerriam Jun 21, 2018

cburgdorf Jun 21, 2018

pipermerriam Jun 21, 2018

cburgdorf Jun 21, 2018

Minimale Viable Tx Pool #898

Minimale Viable Tx Pool #898

Conversation

cburgdorf commented Jun 12, 2018 • edited

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

cburgdorf Jun 13, 2018 • edited

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

cburgdorf Jun 19, 2018 • edited

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

cburgdorf commented Jun 20, 2018

Choose a reason for hiding this comment

cburgdorf Jun 21, 2018 • edited

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

cburgdorf commented Jun 12, 2018 •

edited

cburgdorf Jun 13, 2018 •

edited

cburgdorf Jun 19, 2018 •

edited

cburgdorf Jun 21, 2018 •

edited