-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 #14247 #4330
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
Conversation
edd2d3b to
39c2c4b
Compare
UdjinM6
left a comment
There was a problem hiding this 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 ..."
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
39c2c4b to
32f1460
Compare
UdjinM6
left a comment
There was a problem hiding this 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
PastaPastaPasta
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
|
This pull request has conflicts, please rebase. |
|
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. |
|
Superseded |
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.