review SWAP locks

[mortelli]

This issue stems from a review for PR #1554. Original comment here.

Currently, the Add function applies the swap lock for all of its code. In light of this, we should take a look at the following:

• Do we need this lock applied for all of the Add function?
• Do we need to apply locks anywhere else in the swap code?
• Should we replace some of the Lock() calls with RLock() calls? What about the reverse?
• Do we need more than 1 lock variable? When Lock() is called on the Swap.lock variable, none of the code surrounded by lock calls from this variable can be accessed. These lines of codes might not actually be related to each other, i.e. they might deal with completely different resources, which don't need to have mutually exclusive access between themselves.

Additionally, we are omitting a lock in the getPeer function based solely on the fact that the Add function calls in turn the getPeer function, and nested locks cause a deadlock.

This is suspicious of bad design, so it should be reviewed as well.

[mortelli]

We haven't been able to reach a consensus on these issues.

A meeting needs to be set up—including members of the incentives and core tracks—to end the discussion and settle on the implementation.

[mortelli]

We are going to move towards removing the getter/setter functions and the extra locks, based on the following comments made in PR #1554:

if all these calls are under accountingLock there is no need for extra lock here`
and if there is no lock, there is no need for these functions.
Except that they should include loading from store and not the other way round.
Same for cheques

Originally posted by @zelig

@mortelli @holisticode if you want to use getters and setters - i think they should under a lock just the way they are here.
that being said i do feel you could get rid of the getters and setters altogether and just handle the maps directly under the accounting lock in the methods that actually do the business logic. it would make the behavior of the code easier to read and understand. IMO when using getters and setters, having locks on just some of them is kind of inviting trouble and future deadlocks and quirks are possibly introduced.

Originally posted by @acud

My opinion is to try to minimize the number of locks. There is probably an unnecessary locking for maps. This also can be optimized later.

Originally posted by @janos

[mortelli]

I'm temporarily suspending work on this one due to what's planned for #1604.

[mortelli]

Closed by #1604 (PR #1725).

It seems this locking scheme introduced in #1725 makes sense—and somewhat fixes the original problem in this issue.

However, we have not found a way to programmatically test this scheme yet.

I am opening a new issue for this purpose.