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

Fixing replacing transaction with lower gas_price result. #1343

Merged
merged 1 commit into from Jun 21, 2016

Conversation

tomusdrw
Copy link
Collaborator

  1. Transaction tx=(sender1, nonce, gas_price) in future.
  2. Importing transaction tx2=(sender1, nonce, gas_price+k) to current (state nonce changed in the meantime)
  3. tx2 should be imported to current (and it was) but error was returned (which was incorrect)

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Jun 20, 2016
@GitCop
Copy link

GitCop commented Jun 20, 2016

There were the following issues with your Pull Request

  • Commit: 0ada865
    • Your subject line is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

2 similar comments
@GitCop
Copy link

GitCop commented Jun 20, 2016

There were the following issues with your Pull Request

  • Commit: 0ada865
    • Your subject line is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

@GitCop
Copy link

GitCop commented Jun 20, 2016

There were the following issues with your Pull Request

  • Commit: 0ada865
    • Your subject line is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 20, 2016
@gavofyork
Copy link
Contributor

why would (sender1, nonce, price) be in the future yet (sender1, nonce, price+k) be in current? it's only the price which is different, which shouldn't affect current/future, right?

@tomusdrw
Copy link
Collaborator Author

This should never happen if we assume that queue is always notified by state changes (which should actually be the case right know through remove_all method).

The case described here assumes that (sender1, nonce, price) is inserted when state_nonce < nonce (hence going to future) and after that we try to import (sender1, nonce, price+k) but state_nonce == nonce and queue hasn't been notified about that.

@gavofyork
Copy link
Contributor

the possibility that it can happen seems like a major bug in itself, no?

@tomusdrw
Copy link
Collaborator Author

Hmm, It might actually happen when you send transactions locally and manage nonce on your own, but it's imho highly improbable.

@gavofyork
Copy link
Contributor

gavofyork commented Jun 21, 2016

it should be impossible.

can you outline when the transactionqueue would get into such a state?

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Jun 21, 2016

  1. state_nonce=n-1, queue lock to import using RPC, importing (sender1, nonce=n, gas_price) -> Goes to future
  2. Acquire queue lock to import using RPC
  3. New block is coming, state_nonce=n, transaction queue is not notified (cause lock is acquired by RPC)
  4. importing throught RPC (sender1, nonce=n gas_price), state_nonce=n -> Goes to current

@gavofyork
Copy link
Contributor

this could be fixed by having transaction queue notified of nonce-changes after block imports, right?

@gavofyork
Copy link
Contributor

because 3 is a really big problem - it's very important future transactions get into current when the nonce changes.

@tomusdrw
Copy link
Collaborator Author

Transaction queue is notified about that, but because we have separate locks for transaction queue and state (client) described situation may happen - i.e. lock for queue can be held (in RPC), and in the meantime state is updated, but transaction queue is not yet notified because we're waiting for lock (in client).

@gavofyork
Copy link
Contributor

ahh so s/is not notified/is not yet notified/

@gavofyork gavofyork merged commit e0b4eab into master Jun 21, 2016
@gavofyork gavofyork deleted the txqueue-fix-override branch June 21, 2016 11:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants