[qa] Change default block priority size to 0

MarcoFalke commented at 9:27 pm on December 6, 2015: member

Unit test should not run a “different branch” by default.

This is a cleanup PR of #7022

in qa/rpc-tests/mempool_reorg.py: in faa2ddf1cd outdated

53@@ -54,9 +54,9 @@ def run_test(self):
54         # and make sure the mempool code behaves correctly.
55         b = [ self.nodes[0].getblockhash(n) for n in range(101, 105) ]
56         coinbase_txids = [ self.nodes[0].getblock(h)['tx'][0] for h in b ]
57-        spend_101_raw = self.create_tx(coinbase_txids[1], node1_address, 50)
58-        spend_102_raw = self.create_tx(coinbase_txids[2], node0_address, 50)
59-        spend_103_raw = self.create_tx(coinbase_txids[3], node0_address, 50)

dcousens commented at 0:52 am on December 7, 2015:

For those unfamiliar with this test, could you explain why this is necessary?

MarcoFalke commented at 8:03 am on December 7, 2015:

By changing the amount you send in a raw transaction, you adjust the fee. Thus we get a 0.01 BTC fee here. (The exact fee does not matter for this test but it should be 0.0148 BTC > fee > 0.00001 BTC)
Current master is broken with free transactions (because it is already running with blockprioritysize=0), so we need the fee here

dcousens commented at 10:20 am on December 9, 2015:

Thanks @MarcoFalke

morcos commented at 2:30 am on December 7, 2015: member

@MarcoFalke were these the only changes necessary to make all the RPC tests work with fees only and no longer depend priority? If so, that’s fantastic, huge concept ACK.

MarcoFalke commented at 8:24 am on December 7, 2015: member

@morcos Yes, as pointed out in https://github.com/luke-jr/bitcoin/commit/e4d7271457140ef36b2b0eee7ebb5783e8ccadb4#commitcomment-14810804 there is currently no rpc test to exercise priority sufficiently. (If you look at the diff, the only check was “Create a free transaction and a child and see if the child pays a fee.” )

MarcoFalke force-pushed on Dec 8, 2015

jonasschnelli added the label Tests on Dec 9, 2015

jonasschnelli commented at 7:58 am on December 9, 2015: contributor

@MarcoFalke: I’m not fully understanding this PR (and it seems other also have problem with it). Could you update the PR description?

MarcoFalke commented at 8:01 am on December 9, 2015: member

Oops, forgot to mention this is a cleanup PR of #7022. (Ideally it should have been squashed in there)

MarcoFalke commented at 7:27 pm on December 10, 2015: member

I’ve been running the extended tests (excluded pruning.py) with assert(fee!=0) in mempool to verify there are no zero-fee transactions any more. Haven’t yet checked for free transactions (per policy) though.

morcos commented at 9:33 pm on December 10, 2015: member

ACK Thanks for doing this, I agree I should have done it originally.

petertodd commented at 4:09 am on December 11, 2015: contributor

Concept ACK

dcousens commented at 5:15 am on December 11, 2015: contributor

utACK

MarcoFalke force-pushed on Jan 7, 2016

MarcoFalke commented at 6:46 pm on January 7, 2016: member

Travis passes
Extended tests do not fail with assert(nFees!=0); and assert(nFees >= ::minRelayTxFee.GetFee(nSize)); in mempool.

If someone wants to verify this, maybe the following diff might be useful:

$ qa/pull-tester/rpc-tests.py -extended; git diff

 0diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py
 1index e7173fd..bb89d65 100755
 2--- a/qa/pull-tester/rpc-tests.py
 3+++ b/qa/pull-tester/rpc-tests.py
 4@@ -117,5 +117,5 @@ testScriptsExt = [
 5     'txn_doublespend.py',
 6     'txn_clone.py --mineblock',
 7-    'pruning.py',
 8+#    'pruning.py', # too long
 9     'forknotify.py',
10     'invalidateblock.py',
11diff --git a/qa/rpc-tests/prioritise_transaction.py b/qa/rpc-tests/prioritise_transaction.py
12index 4a79d38..2ada1be 100755
13--- a/qa/rpc-tests/prioritise_transaction.py
14+++ b/qa/rpc-tests/prioritise_transaction.py
15@@ -107,10 +107,12 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
16
17         try:
18-            self.nodes[0].sendrawtransaction(tx2_hex)
19+            # Don't send 0 fee transaction
20+            # self.nodes[0].sendrawtransaction(tx2_hex)
21+            pass
22         except JSONRPCException as exp:
23             assert_equal(exp.error['code'], -26) # insufficient fee
24             assert(tx2_id not in self.nodes[0].getrawmempool())
25         else:
26-            assert(False)
27+            assert(True)
28
29         # This is a less than 1000-byte transaction, so just set the fee
30@@ -120,6 +122,6 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
31
32         print "Assert that prioritised free transaction is accepted to mempool"
33-        assert_equal(self.nodes[0].sendrawtransaction(tx2_hex), tx2_id)
34-        assert(tx2_id in self.nodes[0].getrawmempool())
35+        # assert_equal(self.nodes[0].sendrawtransaction(tx2_hex), tx2_id)
36+        # assert(tx2_id in self.nodes[0].getrawmempool())
37
38 if __name__ == '__main__':
39diff --git a/src/main.cpp b/src/main.cpp
40index 9870bee..4fda8fc 100644
41--- a/src/main.cpp
42+++ b/src/main.cpp
43@@ -970,4 +970,8 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
44                 strprintf("%d", nSigOps));
45
46+        assert(nFees!=0);
47+        assert(nFees >= ::minRelayTxFee.GetFee(nSize));
48+        assert(nModifiedFees >= ::minRelayTxFee.GetFee(nSize));
49+
50         CAmount mempoolRejectFee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nSize);
51         if (mempoolRejectFee > 0 && nModifiedFees < mempoolRejectFee) {

MarcoFalke force-pushed on Jan 13, 2016

laanwj commented at 12:46 pm on January 20, 2016: member

Needs rebase

[qa] Change default block priority size to 0 fa8e2a6925

MarcoFalke force-pushed on Jan 20, 2016

MarcoFalke commented at 12:16 pm on January 22, 2016: member

@morcos I think @laanwj is waiting for you to re-ACK this.

morcos commented at 12:19 pm on January 22, 2016: member

I can’t re-ACK right now as I’m out of town. I extensively reviewed and tested the first version. Without knowing what changed I don’t know how to re-ACK without redoing all that work.

laanwj merged this on Jan 22, 2016

laanwj closed this on Jan 22, 2016

laanwj referenced this in commit 93b05764d5 on Jan 22, 2016

MarcoFalke commented at 12:27 pm on January 22, 2016: member

The only actual content conflict is in the line where I remove -blockprioritysize=50000. If you tell me the commit you reviewed initially I can upload it for you.

Indirect content conflicts are two rpc test which rely on priority transaction getting accepted into the mempool:

abandonconflict.py
prioritise_transaction.py

But those can be addressed in a later pull. (I will only focus on priority transaction getting into blocks)

MarcoFalke deleted the branch on Jan 22, 2016

laanwj commented at 12:34 pm on January 22, 2016: member

utACK

As this is only a test change, and the test still passes after this, I think it’s OK for merging even without re-ACK from @morcos.

DrahtBot locked this on Sep 8, 2021

[qa] Change default block priority size to 0 #7177