Commit Graph

10 Commits

Author SHA1 Message Date
MarcoFalke
41494eea70
Merge #13954: Warn (don't fail!) on spelling errors. Fix typos reported by codespell.
f8a81f73ac lint: Add spell check linter (codespell) (practicalswift)
ada356208e Fix typos reported by codespell (practicalswift)

Pull request description:

  * Check for common misspellings using `codespell`.
  * Fix recently introduced typos reported by `codespell`.

Tree-SHA512: 9974c0e640b411c7d0ebc5b45de253c19bac7fe3002cd98601ff8da8db584224c2fd7d331aee3df612c9f2cfef540d647a9b4c5a1a73fd208dc93ce4bf9e5e3e
2021-07-19 12:51:24 -05:00
MarcoFalke
9156e07334 Merge #18350: test: Fix mining to an invalid target + ensure that a new block has the correct hash internally
7a6627ae87b637bf32c03122865402bd71adf0d1 Fix mining to an invalid target + ensure that a new block has the correct hash internally in Python tests (Samer Afach)

Pull request description:

  Test with block 47 in the `feature_block.py` creates a block with a hash higher than the target, which is supposed to fail. Now two issues exist there, and both have low probability of showing up:

  1. The creation is done with `while (hash < target)`, which is wrong, because hash = target is a valid mined value based on the code in the function `CheckProofOfWork()` that validates the mining target:
  ```
      if (UintToArith256(hash) > bnTarget)
          return false;
  ```
  2. As we know the hash stored in CBlock class in Python is stateful, unlike how it's in C++, where calling `CBlock::GetHash()` will actively calculate the hash and not cache it anywhere. With this, blocks that come out of the method `next_block` can have incorrect hash value when `solve=False`. This is because the `next_block` is mostly used with `solve=True`, and solving does call the function `rehash()` which calculates the hash of the block, but with `solve=False`, nothing calls that method. And since the work to be done in regtests is very low, the probably of this problem showing up is very low, but it practically happens (well, with much higher probability compared to issue No. 1 above).

  This PR fixes both these issues.

Top commit has no ACKs.

Tree-SHA512: f3b54d18f5073d6f1c26eab89bfec78620dda4ac1e4dde4f1d69543f1b85a7989d64c907e091db63f3f062408f5ed1e111018b842819ba1a5f8348c7b01ade96
2021-07-13 21:19:47 -05:00
MarcoFalke
9f4f52ae4e
Merge #13054: tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports.
68400d8b96 tests: Use explicit imports (practicalswift)

Pull request description:

  Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports.

  Wildcard imports make it unclear which names are present in the namespace, confusing both readers and many automated tools.

  An additional benefit of not using wildcard imports in tests scripts is that readers of a test script then can infer the rough testing scope just by looking at the imports.

  Before this commit:

  ```
  $ contrib/devtools/lint-python.sh | head -10
  ./test/functional/feature_rbf.py:8:1: F403 'from test_framework.util import *' used; unable to detect undefined names
  ./test/functional/feature_rbf.py:9:1: F403 'from test_framework.script import *' used; unable to detect undefined names
  ./test/functional/feature_rbf.py:10:1: F403 'from test_framework.mininode import *' used; unable to detect undefined names
  ./test/functional/feature_rbf.py:15:12: F405 bytes_to_hex_str may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  ./test/functional/feature_rbf.py:17:58: F405 CScript may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  ./test/functional/feature_rbf.py:25:13: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  ./test/functional/feature_rbf.py:26:31: F405 satoshi_round may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  ./test/functional/feature_rbf.py:26:60: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  ./test/functional/feature_rbf.py:30:41: F405 satoshi_round may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  ./test/functional/feature_rbf.py:30:68: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
  $
  ```

  After this commit:

  ```
  $ contrib/devtools/lint-python.sh | head -10
  $
  ```

Tree-SHA512: 3f826d39cffb6438388e5efcb20a9622ff8238247e882d68f7b38609877421b2a8e10e9229575f8eb6a8fa42dec4256986692e92922c86171f750a0e887438d9
2021-07-06 20:29:33 +03:00
MarcoFalke
20e1f4a8ef
Merge #13669: Tests: Cleanup create_transaction implementations
44bbceeef1 [Tests] Cleanup feature_block.py, remove unnecessary PreviousSpendableOutput object (Conor Scott)
736f941424 [Tests] Cleanup extra instances of create_transaction (Conor Scott)
157651855f [Tests] Rename create_tx and move to blocktools.py (Conor Scott)

Pull request description:

  There currently exist seven ([1](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_cltv.py#L52-L60), [2](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_csv_activation.py#L88-L95) [3](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_dersig.py#L40-L48), [4](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_nulldummy.py#L100-L108), [5](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L529-L535), [6](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/blocktools.py#L120-L129), [7](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_block.py#L1218-L1220)) implementations of a function called something similar to `create_transaction` in the functional tests, some of which are exact copies of each other.

  This PR aims to clean this up into [three different cases implemented in blocktools.py](https://github.com/conscott/bitcoin/blob/create_tx_cleanup/test/functional/test_framework/blocktools.py#L121-L149)
  1. `create_tx_with_script`: Return transaction object spending generic tx output optionally specifying scriptSig and scriptPubKey
  2. `create_transaction`: Return transaction object spending coinbase tx
  2. `create_raw_transaction`: Return raw transaction (hex string) spending coinbase tx

  I am not committed to any of these function names, so I'll gladly take suggestions on there.

  Additionally there are some related cleanups to feature_block.py tests, specifically removing the [PreviousSpendableOutput](https://github.com/conscott/bitcoin/blob/master/test/functional/feature_block.py#L51-L54) object, which seems like an unnecessary layer given that every instance spends the 0 output.

Tree-SHA512: 63c6233b6f0942c81ba1ca67ea6770809b8c9409314c6d4cf8e5a3991cb9ee92b22bebe88c0dde45cd71e754eb351230c4c404b70ff118f5f43c034452ada65c
2021-07-06 20:29:31 +03:00
PastaPastaPasta
df65bd0238
Merge pull request #4224 from Munkybooty/backports-0.18-pr5
Backports 0.18 pr5
2021-07-03 12:33:17 -05:00
Wladimir J. van der Laan
025a33bb23 Merge #14101: qa: Use named args in validation acceptance tests
fa782a308dbe7bc579c122f63c1c65666fc85e91 qa: Use named args in some tests (MarcoFalke)
b4d33096734d787b0e1d754064039cbb64ce8d61 scripted-diff: Use named arguments in feature_block (MarcoFalke)
749ba35e7c9fbc21dbea27fd1be102b91313d132 scripted-diff: Pass node into p2p_segwit acceptance tests (MarcoFalke)

Pull request description:

  It is confusing to use a list of arguments such as `False, False, 16, ...` where it is unclear what each of them means.

  Run some scripted diffs to put meaning to them.

Tree-SHA512: d768df2375ea3c77145ebb1bf4c2d690581a379031449ded7ae160022d975eb13890aa8c6a44a5eebda8791cb2910a599326e431af76ed9e60afe1d182ada65c

# Conflicts:
#	test/functional/feature_block.py
#	test/functional/mining_basic.py
#	test/functional/p2p_segwit.py
2021-07-02 11:39:23 -04:00
Wladimir J. van der Laan
d33dd31032
Merge #13837: qa: Extract rpc_timewait as test param
fa5b440971a0dfdd64c1b86748a573fcd7dc65d3 qa: Extract rpc_timewait as test param (MarcoFalke)

Pull request description:

  Also increase it for wallet_dump and wallet_groups

Tree-SHA512: 7367bc584228bda3010c453713a1505c54a8ef3d116be47dab9934d30594089dfeb27ffa862f7517fd0ec8b5dc07f4904d67ef2a53dd284cbe2a58982e410e2b
2021-07-02 12:59:29 +03:00
Wladimir J. van der Laan
c02337e1b0
Merge #13517: qa: Remove need to handle the network thread in tests
fa87da2f172ae2e6dc15e9ed156a3564a8ecfbdd qa: Avoid start/stop of the network thread mid-test (MarcoFalke)

Pull request description:

  This simplifies test writing by removing the need to handle the network thread in tests. E.g. start thread, join thread, restart thread mid-test, adding p2p connections at the "right" time, ...

Tree-SHA512: 533642f12fef5496f1933855edcdab1a7ed901d088d34911749cd0f9e044c8a6cb1f89985ac3a7f41a512943663e4e270a61978f6f072143ae050cd102d4eab8
2021-07-02 12:59:26 +03:00
Wladimir J. van der Laan
906be7e144
Merge #11423: [Policy] Several transaction standardness rules
364bae5 qa: Pad scriptPubKeys to get minimum sized txs (MarcoFalke)
7485488 Policy to reject extremely small transactions (Johnson Lau)
0f8719b Add transaction tests for constant scriptCode (Johnson Lau)
9dabfe4 Add constant scriptCode policy in non-segwit scripts (Johnson Lau)

Pull request description:

  This disables `OP_CODESEPARATOR` in non-segwit scripts (even in an unexecuted branch), and makes a positive `FindAndDelete` result invalid. This ensures that the `scriptCode` serialized in `SignatureHash` is always the same as the script passing to the `EvalScript`.

Tree-SHA512: a0552cb920294d130251c48053fa2ff1fbdd26332e62b52147d918837852750f0ce35ce2cd1cbdb86588943312f8154ccb4925e850dbb7c2254bc353070cd5f8
2021-06-05 22:13:01 +03:00
PastaPastaPasta
b07a7b810c
Backport 11796 + 11774 (#3612)
* Merge #11796: [tests] Functional test naming convention

5fecd84 [tests] Remove redundant import in blocktools.py test (Anthony Towns)
9b20bb4 [tests] Check tests conform to naming convention (Anthony Towns)
7250b4e [tests] README.md nit fixes (Anthony Towns)
82b2712 [tests] move witness util functions to blocktools.py (John Newbery)
1e10854 [tests] [docs] update README for new test naming scheme (John Newbery)

Pull request description:

  Splitting #11774 into two parts -- this part updates the README with the proposed naming convention, and adds some checks to test_runner.py that the number of tests violating the naming convention doesn't increase too much. Idea is this part of the change should not introduce merge conflicts or require much rebasing, so reviews of the complicated bits won't become invalidated too often; while the second part will just be file renames, which will require regular rebasing and will introduce merge conflicts with pending PRs, but can be merged later, and should also be much easier to review, since it will only include relatively trivial changes.

Tree-SHA512: b96557d41714addbbfe2aed62fb5a48639eaeb1eb3aba30ac1b3a86bb3cb8d796c6247f9c414c4695c4bf54c0ec9968ac88e2f88fb62483bc1a2f89368f7fc80

* update violation count

Signed-off-by: pasta <pasta@dashboost.org>

* Merge #11774: [tests] Rename functional tests

6f881cc880 [tests] Remove EXPECTED_VIOLATION_COUNT (Anthony Towns)
3150b3fea7 [tests] Rename misc functional tests. (Anthony Towns)
81b79f2c39 [tests] Rename rpc_* functional tests. (Anthony Towns)
61b8f7f273 [tests] Rename p2p_* functional tests. (Anthony Towns)
90600bc7db [tests] Rename wallet_* functional tests. (Anthony Towns)
ca6523d0c8 [tests] Rename feature_* functional tests. (Anthony Towns)

Pull request description:

  This PR changes the functional tests to have a consistent naming scheme:

      tests for individual RPC methods are named rpc_...
      tests for interfaces (REST, ZMQ, RPC features) are named interface_...
      tests that explicitly test the p2p interface are named p2p_...
      tests for wallet features are named wallet_...
      tests for mining features are named mining_...
      tests for mempool behaviour are named mempool_...
      tests for full features that aren't wallet/mining/mempool are named feature_...

  Rationale: it's sometimes difficult for new contributors to know what's already covered by existing tests and where new tests should be added. Naming in a consistent fashion makes it easier to see what's already covered at a glance.

Tree-SHA512: 4246790552d42bbd95f6d5bdf67702b81b3b2c583ce7eaf1fe6d8e254721279b47315973c6e9ae82dad6e4c747f12188160764bf2624c0f8f3b4d39330ec8b16

* rename tests and edit associated strings to align test-suite with test name standards

Signed-off-by: pasta <pasta@dashboost.org>

* fix grammar in test/functional/test_runner.py

Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

* ci: Fix excluded test names

* rename feature_privatesend.py to rpc_privatesend.py

Signed-off-by: pasta <pasta@dashboost.org>

Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
Co-authored-by: xdustinface <xdustinfacex@gmail.com>
2020-07-17 02:44:20 +03:00