[Core] Externalize a shared Mempool interface & implementation - (Issue #388)

[deblasis]

Description

This PR Makes the Mempool interface and implementation a common high-level type that can be reused across different modules

Issue

Fixes #388

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Created generic/configurable fifo mempool
• Implemented TxMempool variant WIP

Testing

• make develop_test
• LocalNet w/ all of the steps outlined in the README

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[deblasis]

Hey @Olshansk

I would really appreciate a quick glance at this one, ONLY TWO FILES!
It's the first pass. Kinda like my design idea. If you like where this is going I'll do some hardening, implement also the P2P variant and of course add tests.

Au contraire, if this sucks just delete the branch and let me know :)

🙏

[Olshansk]

Fascinating. I like the direction and also see us just open-sourcing this as a standalone
package.

Please proceed!

[deblasis]

The next batch, that will include the below todos, should be the last one and then I guess this is ready for a full-fledged review.

TODO:

• tests for utility.mempool
• tests for GenericFIFOSet

I have added tests for the simplest implementation to give you an idea of what I am doing.

[deblasis]

Looking for a couple of smarter eyes on this, E2E tests in consensus are failing at the moment, I am sure it's something stupid but I have tunnel vision now

--- FAIL: TestPacemakerTimeoutIncreasesRound (0.61s)
    utils_test.go:468: [⌚ CLOCK ⏩] advanced by 10ms
    utils_test.go:462: [⌚ CLOCK ⌚] the time is: 10 ms from UNIX Epoch [1970-01-01 00:00:00.01 +0000 UTC]
    pacemaker_test.go:50: 
                Error Trace:    /home/alex/CODE/OSS/pocket/consensus/e2e_tests/pacemaker_test.go:50
                Error:          Received unexpected error:
                                Missing 'consensus.HotstuffMessage' messages; 16 expected but 0 received. (HotStuff step: HOTSTUFF_STEP_NEWROUND, type: HOTSTUFF_MESSAGE_PROPOSE)
                Test:           TestPacemakerTimeoutIncreasesRound

[deblasis]

As discussed on discord, I identified and addressed the issue in c0f7a39

From discord:

it seems that my machine is slower than usual and the previous timeout value wasn't providing enough time for consensus to work its way till the end.

A larger number in there doesn't change the logic of the tests because we use relative values to trigger timeouts, it just gives more real time for the messages to reach the eventsChannel and be checked down the line if that makes sense

@Olshansk , @gokutheengineer and myself concurred that there's some room for improvement wrt determinism, a followup issue/PR will make life and DevEx easier going forward 🚀

[deblasis]

Yo @Olshansk! I think this is ready for another pass. PTAL 🙏

[Olshansk]

Suggested change

	// DISCUSS: could this be improved by incrementally keeping track of highQC when we add/remove messages to the mempool? Probably premature optimization for now but something to keep in mind.
	// CONSIDERATION(M5): could this be improved by incrementally keeping track of highQC when we add/remove messages to the mempool? Probably premature optimization for now but something to keep in mind.

[Olshansk]

Kind of.

To me it just seems to make more sense to lock on the outer structure before operating on the inner structure.

Might not make a difference though.

[Olshansk]

This test suite is REALLY good

[Olshansk]

One last round of small comments and we should be able to merge it in tomorrow

[Olshansk]

Same as before. My inutition just makes me think:

1. Lock outer structure
2. Clear inner structure
   2.1. Capture inner lock
   2.2. Unlock inner lock
3. Clear outer strcture
4. Unlock outer structure

[deblasis]

My 🚂 of 🤔 is:

1. Do what the generic implementation does,
2. Do what's special to this implementation

Basically this: b875b37

[Olshansk]

Won't push back against this since it doesn't matter.

---

Going to share my personal 🚄 of 🤔 thought:

1. In polymorphism (when a subclass inherits from a superclass), we usually do:

super.constructor()
self.constructioner()

2. In this case, where the generic implementation is an internal field, I'm thinking:

self.clearFields()
attributes.clearFields()

[deblasis]

Well, it's not a rule, you can call the super before or after depending on how the subclass needs to behave.

If for example super.constructor() prints something that is dependent on some internal state (accessible and modifiable by the self, it makes sense for the self.constructioner() to happen before.

[Olshansk]

1. Remove pointer?
   1.1 Why are we doing a pointer to a slice? When slices are passed around, it's already just a reference to the first element.
   1.2 If we do this, we should do it for actions too

2. If we don't do this, why is this a pointer but wantItems is not?

The diagram here helped me think through it: https://medium.com/swlh/golang-tips-why-pointers-to-slices-are-useful-and-how-ignoring-them-can-lead-to-tricky-bugs-cac90f72e77b

[deblasis]

I normally use pointers in tests like these when I feel like I might not want to configure some cases with InitialTxs

Basically to provide that flexibility to the tester.

if nil we just skip that.

[Olshansk]

I see the use case and raise you this mini demo:

If we just don't set a slice, it's nil by default.

I would request one of following:

1. Document (in the struct next to the field) why it it's a pointer
2. Not use a pointer at all

Source code for reference:

package main

import "fmt"

func main() {
	type args struct {
		ptrSlice *[]byte
		slice    []byte
	}

	a := &args{
		ptrSlice: &[]byte{},
		slice:    []byte{},
	}

	b := &args{}

	fmt.Println("a.ptrSlice:", a.ptrSlice == nil)
	fmt.Println("a.slice   :", a.slice == nil)

	fmt.Println("b.ptrSlice:", b.ptrSlice == nil)
	fmt.Println("b.slice   :", b.slice == nil)
}

[deblasis]

💯 but I meant it to be used like this:

[deblasis]

@Olshansk I think this is almost there

a couple of open points that might have a quick turnaround anyway but yeah, I like this.
Thanks for your inputs 🙏

[Olshansk]

@deblasis Just followed up on one comment and nothing else: #437 (comment)

[Olshansk]

@deblasis Can you just add a short comment on why we have pointers for the actions and initialTxs (w.r.t. nil comparison)? Otherwise, let's 🚢