Skip to content

Commit e9e4c8a

Browse files
Danielmergify[bot]
authored andcommitted
test(mempool): enhanced TestReactorNoBroadcastToSender (cometbft#4127)
The test should be able now to catch the error introduced by cometbft#3657. ---- To see how it would break, please checkout the branch `cason/test-nobroadcastsender`, then: ``` $ cd mempool $ go test -v -run TestReactorNoBroadcastToSender ``` And wait it to fail, after 2 minutes. You are going to see the busy-loop around, logs entries starting with `Skipping tx`, forever for the same transaction. Important: the test unit only works in the traditional mempool design, without lanes. We should consider adding a variant of it that should also pass when lanes are considered. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent ef20d5d commit e9e4c8a

File tree

1 file changed

+19
-7
lines changed

1 file changed

+19
-7
lines changed

mempool/reactor_test.go

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func TestReactorConcurrency(t *testing.T) {
128128
func TestReactorNoBroadcastToSender(t *testing.T) {
129129
config := cfg.TestConfig()
130130
const n = 2
131-
reactors, _ := makeAndConnectReactors(config, n, nil)
131+
reactors, _ := makeAndConnectReactorsNoLanes(config, n, nil)
132132
defer func() {
133133
for _, r := range reactors {
134134
if err := r.Stop(); err != nil {
@@ -151,6 +151,7 @@ func TestReactorNoBroadcastToSender(t *testing.T) {
151151

152152
// The second peer sends some transactions to the first peer
153153
secondNodeID := reactors[1].Switch.NodeInfo().ID()
154+
secondNode := reactors[0].Switch.Peers().Get(secondNodeID)
154155
for i, tx := range txs {
155156
shouldBroadcast := cmtrand.Bool() || // random choice
156157
// Force shouldBroadcast == true to ensure that
@@ -162,11 +163,11 @@ func TestReactorNoBroadcastToSender(t *testing.T) {
162163

163164
if !shouldBroadcast {
164165
// From the second peer => should not be broadcast
165-
_, err := reactors[0].mempool.CheckTx(tx, secondNodeID)
166+
_, err := reactors[0].TryAddTx(tx, secondNode)
166167
require.NoError(t, err)
167168
} else {
168169
// Emulate a tx received via RPC => should broadcast
169-
_, err := reactors[0].mempool.CheckTx(tx, "")
170+
_, err := reactors[0].TryAddTx(tx, nil)
170171
require.NoError(t, err)
171172
txsToBroadcast = append(txsToBroadcast, tx)
172173
}
@@ -493,13 +494,18 @@ func mempoolLogger(level string) *log.Logger {
493494
}
494495

495496
// makeReactors creates n mempool reactors.
496-
func makeReactors(config *cfg.Config, n int, logger *log.Logger) []*Reactor {
497+
func makeReactors(config *cfg.Config, n int, logger *log.Logger, lanesEnabled bool) []*Reactor {
497498
if logger == nil {
498499
logger = mempoolLogger("info")
499500
}
500501
reactors := make([]*Reactor, n)
501502
for i := 0; i < n; i++ {
502-
app := kvstore.NewInMemoryApplication()
503+
var app *kvstore.Application
504+
if lanesEnabled {
505+
app = kvstore.NewInMemoryApplication()
506+
} else {
507+
app = kvstore.NewInMemoryApplicationWithoutLanes()
508+
}
503509
cc := proxy.NewLocalClientCreator(app)
504510
mempool, cleanup := newMempoolWithApp(cc)
505511
defer cleanup()
@@ -522,15 +528,21 @@ func connectReactors(config *cfg.Config, reactors []*Reactor, connect func([]*p2
522528
return p2p.StartAndConnectSwitches(switches, connect)
523529
}
524530

531+
func makeAndConnectReactorsNoLanes(config *cfg.Config, n int, logger *log.Logger) ([]*Reactor, []*p2p.Switch) {
532+
reactors := makeReactors(config, n, logger, false)
533+
switches := connectReactors(config, reactors, p2p.Connect2Switches)
534+
return reactors, switches
535+
}
536+
525537
func makeAndConnectReactors(config *cfg.Config, n int, logger *log.Logger) ([]*Reactor, []*p2p.Switch) {
526-
reactors := makeReactors(config, n, logger)
538+
reactors := makeReactors(config, n, logger, true)
527539
switches := connectReactors(config, reactors, p2p.Connect2Switches)
528540
return reactors, switches
529541
}
530542

531543
// connect N mempool reactors through N switches as a star centered in c.
532544
func makeAndConnectReactorsStar(config *cfg.Config, c, n int, logger *log.Logger) ([]*Reactor, []*p2p.Switch) {
533-
reactors := makeReactors(config, n, logger)
545+
reactors := makeReactors(config, n, logger, true)
534546
switches := connectReactors(config, reactors, p2p.ConnectStarSwitches(c))
535547
return reactors, switches
536548
}

0 commit comments

Comments
 (0)