[tests] add functional test for mempoolreplacement command line arg #11407

instagibbs commented at 9:24 PM on September 26, 2017: member

Currently untested.

instagibbs force-pushed on Sep 26, 2017

jtimon commented at 10:17 PM on September 26, 2017: contributor

utACK 16a1d649728b5e8882117f9db11bfa3213020d02

fanquake added the label Tests on Sep 26, 2017

instagibbs renamed this:
~~add functional test for mempoolreplacement command line arg~~
[tests] add functional test for mempoolreplacement command line arg
on Sep 26, 2017

in test/functional/replace-by-fee.py:548 in 16a1d64972 outdated

 541 | @@ -539,5 +542,31 @@ def test_rpc(self):
 542 |          assert_equal(json0["vin"][0]["sequence"], 4294967293)
 543 |          assert_equal(json1["vin"][0]["sequence"], 4294967294)
 544 |  
 545 | +    def test_mempool_replace_arg(self):
 546 | +        # ensure that no replacements are allowed with -mempoolreplacement=0
 547 | +        self.stop_nodes()
 548 | +        self.nodes = self.start_nodes(1, self.options.tmpdir, [["-mempoolreplacement=0"]])

promag commented at 9:30 PM on September 27, 2017:

self.restart_node(0, ["-mempoolreplacement=0"])

in test/functional/replace-by-fee.py:569 in 16a1d64972 outdated

 564 | +        mempool = self.nodes[0].getrawmempool()
 565 | +
 566 | +        assert (tx1a_txid in mempool)
 567 | +
 568 | +        self.stop_nodes()
 569 | +        self.nodes = self.start_nodes(1, self.options.tmpdir, self.extra_args)

promag commented at 9:33 PM on September 27, 2017:

self.restart_node(0)

Note that it will use self.extra_args[0] by default.

in test/functional/replace-by-fee.py:566 in 16a1d64972 outdated

 561 | +        tx1b.vout = [CTxOut(int(0.9*COIN), CScript([b'b']))]
 562 | +        tx1b_hex = txToHex(tx1b)
 563 | +        assert_raises_jsonrpc(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx1b_hex, True)
 564 | +        mempool = self.nodes[0].getrawmempool()
 565 | +
 566 | +        assert (tx1a_txid in mempool)

promag commented at 9:35 PM on September 27, 2017:

Nit, remove ()?

jnewbery commented at 6:26 PM on September 28, 2017:

:+1: assert is a statement, not a function

promag commented at 9:36 PM on September 27, 2017: member

utACK 16a1d64.

jonasschnelli commented at 3:41 AM on September 28, 2017: contributor

utACK 16a1d649728b5e8882117f9db11bfa3213020d02

in test/functional/replace-by-fee.py:568 in 16a1d64972 outdated

 563 | +        assert_raises_jsonrpc(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx1b_hex, True)
 564 | +        mempool = self.nodes[0].getrawmempool()
 565 | +
 566 | +        assert (tx1a_txid in mempool)
 567 | +
 568 | +        self.stop_nodes()

jnewbery commented at 6:28 PM on September 28, 2017:

is this restart necessary?

instagibbs commented at 6:49 PM on September 28, 2017:

figured people wouldn't want it disabled for subsequent tests?

jnewbery commented at 7:37 PM on September 28, 2017:

Makes sense, although a stop-start adds a few seconds to the test runtime.

promag commented at 7:52 PM on September 28, 2017:

@jnewbery not with #11006.

instagibbs commented at 7:54 PM on September 28, 2017:

Could just say "this should be the last test to avoid another restart" or something.

jnewbery commented at 9:04 PM on September 28, 2017:

@promag - indeed. I've already concept ACK'ed that. I think it was waiting on input from Wlad? @instagibbs - comment looks good.

jnewbery commented at 6:29 PM on September 28, 2017: member

I've rebased this on master and it breaks (number of arguments to self.start_nodes() has changed since this branched off master)

jnewbery commented at 6:29 PM on September 28, 2017: member

jnewbery commented at 7:04 PM on September 28, 2017: member

(apologies - github.com was giving me errors and it wasn't apparent that my review had been accepted)

instagibbs force-pushed on Sep 28, 2017

instagibbs commented at 7:10 PM on September 28, 2017: member

address nits and rebased onto master

instagibbs force-pushed on Sep 28, 2017

in test/functional/replace-by-fee.py:566 in 803027d7ad outdated

 561 | +        assert_raises_jsonrpc(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx1b_hex, True)
 562 | +        mempool = self.nodes[0].getrawmempool()
 563 | +
 564 | +        assert tx1a_txid in mempool
 565 | +
 566 | +        # We are not restarting the node here to save time, as this is the last test

promag commented at 8:11 PM on September 28, 2017:

@jnewbery stop/start/restart could be deferred to the point they are needed unless a flush option is set. For instance:

start_node(0)
stop_node(0)

would do nothing. This way tests could be more isolated, less dependable of previous tests.

jnewbery commented at 9:07 PM on September 28, 2017:

@promag interesting suggestion. I like the fact that it would make tests more isolated, but I don't like that it takes control of the nodes away from the test-writer. At the moment our tests are all very imperative, and this would be a slight departure from that model.

It's an interesting thought, but one that should be discussed in a separate issue or PR I think.

promag commented at 9:08 PM on September 28, 2017:

separate issue or PR I think.

Sure! Just a thought.

promag commented at 8:19 PM on September 28, 2017: member

utACK 803027d.

jnewbery commented at 9:11 PM on September 28, 2017: member

Looks good to me.

Another way to do this would be to add a second node to this test with -mempoolreplacement=0, and then in test_simple_doublespend() check that self.nodes[0] accepts the replacement and self.nodes[1] rejects it. There are a couple of advantages to doing it in that way:

doesn't introduce delay in stop-starting the node (multiple nodes are started in parallel)
ensures that exactly the same transaction is accepted by node0 and rejected by node1. Eliminates the possibility that the transaction in test_mempool_replace_arg() was rejected by the mempool for some other mempool conflict reason.

Thoughts?

promag commented at 9:18 PM on September 28, 2017: member

Thoughts? @jnewbery LGTM.

add functional test for mempoolreplacement command line arg 1088b5322d

instagibbs force-pushed on Sep 29, 2017

instagibbs commented at 3:31 PM on September 29, 2017: member

updated the test thanks to @jnewbery 's suggestion.

One thing I wasn't sure of was a fast way of checking for mempool rejection at the p2p layer. The last check I'm doing could definitely be racy, but I can't call self.sync_all() since the mempools are bound to diverge.

in test/functional/replace-by-fee.py:137 in 1088b5322d

 132 |          # Extra 0.1 BTC fee
 133 |          tx1b = CTransaction()
 134 |          tx1b.vin = [CTxIn(tx0_outpoint, nSequence=0)]
 135 |          tx1b.vout = [CTxOut(int(0.9*COIN), CScript([b'b']))]
 136 |          tx1b_hex = txToHex(tx1b)
 137 | +        # Replacement still disabled even with "enough fee"

jnewbery commented at 4:13 PM on September 29, 2017:

ubernit: I think this comment could be clearer by adding "when transaction replacement is disabled"

ultranit: swap order with the line below, so you send to node[0] then node[1]

jnewbery commented at 4:13 PM on September 29, 2017: member

Tested ACK 1088b5322d0e7a8366a285e2baa49c766a9ba5bd. Looks good!

One very nitty nit inline. Take or leave as you see fit.

MarcoFalke commented at 7:28 PM on October 2, 2017: member

utACK 1088b5322

MarcoFalke merged this on Oct 2, 2017

MarcoFalke closed this on Oct 2, 2017

MarcoFalke referenced this in commit 8ddf60db7a on Oct 2, 2017

MarcoFalke referenced this in commit 2e432a9d6a on Oct 3, 2017

MarcoFalke referenced this in commit f224100eb8 on Oct 3, 2017

MarcoFalke referenced this in commit 9c534ec47e on Oct 3, 2017

MarcoFalke referenced this in commit 86be17a9f9 on Oct 3, 2017

MarcoFalke referenced this in commit 806c78f014 on Oct 3, 2017

MarcoFalke referenced this in commit 57ee73990f on Oct 23, 2017

DrahtBot locked this on Sep 8, 2021