Skip to content

test: Remove fragile assert_memory_usage_stable #17469

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 15, 2019
Merged

test: Remove fragile assert_memory_usage_stable #17469

merged 1 commit into from
Nov 15, 2019

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 13, 2019

This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it.

Example failures:

@maflcko maflcko added the Tests label Nov 13, 2019
@practicalswift
Copy link
Contributor

ACK fadeb41 -- diff looks good and Travis is happy

Thanks for working on this

@laanwj
Copy link
Member

laanwj commented Nov 14, 2019 •
edited
Loading

ACK, maybe remove the definition of assert_memory_usage_stable in test_node.py as well (and get_mem_rss_kilobytes) as this is the only use.

@jamesob
Copy link
Contributor

jamesob commented Nov 14, 2019

Concept ACK. RIP.

@maflcko
Copy link
Member Author

maflcko commented Nov 14, 2019

I wasn't sure if @jamesob was using it for some projects, but I interpret the reply as "no", so going to remove the assert_memory_usage_stable as well.

@jamesob
Copy link
Contributor

jamesob commented Nov 14, 2019

FWIW I think this is a totally appropriate assertion for a functional test, since it's the highest-level suite of tests we have, and ensuring sane memory use is a feature of the system, but if we can't do it reliably across platform then we should either not do it on unreliable platforms or remove it entirely. My preference would be for the former, but if other people don't want to maintain this then I won't complain.

@maflcko maflcko force-pushed the 1911-testFragileMem branch from fadeb41 to fac942c Compare November 14, 2019 15:57
@laanwj
Copy link
Member

laanwj commented Nov 14, 2019 •
edited
Loading

In theory I agree, in practice a modern OS is so complex with so much going on in the background, that deterministically testing memory use seems to be impossible. It's the same as for timing tests. Sure, this can be done in a controlled environment, but not Travis and definitely not with parallelism of many.
(Edit: it'd probably work better with some LD_PRELOAD library that tracked malloc/free instead of parsing ps RSS output. But anyhow, if anyone wants to do that kinds of test in the future they can.)

@maflcko
Copy link
Member Author

maflcko commented Nov 14, 2019

I think the check itself is useful, but the limitations of the functional test framework make it almost useless in practice. The current status of this tests is: Running for twelve months, not a single reproducible failure.

I think this can be implemented as a fuzz test, making it more powerful, useful and easier to debug in case of failure.

jamesob
jamesob approved these changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@jamesob jamesob left a comment

Choose a reason for hiding this comment

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

ACK fac942c

maflcko pushed a commit that referenced this pull request Nov 15, 2019
Merge #17469: test: Remove fragile assert_memory_usage_stable
c7709cb 
fac942c test: Remove fragile assert_memory_usage_stable (MarcoFalke)

Pull request description:

  This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it.

  Example failures:

  * https://travis-ci.org/bitcoin/bitcoin/jobs/611497963#L14517
  * https://travis-ci.org/MarcoFalke/bitcoin-core/jobs/611029104#L3876

ACKs for top commit:
  jamesob:
    ACK fac942c

Tree-SHA512: 3577e7ce5891d221cb798454589ba796ed0c06621a26351bb919c23bc6bb46aafcd0b11cb02bbfde64b74d67cb2950da44959a7ecdc436491a34e8b045c1ccf4
@maflcko maflcko merged commit fac942c into bitcoin:master Nov 15, 2019
@maflcko maflcko deleted the 1911-testFragileMem branch November 15, 2019 17:48
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 21, 2020
@jasonbcox
Merge #17469: test: Remove fragile assert_memory_usage_stable
a382f87 
Summary:
fac942ca57dce6cfa5655a3ac8664d6a051bc01f test: Remove fragile assert_memory_usage_stable (MarcoFalke)

Pull request description:

  This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it.

  Example failures:

  * https://travis-ci.org/bitcoin/bitcoin/jobs/611497963#L14517
  * https://travis-ci.org/MarcoFalke/bitcoin-core/jobs/611029104#L3876

ACKs for top commit:
  jamesob:
    ACK bitcoin/bitcoin@fac942c

Tree-SHA512: 3577e7ce5891d221cb798454589ba796ed0c06621a26351bb919c23bc6bb46aafcd0b11cb02bbfde64b74d67cb2950da44959a7ecdc436491a34e8b045c1ccf4

Backport of Core [[bitcoin/bitcoin#17469 | PR17469]]

I've skipped backporting Core [[bitcoin/bitcoin#14794 | PR14794]] which attempts to fix the issue using a not-so-robust approach of
looking at env variables to determine if the memory usage threshold should be modified.

This fixes sanitizer failures on CI by removing the problem entirely.

Test Plan:
`ninja check-functional`
and run ASAN on CI

Reviewers: #bitcoin_abc, nakihito

Reviewed By: nakihito

Subscribers: nakihito

Differential Revision: https://reviews.bitcoinabc.org/D6202
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
@ftrader
Merge #17469: test: Remove fragile assert_memory_usage_stable
ab6e90f 
Summary:
fac942ca57dce6cfa5655a3ac8664d6a051bc01f test: Remove fragile assert_memory_usage_stable (MarcoFalke)

Pull request description:

  This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it.

  Example failures:

  * https://travis-ci.org/bitcoin/bitcoin/jobs/611497963#L14517
  * https://travis-ci.org/MarcoFalke/bitcoin-core/jobs/611029104#L3876

ACKs for top commit:
  jamesob:
    ACK bitcoin/bitcoin@fac942c

Tree-SHA512: 3577e7ce5891d221cb798454589ba796ed0c06621a26351bb919c23bc6bb46aafcd0b11cb02bbfde64b74d67cb2950da44959a7ecdc436491a34e8b045c1ccf4

Backport of Core [[bitcoin/bitcoin#17469 | PR17469]]

I've skipped backporting Core [[bitcoin/bitcoin#14794 | PR14794]] which attempts to fix the issue using a not-so-robust approach of
looking at env variables to determine if the memory usage threshold should be modified.

This fixes sanitizer failures on CI by removing the problem entirely.

Test Plan:
`ninja check-functional`
and run ASAN on CI

Reviewers: #bitcoin_abc, nakihito

Reviewed By: nakihito

Subscribers: nakihito

Differential Revision: https://reviews.bitcoinabc.org/D6202
@furszy furszy mentioned this pull request Jun 4, 2021
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jun 28, 2021
@random-zebra
Merge #2410: [Test] Add p2p invalid messages functional test (and tes…
bffe509 
…t framework update)

7b04c0a [Test] Clean duplicate connections creation in mining_pos_coldStaking.py (furszy)
15a799e [Test] MAX_PROTOCOL_MESSAGE_LENGTH PIVXified. (furszy)
0aedf35 [tests] Don't import asyncio to test magic bytes (John Newbery)
9bb5309 Refactor resource exhaustion test (Troy Giorshev)
589a780 Fix "invalid message size" test (Troy Giorshev)
8a0c12b Move size limits to module-global (Troy Giorshev)
b3391c5 Remove two unneeded tests (Troy Giorshev)
1404e68 test: Add various low-level p2p tests (furszy)
8aaf7e1 test: Remove fragile assert_memory_usage_stable (MarcoFalke)
f68e22c [Test] p2p_invalid_messages.py do not change msg.command in test_command and raise sync_with_ping timeout (furszy)
c851c0b Test framework: Wait for verack by default on every new connection (furszy)
c02e9a0 [Test] move framework subversion to string (furszy)
3472a39 Replace coroutine with async def in p2p_invalid_messages.py (Elichai Turkel)
33c7b19 net_processing: align msg checksum check properly. (furszy)
7f71b1b Hash P2P messages as they are received instead of at process-time (furszy)
215a638 test: Skip flaky p2p_invalid_messages test on macOS (Fabian Jahr)
c11f565 qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke)
be979ad test: Fix race in p2p_invalid_messages (MarcoFalke)
6a72f0c qa: Add tests for invalid message headers (MarcoFalke)
51ddd3d Introduce p2p_invalid_messages.py test (furszy)
55a37b5 net: fix missing jump line in "Oversized message from peer.." error. (furszy)
0edfce5 test_node: get_mem_rss fixups (MarcoFalke)
6f21213 tests: add P2PConnection.send_raw_message (James O'Beirne)
ae68c6e tests: add utility to assert node memory usage hasn't increased (James O'Beirne)
8469afe test: forward timeouts properly in send_blocks_and_test (James O'Beirne)
db28a53 Skip is_closing() check when not available. (Daniel Kraft)
be9dacb tests: fixes mininode's P2PConnection sending messages on closing transport (marcoagner)
53599c8 qa: Avoid start/stop of the network thread mid-test (furszy)
688190c [qa] mininode: Expose connection state through is_connected (MarcoFalke)

Pull request description:

  Part of the deep and long net and ser work that I'm doing (and Tor v3 onion addresses support). Friend of #2359.

  Focused on the end goal of implementing the `p2p_invalid_messages` functional test which validates that invalid msg headers, msg types, oversized payloads and inventory msgs aren't accepted nor can cause a resource exhaustion. And an extra covered scenario, in `p2p_invalid_tx.py`, for the orphan pool overflow.

  Plus, to get up to the goal, had to work over the functional test framework as well.

  So.. adapted list:

  * bitcoin#9045.
  * bitcoin#13512.
  * bitcoin#13517.
  * bitcoin#13715.
  * bitcoin#13747.
  * bitcoin#14456.
  * bitcoin#14522.
  * bitcoin#14672.
  * bitcoin#14693.
  * bitcoin#14812.
  * bitcoin#15246.
  * bitcoin#15330.
  * bitcoin#15697.
  * bitcoin#16445.
  * bitcoin#17931.
  * bitcoin#17469.
  * bitcoin#18628 (only `p2p_invalid_tx.py` and `p2p_invalid_messages.py`. We don't support the other tests yet).
  * bitcoin#19177.
  * bitcoin#19264.

ACKs for top commit:
  random-zebra:
    utACK 7b04c0a and merging...

Tree-SHA512: 48d1f1a6acc24a9abce033fbf1f281ba4392147da39d118a694a09d63c3e0610cc1a29d711b434b16cc0d0e030dd9f1a89843870091b6999b862b9ab18a20679
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@jamesob jamesob jamesob approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @practicalswift @laanwj @jamesob