[tests] add functional test for mempoolreplacement command line arg by instagibbs · Pull Request #11407 · bitcoin/bitcoin

instagibbs · 2017-09-26T21:24:57Z

Currently untested.

jtimon · 2017-09-26T22:17:57Z

utACK 16a1d649728b5e8882117f9db11bfa3213020d02

promag

utACK 16a1d64.

promag · 2017-09-27T21:30:47Z

test/functional/replace-by-fee.py

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

promag · 2017-09-27T21:33:22Z

test/functional/replace-by-fee.py

self.restart_node(0)

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

promag · 2017-09-27T21:35:00Z

test/functional/replace-by-fee.py

Nit, remove ()?

👍 assert is a statement, not a function

jonasschnelli · 2017-09-28T03:41:33Z

utACK 16a1d649728b5e8882117f9db11bfa3213020d02

jnewbery

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

jnewbery · 2017-09-28T18:28:38Z

test/functional/replace-by-fee.py

is this restart necessary?

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

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

@jnewbery not with #11006.

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

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

@instagibbs - comment looks good.

jnewbery

jnewbery

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

instagibbs · 2017-09-28T19:10:35Z

address nits and rebased onto master

promag · 2017-09-28T20:11:39Z

test/functional/replace-by-fee.py

@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.

@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.

separate issue or PR I think.

Sure! Just a thought.

promag · 2017-09-28T20:19:39Z

utACK 803027d.

jnewbery · 2017-09-28T21:11:22Z

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 · 2017-09-28T21:18:04Z

Thoughts?

@jnewbery LGTM.

instagibbs · 2017-09-29T15:31:01Z

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.

jnewbery

Tested ACK 1088b53. Looks good!

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

jnewbery · 2017-09-29T16:13:08Z

test/functional/replace-by-fee.py

        tx1b.vin = [CTxIn(tx0_outpoint, nSequence=0)]
        tx1b.vout = [CTxOut(int(0.9*COIN), CScript([b'b']))]
        tx1b_hex = txToHex(tx1b)
+        # Replacement still disabled even with "enough fee"


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]

maflcko · 2017-10-02T19:28:42Z

utACK 1088b53

…and line arg 1088b53 add functional test for mempoolreplacement command line arg (Gregory Sanders) Pull request description: Currently untested. Tree-SHA512: 2dd9d55a3499844e48b3774df9155fd650220b0761da45d16869570356bb0ed17a88d4efa4302a517dd96e1e9cb34113661b3c9df688736f6849201a3d544deb

Github-Pull: bitcoin#11407 Rebased-From: 1088b53

@instagibbs

…endheaders.py 6d51eae qa: Fix race condition in sendheaders.py (Suhas Daftuar) c96b2e4 qa: Fix replace-by-fee race condition failures (Suhas Daftuar) Pull request description: I think #11407 broke replace-by-fee by introducing a race condition. I was observing frequent failures of replace-by-fee locally, always with a mempool sync failure (the sync call was added in #11407). It appeared to me like there were two causes: sometimes the node would be in IBD and not request the transaction that was relayed; other times the blocks generated in make_utxo wouldn't have relayed quickly enough for the spend of the transaction to be accepted. I believe I've fixed both potential errors. ping @instagibbs Edit: I found a race condition in the sendheaders.py test, where if the verack from the python node wasn't processed before the first block in the test was generated, then no block announcement would go out to that peer, breaking the test. Fixed by adding a sync_with_ping after waiting for verack. Tree-SHA512: 6ad160966e432c151c1ce6e88ae67e60e47123523bda3755cf7697a00e1a5ba38de8561751826e3d7cf0e492f8c2aec298e1b4de8424ebbaf497f099a1ef1d07

instagibbs force-pushed the testmempoolreplacearg branch from e2d922d to 16a1d64 Compare September 26, 2017 21:39

fanquake added the Tests label Sep 26, 2017

instagibbs changed the title ~~add functional test for mempoolreplacement command line arg~~ [tests] add functional test for mempoolreplacement command line arg Sep 26, 2017

promag reviewed Sep 27, 2017

View reviewed changes

jnewbery reviewed Sep 28, 2017

View reviewed changes

instagibbs force-pushed the testmempoolreplacearg branch from 16a1d64 to 95eef3e Compare September 28, 2017 19:09

instagibbs force-pushed the testmempoolreplacearg branch from 95eef3e to 803027d Compare September 28, 2017 19:56

promag reviewed Sep 28, 2017

View reviewed changes

add functional test for mempoolreplacement command line arg

1088b53

instagibbs force-pushed the testmempoolreplacearg branch from 803027d to 1088b53 Compare September 29, 2017 15:29

jnewbery reviewed Sep 29, 2017

View reviewed changes

maflcko merged commit 1088b53 into bitcoin:master Oct 2, 2017

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

add functional test for mempoolreplacement command line arg

806c78f

Github-Pull: bitcoin#11407 Rebased-From: 1088b53

sdaftuar mentioned this pull request Oct 20, 2017

qa: Fix race condition failures in replace-by-fee.py, sendheaders.py #11538

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

instagibbs commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Sep 26, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Sep 28, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Sep 28, 2017

Uh oh!

promag Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 28, 2017

Uh oh!

jnewbery commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 28, 2017

Uh oh!

instagibbs commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

instagibbs commented Sep 26, 2017 •

edited

Loading

jnewbery left a comment •

edited

Loading

jnewbery left a comment •

edited

Loading

promag Sep 28, 2017 •

edited

Loading

jnewbery commented Sep 28, 2017 •

edited

Loading

instagibbs commented Sep 29, 2017 •

edited

Loading