Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Supersedes #4330 due to lack of rebase

laanwj and others added 10 commits August 22, 2021 21:34
d20a9fa tests: add tests for invalid P2P messages (James O'Beirne)
62f94d3 tests: add P2PConnection.send_raw_message (James O'Beirne)
5aa31f6 tests: add utility to assert node memory usage hasn't increased (James O'Beirne)

Pull request description:

  - Adds `p2p_invalid_messages.py`: tests based on behavior for dealing with invalid and malformed P2P messages. Includes a test verifying that we can't DoS a node by spamming it with large invalid messages.
  - Adds `TestNode.assert_memory_usage_stable`: a context manager that allows us to ensure memory usage doesn't significantly increase on a node during some test.
  - Adds `P2PConnection.send_raw_message`: which allows us to construct and send messages with tweaked headers.

Tree-SHA512: 720a4894c1e6d8f1551b2ae710e5b06c9e4f281524623957cb01599be9afea82671dc26d6152281de0acb87720f0c53b61e2b27d40434d30e525dd9e31fa671f
…essages

3d305e3 Send fewer spam messages in p2p_invalid_messages (James O'Beirne)

Pull request description:

  Builds on travis are failing because the test node isn't
  able to drop all the bad messages sent within the given
  timeout. Reduce the number of bad messages we're sending
  and increase the timeout to avoid failures on travis.

Tree-SHA512: 11c389619d9590caf7eca74e0efe6d950469415d34220072770689024b350cc08a2d5ec90634237d87ff71ba8b638c1152b8a45ffbb2815a48bde6a88fbb8fc6
5a1f576 qa: clean up assert_memory_usage_stable utility (James O'Beirne)
0cf1632 qa: fix p2p_invalid_messages on macOS (James O'Beirne)

Pull request description:

  Infinite mea culpa for the number of problems with this test.

  This change bumps the acceptable RSS increase threshold from 3% to 50% when spamming the test node with junk 4MB messages. On [@MarcoFalke's macOS test build](https://travis-ci.org/MarcoFalke/btc_nightly) we see RSS grow ~14% from ~71MB to 81MB, so a 50% increase threshold should be more than sufficient to avoid spurious failures.

Tree-SHA512: 150a7b88080fd883c7a5d0b9ffa470f61a97c4885fccc1a06fde6260aaef15640a7c1de7e89c581b245df7807d617ec3d86775330386ec5149ad567492fc5d31
0385109 Add test for rpcpassword hash error (MeshCollider)
13fe258 Error if rpcpassword in conf contains a hash character (MeshCollider)

Pull request description:

  Fixes bitcoin#13143 now bitcoin#13482 was merged

Tree-SHA512: e7d00c8df1657f6b8d0eee1e06b9ce2b1b0a2de487377699382c1b057836e1571dac313ca878b5877c862f0461ba789a50b239d2a9f34accd8a6321f126e3d2a
…ypath serialization

4e4de10 Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders)

Pull request description:

  Related to bitcoin#14689

  We should catch this error before attempting to deserialize it later.

Tree-SHA512: d2f3ea7f363818ac70c81ee988231b2bb50d055b6919f7bff3f27120c85a7048bfa183efae33e23e6b81d684bcb8bb81e5b209abb3acbcaff1d88014f4f1aa93
…nsaction

9b4a36e [qa] Test for duplicate inputs within a transaction (Suhas Daftuar)
b8f8019 Fix crash bug with duplicate inputs within a transaction (Suhas Daftuar)

Pull request description:

Tree-SHA512: 8c7ea34c7fa44188d86c04a690a7cbf8e9deda71ab1f7ca6d11de1f2abb3dd7222627071f86d0d39689a8b302ba9af142f0202466a67e30cd54aed3a08d4eb14
…aiting for the block request

fa21568 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke)
6c787d3 tests: Make feature_block pass on centos (MarcoFalke)

Pull request description:

  This hopefully fixes bitcoin#14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log.

  Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful.

  Unrelated to this, I also include a fix that makes the tests pass on latest CentOS.

Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f
fa5a6ce qa: Raise ci test_runner timeout to 40 mins (MarcoFalke)
fa3df02 travis: Avoid timeout on verify-commits check (MarcoFalke)

Pull request description:

  The verify-commits check is too expensive to run in full (calculate Tree-SHA512 and clean-merge for every single merge commit in history) every day (the cron job runs every ~24h). Since the cron job is running every day, it is also redundant to redo most of the work on the next day.

  So, only check two days worth of commits and assume that travis checked the Tree-SHA512 and clean-merge for all other commits already. The script will still check all the signatures, since the check-result for them depends on external inputs such as current time or the public keys we got from the server.

  [Note that travis is not meant to do the verification for anyone or is meant to be trusted in any way. This check only serves as a belt-and-suspender to notify maintainers in case of a technical issue or script malfunction. But since the script is timing out for months now, its purpose is diminished right now.]

Tree-SHA512: 336c5cbcc03cdf50be96cd61412471be9078d862da8ba2054f337441e062a6067c95fbbd03912e3de6a116f3caa75fd3f01a04864d34aae1489faa3154572815
b4f6e58 Better error message for user when corrupt wallet unlock fails (MeshCollider)

Pull request description:

  Mentioned here: bitcoin#14461 (comment)

  Current behavior is to assert(false) and crash, only info is printed in the log. This shows the message to the user before abort() instead.

Tree-SHA512: 526f9ed9262257fca55caf7153ab913ed958b13b079d2f01db797485614d8c375815a1554276e8cf73d3838104b2691a9cf85c8d097973127ae8de9e111446bf
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

@UdjinM6 UdjinM6 added this to the 18 milestone Aug 29, 2021
@UdjinM6 UdjinM6 merged commit f38aa44 into dashpay:develop Aug 29, 2021
@PastaPastaPasta PastaPastaPasta deleted the pr_4330 branch August 30, 2021 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants