fa1cd9e1ddc6918c3d600d36eadea71eebb242b6 test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794e93b4f5976e81793a4a63aa03a2c8c686 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41bf1d1b7ee552c627f9829b4494b817ce28 test: Remove unused p2p_lock in VersionBitsWarningTest (MarcoFalke)
Pull request description:
This avoids confusion with the `wait_until` member functions, which should be preferred because they take the appropriate locks and scale the timeout appropriately on their own.
ACKs for top commit:
laanwj:
Code review ACK fa1cd9e1ddc6918c3d600d36eadea71eebb242b6
hebasto:
ACK fa1cd9e1ddc6918c3d600d36eadea71eebb242b6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c
ed5cd12869e0691a785199d2d977ce5879095180 test: Distinguish between nodes(bitcoind) and peers(mininodes) in p2p_leak.py (Dhruv Mehta)
f6f082b9343522bc8005f23937ac6ecf56548c98 test: remove `CNodeNoVersionIdle` from p2p_leak.py (Dhruv Mehta)
45cf55ccac94689e48dd0648ed2401918a778024 test: remove `CNodeNoVersionMisbehavior` from p2p_leak.py (Dhruv Mehta)
Pull request description:
- Removes `CNodeNoVersionMisbehavior` per recommendation at https://github.com/bitcoin/bitcoin/pull/19657#issuecomment-669926458
- Removes `CNodeNoVersionIdle` because it is similarly unnecessary
- As someone new to the codebase, I found it easier to understand it if `no_version_disconnect_node` tries to overwhelm the peer with any message that is not version/verack.
- Per recommendation at https://github.com/bitcoin/bitcoin/pull/19727#pullrequestreview-468093555, made a clear distinction between nodes(bitcoind) and peers(mininode interface implementations)
ACKs for top commit:
jnewbery:
tested ACK ed5cd12869e0691a785199d2d977ce5879095180
amitiuttarwar:
utACK ed5cd12869
Tree-SHA512: 310a24c91fd837e7f65177edb55fe6142fb3559fae7867c5cdd9c9a23b1a02202b935ca9a82633fa7649f3de2fa221f6da906a7b5e499fc20f7254085033757d
fa4dfd215f62e88d668311701735c332c264fa2a test: Wait until is_connected in add_p2p_connection (MarcoFalke)
Pull request description:
Moving the wait_until from the individual test scripts to the test framework simplifies two tests
ACKs for top commit:
jnewbery:
Code review ACK fa4dfd215f62e88d668311701735c332c264fa2a
theStack:
ACK fa4dfd215f☕
Tree-SHA512: 36eda7eb323614a4c4f9215f1d7b40b9f9c4036d1c08eb701ea705f3e2986fdabd2fc558965a6aadabeed861034aeaeef3c00f968ca17ed7a27e42e506cda87d
faa9a74c9e99eb43ba0d27fa906767ee88011aeb test: Fail wait_until early if connection is lost (MarcoFalke)
Pull request description:
Calling `minonode.wait_until` needs a connection to make progress (e.g. waiting for an inv), unless the mininode waits for the initial connection or for a disconnection. So for test development and failure debugging, fail early in all `wait_until`, unless opted out.
ACKs for top commit:
jnewbery:
Code review ACK faa9a74c9e99eb43ba0d27fa906767ee88011aeb.
Tree-SHA512: 4be850b96e23b87bc2ff42c028a5045d6f5cdbc9482ce6a6ba01cc5eb26710dab9e2ed547c363aac4bd5825151ee9996fb797261420b631bceeddbfa698d1dec
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2fa1153c6d76efc8113fa01b06943ece util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
4aff7a48a4e0f1075306f181a276b8a74c857022 test: check importing wallets when blocks are pruned throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
437b608df2/src/wallet/rpc/backup.cpp (L513-L518)
ACKs for top commit:
andrewtoth:
ACK 4aff7a48a4e0f1075306f181a276b8a74c857022
Tree-SHA512: fbbf6056cb3759f726b8a5ff25fca51bf47e973e5d655ec164e2bec88e2dbd3b243677869d2cf33af268ea635ca0f2e9f737c4734077fc5a936ac3a24ad4b88b
fadddd13eef4428f5fa7237583d4be41a9335cd9 test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
faa211fc6e3d4984b8edff1d762dd4cba205d982 test: Misc cleanup (MarcoFalke)
fa1668bf5084a190b26b022b9e625a7be3defa6e test: Run pep-8 (MarcoFalke)
facd97ae0f0d816107aa3bc9de321244200636a0 scripted-diff: Renames (MarcoFalke)
Pull request description:
The index on the block filters is running in the background on the validation interface. To avoid intermittent test failures, it needs to be synced.
Also other cleanups.
ACKs for top commit:
lsilva01:
Tested ACK fadddd13ee on Ubuntu 20.04
Tree-SHA512: d858405db426a2f9d5620059dd88bcead4e3fba3ccc6bd8023f624b768fbcfa2203246fb0b2db620490321730d990f0e78063b21a26988c969cb126d4bd697bd
39a9ec579f023ab262a1abd1f0c869be5b1f3f4d Unconditionally check for fRelay field in test framework (Troy Giorshev)
Pull request description:
picking up #20411 (rebased onto master)
There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead, we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
ACKs for top commit:
jnewbery:
utACK 39a9ec579f023ab262a1abd1f0c869be5b1f3f4d
Tree-SHA512: 13a23f1180b7121ba41cb85baa38094b41f4607a7c88b3384775177cb116e76faf5514760624f98a4e8a830767407c46753a7e0285158c33e0c6ce395de8f15c
fada8b019af104a0df7659ede2618f594bb3e78a test: Add missing assignment in mempool_resurrect.py (MarcoFalke)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: f438d85cd14a91eabfc380d9ee120cc7a7f9103cf0cd1cf565f675f386f82d966901c0ad3f60b8c462642fbf0a3791dbbd774f9b07668d22b58eb575c8d702c1
11a32722f09f1d81f34bd09b26248ba99f2e7f07 test: run mempool_resurrect.py even with wallet disabled (Michael Dietz)
Pull request description:
Another functional test rewritten as proposed in #20078
**Request for help:**
`node.gettransaction(txid)` fails for transactions sent with `wallet.send_self_transfer`. Even though the `txid`s look correct, are added to the mempool correctly, and removed from the mempool when a block is mined - all as expected.
However, `node.gettransaction(txid)` throws the error:
```sh
Traceback (most recent call last):
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "/Users/michaeldietz/Documents/bitcoin/test/functional/mempool_resurrect.py", line 43, in run_test
assert_equal(len(list(filter(lambda txid: node.gettransaction(txid)["confirmations"] > 0, spends_ids))), len(spends_ids))
File "/Users/michaeldietz/Documents/bitcoin/test/functional/mempool_resurrect.py", line 43, in <lambda>
assert_equal(len(list(filter(lambda txid: node.gettransaction(txid)["confirmations"] > 0, spends_ids))), len(spends_ids))
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)
```
Anyone know what's going wrong / can point me in the right direction if I'm making a mistake, or `MiniWallet` needs to be improved for this to work correctly?
ACKs for top commit:
MarcoFalke:
ACK 11a32722f09f1d81f34bd09b26248ba99f2e7f07
Tree-SHA512: 13d83a13ec23920db716e99b68670e61329d1cc73b12063d85bc1679ee6425a9951da4d2e392ca1f27760be7be049ccdc6f504e192ed5cd24ed0ba003b66fab3
6b56c1f4d0d5857d9d61a81dc96db1b603c368b5 test: remove last_{block,header}_equals() in p2p_fingerprint.py (Sebastian Falbesoner)
136d96b71f94bde2c7471ed852d447ec008e3a30 test: use wait_for_{block,header} helpers in p2p_fingerprint.py (Sebastian Falbesoner)
Pull request description:
This small PR takes use of the message receiving helper functions `wait_for_block()` and `wait_for_header()` (from module `test_framework.p2p`) in the test `p2p_fingerprint.py`. It also simplifies the checks for very old stale blocks/headers requests by getting rid of the functions `last_block_equals()` and `last_header_equals()` and rather only testing that not any blocks/headers message is received at all. Unneeded sending of requests are also removed and calls to time.sleep(...) substituted by ping syncs.
ACKs for top commit:
guggero:
ACK 6b56c1f4
Tree-SHA512: 9114db70f3804adad4ab658236762d4fa73fef91158c5756dd1af2d24196ea740451b0028667e0c4047f1f89fe1355031921d3dfb973acc1370052a4bc12c2ab
4f67336f1105b7c34a9e8cdafa603edc1d899fb9 test: verify best blockhash after invalidating an unknown block (brunoerg)
Pull request description:
Fixes#26051
Verify the best blockhash is the same after invalidating an unknown block, not the whole `getchaintip` response.
ACKs for top commit:
instagibbs:
ACK 4f67336f1105b7c34a9e8cdafa603edc1d899fb9
Tree-SHA512: 2d71743c1d3a317ef7b750f88437df71d1aed2728d9edac8b763a343406e168b97865ab25ec4c89caf09d002e076458376618cbd0845496375f7179633c88af9
4b1d5a10537ab48e3457606ba1cf2ae26a1cb2b2 test: invalidating an unknown block throws an error (brunoerg)
Pull request description:
While playing with `invalidateblock`, I unintentionally tried to invalidate an unknown block and it threw an error. Looking at the tests I just realized there is no test coverage for this case. This PR adds it.
Top commit has no ACKs.
Tree-SHA512: 25286ead809b3ad022e759127ef3134b271fbe76cb7b50ec2b0c7e2409da8d1b01dc5e80afe73e4564cc9c9c03487a1fe772aea3456988552d2f9c8fb34c730b
4edc6893825fd8c45c53c81c73a6a7801e1b458c doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner)
Pull request description:
As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by https://github.com/bitcoin/bitcoin/pull/25792#discussion_r941180819).
ACKs for top commit:
kouloumos:
ACK 4edc6893825fd8c45c53c81c73a6a7801e1b458c
1440000bytes:
ACK 4edc689382
w0xlt:
ACK 4edc689382
fanquake:
ACK 4edc6893825fd8c45c53c81c73a6a7801e1b458c
Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
1821d92b66 test: add activate_ehf_by_name (Alessandro Rezzi)
de38dca242 feat(consensus): Generalize ehf activation (Alessandro Rezzi)
Pull request description:
## Issue being fixed or feature implemented
Try to sign/mine any ehf deployment and not only mn_rr. As asked in the review this decouples (and improves) commit [e24cb23](e24cb239f8) from PR #5799
## What was done?
See commit description
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Top commit has no ACKs.
Tree-SHA512: 7112a5214b473dea29a892563e6598eca089d2e17fdff8bda08c7777738f1054d2cb07e0a7dccf66214911fec64a6441e84100273ac2ed52abe1d33fb960e8cc
76a84c0a6cac3df711b1ed907a46c905cb85c485 test: speedup wallet_coinbase_category.py (furszy)
Pull request description:
No need to create a chain (200 extra blocks), nor use the cache, for it.
Top commit has no ACKs.
Tree-SHA512: beec64ba6c580d475e19700371ae155f4f3d74325879802da83d02f4153a752d5829b8a4ed77e0c893e79cbc26f66389eed90ac2e8b03a59f6c630ee9333355c
ceec6808d331fa082407a734cd5f3c2f1c7d11b3 test: `-whitebind` and `-bind` with `-listen=0` should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
b9122e95f0/src/init.cpp (L872-L875)
ACKs for top commit:
laanwj:
Code review ACK ceec6808d331fa082407a734cd5f3c2f1c7d11b3
Tree-SHA512: 03068abe7199b1235f029871ab87a3dd4943738c592ad62d82cdcd3e0201e627624960bd3ea1fc6fc1e7da4b8e215ba3393d1cb8130e1108049f764e51dc75c0
5a8c321444c10c7c89c4222c0b47c2d83a1a9ea4 test: check for `getblocktxn` request with out-of-bounds tx index (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `getblocktxn` message handler, in the case that any of the contained indices is out-of-bounds:
a05876619a/src/net_processing.cpp (L2180-L2183)
ACKs for top commit:
dunxen:
ACK 5a8c321
Tree-SHA512: 2743c2c6d8aed57b22f825aefd60ba3e670321b60625a42ea7248e7b0fc41c73e9a5945153567c02824ba3b5f0fce7f4125bffc974973fc608b6ffbe49e14b65
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
that's a result of:
contrib/devtools/copyright_header.py update ./
it is not scripted diff, because it works differentlly on my localhost and in CI:
CI doesn't want to use git commit date which is mocked to 30th Dec of 2023
ea54ba2f42f6d0b23570c665c2369f977bf55cf6 [test] Fix port collisions caused by p2p_getaddr_caching.py (dergoegge)
f9682e75ac184a62c7e29287882df34c25303033 [test_framework] Set PortSeed.n directly after initialising params (dergoegge)
Pull request description:
This PR fixes the issue mentioned [here](https://github.com/bitcoin/bitcoin/pull/25096#discussion_r892558783), to avoid port collisions between nodes spun up by the test framework.
Top commit has no ACKs.
Tree-SHA512: ec9159f0af90db636f7889d664c24e1430cf2bcb3c02a9ab2dcfe531b2a4d18f6e3a0f8ba73071bdf2f7db518df9d5d86a9cd06695e67644d20fe4515fac32b7
fa76d8d4d71d844e217686881d4f630eac3a8e10 test: Actually print TSan tracebacks (MarcoFalke)
Pull request description:
Commit 5e5138a721738f47053d915e4c65f925838ad5b4 made the TSan logs to be printed before returning an error from the ci script.
However, it seems that on Cirrus CI, the `--failfast` option will kill not only all python process and bitcoind child process, but also the parent CI bash script, rendering the `trap` inefficient. I believe this bug was introduced in commit 451b96f7d2796d00eabaec56d831f9e9b1a569cc.
ACKs for top commit:
fanquake:
utACK fa76d8d4d71d844e217686881d4f630eac3a8e10
Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
## Issue being fixed or feature implemented
On my local kubuntu linters have way too much spam
## What was done?
See each commit
## How Has This Been Tested?
Run locally. Amount of warnings decreased from thousands to fewer
amount. Excluding typos, they are:
```
src/coinjoin/client.cpp:1420:5: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/client.cpp:1426:5: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/client.cpp:655:26: warning: Consider using std::copy_if algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/server.cpp:593:33: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/server.cpp:630:106: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/governance/governance.cpp:1057:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1068:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1079:13: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1086:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1094:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1099:5: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1486:34: warning: Consider using std::copy_if algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/commitment.cpp:102:5: warning: Consider using std::all_of or std::none_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/instantsend.cpp:820:38: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/quorums.cpp:831:102: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/quorums.h:300:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:301:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:302:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:303:17: warning: C-style pointer casting [cstyleCast]
src/spork.cpp:119:58: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/statsd_client.cpp:234:63: warning: C-style pointer casting [cstyleCast]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
^---- failure generated from test/lint/lint-cppcheck-dash.sh
Consider install flake8-cached for cached flake8 results.
test/functional/data/invalid_txs.py: error: Source file found twice under different module names: "invalid_txs" and "data.invalid_txs"
test/functional/data/invalid_txs.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
test/functional/data/invalid_txs.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)
^---- failure generated from test/lint/lint-python.s
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
fix failures like https://gitlab.com/dashpay/dash/-/jobs/6175160403
## What was done?
use `minimumAmount` option in `listunspent` rpc call to avoid picking
coins that are too small for asset lock txes
## How Has This Been Tested?
run `feature_asset_locks.py`
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
faa94cb1675d8bd511eb593176cd07aa59465225 test: Check that invalid peer traffic is accounted for (MarcoFalke)
fae243f0cb92b5648d07d0a5033e2f4de862ae99 test: Remove confusing cast to same type (int to int) (MarcoFalke)
Pull request description:
Couldn't find a test for this and it seems something we should test, so I wrote one.
ACKs for top commit:
vasild:
ACK faa94cb16
practicalswift:
ACK faa94cb1675d8bd511eb593176cd07aa59465225: patch looks correct
Tree-SHA512: efcdd35960045cdfbd14480e16e0d1d09e81eb01670ac541ac2b105e1a63818a157c95853270242234a224880873e79957832bf4231374d7fe81de30f35e3abf
## Issue being fixed or feature implemented
To prevent spending withdrawal before that's finalized, mined and
chainlocked, next things should be true:
- txes with spending of unlock and unlock themselves do not receive
islocks. That's true, because IS excluded for txes with no inputs.
- When the unlock is removed from mempool, so are the children.
These functionality has no tests, but that's crucial for consensus be
fine.
## What was done?
Implemented checks to be sure that IS is not send for Withdrawal txes.
Functional test for asset_locks.py are refactored a bit to make it
easier to read.
## How Has This Been Tested?
This PR is tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
9913419cc9db5f8ce7afa0c3774468c330136064 test: remove type: comments in favour of actual annotations (fanquake)
Pull request description:
Now that we require Python 3.6+, we should be using variable type
annotations directly rather than `# type:` comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
ACKs for top commit:
MarcoFalke:
review ACK 9913419cc9db5f8ce7afa0c3774468c330136064
jnewbery:
Code review ACK 9913419cc9db5f8ce7afa0c3774468c330136064
Tree-SHA512: 63aba5eef6c1320578f66cf8a6d85ac9dbab9d30b0d21e6e966be8216e63606de12321320c2958c67933bf68d10f2e76e9c43928e5989614cea34dde4187aad8
bff7c66e67aa2f18ef70139338643656a54444fe Add documentation to contrib folder (Troy Giorshev)
381f77be858d7417209b6de0b7cd23cb7eb99261 Add Message Capture Test (Troy Giorshev)
e4f378a505922c0f544b4cfbfdb169e884e02be9 Add capture parser (Troy Giorshev)
4d1a582549bc982d55e24585b0ba06f92f21e9da Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97bec09dd5fcc043d8659d8ec5dfb87c2 Add CaptureMessage (Troy Giorshev)
dbf779d5deb04f55c6e8493ce4e12ed4628638f3 Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe only some minor changes: 👚
jnewbery:
utACK bff7c66e67aa2f18ef70139338643656a54444fe
theStack:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
## Issue being fixed or feature implemented
Should fix failures like
https://gitlab.com/dashpay/dash/-/jobs/6107697452
## What was done?
See inline comments
## How Has This Been Tested?
Run lots of `feature_governance.py` in parallel multiple times
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
## Issue being fixed or feature implemented
This pull request is a follow-up to
[some](https://github.com/dashpay/dash/pull/5834#discussion_r1470105685)
[feedback](https://github.com/dashpay/dash/pull/5834#discussion_r1467009815)
received on [dash#5834](https://github.com/dashpay/dash/pull/5834) as
the patterns highlighted were present in different parts of the codebase
and hence not corrected within the PR itself but addressed separately.
This is that separate PR 🙂 (with some additional cleanup of my own)
## What was done?
* This pull request will remain a draft until
[dash#5834](https://github.com/dashpay/dash/pull/5834) as it will
introduce more changes that will need to be corrected in this PR.
* Code introduced that is unique to Dash Core (CoinJoin, InstantSend,
etc.) has been excluded from un-Dashification as the purpose of it is to
reduce backport conflicts, which don't apply in those cases.
* `CWallet::CreateTransaction` and the `CreateTransactionTest` fixture
have been excluded as the former originates from
[dash#3668](https://github.com/dashpay/dash/pull/3668) and the latter
from [dash#3667](https://github.com/dashpay/dash/pull/3667) and are
distinct enough to be unique to Dash Core.
* There are certain Dashifications and SegWit-removals that prove
frustrating as it would break compatibility with programs that rely on
the naming of certain keys
* `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and
`getmempoolentry` return `vsize` which is currently an alias of `size`.
I have been advised to retain `vsize` in lieu of potential future
developments. (this was originally remedied in
219a1d08973e7ccda6e778218b9a8218b4aae034 but has since been dropped)
* `getaddressmempool`, `getaddressutxos` and `getaddressdeltas` all
return a value with the key `satoshis`. This is frustrating to rename to
`duffs` for compatibility reasons.
* `decodepsbt` returns (if applicable) `non_witness_utxo` which is
frustrating to rename simply to `utxo` for the same reason.
* `analyzepsbt` returns (if applicable) `estimated_vsize` which
frustrating to rename to `estimated_size` for the same reason.
## How Has This Been Tested?
## Breaking Changes
None
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
HD wallets are old-existsing feature, appeared in Dash years ago, but
enabling HD wallets is not trivial task that requires multiple steps and
command line/rpc calls.
Let's have them enabled by default.
## What was done?
- HD wallets are enabled by default. Currently behavior `dashd`,
`dash-qt` are similar to run with option `-usehd=1`
- the rpc `upgradewallet` do not let to upgrade from non-HD wallet to HD
wallet to don't encourage user use non-crypted wallets (postponed till
v21)
- the initialization of ScriptPubKey is updated to be sure that encypted
HD seed is never written on disk (if passphrase is provided)
- enabled and dashified a script `wallet_upgradewallet.py` which test
compatibility between different versions of wallet
## What is not done?
- wallet tool still does not support passhprase, HD seed can appear on
disk
- there's no dialog that show user a mnemonic phrase and encourage him
to make a paper backup
Before removing a command line 'usehd' (backport bitcoin#11250) need to
make at least one major release for fail-over option (if someone wish to
use non-HD wallets only).
## How Has This Been Tested?
Run unit and functional tests.
Enabled new functional test `wallet_upgradewallet.py` that has been
backported long time ago but waited this PR to be enabled.
## Breaking Changes
HD wallets are created by default.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Fix failures like https://gitlab.com/dashpay/dash/-/jobs/6120923632
## What was done?
Handle disconnects and reconnection of the revoked MN in the right
place.
## How Has This Been Tested?
Run multiple `feature_dip3_v19.py` in parallel a few times
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
