Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC0f144de39fd3: test: test mempool rejection in interface_zmq.py

Summary

Add a test to generate a "R" notification by:

sending a tx on node0 while the nodes are disconnected
verifying that this transaction is added to node0's mempool
generating a conflicting tx on node1
mining the conflicting tx into a block on node1
reconnecting the nodes
checking that the first tx is rejected on node0

Also add noban permission to all nodes to avoid the tx relay delays, because there are a lot of calls to sync_all and this diff adds more of them. This reduces the test run time from 45s to 14s.

This replaces the RBF tests that were in [[https://github.com/bitcoin/bitcoin/pull/19572 | core#19572]] backported in D10306.

Test Plan

test/functional/test_runner.py interface_zmq

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Oct 12 2021, 12:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 12 2021, 12:44

PiRK requested review of this revision.Oct 12 2021, 12:44

teamcity edited the summary of this revision. (Show Details)Oct 12 2021, 12:45

PiRK mentioned this in D10306: Add functional tests for zmq sequence topic and mempool sequence logic.Oct 12 2021, 12:45

teamcity edited the summary of this revision. (Show Details)Oct 12 2021, 12:50

Harbormaster completed remote builds in B16974: Diff 30453.Oct 12 2021, 12:59

Fabien requested changes to this revision.Oct 13 2021, 12:29

Fabien added a subscriber: Fabien.

Fabien added inline comments.

test/functional/interface_zmq.py
120 ↗	(On Diff #30453)	just use self.extra_args here, so you have a single spot to update
211 ↗	(On Diff #30453)	dito
289 ↗	(On Diff #30453)	dito
357 ↗	(On Diff #30453)	You may want to wrap this into a function to avoid having to search for where the difference is from the previous tx. Also you avoid tx1 and tx2 vars which are not needed
358 ↗	(On Diff #30453)	This comment is wrong, you got it the opposite way. Also it's a good idea to make sure tx2 is in the block.
364 ↗	(On Diff #30453)	you could have used `getbestblockhash` but it's not even needed, as you can look at the output from `generatetoaddress` above
374 ↗	(On Diff #30453)	don't reuse the same variable, this is confusing. You should change the name of the previous one to something like txid_to_be_replaced
390 ↗	(On Diff #30453)	Don't mix these random refactors with the actual changes
439 ↗	(On Diff #30453)	what is supposed to be tested here ?
511 ↗	(On Diff #30453)	That belongs to D10306
520 ↗	(On Diff #30453)	dito

This revision now requires changes to proceed.Oct 13 2021, 12:29

PiRK added inline comments.Oct 13 2021, 14:30

test/functional/interface_zmq.py
439 ↗	(On Diff #30453)	It is the same replacement sequence as tested in this diff, but more elaborate thanks to RBF: a replaceable tx is added (A), then bumped (R, A), then the original pre-bump tx is mined into a block (conflict R, block connected C). I don't think there is a way we can replicate that intermediate (R, A), and that's good news for 0-conf :)

Address review.

Refactor the conflicting transaction generation and mining into a method, and used it also in test_mempool_sync to replace the missing RBF notification.

fix a mistake in a comment: the number of remaining tx is 5. I changed it to 4 while when earlier I used range(num_txs - 1), then reverted that change. Just remove the number from the comment, so it cannot get wrong in the future

Harbormaster completed remote builds in B17000: Diff 30491.Oct 13 2021, 15:21

Harbormaster completed remote builds in B17001: Diff 30492.Oct 13 2021, 15:31

Fabien accepted this revision.Oct 13 2021, 15:58

Fabien added inline comments.