internal/ethapi: add mutex around signing + nonce assignment

[holiman]

Follow-up PR to #14483 .
This PR adds per-sender-account specific mutexes, and removes the pollution of global by placing it inside the AccountManager.

Also enables it for the personal.sendTransaction part of the api

[obscuren]

This can't be right, why are you doing this twice? The am.nonceLock.mut.Lock() should go on top with a defer ...Unlock().

[holiman]

So, it's done twice so that in 99% of all cases, there's no need to even obtain the lock (if the noncelock for that address already exists, it will exit at the first check).

If the noncelock does not exist, it needs to lock the mut. And at that point, it needs to check again, because there may have been two threads doing that unsynchronized first check and now about to create the address-lock.

I don't want to use defer, because there's a tricky thing here, regarding scoping. I'm not sure when the deferred method is executed, but if it's at the end, I don't think it's very nice.

This is how I imagine a deferred will look like without syntactic sugar:

func (am *Manager) LockNonce(address common.Address) {
	if _, exist := am.nonceLock.locks[address]; !exist {
		am.nonceLock.mut.Lock()
		if _, exist := am.nonceLock.locks[address]; !exist {
			am.nonceLock.locks[address] = new(sync.Mutex)
		}
		
	}
	am.nonceLock.locks[address].Lock()
	am.nonceLock.mut.Unlock()
	return
}

The problem with that is that it will try to obtain the addresslock before having released the mut. So if some other process already has the specific addresslock locked, this will stall other callers trying to lock the nonce for totally separate addresses, since they will be unable to obtain mut.
Course, I may be wrong about how defer works (?)

[holiman]

I tested a bit with defer and I think I'm right. But yeah, it could be written as

func (am *Manager) LockNonce(address common.Address) {
	am.nonceLock.mut.Lock()
	if _, exist := am.nonceLock.locks[address]; !exist {
		am.nonceLock.locks[address] = new(sync.Mutex)
	}
	am.nonceLock.mut.Unlock()
	am.nonceLock.locks[address].Lock()
	return
}

That's less code, but all callers will compete on that central lock.

[obscuren]

You're kind of going against Go's principles here though and I'm not sure whether that's a good idea.

defer is executed at the end of the function's scope, including return statement but excluding the actual returning (i.e. setting of return variables, both named and unnamed) and the nicety is that it will be executed regardless of where the return is.

The dual-check is really, really odd and there's an argument for infinite amount of these checks. I strongly suggest this is written as:

func (am *Manager) LockNonce(address common.Address) {
	am.nonceLock.mut.Lock()
        defer am.nonceLock.mut.Unlock()

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

IMO this is better code and less cause for confusion.

[holiman]

I don't like that example, for the precise reason that it keeps holding the mut lock needlessly, while trying to get the address-specific lock. But since you really want me to use the defer pattern, I'll refactor it into two methods instead, so we can avoid that.

[holiman]

The dual-check is really, really odd and there's an argument for infinite amount of these checks.

No? Just two.

1. Check existence (unsynched). If exists, goto 6.
2. sync
3. Check existence (synched). If exists, goto 6.
4. create
5. unsync
6. xxx...

[lmars]

FYI the check -> lock -> check -> create pattern is used in the recently merged sync.Map implementation (see here).

[obscuren]

Interesting. Thanks.

[holiman]

I've changed it now, according to @obscuren 's comments. However, I still think that it is suboptimal the way it is right now, since it will lock the mut regardless if the account-specific lock exists or not (it strictly only needs to sync if it doesn't exist) . But it's not a big deal, I guess.

[obscuren]

We can settle the argument if you can provide proof that your version is significantly more performant :-)

[holiman]

As long as I got away with releasing the outer lock before obtaining the inner lock, I'm can live with the way it is :)

Done

[fjl]

@holiman force-pushed the change we discussed off GitHub. Code changes are now almost entirely local to package ethapi, no new API surface is added.

[holiman]

LGTM!