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

Lock when auto-filling transaction nonce #14483

Merged
merged 2 commits into from
May 19, 2017
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented May 17, 2017

Supersedes #14395 .
Fixes #14375 .

More context in the bug.
This solves the problems of transactions being submitted simultaneously, and getting the same nonce, due to the gap (due to signing) between nonce-issuance and nonce-update. With this PR, a lock will need to be aquired whenever a nonce is used, and released when the transaction either is submitted or errors out.

A drawback with this solution is that the lock is global, so if the caller wants to send transactions from multiple accounts, they will still have to wait on the same lock. Ideally, one could use some sort of granular lock, granularMutex.lock(<account>). Didn't find any such things in the standard libs, and I'm not go-hardcore enough to build it. And it may be over-engineering anyway.

@holiman holiman requested review from fjl, karalabe and bas-vk May 17, 2017 12:34
@GitCop
Copy link

GitCop commented May 17, 2017

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 3cc0155
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


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

@holiman holiman changed the title Syncnonce Lock when auto-filling transaction nonce May 17, 2017
@obscuren
Copy link
Contributor

Can we improve this per your suggestion? Can we keep a map of mutexes for each account?

@holiman
Copy link
Contributor Author

holiman commented May 18, 2017

Well, my suggestion was more that I thought there ought to exist, already, some form of lock primitive, perhaps based around atomic integers, which could internally use sublocks.

It feels a bit clumsy to hold a map of mutexes.. and then you need an outer mutex while checking/creating the 'submutex'...

@obscuren
Copy link
Contributor

You can use a RWMutex for the map mutex, it's not that bad. I think you'll get very limited amount of write locks on the map lock since creation/deletion of new addresses isn't that common (??).

@obscuren
Copy link
Contributor

obscuren commented May 18, 2017

Something like this which is using a sync.Mutex

type accounts struct {
    mut sync.Mutex
    map[common.Address]*sync.Mutex
}

func (a accounts) Lock(address common.Address) {
    a.mut.Lock()
    defer a.mut.Unlock()

    
    if _, exist := a[address]; !exist {
        a[address] = new(sync.Mutex)
    }
    a[address].Lock()
}

or if you want to go fancy you could register on the account creation event, add a new mutex whenever you receive the event and remove the whole if _, exist; !exist {. This allows you to RLock() instead of Lock.

@holiman
Copy link
Contributor Author

holiman commented May 18, 2017

I suspect it's all overengineering..

so this may be over-overengineering, but how about:

type accounts struct {
    mut sync.Mutex
    map[common.Address]*sync.Mutex
}

func (a accounts) Lock(address common.Address) {

    if lock, exist := a[address]; exist {
	    lock.Lock()
	    return
    }

    a.mut.Lock()
    defer a.mut.Unlock()
    if _, exist := a[address]; !exist {
	    a[address] = new(sync.Mutex)
    }
    a[address].Lock()
}
func (a accounts) Unlock(address common.Address) {

    if lock, exist:= a[address]; exist {
	    lock.Unlock()
	    return
    }
    //panic ?
}

@fjl fjl merged commit 83721a9 into ethereum:master May 19, 2017
@holiman holiman deleted the syncnonce branch May 19, 2017 13:09
@karalabe karalabe added this to the 1.6.2 milestone May 22, 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.

None yet

5 participants