Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Aug 10, 2021

Commit "baa7c45 Fixed test by updating length of message accoringly dash" is a fix for p2p_invalid_message from previous commit and probably should be squashed with original commit "bd75487 Merge bitcoin#14522: tests: add invalid P2P message tests".

All other are taken as it is with only resolved conflicts, no any additional edits.

@knst knst changed the title backport 0.18 batch 1 backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 #14365 Aug 10, 2021
@knst knst force-pushed the knst-0.18-batch-1 branch from edd2d3b to 39c2c4b Compare August 10, 2021 17:32
@knst knst changed the title backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 #14365 backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 Aug 10, 2021
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.

14700 and 14705 should be named as "partial merge ..."

laanwj and others added 10 commits August 17, 2021 03:07
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
…in a transaction

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
…check

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
@knst knst force-pushed the knst-0.18-batch-1 branch from 39c2c4b to 32f1460 Compare August 16, 2021 20:10
@knst knst changed the title backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 #14247 Aug 16, 2021
@UdjinM6 UdjinM6 added this to the 18 milestone Aug 17, 2021
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 👍 (linter failure is unrelated and should be fixed by #4344 )

utACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

A few comments, looks good

conn2 = node.add_p2p_connection(P2PDataStore())

msg_limit = 4 * 1000 * 1000 # 4MB, per MAX_PROTOCOL_MESSAGE_LENGTH
msg_limit = 3 * 1024 * 1024 # 3MB, per MAX_PROTOCOL_MESSAGE_LENGTH
Copy link
Member

Choose a reason for hiding this comment

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

As a tangent, @UdjinM6 do you know why our limit is different?

Copy link

Choose a reason for hiding this comment

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

IIRC: We bumped it while switching to 2MB blocks, they bumped it while implementing segwit (which we ignore so we never aligned this limit).


node.p2p.send_blocks_and_test([block2], node, success=False, request_block=False, reject_code=16, reject_reason=b'bad-txns-duplicate')

# Check transactions for duplicate inputs
Copy link
Member

Choose a reason for hiding this comment

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

Why is 14247 partial? Why can't we do what's done in validation.cpp. If that was already done at some point, then this PR shouldN'T be marked partial

git log --merges --before="2 days ago" -1 --format='%H' > ./contrib/verify-commits/trusted-sha512-root-commit
while read -r LINE; do travis_retry gpg --keyserver hkp://subset.pool.sks-keyservers.net --recv-keys $LINE; done < contrib/verify-commits/trusted-keys &&
travis_wait 50 contrib/verify-commits/verify-commits.py;
travis_wait 50 contrib/verify-commits/verify-commits.py --clean-merge=2;
Copy link
Member

Choose a reason for hiding this comment

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

imo 14705 should not be marked partial. the test_runner changes aren't really relevant to us.

@github-actions
Copy link

This pull request has conflicts, please rebase.

@PastaPastaPasta
Copy link
Member

Please see https://github.com/PastaPastaPasta/dash/tree/pr_4330

This is a rebased branch, with 14247 and 14705 being not marked partial as I suggested. Please apply this as the branch, if you would like me to just create a new PR, I can do that as well. I tried just pushing to this PR, but it seems you have that setting disabled.

@PastaPastaPasta
Copy link
Member

Superseded

@UdjinM6 UdjinM6 removed this from the 18 milestone Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants