QA: Use GBT to get block versions correct #19401

luke-jr commented at 6:33 PM on June 28, 2020: member

The goal here is to decouple unrelated tests from the details of block versions.

Currently, these tests are forcing specific versions of blocks for no real reason.

DrahtBot added the label Tests on Jun 28, 2020

DrahtBot commented at 10:13 PM on June 28, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr referenced this in commit 7562c296d2 on Jul 2, 2020

luke-jr referenced this in commit 0608d57cd2 on Jul 2, 2020

in test/functional/test_framework/blocktools.py:61 in 01b8bd792b outdated

  54 | @@ -51,19 +55,29 @@
  55 |  # From BIP141
  56 |  WITNESS_COMMITMENT_HEADER = b"\xaa\x21\xa9\xed"
  57 |  
  58 | +NORMAL_GBT_REQUEST_PARAMS = {"rules": ["segwit"]}
  59 |  
  60 | -def create_block(hashprev, coinbase, ntime=None, *, version=1):
  61 | +
  62 | +def create_block(hashprev=None, coinbase=None, ntime=None, *, version=None, tmpl={}, txlist=None):

adamjonas commented at 7:06 PM on July 3, 2020:

Passing in a dictionary as a default parameter value is triggering the linter.

luke-jr commented at 7:56 PM on July 3, 2020:

So how do we fix the linter?

MarcoFalke commented at 9:20 PM on July 3, 2020:

Something like this?

def create_block(hashprev=None, coinbase=None, ntime=None, *, version=None, tmpl=None, txlist=None):
  tmpl = {} if tmpl is None else tmpl

MarcoFalke commented at 9:21 PM on July 3, 2020:

or use the suggestion from the linter

luke-jr commented at 9:23 PM on July 3, 2020:

There isn't a problem with this PR. The problem is the linter being wrong.

luke-jr commented at 9:33 PM on July 3, 2020:

Changing this to None here since it conceptually makes more sense anyway, but in other cases the linter should probably be fixed instead.

luke-jr force-pushed on Jul 3, 2020

in test/functional/test_framework/blocktools.py:64 in 6b222dcb6e outdated

  64 |      block = CBlock()
  65 | -    block.nVersion = version
  66 | -    if ntime is None:
  67 | -        import time
  68 | -        block.nTime = int(time.time() + 600)
  69 | +    if tmpl is None: tmpl = {}

MarcoFalke commented at 11:59 AM on July 5, 2020:

You'll have to make this multi-line or exclude E701 in the lint script

luke-jr requested review from MarcoFalke on Jul 5, 2020

instagibbs commented at 1:30 PM on July 24, 2020: member

no opinion on linter thing but

utACK https://github.com/bitcoin/bitcoin/pull/19401/commits/dfe8110e411b5321b38e6658d4661d88e7ea421a

in test/functional/feature_nulldummy.py:41 in 6b222dcb6e outdated

  36 | @@ -37,14 +37,15 @@ def trueDummy(tx):
  37 |  class NULLDUMMYTest(BitcoinTestFramework):
  38 |  
  39 |      def set_test_params(self):
  40 | -        self.num_nodes = 1
  41 | +        # Need two nodes only so GBT doesn't complain that it's not connected
  42 | +        self.num_nodes = 2

ajtowns commented at 11:45 PM on September 3, 2020:

Requiring connected nodes is inconvenient for generating the initial blocks for custom signets as well. How about:

--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -652,11 +652,13 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
     if(!node.connman)
         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
 
+    if (!Params().IsTestChain()) {
         if (node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL) == 0)
             throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, PACKAGE_NAME " is not connected!");
 
         if (::ChainstateActive().IsInitialBlockDownload())
             throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is in initial sync and waiting for blocks...");
+    }
 
     static unsigned int nTransactionsUpdatedLast;
     const CTxMemPool& mempool = EnsureMemPool(request.context);

instead?

luke-jr commented at 6:18 PM on September 12, 2020:

Seems like an unnecessary special-casing for tests. Best to just make the tests work right.

in test/functional/test_framework/blocktools.py:78 in d01b86ddfa outdated

  80 | +    if coinbase is None:
  81 | +        coinbase = create_coinbase(height=tmpl['height'])
  82 |      block.vtx.append(coinbase)
  83 | +    if txlist:
  84 | +        for tx in txlist:
  85 | +            if not hasattr(tx, 'calc_sha256'):

fjahr commented at 2:28 PM on September 9, 2020:

nit: seems like the better choice

            if not isinstance(tx, CTransaction):

luke-jr commented at 6:23 PM on September 12, 2020:

We don't really care what the class is, so long as it has calc_sha256

in test/functional/feature_bip68_sequence.py:276 in 6b222dcb6e outdated

 271 | @@ -272,6 +272,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
 272 |  
 273 |          # Advance the time on the node so that we can test timelocks
 274 |          self.nodes[0].setmocktime(cur_time+600)
 275 | +        tmpl = self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)

fjahr commented at 2:43 PM on September 9, 2020:

nit: for clarification

        # Save block template now to use for the reorg later
        tmpl = self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)

fjahr commented at 2:53 PM on September 9, 2020: member

tACK dfe8110e411b5321b38e6658d4661d88e7ea421a

Personally I wouldn't remove the linter rule but no strong feelings.

QA: blocktools: Accept block template to create_block 1df2cd1c8f

QA: Use GBT to get block versions correct d438d609cd

luke-jr force-pushed on Sep 12, 2020

luke-jr commented at 6:25 PM on September 12, 2020: member

Restored linter rule against common sense due to widespread popularity. Added suggested comment.

fjahr commented at 11:50 PM on September 12, 2020: member

re-ACK d438d609cd64fe532d94e45000495de93ef99aa6

benthecarman commented at 12:21 PM on October 15, 2020: contributor

ACK d438d60

MarcoFalke merged this on Oct 16, 2020

MarcoFalke closed this on Oct 16, 2020

sidhujag referenced this in commit 07ae1c9944 on Oct 16, 2020

Fabcien referenced this in commit fe29b3461f on Dec 20, 2021

Fabcien referenced this in commit 3bac012a85 on Dec 20, 2021

DrahtBot locked this on Feb 15, 2022