New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 22, 2016

Conversation

Projects
None yet
6 participants
@MarcoFalke
Member

MarcoFalke commented Dec 6, 2015

Unit test should not run a "different branch" by default.

This is a cleanup PR of #7022

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

This comment has been minimized.

@dcousens

dcousens Dec 7, 2015

Contributor

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

@dcousens

dcousens Dec 7, 2015

Contributor

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Dec 7, 2015

Member
  • 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
@MarcoFalke

MarcoFalke Dec 7, 2015

Member
  • 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

This comment has been minimized.

@dcousens

dcousens Dec 9, 2015

Contributor

Thanks @MarcoFalke

@dcousens
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Dec 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.

Member

morcos commented Dec 7, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 7, 2015

Member

@morcos Yes, as pointed out in luke-jr@e4d7271#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." )

Member

MarcoFalke commented Dec 7, 2015

@morcos Yes, as pointed out in luke-jr@e4d7271#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." )

@jonasschnelli jonasschnelli added the Tests label Dec 9, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 9, 2015

Member

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

Member

jonasschnelli commented Dec 9, 2015

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 9, 2015

Member

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

Member

MarcoFalke commented Dec 9, 2015

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 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.

Member

MarcoFalke commented Dec 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Dec 10, 2015

Member

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

Member

morcos commented Dec 10, 2015

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

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Dec 11, 2015

Contributor

Concept ACK

Contributor

petertodd commented Dec 11, 2015

Concept ACK

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 11, 2015

Contributor

utACK

Contributor

dcousens commented Dec 11, 2015

utACK

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 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

diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py
index e7173fd..bb89d65 100755
--- a/qa/pull-tester/rpc-tests.py
+++ b/qa/pull-tester/rpc-tests.py
@@ -117,5 +117,5 @@ testScriptsExt = [
     'txn_doublespend.py',
     'txn_clone.py --mineblock',
-    'pruning.py',
+#    'pruning.py', # too long
     'forknotify.py',
     'invalidateblock.py',
diff --git a/qa/rpc-tests/prioritise_transaction.py b/qa/rpc-tests/prioritise_transaction.py
index 4a79d38..2ada1be 100755
--- a/qa/rpc-tests/prioritise_transaction.py
+++ b/qa/rpc-tests/prioritise_transaction.py
@@ -107,10 +107,12 @@ class PrioritiseTransactionTest(BitcoinTestFramework):

         try:
-            self.nodes[0].sendrawtransaction(tx2_hex)
+            # Don't send 0 fee transaction
+            # self.nodes[0].sendrawtransaction(tx2_hex)
+            pass
         except JSONRPCException as exp:
             assert_equal(exp.error['code'], -26) # insufficient fee
             assert(tx2_id not in self.nodes[0].getrawmempool())
         else:
-            assert(False)
+            assert(True)

         # This is a less than 1000-byte transaction, so just set the fee
@@ -120,6 +122,6 @@ class PrioritiseTransactionTest(BitcoinTestFramework):

         print "Assert that prioritised free transaction is accepted to mempool"
-        assert_equal(self.nodes[0].sendrawtransaction(tx2_hex), tx2_id)
-        assert(tx2_id in self.nodes[0].getrawmempool())
+        # assert_equal(self.nodes[0].sendrawtransaction(tx2_hex), tx2_id)
+        # assert(tx2_id in self.nodes[0].getrawmempool())

 if __name__ == '__main__':
diff --git a/src/main.cpp b/src/main.cpp
index 9870bee..4fda8fc 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -970,4 +970,8 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
                 strprintf("%d", nSigOps));

+        assert(nFees!=0);
+        assert(nFees >= ::minRelayTxFee.GetFee(nSize));
+        assert(nModifiedFees >= ::minRelayTxFee.GetFee(nSize));
+
         CAmount mempoolRejectFee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nSize);
         if (mempoolRejectFee > 0 && nModifiedFees < mempoolRejectFee) {
Member

MarcoFalke commented Jan 7, 2016

  • 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

diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py
index e7173fd..bb89d65 100755
--- a/qa/pull-tester/rpc-tests.py
+++ b/qa/pull-tester/rpc-tests.py
@@ -117,5 +117,5 @@ testScriptsExt = [
     'txn_doublespend.py',
     'txn_clone.py --mineblock',
-    'pruning.py',
+#    'pruning.py', # too long
     'forknotify.py',
     'invalidateblock.py',
diff --git a/qa/rpc-tests/prioritise_transaction.py b/qa/rpc-tests/prioritise_transaction.py
index 4a79d38..2ada1be 100755
--- a/qa/rpc-tests/prioritise_transaction.py
+++ b/qa/rpc-tests/prioritise_transaction.py
@@ -107,10 +107,12 @@ class PrioritiseTransactionTest(BitcoinTestFramework):

         try:
-            self.nodes[0].sendrawtransaction(tx2_hex)
+            # Don't send 0 fee transaction
+            # self.nodes[0].sendrawtransaction(tx2_hex)
+            pass
         except JSONRPCException as exp:
             assert_equal(exp.error['code'], -26) # insufficient fee
             assert(tx2_id not in self.nodes[0].getrawmempool())
         else:
-            assert(False)
+            assert(True)

         # This is a less than 1000-byte transaction, so just set the fee
@@ -120,6 +122,6 @@ class PrioritiseTransactionTest(BitcoinTestFramework):

         print "Assert that prioritised free transaction is accepted to mempool"
-        assert_equal(self.nodes[0].sendrawtransaction(tx2_hex), tx2_id)
-        assert(tx2_id in self.nodes[0].getrawmempool())
+        # assert_equal(self.nodes[0].sendrawtransaction(tx2_hex), tx2_id)
+        # assert(tx2_id in self.nodes[0].getrawmempool())

 if __name__ == '__main__':
diff --git a/src/main.cpp b/src/main.cpp
index 9870bee..4fda8fc 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -970,4 +970,8 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
                 strprintf("%d", nSigOps));

+        assert(nFees!=0);
+        assert(nFees >= ::minRelayTxFee.GetFee(nSize));
+        assert(nModifiedFees >= ::minRelayTxFee.GetFee(nSize));
+
         CAmount mempoolRejectFee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nSize);
         if (mempoolRejectFee > 0 && nModifiedFees < mempoolRejectFee) {
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 20, 2016

Member

Needs rebase

Member

laanwj commented Jan 20, 2016

Needs rebase

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 22, 2016

Member

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

Member

MarcoFalke commented Jan 22, 2016

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

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 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.

Member

morcos commented Jan 22, 2016

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 laanwj merged commit fa8e2a6 into bitcoin:master Jan 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 22, 2016

Merge #7177: [qa] Change default block priority size to 0
fa8e2a6 [qa] Change default block priority size to 0 (MarcoFalke)
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 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)

Member

MarcoFalke commented Jan 22, 2016

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 MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-rpcNoPrio branch Jan 22, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 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.

Member

laanwj commented Jan 22, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment