Skip to content

Conversation

@jnewbery
Copy link
Contributor

Builds on #11771. Please review that PR first

Next step in #10603.

  • first commit tidies up invalidblockrequest.py
  • second commit removes usage of ComparisonTestFramework

@jnewbery
Copy link
Contributor Author

jnewbery commented Dec 2, 2017

This requires rebase, but I'll leave it as it is until #11771 is reviewed/merged.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed last two commits only. Other commits are part of #11771 (and out of date).

  • utACK 62bb3a0892558cf8813a22e1bf7860598dde3441 [tests] Change invalidblockrequest to use BitcoinTestFramework
  • utACK 1d26a167c73b3e94509b3d39fa5875fdb9601eb1 [tests] Fix flake8 warnings in invalidblockrequest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Change invalidblockrequest to use BitcoinTestFramework"

Maybe avoid 0x prefix here and above by calling int(hash, 16)

@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 13, 2018

This requires rebase now that #11771 has been merged.

Review comments from @conscott to address in this PR:

conscott and others added 3 commits February 13, 2018 19:03
Remove unused variable reassignments in p2p_invalid_tx.py and call
send_txs_and_test() with valid transaction.
[tests] update tests from changes to mininode in bitcoin#11771 - added by @conscott

[tests] trivial update to hex conversion for readability - added by @conscott
@jnewbery jnewbery force-pushed the refactor_invalidblockrequest branch from 62bb3a0 to e97b113 Compare February 14, 2018 00:04
@jnewbery
Copy link
Contributor Author

Thanks to @conscott for rebasing this and fixing up the remaining issues from #11771 and Russ's review comment.

@jamesob
Copy link
Contributor

jamesob commented Feb 14, 2018

👍

utACK e97b113

@maflcko
Copy link
Member

maflcko commented Mar 13, 2018

utACK e97b113

@maflcko maflcko merged commit e97b113 into bitcoin:master Mar 13, 2018
maflcko pushed a commit that referenced this pull request Mar 13, 2018
…amework

e97b113 [tests] Change invalidblockrequest to use BitcoinTestFramework (John Newbery)
2b7064e [tests] Fix flake8 warnings in invalidblockrequest (John Newbery)
54b8c58 [test] Fix nits leftover from 11771 (Conor Scott)

Pull request description:

  Builds on #11771. Please review that PR first

  Next step in #10603.

  - first commit tidies up invalidblockrequest.py
  - second commit removes usage of ComparisonTestFramework

Tree-SHA512: 14b10c09c8c0ebef4a9176eb5b883a275d04c096785ee31b84ef594eed346ec6344d7ed32184c5fb397e744725df3911f45cdfadd0810e5a52eaa256084e3456
@jnewbery
Copy link
Contributor Author

Not an issue with your code, but eq() is not implemented for CTransaction, so the argument to assert is always true, since none of the tx objects are the same reference (they were copied).

Agree - this assert:

assert(block2_orig.vtx != block2.vtx)
should be removed in a future PR.

@jnewbery jnewbery deleted the refactor_invalidblockrequest branch March 18, 2018 23:05
maflcko pushed a commit that referenced this pull request Apr 5, 2018
…TestFramework]

9c92c8c [tests] Remove Comparison Test Framework (John Newbery)
e80c640 [tests] Remove bip9-softforks.py (John Newbery)

Pull request description:

  Builds on #11772, #11773 and #11817. Please review those PRs first.

  Final step in #10603.

  - First commit removes bip9-softforks.py.  bip9-sofforks.py was intended to be a generic test for versionbits deployments. However, it only tests CSV activation and was not updated to test segwit activation. CSV activation is tested by bip68-112-113-p2p.py, so this test is duplicated effort. Rather than try to update it to use the BitcoinTestFramework, just remove it. (see btcdrak#8 for previous discussion around the redundancy of bip9-softforks.py)
  - Second commit removes the now unused BitcoinComparisonFramework class and the comptool and blockstore modules.

Tree-SHA512: 4bb7196d521048b3b8ba95c87dde73005a1ac73d29ccbb869f11ce9a71089686e7eacd7335337853041dfbd3a5b110172b105adbada58779814d4db22b1376f5
codablock pushed a commit to codablock/dash that referenced this pull request Oct 3, 2019
…nTestFramework

e97b113 [tests] Change invalidblockrequest to use BitcoinTestFramework (John Newbery)
2b7064e [tests] Fix flake8 warnings in invalidblockrequest (John Newbery)
54b8c58 [test] Fix nits leftover from 11771 (Conor Scott)

Pull request description:

  Builds on bitcoin#11771. Please review that PR first

  Next step in bitcoin#10603.

  - first commit tidies up invalidblockrequest.py
  - second commit removes usage of ComparisonTestFramework

Tree-SHA512: 14b10c09c8c0ebef4a9176eb5b883a275d04c096785ee31b84ef594eed346ec6344d7ed32184c5fb397e744725df3911f45cdfadd0810e5a52eaa256084e3456
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants