Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Parity 1.11.6 is rejecting transactions from local accounts #9087

Closed
peterbitfly opened this issue Jul 10, 2018 · 10 comments · Fixed by #9143
Closed

Parity 1.11.6 is rejecting transactions from local accounts #9087

peterbitfly opened this issue Jul 10, 2018 · 10 comments · Fixed by #9143
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust. Z1-question 🙋‍♀️ Issue is a question. Closer should answer.
Milestone

Comments

@peterbitfly
Copy link

peterbitfly commented Jul 10, 2018

Before filing a new issue, please provide the following information.

I'm running:

  • Which Parity version?: 1.11.6
  • Which operating system?: Linux
  • How installed?: binaries
  • Are you fully synchronized?: yes
  • Which network are you connected to?: ethereum
  • Did you try to restart the node?: yes

Your issue description goes here below. Try to include actual vs. expected behavior and steps to reproduce the issue.

The new tx queue implementation in 1.11.6 seems to treat transactions from local accounts different compared to previous versions. Previously the tx queue always accepted transactions from local accounts. The new tx queue implementation rejects them with the following message:

2018-07-10 16:24:30  IO Worker #2 TRACE txqueue  [0x1af684a0ac8011232397277a8b62616b9009b087283a1ae946a58d27555f94a3] Rejected tx early, cause it doesn't have any chance to get to the pool: (gas price: 1000000000 < 13000000000)
2018-07-10 16:26:37  IO Worker #3 TRACE txqueue  [0x1af684a0ac8011232397277a8b62616b9009b087283a1ae946a58d27555f94a3] Rejected tx early, cause it doesn't have any chance to get to the pool: (gas price: 1000000000 < 10000000000)

Expected behavior: Transactions with sender = local account should never be rejected from the tx queue independent of the status of the tx pool.

Edit: Might be caused by 7f4d825

@Tbaut
Copy link
Contributor

Tbaut commented Jul 11, 2018

@tomusdrw can you have a look?
My understanding was that local transaction would make it to the pool no matter what gasPrice they use.

@Tbaut Tbaut added Z1-question 🙋‍♀️ Issue is a question. Closer should answer. Z0-unconfirmed 🤔 Issue might be valid, but it’s not yet known. M4-core ⛓ Core client code / Rust. labels Jul 11, 2018
@Tbaut Tbaut added this to the 2.0 milestone Jul 11, 2018
@tomusdrw
Copy link
Collaborator

@ppratscher are you submitting the transactions via eth_signTransaction or eth_sendRawTransaction? The min effective gas price check is actually guarded by !is_own so the transaction submitted on a local node should never reach that check.

The situation is indeed a bit different if we have a local account, but the transaction is received over the network (i.e. sent from some other node). Is that the use case you are having?

@peterbitfly
Copy link
Author

Yeah, the node does receive that transaction via the P2P network and not via a local RPC call.

@tomusdrw tomusdrw added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. and removed Z0-unconfirmed 🤔 Issue might be valid, but it’s not yet known. labels Jul 11, 2018
@tomusdrw
Copy link
Collaborator

I guess the solution would be to add a CLI flag to bring back the previous behaviour. The optimization here was introduced to improve performance of the node in case the queue is full, and avoid doing (very costly) ecrecover for every incoming transaction.

So with the flag it would be possible to opt-in to the less performant old behaviour.

@ppratscher As a workaround I'd suggest:

  1. Running with larger queue size. 1.11.6 should be able to easily handle large transaction pools (we have one node running with 256k limit with 160k transactions in the pool).
  2. Increasing min-gas-price, so that external transactions don't occupy the entire pool - this will allow local transactions to enter, since minimal effective gas price check won't be in place (it's only activated if the pool is saturated).

@peterbitfly
Copy link
Author

Thanks, we will try to increase the queue size and check how it affects the mining performance and uncle rate of the node. Unfortunately the second workaround is not really an option as we also need to include external transactions in the blocks.

Indeed it would be great to have an opt-in flag for the previous behavior available as the additional ecrecover operations should not be an issue with our hardware.

@peterbitfly
Copy link
Author

@tomusdrw Unfortunately as soon as we increase the tx queue size we are again running into #9070 and the import of a locally mined block takes more than two seconds. The import of normal blocks is in the range of 30-250ms.

Log of a block mined with 1.11.6 and a tx-queue-size of 256000:

2018-07-11 14:17:44  Submitted block imported OK. #5944670: fe17a4872ef9f70310bedb8caa634e9c1de1b523fa334fe6afdfccff24a37e4b
2018-07-11 14:17:46  Imported #5944670 0xfe17…7e4b (274 txs, 7.98 Mgas, 2632 ms, 34.62 KiB)

Log of a block mined with 1.11.6 and a tx-queue-size of 2048 as per your recommendations:

2018-07-11 14:25:10  Submitted block imported OK. #5944706: 11bb26481f82f445f8b976a84fd9badceb8ddbe9c455ab019997d083dec96097
2018-07-11 14:25:10  Imported #5944706 0x11bb…6097 (196 txs, 8.00 Mgas, 117 ms, 36.37 KiB)

Therefore increasing the tx queue size is not feasible for us.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Jul 11, 2018

@ppratscher The long time you are seeing is related to how the informant is implemented, it waits for all the local work to be finished before reporting the block time, but it's not the actual time it takes to import the block. Indeed clearing out larger queues takes longer (seconds), but recently in 1.11.6 new pending block is created before the queue is being cleared (see #9024 (comment)), so it shouldn't increase uncle rate or prevent propagation.

Let me know if that's not the case, I'll look into optimizing that further. A log captured with -lclient=trace,miner=trace would also be helpful (optionally with perf=trace if it doesn't produce too much).

@peterbitfly
Copy link
Author

@tomusdrw I sent you the logs directly via DM on gitter + some additional info

@Tbaut
Copy link
Contributor

Tbaut commented Jul 11, 2018

@ppratscher could you please also post it here (in a gist?) for the record :)

@tomusdrw
Copy link
Collaborator

@Tbaut stratum notification is being sent after chain_new_blocks is finished, so miners are not notified about the new pending block (even though it's already created). I'm preparing a patch soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust. Z1-question 🙋‍♀️ Issue is a question. Closer should answer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants