056d869571 refactor: use testdummy in feature_mnehf functional test, removed useless checks (Konstantin Akimov)
0351469bb5 refactor: removed duplicated meaningless condition from Check mnhftx (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Using mn_rr in feature_mnehf.py is a blocker for burying mn_rr fork.
## What was done?
Removed useless conditions, uses testdummy fork instead mn_rr in ehf functional test.
## How Has This Been Tested?
Run it `test/functional/feature_mnehf.py`
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 056d869571
PastaPastaPasta:
utACK 056d869571
Tree-SHA512: aa016dc21fee3afdb5a4172e757cdd0b92867c23eb5241287f641d3c97d363ab1c46eba06423481ecc7ba05f6c0010b65eadfe77d4a1fd6074706cc6f3a71309
cc998abec1 fmt: apply `clang-format-diff.py` suggestions (Kittywhiskers Van Gogh)
0401c581eb stats: `const`-ify variables and arguments (Kittywhiskers Van Gogh)
9f96723774 stats: stop using error codes, switch over to `bool` (Kittywhiskers Van Gogh)
1a81979c1e stats: initialize socket after we have a valid socket address (Kittywhiskers Van Gogh)
dbbfc8d766 stats: use `Socks` wrapper, use `CService` to generate our `sockaddr` (Kittywhiskers Van Gogh)
2def905044 stats: move init logic into constructor (Kittywhiskers Van Gogh)
4bc727cd6c stats: clean up randomization code, move `FastRandomContext` inward (Kittywhiskers Van Gogh)
840241eefd stats: cleanup error logging, improve code sanity (Kittywhiskers Van Gogh)
85890ddb13 docs: add copyright notice to source file, update notice in header (Kittywhiskers Van Gogh)
a9d1b1494d stats: move `_StatsdClientData` variables into `StatsdClient` (Kittywhiskers Van Gogh)
30c30c1397 stats: fetch all arguments needed when constructing `g_stats_client` (Kittywhiskers Van Gogh)
5133d88415 stats: s/statsClient/g_stats_client/g (Kittywhiskers Van Gogh)
f81951dd00 stats: make `statsClient` a `std::unique_ptr`, denote as global variable (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Support for transmitting stats to a Statsd server has been courtesy of Statoshi ([repo](https://github.com/jlopp/statoshi)), implemented Dec, 2020 by [dash#2515](https://github.com/dashpay/dash/pull/2515) but since then, it hasn't gotten much attention aside from benefiting from codebase-wide changes and the occasional compiler appeasement. This pull request aims to give our statistics code some TLC.
Changes include:
* Limiting initialization to solely during construction and moving the responsibility of fetching arguments outside of `statsd::StatsdClient`.
* Using the RAII `Socks` wrapper as early as possible (we still need to construct a raw socket ourselves but this is done in the initializer and control is moved to the wrapper and everywhere else, the wrapper is used)
* Utilizing existing networking code to generate the socket address
* This lets us trivially allow IPv6 connections as the responsibility to construct it safely is moved to `CService`.
* Using `std::string` and our string manipulation capabilities (replacing `snprintf` with `strprintf`), replacing platform-specific types (replacing `short` with `uint16_t`).
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK [cc998ab](cc998abec1)
UdjinM6:
utACK cc998abec1
Tree-SHA512: 433c92160d6ac7ebb8582ada3cbb65ead7913618266b773619a528c90dfe0e286aafa46dc3b0bca62f246938e5948a732080e2cddba942d3627f007ca6efcc1f
6c7335e002 merge bitcoin#24331: Revert back `MoveFileExW` call for MinGW-w64 (Kittywhiskers Van Gogh)
15e794bdd8 merge bitcoin#24238: use arc4random on OpenBSD (Kittywhiskers Van Gogh)
e039aecbdc merge bitcoin#23936: Add and use EnsureArgsman helper (Kittywhiskers Van Gogh)
b4bfacfd24 merge bitcoin#23769: Disallow copies of CChain (Kittywhiskers Van Gogh)
5b66688160 merge bitcoin#22362: Drop only invalid entries when reading banlist.json (Kittywhiskers Van Gogh)
109c963f6a merge bitcoin#23175: Add CJDNS network to -addrinfo and -netinfo (Kittywhiskers Van Gogh)
d57c96ea37 merge bitcoin#23398: add return message to savemempool RPC (Kittywhiskers Van Gogh)
22e59fb464 merge bitcoin#23054: Use C++11 member initializer in CTxMemPoolEntry (Kittywhiskers Van Gogh)
d158063b6d merge bitcoin#22653: Rename JoinErrors and re-use it (Kittywhiskers Van Gogh)
e24324d266 merge bitcoin#22221: Pass block reference instead of pointer to PeerManagerImpl::BlockRequested (Kittywhiskers Van Gogh)
68657efc03 merge bitcoin#22141: net processing: Remove hash and fValidatedHeaders from QueuedBlock (Kittywhiskers Van Gogh)
c0e6792e27 merge bitcoin#20018: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* When backporting [bitcoin#23054](https://github.com/bitcoin/bitcoin/pull/23054), `sigOpCount` and `nSigOpCountWithAncestors` were switched from `unsigned int` to `int64_t`. This change was done to prevent integer narrowing when accepting the `int64_t` taken from the constructor.
This isn't a problem upstream as the same changes were as part of [bitcoin#8149](https://github.com/bitcoin/bitcoin/pull/8149/files#diff-8a2230436880b65a205db9299ab2e4e4adb1d4069146791b5db566f3fb752adaL90-L107), which was omitted but the type changes remain valid as sigop count cannot be a negative number.
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 6c7335e002
UdjinM6:
utACK 6c7335e002
Tree-SHA512: 29cae42dd82235305d3558562bae346170e742ce0b65897e396b331294b39cad0dd831fa9a09b34780a67844e55292e5b4e784cf544a894cc3f8897afe617ca1
- Use `uint16_t` instead of `short`, `int64_t` instead of `size_t`
- Get rid of the `errmsg` buffer and use `LogPrintf` to report errors
- Use `strprintf` instead of `snprintf`
- Rephrase networking error logs to allow inclusion of error strings
0c5179462e test: rework feature_governance_cl.py (UdjinM6)
11ac0819da feat: bump_mocktime also bumps schedulers now (UdjinM6)
1937f503fe feat: regtest-only: do not auto-reset mnsync after 1h (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
1. The original idea behind forced reset was to help desktop clients which go into sleep/hibernation mode from time to time to sync with no issues once they are online again. For regtest however it doesn't do anything good and only causes issues.
2. We rely on schedulers a lot, bumping them should let nodes behave more like on a real network.
3. Forcing mnsync to skip governance votes doesn't always work as we'd expect cause we don't control connection creation. To make it more deterministic create a proposal that should get into one superblock only. This way both the proposal and the trigger will expire/be deleted after another superblock meaning we can be 100% sure the isolated node never gets any of them and can only sync thanks to chainlock.
## 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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 0c5179462e
Tree-SHA512: 9a40eeaba880f3f27f86736e92afa19a4ecb2a2d157bc42b65dd9da0d4109c9cd1d83a5abdf1ec16be2f8a8b31821fb700a7f0d2265c094fd4fdff7f18bc6ec7
57fb0874ef feat: broadcast dsq messages using the inventory system (pasta)
Pull request description:
## Issue being fixed or feature implemented
DSQ messages are 142 bytes.
Previously, assuming a relatively highly connected masternode hosting 100 connection, each round of coinjoin will result in 14.2KB (100*142) of inbound and outbound traffic each.
## What was done?
Now, using the inventory system, a message will first use 36 bytes per peer (sending and receiving), plus the size of a `getdata` message and the actual message itself. As a result, bandwidth usage for 1 round of mixing would be closer to 36 * 100 + 142 (dsq) + 36 (getdata) = ~3.8KB, a reduction of around ~73%
## How Has This Been Tested?
Has not been; @UdjinM6 especially please review well :)
## Breaking Changes
Does introduce a new protocol version, but in a backwards compatible way. I don't think this would need to be delayed to v22 for any reason.
## 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
- [ ] 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)_
Top commit has no ACKs.
Tree-SHA512: 3dc39a339cba29d8cf207cec76ecace5ad0e11d1892ca0f65f9253a2b1d90313da21c6c178c2476756c5566ece0fab777006cd609b7984df906a9206c25d921d
DSQ messages are 142 bytes.
Previously, assuming a relatively highly connected masternode hosting 100 connection, each round of coinjoin will result in 14.2KB (100*142) of inbound and outbound traffic each.
Now, using the inventory system, a message will first use 36 bytes per peer (sending and receiving), plus the size of a `getdata` message and the actual message itself. As a result, bandwidth usage for 1 round of mixing would be closer to 36 * 100 + 142 (dsq) + 36 (getdata) = ~3.8KB, a reduction of around ~73%
23812555b1 fix: possible deadlock during calculation of signals for historical blocks during re-index (Konstantin Akimov)
1087489fd4 feat: bury v20 deployment (Konstantin Akimov)
64cedb30bd feat: actually test something EHF unit tests (Konstantin Akimov)
762a808b8c chore: drop irrelevant bip9 code from feature_llmq_rotation.py (Konstantin Akimov)
7735631aad fix: remove v20 from test feature_llmq_evo as far as mn_rr used (Konstantin Akimov)
ca83b26815 fix: crash in CreditPool: it meant to check that DIP0003 is activated (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
V20 is activated on mainnet: time to bury it!
https://github.com/dashpay/dash/issues/6186
## What was done?
Hard-fork v20 is buried and it requires to implement multiple fixes, simplifications, refactoring:
- some tests for EHF moved from functional tests to unit tests
- fixed crash in Credit Pool if DIP3 is not activated yet
- added a requirement for v20 activation for `CMNHFManager::GetSignalsStage`
- removed useless code from functional test feature_llmq_rotation
- renamed variables "v20" to "mn_rr" in feature_llmq_evo.py so far as actually used fork is mn_rr
## How Has This Been Tested?
Some unit and functional tests to succeed.
Done reindex (just in case):
src/qt/dash-qt -reindex -assumevalid=0
src/qt/dash-qt -reindex -assumevalid=0 -testnet
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
ACK 23812555b1
PastaPastaPasta:
utACK 23812555b1
Tree-SHA512: eec35745baa695f3f286d39b6a61fa0a9f34820b13d1dd4cfbd1efe707850283892c39bf7fe49c49c812e0c02465d64df11480b3f12aa7f21b59a71eeae7260e
e92aad7cff test: make sure MNs don't vote twice even when they are allowed to (UdjinM6)
3d75390e4e fix: correct conditions for YES voting (UdjinM6)
ec1392c6de chore: make clang-format and linter happy (UdjinM6)
a6320865c4 fix: avoid voting for the same trigger multiple times (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
We just had a sb voting period and I noticed that the network is way too spammy and produces too many votes (10x+ the expected numbers). It turns out that relying on `ProcessVoteAndRelay` on mainnet is not enough because rate-check expires too soon and MNs are able to vote again and again. On testnet it was never an issue because the voting period there is really short.
## What was done?
Check known votes to make sure we never voted earlier.
## How Has This Been Tested?
Run tests, run a MN on mainnet and check logs.
## 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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK e92aad7cff
Tree-SHA512: 142e23d3a19fa9527fa5257eb790e558d3507a7a857f17c6e02fd58eeb5643fcfb48d824d227e0ea7cd3dae6a6d7d871b3af88b13077f5af074ed1911e42bb28
0a7a234bd3 merge bitcoin#28110: correct Fedora systemtap dep (Kittywhiskers Van Gogh)
c92eb67c40 merge bitcoin#27458: Detect USDT the same way how it is used in the code (Kittywhiskers Van Gogh)
f4a53ba8ce merge bitcoin#26945: systemtap 4.8 (Kittywhiskers Van Gogh)
88696129f3 merge bitcoin#25794: don't rely on block_connected USDT event order in tests (Kittywhiskers Van Gogh)
457bbd3f8b merge bitcoin#25360: SystemTap 4.7 (RISC-V support) (Kittywhiskers Van Gogh)
f3b219ad0d merge bitcoin#24358: USDT tracepoint interface tests (Kittywhiskers Van Gogh)
5b10a5a2fd merge bitcoin#23907: utxocache tracepoints follow up (Kittywhiskers Van Gogh)
c3d7e3a192 merge bitcoin#26944: fix systemtap download URL (Kittywhiskers Van Gogh)
264e02fcc7 merge bitcoin#23724: add systemtap's sys/sdt.h as depends for GUIX builds with USDT tracepoints (Kittywhiskers Van Gogh)
6cc596b99a merge bitcoin#22902: utxocache tracepoints (Kittywhiskers Van Gogh)
644a47ef9a merge bitcoin#23302: drop GetHash().ToString() argument from the `validation:block_connected` tracepoint (Kittywhiskers Van Gogh)
bfdc9ad364 merge bitcoin#23375: more deterministic coin selection for coinbase UTXOs (oldest first) (Kittywhiskers Van Gogh)
5718716cd2 merge bitcoin#22955: Rename fBlocksOnly, Add test (Kittywhiskers Van Gogh)
cacc31213b merge bitcoin#22006: first tracepoints and documentation on User-Space, Statically Defined Tracing (USDT) (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* In [bitcoin#22955](https://github.com/bitcoin/bitcoin/pull/22955), `fBlocksOnly` has not been renamed to `reject_tx_invs` as Dash uses the inventory system beyond relaying transaction data with the block-relay-only blocklist have a greater set of prohibited messages. This renders the new name misleading but coining a new name may be a source of confusion, making retaining the legacy name desirable.
* Additionally, because the word "transaction" isn't hardcoded into the log message (instead opting to use `CInv::GetCommand()`'s output instead, which for transactions, are "tx"), the expected log message in `p2p_blocksonly.py` has been adjusted accordingly.
* [bitcoin#24358](https://github.com/bitcoin/bitcoin/pull/24358) depends on [bitcoin#22955](https://github.com/bitcoin/bitcoin/pull/22955) and [bitcoin#23375](https://github.com/bitcoin/bitcoin/pull/23375) in order to work as without the latter backport, `interface_usdt_utxocache.py` will return the following error
<details>
<summary>Test run:</summary>
```
debian@debian:~/dash$ sudo ./test/functional/interface_usdt_utxocache.py
[sudo] password for debian:
2024-08-26T17:08:05.234000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_n5rb0xy4
2024-08-26T17:08:07.023000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API
2024-08-26T17:08:07.026000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/debian/dash/test/functional/test_framework/test_framework.py", line 160, in main
self.run_test()
File "/home/debian/dash/./test/functional/interface_usdt_utxocache.py", line 149, in run_test
self.test_uncache()
File "/home/debian/dash/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache
invalid_tx = self.wallet.create_self_transfer(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/debian/dash/test/functional/test_framework/wallet.py", line 166, in create_self_transfer
assert_equal(mempool_valid, tx_info['allowed'])
File "/home/debian/dash/test/functional/test_framework/util.py", line 51, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(True == False)
2024-08-26T17:08:07.533000Z TestFramework (INFO): Stopping nodes
2024-08-26T17:08:08.535000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_n5rb0xy4
2024-08-26T17:08:08.535000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_n5rb0xy4/test_framework.log
2024-08-26T17:08:08.535000Z TestFramework (ERROR):
2024-08-26T17:08:08.535000Z TestFramework (ERROR): Hint: Call /home/debian/dash/test/functional/combine_logs.py '/tmp/dash_func_test_n5rb0xy4' to consolidate all logs
2024-08-26T17:08:08.536000Z TestFramework (ERROR):
2024-08-26T17:08:08.536000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-08-26T17:08:08.536000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-08-26T17:08:08.536000Z TestFramework (ERROR):
```
</details>
with the underlying error that has been alluded to in d2c4904
```
{'txid': '44b58b10e69321dacf00724f1893c9ecb50fc1f89ed7c70a6c0b3c08f7dc750f', 'allowed': False, 'reject-reason': 'bad-txns-premature-spend-of-coinbase'}
```
* In [bitcoin#24358](https://github.com/bitcoin/bitcoin/pull/24358), all `interface_usdt_*.py` tests needed minor syntax changes to account for how we invoke certain RPCs. `interface_usdt_utxocache.py` required tweaking with the blocks needed to be eligible for pruning (`450` vs. `350` upstream) and stop the node explicitly to account for the governance validation warning .
This is important as the placement assumes that `test_flush()` is the last test, should this change, the node may need to be manually started before continuing on with the tests after.
* Some `TRACE*` entries may not exactly match the backports they come from because the variables or functions they access may have been amended by a backport done _after_ the backport that introduce the trace entry.
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [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)_
ACKs for top commit:
UdjinM6:
utACK 0a7a234bd3
PastaPastaPasta:
utACK [0a7a234](0a7a234bd3)
Tree-SHA512: 8df11392dd8d152c18d55ac70a446d1eec336bdf1a984cbf41c3202c353358180e05ba4b7182ec2962ea09eefa41d1dc3cd383d358f9b3dec57ce8b67c6e6afd
9876c2d78b docs: add partial release notes (UdjinM6)
b330318db7 refactor: drop circular dependency (UdjinM6)
e54fe42ce8 refactor: use `key_to_p2pkh_script` in more places (UdjinM6)
3ed6246889 test: check `creditOutputs` format (UdjinM6)
ba0e64505b fix: `creditOutputs` in AssetLock tx json output should be an array of objects, not debug strings (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Txout-s in `creditOutputs` for AssetLock txes should be shown the way txout-s are shown in other places. We should not be using debug strings there.
Example: `getrawtransaction 50757f651f335e22c5a810bd05c1e5aac0d95b132f6454e2a72683f88e3983f3 1`
develop:
```
"assetLockTx": {
"version": 1,
"creditOutputs": [
"CTxOut(nValue=0.01000000, scriptPubKey=76a914cdfca4ae1cf2333056659a2c)"
]
},
```
This PR:
```
"assetLockTx": {
"version": 1,
"creditOutputs": [
{
"value": 0.01000000,
"valueSat": 1000000,
"scriptPubKey": {
"asm": "OP_DUP OP_HASH160 cdfca4ae1cf2333056659a2c8dc656f36d228402 OP_EQUALVERIFY OP_CHECKSIG",
"hex": "76a914cdfca4ae1cf2333056659a2c8dc656f36d22840288ac",
"address": "yf6c2VSpWGXUgmjQSHRpfEcTPsbqN4oL4c",
"type": "pubkeyhash"
}
}
]
},
```
kudos to @coolaj86 for finding the issue
## What was done?
Change `CAssetLockPayload::ToJson()` output to be closer to [`TxToUniv()`](https://github.com/dashpay/dash/blob/develop/src/core_write.cpp#L262-L272)
NOTE: `refactor: use key_to_p2pkh_script in more places` commit is a bit unrelated but I decided to add it anyway to make it easier to follow assetlock creation vs getrawtransaction rpc check.
## How Has This Been Tested?
Try example above, run tests
## Breaking Changes
RPC output is different for AssetLock txes
## 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
ACKs for top commit:
PastaPastaPasta:
utACK 9876c2d78b
Tree-SHA512: 158c98ac9e4979bb29c4f54cb1b71806f22aaec92218d92cd2b2e9b9f74df721563e7a6c5f517ea358ac74659fa79f51d1b683002a1cdceb1b8ee80f8fd79375
`fBlocksOnly` has not been renamed as Dash uses the inv system for
relaying far more than transaction data and the blocklist of invs in
block-relay-only mode extends beyond what is conveyed by `reject_tx_invs`
f032119456 merge bitcoin#22910: Encapsulate asmap in NetGroupManager (Kittywhiskers Van Gogh)
8020bfa8c1 merge bitcoin#24665: document clang tidy named args (Kittywhiskers Van Gogh)
40a22e457a merge bitcoin#24201: Avoid InitError when downgrading peers.dat (Kittywhiskers Van Gogh)
cdcaf2278c merge bitcoin#23373: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change (Kittywhiskers Van Gogh)
b30f0fa441 test: remove `connman` local from `BasicTestingSetup` (Kittywhiskers Van Gogh)
df43565464 merge bitcoin#23826: Make AddrMan unit tests use public interface, extend coverage (Kittywhiskers Van Gogh)
c14a54089f merge bitcoin#23780: update `addrman_tests.cpp` to use output from `AddrMan::Good()` (Kittywhiskers Van Gogh)
8b2db6bce4 merge bitcoin#23713: refactor addrman_tried_collisions test to directly check for collisions (Kittywhiskers Van Gogh)
5b5dd39f45 merge bitcoin#23492: tidy up addrman unit tests (Kittywhiskers Van Gogh)
aba0ebd400 merge bitcoin#23477: tidy up unit tests (Kittywhiskers Van Gogh)
cdc8321c4d merge bitcoin#22872: improve checkaddrman logging with duration in milliseconds (Kittywhiskers Van Gogh)
8d22fe9945 merge bitcoin#23084: avoid non-determinism in asmap-addrman test (Kittywhiskers Van Gogh)
ba4696718e partial bitcoin#23025: update nanobench add `-min_time` (Kittywhiskers Van Gogh)
c28b05c5ca merge bitcoin#22831: add addpeeraddress "tried", test addrman checks on restart with asmap (Kittywhiskers Van Gogh)
c4fe6085c8 merge bitcoin#22226: add unittest core dump instructions (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* In [bitcoin#22831](https://github.com/bitcoin/bitcoin/pull/22831), when restarting the node in `rpc_net.py`'s `test_addpeeraddress()`, existing commands need to be appended to `extra_args` to ensure they're retained when the node is restarted (default behavior is to overwrite the argument list with `extra_args`) to prevent the test from hanging and then failing due to missing fast DIP3 activation params from the arguments list.
Missing arguments result in a block database sanity check failure on restart due to `bad-qc-premature` arising from the activation height being higher than the height of a block with a quorum commitment.
* `NodeContext` was moved from `TestingSetup` to `BasicTestingSetup` in [bitcoin#18571](https://github.com/bitcoin/bitcoin/pull/18571) ([dash#4844](https://github.com/dashpay/dash/pull/4844)) but `connman` as a `BasicTestingSetup` variable still stuck around (despite `NodeContext`'s presence making this unnecessary).
To prepare for [bitcoin#22910](https://github.com/bitcoin/bitcoin/pull/22910), the remnant variable has been replaced with `m_node.connman` and adjustments have been made to that effect.
## Breaking Changes
None observed.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [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)_
ACKs for top commit:
UdjinM6:
utACK f032119456
PastaPastaPasta:
utACK f032119456
Tree-SHA512: b29c292ecda54cda8301ea804b433f80476a1cdbb72bd48740cc9b2e885a4ff52350e5e42f112426856282bd6d961f0e37f1b23020c52f07238413070bbc504a
`p2p_ibd_txrelay.py` was introduced in bitcoin#19423 but not backported
as Dash doesn't have feefilter capabilities but this backport has the
test check for additional cases which are within Dash's capabilities, so
the test has been committed in with the feefilter portions minimally
stripped out
ef4d74a669 test: remove dead code from `p2p_initial_headers_sync.py` to favor of disable mocktime (UdjinM6)
4d9837c21e refactor: add a new flag disable_mocktime to set_test_params() (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
To disable mocktime you should re-implement setup_nodes(). It seems as bug-friendly solution
## What was done?
This PR introduce a new flag "disable_mocktime" which can be set in `set_test_params`.
It seems more error prune and the code is shorter
## How Has This Been Tested?
Run unit/functional tests including future changes from https://github.com/dashpay/dash/pull/6235
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK ef4d74a669
kwvg:
utACK ef4d74a669
PastaPastaPasta:
utACK ef4d74a669
Tree-SHA512: c6be8002cae4d7824e150938957464c156931d0b6f7fc41c430a83d662865431f1b56cb11df73f56db54a140f36b0addd68b2917e25c5c941fae52e8d322bc25
We need to continue inheriting the existing set of arguments to prevent
block invalidation due to missing arguments that allow for fast DIP3
activation (will manifest as `bad-qc-premature`)
be5c84f41d Merge bitcoin/bitcoin#22089: test: MiniWallet: fix fee calculation for P2PK and check tx vsize (MarcoFalke)
247141d32c Merge bitcoin/bitcoin#22210: test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
dad3ae3f33 Merge bitcoin/bitcoin#22130: test: refactor: dedup utility function chain_transaction() (MarcoFalke)
9dff334f47 Merge bitcoin-core/gui#361: Fix gui segfault caused by bitcoin/bitcoin#22216 (Hennadii Stepanov)
1087849955 Merge bitcoin/bitcoin#22216: refactor: Make SetupServerArgs callable without NodeContext (MarcoFalke)
fd94de6888 Merge bitcoin/bitcoin#21178: test: run mempool_reorg.py even with wallet disabled (MarcoFalke)
f1f5723fcf fix: missing changes from bitcoin#14123 (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## What was done?
See commits for list of backported changes. It also have some missing changes from bitcoin#14123
## 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
ACKs for top commit:
UdjinM6:
utACK be5c84f41d
PastaPastaPasta:
utACK be5c84f41d
Tree-SHA512: 553ffde63c8409799cf6b3b87bf1ee285fbf58b13c08d04cdac29bc0e4dd75059feaa2f163803084ae85175397512517b68a6e0e0cc602d981f38ac70d96e393
b75e83b298 merge bitcoin#24218: Fix implicit-integer-sign-change (Kittywhiskers Van Gogh)
8ecc22f51f merge bitcoin#23471: Improve ZMQ documentation (Kittywhiskers Van Gogh)
2965093c4a merge bitcoin#22079: Add support to listen on IPv6 addresses (Kittywhiskers Van Gogh)
3ac3714957 merge bitcoin#21310: fix sync-up by matching notification to generated block (Kittywhiskers Van Gogh)
7b0c725c59 merge bitcoin#21008: fix zmq test flakiness, improve speed (Kittywhiskers Van Gogh)
5e87efd04b merge bitcoin#20523: deduplicate 'sequence' publisher message creation/sending (Kittywhiskers Van Gogh)
99c730f0f3 merge bitcoin#20953: dedup zmq test setup code (node restart, topics subscription) (Kittywhiskers Van Gogh)
982c1f03d4 merge bitcoin#19572: Create "sequence" notifier, enabling client-side mempool tracking (Kittywhiskers Van Gogh)
b0b4e0fa7f zmq: Make `g_zmq_notification_interface` a smart pointer (Kittywhiskers Van Gogh)
0a1ffd30b9 zmq: extend appending address to log msg for Dash-specific notifications (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* [bitcoin#19572](https://github.com/bitcoin/bitcoin/pull/19572) introduces tests in `interface_zmq.py` that validate newly introduced "sequence" reporting of all message types (`C`, `D`, `R` and `A`). The `R` message type (removed from mempool) is tested by leveraging the RBF mechanism, which isn't present in Dash.
In order to allow the tests to successfully pass, all fee bumping and RBF-specific code had to be removed from the tests. This test also involves creating a new block to test the `C` message (connected block) that would return a `time-too-new` error and fail unless mocktime has been disabled (which has been done in this test).
* When backporting [bitcoin#18309](https://github.com/bitcoin/bitcoin/pull/18309) ([dash#5728](https://github.com/dashpay/dash/pull/5728)), Dash-specific ZMQ notifications did not have those changes applied to them and that particular backport was merged upstream *after* [bitcoin#19572](https://github.com/bitcoin/bitcoin/pull/19572), meaning, for completion, the changes from [bitcoin#18309](https://github.com/bitcoin/bitcoin/pull/18309) have been integrated into [bitcoin#19572](https://github.com/bitcoin/bitcoin/pull/19572)'s backport.
As for the Dash-specific notifications, they have been covered to ensure uniformity in a separate commit.
* The ZMQ notification interface has been converted to a smart pointer in the interest of safety (this change is eventually done upstream in 8ed4ff8e05d61a8e954d72cebdc2e1d1ab24fb84 ([bitcoin#27125](https://github.com/bitcoin/bitcoin/pull/27125)))
## Breaking Changes
None expected.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [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)_
ACKs for top commit:
UdjinM6:
utACK b75e83b298
PastaPastaPasta:
utACK b75e83b298
knst:
utACK b75e83b298
Tree-SHA512: 9f860d1203bebe0914a5102f101f646873d14754830d651fb91ed0d1285a6c1a58ffc492b07d4768324d94f53171c9a4da974cf4a0b1e5c665979eace289f6f0
2eadcf2f68 partial Merge bitcoin/bitcoin#29007: test: create deterministic addrman in the functional tests (stratospher)
f95ca4ed3e fix: unify with bitcoin: removed requirement of txindex=0 (Konstantin Akimov)
bf46e7a8e1 partial Merge bitcoin/bitcoin#28799: wallet: cache descriptor ID to avoid repeated descriptor string creation (Andrew Chow)
05e5966199 partial Merge bitcoin/bitcoin#27920: wallet: bugfix, always use apostrophe for spkm descriptor ID (furszy)
Pull request description:
## Issue being fixed or feature implemented
Lately our CI for tsan is flapping many functional tests and take long times.
This PR has several important changes backported from the latest bitcoin's version to improve CI experience
## What was done?
This PR has several backports that improved CI experience drastically.
**Firstly, it aims to fix flapping test p2p_node_network_limited.py**
For example: https://gitlab.com/dashpay/dash/-/jobs/7692635307
<details>
<summary>p2p_node_network_limited.py | ✖ Failed | 28 s</summary>
```
test 2024-08-29T02:50:53.929000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_framework.py", line 158, in main
self.run_test()
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/p2p_node_network_limited.py", line 79, in run_test
self.nodes[0].disconnect_p2ps()
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_node.py", line 611, in disconnect_p2ps
wait_until_helper(check_peers, timeout=5)
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/util.py", line 262, in wait_until_helper
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def check_peers():
for p in self.getpeerinfo():
for p2p in self.p2ps:
if p['subver'] == p2p.strSubVer:
return False
return True
''' not true after 5.0 seconds
```
</details>
**Secondly, it improves performance of Descriptor wallets significantly for case of `tsan` CI**. It is tiny improvement for Release build and local runs, but some fucnctional tests run as fast as twice:
https://gitlab.com/dashpay/dash/-/jobs/7694458953https://gitlab.com/dashpay/dash/-/jobs/7665132625
```
wallet_create_tx.py --descriptors | ✓ Passed | 236 s <-- new version
wallet_create_tx.py --legacy-wallet | ✓ Passed | 108 s
wallet_basic.py --descriptors | ✓ Passed | 135 s <---- new version
wallet_basic.py --legacy-wallet | ✓ Passed | 97 s
wallet_create_tx.py --descriptors | ✓ Passed | 456 s <-- old version
wallet_create_tx.py --legacy-wallet | ✓ Passed | 98 s
wallet_basic.py --descriptors | ✓ Passed | 189 s <--- old version
wallet_basic.py --legacy-wallet | ✓ Passed | 131 s
```
See performance investigation here: https://github.com/dashpay/dash/pull/6226
## 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
ACKs for top commit:
PastaPastaPasta:
utACK 2eadcf2f68
UdjinM6:
utACK 2eadcf2f68
Tree-SHA512: 127fbaa65c160aa95e2145a6b40d3811f7c42e36fbee9ce98a9ac021abd9cbe6edc7791870b331a54855ba891e3804885db7936ef212647b693f50f79a60d232
BACKPORT NOTICE
It includes only this commit: be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac
[init] Remove -addrmantest command line arg
-addrmantest is only used in `p2p_node_network_limited.py` test to
test if the node self-advertises a hard-coded local address
(which wouldn't be advertised in the tests because it's unroutable
without the test-only code path) to check pruning-related services
are correct in that addr.
Remove -addrmantest because the self advertisement happens because
of hard coded test path logic, and expected services are nominal
due to how easily the test-only code could diverge from mainnet
logic. It's also being used only in 1 test.
0f7dc893ea1776515173dcd0bfe6826e963c90f3 test: compare `/chaininfo` response with `getblockchaininfo` RPC (brunoerg)
Pull request description:
The `/chaininfo` REST endpoint gets its infos from `getblockchaininfo` RPC, so this PR adds an `assert_equal` (in `interface_rest`) to ensure both responses are the same. Obs: other endpoints do the same for their respective RPC.
ACKs for top commit:
0xB10C:
Concept and Code Review ACK 0f7dc893ea1776515173dcd0bfe6826e963c90f3. Belts-and-spenders.
Tree-SHA512: 51cbcf988090272e406a47dc869710740b74e2222af29c05ddcbf53bd49765cdc59efb525e970867f091b3d2efec4fb13371a342d9e484e51144b760265bc5b8
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
47c48b5f35b4910fcf87caa6e37407e67d879c80 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz)
77349713b189e80f2c140db4df50177353a1cb83 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz)
86dbd54ae8a8f9c693c0ea67114bbff24a0754df test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz)
Pull request description:
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds
I noticed this improvement should probably be made when testing performance/runtimes of https://github.com/bitcoin/bitcoin/pull/22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here
ACKs for top commit:
theStack:
Tested ACK 47c48b5f35b4910fcf87caa6e37407e67d879c80
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
accf3d5868460b4b14ab607fd66ac985b086fbb3 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c24d2f0abd442c1c33098e3121572ce [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7beddfdb74c284720d209c81dfdb94f [test] helper function to increase transaction weight (glozow)
f8253d69d6f02850995a11eeb71fedc22e6f6575 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d33aa7ef87994e452bced7f192d021a0 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa139c8363e9b38bbc1ba0dca55700b8a7 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3ec080a2d7a7988c7b35088dff6c5830 [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d2bbedf9527b914c0cc8267b8a7c21b MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aaf72aab8a9c5827b1f162f3b8ac38f4 misc package validation doc improvements (glozow)
Pull request description:
This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).
Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.
#### Motivation
It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:
- We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
- We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.
ACKs for top commit:
JeremyRubin:
utACK accf3d5
ariard:
ACK accf3d5.
Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
1f4e1a17ed test: add test for governance inv expiration (UdjinM6)
06b4dba0d3 feat: make CInv python implementation aware of governance invs (UdjinM6)
c7c930ece6 fix: use correct condition in logs (UdjinM6)
d41d87a5be fix: use `std::chrono::seconds` (UdjinM6)
d6fe7146ff chore: make clang-format and linter happy (UdjinM6)
b57a9220c1 refactor: sqash 2 hash maps into one, use proper naming (UdjinM6)
4116ba3253 fix: let internal govobj/vote inv request trackers expire after 60 sec (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Another issue noticed during recent sb voting period. Lots of inv are coming from peers that never send the object/vote they announced. Let's not wait for these forever, 60 seconds should be enough.
## What was done?
Add a "timer" to each request.
## How Has This Been Tested?
Run tests, run a MN on mainnet and check logs.
## 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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 1f4e1a17ed
Tree-SHA512: f5c5a896f61b5aed27c6f42c926254156c604edb4efd2a3cd3738a7e8d1ed7bafffadc584148e4a5c1a172c2b3d61a06884a1f6d56eb4ebe37a4b0dbc050edd6
d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1c0ef2dcd307023aaaab40373c8bf17db1 test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.
Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):
| DER signature size [bytes] | #occurences (ratio) |
| ------------- | ------------- |
| 71 | 498893 (49.89%) |
| 70 | 497244 (49.72%) |
| 69 | 3837 (0.38%) |
| 68 | 22 (0.0022%) |
Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).
~~The idea of grinding signatures to a fixed size (similar to https://github.com/bitcoin/bitcoin/pull/13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~
For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816
ACKs for top commit:
MarcoFalke:
Concept ACK d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206
Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
