fa80e10d94dbf86da84fc761b09fb631155a5b25 test: Add feature_taproot.py --previous_release (MarcoFalke)
85ccffa26686c6c9adbd18bdde37fc1747281bab test: move releases download incantation to README (Sjors Provoost)
29d6b1da2a862bfbb14e7821979c97416c5400e8 test: previous releases: add v0.20.1 (Sjors Provoost)
Pull request description:
Disabling the new consensus code at runtime is fine, but potentially fragile and incomplete. Fix that by giving the option to run with a version that has been compiled without any taproot code.
ACKs for top commit:
Sjors:
tACK fa80e10
NelsonGaldeman:
tACK fa80e10d94dbf86da84fc761b09fb631155a5b25
Tree-SHA512: 1a1feef823f08c05268759645a8974e1b2d39a024258f5e6acecbe25097aae3fa9302c27262978b40f1aa8e7b525b60c0047199010f2a5d6017dd6434b4066f0
179a051704321ba40277a5855d6ac0dbb45689dd util: improves error messages on get_previous_releases script (Nelson Galdeman)
Pull request description:
When previous releases are fetched and the specified version wasn't added to the checksum list we used to get a "Checksum did not match" which isn't true (https://github.com/bitcoin-core/bitcoincore.org/issues/753#issuecomment-879546719).
If the specified version number is not on the list, it now logs cannot do the comparison instead.
ACKs for top commit:
practicalswift:
cr ACK 179a051704321ba40277a5855d6ac0dbb45689dd
theStack:
tACK 179a051704321ba40277a5855d6ac0dbb45689dd, tested on Debian bullseye/sid
Tree-SHA512: 2a07ce75232f853fd311c43581f8faf12d423668946ae6ad784feece5b4d0edd57fc018ba1f0c5a73bfaccb326e0df9a643580d16bf427c1ec3ff34a9cdbc80c
fa1d5e51374bce9607b429836bbd3fe059e3c78c test: Fix get_previous_releases.py for aarch64 (MarcoFalke)
Pull request description:
Otherwise it will fail with "Not sure which binary to download..."
ACKs for top commit:
laanwj:
Code review ACK fa1d5e51374bce9607b429836bbd3fe059e3c78c
Tree-SHA512: 0db71e898a431665757ce835016a4e05c629a95abc4a2951eac9bd9b5876ec3dc3d6f156d58565e2bcdf918cde4f2649183d4a58038ac13c705a7e914c0094d1
6de942908726480fb2919ed1f1b7906a63ec576d qa: Changes v0.17.1 to v0.17.2 (nthumann)
Pull request description:
As of 0374e821bd v0.17.2 is downloaded instead of v0.17.1 for functional testing. This causes `test/functional/feature_backwards_compatibility.py` to fail, because it [requires](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_backwards_compatibility.py#L57) v0.17.1.
Steps to reproduce:
Run `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2`. It cannot be downloaded at all because the sha256sum is missing [here](c1e0c2ad3b/test/get_previous_releases.py (L23)).
Or adjust the command and run `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`, then run `test/functional/test_runner.py feature_backwards_compatibility`. It´ll fail because the test is missing v0.17.1.
This PR changes v0.17.1 to v0.17.2 in this test and in a few comments.
ACKs for top commit:
laanwj:
ACK 6de942908726480fb2919ed1f1b7906a63ec576d
fanquake:
ACK 6de942908726480fb2919ed1f1b7906a63ec576d - looks correct. Surprised this wasn't caught/part of #19813. In future you could add any explanations & extra info as part of your commit message as well (even though PR descriptions are included as part of the merge).
Tree-SHA512: bbe50c4fd5c1aedd6dc1cdc3d93ef9005db1c67adca3f263b6b0d869c40b495a3221e706c9389fedea4748e31911dbd591062f60ce9836e58099fbdd9515b4d9
0374e821bd9e9498ce9c03aa8e5435870019978b util: Hard code previous release tarball checksums (Hennadii Stepanov)
bd897ce79f72a44a2e609f95433e251a3fd9eb9c scripted-diff: Move previous_release.py to test/get_previous_releases.py (Hennadii Stepanov)
Pull request description:
#19205 introduced signature verifying for the downloaded `SHA256SUMS.asc`.
This approach is brittle and does not work in CI environment for many reasons:
- https://github.com/bitcoin/bitcoin/issues/19812#issuecomment-680760663
- https://github.com/bitcoin/bitcoin/pull/19013#discussion_r459590779
This PR:
- implements **Sjors**' [idea](https://github.com/bitcoin/bitcoin/pull/19205#pullrequestreview-426080048):
> Alternatively we might as well hard code the checksum for each `tar.gz` release in the source code, here.
- is an alternative to 5a2c31e528e6bd60635096f233252f3c717f366d (#19013)
- fixes#19812
- updates v0.17.1 to v0.17.2
ACKs for top commit:
MarcoFalke:
cr ACK 0374e821bd9e9498ce9c03aa8e5435870019978b
Sjors:
tACK 0374e821bd9e9498ce9c03aa8e5435870019978b
Tree-SHA512: cacdcf9f5209eae7da357abb3445585ad2f980920fd5bf75527ce89974d3f531a4cf8b5b35edfc116b23bfdfb45c0437cb14cbc416d76ed2dc5b9e6d33cdad71
d135c294764add81683ba47575f9a5dde7d7c07f [ci] make list of previous releases to download a setting (Sjors Provoost)
9c246b873c74834a121edba00fcaecf0cba6f9b4 [test] backwards compatibility: bump v0.19.0.1 to v0.19.1 (Sjors Provoost)
89a28e02fa46f3d5eb07ab02aa34aa95c6fcee11 [test] add v0.16.3 backwards compatibility test (Sjors Provoost)
Pull request description:
Thanks to #18774's `adjust_bitcoin_conf_for_pre_17` we can now test backwards compatibility for v0.16.3, both for sync and loading a recent wallet.
This PR bumps v0.19.0.1 to v0.19.1.
I also made the version list consistent for the `contrib/devtools/previous_release.sh` instruction, between both tests.
ACKs for top commit:
MarcoFalke:
ACK d135c294764add81683ba47575f9a5dde7d7c07f
Tree-SHA512: 5ff137a7a934237fa220f1c2807ce9abeeb75929266558bf3e4045bec7dfcd0a8747fa74d700065c568330b18badf58c60c308eb13d1eed444d4bbfe6decc48b
16d4b3fd6d5aad18ebb731a5006a15180d3661ef test: mempool.dat compatibility between versions (Ivan Metlushko)
Pull request description:
Rationale: Verify mempool.dat compatibility between versions
The format of mempool.dat has been changed in #18038
The tests verifies the fix made in #18807 and ensures that the file format is compatible between current version and v0.19.1
The test verifies both backward and forward compatibility.
This PR also adds a log when we fail to add a tx loaded from mempool.dat.
It was useful when debugging this test and could be potentially useful to debug other scenarios as well.
Closes#19037
ACKs for top commit:
Sjors:
tACK 16d4b3fd6d5aad18ebb731a5006a15180d3661ef
Tree-SHA512: 00a38bf528c6478cb0da467af216488f83c1e3ca4d9166c109202ea8284023e99d87a3d6e252c4d88d08d9b5ed1a730b3e1970d6e5c0aef526fa7ced40de7490
c0c43ae1471347ea93614e9a25989f13b021f8a8 test: skip backwards compat tests if not compiled with wallet (fanquake)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: d9975a1490e69134408b6b724cea26a6c1397d43f59850283b9e338ae38e00fefbcd868fb141e0a4bb55f02076690a99331f29cfa2d0fa66c165032b24a94081
c456145b2c65f580683df03bf10cd39000cf24d5 [test] add 0.19 backwards compatibility tests (Sjors Provoost)
b769cd142deda74fe46e231cc7b687a86514f2f1 [test] add v0.17.1 wallet upgrade test (Sjors Provoost)
9d9390dab716f07057c94e8e21f3c7dd06192f35 [tests] add wallet backwards compatility tests (Sjors Provoost)
c7ca6308968b29a0e0edc485cd06e68e5edb7c7d [scripts] support release candidates of earlier releases (Sjors Provoost)
8b1460dbd1b732f06d4cebe1fa6844286c7a0056 [tests] check v0.17.1 and v0.18.1 backwards compatibility (Sjors Provoost)
ae379cf7d12943fc192d58176673bcfe7d53da53 [scripts] build earlier releases (Sjors Provoost)
Pull request description:
This PR adds binaries for 0.17, 0.18 and 0.19 to Travis and runs a basic block propagation test.
Includes test for upgrading v0.17.1 wallets and opening master wallets with older versions.
Usage:
```sh
contrib/devtools/previous_release.sh -f -b v0.19.0.1 v0.18.1 v0.17.1
test/functional/backwards_compatibility.py
```
Travis caches these earlier releases, so it should be able to run these tests with little performance impact.
Additional scenarios where it might be useful to run tests against earlier releases:
* creating a wallet with #11403's segwit implementation, copying it to an older node and making sure the user didn't lose any funds (although this PR doesn't support `v0.15.1`)
* future consensus changes
* P2P changes (e.g. to make sure we don't accidentally ban old nodes)
ACKs for top commit:
MarcoFalke:
ACK c456145b2c65f580683df03bf10cd39000cf24d5 🔨
Tree-SHA512: 360bd870603f95b14dc0cd629532cc147344f632b808617c18e1b585dfb1f082b401e5d493a48196b719e0aeaee533ae0a773dfc9f217f704aae898576c19232
## Motivation
CoinJoin's subsystems are initialized by variables and managers that
occupy the global context. The _extent_ to which these subsystems
entrench themselves into the codebase is difficult to assess and moving
them out of the global context forces us to enumerate the subsystems in
the codebase that rely on CoinJoin logic and enumerate the order in
which components are initialized and destroyed.
Keeping this in mind, the scope of this pull request aims to:
* Reduce the amount of CoinJoin-specific entities present in the global
scope
* Make the remaining usage of these entities in the global scope
explicit and easily searchable
## Additional Information
* The initialization of `CCoinJoinClientQueueManager` is dependent on
blocks-only mode being disabled (which can be alternatively interpreted
as enabling the relay of transactions). The same applies to
`CBlockPolicyEstimator`, which `CCoinJoinClientQueueManager` depends.
Therefore, `CCoinJoinClientQueueManager` is only initialized if
transaction relaying is enabled and so is its scheduled maintenance
task. This can be found by looking at `init.cpp`
[here](93f8df1c31/src/init.cpp (L1681-L1683)),
[here](93f8df1c31/src/init.cpp (L2253-L2255))
and
[here](93f8df1c31/src/init.cpp (L2326-L2327)).
For this reason, `CBlockPolicyEstimator` is not a member of `CJContext`
and its usage is fulfilled by passing it as a reference when
initializing the scheduling task.
* `CJClientManager` has not used `CConnman` or `CTxMemPool` as `const`
as existing code that is outside the scope of this PR would cast away
constness, which would be unacceptable. Furthermore, some logical paths
are taken that will grind to a halt if they are stored as `const`.
Examples of such a call chains would be:
* `CJClientManager::DoMaintenance >
CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating >
CCoinJoinClientSession::DoAutomaticDenominating >
CCoinJoinClientSession::StartNewQueue > CConnman::AddPendingMasternode`
which modifies `CConnman::vPendingMasternodes`, which is non-const
behaviour
* `CJClientManager::DoMaintenance >
CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating >
CCoinJoin::IsCollateralValid > AcceptToMemoryPool` which adds a
transaction to the memory pool, which is non-const behaviour
* There were cppcheck [linter
failures](https://github.com/dashpay/dash/pull/5337#issuecomment-1685084688)
that seemed to be caused by the usage of `Assert` in
`coinjoin/client.h`. This seems to be resolved by backporting
[bitcoin#24714](https://github.com/bitcoin/bitcoin/pull/24714). (Thanks
@knst!)
* Depends on #5546
---------
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Move funds from the coinbase, into the Asset Lock Pool. This is to incentivize MNs to upgrade to platform, because only MNs running platform will get these migrated rewards
## 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)_
## What was done?
- remove dependency of Asset Lock txes on CCreditPool
- new case for functional tests of Asset Locks - more than one output
for Asset Lock tx.
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
Slightly changes behaviour of TxMempool. Tx can be accepted in mempool
even if Asset Unlock transaction with same index is already mined. But
final consensus rules are same.
## 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
0306d78cb49d1684cc96ba3512b582a1fdaf78cc Use getbalances in wallet_address_types tests (Jon Atack)
7eacdc5167c8db94df84e206db85817bc64e4921 Shift coverage from getunconfirmedbalance to getbalances in wallet_abandonconflict tests (Jon Atack)
3e6f7377f600e47e5e3d439fc5d6ccf3db210038 Improve getbalances coverage in wallet_balance tests (Jon Atack)
Pull request description:
<strike>This PR updates several tests and then removes the `getunconfirmedbalance` RPC which was deprecated in facfb4111d14a3b06c46690a2cca7ca91cea8a96 a year ago.
Next steps: remove the deprecated `getwalletinfo` fields and the `getbalance` RPC in follow-ups, if there seems to be consensus on those removals.</strike>
Update:
`getunconfirmedbalance` RPC was deprecated in facfb4111d14a3b06c46690a2cca7ca91cea8a96 a year ago, but following the review comments below, this PR now only updates the test coverage to use `getbalances` while still leaving basic coverage for `getunconfirmedbalance` in wallet_balance.py.
That said, I've seen 3 regular contributors confused in the past 10 days by "DEPRECATED" warnings in the code that are not following the deprecation policy in [JSON-RPC-interface.md#versioning](https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning).
ISTM these warnings should either be removed, or the calls deprecated (`-deprecatedrpc`), or the policy updated to describe these warnings as a pre-deprecation practice.
ACKs for top commit:
jnewbery:
utACK 0306d78cb
Tree-SHA512: 692e43e9bed5afa97d905740666e365f0b64e559e1c75a6a398236d9e943894e3477947fc11324f420a6feaffa0c0c1532aa983c50090ca39d06551399e6ddd1
fa4632c41714dfaa699bacc6a947d72668a4deef test: Move boost/stdlib includes last (MarcoFalke)
fa488f131fd4f5bab0d01376c5a5013306f1abcd scripted-diff: Bump copyright headers (MarcoFalke)
fac5c373006a9e4bcbb56843bb85f1aca4d87599 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c41714dfaa699bacc6a947d72668a4deef
jonatack:
ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
fa6c114ae604571435e8c4d25906a8b6d5b9984c test: Add sanitizer suppressions for AMD EPYC CPUs (MarcoFalke)
Pull request description:
Currently the ci system only runs on intel cpus (and some arm devices), but it won't run on CPUs `Using the 'shani(1way,2way)' SHA256 implementation` (excerpt from debug log).
For reference, google cloud CPUs (which is what Cirrus CI uses) print `Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation`
The traceback I got:
```
crypto/sha256_shani.cpp:87:18: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
#0 0x55c0000e95ec in sha256_shani::Transform(unsigned int*, unsigned char const*, unsigned long) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256_shani.cpp:87:18
#1 0x55bfffb926f8 in (anonymous namespace)::SelfTest() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:517:9
#2 0x55bfffb906ed in SHA256AutoDetect[abi:cxx11]() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:626:5
#3 0x55bfff87ab97 in BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:104:5
#4 0x55bffe885877 in main /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_main.cpp:52:27
#5 0x7f20c3bf60b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
#6 0x55bffe7a5f6d in _start (/root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x1d00f6d)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow crypto/sha256_shani.cpp:87:18 in
ACKs for top commit:
laanwj:
Anyhow ACK fa6c114ae604571435e8c4d25906a8b6d5b9984c
Tree-SHA512: 968a1d28eedec58c337b1323862f583cb1bcd78c5f03396940b9ab53ded12f8c6652877909aba05ee5586532137418fd817ff979bd7bef6e07856094f9d7f9b1
## 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>
590bda79e876d9b959083105b8c7c41dd87706eb scripted-diff: Remove setup_clean_chain if default is not changed (Fabian Jahr)
98892f39e3d079c73bff7f2a5d5420fa95270497 doc: Improve setup_clean_chain documentation (Fabian Jahr)
Pull request description:
The first commit improves documentation on setup_clean_chain which is misunderstood quite frequently. Most importantly it fixes the TestShell docs which are simply incorrect.
The second commit removes the instances of `setup_clean_clain` in functional tests where it is not changing the default.
This used to be part of #19168 which also sought to rename`setup_clean_chain`.
ACKs for top commit:
jonatack:
ACK 590bda79e876d9b959083105b8c7c41dd87706eb
Tree-SHA512: a7881186e65d31160b8f84107fb185973b37c6e50f190a85c6e2906a13a7472bb4efa9440bd37fe0a9ac5cd2d1e8559870a7e4380632d9a249eca8980b945f3e
e6fe1c37d0a2f8037996dd80619d6c23ec028729 rpc: Improve avoidpartialspends and avoid_reuse documentation (Fabian Jahr)
8f073076b102b77897e5a025ae555baae3d1f671 wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 (Fabian Jahr)
Pull request description:
Follow-up to #17824.
This increases OUTPUT_GROUP_MAX_ENTRIES to 100 which means that OutputGroups will now be up to 100 outputs large, up from previously 10. The main motivation for this change is that during the PR review club on #17824 [several participants signaled](https://bitcoincore.reviews/17824.html#l-339) that 100 might be a better value here.
I think fees should be manageable for users but more importantly, users should know what they can expect when using the wallet with this configuration, so I also tried to clarify the documentation on `-avoidpartialspends` and `avoid_reuse` a bit. If there are other additional ways how or docs where users can be made aware of the potential consequences of using these parameters, please let me know. Another small upside is that [there seem to be a high number of batching transactions with 100 and 200 inputs](https://miro.medium.com/max/3628/1*sZ5eaBSbsJsHx-J9iztq2g.png)([source](https://medium.com/@hasufly/an-analysis-of-batching-in-bitcoin-9bdf81a394e0)) giving these transactions a bit of a larger anonymity set, although that is probably a very weak argument.
ACKs for top commit:
jnewbery:
ACK e6fe1c37d0
Xekyo:
retACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
rajarshimaitra:
tACK `e6fe1c3`
achow101:
ACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
glozow:
code review ACK e6fe1c37d0
Tree-SHA512: 79685c58bafa64ed8303b0ecd616fce50fc9a2b758aa79833e4ad9f15760e09ab60c007bc16ab4cbc4222e644cfd154f1fa494b0f3a5d86faede7af33a6f2826
fad0ae6bb8e10b5cb82a5ec014e59b5aafc85b5e doc: Rename fuzz seed_dir to corpus_dir (MarcoFalke)
Pull request description:
The fuzz corpus directory might contain hand-crafted seeds, but generally it is a set of test inputs. See also https://github.com/google/fuzzing/blob/master/docs/glossary.md#corpus
ACKs for top commit:
practicalswift:
cr ACK fad0ae6bb8e10b5cb82a5ec014e59b5aafc85b5e: patch looks correct and "why not?" :)
fanquake:
ACK fad0ae6bb8e10b5cb82a5ec014e59b5aafc85b5e - did not test
Tree-SHA512: 38c952feb07aeeeb038b3261a12c824fab9ce5153d75f0ecf6d3f43db4f50998eeb2b14b11b7155f529189c93783fa2c11c81059021a04398c43f3505b31a2d4
ba7e17e073f833eccd4c7c111ae9058c3f123371 rpc, test: document {previous,next}blockhash as optional (Sebastian Falbesoner)
Pull request description:
This PR updates the result help of the following RPCs w.r.t. the `previousblockhash` and `nextblockhash` fields:
- getblockheader
- getblock
Also adds trivial tests on genesis block (should not contain "previousblockhash") and best block (should not contain "nextblockhash").
Top commit has no ACKs.
Tree-SHA512: ef42c5c773fc436e1b4a67be14e2532e800e1e30e45e54a57431c6abb714d2c069c70d40ea4012d549293b823a1973b3f569484b3273679683b28ed40abf46bb
c7437185589926ec8def2af6bede6a407b3d2e4a test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #18628. In addition to the hash-functions limit test introduced with commit fa4c29bc1d, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki):
ad message type `filterload`:
> The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**.
ad message type `filteradd`:
> The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object).
Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit).
Also fixes#18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with #18544) below to also use the more commonly used `assert_debug_log` method.
ACKs for top commit:
MarcoFalke:
ACK c7437185589926ec8def2af6bede6a407b3d2e4a
robot-visions:
ACK c7437185589926ec8def2af6bede6a407b3d2e4a
jonasschnelli:
utACK c7437185589926ec8def2af6bede6a407b3d2e4a. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524
Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be
a9ecbdfcaa15499644d16e9c8ad2c63dfc45b37b test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996b340c19cebab9efb6c89d20fe051ef net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)
Pull request description:
This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:
c0b389b335/src/net.h (L812)
and after a loaded filter is removed again through a `filterclear` message:
c0b389b335/src/net_processing.cpp (L3201)
The behaviour was introduced by commit 37c6389c5a (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).
This default match-everything filter leads to some unintended side-effects:
1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) c0b389b335/src/net_processing.cpp (L1504-L1507)
2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) c0b389b335/src/net_processing.cpp (L3182-L3186)
This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.
Here is a before/after comparison in behaviour:
| code part / scenario | master branch | PR branch |
| --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
| `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload` |
| `filteradd` processing, no filter was loaded | nothing | peer's banscore increases by 100 (i.e. disconnect) |
On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.
ACKs for top commit:
MarcoFalke:
re-ACK a9ecbdfcaa, only change is in test code 🕙
Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
cd543d9193ac1882c1b4a8a84e3ac7356a8b7ce9 test: check misbehavior more independently in p2p_filter.py (Danny Lee)
Pull request description:
This expands on #18672 in two ways:
- Check positive cases (`filterload` accepted, `filteradd` accepted) in addition to the negative cases added in #18672
- Address MarcoFalke 's [suggestion](https://github.com/bitcoin/bitcoin/pull/18672#discussion_r412101752) to successfully load a filter before testing `filteradd`
ACKs for top commit:
theStack:
re-ACK cd543d9193
Tree-SHA512: f82402f6287ccddf08b38b6432d5e2b2b2ef528802a981d04c24bac459022f732d9090d4849d72d3d1eb2c757161dcb18c4c036b6e11dc80114e9cd49f21c3bd
By design we can have more and more and more gaps in indexes list so far as
we can not re-sign expired transaction of asset-unlock. CRangesList is protected from this situation
## Issue being fixed or feature implemented
## What was done?
Renaming of all classes/variables/functions/rpcs from `hpmn` to `evo`.
## How Has This Been Tested?
All unit and func tests are passing.
Sync of Testnet.
## Breaking Changes
All protx RPCs ending with `_hpmn` were converted to `_evo`.
`_hpmn` RPCs are now deprecated.
Although, they can still be enabled by adding `-deprecatedrpc=hpmn`.
## 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
- [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>
## Issue being fixed or feature implemented
Execute command when the best chainlock changes (`%s` in cmd is replaced
by chainlocked block hash). Same as `-blocknotify` but for chainlocks.
Let `-instantsendnotify` replace `%w` with wallet name like
`-walletnotify` does.
## What was done?
## 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
fab46b34f4b13abbb0af276c3fb548f25ccc28bd test: Fix restart node race (MarcoFalke)
Pull request description:
It is not allowed to start a node before it has been fully stopped. Otherwise it could lead to intermittent issues due to access issues (e.g. cookie file https://cirrus-ci.com/task/6409665024098304?command=ci#L4793)
Fix that by waiting for the node to fully stop.
ACKs for top commit:
laanwj:
code review ACK fab46b34f4b13abbb0af276c3fb548f25ccc28bd
Tree-SHA512: 7605cac0573a7b04f05ff110d0131e8940d87f7baf6d698505ed16b363d4d15b1e552c5ffd1a187c8fe5639f7e265c3122734c85283275746e46bd789614fd21
fab48da908f3f81135b9163edf5011d1e5f6ef6e test: Fix intermittent wallet_multiwallet issue with got_loading_error (MarcoFalke)
fa8e15f7b75e35846b86e8627a3612e31eb22dcb test: pep8 wallet_multiwallet.py (MarcoFalke)
Pull request description:
Failing the test after 10 iterations without a loading error is problematic because it may take 11 iterations to get a loading error.
Fix that by running until a loading error occurs, which should happen in almost all runs within the first 10 iterations.
ACKs for top commit:
ryanofsky:
Code review ACK fab48da908f3f81135b9163edf5011d1e5f6ef6e. This seems like a good workaround. I think more ideally think load and unload RPCs would not have racy status reporting (suggested previously https://github.com/bitcoin/bitcoin/pull/19300#pullrequestreview-435362710 and
Tree-SHA512: 6b80b26d916276efe2a01af93bca7dbf71a3e67db9d3deb15175070719bf7d1325a1410d93e74c0316942e388faa2ba185dc9d3759c82d1c73c3c509b9997f05
fa918dd537fea775c19a590e5f9161bf51a5839b test: Use Popen.wait instead of RPC in assert_start_raises_init_error (MarcoFalke)
Pull request description:
Using RPC (`wait_for_rpc_connection`) has several issue:
* It polls in a loop, which might be slow
* It tries to read the RPC cookie file, which might not be present, thus leading to intermittent issues
Fix both by using `Popen.wait`
ACKs for top commit:
laanwj:
Code review ACK ~~faf7b05be9c86ee61c39e5314511fe2410128a6b~~ fa918dd537fea775c19a590e5f9161bf51a5839b
darosior:
ACK fa918dd537fea775c19a590e5f9161bf51a5839b
Tree-SHA512: 5368ad0d0ea2deb0af9582a42667c9290efe8f2705f37a236afc2c7908b04265ab342e2dd356a57156e99389f4a27ab6da9fa7bf9161fb7568240aa005e693b9
## Issue being fixed or feature implemented
Bad naming is noticed in https://github.com/dashpay/dash/pull/5026 by
thephez
## What was done?
Renamed `assetLockedAmount` in CbTx to `creditPoolBalance`
Renamed also some local variables and functions to make it matched also.
## How Has This Been Tested?
Run functional/unit tests - succeed
Called python's rpc binding `node.getblock(block_hash)['cbTx']`:
Got this result:
```
{'version': 3, 'height': 1556, 'merkleRootMNList': '978b2b4d1b884de62799b9eaee75c7812fea59f98f80d5ff9c963b0f0f195e14', 'merkleRootQuorums': 'bc7a34eb114f4e4bf38a11080b5d8ac41bdb36dd41e17467bae23c94ba06b013', 'bestCLHeightDiff': 0, 'bestCLSignature': '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000', 'creditPoolBalance': Decimal('7.00141421')}
```
## Breaking Changes
Renamed `assetLockedAmount` in CbTx to `creditPoolBalance`. @shumkov be
informed
## 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
NOTE: There is slight difference with original backport due to future changes
in bitcoin#19272, bitcoin#19763 - otherwise functional test p2p_addr_relay.py fails
fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde test: Add basic addr relay test (MarcoFalke)
fa1793c1c44a3f75a09f9c636467b8274c541bdd net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003f53708b6d5df1ed4e7f8a7c68aa3ac net: Make addr relay mockable (MarcoFalke)
Pull request description:
As usual:
* Switch to std::chrono time to be type-safe and mockable
* Add basic test that relies on mocktime to add code coverage
ACKs for top commit:
naumenkogs:
utACK fa1da3d
promag:
ACK fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.
Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
fixup - see #19272, #19763
faede1b293354560317b67f0b4e6874dcac6ef41 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (MarcoFalke)
Pull request description:
Should fix issues such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/671910152#L7034
Top commit has no ACKs.
Tree-SHA512: ac659f29c5ec91985c916b734e24911cbf4e2c5c4b1f1891a7e6c2d2511ec285167550fb03848eee4a7a3cbc9f8cdb0c766f4e881d9e44368c7415d007006368
7a2ecf16df938dd95d3130a46082def7a02338eb Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr)
2952c46b923042f2de801f319e03ed5c4c4eb735 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr)
d7092c392e10889cd7a080b3d22ed6446a59b87a QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr)
Pull request description:
Follow-up to #18192, not strictly necessary for 0.20
ACKs for top commit:
MarcoFalke:
re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰
jnewbery:
utACK 7a2ecf16df938dd95d3130a46082def7a02338eb
Tree-SHA512: e0933ee40f705b751697dc27249e1868ed4874254b174ebdd0a7150125d8c818402e66df2371718c7eeb90e67ee2317215fb260aa9b9d7b9b45ee436de2988ff
It's highly unlikely the test will ever deal with chains with >4500
blocks, so only the subset of the subsidy logic that is needed to
validate `gettxoutsetinfo` output has been included
## Issue being fixed or feature implemented
There's one type of output that potentially can be useful for bloom
filter.
It's follow-up for TODO for dashpay/dash#4857.
Asset Lock transactions have:
- standard inputs (covered by regular bloom filter implementation)
- standard outputs (covered by regular bloom filter implementation)
- special outputs that have public key to proof owing this credits on
platform and claiming it.
Asset Unlock transactions have:
- no inputs (no need bloom)
- standard outputs (covered by regular bloom filter implementation)
So far as there's only one special case, let's have this data in the
bloom filter because it can potentially help to show information such as
"Deposit to platform" on mobile clients.
## What was done?
- added special case for Asset Lock transactions for bloom filter
## How Has This Been Tested?
Run unit/functional tests. Doesn't actually tested how bloom filter
works.
## 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
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb854ca827a5a3925394f9e09d29b898 lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct!
MarcoFalke:
review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
fa40168ab3102b9ad850f967a0e7fa22dbfbd0c6 Remove unused bits from service flags enum (MarcoFalke)
Pull request description:
Remove service bits that haven't been observed on the active network for years and won't ever be observed on the network with this meaning. Keeping this dead assignment in our source code forever doesn't add any value.
I somehow forgot to do this in commit fa0d0ff6e1bee60fde63724ae28a51aac5a94d4a.
ACKs for top commit:
laanwj:
Code review ACK fa40168ab3102b9ad850f967a0e7fa22dbfbd0c6
practicalswift:
cr ACK fa40168ab3102b9ad850f967a0e7fa22dbfbd0c6
fanquake:
ACK fa40168ab3102b9ad850f967a0e7fa22dbfbd0c6
Tree-SHA512: 376e5ac05940493cf2209fea60515c843e978c4b476f2524f6bf7a37a646d237c3ddcf6c0fa23641f9ba550f625609703d9b51b4be631a7f2a90e1092b557232
fa8abdc9953e381715493b259908e246914793b0 rpc: Use FeeModes doc helper in estimatesmartfee (MarcoFalke)
Pull request description:
Not sure why this doesn't use the doc helper, probably an oversight?
ACKs for top commit:
laanwj:
Code review ACK fa8abdc9953e381715493b259908e246914793b0
Tree-SHA512: 1f2dc8356e3476ddcf9cafafa7f9865ad95bed1e3067c0edab8e3c483e374bdbdbecc066167554b4a1b479e28f6a52c4ae6a75a70c67ee4e1ff4f3ba36b04001
## Issue being fixed or feature implemented
Since v19, Evo nodes are paid 4x blocks in a row.
This needs to be reverted when MN Reward Reallocation activates.
## What was done?
Starting from MN Reward Reallocation activation, Evo nodes are paid one
block in a row (like regular masternodes).
In addition, `nConsecutivePayments` isn't incremented anymore for Evo
nodes.
## How Has This Been Tested?
`feature_llmq_hpmn.py` with MN Reward Reallocation activation.
## Breaking Changes
no
## 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)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
a6739cc86827759c543bf81f5532ec46e40549c3 rpc: Add specific error code for "wallet already loaded" (Wladimir J. van der Laan)
Pull request description:
Add a separate RPC error code for "wallet already loaded" to avoid having to match on message to detect this.
Requested by shesek for rust-bitcoinrpc.
If concept ACKed needs:
- [ ] Release note
- [x] A functional test (updated the existing test to make it pass, I think this is enough)
ACKs for top commit:
jonasschnelli:
Code Review ACK a6739cc86827759c543bf81f5532ec46e40549c3
promag:
Code review ACK a6739cc86827759c543bf81f5532ec46e40549c3.
Tree-SHA512: 9091872e6ea148aec733705d6af330f72a02f23b936b892ac28f9023da7430af6332418048adbee6014305b812316391812039e9180f7f3362d11f206c13b7d0
fa39c8a3e8f1090103468780d122a4cf4191bc13 test: Work around libFuzzer deadlock (MarcoFalke)
Pull request description:
Only required part is `symbolize=0`, but the other changes shouldn't hurt
ACKs for top commit:
practicalswift:
cr ACK fa39c8a3e8f1090103468780d122a4cf4191bc13: patch looks correct
Tree-SHA512: 9cddf1de46ad12aea9b8be2c1acb86ba0e07ffdb52f8155d943edf970955551c7cb049a3a6c027846b45dab0dc0966dec42999476ebde50aa761a08dbb751eae
bfa9309ad606102f24c9bd3c33dfe78949f09418 Use COINBASE_MATURITY constant in functional tests. (Kiminuo)
525448df9dc2ab6b7e960ff138956ae3e2efdf60 Move COINBASE_MATURITY from `feature_nulldummy` test to `blocktools`. (Kiminuo)
Pull request description:
`COINBASE_MATURITY` constant was added to `feature_nulldummy` test in #21373. This PR moves the constant to `blocktools.py` file and uses the constant in more tests as suggested [here](https://github.com/bitcoin/bitcoin/pull/21373#discussion_r605418462).
Edit: Goal of this PR is to replace integer constants with `COINBASE_MATURITY` but not necessarily in *all* cases because that would mean to read and fully understand all tests. That's out of my time constraints. Any reports where `COINBASE_MATURITY` should be used are welcome though!
ACKs for top commit:
theStack:
ACK bfa9309ad606102f24c9bd3c33dfe78949f09418 🌇
Tree-SHA512: 01f04645f05a39028681f355cf3d42dd63ea3303f76d93c430e0fdce441934358a2d847a54e6068d61932f1b75e1d406f51859b057b3e4b569f7083915cb317f
92e28fa8b2590cce0e8f0adadae80e46cb63a9ef test: remove unused constants in functional tests (Sebastian Falbesoner)
Pull request description:
This mini-PR gets rid of constants in functional tests that are not used anymore. Found by [vulture ](https://pypi.org/project/vulture/)via the following script that has been lying around here locally for quite some time (I think it was once proposed by practicalswift, but I don't remember the concrete topic/PR):
```
#!/bin/sh
for F in $(git ls-files -- "*.py"); do vulture "$F" | grep "unused variable"; done
```
ACKs for top commit:
practicalswift:
ACK 92e28fa8b2590cce0e8f0adadae80e46cb63a9ef: patch looks correct.
Tree-SHA512: 16516abc8014207bcefdf0545dffaecff1fbba66f45b54c02371dcfd1f18194855c6b72598c11b5407009561eafe8048d47af3471f0efb1795d52477d5a0232e
## Issue being fixed or feature implemented
Current implementation of MnEhfTx is not matched with DIP-0023, this PR
fixes it. It is a prior work for
https://github.com/dashpay/dash/pull/5469
## What was done?
- requestID is fixed from `clsig{quorumHeight}` to `mnhf{versionBit}` +
fixes for signature validation properly
- v20 is minimal height to accept MnEHF special transactions
- versionBit is not BLS version - removed unrelated wrong code and
validations
- TxMempool will accept MnEHF transaction even if inputs/outputs are
zeroes and no fee
- implemented python's serialization/deserialization of MnEHF
transactions for future using in functional tests
## How Has This Been Tested?
Run functional/unit tests. Beside that there's new functional test in
https://github.com/dashpay/dash/pull/5469 that actually test format of
transaction and signature validation - to be merged later.
## Breaking Changes
Payload of MnEhf tx is changed, related consensus rules are changed.
## 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
89bdad5b25ae4ac03a486f729a5b58ae6f21946d RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint (Luke Dashjr)
Pull request description:
Allow specifying the `wallet_name` param to `unloadwallet` on RPC wallet endpoints, so long as it matches the endpoint wallet.
ACKs for top commit:
jonatack:
ACK 89bdad5b25ae4ac03a486f729a5b58ae6f21946d
MarcoFalke:
review ACK 89bdad5b25ae4ac03a486f729a5b58ae6f21946d
Tree-SHA512: efb399c33f7b5596870a26a8680f453ca47aa7a6db4e550f9435d13044f1c4bad0ae11e8f0205213409d08b75c4188c3be782e54aafab1f65b97eb8cf5c252a9
c92387232f750397da7d131f262c150a608408c2 refactor: Extract ParseOpCode from ParseScript (João Barbosa)
Pull request description:
Seems more natural to have `mapOpNames` "hidden" in `ParseOpCode` than in `ParseScript`.
A second lookup in `mapOpNames` is also removed.
ACKs for top commit:
laanwj:
ACK c92387232f750397da7d131f262c150a608408c2
theStack:
re-ACK c92387232f750397da7d131f262c150a608408c2
Tree-SHA512: d59d1964760622cf365479d44e3e676aa0bf46b60e77160140d967e012042df92121d3224c7551dc96eff5ff3294598cc6bade82adb3f60d28810e18e60e1257
## Issue being fixed or feature implemented
The logic for additional indexes is incomplete, handling of P2PK on
block disconnect is broken (luckily no one is using P2PK and reorgs are
rare) and there are a few other small issues that would be nice to have
fixed.
## What was done?
Pls see individual commits
## How Has This Been Tested?
Run `feature_dbcrash.py`, it should succeed (NOTE: it takes ~30 minutes
to complete, that's normal).
Run `feature_addressindex.py`, `feature_timestampindex.py` and
`feature_spentindex.py` (and other tests) should still succeed too.
## 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)_
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>
## Issue being fixed or feature implemented
V19 is active on mainnet/testnet now, no need to check activation bits
anymore. This PR also bumps `MinBIP9WarningHeight` to
post-v19-activation height which should stop `unknown new rules
activated (versionbit 8)` warning from appearing.
## What was done?
Bury v19, bump `MinBIP9WarningHeight`
## How Has This Been Tested?
Run tests, reindex on mainnet/testnet.
## 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)_
5aadd4be1883386a04bef6a04e9a1142601ef7a7 Convert amounts from float to decimal (Prayank)
Pull request description:
> decimal is preferred in accounting applications
https://docs.python.org/3.8/library/decimal.html
Decimal type saves an exact value so better than using float.
~~3 variables declared with type as 'Decimal' in [test/functional/mempool_accept.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_accept.py): fee, fee_expected, output_amount~~
~~Not required to convert to string anymore for using the above variables as decimal~~
+ fee, fee_expected, output_amount
~~+ 8 decimal places~~
+ Using value of coin['amount'] as decimal and removed 'int'
+ Removed unnecessary parentheses
+ Remove str() and use quotes
Fixes https://github.com/bitcoin/bitcoin/issues/20011
ACKs for top commit:
guggero:
ACK 5aadd4be1883386a04bef6a04e9a1142601ef7a7
Tree-SHA512: 5877cf3837e5b65bec0fc8909de141a720bfa02a747513e21d20f3c41ec0cfecc524d2c347a96596b0a1a97900da2acf08b799f26b11d537e4dcddc6ce45f38e
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
a2324e4d3f47f084b07a364c9a360a0bf31e86a0 test: Improve naming and logging of avoid_reuse tests (Fabian Jahr)
1abbdac6777bc5396d17a6772c8176a354730997 wallet: Prefer full destination groups in coin selection (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17843)
In the case of destination groups of >10 outputs existing in a wallet with `avoid_reuse` enabled, the grouping algorithm is adding left-over outputs as an "incomplete" group to the list of groups even when a full group has already been added. This leads to the strange behavior that if there are >10 outputs for a destination the transaction spending from that will effectively use `len(outputs) % 10` as inputs for that transaction.
From the original PR and the code comment I understand the correct behavior should be the usage of 10 outputs. I opted for minimal changes in the current code although there maybe optimizations possible for cases with >20 outputs on a destination this sounds like too much of an edge case right now.
ACKs for top commit:
jonatack:
Re-ACK a2324e4
achow101:
ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0
kallewoof:
ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0
meshcollider:
Tested ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0 (verified the new test fails on master without this change)
Tree-SHA512: 4743779c5d469fcd16df5baf166024b1d3c8eaca151df1e8281b71df62b29541cf7bfee3f8ab48d83e3b34c9256e53fd38a7b146a54c79f9caa44cce3636971a
25dac9fa65243ca8db02df22f484039c08114401 doc: add release notes for explicit fee estimators and bumpfee change (Karl-Johan Alm)
05227a35545d7656450874b3668bf418c73813fb tests for bumpfee / estimate_modes (Karl-Johan Alm)
3404c1b753432c4859a4ca245f01c240610a00cb policy: optional FeeEstimateMode param to CFeeRate::ToString (Karl-Johan Alm)
6fcf4484302d13bd7739b617470d8c8e31974908 rpc/wallet: add two explicit modes to estimate_mode (Karl-Johan Alm)
b188d80c2de9ebb114da5ceea78baa46bde7dff6 MOVEONLY: Make FeeEstimateMode available to CFeeRate (Karl-Johan Alm)
5d1a411eb12fc700804ffe5d6e205234d30edd5f fees: add FeeModes doc helper function (Karl-Johan Alm)
91f6d2bc8ff4d4cd1b86daa370ec9d2d9662394d rpc/wallet: add conf_target as alias to confTarget in bumpfee (Karl-Johan Alm)
69158b41fc488e4f220559da17a475eff5923a95 added CURRENCY_ATOM to express minimum indivisible unit (Karl-Johan Alm)
Pull request description:
This lets users pick their own fees when using `sendtoaddress`/`sendmany` if they prefer this over the estimators.
ACKs for top commit:
Sjors:
re-utACK 25dac9fa65: rebased, more fancy C++,
jonatack:
ACK 25dac9fa65243ca8db02df2 I think this should be merged after all this time, even though it looks to me like there are needed follow-ups, fixes and test coverage to be added (see further down), which I don't mind helping out with, if wanted.
fjahr:
Code review ACK 25dac9fa65243ca8db02df22f484039c08114401
Tree-SHA512: f31177e6cabf3187a43cdfe93477144f8e8385c7344613743cbbd16e8490d53ff5144aec7b9de6c9a65eb855b55e0f99d7f164dee4b6bf3cfea4dce51cf11d33
## 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
It splits from https://github.com/dashpay/dash/pull/5150/ by
@PastaPastaPasta request.
## What was done?
See commits
## How Has This Been Tested?
Run unit/functional 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
b1f59d55d920d2b35269b474762f94fec87bfb16 RPC/Wallet: unloadwallet: Clarify docs/error when both the RPC endpoint and wallet_name parameter specify a wallet (Luke Dashjr)
Pull request description:
Just documentation clarifications from #20448
ACKs for top commit:
MarcoFalke:
review ACK b1f59d55d920d2b35269b474762f94fec87bfb16
jonatack:
re-ACK b1f59d55d920d2b35269b474762f94fec87bfb16 per `git diff e8303a0 b1f59d5`
Tree-SHA512: ac068b0aa7ceed49496367fdd9425b59dbba18b56e89b26afc22a6c8ece51f0b92a169cacd55740b1cadab2b32f4f8e8700e609066ab7e59d3b53c7891da585e
## Issue being fixed or feature implemented
Allow generating 12, 18, 24 words mnemonics. Default to 12 words as it's
the most popular option/de-facto a standard now imo.
## What was done?
Add `-mnemonicbits` option, add tests
## How Has This Been Tested?
run tests, play with wallets on regtest
## Breaking Changes
n/a, old wallets should not be affected
## 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
Given the hard fork that happened on testnet, there is now lots of the
transactions that were made on the fork that is no longer valid. Some
transactions could be relayed and mined again but some like coinjoin
mixing won't be relayed because of 0 fee and transactions spending
coinbases from the forked branch are no longer valid at all.
## What was done?
Introduce `wipewallettxes` RPC and `wipetxes` command for `dash-wallet`
tool to be able to get rid of some/all txes in the wallet.
## How Has This Been Tested?
run tests, use rpc/command on testnet wallet
## 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
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
Disabled or non-enforced Chainlocks does not mean you can safely mine
non-locked txes, you could end up mining a block that is going to be
rejected by everyone else if a conflicting tx (missing on your node)
would be IS-locked. I can't find any reason why we have this besides "if
Chainlocks are disabled then smth is wrong so let them all be mined" but
we have spork_2 and spork_3 to control IS behaviour and we check them in
`IsTxSafeForMining` already, that would be a much more straightforward
way to deal with a potential issue.
Noticed this while reviewing #5150 and also while testing v19.2 during
recent testnet v19 re-fork.
## What was done?
Drop this check, adjust tests
## How Has This Been Tested?
Run tests locally
## Breaking Changes
Not quote breaking changes but a change in behaviour: with CLs disabled
it will now take 10 minutes for non-locked txes to be mined, same as
when CLs are enabled.
## 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
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)_
451b96f7d2796d00eabaec56d831f9e9b1a569cc test: kill process group to avoid dangling processes (S3RK)
Pull request description:
This is an alternative to #19281
This PR fixes a problem when after test failure with `--failfast` option there could be dangling nodes. The nodes will continue to occupy rpc/p2p ports on the machine and will cause further test failures.
If there are any dangling nodes left at the end of the test run we kill the whole process group.
Pros: the operations is immediate and won't lead to CI timeout
Cons: the test_runner process is also killed and exit code is 137
Example output:
```
...
Early exiting after test failure
TEST | STATUS | DURATION
rpc_decodescript.py | ✓ Passed | 2 s
rpc_deprecated.py | ✓ Passed | 2 s
rpc_deriveaddresses.py | ✓ Passed | 2 s
rpc_dumptxoutset.py | ✖ Failed | 2 s
ALL | ✖ Failed | 8 s (accumulated)
Runtime: 4 s
Killed: 9
> echo $?
137
```
ACKs for top commit:
MarcoFalke:
review ACK 451b96f7d2796d00eabaec56d831f9e9b1a569cc
aitorjs:
ACK 451b96f7d2796d00eabaec56d831f9e9b1a569cc. Manual testing with and without **--failfast**.
Tree-SHA512: 87e510a1411b9e7571e63cf7ffc8b9a8935daf9112ffc0f069d6c406ba87743ec439808181f7e13cb97bb200fad528589786c47f0b43cf3a2ef0d06a23cb86dd
fa92af5af39a08982f785542df5419d6d5a4706d ci: Run feature_block and feature_abortnode in valgrind (MarcoFalke)
fa01febeaf801bade77a613e64f18b556ae16d86 test: Remove ci timeout restriction in test_runner (MarcoFalke)
Pull request description:
Also revert commit 0a4912e46a, because some tests take too long for this to be useful anymore.
Top commit has no ACKs.
Tree-SHA512: 363f14766e1f4a5860ab668a516b41acebc6fbdf11d8defb3a95a772dbf82304ca1f5f14b1dbad97f2029503e03d92e8c69df0466a8872409c20665838f617ed
4444edc2e6671d3f73de3725447130f73ecf0375 ci: Enable all functional tests in valgrind (MarcoFalke)
Pull request description:
The travis timeout for our repo has been bumped to 2h, so we can run all tests in valgrind now
ACKs for top commit:
practicalswift:
ACK 4444edc2e6671d3f73de3725447130f73ecf0375 -- regarding the three disabled cases (`feature_abortnode`, `feature_block` and `rpc_bind`): not a big deal since MSan will take care of those once #18288 is merged. More is more :)
Tree-SHA512: ea2f798112911b6d1f3d88cfcdf0a7cdb698687248343703d6fe55da144542c961c15d888bffb41672c10aa76765615cb7c7ff93d468bfad3c51f962f24e7abb
