You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Merge #6718: test: consolidate masternode info tracking (MasternodeInfo) in functional tests, implement helpers, add type annotations
0532baf refactor: add helper for finding collateral vout (Kittywhiskers Van Gogh)
89b2548 refactor: add helper for transaction burial (Kittywhiskers Van Gogh)
f3777c5 refactor: introduce address generation helper, make constructor minimal (Kittywhiskers Van Gogh)
fa56126 refactor: add type annotations for `MasternodeInfo` members (Kittywhiskers Van Gogh)
dfca0ee refactor: use `MasternodeInfo` in feature_dip3_deterministicmns.py (Kittywhiskers Van Gogh)
cd7ffcf refactor: store only the node index in `MasternodeInfo` (Kittywhiskers Van Gogh)
07c7319 refactor: use `nodeIdx` instead of `node.index` with `MasternodeInfo` (Kittywhiskers Van Gogh)
08e81d8 refactor: store P2P port instead of full address in `MasternodeInfo` (Kittywhiskers Van Gogh)
3e25b30 refactor: store the funding address in `MasternodeInfo` (Kittywhiskers Van Gogh)
08d18d7 refactor: annotate usage of `MasternodeInfo` (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
While working on functional tests for [dash#6666](#6666) (which in turn, relies on the foundation built by [dash#6674](#6674), see `rpc_netinfo.py`, [source](https://github.com/dashpay/dash/blob/c788531d35c48820af951799ff8b67ee2dadb8b3/test/functional/rpc_netinfo.py)), the existing test infrastructure, while well optimized for testing interactions _between_ masternodes, didn't offer enough flexibility for testing _creation_ of masternodes.
The tests that need to be implemented for extended addresses mimic `feature_dip3_deterministicmns.py` ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/feature_dip3_deterministicmns.py)) in objective and while taking cues from it, it was found that instead of the `MasternodeInfo` used in most of the codebase ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/test_framework.py#L1138-L1153)), `feature_dip3_deterministicmns.py` implements its own object, `Masternode` ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/feature_dip3_deterministicmns.py#L223-L227)).
In a similar vein, `rpc_netinfo.py`, as implemented in c788531, implemented its own variant, `Node` ([source](https://github.com/dashpay/dash/blob/c788531d35c48820af951799ff8b67ee2dadb8b3/test/functional/rpc_netinfo.py#L31-L194)) to address the testing needs for extended addresses. It became clear that without additional intervention, the test suite would have three different ways of representing masternode information (`MasternodeInfo`, `Masternode`, `Node`) and that it would be more beneficial to consolidate all three approaches together.
So, taking cue from `Node` (from `rpc_netinfo.py`, not included in `develop` as of this writing, [source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/rpc_netinfo.py)), this pull request aims to clean up, consolidate and improve upon `MasternodeInfo` in an attempt to remove the need for `Node`.
## Additional Information
* PEP 526 ("Syntax for Variable Annotations") prohibit annotation of variables defined by a `for` or `with` statement ([source](https://peps.python.org/pep-0526/#where-annotations-aren-t-allowed)), to get around that, the syntax for type comments ([source](https://peps.python.org/pep-0484)) as defined in PEP 484 ("Type Hints") have been used instead.
* The decision to remove `node` from `MasternodeInfo` despite its convenience was rooted in the observation that there are cases where `nodeIdx` and `node` are assigned separately ([source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/test_framework.py#L1480), [source](https://github.com/dashpay/dash/blob/09aa42ef0f20dc94ea63bdc8e7e064d494c9ae8f/test/functional/test_framework/test_framework.py#L1499)), which highlighted the fact that `MasternodeInfo` has no control over the lifetime of the `TestNode` instance as it is ultimately controlled by `BitcoinTestFramework`.
To avoid potential error and to make it syntactically clear that ownership is vests with `BitcoinTestFramework`, `node` has been removed and replaced with `get_node()`, which alongside some basic sanity checks, is just an alias for `self.nodes[mn.nodeIdx]` but is still less tedious to type or read.
* The reason why functions like `generate_addresses()` accept a separate `TestNode` ([source](https://github.com/dashpay/dash/blob/c5444b3f7c06aa5b060fd542773df6dd5a9bae1f/test/functional/test_framework/test_framework.py#L1160-L1174)) is due to the practice of using a single node to instantiate all masternodes instead of keeping them as self-contained instances that mine their own blocks and fund their own collaterals (for instance, `DashTestFramework` uses `nodes[0]` to setup all the masternodes even if the masternodes themselves had a different index).
* The decision to replace `addr` with `nodePort` has been inspired by the need to register masternodes with non-loopback addresses (like in `feature_dip3_deterministicmns.py` or `rpc_netinfo.py`). As a node that is intended to be used will always need the same standardized loopback address, we can safely resort to only storing the port number and save a few `p2p_port()` calls along the way.
## Breaking Changes
None expected. Affects only functional tests.
## 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
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 0532baf
UdjinM6:
utACK 0532baf
Tree-SHA512: c2b5b0f9200c2714159a5e0f6f52174e38eae426c01620cf15e9adb8b8e67bbb078d658edb21b052a1fc9896ba098cffa02248aaa980be5d5cb145e9929c4568
0 commit comments