Skip to content
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: Read reject reasons from debug log, not p2p messages #14119

Merged
merged 1 commit into from Sep 8, 2018

Conversation

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Aug 31, 2018

For local testing we don't need to rely on p2p messages just to assert a reject reason.

Replace reading p2p messages with reading from the debug log file.

@MarcoFalke MarcoFalke force-pushed the Mf1808-qaRejectLog branch 4 times, most recently from 45b499b to fa710b6 Aug 31, 2018
@MarcoFalke MarcoFalke force-pushed the Mf1808-qaRejectLog branch from fa710b6 to fac3e22 Aug 31, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Aug 31, 2018

No more conflicts as of last run.

@laanwj
Copy link
Member

@laanwj laanwj commented Sep 1, 2018

Concept ACK

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Sep 7, 2018

tACK fac3e22

@laanwj laanwj merged commit fac3e22 into bitcoin:master Sep 8, 2018
2 checks passed
laanwj added a commit that referenced this issue Sep 8, 2018
fac3e22 qa: Read reject reasons from debug log, not p2p messages (MarcoFalke)

Pull request description:

  For local testing we don't need to rely on p2p messages just to assert a reject reason.

  Replace reading p2p messages with reading from the debug log file.

Tree-SHA512: fa59598ecf5e00cfb420ef1892d90aa415501fd882e1c608894dc577b0d00e93a442326d3a9167fef77d26aafbe345b730b49109982ccad68a5942384564a90b
@@ -824,7 +824,7 @@ def run_test(self):
tx.vin.append(CTxIn(COutPoint(b64a.vtx[1].sha256, 0)))
b64a = self.update_block("64a", [tx])
assert_equal(len(b64a.serialize()), MAX_BLOCK_BASE_SIZE + 8)
self.sync_blocks([b64a], success=False, reject_code=1, reject_reason=b'error parsing message')
self.sync_blocks([b64a], success=False, reject_reason='non-canonical ReadCompactSize(): iostream error')
Copy link
Contributor

@ken2812221 ken2812221 Sep 8, 2018

This is non-canonical ReadCompactSize(): iostream stream error on Windows. See https://ci.appveyor.com/project/DrahtBot/bitcoin/build/master.972. Is there a way to specify the second variant? Or just discard the message to be non-canonical ReadCompactSize()

Fixed in #14007

Copy link
Contributor

@AkioNak AkioNak Sep 13, 2018

FYI
On macOS, this is 'non-canonical ReadCompactSize(): unspecified iostream_category error'.
I think #14007 will also fix this because of discarding the message to be 'non-canonical ReadCompactSize().

@MarcoFalke MarcoFalke deleted the Mf1808-qaRejectLog branch Sep 8, 2018
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 16, 2020
Summary:
This was leftover due to one or more out-of-order backport(s).

Partial backport of Core [[bitcoin/bitcoin#14119 | PR14119]]
https://github.com/bitcoin/bitcoin/pull/14119/files#diff-1fe2787f6e875dad2060f80b1ee79320L137-L148

Test Plan:
  ninja check-functional

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6599
random-zebra added a commit to PIVX-Project/PIVX that referenced this issue May 4, 2021
…tx functional tests coverage.

2d994c4 qa: adding test case for output value > input value out of range (furszy)
6165c80 qa: update and enable p2p_invalid_tx.py functional test. (furszy)
bb62699 Backport chainparams and validation `fRequireStandard` flags functionality + init 'acceptnonstdtxn' option to skip (most) "non-standard transaction" checks, for testnet/regtest only. (furszy)
0280c39 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
ec7d0b0 [qa] Test for duplicate inputs within a transaction (Suhas Daftuar)
a501fe1 qa: update and enable p2p_invalid_block.py. (furszy)
f83d140 qa: Read reject reasons from debug log, not p2p messages (furszy)
8a50165 [tests] Add P2PDataStore class (furszy)
ebed910 qa: Correct coinbase input script height. (furszy)
cae2d43 [tests] Reduce NodeConn connection logging from info to debug (John Newbery)

Pull request description:

  Introduced the `P2PDataStore` class into the functional test suite and updated it up to a point where was able to enable the `p2p_invalid_block.py` and `p2p_invalid_tx.py` functional tests. Plus updated both tests to a much recent version, expanding its testing coverage as well.
  Now the test suite is covering the primary CVE that btc had in the past.

  As both tests were simply unusable before and we have a different synchronization process than upstream, plus we didn't have some params and init/global flags, this work isn't following a PR by PR back port path. Had to made my own way up to the complete tests support.

  Main pilar PRs from upstream from where this work was based:
  * bitcoin#6329.
  * bitcoin#11771.
  * bitcoin#14119 (only `p2p_invalid_tx.py`, `p2p_invalid_block.py` and `mininode.py` changes).
  * bitcoin#14247 (only 9b4a36e).
  * bitcoin#14696.

ACKs for top commit:
  random-zebra:
    ACK 2d994c4

Tree-SHA512: 548830b5fbf8b7f383832a7eea96179d089a4c9c222ef34007846f53736b07c873e37084235f1575eac157b63308c00ab3be78b71d3ed6520f4f73f1c8de81a5
@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
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants