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

shuffle pending accounts in the transaction pool #501

Closed
wants to merge 3 commits into from

Conversation

JekaMas
Copy link
Contributor

@JekaMas JekaMas commented Aug 23, 2022

Prevents possible vulnerability with pre-mined accounts

Copy link
Contributor

@ssandeep ssandeep left a comment

Choose a reason for hiding this comment

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

Should we also propose this change in geth?

@temaniarpit27
Copy link
Contributor

@JekaMas can you please look into the CI failure?

}

// ShuffleMap returns a shuffled slice of keys to iterate by
func ShuffleMap[K comparable, V any](m map[K]V) []K {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't deterministically random I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as it uses crypto/rand, it isn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a good explanation: https://stackoverflow.com/a/55925880

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, although it only depends on non cryptographically strong hash and a seed. The PR introduces cryptographically strong solution.

@0xsharma
Copy link
Contributor

0xsharma commented Sep 1, 2022

@JekaMas Can you please have a look at the CI ? Thank you !

@temaniarpit27
Copy link
Contributor

@JekaMas CI is failing. Can you please look into this?

@JekaMas
Copy link
Contributor Author

JekaMas commented Sep 19, 2022

I believe, we need to rethink whole idea of random ordering of transactions - ethereum/go-ethereum#21350

@JekaMas
Copy link
Contributor Author

JekaMas commented Oct 6, 2022

Will redo with proving tests.

@JekaMas JekaMas closed this Oct 6, 2022
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.

5 participants