Page MenuHomePhabricator

test: test mempool rejection in interface_zmq.py
ClosedPublic

Authored by PiRK on Oct 12 2021, 12:44.

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 requested review of this revision.Oct 12 2021, 12:44
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
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

Fabien added inline comments.
test/functional/interface_zmq.py
118 ↗(On Diff #30492)

Nit: the extra_args being the "constant" part they should be placed first

This revision is now accepted and ready to land.Oct 13 2021, 15:58