PoS Mempool Struct

[AeonSw4n]

No description provided.

[AeonSw4n]

Current dependencies on/for this PR:

• feature/proof-of-stake
  • PR PoS Mempool #578
    • PR PoS Mempool Struct #603 👈

This comment was auto-generated by Graphite.

[lazynina]

I think the name is a little confusing is all. @tholonious - how do you feel about the naming convention here? If it's clear to you, then I'm happy to keep it as is so we don't have a long and unwieldy name. I know what it's doing now, but the name didn't make it clear to me at first glance.

[lazynina]

Should we return the error here instead of glogging it?

[tholonious]

I guess the assumption here is that this loop will never error because the txn no longer exists in the transaction register, in which case the dmp.removeTransactionNoLock() call will never error.

@AeonSw4n, IMO it's fine to replace the dmp.removeTransactionNoLock() call here with a helper function that just does the following:

	// Decrease the appropriate ledger's balance by the transaction fee.
	dmp.ledger.DecreaseBalance(*userPk, txn.Fee)

	// Emit an event for the removed transaction.
	if persistToDb {
		event := &MempoolEvent{
			Txn:  txn,
			Type: MempoolEventRemove,
		}
		dmp.persister.EnqueueEvent(event)
	}

This would guarantee there's no error returned, and clarify for us in the future how pruneNoLock() differs from removeTransactionNoLock().

[AeonSw4n]

Yes, this glog should never happen, except maybe if prunedTxn or prunedTxn.Hash were nil. Either way, it's mostly a dummy error handler.

@tholonious re the code split, I prefer making the call to removeTransactionNoLock. It's nice that both "removeTransaction" and "removeTransactioNoLock" erase transactions. This property will not hold for the new "sub-remove" function.

Imo it's also intuitive to call an internal remove function for each pruned txn. Perhaps making assumptions that dmp.removeTransactionNoLock() doesn't panic on non-existing transactions isn't that bad?

[tholonious]

@AeonSw4n, that seems fine. The implementation of dmp.removeTransactionNoLock() is encapsulated within the same struct anyway so that's a reasonable assumption.

Definitely add a comment in the code here that this function call will never error so it's clearer to readers

[lazynina]

Should we move this to a helper function and use t.Cleanup? This way we can reduce code duplication across tests. Maybe even a NewTestPoSMempool function that takes in the appropriate attributes and then cleans itself up properly upon test completion.

[AeonSw4n]

Nice! It's cool how t.Cleanup is like a better defer.

[tholonious]

I guess the assumption here is that this loop will never error because the txn no longer exists in the transaction register, in which case the dmp.removeTransactionNoLock() call will never error.

@AeonSw4n, IMO it's fine to replace the dmp.removeTransactionNoLock() call here with a helper function that just does the following:

	// Decrease the appropriate ledger's balance by the transaction fee.
	dmp.ledger.DecreaseBalance(*userPk, txn.Fee)

	// Emit an event for the removed transaction.
	if persistToDb {
		event := &MempoolEvent{
			Txn:  txn,
			Type: MempoolEventRemove,
		}
		dmp.persister.EnqueueEvent(event)
	}

This would guarantee there's no error returned, and clarify for us in the future how pruneNoLock() differs from removeTransactionNoLock().