8f5dc8800aeb524eee2fa2451cd22883b7b2bfec test: display command line options passed to send_cli() in debug log (Jon Atack)
Pull request description:
as per https://github.com/bitcoin/bitcoin/pull/18691#discussion_r411382589, and revert two cli calls changed in #18691 from rpc commands back to command line options (these were the only occurrences).
ACKs for top commit:
MarcoFalke:
ACK 8f5dc8800aeb524eee2fa2451cd22883b7b2bfec
Tree-SHA512: fcb3eca00aa4099066028c90d5e50a02e074366e09a17f5f5b937d9f7562dd054ff65681aa0ad4c94f6de1e98b1e2b9ac4cd084ddc297010253989a80483b1b9
## Issue being fixed or feature implemented
This is an implementation of DIP0027 "Credit Asset Locks".
It's a mechanism to fluidly exchange between Dash and credits.
## What was done?
This pull request includes:
- Asset Lock transaction
- Asset Unlock transaction (withdrawal)
- Credit Pool in coinbase
- Unit tests for Asset Lock/Unlock tx
- New functional test `feature_asset_locks.py`
RPC: currently locked amount (credit pool) is available through rpc call
`getblock`.
## How Has This Been Tested?
There added new unit tests for basic checks of transaction validity
(asset lock/unlock).
Also added new functional test "feature_asset_locks.py" that cover
typical cases, but not all corner cases yet.
## Breaking Changes
This feature should be activated as hard-fork because:
- It adds 2 new special transaction and one of them [asset unlock tx]
requires update consensus rulels
- It adds new data in coinbase tx (credit pool)
## 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
**To release DIP 0027**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
0956e46bff7f0b6da65a4de6d4f8261fe9d7055c test: use zero-argument super() shortcut (Python 3.0+) (Sebastian Falbesoner)
Pull request description:
This mini-PR replaces all calls to `super(...)` with arguments with the zero-argument shortcut `super()` where applicable. See [PEP 3135](https://www.python.org/dev/peps/pep-3135/#specification):
> The new syntax:
>
> super()
>
> is equivalent to:
>
> super(__class__, <firstarg>)
>
> where __class__ is the class that the method was defined in, and <firstarg> is
> the first parameter of the method (normally self for instance methods, and cls
> for class methods).
ACKs for top commit:
fanquake:
ACK 0956e46bff7f0b6da65a4de6d4f8261fe9d7055c
Tree-SHA512: 4ac66fe7ab2be2e8a514e5fcfc41dbb298f21b23ebb7b7b0310d704b0b3cef8adf287a8d80346d1ea9418998c597b4f0ff1f66148d0d806bb43db6607e0fe1cf
## Issue being fixed or feature implemented
Implementation of Randomness Beacon Part 3.
Starting from v20 activation fork, members for quorums are sorted using
(if available) the best CL signature found in Coinbase.
If no CL signature is present yet, then the usual way is used (By using
Blockhash instead)
The actual new way to shuffle is already implemented in
https://github.com/dashpay/dash/pull/5366.
SPV clients also need to calculate members, but they only know block
headers.
Since Coinbase is in the actual block, then they lack the required
information to correctly calculate quorum members.
## What was done?
- Message `MNLISTIDFF` is enriched with a new field `quorumsCLSigs`.
This field holds the Chainlock Signature required for each set of
indexes corresponding to quorums in field `newQuorums`.
- Protocol version has been bumped to `70230`.
- Clients with protocol version greater or equal to `70230` will receive
the new field `quorumsCLSigs`.
- The same field is returned in `protx diff` RPC.
Note:
- Field `quorumsCLSigs` will populated only after v20 activation
- If for one or more quorums, no non-null CL sig was found in CbTx then
a null signature is returned in `quorumsCLSigs`.
## How Has This Been Tested?
- Functional test mininode's protocol version was bumped to `70230`.
- `feature_llmq_rotation.py` checks that `quorumsCLSigs` match in both
P2P and RPC messages.
## Breaking Changes
No
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
## Issue being fixed or feature implemented
Version field should always be the first field of a message for better
readibility.
## What was done?
- Introduced new protocol version `MNLISTDIFF_VERSION_ORDER` (`70229`).
- `nVersion` serialisation order is changed for clients with protocol
version greater than or equal to `70229`.
- For clients with protocol version >= `70225` and < `70229` the old
order is used: can be deprecated in the future.
- Increased functional test P2P mininode's protocol version to `70229`.
## How Has This Been Tested?
`feature_llmq_rotation.py` with new protocol version.
## 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
- [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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Mobile wallets would have to convert 4k+ pubkeys at the V19 fork point
and it's a pretty hard job for them that can easily take 10-15 seconds
if not more. Also after the HF, if a masternode list is requested from
before the HF, the operator keys come in basic scheme, but the
merkelroot was calculated with legacy. From mobile team work it wasn't
possible to convert all operator keys to legacy and then calculate the
correct merkleroot.
~This PR builds on top of ~#5392~ #5403 (changes that belong to this PR:
26f7e966500bdea4c604f1d16716b40b366fc707 and
4b42dc8fcee3354afd82ce7e3a72ebe1659f5f22) and aims to solve both of
these issues.~
cc @hashengineering @QuantumExplorer
## What was done?
Introduce `nVersion` on p2p level for every CSimplifiedMNListEntry. Set
`nVersion` to the same value we have it in CDeterministicMNState i.e.
pubkey serialization would not be via basic scheme only after the V19
fork, it would match the way it’s serialized on-chain/in
CDeterministicMNState for that specific MN.
## How Has This Been Tested?
run tests
## Breaking Changes
NOTE: `testnet` is going to re-fork at v19 forkpoint because
`merkleRootMNList` is not going to match
## 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
- [ ] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
fix a couple of issues in helpers, extend feature_dip3_v19.py to check
more after v19 fork
## What was done?
pls see individual PRs
## 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
Currently, Chainlocks are either enabled or disabled. This PR adds a
third state: enabled but we will not sign new ones.
Should probably backport this to v19.x
## What was done?
Spork state != 0 but active will now result in chain locks being
enforced but not created.
## How Has This Been Tested?
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] 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)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
0ed2d8e07d3806d78d03a77d2153f22f9d733a07 test: add BIP37 remote crash bug [CVE-2013-5700] test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the missing message type `filteradd` to the test framework and checks that the BIP37 implementation is not vulnerable to the "remote crash bug" [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700) anymore. Prior to v.0.8.4, it was possible to trigger a division-by-zero error on the following line in the function `CBloomFilter::Hash()`:
f0d6487e29/src/bloom.cpp (L45)
By setting a zero-length filter via `filterload`, `vData.size()` is 0, so the modulo operation above, called on any .insert() or .contains() operation then crashed the node. The test uses the approach of just sending an arbitrary `filteradd` message after, which calls `CBloomFilter::insert()` (and in turn `CBloomFilter::Hash()`) on the node. The vulnerability was fixed by commit 37c6389c5a (an intentional covert fix, [according to gmaxwell](https://github.com/bitcoin/bitcoin/issues/18483#issuecomment-608224095)), which introduced flags `isEmpty`/`isFull` that wouldn't call the `Hash()` member function if `isFull` is true (set to true by default constructor).
To validate that the test fails if the implementation is vulnerable, one can simply set the flags to false in the member function `UpdateEmptyFull()` (that is called after a filter received via `filterload` is constructed), which activates the vulnerable code path calling `Hash` in any case on adding or testing for data in the filter:
```diff
diff --git a/src/bloom.cpp b/src/bloom.cpp
index bd6069b..ef294a3 100644
--- a/src/bloom.cpp
+++ b/src/bloom.cpp
@@ -199,8 +199,8 @@ void CBloomFilter::UpdateEmptyFull()
full &= vData[i] == 0xff;
empty &= vData[i] == 0;
}
- isFull = full;
- isEmpty = empty;
+ isFull = false;
+ isEmpty = false;
}
```
Resulting in:
```
$ ./p2p_filter.py
[...]
2020-04-03T14:38:59.593000Z TestFramework (INFO): Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed
2020-04-03T14:38:59.695000Z TestFramework (ERROR): Assertion failed
[...]
[... some exceptions following ...]
```
ACKs for top commit:
naumenkogs:
utACK 0ed2d8e07d3806d78d03a77d2153f22f9d733a07
Tree-SHA512: 02d0253d13eab70c4bd007b0750c56a5a92d05d419d53033523eeb3ed80318bc95196ab90f7745ea3ac9ebae7caee3adbf2a055a40a4124e0915226e49018fe8
00559229588feb19de2a0cb7506f70c483a1f433 test: add BIP37 'filterclear' test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the message type `filterclear` to the test framework and adds a simple test to `p2p_filter.py`, checking that arbitrary txs get relayed again after deleting the filter.
ACKs for top commit:
naumenkogs:
utACK 00559229588feb19de2a0cb7506f70c483a1f433
Tree-SHA512: fe64e99a526865770707d8077b9968d3923f248045ec7fa56cd380dba85ac77a71a473d244ef3aede2fc0d287b8d7c6bc0156b6033b0c949c2058cc08e255697
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
## Issue being fixed or feature implemented
Speed thing up: 8075fc0c61
Unify things: ff1a390224 (and _probably_
fix issues like https://gitlab.com/dashpay/dash/-/jobs/4304343867),
876f5c3a9f,
ed58cdda13
Let tsan tests finish on smaller/slower machines:
ba1e3360f9
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests locally and my in gitlab ci
https://gitlab.com/UdjinM6/dash/-/jobs/4319419014
## 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
- [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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
I think the logic in activate_by_name is broken
## What was done?
fix it
## 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
## What was done?
- Bumped version of `CbTx`. Added fields `bestCLHeightDiff`,
`bestCLSignature`
- Miner starting from v20 fork, includes best CL signature in `CbTx` (if
available) or null signature.
- All nodes should verify included CL signature before accepting the
block.
## How Has This Been Tested?
Basically, activated v20 on in the beginning of
`feature_llmq_chainlocks.py`
## Breaking Changes
Yes, new version of CbTx
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [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
Functional tests in CI and locally often fails without a good reason
(pretty randomly)
## What was done?
It was re-implemented `get_recovered_sig` and updated `create_raw_tx`
for better selection/change output.
## How Has This Been Tested?
Run functional times with bug amount of parrallel jobs:
```
test/functional/test_runner.py -j 20 feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py
```
Without these changes usually 2-3 instance fails.
With these changes all failures happened only for `p2p_addrv2_relay.py`
and `mempool_unbroadcast.py`. Beside feature_llmq_is_conflicts.py
improved stability of `interface_zmq_dash.py` also.
## Breaking Changes
No 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
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery)
8319e738f9f118025b332e4fa804d4c31e4113f4 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne)
0328dcdcfcb56dc8918697716d7686be048ad0b3 [Consensus] Bury segwit deployment (John Newbery)
1c93b9b31c2ab7358f9d55f52dd46340397c906d [Consensus] Bury CSV deployment height (John Newbery)
3862e473f0cb71a762c0306b171b591341d58142 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery)
Pull request description:
This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
ACKs for top commit:
ajtowns:
ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 ; checked diff to previous acked commit, checked tests still work
ariard:
ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
MarcoFalke:
ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 (still didn't check if the mainnet block heights are correct, but the code looks good now)
Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912
8925df86c4df16b1070343fef8e4d238f3cc3bd1 doc: update release notes (Jon Atack)
8bb405bbadf11391ccba7b334b4cfe66dc85b390 test: getaddressinfo labels purpose deprecation test (Jon Atack)
60aba1f2f11529add115d963d05599130288ae28 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack)
7851f14ccf2bcd1e9b2ad48e5e08881be06d9d21 rpc: incorporate review feedback from PR 17283 (Jon Atack)
Pull request description:
This PR builds on #17283 (now merged) and is followed by #17585.
It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs.
before
```
"labels": [
{
"name": "DOUBLE SPEND",
"purpose": "receive"
}
```
after
```
"labels": [
"DOUBLE SPEND"
]
```
The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`.
For context, see:
- https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427)
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output.
Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those.
ACKs for top commit:
jnewbery:
reACK 8925df8
promag:
Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1.
meshcollider:
Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1
Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151 test: add rpc getaddressinfo labels test coverage (Jon Atack)
0f3539ac6d772fc646b5f184fa1efe77bf632f6a test: add listlabels test in wallet_labels.py (Jon Atack)
1388de83900eaced906d369fe9e8887ae74b2dcf rpc: add getaddressinfo code documentation (Jon Atack)
2ee0cb3330ccf70f0540cb42370796e32eff1569 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack)
8d1ed0c263f8cdff7189f02040b5d02238d93da0 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack)
5a0ed850700dfb19167d40b38f80313bd5e427ca rpc: improve getaddressinfo RPCHelpman content (Jon Atack)
70cda342cd20d0e0cd9f28405457544036968f2d rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack)
Pull request description:
This PR is a continuation of the work in https://github.com/bitcoin/bitcoin/pull/12892.
Main motivations:
- There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address.
- `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers.
Changes by order of commits:
- [x] improve/fix getaddressinfo RPCHelpman layout formatting
- [x] improve/fix getaddressinfo RPCHelpman content
- [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman
- [x] update getaddressinfo RPCExamples addresses to bech32
- [x] add getaddressinfo code docs
- [x] add a `listlabels` test assertion in wallet_labels.py
- [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests
Here are gists of the CLI help output:
[`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810)
[`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba)
It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._
ACKs for top commit:
fjahr:
Re-ACK 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151
jnewbery:
ACK 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151.
Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
----
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
This was reported/requested by @HashEngineering:
> Older versions of our App won't sync due to if (obj.nVersion ==
BASIC_BLS_VERSION) . Older versions don't know what version a SML Entry
is. As such, they will never read the type field. On the android client
this causes an offset problem when reading the mnlistdiff and it will
throw an exception that bans the peer that supplied it. Soon enough, no
peers will be left to connect to because they will all give the android
client bad data.
## What was done?
With this PR, SML will serialise the new v19 fields (`nType`,
`platformHTTPPort`, `platformNodeID`) if the client's version is at
least equal to `70227`.
Note: Serialisation for hashing skips the above rule.
Also, functional test mininode protocol version is set to `70227`.
## 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
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
should hopefully fix some sporadic ci test failures (like
https://gitlab.com/dashpay/dash/-/jobs/4052206622#L1962)
## What was done?
tweaked dynamically_add/update functions to make checks more consistent
and avoid some edge cases, pls see individual commits
## How Has This Been Tested?
`feature_llmq_hpmn.py` and `feature_dip3_v19.py` still work locally,
let's see if ci is now (constantly) happy about these too...
## 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
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
3ebde2143aa98af213872b98b474d904e55056f7 [test] Fix wait condition in disconnect_p2ps (Amiti Uttarwar)
Pull request description:
#19315 currently has a [test failure](https://cirrus-ci.com/task/4545582645641216) because of a race. `disconnect_p2ps` is intended to have a `wait_until` clause that prevents this race, but the conditional doesn't match since its comparing two different object types. `MY_SUBVERSION` is defined in messages.py as a byte string, but is compared to the value returned by the RPC. This PR simply converts types to ensure they match, which should prevent the race from occurring.
HUGE PROPS TO jnewbery for discovering the issue 🔎
ACKs for top commit:
jnewbery:
ACK 3ebde2143aa98af213872b98b474d904e55056f7
glozow:
Code review ACK 3ebde2143a
Tree-SHA512: ca096b80a3e4d757a645f38846d6dc89d6a3d35c3435513a72d278e305faddd4aff9e75a767941b51b2abbf59c82679bac1e9a0140d6f285efe3053e51bcc2a8
638441928a446726ce3a7fb20433a5478e7585bb test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner)
Pull request description:
While working on the test for #19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore.
ACKs for top commit:
guggero:
Code review ACK 638441928a446726ce3a7fb20433a5478e7585bb.
epson121:
Code review ACK 638441928a446726ce3a7fb20433a5478e7585bb
Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d
854382885f18aa9a95cdde3d11591b05c305ad3f refactor: test: improve wait_for{header,merkleblock} interface (Sebastian Falbesoner)
1356a45ef042e7bd3d539fbb606d6b1be547d00f test: complete impl. of msg_merkleblock and wait_for_merkleblock (Sebastian Falbesoner)
Pull request description:
Implements the missing initialization/serialization methods for `msg_merkleblock`, based on the already present class `CMerkleBlock`. Also changes the method `wait_for_merkleblock()` to be more precise by waiting for a merkleblock with a specified blockhash instead of an arbitrary one.
In the BIP37 test `p2p_filter.py`, this new method is used to make the test of receiving merkleblock and tx if a filter is set to be more precise, by checking if they also arrive in the right order.
In the course of this PR, also the interface for the methods `wait_for_merkleblock()` and `wait_for_header()` are improved to take a hex string instead of an integer, which is more typesafe and less of a burden to the caller.
ACKs for top commit:
MarcoFalke:
ACK 854382885f18aa9a95cdde3d11591b05c305ad3f
Tree-SHA512: adaf0ac728ef0b9929cb417a7a7b4c1346c400b2d365bf6914515c67b6cfe8f4a7ecc62fb514afdce9792f0bed833416f6bca6b9620f3d5dcdc66e4d5b0b7ea3
## Issue being fixed or feature implemented
1. we need to move time forward to let invs being relayed
2. nNextInvSend in SendMessages can be bumped up to 30+ seconds into the
future in rare cases
make sure timeouts in tests are high enough to relay tx inv/wait for
corresponding islock
## What was done?
tl;dr: bump mocktime while waiting, wait longer
extracted fixes from https://github.com/dashpay/dash/pull/5288 but I
expect this to fix other sporadic test failures too
## How Has This Been Tested?
tests are ok locally and in https://github.com/dashpay/dash/pull/5288
## 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
**For repository code-owners and collaborators only**
- [ ] I have assigned this pull request to a milestone
19a354b11f85a3c6c81ff83bf702bf7a40cf5046 Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
MarcoFalke:
ACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
promag:
Code review ACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046.
meshcollider:
utACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd86715039586d92176eee16e9c6644d2547f0 Avoid using mutable default parameter values (practicalswift)
Pull request description:
Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.
Examples of this gotcha caught during review:
* https://github.com/bitcoin/bitcoin/pull/16673#discussion_r317415261
* https://github.com/bitcoin/bitcoin/pull/14565#discussion_r241942304
Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:
```
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})
```
In contrast to:
```
>>> def f(i, j=None, k=None):
... if j is None:
... j = []
... if k is None:
... k = {}
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})
```
The latter is typically the intended behaviour.
This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)
ACKs for top commit:
Sjors:
Oh Python... ACK e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1. Testing tip: swap the two commits.
Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
afc0966d725aeeb8842dc264bd48f0e9c41f6a34 Moved and renamed hash256 from util.py to zmq_interface.py (Elichai Turkel)
Pull request description:
Right now there are two `hash256(bytes)` in the test framework:
first: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L186
second: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/messages.py#L60
While they have the same name they're actually doing different things, one just does a sha256d and the other sha256d and reverses the bytes.
so I renamed the second one to be `hash256r` to signify that it's hash256 reversed.
ACKs for top commit:
MarcoFalke:
unsigned ACK afc0966d725aeeb8842dc264bd48f0e9c41f6a34
fanquake:
ACK afc0966d725aeeb8842dc264bd48f0e9c41f6a34
Tree-SHA512: fb0e2db6f09c0248d92f2fd72d05a78cec1bebb44449239dbeecefa62cf4bd01d180b2e6dbcee48a8a9cea79a909e224256cabdd0739f334c2943647fe0c5fe4
8a22fd01140bd957036fc00419b147e4268ae9b1 avoided os-dependant path (Ferdinando M. Ametrano)
Pull request description:
The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust
ACKs for top commit:
MarcoFalke:
ACK 8a22fd01140bd957036fc00419b147e4268ae9b1
Tree-SHA512: 813f27aea33f97c8afac52e716a55fc5d7fb69621023aba99d40df7e1d145e0ec8d1eee49ddd403b219bf0e0e168e0e987b05c78eaef611f744d99bf2fc8bc91
## Issue being fixed or feature implemented
fix `p2p_quorum_data.py` test broken by #5276
## What was done?
adjust data request expiration timeout in tests
## How Has This Been Tested?
`./test/functional/test_runner.py p2p_quorum_data.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
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
<!--
*** Please remove the following help text before submitting: ***
Provide a general summary of your changes in the Title above
Pull requests without a rationale and clear improvement may be closed
immediately.
Please provide clear motivation for your patch and explain how it
improves
Dash Core user experience or Dash Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always
welcome.
* All other changes should have accompanying unit tests (see
`src/test/`) or
functional tests (see `test/`). Contributors should note which tests
cover
modified code. If no tests exist for a region of modified code, new
tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or
an
explanation of the potential issue as well as reasoning for the way the
bug
was fixed.
* Features are welcome, but might be rejected due to design or scope
issues.
If a feature is based on a lot of dependencies, contributors should
first
consider building the system outside of Dash Core, if possible.
-->
## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Before this fix, uniqueness of HPMN `platformNodeID` was checked only
while processing a block containing a `ProRegTx` or a `ProUpServTx`.
This is not enough as a `ProRegTx` or `ProUpServTx` containing duplicate
HPMN `platformNodeID` must be rejected at tx broadcast level.
## What was done?
<!--- Describe your changes in detail -->
Checking uniqueness when calling respective RPC and when receiving such
txs.
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
## Breaking Changes
<!--- Please describe any breaking changes your code introduces -->
## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
faff9e4bb431919a4bc7e4dc4a9ca188e2d18113 test: Remove unused, undocumented and misleading CScript.__add__ (MarcoFalke)
Pull request description:
See the corresponding pull #18612
ACKs for top commit:
laanwj:
ACK faff9e4bb431919a4bc7e4dc4a9ca188e2d18113 provided it passes Travis
Tree-SHA512: 5d9c4d5b6453c70b24a6960d3b42834e9b31f6dbb99ac47a6abfd85f2739d5372563e7188c22aceabeee1c37eb218bf580848356f4a77268d65f178a9419b269
9e1cb1adf1800efe429e348650931f2669b0d2c0 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67166a6ab7c0f33f7ec1990d3c31761e [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f29c63d57af05bfbdd6035bb9c965de2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5833a5c5fc735ef00c0a80f5fab7a2c [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c744a103b633c1051e8fbc01e612097dc [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb0ec73f10b196e79b637aa601c0a6b7 [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba5498318233ab81decbc585e9619d8ffe2df1b0 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b4e5ae249b8011360c6b0f7dc731581 [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15de762fdaf0937a0877d17b0c2bce16e [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab5a5a41685f437db9081fa7b395fa73 [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
651f1d816f054cb9c637f8a99c9360bba381ef58 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb9860254eb787ebe2734fd6a26bcf365c1 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94c5a9195c8eabd193204c499cb4bfdb [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d16006960443c2efe37c896e46edae9dca86c57d [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
amitiuttarwar:
ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 🎉
MarcoFalke:
Review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
9a40cfc558b3f7fa4fff1270f969582af17479a5 [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb414e2d000830287d9dd3fed7fc2eb570d2 [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d2e1288367e8da94adb2b2a88be99e4751 [test] logging and style followups for bloomfilter tests (gzhao408)
Pull request description:
Followup to #19083 which adds bloomfilter-related tests.
1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests.
2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784)
ACKs for top commit:
jonatack:
Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
MarcoFalke:
ACK 9a40cfc558b3f7fa4fff1270f969582af17479a5 🐂
Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87
fa156999695ddaeb016d8320bee62f8d96679d55 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa156999695ddaeb016d8320bee62f8d96679d55 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
7daffc6a90a797ce7c365a893a31a31b0206985c [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90a797ce7c365a893a31a31b0206985c
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
## Issue being fixed or feature implemented
## What was done?
1. Increased protocol version of mininode to match v19 changes in
`MNLISTDIFF` P2P message
2. Added verification of MNs and HPMNs (dip4) in `feature_llmq_hpmn.py`
## 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
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
## What was done?
Implementation of 4k collateral HPMN.
## 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
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
Co-authored-by: PastaPastaPasta <6443210+pastapastapasta@users.noreply.github.com>
Co-authored-by: UdjinM6 <1935069+Udjinm6@users.noreply.github.com>
Co-authored-by: Konstantin Akimov <545784+knst@users.noreply.github.com>
b651ef7e1c submitheader: more directly test missing prev block header (Gregory Sanders)
1e7f741745 remove some magic mining constants in functional tests (Gregory Sanders)
Pull request description:
The fewer magic numbers the better.
Also more directly tested a `submitheader` case of bad previous blockhash.
Tree-SHA512: 52b01a6aa199fa909eea4e9e84409a901933e545724e33149cc4132c82168199fd678809b6d94d95c9ff6ad02238a9552363620d13b8beaa5d4b67ade9ef425c
