PoS Mempool Balance Ledger

[AeonSw4n]

No description provided.

[AeonSw4n]

Current dependencies on/for this PR:

• feature/proof-of-stake
  • PR PoS Mempool #578
    • PR PoS Mempool Balance Ledger #599 👈
      • PR PoS Mempool Transaction Validation #600
        • PR PoS Database Interface #601
          • PR Storage Nested Context #618
            • PR Storage BoltDB #619
          • PR BoltDB Implementation #617
          • PR PoS Mempool Persister #602
            • PR PoS Mempool Struct #603

This comment was auto-generated by Graphite.

[tholonious]

The logic LGTM overall. Just left some naming suggestions in-line. When reading through the code, I found the terms "Increase" and "Decrease" in the public interface to be ambiguous: i.e. does "Increase" reserve more coins, or does it free coins to the pk's spendable balance so they can be spent?

Since the role of the ledge is to track "reserved" balance, I suggest using more explicit verbs like "Reserve" and "Free" so it's dead-simple for readers to understand what the public functions are doing.

[lazynina]

seems like we're starting to have a bit of a pattern for having a RWLock around a map. Should we create a struct that encapsulates this so we can reuse elsewhere? we could add something like this to generics.go

type RWLockMap[K comparable, V any] struct {
	sync.RWMutex
	_innerMap map[K]V
}

func NewRWLockMap[K comparable, V any]() *RWLockMap[K, V] {
	return &RWLockMap[K, V]{_innerMap: make(map[K]V)}
}

func (rwLockMap *RWLockMap[K, V]) Get(key K) (V, bool) {
	rwLockMap.RLock()
	defer rwLockMap.RUnlock()
	value, exists := rwLockMap._innerMap[key]
	return value, exists
}

func (rwLockMap *RWLockMap[K, V]) Set(key K, value V) {
	rwLockMap.Lock()
	defer rwLockMap.Unlock()
	rwLockMap._innerMap[key] = value
}

func (rwLockMap *RWLockMap[K, V]) Delete(key K) {
	rwLockMap.Lock()
	defer rwLockMap.Unlock()
	delete(rwLockMap._innerMap, key)
}

func (rwLockMap *RWLockMap[K, V]) Size() int {
	rwLockMap.RLock()
	defer rwLockMap.RUnlock()
	return len(rwLockMap._innerMap)
}

and then BalanceLedger can be defined as

type BalanceLedger RWLockMap[PublicKey, uint64]

[AeonSw4n]

Cool idea! Abstracting away a thread-safe map into a reusable struct is a clean solution. However, as I've started implementing this generic and using it in BalanceLedger, I've encountered some issues. The main issue occurred when I tried rewriting the IncreaseBalance function. In this case, we need to perform a read and then a write. We first read the user's balance and then write the balance + amount. The problem is that the RwLockMap functions Get and Set release locks. So, theoretically, some undesirable instruction could sneak its way in between and 180 the balance. We would need something that does a Get and a Set under a single lock.

To solve this, I came up with an Update method:

func (m *RWLockMap[K, V]) Update(key K, expression func(V, bool) V) {
	m.Lock()
	defer m.Unlock()

	value, exists := m._innerMap[key]
	newValue := expression(value, exists)

	m._innerMap[key] = newValue
}

This method works nicely because we can encapsulate the "+ amount" logic inside the expression argument. But even with this function, some things become problematic. Let's say we want to check for the overflow after a read but before the write. In such a case, we want to return an error. However, this means we would need to do this inside the expression, which should, in turn, actually return (V, error). Similarly, Update should now return an error. More complexity.

Though I'm chill with complexity. What do you think? I'm happy to add this generic if you're fine with having the Update method.

[tholonious]

In this case, migrating to a generic RWLockMap data structure may be overkill. You make the call on whether to implement it or not @AeonSw4n. IMO, the code reads clearly as-is and the amount of code de-duplication we gain by implementing the RWLockMap is minimal.