Handle pending transactions #60

monokh · 2018-02-10T10:01:54Z

What was wrong?

Eth tester does not handle pending transactions correctly when disabling auto mining transactions:

Sending a transaction with the same nonce causes an invalid nonce error when auto mining transactions are disabled.

The issue is that send_transaction and send_raw_transaction immediately execute on the vm's state and thereby increment the valid nonce.

How was it fixed?

Implement a transaction pool at the eth-tester level so that transactions are only executed when auto mining is enabled again.

Cute Animal Picture

Fixes #59

monokh · 2018-02-10T14:15:50Z

eth_tester/utils/backend_testing.py

        eth_tester.disable_auto_mine_transactions()
        transaction_hash = eth_tester.send_transaction({
            "from": eth_tester.get_accounts()[0],
            "to": BURN_ADDRESS,
            "gas": 21000,
        })
-        receipt = eth_tester.get_transaction_receipt(transaction_hash)


According to eth_getTransactionReceipt this shouldn't work.

Changing this test made me realise that get_transaction_by_hash should be able to retrieve the pending transaction - it doesn't right now.

It does now

…es from same account

monokh · 2018-02-12T07:04:31Z

eth_tester/main.py

+            # Remove any pending transactions with the same nonce
+            self._pending_transactions = [tx for tx in self._pending_transactions
+                                          if not (pending_transaction['nonce'] == tx['nonce']
+                                                  and pending_transaction['from'] == tx['from'])]


and chainId equals?

Probably, but way less important than the others.

I expect the node to reject a transaction if you send it with a different chainid than the node is syncing with, so the handling of failed transactions should be sufficient.

monokh · 2018-02-12T07:06:41Z

eth_tester/main.py

@@ -244,15 +273,25 @@ def get_nonce(self, account, block_number="latest"):
    #
    # Blocks, Transactions, Receipts
    #
+
+    def _get_pending_transaction_by_hash(self, transaction_hash):
+        for transaction in self._pending_transactions:


Not very performant. If that is a concern, can make _pending_transaction a dict with key=hash value=transaction

Eh, it'd need to be an ordered dict to preserve transaction order right? I think that would be fine, but it's not a big deal. The performance hit here isn't likely to be measurable against the actual evm execution overhead.

Not a concern, at least for now. If someone wants to test a really high pending count, we can revisit.

…ct txs

monokh · 2018-02-12T12:55:42Z

If this is ok, docs for get_transaction_by_hash and enable_auto_mine_transactions will need updating to reflect the change of behaviour

pipermerriam · 2018-02-12T18:41:59Z

eth_tester/main.py

+            # Remove any pending transactions with the same nonce
+            self._pending_transactions = [tx for tx in self._pending_transactions
+                                          if not (pending_transaction['nonce'] == tx['nonce']
+                                                  and pending_transaction['from'] == tx['from'])]


Probably, but way less important than the others.

pipermerriam · 2018-02-12T18:46:19Z

eth_tester/main.py

+        else:
+            pending_transaction = self.get_transaction_by_hash(transaction_hash)
+            # Remove any pending transactions with the same nonce
+            self._pending_transactions = [tx for tx in self._pending_transactions


Can we unroll this comprehension into a small stand alone utility function for better readability as well as easier stand-alone testing.

@to_tuple def (tx, pending_transactions): for transaction in pending_transactions: ... yield transaction

Something that's got the same logic, but unrolled into an easier to read code body.

pipermerriam · 2018-02-12T18:48:39Z

eth_tester/main.py

@@ -244,15 +273,25 @@ def get_nonce(self, account, block_number="latest"):
    #
    # Blocks, Transactions, Receipts
    #
+
+    def _get_pending_transaction_by_hash(self, transaction_hash):
+        for transaction in self._pending_transactions:


Eh, it'd need to be an ordered dict to preserve transaction order right? I think that would be fine, but it's not a big deal. The performance hit here isn't likely to be measurable against the actual evm execution overhead.

pipermerriam · 2018-02-12T18:49:56Z

eth_tester/main.py

-        self.validator.validate_outbound_transaction(raw_transaction)
-        transaction = self.normalizer.normalize_outbound_transaction(raw_transaction)
-        return transaction
+        pending_transaction = self._get_pending_transaction_by_hash(transaction_hash)


Rather than having this function return None when the transaction isn't found, I think it'd be better if it raised TransactionNotFound. This way nobody accidentally returns None because they forgot to check the return value.

Good point, it also makes the flow in get_transaction_by_hash easier to read

…hen not found

carver

I'd like to hear your thoughts on everything, but the try/finally block is the only thing that really needs to be addressed (either by a code change, or explaining why I'm wrong :P... ) before being merged.

