Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core, miner: avoid miscaching while getting sender from tx #14773

Merged
merged 1 commit into from
Sep 7, 2017

Conversation

markya0616
Copy link
Contributor

@markya0616 markya0616 commented Jul 7, 2017

The tx sender is usually mis-cached because of different signer if we are running a new chain. Get the correct signer to get sender cache.

@markya0616 markya0616 force-pushed the cache_from branch 2 times, most recently from 0628250 to 3b6a4aa Compare July 7, 2017 15:55
@karalabe
Copy link
Member

Could you detail a bit what the exact issue was, the scenario where it came up and why this fix solves it? I don't quite understand what you're trying to fix here.

@markya0616
Copy link
Contributor Author

markya0616 commented Jul 10, 2017

In our team, we are trying to set up a private blockchain environment to evaluate the system performance. We found deriveSigner() may derive the wrong signer. We cannot get Sender from cache because of the inconsistent signer. If we don't fix this issue, the performance of Ethereum is not good especially you're running a new chain.

@markya0616 markya0616 force-pushed the cache_from branch 2 times, most recently from 3ca65fb to 2568646 Compare July 17, 2017 03:20
@markya0616
Copy link
Contributor Author

@karalabe Do you have any other comments for this PR?

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karalabe deriveSigner can pick the homestead signer, which is usually not the one cached in the tx struct. It's hard to put a number on this change, but I'm convinced that it's correct and may be faster.

@fjl fjl requested a review from karalabe August 1, 2017 08:19
func (t *TransactionsByPriceAndNonce) Shift() {
signer := deriveSigner(t.heads[0].data.V)
// derive signer but don't cache.
func (t *TransactionsByPriceAndNonce) Shift(signer Signer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pinged @karalabe in a voice call and he doesn't like this change because calling Shift with some signer other than EIP155 can derive a different sender address and mess up the tx set. A suggestion: maybe we should store the signer to use on the TransactionsByPriceAndNonce struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjl Thanks for the feedback. I updated my commit to store the signer in TransactionsByPriceAndNonce and check the sender address before we construct it. Please help review. Thanks!

@fjl
Copy link
Contributor

fjl commented Aug 8, 2017

@karalabe ping

@markya0616
Copy link
Contributor Author

@karalabe Do you have other suggestion for this PR?

@markya0616
Copy link
Contributor Author

@karalabe @fjl Any feedbacks about this PR?

@fjl fjl merged commit c1740e4 into ethereum:master Sep 7, 2017
@karalabe karalabe added this to the 1.7.0 milestone Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants