++ glad to see this is still here - we need to be able to claim this lock to avoid issues around recheck/commit.

See further: https://github.com/hyperledger/burrow/blob/develop/consensus/tendermint/abci/app.go#L321-L344

This needs to be fixed

can't it just return *mempoolTx?

or is that the same problem?

No, because later in the broadcastTxRoutine loop we're calling next.WaitNextChan followed by next.Next(). I don't think we want either of these methods in the MempoolTx interface.

What we want is

a simple interface for Reactor to subscribe/get txs from the mempool, so it can broadcast them to peers (order of txs for a single peer must be preserved).

Current design 👍 👍

A bunch of goroutines (broadcastTxRoutine) are blocked on a single channel. Once the channel is closed, they all access concurrent list (clist) and send txs to peers (preserving order). Later, at some point, the channel is recreated.

I like the current implementation, but struggling to find a good abstraction for it (to separate mempool and mempool reactor).

Iterator interface (plus a channel to get notified about txs) 👎

type Iterator interface {
  Next() bool
  Tx() MempoolTx
}

type Mempool interface {
  # creates an iterator starting from Nth item in the list/tree/any ordered structure
  Iterator(offset int) Iterator
  
  # Closed once there're txs in the mempool. Reopened after.
  TxsWaitChan() <-chan struct{}
}

Txs method (plus a channel to get notified about txs) 👎

type Mempool interface {
  # returns an array of txs in the mempool
  Txs(n, offset int) []MempoolTx
  
  # Closed once there're txs in the mempool. Reopened after.
  TxsWaitChan() <-chan struct{}
}

A single channel exposed by mempool 👍

type Mempool interface {
  TxsChan() <-chan MempoolTx
}

exposing a single channel should allow us to remove TxsWaitChan() <-chan struct{} method. But it's gets messy when it comes down to impl details - unbuffered vs buffered channel + we'll need an additional channel for each peer.

On the second thought, maybe it's okay for MempoolReactor to require the CList implementation for now. It can be a follow-up PR.

#3589

don't understand why it was exposed in the first place

Not sure the reasoning, but as mentioned above we need the Mempool locker and until you added node.Mempool() (in this PR) node.MempoolReactor().Mempool was the only way to get it...

Come to think of it I think it was the only way to get CheckTx too, which I've always found handy :)

same here. seems like an implementation detail

remove and force people to always use CheckTxWithInfo?

Probably a good idea to discuss this in a separate issue.

#3590

better abstraction for stats?

Why is this a pointer while in rpc/core/pipe.go it was changed to not be a pointer?

Because node's mempool calls InitWAL/CloseWAL, but I don't want to add these methods to Mempool interface

I created mempoolWithWAL interface

Made them a part of Mempool interface for now.

1. WAL is an implementation details and shouldn't really be a part of the interface.
2. But we need a way to init/close associated resources
3. And we need a way to configure them (wal path, etc.)

so we should either make node.Mempool a concrete type OR make InitWAL/CloseWAL/ConfigureWAL a part of any mempool interface (or make them more abstract like Start/Stop/Config)

Same: is there an issue tracking this?

I don't think so. There's #1993, which is one of the bugs

cannot use mempool (variable of type *mempool.Mempool) as mempool.Mempool value in struct literal: missing method CheckTx (from typecheck)

File is not gofmt-ed with -s (from gofmt)