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

Relock unlocked accounts after first use #1120

Merged
merged 2 commits into from
May 21, 2016
Merged

Relock unlocked accounts after first use #1120

merged 2 commits into from
May 21, 2016

Conversation

gavofyork
Copy link
Contributor

No description provided.

@gavofyork gavofyork added the A0-pleasereview 🤓 Pull request needs code review. label May 21, 2016
@gavofyork
Copy link
Contributor Author

Closes #1086

@arkpar
Copy link
Collaborator

arkpar commented May 21, 2016

Won't this break compatibility with DApps that unlock an account and do a series of transactions?
Also, it is still unsafe. An spamming attacker might eventually put their transaction in front.

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 21, 2016
@gavofyork
Copy link
Contributor Author

gavofyork commented May 21, 2016

i know of no dapps that do transaction batches which assume the account will remain unlocked throughout. in any case it would be a generally broken assumption, since unlockAccount is undocumented geth-specific API which makes no guarantees about how long the account should stay unlocked.

and though not a perfect solution, it's infinitely safer than leaving it as the status quo which leaves it unlocked for 20 minutes.

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels May 21, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 21, 2016
@gavofyork gavofyork merged commit b53d005 into master May 21, 2016
@gavofyork gavofyork deleted the relockonuse branch May 23, 2016 17:20
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.

2 participants