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
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
## 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>
d841301010914203fb5ef02627c76fad99cb11f1 test: Add docstring to wait_until() in util.py to warn about its usage (Seleme Topuz)
1343c86c7cc1fc896696b3ed87c12039e4ef3a0c test: Update wait_until usage in tests not to use the one from utils (Seleme Topuz)
Pull request description:
Replace global (from [test_framework/util.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L228)) `wait_until()` usages with the ones provided by `BitcoinTestFramework` and `P2PInterface` classes.
The motivation behind this change is that the `util.wait_until()` expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. `BitcoinTestFramework` offers a `wait_until()` which has an understandable amount of default `timeout` and a shared `timeout_factor`. Moreover, on top of these, `mininode.wait_until()` also has a shared lock.
closes#19080
ACKs for top commit:
MarcoFalke:
ACK d841301010914203fb5ef02627c76fad99cb11f1 🦆
kallewoof:
utACK d841301010914203fb5ef02627c76fad99cb11f1
Tree-SHA512: 81604f4cfa87fed98071a80e4afe940b3897fe65cf680a69619a93e97d45f25b313c12227de7040e19517fa9c003291b232f1b40b2567aba0148f22c23c47a88
dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64 refactor: test: use _ variable for unused loop counters (Sebastian Falbesoner)
Pull request description:
This tiny PR substitutes Python loops in the form of `for x in range(N): ...` by `for _ in range(N): ...` where applicable. The idea is indicating to the reader that a block (or statement, in list comprehensions) is just repeated N times, and that the loop counter is not used in the body, hence using the throwaway variable. This is already done quite often in the current tests (see e.g. `$ git grep "for _ in range("`). Another alternative would be using `itertools.repeat` (according to Python core developer Raymond Hettinger it's [even faster](https://twitter.com/raymondh/status/1144527183341375488)), but that doesn't seem to be widespread in use and I'm not sure about a readability increase.
The only drawback I see is that whenever one wants to debug loop iterations, one would need to introduce a loop variable again. Reviewing this is basically a no-brainer, since tests would fail immediately if a a substitution has taken place on a loop where the variable is used.
Instances to replace were found by `$ git grep "for.*in range("` and manually checked.
ACKs for top commit:
darosior:
ACK dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64
instagibbs:
manual inspection ACK dac7a111bd
practicalswift:
ACK dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64 -- the updated code is easier to reason about since the throwaway nature of a variable is expressed explicitly (using the Pythonic `_` idiom) instead of implicitly. Explicit is better than implicit was we all know by now :)
Tree-SHA512: 5f43ded9ce14e5e00b3876ec445b90acda1842f813149ae7bafa93f3ac3d510bb778e2c701187fd2c73585e6b87797bb2d2987139bd1a9ba7d58775a59392406
## Issue being fixed or feature implemented
we should not vote on triggers from the past
## What was done?
## How Has This Been Tested?
n/a
## 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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Addressed issues and comments from [PR
comment](https://github.com/dashpay/dash/pull/5469#discussion_r1317886678)
and [PR
comment](https://github.com/dashpay/dash/pull/5469#discussion_r1338704082)
`Params()` should be const; global variable `CMNHFManager` is a better
out-come.
## What was done?
The helpers and direct calls of `UpdateMNParams` for each block to
update non-constant member in `Params()` is not needed anymore. Instead
`CMNHFManager` takes cares about status of Signals for each block,
update them dynamically and save in evo db.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
Changed rpc `getblockchaininfo`.
the field `ehf` changed meaning: it's now only a flag -1/0; but it is
introduced a new field `ehf_height` now that a height.
## 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>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: thephez <thephez@users.noreply.github.com>
## Issue being fixed or feature implemented
MNs don't really vote NO on triggers that do not match their local
candidates because:
1. they bail out too early when they see that they are not the payee
2. the hash for objects to vote NO on was picked incorrectly.
## What was done?
Moved voting out of `CreateGovernanceTrigger` and into its own
`VoteGovernanceTriggers`. Refactored related code to use `optional`
while at it, dropped useless/misleading `IsValid()` call. Added some
safety belts, logging, tests.
## How Has This Been Tested?
Run tests.
## 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)_
## Issue being fixed or feature implemented
```
test/functional/feature_governance.py:205:59: F821 undefined name 'p0_amount'
test/functional/feature_governance.py:205:95: F821 undefined name 'p1_amount'
test/functional/feature_governance.py:205:131: F821 undefined name 'p2_amount'
```
## What was done?
add missing `self.`
## How Has This Been Tested?
run linter and `feature_governance.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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Noticed a couple of things while I was trying to figure out if an
[issue](https://github.com/dashpay/dash/pull/5627#discussion_r1367153099)
@knst mentioned in #5627 could actually exist:
1. `GetPaymentsLimit()` won't work correctly with historical blocks rn.
We don't use it that way internally but it could be done via rpc and it
should provide correct results.
2. superblock params on regtest are too small to test them properly
3. because of (2) and a huge v20 activation window (comparing to sb
params) `feature_governance.py` doesn't test v20 switching states.
There's also no "sb on v20 activation block" test.
~NOTE: based on #5639 atm~
## What was done?
fix it, pls see individual commits
## How Has This Been Tested?
run tests
## 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)_
## Issue being fixed or feature implemented
sb produced by sentinel:
>"DataString": ... \"payment_amounts\": \"20.00000000|20.00000000\", ...
>...
> "YesCount": 83,
sb produced by core:
>"DataString": ... \"payment_amounts\": \"20.00|20.00\", ...
> "YesCount": 13,
These 2 triggers are for the same block (900552), proposal hashes and
addresses are also the same but the difference in `payment_amounts`
format makes it look like a different trigger for core and this creates
a race.
## What was done?
Use `ValueFromAmount` instead of `FormatMoney` to avoid trimming
## How Has This Been Tested?
run 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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
#5564 is a bit too optimistic about incoming triggers
## What was done?
Rework governance logic to only approve triggers that match our
expectations i.e. have the same data hash as our own trigger would have
if we would have to submit it.
## How Has This Been Tested?
Run tests
## Breaking Changes
Voting is done in `CreateGovernanceTrigger` only now meaning that it
only happens on next block for incoming triggers. Tweaked tests
accordingly.
## 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)_
## Issue being fixed or feature implemented
In case MNs didn't submit their own trigger, should vote for funding yes
when receiving triggers from other nodes.
## What was done?
Check if already submitted theirs and vote accordingly.
## How Has This Been Tested?
## Breaking Changes
## 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
Implementation of issue https://github.com/dashpay/dash-issues/issues/43
## What was done?
Masternode will try to create, sign and submit a Superblock (GovTrigger)
during the `nSuperblockMaturityWindow`.
## 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 _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