carver · 2018-02-16T05:11:19Z

eth_tester/main.py

+            self._pending_transactions = remove_matching_transaction_from_list(
+                self._pending_transactions, pending_transaction)
+            self._pending_transactions.append(pending_transaction)
+            self.revert_to_snapshot(snapshot)


I'm thinking this revert might need to be in a try/finally block. Otherwise, if there is any kind of exception after a state change, we will fail to roll back.

I'm not sure I understand. Isn't revert_to_snapshot doing the rollback? And if that fails, is there really a meaningful way to continue?

It feels to me like the whole function should completely cleanly otherwise the final state is not really usable

Ah, let me clarify with some code. Something like this:

snapshot = self.take_snapshot() try: transaction_hash = func(self, *args, **kwargs) pending_transaction = self.get_transaction_by_hash(transaction_hash) ... self._pending_transactions.append(pending_transaction) finally: self.revert_to_snapshot(snapshot)

Oh 😄 I see what you mean. That's it updated.

carver · 2018-02-16T05:20:28Z

eth_tester/utils/transactions.py

@@ -0,0 +1,31 @@
+# TODO: Should this be moved to a common package like eth-utils?


Yeah, VALID_TRANSACTION_PARAMS seems to have popped up in several places already, so worth talking about extracting it. eth-utils feels a little wrong, nothing else in there is quite so tightly bound to chain implementation details as "which fields are in a transaction". Maybe we could add a Transaction rlp class to https://github.com/ethereum/eth-rlp and have VALID_TRANSACTION_PARAMS be something like Transaction.fields.

Sounds good. I thought maybe the utility functions could also go somewhere. Would like to tackle this as a separate issue as both eth-tester and web3.py need changed.

Note that we shouldn't call it Transaction because we're on the cusp of having multiple disparate transaction types with different fields. Should be namespaced in some way to allow for easy introduction of new transaction types.

carver · 2018-02-16T05:23:02Z

eth_tester/main.py

@@ -285,6 +326,10 @@ def get_transaction_receipt(self, transaction_hash):
    #
    def enable_auto_mine_transactions(self):
        self.auto_mine_transactions = True
+        sent_transaction_hashes = [self.send_transaction(extract_valid_transaction_params(tx))


Can we be strict here and reject transactions that have invalid transaction params?

I think this is too much of un unlikely scenario to be handled. These transactions are added to the list from get_transaction_by_hash which should be valid. The only real purpose of this extraction is to not send the params like v,r,s from the dict that is returned by get_transaction_by_hash.

We could add validation when we add the transaction to the list but I don't really see the purpose.

These transactions are added to the list from get_transaction_by_hash

Fair enough, I'm okay skipping this validation.

carver · 2018-02-17T19:50:09Z

eth_tester/main.py

+        if not self.auto_mine_transactions:
+            snapshot = self.take_snapshot()
+
+        transaction_hash = func(self, *args, **kwargs)


I think we should revert if func raises an exception too.

Maybe we should just have two main blocks, whether auto_mine is on, and just duplicate the func() call, like:

+ if self.auto_mine_transactions: + transaction_hash = func(self, *args, **kwargs) + self.mine_block() + else: + snapshot = self.take_snapshot() + try: + transaction_hash = func(self, *args, **kwargs) + pending_transaction = self.get_transaction_by_hash(transaction_hash) + # Remove any pending transactions with the same nonce + self._pending_transactions = remove_matching_transaction_from_list( + self._pending_transactions, pending_transaction) + self._pending_transactions.append(pending_transaction) + finally: + self.revert_to_snapshot(snapshot)

I find that easier to follow than the repeated "if auto-mine" tests.

Ah yes that definitely is easier to read 😄

pipermerriam

@monokh if this is ready, it looks good to me.

pipermerriam · 2018-02-19T19:36:51Z

@monokh lemme know if your ready for me to merge/release this.

monokh · 2018-02-19T22:03:05Z

@pipermerriam Yes, please merge and release :) then I can update ethereum/web3.py#586 to use the new version. I tested locally and all the failing tests were resolved with these 'eth-tester' changes

Exclude huge and unnecessary venv*/ from dist

Handle pending transactions: initial implementation

5bdd09f

monokh changed the title ~~Handle pending transactions: initial implementation~~ Handle pending transactions Feb 10, 2018

monokh commented Feb 10, 2018

View reviewed changes

Handle pending transactions: modify tx receipt test

33fa647

monokh force-pushed the handle-pending-transactions branch from a54e162 to 33fa647 Compare February 10, 2018 14:16

monokh added 4 commits February 11, 2018 17:37

Handle pending transactions: test for replacement transaction

d8f5e94

Handle pending transactions: store full tx in pending txs

806c16a

Handle pending transactions: removing pending txs only duplicate nonc…

3e65cc4

…es from same account

Handle pending transactions: get transaction by hash for pending txs

246f0c3

monokh commented Feb 12, 2018

View reviewed changes

monokh added 2 commits February 12, 2018 15:18

Handle pending transactions: test for multiple accounts being used

7c1f82e

Handle pending transactions: test for enabling mining returning corre…

27079c1

…ct txs

monokh force-pushed the handle-pending-transactions branch from 9d5b036 to 27079c1 Compare February 12, 2018 09:45

pipermerriam requested changes Feb 12, 2018

View reviewed changes

monokh added 2 commits February 13, 2018 12:14

Handle pending transactions: Extract transaction matching into util

afaee7c

Handle pending transactions: getting pending transaction will raise w…

b483e29

…hen not found

This was referenced Feb 15, 2018

Resend transaction API ethereum/web3.py#586

Merged

API for re-sending transactions ethereum/web3.py#492

Closed

carver requested changes Feb 16, 2018

View reviewed changes

Handle pending transactions: handle auto mining in try finally block

bc0018c

carver reviewed Feb 17, 2018

View reviewed changes

Handle pending transactions: cleanup auto mining decorator

cb6d096

carver approved these changes Feb 18, 2018

View reviewed changes

pipermerriam approved these changes Feb 19, 2018

View reviewed changes

carver merged commit 0b18e94 into ethereum:master Feb 20, 2018

carver mentioned this pull request Feb 23, 2018

Overwritten transactions are not removed #57

Closed

ynouri mentioned this pull request Feb 25, 2018

Traceback when sending transaction without nonce ethereum/web3.py#613

Closed

pacrob pushed a commit to pacrob/eth-tester that referenced this pull request Oct 25, 2023

Merge pull request ethereum#60 from ethereum/exclude-venv-from-dist

0144dd9

Exclude huge and unnecessary venv*/ from dist

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle pending transactions #60

Handle pending transactions #60

monokh commented Feb 10, 2018 •

edited

Loading

monokh Feb 10, 2018

monokh Feb 11, 2018

monokh Feb 12, 2018

pipermerriam Feb 12, 2018

carver Feb 12, 2018

monokh Feb 12, 2018

pipermerriam Feb 12, 2018

carver Feb 12, 2018

monokh commented Feb 12, 2018

pipermerriam Feb 12, 2018

pipermerriam Feb 12, 2018

pipermerriam Feb 12, 2018

pipermerriam Feb 12, 2018

monokh Feb 13, 2018

carver left a comment

carver Feb 16, 2018

monokh Feb 16, 2018

carver Feb 16, 2018 •

edited

Loading

monokh Feb 17, 2018

carver Feb 16, 2018

monokh Feb 16, 2018

pipermerriam Feb 19, 2018 •

edited

Loading

carver Feb 16, 2018

monokh Feb 16, 2018

carver Feb 16, 2018

carver Feb 17, 2018 •

edited

Loading

monokh Feb 18, 2018

pipermerriam left a comment

pipermerriam commented Feb 19, 2018

monokh commented Feb 19, 2018

		@@ -0,0 +1,31 @@
		# TODO: Should this be moved to a common package like eth-utils?

Handle pending transactions #60

Handle pending transactions #60

Conversation

monokh commented Feb 10, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

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

monokh commented Feb 12, 2018

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

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver Feb 16, 2018 • edited Loading

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

pipermerriam Feb 19, 2018 • edited Loading

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

carver Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam left a comment

Choose a reason for hiding this comment

pipermerriam commented Feb 19, 2018

monokh commented Feb 19, 2018

monokh commented Feb 10, 2018 •

edited

Loading

carver Feb 16, 2018 •

edited

Loading

pipermerriam Feb 19, 2018 •

edited

Loading

carver Feb 17, 2018 •

edited

Loading