6bfbc97d716faad38c87603ac6049d222236d623 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack)
0997019e7681efb00847a7246c15ac8f235128d8 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow)
Pull request description:
Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.
Fixes#21104
ACKs for top commit:
jonatack:
ACK 6bfbc97d716faad38c87603ac6049d222236d623
meshcollider:
utACK 6bfbc97d716faad38c87603ac6049d222236d623
kristapsk:
ACK 6bfbc97d716faad38c87603ac6049d222236d623. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.
Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
6fa72ceb8021c3b5aea62f6cfe92665c29212923 test: add coverage for passing fee rate as a string (Jon Atack)
ce207d6b93d35bc02fcd2dd28f1fd95869261d43 wallet, bugfix: allow send to take string fee rate values (Jon Atack)
Pull request description:
RPC send currently only accepts fee rates as numbers, which is a user-facing bug. It should accept fee rates as an amount, e.g. a string or a number, as documented in its help and like sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, and bumpfee. Provide a fix and regression test coverage.
ACKs for top commit:
MarcoFalke:
review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
achow101:
Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
promag:
Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923.
Tree-SHA512: 735f9269cb1b81953764b5283449c0b154bd62de034225be5bcedc515c84faf767fe8fe0741008679fe412922c847b00d116cb11aab775236b779c847ba87167
fa69c2c78455fd0dc436018fece9ff7fc83a180d wallet: Do not treat default constructed types as None-type (MarcoFalke)
fac4e136fa3d0fab7fde900a6be921313e16e7a6 refactor: Change pointer to reference because it can not be null (MarcoFalke)
Pull request description:
Equating `0==None` and `""==None` is confusing, unneeded and undocumented
ACKs for top commit:
jonatack:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
achow101:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
Sjors:
tACK fa69c2c78455fd0dc436018fece9ff7fc83a180d modulo `unset`
Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac
9f08780dd7946b63476e9736745131db8e7f4e93 Use the correct incremental fee constant in bumpfee help (Jon Atack)
3f1e10b2b1cd11f7112fbad6355464bd4adbbc5c Update feeRate (BTC/kvB) to fee_rate (sat/vB) in wallet_bumpfee (Jon Atack)
1b3d7009280595108eb22ac1188bc4367804fc5d Allow zero-fee fundrawtxn and walletcreatefundedpsbt calls (Jon Atack)
Pull request description:
- Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525406176. A check to raise an error on zero-fee txns was mistakenly extended in a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include fundrawtransaction and walletcreatefundedpsbt. This commit re-overrides zero fee rate checking for these two RPCs, not only for the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new fee_rate (sat/vB) arg. Negative fee rates will still raise "amount out of range" by the MoneyRange check in src/bitcoin-tx.cpp::AmountFromValue.
- Updates a wallet bumpfee test from feeRate (BTC/kvB) to fee_rate (sat/vB)
- Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525405363 to use the correct incremental fee rate constant in the bumpfee help (thanks Marco Falke for the catch) and rectifies "1.000 sat/vB sat/vB" in the help to "1.000 sat/vB"
ACKs for top commit:
MarcoFalke:
review ACK 9f08780dd7946b63476e9736745131db8e7f4e93 🐾
promag:
Code review ACK 9f08780dd7946b63476e9736745131db8e7f4e93.
Xekyo:
Code review reACK 9f08780dd7946b63476e9736745131db8e7f4e93.
Tree-SHA512: 413dfb4f23ebaf3d2ef210dd04610a843272e64eabba428699f5de4d646a86ac4911dab66b5e2f5ebea53b76e4be8347ef40824c1592c750d5eaa12579d3cdf6
05e82d86b09d914ebce05dbc92a7299cb026847b wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack)
9a670b4f07a6140de809d73cbd7f3e614eb6ea74 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack)
be481b72e24fb6834bd674cd8daee67c6938b42d wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack)
449b730579566459e350703611629e63e54657ed wallet: provide valid values if invalid estimate mode passed (Jon Atack)
6da3afbaee5809ebf6d88efaa3958c505c2d71c7 wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack)
173b5b5fe07d45be5a1e5bc7a5df996f20ab1e85 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack)
7f9835a05abf3e168ad93e7195cbaa4bf61b9b07 wallet: remove fee rates from conf_target helps (Jon Atack)
b7994c01e9a3251536fe6538a22f614774eec82d wallet: add fee_rate unit warnings to bumpfee (Jon Atack)
410e471fa42d3db04e8879c71f8c824dcc151a83 wallet: remove redundant bumpfee fee_rate checks (Jon Atack)
a0d495747320c79b27a83c216dcc526ac8df8f24 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack)
e21212f01b7c41eba13b0479b252053cf482bc1f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack)
6112cf20d43b0be34fe0edce2ac3e6b27cae1bbe wallet: add CFeeRate ctor doxygen documentation (Jon Atack)
3f7279161347543ce4e997d78ea89a4043491145 wallet: fix bug in RPC send options (Jon Atack)
Pull request description:
This PR builds on #11413 and #20220 to address #19543.
- replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs
- allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument
- fix a bug in the experimental send RPC described in https://github.com/bitcoin/bitcoin/pull/20220#discussion_r513789526 where args were not being passed correctly into the options values
- update the feerate error message units for these RPCs from BTC/kB to sat/vB
- update the test coverage, help docs, doxygen docs, and some of the RPC examples
- other changes to address the excellent review feedback
See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309
ACKs for top commit:
achow101:
re-ACK 05e82d8
MarcoFalke:
review ACK 05e82d86b0 did not test and found a few style nits, which can be fixed later 🍯
Xekyo:
tACK 05e82d86b09d914ebce05dbc92a7299cb026847b
Sjors:
utACK 05e82d86b09d914ebce05dbc92a7299cb026847b
Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
0be29000c011dec0722481dbebb159873da6fa54 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be40667876c705e586849ea9c9e44cf451c wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c0050837ec65765208dd54dde354145fbe098 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d5160fc621c0299179b91403756b61d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa60313e4ae67da49e5ba4535038b71b453 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e52fdcd86c87628aa595c03a071ca8c test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1c68e74a84d868a454f508bada6b09d wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f8425a2515022d51f1f5b4911329fbf55 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f841a54ee0d9744ed7fd09034b0ddad wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d34f76f9e1ffd2e31f274ea6b22f894 wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1c9da84c474c5161b1910d3328ef0da wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)
Pull request description:
Follow-up to #11413 providing a base to build on for #19543:
- bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
- adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
- improves a few related RPC error messages and `ParseConfirmTarget()` / error message
- fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units
This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.
ACKs for top commit:
kallewoof:
Concept/Tested ACK 0be29000c011dec0722481dbebb159873da6fa54
meshcollider:
Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
fa1b63c01887adff83f16b1bbba3bd159dc51104 test: Replace hashlib.new with named constructor (MarcoFalke)
Pull request description:
A small refactor that doesn't matter too much, but it using the named constructor is nice because:
* It clarifies that it is a built-in function
* It is (trivially) faster and less code.
ACKs for top commit:
Zero-1729:
ACK fa1b63c01887adff83f16b1bbba3bd159dc51104
w0xlt:
ACK fa1b63c
Tree-SHA512: d23dc4552c1e6fc1f90f8272e47e4efcbe727f0b66a6f6a264db8a50ee6cb6d57a2809befcb95fda6725136672268633817a03dd1859f2298d20e3f9e0ca4a7f
We need to disable mocktime so that the node doesn't resort to direct
fetching. We also need to delay DIP3's activation so that blocks don't
get rejected for not having a valid DIP3-compliant coinbase.
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
fa3bdbd37be81b613e48a10aa77dfd3bcede61e1 test: remove unused sanitizer suppressions (MarcoFalke)
Pull request description:
Looks like those are not needed (anymore)
ACKs for top commit:
fanquake:
ACK fa3bdbd37be81b613e48a10aa77dfd3bcede61e1
Tree-SHA512: 4bedb6363aba8ea7763291ee0cd074e6bfd77e691bb32999c3959393864dc396bacba1eced2b10d9d600b66e8b83b91f7bc6692331dbd113bbaa87e72d11e2e8
In upcoming commits, message sending will be split off into a separate
file and stats capabilities will be fleshed out. Prepare for that by
giving it its own directory.
Also get rid of `statsd` namespace, it is entirely unnecessary.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
9bdda50151dd808cbad094d457bf0ed7939a7c87 Enable TLS in links in documentation (Jeremy Rand)
Pull request description:
This PR enables TLS in several documentation links, which improves security.
ACKs for top commit:
fanquake:
ACK 9bdda50151dd808cbad094d457bf0ed7939a7c87
Tree-SHA512: 9d04d8771a9daf3c3b9914ff324e2eabfdf3ff5ae7f7dc92b84a1f3527010ceb860e73873a8f24d6051763eb472d9ea324ccbd6129a40318a520ca88c05f0586
faa75fa19335e3e826efa4f2280609a2db34425d Avoid unsigned integer overflow in bitcoin-tx (MarcoFalke)
Pull request description:
While `npos` means "largest unsigned value" and adding `1` to it yields `0`, it may be clearer to just assign `0` to it and only increment otherwise.
This also allows to remove a file-wide suppression for `unsigned-integer-overflow`.
ACKs for top commit:
hebasto:
ACK faa75fa19335e3e826efa4f2280609a2db34425d, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK faa75fa19335e3e826efa4f2280609a2db34425d
Tree-SHA512: c24436641e5d801341c948b812c7f711d5dff70efdf04af00fd3221f4b81d93f25608dddaa36230ba81ca7ab0d18bdd957095d4561e22621e4d69017934f0a16
fa24a3df8796cbf4eeb35d950a4c848d605e5b22 rpc: Quote user supplied strings in error messages (MarcoFalke)
Pull request description:
I can't see a downside doing this and this fixes a fuzzing crash
Background:
This is a follow-up to commit 926fc2a0d4ff64cf2ff8e1dfa64eca2ebd24e090, which introduced the "starts_with-hack". Maybe an alternative to the hack would be to assign a unique error code to internal bugs? However, I think this can be done in an separate pull request and the changes here make sense even on their own.
ACKs for top commit:
fanquake:
ACK fa24a3df8796cbf4eeb35d950a4c848d605e5b22 - to fix the fuzzers.
Tree-SHA512: d998626406a64396a037a6d1fce22fce3dadb7567c2f9638e450ebe8fb8ae77d134e15dd02555326732208f698d77b0028bc62be9ceee9c43282b61fe95fccbd
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
fa7d71f270b89c9d06230d4ff262646f9ea29f4a test: Run pep-8 on touched test (MarcoFalke)
fab7e99c2a4b02a41b7448b45f0e6cdfdbb53ac3 test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
fab871f649e3da4a5a5f6cffac3fc748bb1ca900 test: Remove unused generate() from test (MarcoFalke)
faff3f35b778d9af3d649b303d7edab49bfe40b4 test: Add txin.sequence option to MiniWallet (MarcoFalke)
Pull request description:
This comes with nice benefits:
* Less code and complexity
* Test can be run without wallet compiled in
Also add some additional checks for `getmempoolentry` (#22209) and other cleanups 🎨
ACKs for top commit:
mjdietzx:
Tested ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a thanks for the explanations, nicely done
theStack:
ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a 🍷
Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a
01eedf3821f2c3ee6bab8733f8549531c844add7 test: doc: improve doc for chain_transaction() helper (Sebastian Falbesoner)
6e63e366d609312ab984b4439de2d59bb618620b test: refactor: dedup utility function chain_transaction() (Sebastian Falbesoner)
Pull request description:
Both tests `mempool_packages.py` and `mempool_package_onemore.py` define a utility function `chain_transaction` with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties:
* pass a list of parent_txids/vouts instead of single values
* always mark the BIP125-replaceable flag for txs, created via `createrawtransaction` (this is needed by the `mempool_package_onemore.py` test, but doesn't hurt the other one)
This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function `chain_transaction` in `rpc_packages.py`, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx.
ACKs for top commit:
mjdietzx:
reACK 01eedf3821f2c3ee6bab8733f8549531c844add7
klementtan:
Code review ACK 01eedf3821f2c3ee6bab8733f8549531c844add7
MarcoFalke:
review ACK 01eedf3821f2c3ee6bab8733f8549531c844add7 🙅
Tree-SHA512: ac7105d02c23f53d76d4ec9dc8de1074dd8faefeecd44b107921b78665279498966152fed312ecbe252a1c34a9643d531166329a4fea0e773df3bb75d43092b0
a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47 test: run mempool_reorg.py even with wallet disabled (Darius Parvin)
Pull request description:
Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.
As part of this PR I created a new method in `MiniWallet`, `create_self_transfer`, to return a raw tx (without broadcasting it) and its associated utxo.
ACKs for top commit:
MarcoFalke:
cr ACK a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47
Tree-SHA512: 316a38faffadcb87499c1d6eca21e9696cef65362bbffcf621788a9b771bb1fa2971b1c7835cbd34b952d7612ad83afbca824cd8be39ecd6b994e8963027f991
5a1bef60a03b57de708a1a751bd90b8245fd8b83 test: refactor: remove binascii from test_framework (Zero-1729)
Pull request description:
This PR continues the work started in PR #22593, regarding using the `bytes` built-in module. In this PR specifically, instances of `binascii`'s methods `hexlify`, `unhexlify`, and `a2b_hex` have been replaced with the build-in `bytes` module's `hex` and `fromhex` methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.
Additionally, certain changes made are based on the following assumption:
```
bytes.hex(data) == binascii.hexlify(data).decode()
bytes.hex(data).encode() == binascii.hexlify(data)
```
Ran the functional tests to ensure behaviour is still consistent and changes didn't break existing tests.
closes#22605
ACKs for top commit:
theStack:
Code-review ACK 5a1bef60a03b57de708a1a751bd90b8245fd8b83 🔢
Tree-SHA512: 8f28076cf0580a0d02a156f3e1e94c9badd3d41c3fbdfb2b87cd8a761dde2c94faa5f4c448d6747b1ccc9111c3ef1a1d7b42a11c806b241fa0410b7529e2445f
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
715bc1af66 test: check `masternode winners` before and after mn_rr (UdjinM6)
9d47cd2226 fix: adjust payee predictions after mn_rr activation (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Payment predictions in GUI are wrong when mn_rr is active, `masternode winners` RPC is affected by the same issue too. Actual payments aren't affected.
## What was done?
Adjust calculations, add tests for `masternode winners`.
## How Has This Been Tested?
Run dash-qt on testnet, check "Next Payment" on "Masternode" tab. 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
ACKs for top commit:
PastaPastaPasta:
utACK 715bc1a
Tree-SHA512: 293c77974bcb50c6f9c51449d7bb12f89ad8db5871cad3a6083fe1951fe77e0deba8de7688b2f600fabe977bdc7390a66a984a6a076be19183c23742e00e27bf
c8092fb7fc Merge bitcoin/bitcoin#25214: multiprocess build fix: ipc/capnp/init.capnp.h: No such file or directory (fanquake)
1c40b5d161 Merge bitcoin/bitcoin#25088: Wallet: Ensure m_attaching_chain is set before registering for signals (Andrew Chow)
247fe418d6 Merge bitcoin/bitcoin#24984: wallet: ignore chainStateFlushed notifications while attaching chain (Andrew Chow)
93e7c38788 Merge bitcoin/bitcoin#24338: util: Work around libstdc++ create_directories issue (fanquake)
e80c2a383a Merge bitcoin/bitcoin#24308: util: use stronger-guarantee rename method (MarcoFalke)
f3775d96b0 Merge bitcoin/bitcoin#24251: Re-enable windows path tests disabled by #20744 (fanquake)
f2f298286e Merge bitcoin/bitcoin#23707: fuzz: Fix RPC internal bug detection (MarcoFalke)
d06b693864 Merge bitcoin/bitcoin#23654: fuzz: Rework rpc fuzz target (MarcoFalke)
e45229de76 Merge bitcoin/bitcoin#23385: refactor: get wallet path relative to wallet_dir (fanquake)
9d9d97d4fd Merge bitcoin/bitcoin#22218: multiprocess: Add ipc::Context and ipc::capnp::Context structs (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
## How Has This Been Tested?
Built and tests locally
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK c8092fb7fc
UdjinM6:
utACK c8092fb7fc
Tree-SHA512: 7e3f95737b8c945e737396f130cddca24f71aa4d7f5ccb8ead62b8ba90e187af7036c1be0984e7f000a058606cc74e5381768dc2aad6e56d07a8238934198dcc
f4ba2bb769 feat: enforce DIP0001 from first block on regtest and drop fDIP0001ActiveAtTip (Konstantin Akimov)
5fa64bc4f8 feat: put brr activation to height=1 (Konstantin Akimov)
593c6cff14 feat: instant activation dip-0008 on regtest on first block (Konstantin Akimov)
cfd7ea2bc3 feat: activate DIP0020 on regtest from block 1 (Konstantin Akimov)
d1676b0280 Merge bitcoin/bitcoin#22818: test: Activate all regtest softforks at height 1, unless overridden (merge-script)
Pull request description:
## Issue being fixed or feature implemented
## What was done?
Backport bitcoin#22818 which helped to activate all forks from block-1 at regtest.
Activate next dash's softforks at block 1:
- DIP-0001 (blocksize 2mb)
- DIP-0020 (opcodes)
- DIP-0008 (chainlocks)
- BRR (block reward reallocation)
## How Has This Been Tested?
## Breaking Changes
All changes are relevant to RegTest only
## 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 f4ba2bb769
Tree-SHA512: 8d095365ff9e06ddcf47dbd457310ea2326998f0627d409651ab2fd35f6c1407cd3d2a23a4c636de359547782f4c43821944528229f3ea800cc65d3537595ea8
f4b896ef5f merge bitcoin#24266: Avoid buggy std::filesystem:::create_directories() call (Kittywhiskers Van Gogh)
b02d5e34ff merge bitcoin#30217: Update Boost download link (Kittywhiskers Van Gogh)
9e80893a3c partial bitcoin#28622: use macOS 14 SDK (Xcode 15.0) (Kittywhiskers Van Gogh)
05fb206f76 merge bitcoin#25898: remove WSL 1 workaround in fs (Kittywhiskers Van Gogh)
da5b433909 merge bitcoin#26952: Avoid `BOOST_NO_CXX98_FUNCTION_BASE` macro redefinition (Kittywhiskers Van Gogh)
7a1f48e53c merge bitcoin#25465: remove boost library detection (Kittywhiskers Van Gogh)
1d8b8900ef merge bitcoin#26557: Update Boost to 1.81.0 in depends (Kittywhiskers Van Gogh)
1ad64dadca merge bitcoin#25873: Boost 1.80.0 (Kittywhiskers Van Gogh)
d2c968bf91 merge bitcoin#25808: work around u8path deprecated-declaration warnings with libc++ (Kittywhiskers Van Gogh)
aa361b2717 merge bitcoin#24301: header-only Boost (Kittywhiskers Van Gogh)
357d1b6792 merge bitcoin#24252: Represent paths with fs::path instead of std::string (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on https://github.com/dashpay/dash/pull/6138
## 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:
UdjinM6:
utACK f4b896ef5f
Tree-SHA512: a23b97a4e2ff5749b8b964a76fab40aa670d1b2c43debdd125d9a747f7e411523b080dc222ef5e2c6dcd9b190afd757392c33eb9e003dc1408d33f52b2e0ecb6
The flag fDIP0001ActiveAtTip shows a status of activation of DIP0001
This flag is used currently only in net.cpp for setup block buffer size.
Assume the bigger by default so far as DIP0001 is activated years ago
fa4db8671bb604e11b43a837f91de8866226f166 test: Activate all regtest softforks at height 1, unless overridden (MarcoFalke)
faad1e5ffda255aecf1b0ea2152cd4f6805e678f Introduce -testactivationheight=name@height setting (MarcoFalke)
fadb2ef2fa8561882db463f35df9b8a0e9609658 test: Add extra_args argument to TestChain100Setup constructor (MarcoFalke)
faa46986aaec69e4cf016101ae517ce8778e2ac5 test: Remove version argument from build_next_block in p2p_segwit test (MarcoFalke)
fa086ef5398b5ffded86e4f0d6633c523cb774e9 test: Remove unused ~TestChain100Setup (MarcoFalke)
Pull request description:
All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
ACKs for top commit:
laanwj:
Code review ACK fa4db8671bb604e11b43a837f91de8866226f166
theStack:
re-ACK fa4db8671bb604e11b43a837f91de8866226f166
Tree-SHA512: 6397d46ff56ebc48c007a4cda633904d6ac085bc76b4ecf83097c546c7eec93ac0c44b88083b2611b9091c8d1fb8ee1e314065de078ef15e922c015de7ade8bf
e4e7c440f4 fix: use proper chain instead using ActiveChain for test framework (Konstantin Akimov)
65b92fa093 Merge bitcoin/bitcoin#21862: test: Set regtest.BIP65Height = 111 to speed up tests (fanquake)
adcf095ab9 Merge bitcoin/bitcoin#22718: doc: Add missing PR 16333 release note (MarcoFalke)
101a863399 Merge bitcoin/bitcoin#16333: test: Set BIP34Height = 2 for regtest (MarcoFalke)
71af8816ef Merge bitcoin/bitcoin#22907: test: Avoid intermittent test failure in feature_csv_activation.py (merge-script)
fc25503cbc Merge bitcoin/bitcoin#22632: test: Set regtest.BIP66Height = 102 to speed up tests (W. J. van der Laan)
cbd2be8e18 Merge bitcoin/bitcoin#22597: consensus/params: simplify ValidDeployment check to avoid gcc warning (MarcoFalke)
8928146bfa Merge bitcoin/bitcoin#22550: test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout) (MarcoFalke)
fb00431b7c Merge bitcoin/bitcoin#22385: refactor: Use DeploymentEnabled to hide VB deployments (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin related to hard-fork mechanism and accelerated action
## What was done?
see commits for backports.
Also fixed an issue of using from ChainState on RegTest: should be used specified chain, not the "best chain".
## How Has This Been Tested?
Run unit & functional tests
## Breaking Changes
That's a breaking changes for RegTest
## 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 e4e7c440f4
Tree-SHA512: c02813179177781a0da3beaff65732e12017f0056565b897891b2c5a06fda125a4e032571a02fad9d8640728df82c117393bd78bc6d406e0a30f72587b2d7344
b223c3c21e89f6af76b5401413880923f7c444d6 test: Add functional test for symlinked blocks directory (laanwj)
ddb75c2e87a60ed24065bdf0c3bfabf4e058cef1 test: Add fs_tests/create_directories unit test (Hennadii Stepanov)
1f46b6e46e1454b91ff7ceb31853bc440952f8eb util: Work around libstdc++ create_directories issue (laanwj)
Pull request description:
Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than #24266, which worked around one instance of the problem.
The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
ACKs for top commit:
jonatack:
re-ACK b223c3c21e89f6af76b5401413880923f7c444d6 per `git range-diff df08250 67019cd b223c3c`
hebasto:
re-ACK b223c3c21e89f6af76b5401413880923f7c444d6
w0xlt:
re-ACK b223c3c
vasild:
ACK b223c3c21e89f6af76b5401413880923f7c444d6
Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc
40c0e06047 fix: use coinstatsindex instead blockfilterindex in feature_prunning (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
This is follow-up changes for backports bitcoin#15946 and bitcoin#19521
It fixes failure:
TestFramework (INFO): Test invalid pruning command line options
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_node.py", line 511, in assert_start_raises_init_error
ret = self.process.wait(timeout=self.rpc_timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 1264, in wait
return self._wait(timeout=timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 2038, in _wait
raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['dashsrc/dashd', '-datadir=/tmp/dash_func_test_m8w6q7a2/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-mocktime=1417713337', '-uacomment=testnode0', '-logthreadnames', '-logsourcelocations', '-createwalletbackups=0', '-prune=550', '-blockfilterindex', '-mocktime=1417713337']' timed out after 120 seconds
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "dashtest/functional/feature_pruning.py", line 495, in run_test
self.test_invalid_command_line_options()
File "dashtest/functional/feature_pruning.py", line 149, in test_invalid_command_line_options
self.nodes[0].assert_start_raises_init_error(
File "dashtest/functional/test_framework/test_node.py", line 541, in assert_start_raises_init_error
self._raise_assertion_error(assert_msg)
File "dashtest/functional/test_framework/test_node.py", line 180, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] dashd should have exited within 120s with expected error Error: Prune mode is incompatible with -blockfilterindex.
TestFramework (INFO): Stopping nodes
## What was done?
Removed `blockfilterindex` test, enabled `coinstatsindex` test.
## How Has This Been Tested?
Run manually: `test/functional/feature_pruning.py`
This test is not run by our CI
## 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 40c0e06047
UdjinM6:
ACK 40c0e06047
Tree-SHA512: 94005c52a39cc02be8df65898844584fb247b71abab36341c3d8b4e5018902931ed8554734f27bb5d54fae2b7867ba40f136fc089fd6f9f0e57852203a5d4011
That follow-up changes for backports bitcoin#15946 and bitcoin#19521
It fixes failure:
TestFramework (INFO): Test invalid pruning command line options
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_node.py", line 511, in assert_start_raises_init_error
ret = self.process.wait(timeout=self.rpc_timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 1264, in wait
return self._wait(timeout=timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 2038, in _wait
raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['dashsrc/dashd', '-datadir=/tmp/dash_func_test_m8w6q7a2/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-mocktime=1417713337', '-uacomment=testnode0', '-logthreadnames', '-logsourcelocations', '-createwalletbackups=0', '-prune=550', '-blockfilterindex', '-mocktime=1417713337']' timed out after 120 seconds
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "dashtest/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "dashtest/functional/feature_pruning.py", line 495, in run_test
self.test_invalid_command_line_options()
File "dashtest/functional/feature_pruning.py", line 149, in test_invalid_command_line_options
self.nodes[0].assert_start_raises_init_error(
File "dashtest/functional/test_framework/test_node.py", line 541, in assert_start_raises_init_error
self._raise_assertion_error(assert_msg)
File "dashtest/functional/test_framework/test_node.py", line 180, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] dashd should have exited within 120s with expected error Error: Prune mode is incompatible with -blockfilterindex.
TestFramework (INFO): Stopping nodes
c467cfffcebb30f829eeb8160166a6b941d97ed6 test: add coverage for `purpose` arg in `listlabels` (brunoerg)
Pull request description:
This PR adds test coverage for `listlabels` command when specifying the `purpose` (send and receive).
dcdfd72861/src/wallet/rpc/addresses.cpp (L698-L704)
ACKs for top commit:
kristapsk:
ACK c467cfffcebb30f829eeb8160166a6b941d97ed6
Tree-SHA512: 7e7143c1264692f7b22952e7c70dbe9ed3f5dcd2e3b69962a47be9f9c21b3f4a9089ca87962fbc8ff9116e7d2dbeb7f36d6a132c9ac13724a255cfe1b32373a8
a3f5e541523a843e834df1858e16f89188fe19a2 test: Drop no longer needed `race:epoll_ctl` TSan suppression (Hennadii Stepanov)
Pull request description:
The removed suppression seems no needed.
I cannot point the exact commit/PR which makes this change possible.
Top commit has no ACKs.
Tree-SHA512: 8ee79cbdb2bc62796d72c69be4a818379132eae47be33951e8b9d224b049ff77e867004801c7cb0cc564a5374f318dafd9142b5231e9bd428f80acc75253933e
2ef5294a5bb68ceb3797d2638567a172cc21699f rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff7b2f5e2820993443a6f868f6b0b20a test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
Pull request description:
The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
Fix all issues.
ACKs for top commit:
brunoerg:
ACK 2ef5294a5bb68ceb3797d2638567a172cc21699f
Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
faf7e485e901d6c72db5d969b526fa148060a003 Set regtest.BIP65Height = 111 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 65. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 65, which is enforced on mainnet for all new blocks.
ACKs for top commit:
theStack:
re-ACK faf7e485e901d6c72db5d969b526fa148060a003 📍
Zero-1729:
re-ACK faf7e485e901d6c72db5d969b526fa148060a003
kristapsk:
ACK faf7e485e901d6c72db5d969b526fa148060a003
Tree-SHA512: 79a8263e7233838666b9b636b496a8b9eb12398c779f9434677e1d62816732c0a7c7b3e73965be1fb0038d35e05e5a90e665bd74e9610104127dfc4ea38169bf
222290f54388270937cb6c174195717e2214ec0d test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be478f0323eafa1d560ea2c56f04fb23 test: Create all blocks with version 4 or higher (MarcoFalke)
Pull request description:
BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.
I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)
This pull is done in two commits:
* test: Create all blocks with version 4 or higher:
Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.
* test: Set BIP34Height = 2 for regtest:
This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed
ACKs for top commit:
ajtowns:
ACK 222290f54388270937cb6c174195717e2214ec0d
jonatack:
ACK 222290f54388270937cb6c174195717e2214ec0d tested and reviewed rebased to current master 5e213822f86d
theStack:
Tested ACK 222290f54388270937cb6c174195717e2214ec0d
Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
fa676dbac87919061de9f82bce65e373e8d85bd1 test: pep-8 whitespace (MarcoFalke)
faed284eabb250a07331dfca22bb8f96a95c72ea test: Avoid intermittent test failure in feature_csv_activation.py (MarcoFalke)
Pull request description:
Otherwise there will be disconnects if the test runs longer than the default peertimeout (60s):
```
node0 2021-09-05T20:28:30.973116Z (mocktime: 2021-09-01T07:17:29Z) [net] [net.cpp:1323] [InactivityCheck] socket receive timeout: 393061s peer=0
```
Fix that by skipping `InactivityCheck` via a large `-peertimeout`.
ACKs for top commit:
fanquake:
ACK fa676dbac87919061de9f82bce65e373e8d85bd1
Tree-SHA512: 061c0585a805aa2f8e55c4beedd4b8498a2951f33d60aa3632dda0a284db3a627d14a23dbd57e8a66c69a1612f39418e3a755c8ca97f6ae1105c0d70f0d1a801
fafe896a0b870d85250927bd5374caf73d379468 test: Set regtest.BIP66Height = 102 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 66. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 66, which is enforced on mainnet for all new blocks.
ACKs for top commit:
GeneFerneau:
Concept + code review ACK [fafe896](fafe896a0b)
0xB10C:
crACK fafe896a0b870d85250927bd5374caf73d379468
laanwj:
ACK fafe896a0b870d85250927bd5374caf73d379468
Zero-1729:
tACK fafe896
kristapsk:
ACK fafe896a0b870d85250927bd5374caf73d379468. Full functional test suite showed few second speed incrase on my laptop (although I didn't do proper benchmarking with multiple runs, just single `time ./test/functional/test_runner.py` on current master vs this PR).
theStack:
Tested ACK fafe896a0b870d85250927bd5374caf73d379468
hg333:
tACK fafe896a0b
Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
BACKPORT NOTICE:
we keep and maintain cppcheck linter as lint-cppcheck-dash.sh
efae252f3072da598160670691757a0d60b9beb4 test: Remove extended lint (cppcheck) (laanwj)
Pull request description:
These are unreferenced in the CI and documentation, and have been since 2019 (see #17549).
I'm not sure the cppcheck is worthwhile. It takes a long time to run (I think this is why it isn't in the normal lints), and right
now it only appears to find implicit constructors. The list of exceptions is out of date. But if anyone wants to bring it back at any
time in the future they can do so from git history (and port it to Python).
ACKs for top commit:
fanquake:
ACK efae252f3072da598160670691757a0d60b9beb4
Tree-SHA512: 1a770b5d20ff1199d0d6bc471ae3d2c3438f0f0b169ce8d2fe73480daf8d3a7146c066b799afc90aa7898982c5fee79c1daca10e16e2bff0a7b38850aedd55b2
f4cb0fbfe1 fix: no need to relay quorum commitment in case of block undo (Konstantin Akimov)
0431a33919 fix: follow-up changes for bitcoin#14193. (Konstantin Akimov)
86b76d19b6 Merge bitcoin/bitcoin#21812: ci: Enable D_GLIBCXX_DEBUG for multiprocess task (fanquake)
334496ea7e Merge bitcoin/bitcoin#21775: p2p: Limit m_block_inv_mutex (MarcoFalke)
23b83109ea Merge bitcoin/bitcoin#21750: net: remove unnecessary check of CNode::cs_vSend (MarcoFalke)
b34514191f Merge bitcoin/bitcoin#21738: test: Use clang-12 for ASAN, Add missing suppression (fanquake)
3411577473 Merge bitcoin/bitcoin#19160: multiprocess: Add basic spawn and IPC support (W. J. van der Laan)
970048d917 fix: missing changes from bitcoin#19267 - run multiprocess on CI (Konstantin Akimov)
f2b7ee73db fix: follow-up bitcoin#15402 - removed dead code (Konstantin Akimov)
274068cdbc fix: follow-up bitcoin/bitcoin#21732 - minor missing typo (MarcoFalke)
e9450a8b36 Merge #21669: test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke)
ef92c3065c Merge #21663: ci: Fix macOS brew install command (W. J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
Just regular backports from v22
## What was done?
See commits for backports.
Also there're 2 bugs are fixed which became visible after backporting bitcoin#21775 - both are related to possible deadlocks in net_processing
## How Has This Been Tested?
Run unit and functional tests. Enabled multiprocess builds on CI
## 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 f4cb0fbfe1
PastaPastaPasta:
utACK f4cb0fbfe1
Tree-SHA512: 3204c2aa243fa4834ccf4ff4672d183cf9b35f87b857df8543572cd685729e15fca39f85b27194233e57cbc1746e36b556efab95ce20d0aa0a7d4476a9f3c6c0
92834d1ef2 fix: help p2p_timeouts to succeed on the my localhost (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Locally on my environment the functional tests `p2p_timeouts.py` fails in 80% runs.
Output:
```
stdout:
2024-08-08T05:02:32.216000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:35.079000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
'''
2024-08-08T05:02:35.080000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "DASH/test/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "DASH/test/functional/p2p_timeouts.py", line 93, in run_test
no_verack_node.wait_for_disconnect(timeout=1)
File "DASH/test/functional/test_framework/p2p.py", line 495, in wait_for_disconnect
self.wait_until(test_function, timeout=timeout, check_connected=False)
File "DASH/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "DASH/test/functional/test_framework/util.py", line 267, in wait_until_helper
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
''' not true after 1.0 seconds
2024-08-08T05:02:35.581000Z TestFramework (INFO): Stopping nodes
2024-08-08T05:02:36.582000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0/test_framework.log
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Hint: Call DASH/test/functional/combine_logs.py '/tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0' to consolidate all logs
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z 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-08T05:02:36.582000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
stderr:
TEST | STATUS | DURATION
p2p_timeouts.py | ✓ Passed | 4 s
p2p_timeouts.py | ✖ Failed | 4 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 7 s
ALL | ✖ Failed | 37 s (accumulated)
Runtime: 7 s
```
## What was done?
Increased a timeout to see for first disconnect event +1 second.
## How Has This Been Tested?
100% succeed:
```
$ test/functional/test_runner.py -j20 p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py p2p_timeouts.py
p2p_timeouts.py | ✓ Passed | 4 s
p2p_timeouts.py | ✓ Passed | 5 s
p2p_timeouts.py | ✓ Passed | 5 s
p2p_timeouts.py | ✓ Passed | 6 s
p2p_timeouts.py | ✓ Passed | 6 s
p2p_timeouts.py | ✓ Passed | 7 s
p2p_timeouts.py | ✓ Passed | 7 s
p2p_timeouts.py | ✓ Passed | 8 s
p2p_timeouts.py | ✓ Passed | 8 s
p2p_timeouts.py | ✓ Passed | 9 s
p2p_timeouts.py | ✓ Passed | 9 s
p2p_timeouts.py | ✓ Passed | 10 s
p2p_timeouts.py | ✓ Passed | 10 s
p2p_timeouts.py | ✓ Passed | 11 s
p2p_timeouts.py | ✓ Passed | 11 s
p2p_timeouts.py | ✓ Passed | 12 s
```
## 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:
ACK 92834d1ef2
PastaPastaPasta:
utACK 92834d1ef2
Tree-SHA512: 598178fd97e82def16b32cbaf1f476d3416768456a7f92fb4faadc041b73147cc7be3e6760287bde22d1a3e5a5a9190124ede6da81a1722feba1e80fcc3ae4e3
bitcoin#25454 introduces a 10 point penalty for remitting more than
MAX_BLOCKS_TO_ANNOUNCE unconnected block headers. Whether they are
connected or not is determined by taking the first entry and running
its hashPrevBlock through LookupBlockIndex. This new behaviour causes a
test failure in p2p_dos_header_tree.py in Dash.
Bitcoin doesn't face a test failure with this new behaviour as the first
non-fork block in its test data is the dump for block 1 (00000000b873e7
9784647a6c82962c70d228557d24a747ea4d1b8bbe878e1206) but Dash uses block
0 (00000bafbc94add76cb75e2ec92894837288a481e5c005f6563d91623bf8bc2c),
the genesis block.
By definition of a genesis block, it has a hashPrevBlock of 0, which
cannot be looked up. This trips the penalty. This doesn't cause any
problems in the field as nobody is expected to ever broadcast the
genesis block but it does cause a test failure for us.
We need to correct that by getting rid of the genesis block from the
test data.
12f094ec215aacf30e4e380c0399f80d4e45c345 test: use constants for CSV/CLTV activation heights in rpc_signrawtransaction (Sebastian Falbesoner)
746f203f1950a7df50b9a7de87a361cc7354ffb4 test: introduce `generate_to_height` helper, use in rpc_signrawtransaction (Sebastian Falbesoner)
e3237b1cd07a5099fbb0108218194eb653b6a9f3 test: check that CSV/CLTV are active in rpc_signrawtransaction (Sebastian Falbesoner)
Pull request description:
This PR primarily aims to solve the current RPC timeout problem for test rpc_signrawtransaction.py, as described in #22542. In the course of that the test is also improved in other ways (see https://github.com/bitcoin/bitcoin/pull/22542#pullrequestreview-714297804).
Reviewers guideline:
* In `test_signing_with_cltv()`, a comment is fixed -- it wrongly referred to CSV, it should be CLTV.
* As preparation, assertions are added that ensure that CSV and CLTV have been really activated after generating blocks by checking the 'softforks' output of the getblockchaininfo() RPC. Right now in master, one could remove (or decrease, like in #22542) the generate calls and the test would still pass, when it shouldn't.
* A helper `generate_to_height()` is introduced which improves the previous way of reaching a block height in two ways:
- instead of blindly generating TH blocks to reach target block height >= TH, the current block height CH is taken into account, and only (TH - CH) are generated in total
- to avoid potential RPC timeouts, the block generation is split up into multiple generatetoaddress RPC calls ([as suggested by laanwj](https://github.com/bitcoin/bitcoin/pull/22542#issuecomment-886237866)); here chunks of 200 blocks have been chosen
* The helper is used in the affected sub-tests, which should both speed-up the test (from ~18s to ~12s on my machine) and avoid potential timeouts
* Finally, the activation constants for CSV and CLTV are used instead of using magic numbers 500 and 1500
Open questions:
* Any good naming alternatives for `generate_to_height()`? Not really happy with the name, happy to hear suggestions
* Where to put the CSV and CLTV activation height constants in the test_framewor folder? I guess importing constants from other tests isn't really the desired way to go
ACKs for top commit:
laanwj:
Code review and tested ACK 12f094ec215aacf30e4e380c0399f80d4e45c345
rajarshimaitra:
reACK 12f094ec21
Tree-SHA512: 14509f6d3e5a5a05d6a30a3145bb82cd96a29d9d8a589abf1944a8bf34291cde78ce711195f52e9426588dc822b3618ec9b455e057360021ae46152bb7613516
stdout:
2024-08-08T05:02:32.216000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:35.079000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
'''
2024-08-08T05:02:35.080000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "DASH/test/functional/test_framework/test_framework.py", line 159, in main
self.run_test()
File "DASH/test/functional/p2p_timeouts.py", line 93, in run_test
no_verack_node.wait_for_disconnect(timeout=1)
File "DASH/test/functional/test_framework/p2p.py", line 495, in wait_for_disconnect
self.wait_until(test_function, timeout=timeout, check_connected=False)
File "DASH/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "DASH/test/functional/test_framework/util.py", line 267, in wait_until_helper
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
''' not true after 1.0 seconds
2024-08-08T05:02:35.581000Z TestFramework (INFO): Stopping nodes
2024-08-08T05:02:36.582000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0/test_framework.log
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z TestFramework (ERROR): Hint: Call DASH/test/functional/combine_logs.py '/tmp/test_runner_∋_🏃_20240808_120217/p2p_timeouts_0' to consolidate all logs
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
2024-08-08T05:02:36.582000Z 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-08T05:02:36.582000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-08-08T05:02:36.582000Z TestFramework (ERROR):
stderr:
TEST | STATUS | DURATION
p2p_timeouts.py | ✓ Passed | 4 s
p2p_timeouts.py | ✖ Failed | 4 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 5 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 6 s
p2p_timeouts.py | ✖ Failed | 7 s
ALL | ✖ Failed | 37 s (accumulated)
Runtime: 7 s
38ecd6f951 test: reduce BRRHeight on regtest (Odysseas Gabrielides)
Pull request description:
## Issue being fixed or feature implemented
In regtest, Block Reward Reallocation is buried at height 2500, which happens after v20 activation.
## What was done?
Reduced BRRHeight in regtest from 2500 to 1000.
The purpose of this change is to simplify regtests of Platform as well.
Note: This change affects only regtest.
## How Has This Been Tested?
tests
## Breaking Changes
no, this only affects Regtest (where we make no guarantees about breaking stuff)
## 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)_
Top commit has no ACKs.
Tree-SHA512: b5a1c2b2c2b70682f266d9d0af9e048a03417c0cb2480eb5ab5838965342b6465acd10d8dac5a0d3c5c5f59f4e36ac5b909a838bc3805c2265a83776e92b4827
0f239203a8 partial bitcoin#24169: Add --enable-c++20 option (Kittywhiskers Van Gogh)
a3b79267e0 merge bitcoin#20744: Use std::filesystem. Remove Boost Filesystem & System (Kittywhiskers Van Gogh)
be7ac493d0 merge bitcoin#24167: consistently use fsbridge:: for ifstream / ofstream (Kittywhiskers Van Gogh)
7ffea4348f merge bitcoin#24104: Make compatible with boost 1.78 (Kittywhiskers Van Gogh)
7c270e6883 merge bitcoin#24026: Block unsafe std::string fs::path conversion copy_file calls (Kittywhiskers Van Gogh)
b0d2484a0b merge bitcoin#23522: Improve fs::PathToString documentation (Kittywhiskers Van Gogh)
20d359b570 partial bitcoin#23469: Remove Boost build note from build-unix.md (Kittywhiskers Van Gogh)
193f6fde2e merge bitcoin#23446: Mention that BerkeleyDB is for legacy wallet in build-unix (Kittywhiskers Van Gogh)
ecfac10b8e merge bitcoin#22937: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method (Kittywhiskers Van Gogh)
23fe7e2f07 chore: dashify symbols in some unit tests (Kittywhiskers Van Gogh)
28b96a071d merge bitcoin#22840: fix unoptimized libraries in depends (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on https://github.com/dashpay/dash/pull/6085
* Depends on https://github.com/dashpay/dash/pull/6137
* Dependency for https://github.com/dashpay/dash/pull/6150
## 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:
knst:
utACK 0f239203a8
PastaPastaPasta:
utACK 0f239203a8
Tree-SHA512: 4b8f0ae55185ece27d8084a5339196b7ed993c8138f4c59a0db3e16729d4edf4a59a68a8c7309c32df57734c07182821c4878b55c253da5df763204ad7158426
21f174aff1 feat: improve query categorisation in Qt App (Konstantin Akimov)
c863473286 test: add spending asset unlock tx in functional tests (Konstantin Akimov)
1fb67ece0e feat: make a support of Qt app to show Platform Transfer transaction as a new type of transaction (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Transfers from platform have incorrectly shown amount in Dash Core wallet app.
They also shown in Qt app as self-send that is not completely true.
## What was done?
Added new type of transaction to Qt App, added a filter for its type, fixed calculation of output for tx records.
As well added a new type of transaction `platform-transfer` in rpc output of `gettransaction` RPC
## How Has This Been Tested?
Make a Platform Transfer transaction on RegTest and check it in Dash Core
Helper to see it: export dpath=/tmp/dash_func_test_PATHPATH/ ; src/qt/dash-qt -regtest -conf=$dpath/node0/dash.conf -datadir=$dpath/node0/ -debug=0 -debuglogfile=/dev/stdout
## Breaking Changes
There's new type of transaction "platform-transfer" in rpc output of `gettransaction`.
**This PR DOES NOT change any consensus rules.**
Breaking changes that makes withdrawal transaction immature is moved to https://github.com/dashpay/dash/pull/6128
## 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
Top commit has no ACKs.
Tree-SHA512: ec2a54a910f121ad30ff8e94cf17080b5b3c651872e9bc3de9ec0924ca7f7a0e526b74b05cde26aaf860e3809e67f66142112319a69c216527e5bcb1b8a2b8f6
f22ade31b9 tests: more strict test for withrawal 1000 and minor improvements (UdjinM6)
4ad18f64f5 fix: properly test hard limit of 1000 dash (Konstantin Akimov)
a2fe2b27d9 test: minor improvements for credit pool functional test (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
DIP for Credit Pool says:
```
The withdrawal should not be mined if:
* It requests more DASH than the credit pool contains
* It requests more than 1000 DASH
* The credit pool contains more than 1000 DASH, and the withdrawal would result in more than a 10% reduction in the credit pool over the 576-block window
* The credit pool contains less than 1000 DASH, and the withdrawal would result in more than 100 DASH being removed from the pool over the 576-block window
```
Though, current functional test for asset locks improperly test this case, because threshold for big withdrawal happens by 10%, not 1000 dash.
## What was done?
Improvements for functional asset lock test to actually test a limit 1000 dash, not just 10%
## How Has This Been Tested?
See changes
## Breaking Changes
N/A, changes only for tests
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] 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 f22ade31b9
Tree-SHA512: 2fdbfa85a3fc41683d68d1577916178ad686ccf0fba6abb22dc84a7ad69e0d44f876e371a24935c5167baa5491000662cc98cc1cd205e3817f0ffc65d2b4953d
b73f48f3b9 Merge bitcoin/bitcoin#22057: test: use MiniWallet (P2PK mode) for feature_dersig.py (MarcoFalke)
d5a8d5e6a0 Merge bitcoin/bitcoin#22048: test: MiniWallet: introduce enum type for output mode (MarcoFalke)
f4cd20b115 Merge bitcoin/bitcoin#21945: test: add P2PK support to MiniWallet (MarcoFalke)
7be6db6dca docs: add an explanation for vsize in MiniWallet (Konstantin Akimov)
5d10b41302 Merge bitcoin/bitcoin#21840: test: Misc refactor to get rid of &foo[0] raw pointers (MarcoFalke)
7522ee9868 Merge bitcoin/bitcoin#21900: test: use MiniWallet for feature_csv_activation.py (MarcoFalke)
c6f603c26f Merge bitcoin/bitcoin#21897: rpc: adjust incorrect RPCHelpMan types (MarcoFalke)
1dffe3ab9f Merge bitcoin/bitcoin#21867: test: use MiniWallet for p2p_blocksonly.py (MarcoFalke)
81d21eea14 Merge #21557: test: small cleanup in RPCNestedTests tests (MarcoFalke)
cc169c2457 partial Merge #20842: docs: consolidate typo & url fixing (MarcoFalke)
2be1604405 Merge #20459: rpc: Fail to return undocumented return values (MarcoFalke)
Pull request description:
## What was done?
Backports from v22 bitcoin.
Mostly related to MiniWallet and RPC improvements, 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
ACKs for top commit:
UdjinM6:
utACK b73f48f3b9
PastaPastaPasta:
utACK b73f48f3b9
Tree-SHA512: 588f3a30697c0d77dadcc463aba71a00bf26eeef41b0cb8b9197799a217ebeb1d1ce7b5021ccc4576f0e9ca0e75ad840820cdc682fe8f120596788a528727a0b
764b3a3239 test: disable mocktime in p2p_eviction.py (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
No idea why CI has no issues but `p2p_eviction.py` fails locally after #6103 (my guess is that it's because P2PInterface can't work with mocktime properly).
## What was done?
Disable mocktime in `p2p_eviction.py`
## How Has This Been Tested?
Run tests locally
## 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)_
Top commit has no ACKs.
Tree-SHA512: a9be9032c7697ff47b2256395f0fb126deeccd9bee6f101a71a1f88e1f25b08fa039ed5eb4cd4b1b308e8136d64510a544b7019ed9147ea2e80f8cb83ff25412
84934bf70e11fe4cda1cfda60113a54895d4fdd5 multiprocess: Add echoipc RPC method and test (Russell Yanofsky)
7d76cf667eff512043a28d4407cc89f58796c42b multiprocess: Add comments and documentation (Russell Yanofsky)
ddf7ecc8dfc64cf121099fb047e1ac871de94f4c multiprocess: Add bitcoin-node process spawning support (Russell Yanofsky)
10afdf0280fa93bfffb0a7665c60dc155cd84514 multiprocess: Add Ipc interface implementation (Russell Yanofsky)
745c9cebd50fea1664efef571dc1ee1bddc96102 multiprocess: Add Ipc and Init interface definitions (Russell Yanofsky)
5d62d7f6cd48bbc4e9f37ecc369f38d5e1e0036c Update libmultiprocess library (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR adds basic process spawning and IPC method call support to `bitcoin-node` executables built with `--enable-multiprocess`[*].
These changes are used in https://github.com/bitcoin/bitcoin/pull/10102 to let node, gui, and wallet functionality run in different processes, and extended in https://github.com/bitcoin/bitcoin/pull/19460 and https://github.com/bitcoin/bitcoin/pull/19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket.
These changes can also be used to implement new functionality outside the `bitcoin-node` process like external indexes or pluggable transports (https://github.com/bitcoin/bitcoin/pull/18988). The `Ipc::spawnProcess` and `Ipc::serveProcess` methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test."
Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins.
Additional notes about this PR can be found at https://bitcoincore.reviews/19160
[*] Note: the `--enable-multiprocess` feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in [doc/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md)
ACKs for top commit:
fjahr:
re-ACK 84934bf70e11fe4cda1cfda60113a54895d4fdd5
ariard:
ACK 84934bf. Changes since last ACK fixes the silent merge conflict about `EnsureAnyNodeContext()`. Rebuilt and checked again debug command `echoipc`.
Tree-SHA512: 52a948b5e18a26d7d7a09b83003eaae9b1ed2981978c36c959fe9a55abf70ae6a627c4ff913a3428be17400a3dace30c58b5057fa75c319662c3be98f19810c6
3e05a57297ddc9c55604a41e50a7a94d220db7ee test: use MiniWallet (P2PK mode) for feature_dersig.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_dersig.py) to be run even with the Bitcoin Core wallet disabled. A valid DER-signature is created by using the recently introduced P2PK-Mode of the MiniWallet (#21945).
ACKs for top commit:
MarcoFalke:
cr ACK 3e05a57297ddc9c55604a41e50a7a94d220db7ee
Tree-SHA512: 0fb8da8ed8b47f68bcb57301eb4f0171a6c9e44539b7554626969347e5d6f80b3b9085f2cc160cd038a990f0d81b8b614846260fbed43b5f950d77f1b7aa81cf
6cebac598e5e85eadd60eb1274d7f33d63ce1108 test: MiniWallet: introduce enum type for output mode (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #21945 which lifted the number of MiniWallet's tx output modes from 2 to 3 (by adding P2PK Support).
Since the current way of specifying the mode on the ctor via two booleans is ugly and error-prone (see table in comment https://github.com/bitcoin/bitcoin/pull/21945#issuecomment-842526575), a new Enum type `MiniWalletMode` is introduced that can hold the following values:
- ADDRESS_OP_TRUE
- RAW_OP_TRUE
- RAW_P2PK
Also adds documentation that should guide the user on which mode is useful for what etc. with a summary table. (Can also be split up in a separate commit or shortened if that is desired, maybe it's considered to be too verbose).
ACKs for top commit:
MarcoFalke:
cr ACK 6cebac598e5e85eadd60eb1274d7f33d63ce1108
Tree-SHA512: cbbc10806d9d9e62829548094415e9f1a281cd247b9a9fc7f7f33b923c723aa03e7bc3024623a77fb1f7da4d73455fa8244840f746980d32acdad97ee12100da
4bea30169218e2f21e0c93a059966b41c8edd205 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64e83f5b8e63f12729d5f77b1c920b136e4 test: MiniWallet: add P2PK support (Sebastian Falbesoner)
Pull request description:
This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/21900#discussion_r629524841). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.
Possible follow-ups:
* Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
* Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
* Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)
ACKs for top commit:
laanwj:
Code review ACK 4bea30169218e2f21e0c93a059966b41c8edd205
Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
bd7f27d16dacf6f7de3b4f6bd052def41d9601be refactor: feature_csv_activation.py: move tx helper functions to methods (Sebastian Falbesoner)
2eca46b0aa0ecf4738500b53523d7013985b387d test: use MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Short reviewers guideline:
- Since we exclusively work with anyone-can-spend outputs here (raw scriptPubKey = OP_TRUE), signing is not needed anymore. The function `sign_transaction` and its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple `.rehash()` call is sufficient. Also, generating an address `self.nodeaddress` (and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed.
- The test repeatedly uses the same input for creating different txs (e.g. with different txversions 1 and 2). To let `MiniWallet` create a tx with a specific input, we have to call `.get_utxo()` before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (`mark_as_spent=False`). With the behaviour on master, the second call to `.get_utxo()` with the same input would fail.
- To keep the diff in the first commit short, the `miniwallet` is set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can use `self.nodes[0]` directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis.
ACKs for top commit:
laanwj:
Code review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be
MarcoFalke:
review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be 🐕
Tree-SHA512: 24fb6a0f7702bae40d5271d197119827067d4b597e954d182e4c1aa5d0fa870368eb3ffed469b26713fa8ff8eb3ecc06abc80b2449cd68156d5559e7ae8a2b11
9f767e84381d678ed24e3f7f981976f9da34971e test: use MiniWallet for p2p_blocksonly.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_blocksonly.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Note that MiniWallet creates segwit transactions by default, i.e. txid and wtxid are not identical and we have to return both from `check_p2p_tx_violation(...)`: wtxid is needed to match an expected `"received getdata for: wtx ..."` debug output, whereas the txid is needed to wait for a certain tx via `wait_for_tx(...)`.
ACKs for top commit:
jonatack:
ACK 9f767e84381d678ed24e3f7f981976f9da34971e tested with `--disable-wallet`
Tree-SHA512: f08001f02c3c310ccdf713af0ba17304368a36414f412749908bbe8c03ad1e902190b8768b79f3b4909855762f285e7ab1b627cc4f45c90b42bb097a43cb4318
BACKPORT NOTICE:
missing changes in src/test/validation_tests.cpp (signet)
1112035d32ffe73a4522226c8cb2f6a5878d3ada doc: fix various typos (Ikko Ashimine)
e8640849c775efcf202dbd34736fed8d61379c49 doc: Use https URLs where possible (Sawyer Billings)
Pull request description:
Consolidates / fixes the changes from #20762, #20836, #20810. There is no output when `test/lint/lint-all.sh` is run.
Closes#20807.
ACKs for top commit:
MarcoFalke:
ACK 1112035d32ffe73a4522226c8cb2f6a5878d3ada
Tree-SHA512: 22ca824688758281a74e5ebc6a84a358142351434e34c88c6b36045d2d241ab95fd0958565fd2060f98317e62e683323b5320cc7ec13592bf340e6922294ed78
fadea0bf371a38620b7f1f93f87d1da76d3314e0 Revert "test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles" (MarcoFalke)
fadbd9988590ba94e3fd2d87d773f3b09d73ef46 test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke)
Pull request description:
The double lock warnings appeared in #19041, but they didn't make any sense. Also, our sync module would detect double locks, if there were any.
Bumping to clang-12 allows us to remove the spurious suppressions needed to run the tests, so do that.
ACKs for top commit:
practicalswift:
cr ACK fadea0bf371a38620b7f1f93f87d1da76d3314e0 assuming CI passes and more specifically that newer Clang agrees that these TSan suppressions are no longer needed.
Tree-SHA512: c411221a4b74d0af6ca8d686639b4f40b41c15906ccbb6647e8d569d6ab088264faafe075e1ac9523d5c0024b85f15a597bb3eedc7f07d4f5816245f75cfc08b
fa5362a9a0c5665c1a4de51c3ce4758c93a9449e rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs (MarcoFalke)
Pull request description:
Wallet RPCs that allow a rescan based on block-timestamp or block-height
need to sync with the active chain first, because the user might assume
the wallet is up-to-date with the latest block they got reported via a
blockchain RPC.
ACKs for top commit:
meshcollider:
utACK fa5362a9a0c5665c1a4de51c3ce4758c93a9449e
Tree-SHA512: d4831f1f08f854f9a49fc969de86c438f856e41c2163c801a6ff36dc2f6299cb342b44663279c524a8b7ca9a50895db1243cd7d49bed79277ada857213f20a26
7e32fde912b3924fdb27ec1f658ac11fcf160b3e test: feature_cltv.py: don't return tx copies in modification functions (Sebastian Falbesoner)
9ab2ce0a6673acc7ee0f85158fc087fce0fc7dd8 test: drop unused node parameters in feature_cltv.py (Sebastian Falbesoner)
0c2139a3f160d1d443460e4c5928109a6ab8cd11 test: fix typo in feature_cltv.py (s/ctlv/cltv/) (Sebastian Falbesoner)
Pull request description:
This tiny PR cleans up the test `feature_cltv.py` in the following ways:
* fixes a typo (s/ctlv/cltv/); compared to CHECKLOCKTIMEVERIFY, CHECKTIMELOCKVERIFY probably also sounds good and you [even get some search results for it](https://www.google.com/search?q=%22CHECKTIMELOCKVERIFY%22), but it's still wrong ;)
* drops the unused "node" parameters from the tx modification functions
* don't return a copy from the tx modification functions; it's modified in-place, hence a copy is not needed and `cltv_validate(tx, ...)` looks more natural than `tx = cltv_validate(tx, ...)`
ACKs for top commit:
MarcoFalke:
review ACK 7e32fde912b3924fdb27ec1f658ac11fcf160b3e 📼
Tree-SHA512: d2e6230977442f6a511d0f7c99431a44ad3a423647f4f327ce2ce8efe78bf9616c0d2093f5e3c3550f690dcb3f625ddf53227505c01ced70227425f249c25364
0d9fdd329e81cb171d687042290f4e6b1507d7f4 test, doc: refer to the correct variable names in p2p_invalid_tx.py (aitorjs)
Pull request description:
_tx_orphan_no_fee_ and _tx_orphan_invalid_ don't exist as transactions.
Have been replaced by _tx_orphan_2_no_fee_ and _tx_orphan_2_invalid_ respectively.
**Motivation**: Comments are more accurate and easy understandable under the tests context (I think).
ACKs for top commit:
kristapsk:
utACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4
theStack:
ACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4 📃
Tree-SHA512: a4cafd931e51fe2a67085e10e9c61178c864c14982664d112b76327e040af08cd1de04eca4a8ae980fad57ba7078017ce02fc60e7658f38380e8172c2ae28b77
127b4608e9dbb8217c74c9332e82fcec8c326fa8 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6457f21596793a7149dc6dfea1dc871 util: Check if specified config file cannot be opened (nthumann)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22612.
When running e.g. `./src/bitcoind -datadir=/tmp/bitcoin -regtest -conf=/tmp/bitcoin/regtest/bitcoin.conf` and the specified config cannot be opened (doesn't exist, permission denied, ...), the initialization silently uses the default config.
As voidburn already noted:
> I can't think of a situation in which a config file is specified explicitly (in the startup options, as per service unit linked above), but inaccessible, where the fail condition should be to keep booting using defaults instead.
With this patch applied, the initialization will fail immediately, if the specified config file cannot be opened. If no config file is explicitly specified, the behavior is unchanged. This not only affects `bitcoind`, but also `bitcoin-cli` and `bitcoin-qt`.
In the example below the datadir is accessible, but the config file is not due to insufficient permissions:
```
$ ./src/bitcoind -datadir=/tmp/bitcoin -regtest --debug=1 -conf=/tmp/bitcoin/regtest/bitcoin.conf
Error: Error reading configuration file: specified config file "/tmp/bitcoin/regtest/bitcoin.conf" could not be opened.
```
ACKs for top commit:
0xB10C:
ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Zero-1729:
tACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
theStack:
Tested ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
1bf0bf492f merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh)
5c1eb67c42 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh)
c440304c85 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh)
e303a4ec45 merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh)
301163c65e merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh)
732e871a6b merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh)
b402fd57fa merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh)
a08f2f48bf merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh)
472caa048a merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh)
d69ca833df merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh)
6df927fc60 chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6078
* Dependent on https://github.com/dashpay/dash/pull/6074
* Dependent on https://github.com/dashpay/dash/pull/6083
* Dependent on https://github.com/dashpay/dash/pull/6119
* Dependency for https://github.com/dashpay/dash/pull/6138
* In [bitcoin#24050](https://github.com/bitcoin/bitcoin/pull/24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic.
* Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](37e026a038/src/spork.cpp (L44))) and upstream's usage of it to process translatable strings ([source](37e026a038/src/util/translation.h (L55-L62))).
Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore.
## 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 **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 1bf0bf492f (with one nit)
knst:
utACK 1bf0bf492f
PastaPastaPasta:
utACK 1bf0bf492f
Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
Bitcoin uses underscore in util/translation.h for translatable strings
but underscores are also a placeholder used in multiple languages, incl.
C++. The next commit will be introducing backports, one of them will be
placing util/translation.h into validation.h, which is a header used by
Dash-specific code.
This causes a conflict between the normal usage of underscore as a
placeholder and Bitcoin's usage of it as a function name, which is
reported by the Dash-specific linter. We need to exclude shadowing
warnings from the linter to account for this.
b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)
Pull request description:
The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:
f8462a6d27/test/functional/feature_cltv.py (L26-L35)
This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
* _the stack is empty_
➡️ prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
* _the top item on the stack is less than 0_
➡️ prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
* _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
* _the top stack item is greater than the transaction's nLockTime field_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
* _the nSequence field of the txin is 0xffffffff_
➡️ prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500
The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").
ACKs for top commit:
MarcoFalke:
review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
06e1fb0b170a69996a7ce1ef5203785a7bc6b278 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet)
Pull request description:
This patch includes two new format placeholders for walletnotify:
%b - the hash of the block containting the transaction (zeroed if a mempool transaction)
%h - the height of the block containing the transaction (zero if a mempool transaction)
I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes.
**Motivation**
The walletnotify option is used to be notified of new transactions relevant to the wallet of the node.
A common usage pattern is to perform afterwards additional RPC calls to determine:
1. If this is a mempool transaction or not (i.e. are there any confirmations?)
2. What block was it included in?
3. Did this transaction was seen before and is now seen again because of a fork?
All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.
Please let me know of any questions and suggestions.
ACKs for top commit:
laanwj:
ACK 06e1fb0b170a69996a7ce1ef5203785a7bc6b278
Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
bfc083e9b7 Merge #21574: Drop JSONRPCRequest constructors after #21366 (MarcoFalke)
c0cdb0488b Merge #21572: Fix wrong wallet RPC context set after #21366 (MarcoFalke)
2f7814acdd Merge #21035: Remove pointer cast in CRPCTable::dumpArgMap (MarcoFalke)
d3b1ef374c refactor: simplify implementation of RPC composite commands (Konstantin Akimov)
3270becc9b chore: add TODO to make client parsing for composite commands (Konstantin Akimov)
d55759fa79 Merge #20012: rpc: Remove duplicate name and argNames from CRPCCommand (Wladimir J. van der Laan)
1d87ce4e86 Merge #18531: rpc: remove deprecated CRPCCommand constructor (MarcoFalke)
a7e538d7ae fix: missing changes from bitcoin#19250 (Konstantin Akimov)
68c5da41dc feat: fix help message - all subcommands support it now! (Konstantin Akimov)
d3e181f516 fix: add missing client's argument parsing for RPC commands (Konstantin Akimov)
37bd4009c1 refactor: use monostate instead std::optional in CoreContext (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin v22 rpc command related
## What was done?
See commits for backports.
Also:
- refactored and significantly simplified implementation of composite commands
- added missing changes from bitcoin#19250
- fix help message for rpc `help` - all subcommands support it now
- add missing client's argument parsing for RPC commands
- CoreContext uses std::monostate instead nullopt which is best-practice for std::variant
## How Has This Been Tested?
Run unit/functional tests.
Checked autocomplete for various commands
Checked help for various commands
## 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 bfc083e9b7
UdjinM6:
utACK bfc083e9b7
Tree-SHA512: b7b586eac2f848a6808c677252a5965577bc783778fd70d3025b8d510113de2b177d423d72ea5f61ddd8905673bf3458e55810ada371ee235fbaa19de8d2d36f
69c37f4ec2 rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD` (Kittywhiskers Van Gogh)
619b640a77 wallet: unify HD chain generation in CWallet (Kittywhiskers Van Gogh)
163d31861c wallet: unify HD chain generation in LegacyScriptPubKeyMan (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
When filming demo footage for https://github.com/dashpay/dash/pull/6093, I realized that if I tried to create an encrypted blank legacy wallet and run `upgradetohd [mnemonic]`, the client would crash.
```
dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
Posix Signal: Aborted
No debug information available for stacktrace. You should add debug information and then run:
dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===
```
The expected set of operations when performing privileged operations is to first use `walletpassphrase [passphrase] [time]` to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., `upgradetohd` imports **always** need the passphrase, if encrypted).
You might have noticed that I used `upgradetohd [mnemonic]` instead of the correct syntax, `upgradetohd [mnemonic] "" [passphrase]` that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.
```
Error: Please enter the wallet passphrase with walletpassphrase first.
```
Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so `EnsureWalletIsUnlocked()` is happy, so now the following happens.
```
upgradetohd()
| Insufficient validation has allowed us to supply a blank passphrase
| for an encrypted wallet
|- CWallet::UpgradeToHD()
|- CWallet::GenerateNewHDChainEncrypted()
| We get our hands on vMasterKey by generating the key from our passphrase
| and using it to unlock vCryptedMasterKey.
|
| There's one small problem, we don't know if the output of CCrypter::Decrypt
| isn't just gibberish. Since we don't have a passphrase, whatever came from
| CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the
| vMasterKey we just got is gibberish
|- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
|- res = LegacyScriptPubKeyMan::EncryptHDChain()
| |- EncryptSecret()
| |- CCrypter::SetKey()
| This is where everything unravels, the gibberish key's size doesn't
| match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
|- assert(res)
We assume are inputs are safe so there's no real reason we should crash.
Except our inputs aren't safe, so we crash. Welp! :c
```
This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (19512988c6), trying to do the same thing would return you a vague error
```
Failed to generate encrypted HD wallet (code -4)
```
In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to `upgradetohd`, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.
```
upgradetohd()
| We've been supplied a passphrase, so we can try and validate it by
| trying to unlock the wallet with it. If it fails, we know we got the
| wrong passphrase.
|- CWallet::Unlock()
| | Before we bother unlocking the wallet, we should check if we're
| | already unlocked, if we are, we can just say "unlock successful".
| |- CWallet::IsLocked()
| | Wallet is indeed unlocked.
| |- return true;
| The validation method we just tried to use has a bail-out mechanism
| that we don't account for, the "unlock" succeded so I guess we have the
| right passphrase.
[...] (continue call chain as mentioned earlier)
|- assert(res)
Oh...
```
This pull request aims to resolve crashes caused by the above two edge cases.
## Additional Information
As this PR was required me to add additional guardrails on `GenerateNewCryptedHDChain()` and `GenerateNewHDChainEncrypted()`, it was taken as an opportunity to resolve a TODO ([source](9456d0761d/src/wallet/wallet.cpp (L5028-L5038))). The following mitigations have been implemented.
* Validating `vMasterKey` size (any key not of `WALLET_CRYPTO_KEY_SIZE` size cannot be used for encryption and so, cannot be a valid key)
* Validating `secureWalletPassphrase`'s presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
* Using `Unlock()` to validate the correctness of `vMasterKey`. (the two other instances of iterating through `mapMasterKeys` use `Unlock()`, see [here](1394c41c8d/src/wallet/wallet.cpp (L5498-L5500)) and [here](1394c41c8d/src/wallet/wallet.cpp (L429-L431)))
* `Lock()`'ing the wallet before `Unlock()`'ing the wallet to avoid the `IsLocked()` bail-out condition and then restoring to the previous lock state afterwards.
* Add an `IsCrypted()` check to see if `upgradetohd`'s `walletpassphrase` is allowed to be empty.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 69c37f4ec2
UdjinM6:
utACK 69c37f4ec2
PastaPastaPasta:
utACK 69c37f4ec2
Tree-SHA512: 4bda1f7155511447d6672bbaa22b909f5e2fc7efd1fd8ae1c61e0cdbbf3f6c28f6e8c1a8fe2a270fdedff7279322c93bf0f8e01890aff556fb17288ef6907b3e
earlier it was possible to make it all the way to `EncryptSecret`
without actually having the passphrase in hand until being told off
by `CCrypter::SetKey`, we should avoid that.
also, let's get rid of checks that `UpgradeToHD` is now taking
responsibility for. no point in checking if the wallet is unlocked
as it has no bearing on your ability to upgrade the wallet.
c72ec70fdf feat: implement governance RPCs votealias and votemany for descriptor wallets (Konstantin Akimov)
490832959d refactor: new method to generate a signing message in CGovernanceVote (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPCs `governance votemany` and `governance votealias` use forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## What was done?
Use CWallet's interfaces instead LegacyScriptPubKeyMan
## How Has This Been Tested?
Functional tests `feature_governance.py` and `feature_governance_cl.py` to run by both ways - legacy and descriptor wallets.
Run unit and functional tests.
Extra test done locally:
```diff
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
```
to flip flag descriptor wallets/legacy wallets for all functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [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
ACKs for top commit:
UdjinM6:
utACK c72ec70fdf
PastaPastaPasta:
utACK c72ec70fdf
Tree-SHA512: 2c18f0d4acb1c4d57da81bf54f0d155682f558eeb7271df7e6fe75c126ef7f047562794a6730e3ca5351abc4e2daded06b874c2ab77f9c47b840c89d8a158c9f
fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204 rpc: Remove duplicate name and argNames from CRPCCommand (MarcoFalke)
fa92912b4bb4629addcbfdfb7cc000be701614af rpc: Use RPCHelpMan for check-rpc-mappings linter (MarcoFalke)
faf835680be39811827504f77005b6603165f53e rpc: [refactor] Use concise C++11 code in CRPCConvertTable constructor (MarcoFalke)
Pull request description:
Currently, the RPC argument names are specified twice to simplify consistency linting. To avoid having to specify the argnames twice when adding new arguments, remove the linter and add an equivalent test based on RPCHelpMan.
ACKs for top commit:
laanwj:
ACK fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204
Tree-SHA512: 3f5f32f5a09b22d879f24aa67031639d2612cff481d6aebc6cfe6fd757cafb3e7bf72120b30466f59292a260747b71e57322c189d5478b668519b9f32fcde31a
faaf9c58e4aa809019d4ca12747dd47411988e37 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke)
fa19bb2cd8c575593583138a84e6bb3444d6196d remove dead rpc code (MarcoFalke)
Pull request description:
Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37
promag:
Tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37.
ryanofsky:
Code review ACK faaf9c58e4aa809019d4ca12747dd47411988e37. Two obviously good simplifications.
Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee
2db69d7b81 chore: add release notes for "quorum platformsign" (Konstantin Akimov)
283c5f89a2 feat: create new composite command "quorum platformsign" (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
It splits from #6100
With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only.
## What was done?
Implemented a new composite command "quorum platformsign"
This composite command let to limit quorum type for signing for case of whitelist.
After that old way to limit platform commands can be deprecated - #6105
## How Has This Been Tested?
Updated a functional tests to use platform signing for Asset Unlocks feature.
## 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
ACKs for top commit:
UdjinM6:
utACK 2db69d7b81
PastaPastaPasta:
utACK 2db69d7b81
Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
85abbb97b4 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778bb0 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59787 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/66https://github.com/dashpay/dash-issues/issues/65
## What was done?
Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see https://github.com/dashpay/dash/pull/6052https://github.com/dashpay/dash/pull/6051https://github.com/dashpay/dash/pull/6055 and other related PRs
This PR makes whitelist feature to be compatible with composite commands.
Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.
Platform at their side can use config such as this one (created based on shumkov's example):
```
rpc: {
host: '127.0.0.1',
port: 9998,
users: [
{
user: 'dashmate',
password: 'rpcpassword',
whitelist: null,
lowPriority: false,
},
{
username: 'platform-dapi',
password: 'rpcpassword',
whitelist: [],
lowPriority: true,
},
{
username: 'platform-drive-consensus',
password: 'rpcpassword',
whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
lowPriority: false,
},
{
username: 'platform-drive-other',
password: 'rpcpassword',
whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
],
lowPriority: true,
},
],
allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
},
```
## How Has This Been Tested?
Updated functional tests, see commits
## 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
ACKs for top commit:
UdjinM6:
LGTM, utACK 85abbb97b4
PastaPastaPasta:
utACK 85abbb97b4
Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
1840c9441a fix: drop extra pings - follow up for #18638 (UdjinM6)
264e7f9e62 Merge #18638: net: Use mockable time for ping/pong, add tests (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Split from https://github.com/dashpay/dash/pull/6102
## What was done?
So far as bitcoin#19499 is backported, bitcoin#18638 can be finished and removed related workarounds.
## 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 1840c9441a
PastaPastaPasta:
utACK 1840c9441a
Tree-SHA512: f34657c14514d6ee596175b3faf9ee44e58e8b0339939a0708d58ab2d119786830c183f9b236ed87c08ef8e1dbd031a38fc596b5aa4d38e10521658df4330e79
d3e842f605 refactor: remove dead code which has no use since composite commands are refactored (Konstantin Akimov)
58c5d431fe fix: follow-up to #6017 - enable one more assert in wallet_descriptor test (Konstantin Akimov)
dbed4a31af fix: update comment for wallet_keypool_hd due to bitcoin#17681 DNM (Konstantin Akimov)
4741bcc5c3 chore: remove outdated todo - removed by bitcoin#16898 (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Multiple TODO is reviewed and fixes in this PR
## 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
ACKs for top commit:
UdjinM6:
utACK d3e842f605
PastaPastaPasta:
utACK d3e842f605
Tree-SHA512: 25080abcce8fa3dbb4e4c71d321b24cb9d23c073e6caf75366be58623023af50d28bc1cdac4098eb78a87b4fe0f3c33d39bb06257f0590b3e42e5fcad161ea2d
0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 refactor: replace static private member method to static method (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.
This PR aims to fix this issue, as requested by shumkov for needs of platform:
> mailformed signature and can’t verify signature due to unknown quorum is the same error?
> possible to distingush ?
## What was done?
Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`
## How Has This Been Tested?
Functional test `feature_llmq_chainlocks.py` is updated
## Breaking Changes
`submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.
## 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
ACKs for top commit:
UdjinM6:
utACK 0133c9866d
PastaPastaPasta:
utACK 0133c9866d
Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2fa1153c6d76efc8113fa01b06943ece util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )
load_on_startup used to be an argument 5 but now has a number 6.
Both arguments 5 and 6 are boolean and it can confuse an user.
To prevent confusion if user is not aware about this breaking changes,
the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup.
This requirement can be removed when major amount of users updated to v21
6c5246803d test: add tests for both current and future behaviour (UdjinM6)
4e86bda4dc feat: stricter bestCLHeightDiff checks (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Current `bestCLHeightDiff` checks are too relaxed
## What was done?
Make`bestCLHeightDiff` checks stricter
## How Has This Been Tested?
Run a node on mainnet/testet, run tests
## Breaking Changes
Old nodes aren't aware of this new logic so it's activated via `mn_rr` hardfork
## 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)_
ACKs for top commit:
PastaPastaPasta:
utACK 6c5246803d
Tree-SHA512: 2028d0ceb00a2270c92831ef38488d009d0bac47be4fc6a23ac93efdcf74847f1b9e99a529863fb4e14c65f120adda4e12c5b9e084d0f667d5f0fbaf80e3701d
b23d94b14f partial bitcoin#23706: getblockfrompeer followups (Kittywhiskers Van Gogh)
7e5cc5e375 merge bitcoin#23702: Add missing optional to getblockfrompeer (Kittywhiskers Van Gogh)
c294457b52 merge bitcoin#20295: getblockfrompeer (Kittywhiskers Van Gogh)
63ac87f011 merge bitcoin#22722: update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (Kittywhiskers Van Gogh)
07e4c2cdd0 merge bitcoin#21934: Include versionbits signalling details during LOCKED_IN (Kittywhiskers Van Gogh)
960e7687d4 merge bitcoin#19651: importdescriptors update existing (Kittywhiskers Van Gogh)
1f31823fed merge bitcoin#21910: remove redundant fOnlySafe argument (Kittywhiskers Van Gogh)
69c5aa8947 merge bitcoin#21359: include_unsafe option for fundrawtransaction (Kittywhiskers Van Gogh)
169dce7e50 merge bitcoin#20286: deprecate addresses and reqSigs from rpc outputs (Kittywhiskers Van Gogh)
7cddf70c58 merge bitcoin#20867: Support up to 20 keys for multisig under Segwit context (Kittywhiskers Van Gogh)
7c59923845 merge bitcoin#18344: Fix nit in getblockchaininfo (Kittywhiskers Van Gogh)
ec0803a0f5 refactor: align `TxToUniv` argument list with upstream (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Closes https://github.com/dashpay/dash/issues/6000
* `TxToUniv`'s argument list needed to be rearranged to match upstream as closely as possible (i.e. placing Dash-specific arguments at the end of the list to allow for code to be backported unmodified, relying on default arguments instead of having to modify each invocation to insert the default argument in between).
This was due to a new `TxToUniv` variant being introduced in [bitcoin#20286](https://github.com/bitcoin/bitcoin/pull/20286)
* The maximum number of public keys in a multisig remains the same. The upper limit for bare multisigs is and always has been `MAX_PUBKEYS_PER_MULTISIG` ([source](19512988c6/src/script/interpreter.cpp (L1143-L1144))), which has always been 20 ([source](19512988c6/src/script/script.h (L28-L29))). The limit of up to 16 comes from P2SH overhead ([source](19512988c6/src/script/descriptor.cpp (L877-L880))) and that hasn't changed.
In effect, what [bitcoin#20867](https://github.com/bitcoin/bitcoin/pull/20867) does to Dash Core is change the error from "too many public keys" (as we'll be testing against the bare multisig limit) to "excessive redeemScript size" ([source](19512988c6/src/rpc/util.cpp (L223-L225))) (which is the _true_ limitation).
* Backporting [bitcoin#21934](https://github.com/bitcoin/bitcoin/pull/21934) required a minor change in the condition needed to emit `activation_height` as `has_signal` in the preceding block will evaluate true for both `STARTED` and `LOCKED_IN` while earlier it would only for `STARTED`.
* In [bitcoin#22722](https://github.com/bitcoin/bitcoin/pull/22722), a `self.stop_node` had to be added to account for the absurd fee warning that a new test condition introduces.
* [bitcoin#23706](https://github.com/bitcoin/bitcoin/pull/23706) is partial due to commits in it that rely on RPC type enforcement, which is currently not implemented in Dash Core.
* `feature_dip3_deterministicmns.py` depends on the reporting of `addresses` to validate multisigs containing expected payees. There is no replacement RPC call to report the pubkeys that compose a multisig address. In the interm, the `-deprecatedrpc=addresses` flag has been enabled to allow the test to run otherwise unmodified.
## Breaking Changes
* The following RPCs: `gettxout`, `getrawtransaction`, `decoderawtransaction`, `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`, `/rest/block` deprecated the following fields (which are no longer returned in the responses by default): `addresses`, `reqSigs`.
The `-deprecatedrpc=addresses` flag must be passed for these fields to be included in the RPC response. Note that these fields are attributes of the `scriptPubKey` object returned in the RPC response. However, in the response of `decodescript` these fields are top-level attributes, and included again as attributes of the `scriptPubKey` object.
* When creating a hex-encoded Dash transaction using the `dash-tx` utility with the `-json` option set, the following fields: `addresses`, `reqSigs` are no longer returned in the tx output of the response.
* The error message for attempts at making multisigs with >16 pubkeys will change to an "excessive redeemScript size" instead of the previous "too many public keys".
## 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 b23d94b14f
PastaPastaPasta:
utACK b23d94b14f
Tree-SHA512: 659d9ba3a0a9f8594b307a7056ab172309d5a0d9efec605bc4b345f7e0fb1032ad303af9e8f51dbd6549e82d0b738ad41eae8a5b3aebf59081f39df0d4b5ec7c
adba60924c addrman: allow for silent overwriting of inconsistent peers.dat (Kittywhiskers Van Gogh)
fbb2b51d75 merge bitcoin#26909: prevent peers.dat corruptions by only serializing once (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
[bitcoin#22762](https://github.com/bitcoin/bitcoin/pull/22762) (backported as part of [dash#6043](https://github.com/dashpay/dash/pull/6043)) did away with then-existing behaviour of overwriting `peers.dat` silently if found corrupt with the rationale of preventing situations where the wrong file is pointed at or the file is written by a higher version of Core. Alongside a change in behaviour, refactoring also took place and further changes were built on top of them.
Since then, there have been reports of an increasing number of "Corrupt data. Consistency check failed with code -5: iostream error" errors from builds based on `develop`. Reverting the pull request that introduced this change in behaviour is non-trivial due to the number of backports that build on top of the refactoring brought along with it.
Nor were any other error messages found except for the one mentioned above. The tendency for `peers.dat` to corrupt itself has also been documented upstream ([bitcoin#26599](https://github.com/bitcoin/bitcoin/issues/26599)), with the issue marked as closed with the merger of [bitcoin#26909](https://github.com/bitcoin/bitcoin/pull/26909).
Therefore, to remedy the above problem, alongside backporting [bitcoin#26909](https://github.com/bitcoin/bitcoin/pull/26909), to avoid inconvenience, instead of reverting all progress made from backporting (as the benefits of not overwriting `peers.dat` for having the wrong magic altogether, for example, is something that doesn't need to be reverted), only inconsistent `peers.dat` files will be overwritten and the action logged with no user intervention required.
## 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
- [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:
UdjinM6:
utACK adba60924c
knst:
utACK adba60924c
PastaPastaPasta:
utACK adba60924c
Tree-SHA512: 3e09e7a77c82cce333fe9f02f137485e362f7816c450aef3d18950b9fd57276b4a21cbd1fe90b3eefd62ede0947970ed367c5917930a0656833bc38c0629e408
0213fbebe6 merge bitcoin#21866: Farewell, global Chainstate! (Kittywhiskers Van Gogh)
e3687f790a test, bench: remove globals vCoins and testWallet from test and bench (Kittywhiskers Van Gogh)
0f4184cd70 refactor: drop usage of chainstate globals in spork logic (Kittywhiskers Van Gogh)
208b1c079b refactor: drop usage of chainstate globals in masternode logic (Kittywhiskers Van Gogh)
303c6bb4db refactor: drop usage of chainstate globals in llmq logic (Kittywhiskers Van Gogh)
fa20718b4f refactor: drop usage of chainstate globals in asset locks logic (Kittywhiskers Van Gogh)
21cc12c62a refactor: drop usage of chainstate globals in governance logic (Kittywhiskers Van Gogh)
a475f5f4e5 refactor: drop usage of chainstate globals in coinjoin logic (Kittywhiskers Van Gogh)
ed56dbdbc4 refactor: don't use globals to access members we can directly access (Kittywhiskers Van Gogh)
c48c0e79f3 refactor: stop using `::ChainstateActive()` in `GetBlockHash` (Kittywhiskers Van Gogh)
6abf7f8b63 refactor: stop using `::Chain`{`state`}`Active()` in `GetUTXO*` (Kittywhiskers Van Gogh)
f6f7df3731 rpc: don't use GetUTXOCoin in CDeterministicMN::ToJson() (Kittywhiskers Van Gogh)
Pull request description:
```
Thank you, I'll say goodbye soon
Though its the end of these globals, don't blame yourself now
And if its true, I will surround you and give life to a chainstate
That's our own
```
## Additional Information
* In `CDeterministicMN::ToJson()`, `collateralAddress` is extracted by finding the `scriptPubKey` of a transaction output for a masternode, originally this used `GetUTXOCoin` but doesn't work for spent tranasction outputs (as they're _not_ UTXOs), so in [dash#5607](https://github.com/dashpay/dash/pull/5607), a fallback was introduced that looks through the general transaction set if going through the UTXO set yielded nothing.
`GetUTXOCoin` accesses the active chainstate to get ahold of the UTXO set, this was done through globals. The removal of chainstate globals meant that whoever was calling `GetUTXOCoin` should have access to the chainstate handy. This is trivial in RPC code where `ToJson()` is used ([source](5baa522225/src/rpc/evo.cpp (L1286))) through `Ensure`(`Any`)`Chainman`. Not the case in Qt code ([source](5baa522225/src/qt/masternodelist.cpp (L369))), which is supposed to be given restricted access to information by the interface.
As the fallback seems to be capable of fetching UTXOs and spent outputs, we can remove the `GetUTXOCoin` method and make the fallback the only method.
* In `develop`, as of this writing, `CChainState` members `FlushStateToDisk` and {`Enforce`, `Invalidate`, `MarkConflicting`}`Block` were accessing their internals through the global, despite having direct access to them. As the globals they were calling are going to be bid farewell, they needed to be changed to access its members instead.
The reason for going the roundabout way is unknown.
* `CDSNotificationInterface` takes in a `ChainstateManager` (instead of the `CChainState` it actually requires) as at the time of interface initialization ([source](5baa522225/src/init.cpp (L1915-L1918))), the active chainstate hasn't been loaded in yet as that happens further down ([source](5baa522225/src/init.cpp (L1988-L1991))).
As `CDSNotificationInterface::InitializeCurrentBlockTip()` is called well after it is initialized, we can resolve to the active chainstate in there.
* As `GetCreditPoolDiffForBlock` requires access to `ChainstateManager` as `GetCreditPoolDiffForBlock` > `ProcessLockUnlockTransaction` > `CheckAssetLockUnlockTx` > `CheckAssetUnlockTx` > `ChainstateManager::m_blockman.LookupBlockIndex()` and `BlockAssembler` only has `CChainState`, it had to be reworked around `ChainstateManager`.
~~`CChainState` is passed as a direct argument while `ChainstateManager` can be fetched from `NodeContext`. Unlike `CTxMemPool`, which can be passed custom instances ([source](5baa522225/src/rpc/mining.cpp (L381-L382)), [source](5baa522225/src/test/util/setup_common.cpp (L391-L392))), `CChainState`'s argument value is taken from `NodeContext::chainstate.ActiveChainstate()` and since we're now accepting `ChainstateManager` wholesale, we can dispense with accepting `CChainState` as an argument.~~
~~Changes to that effect have been made.~~
AssumeUTXO introduces the need to be able to use different `CChainState`s, so this underlying assumption no longer holds true, the above described changes have been reverted. Asset locks code has been refactored to use `BlockManager` directly (which does come with the downside of needing to hold `cs_main` for longer than strictly necessary, this is why only asset locks uses `BlockManager` directly while other cases still benefit from having `ChainstateManager` as a whole).
* `CMNHFManager::ConnectManagers` will be taking in a `ChainstateManager` pointer due to the `GetSignalsStage` > `GetForBlock` > `ProcessBlock` > `extractSignals` > `CheckMNHFTx` > `ChainstateManager::m_blockman.LookupBlockIndex()` chain.
* The use of a bespoke `NodeContext` in `coinselector_tests` breaks tests if any interface call relies on a chainstate as `testNode` doesn't initialize one. For the most part, this was masked by `WalletTestingSetup` populating the chainstate globals from its own `NodeContext` even if the tests themselves preferred to use their own stripped down `testNode`.
Though, removing the chainstate globals meant that they can no longer rely on `WalletTestingSetup`'s `NodeContext` to mask the barebones `testNode` global being used in the test (specifically, `addCoins` > `listMNCollaterials` > `ChainActive()` worked because `ChainActive()` accessed `WalletTestingSetup`'s `NodeContext` but when `ChainActive()` was gone and replaced with `NodeContext::chainman.ActiveChain()`, it uses `testNode`'s `ChainstateManager`, which doesn't exist, which causes it to crash).
To remedy this, a5595b13 and 5e54aa9b from [bitcoin#23288](https://github.com/bitcoin/bitcoin/pull/23288) were adapted for the limited purpose of eliminating `testNode` and using `WalletTestingSetup`'s `NodeContext` instead. This comes with the unfortunate effect of skipping a lot of the refactoring, cleanups and optimizations done before and adapting the ones after them non-trivial.
It is therefore best recommended that the commit be reverted and changes implemented step-by-step in a pull request at some point in the future. For now, it's kept around here for the sake of this pull request, which, if merged, should prevent more chainstate globals use from leaking into the codebase.
<details>
<summary>Pre-fix crash stacktrace: </summary>
```
dash@71aecd6afb45:/src/dash$ lldb-16 ./src/test/test_dash
(lldb) target create "./src/test/test_dash"
Current executable set to '/src/dash/src/test/test_dash' (x86_64).
(lldb) r -t coinselector_tests
Process 395006 launched: '/src/dash/src/test/test_dash' (x86_64)
Running 4 test cases...
node/interfaces.cpp:711 chainman: Assertion `m_node.chainman' failed.
Process 395006 stopped
* thread #1, name = 'd-test', stop reason = signal SIGABRT
frame #0: 0x00007ffff7a7300b libc.so.6`__GI_raise(sig=<unavailable>) at raise.c:51:1
(lldb) bt
* thread #1, name = 'd-test', stop reason = signal SIGABRT
* frame #0: 0x00007ffff7a7300b libc.so.6`__GI_raise(sig=<unavailable>) at raise.c:51:1
frame #1: 0x00007ffff7a52859 libc.so.6`__GI_abort at abort.c:79:7
frame #2: 0x00005555563cba33 test_dash`assertion_fail(file="node/interfaces.cpp", line=711, func="chainman", assertion="m_node.chainman") at check.cpp:13:5
frame #3: 0x0000555555fb47aa test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(std::vector<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>, std::allocator<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>>> const&) [inlined] std::unique_ptr<ChainstateManager, std::default_delete<ChainstateManager>>& inline_assertion_check<true, std::unique_ptr<ChainstateManager, std::default_delete<ChainstateManager>>&>(val=nullptr, file=<unavailable>, line=711, func=<unavailable>, assertion=<unavailable>) at check.h:62:13
frame #4: 0x0000555555fb4781 test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(std::vector<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>, std::allocator<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>>> const&) [inlined] node::(anonymous namespace)::ChainImpl::chainman(this=0x000055555723e830)at interfaces.cpp:711:45
frame #5: 0x0000555555fb477d test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(std::vector<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>, std::allocator<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>>> const&) [inlined] node::(anonymous namespace)::ChainImpl::listMNCollaterials(this=<unavailable>)::'lambda'()::operator()() const at interfaces.cpp:788:34
frame #6: 0x0000555555fb474f test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(this=0x000055555723e830, outputs=size=0) at interfaces.cpp:788:34
frame #7: 0x00005555565bcd07 test_dash`CWallet::AddToWallet(this=0x00005555571701e0, tx=<unavailable>, confirm=<unavailable>, update_wtx=<unavailable>, fFlushOnClose=<unavailable>) at wallet.cpp:886:46
frame #8: 0x0000555555bed3ef test_dash`coinselector_tests::add_coin(wallet=0x00005555571701e0, nValue=0x00007fffffffc7c0, nAge=144, fIsFromMe=false, nInput=0, spendable=<unavailable>) at coinselector_tests.cpp:77:29
frame #9: 0x0000555555bead3e test_dash`coinselector_tests::bnb_search_test::test_method() [inlined] coinselector_tests::add_coin(nValue=0x00007fffffffc7c0, nAge=144, fIsFromMe=false, nInput=0, spendable=false) at coinselector_tests.cpp:88:5
frame #10: 0x0000555555bead20 test_dash`coinselector_tests::bnb_search_test::test_method(this=0x00007fffffffcad0) at coinselector_tests.cpp:278:5
frame #11: 0x0000555555be6607 test_dash`coinselector_tests::bnb_search_test_invoker() at coinselector_tests.cpp:138:1
```
</details>
## Breaking Changes
* Backporting `coinselector_tests` changes are now much more annoying.
* The following RPCs, `protx list`, `protx listdiff`, `protx info` will no longer report `collateralAddress` if the transaction index has been disabled (`txindex=0`).
## 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:
UdjinM6:
utACK 0213fbebe6
knst:
utACK 0213fbebe6
PastaPastaPasta:
utACK 0213fbebe6
Tree-SHA512: 839f3f5b2af018520f330c4f4727622471d6225640c98853f28c3d88c4b6c728091b5e0c35b320e82979e5cd1357902fa1212afa4b6977967f05c636a25cc3b0
c9a600e0fa fix: linkage error - message signer better to be common code rather than libconsensus (Konstantin Akimov)
8299b3b369 feat: protxregistar implementation for descriptor wallets (Konstantin Akimov)
6f45432f76 refactor: removed unused SignSpecialTxPayloadByString (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPC `protx updateregistar` uses forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
## What was done?
New method `SignSpecialTxPayloadByHash` is implemented in interface instead exporting raw private key for some address.
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## How Has This Been Tested?
Functional test `feature_dip3_deterministicmns.py` to run by both ways - legacy and descriptor wallets.
Run unit and functional tests.
Extra test done locally:
```diff
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
```
to flip flag descriptor wallets/legacy wallets for all functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [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
ACKs for top commit:
PastaPastaPasta:
utACK c9a600e0fa
Tree-SHA512: 00aea1cd9db3537b9a9dcdee096d47ea48337edeea3f52ad54aea91781678b8641ac2dd86b67f61f87e3912945bcb5361a42a3279b6c08bb8d9f096bed8fe842
3971613285 feat: functional tests for RPC getgovernanceinfo (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/63
## What was done?
It adds functional test for `getgovernanceinfo`
## How Has This Been Tested?
Run unit/functional tests
Check output of `test/functional/test_runner.py -j20 --previous-releases --coverage --extended`
```
Uncovered RPC commands:
- cleardiscouraged
- debug
- getblockheaders
- getmerkleblocks
- getpoolinfo
- voteraw
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 3971613285
Tree-SHA512: f2d93db9068a707f2b33dd841b14f266db226fc5e907f19a49cf7a856b840f0c2c1f521c462996888933bb0ed636c288db12b1d36978c124cb536ea1ab9f3892
241f073932 feat: rpc external users are comma separated list (Konstantin Akimov)
68def970ad refactor: re-order arguments options alphabetically (Konstantin Akimov)
c7efd56a07 feat: rpc external users: use 2 queues but no extra threads (Konstantin Akimov)
c575a5808a feat: change handler to '/' for external users, use only rpc user name to choose queue (Konstantin Akimov)
f1c1fd873e feat: implementation for /external handler for RPC (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
To avoid struggling to response to critical rpc requests, and split them from 3rd parties who uses a node as an external service, there are introduced one more queue of requests that will be served without throttling for instance consensus important rpcs
## What was done?
new command line arguments:
- `rpcexternaluser` - List of comma-separated usernames for JSON-RPC external connections. If not specified, there's no special queue is created, all requests in one queue
- `rpcexternalworkqueue=<n>` - Set the depth of the work queue to service external RPC calls
## How Has This Been Tested?
Functional test `rpc_platform_filter.py` is updated to test new functionality
## Breaking Changes
NA
## 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
ACKs for top commit:
UdjinM6:
utACK 241f073932
PastaPastaPasta:
utACK 241f073932
Tree-SHA512: 15b371f24f5302853b85419e2b20c29749d6aae1c98a541d7471f1d3a681643063302c2a5ecce04dfad2da9101ea69d2f08a7e0e11a28609c6011d78273c57a7
18328279ec fix: force mnsync to skip gov obj sync on reconnection (UdjinM6)
08331bb950 fix: apply suggestions (UdjinM6)
3c3489d7a1 test: add test (UdjinM6)
41ab95dbf8 feat: skip governance checks for blocks below the best chainlock (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
A node can miss governance trigger sometimes and then it would stuck not being able to sync any further. This issue can be fixed manually by resetting sync status and reconsidering the "invalid" block. However, that's inconvenient. Also, what it does under the hood is it simply disables some parts of block validation. We could do that automagically and more precise if we would trust ChainLocks instead.
## What was done?
Skip governance checks for blocks below the best known chainlock, add tests.
## How Has This Been Tested?
Run tests.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 18328279ec
PastaPastaPasta:
utACK 18328279ec
Tree-SHA512: 3cc4e2707e24b36c9f64502561667d0cb66eced7019db7941781ab1b84cfd267b3dab4684c71b059e074450ea76dc8e342744bffdd1ca1be6ccceb34b3580659
faa137eb9eac5554504b062a6dc865ca87fd572b test: Speed up rpc_blockchain.py by removing miniwallet.generate() (MarcoFalke)
fa1fe80c757df0adcbfaf41b5c5c8a468bc07b6f test: Change address type from P2PKH to P2WSH in rpc_blockchain (MarcoFalke)
fa4d8f3169e38cbdbae20258efebe7070c49f522 test: Cache 25 mature coins for ADDRESS_BCRT1_P2WSH_OP_TRUE (MarcoFalke)
fad25153f5c8e88f72cf666b16b0b0dbdc45d3b1 test: Remove unused bug workaround (MarcoFalke)
faabce7d07c5776e4116b1a7ad1f6c408a4a4e46 test: Start only the number of nodes that are needed (MarcoFalke)
Pull request description:
Speed up various tests:
* Remove unused nodes, which only consume time on start/stop
* Remove unused "bug workarounds"
* Remove the need for `miniwallet.generate()` by adding `miniwallet.scan_blocks()`. (On my system, with valgrind, generating 105 blocks takes 3.31 seconds. Rescanning 5 blocks takes 0.11 seconds.)
ACKs for top commit:
laanwj:
Code review ACK faa137eb9eac5554504b062a6dc865ca87fd572b
Tree-SHA512: ead1988d5aaa748ef9f8520af1e0bf812cf1d72e281ad22fbd172b7306d850053040526f8adbcec0b9a971c697a0ee7ee8962684644d65b791663eedd505a025
fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke)
fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a68820758f9540653920deaeae6abe79
fjahr:
Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
faff3991a9be0ea7be31685fb46d94c212c5da34 ci: Fuzz with integer sanitizer (MarcoFalke)
Pull request description:
Otherwise the suppressions file will go out of sync
ACKs for top commit:
practicalswift:
cr ACK faff3991a9be0ea7be31685fb46d94c212c5da34: patch looks correct
Tree-SHA512: 349216d071a2c5ccf24565fe0c52d7a570ec148d515d085616a284f1ab9992ce10ff82eb17962dddbcda765bbd3a9b15e8b25f34bdbed99fc36922d4161d307c
faaad1bbac46cfeb22654b4c59f0aac7a680c03a p2p: Ignore version msgs after initial version msg (MarcoFalke)
fad68afcff731153d1c83f7f56c91ecbb264b59a p2p: Ignore non-version msgs before version msg (MarcoFalke)
Pull request description:
Handshake misbehaviour doesn't cost us more than any other unknown message, so it seems odd to treat it differently
ACKs for top commit:
jnewbery:
utACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a
practicalswift:
ACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a: patch looks correct
Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff
fbffe06dad fix: suppress lint warnings for edge due to both missing epoll and kpoll (Konstantin Akimov)
b799683d60 fix: pass reference instead copy of argument in util/edge (Konstantin Akimov)
9d941aacb9 fix: removed unused assigned (Konstantin Akimov)
d9e2e47685 fix: add a missing file util/wpipe to linter lists (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Some source files in src/util is missing to specify as dash specific for linters
## What was done?
Added to list of dash's linters
## How Has This Been Tested?
Run `test/lint/lint-all.sh`
## 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 fbffe06dad
PastaPastaPasta:
utACK fbffe06dad
kwvg:
utACK fbffe06dad
Tree-SHA512: 05bd40f987a93b47ca939f37d6e5b62e2044f2acce7a6ea6c361594eace808da17b17e0149095daecd0fb5418c6cbedbc70ad07ac3b7a1156a94188b04c2fd00
fb8a4db8f6 Merge bitcoin/bitcoin#26717: test: Improve `check-doc.py` pattern (MarcoFalke)
349cad2865 Merge bitcoin/bitcoin#26708: clang-tidy: Fix `modernize-use-nullptr` in headers (MarcoFalke)
6bf786d168 Merge bitcoin/bitcoin#25735: net: remove useless call to IsReachable() from CConnman::Bind() (fanquake)
012b0b7169 Merge bitcoin/bitcoin#24258: test: check localaddresses in getnetworkinfo for nodes with proxy (MarcoFalke)
c67f527b0b Merge bitcoin-core/gui#448: Add helper to load font (Hennadii Stepanov)
8e0abeb1c1 Merge bitcoin-core/gui#345: Connection Type Translator Comments (Hennadii Stepanov)
688b66e9d1 Merge bitcoin-core/gui#266: Doc: Copyright: Fix embedded font file location (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Trivial backports
## What was done?
## How Has This Been Tested?
Built and ran tests locally; p2p_addr_relay.py fails locally. Not sure why
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK fb8a4db8f6
knst:
utACK fb8a4db8f6
Tree-SHA512: abb9469f25c6d45acea01da6d2b9cb6df94822f61d06f80e3008b32d2522016370a1e0b0c9ff95b92df22c4f227fc40f7765d76f1987eac7603155fe2d894593
89bb25d22a0e1c700dba4e3b754984c9b2b14836 test: check localaddresses in getnetworkinfo for nodes with proxy (brunoerg)
Pull request description:
This PR adds test coverage for the field `localaddresses` for `getnetworkinfo`. In this case, it verifies if this field is empty for all nodes since they are using proxy.
Reference:
515200298b/src/init.cpp (L449)
ACKs for top commit:
jonatack:
ACK 89bb25d22a0e1c700dba4e3b754984c9b2b14836
Tree-SHA512: 3c765c7060b6972c1ae5a1104734cd7669b650b5f6aa4f623f4299567732260da5083fef306a7c1e71c931f5d1396f24abad251d95c3d82b1f3ee0efee7fcd1f
1cbf3b9a53 merge bitcoin-core/gui#206: Display fRelayTxes and bip152_highbandwidth_{to, from} in peer details (Kittywhiskers Van Gogh)
239062192e merge bitcoin#20764: cli -netinfo peer connections dashboard updates (Kittywhiskers Van Gogh)
06a6f8444c merge bitcoin#25147: follow ups to #20799 (removing support for v1 compact blocks) (Kittywhiskers Van Gogh)
6274a571b7 merge bitcoin#20799: Only support version 2 compact blocks (Kittywhiskers Van Gogh)
f4ce573538 merge bitcoin#22340: Use legacy relaying to download blocks in blocks-only mode (Kittywhiskers Van Gogh)
73b8f84fdb merge bitcoin#22147: p2p: Protect last outbound HB compact block peer (Kittywhiskers Van Gogh)
2ce481849a merge bitcoin#20599: Tolerate sendheaders and sendcmpct messages before verack (Kittywhiskers Van Gogh)
799214b2c8 merge bitcoin#19776: expose high bandwidth mode state via getpeerinfo (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Version 2 of BIP152 high-bandwidth mode/compact blocks implements SegWit support.
As Dash does not implement SegWit, there has never been a need to implement v2 (and therefore, have all the code necessary to support both v1 and v2, that gets removed as part of making support v2 only).
* Despite that, the changes surrounding removing support for both versions (that in our case, do not apply as we never have supported v2) refactor the code in other ways and influence their behaviour. In the interest of upstream alignment, those changes have been backported.
* [bitcoin#19776](https://github.com/bitcoin/bitcoin/pull/19776) doesn't seem to work on its own without successive backports, specifically [bitcoin#20799](https://github.com/bitcoin/bitcoin/pull/20799), despite the latter being a later backport.
<details>
<summary>19776-only p2p_compactblocks.py run (9f2c868947cc254d021e1a9bd00eb7bc80061e81)</summary>
```
dash@825a14c32b73:/src/dash$ ./test/functional/p2p_compactblocks.py
2024-06-09T12:29:09.777000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_kb2nr5oe
2024-06-09T12:29:16.341000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2024-06-09T12:29:31.432000Z TestFramework (INFO): Testing compactblock construction...
2024-06-09T12:29:40.068000Z TestFramework (INFO): Testing compactblock requests...
2024-06-09T12:29:44.597000Z TestFramework (INFO): Testing getblocktxn handler...
2024-06-09T12:29:59.808000Z TestFramework (INFO): Testing compactblock requests/announcements not at chain tip...
2024-06-09T12:30:03.855000Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
2024-06-09T12:30:05.868000Z TestFramework (INFO): Testing reconstructing compact blocks from all peers...
2024-06-09T12:30:09.389000Z TestFramework (INFO): Testing end-to-end block relay...
2024-06-09T12:30:10.404000Z TestFramework (INFO): Testing handling of invalid compact blocks...
2024-06-09T12:30:12.418000Z TestFramework (INFO): Testing invalid index in cmpctblock message...
2024-06-09T12:30:14.384000Z TestFramework (INFO): Testing high-bandwidth mode states via getpeerinfo...
2024-06-09T12:30:16.893000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/src/dash/test/functional/test_framework/test_framework.py", line 158, in main
self.run_test()
File "./test/functional/p2p_compactblocks.py", line 849, in run_test
self.test_highbandwidth_mode_states_via_getpeerinfo()
File "./test/functional/p2p_compactblocks.py", line 791, in test_highbandwidth_mode_states_via_getpeerinfo
hb_test_node.send_and_ping(msg_block(block))
File "/src/dash/test/functional/test_framework/p2p.py", line 579, in send_and_ping
self.sync_with_ping(timeout=timeout)
File "/src/dash/test/functional/test_framework/p2p.py", line 596, in sync_with_ping
self.wait_until(test_function, timeout=timeout)
File "/src/dash/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "/src/dash/test/functional/test_framework/util.py", line 249, in wait_until_helper
if predicate():
File "/src/dash/test/functional/test_framework/p2p.py", line 484, in test_function
assert self.is_connected
AssertionError
2024-06-09T12:30:17.396000Z TestFramework (INFO): Stopping nodes
2024-06-09T12:30:18.400000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_kb2nr5oe
2024-06-09T12:30:18.401000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_kb2nr5oe/test_framework.log
2024-06-09T12:30:18.401000Z TestFramework (ERROR):
2024-06-09T12:30:18.401000Z TestFramework (ERROR): Hint: Call /src/dash/test/functional/combine_logs.py '/tmp/dash_func_test_kb2nr5oe' to consolidate all logs
2024-06-09T12:30:18.401000Z TestFramework (ERROR):
2024-06-09T12:30:18.401000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-06-09T12:30:18.402000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-06-09T12:30:18.402000Z TestFramework (ERROR):
```
</details>
<details>
<summary>20799-incl p2p_compactblocks.py run (aa116c4f0b4753b615f9483aa03adec5ee4fd655)</summary>
```
dash@825a14c32b73:/src/dash$ ./test/functional/p2p_compactblocks.py
2024-06-09T12:34:27.169000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_7d65lmhz
2024-06-09T12:34:32.695000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2024-06-09T12:34:51.288000Z TestFramework (INFO): Testing compactblock construction...
2024-06-09T12:34:55.325000Z TestFramework (INFO): Testing compactblock requests...
2024-06-09T12:34:59.861000Z TestFramework (INFO): Testing getblocktxn handler...
2024-06-09T12:35:07.460000Z TestFramework (INFO): Testing compactblock requests/announcements not at chain tip...
2024-06-09T12:35:09.503000Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
2024-06-09T12:35:11.519000Z TestFramework (INFO): Testing reconstructing compact blocks from all peers...
2024-06-09T12:35:15.039000Z TestFramework (INFO): Testing end-to-end block relay...
2024-06-09T12:35:16.055000Z TestFramework (INFO): Testing handling of invalid compact blocks...
2024-06-09T12:35:17.062000Z TestFramework (INFO): Testing invalid index in cmpctblock message...
2024-06-09T12:35:19.139000Z TestFramework (INFO): Testing high-bandwidth mode states via getpeerinfo...
2024-06-09T12:35:22.159000Z TestFramework (INFO): Stopping nodes
2024-06-09T12:35:23.163000Z TestFramework (INFO): Cleaning up /tmp/dash_func_test_7d65lmhz on exit
2024-06-09T12:35:23.163000Z TestFramework (INFO): Tests successful
```
</details>
* The backport of [bitcoin-core/gui#206](https://github.com/bitcoin-core/gui/pull/206) is a continuation of 3e8ba24c87 from [dash#5964](https://github.com/dashpay/dash/pull/5964)
* The backport of [bitcoin#20764](https://github.com/bitcoin/bitcoin/pull/20764) is a continuation of bd934c71eb from [dash#6034](https://github.com/dashpay/dash/pull/6034)
## Breaking changes
* The `getpeerinfo` RPC returns two new boolean fields, `bip152_hb_to` and `bip152_hb_from`, that respectively indicate whether we selected a peer to be in compact blocks high-bandwidth mode or whether a peer selected us as a compact blocks high-bandwidth peer.
High-bandwidth peers send new block announcements via a `cmpctblock` message rather than the usual inv/headers announcements. See BIP 152 for more details.
* Blocks-only mode will use legacy relaying instead of BIP152 high-bandwidth mode
## 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 1cbf3b9a53
PastaPastaPasta:
utACK 1cbf3b9a53
knst:
utACK 1cbf3b9a53
Tree-SHA512: 5947b622d8d57a1dc9445cd6e07d4ad690379416d0fcf04ed574269975d1beb704691a79ff081341f3c800cf11869d401f1ed90baa5449f371f9ce658f2d2e95
a93fec6f2d merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging (Kittywhiskers Van Gogh)
d1a4b14b48 merge bitcoin#23354: Introduce new V4 format addrman (Kittywhiskers Van Gogh)
7a97aabfe0 test: restore pre-bitcoin#23306 tests to validate port distinguishment (Kittywhiskers Van Gogh)
1a050d6cb4 merge bitcoin#23306: Make AddrMan support multiple ports per IP (Kittywhiskers Van Gogh)
d56702aa0c merge bitcoin#23140: Make CAddrman::Select_ select buckets, not positions, first (Kittywhiskers Van Gogh)
19b0145379 merge bitcoin#22839: improve addrman logging (Kittywhiskers Van Gogh)
3910c68028 merge bitcoin#23053: Use public methods in addrman fuzz tests (Kittywhiskers Van Gogh)
b6ec8ab6df merge bitcoin#22950: Pimpl AddrMan to abstract implementation details (Kittywhiskers Van Gogh)
236cf36d88 merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize (Kittywhiskers Van Gogh)
2420ac9e53 merge bitcoin#23041: Add addrman deserialization error tests (Kittywhiskers Van Gogh)
8aefa9b93a merge bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted (Kittywhiskers Van Gogh)
c9a645f814 merge bitcoin#22879: Fix format string in deserialize error (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6040.
* Dash already introduced support for storage of address-port pairs (referred to as "port discrimination") and allowed the usage of non-default ports in P2P with [dash#2168](https://github.com/dashpay/dash/pull/2168).
* Albeit this was only permitted for networks with `fAllowMultiplePorts` enabled (which at the time was `devnet` and as it stands currently, on every network except `mainnet`).
* Keeping in line with the above policy (discussion on lifting such restrictions on `mainnet` is outside the scope of this PR), when backporting [bitcoin#23306](https://github.com/bitcoin/bitcoin/pull/23306), changes have been made to retain the effects of `discriminate_ports`.
* This involves appending placing a `!m_discriminate_ports` condition to behaviour that otherwise would be _removed_ entirely.
* Additionally, in line with upstream backports, changes have been made that render port distinguishment _enabled_ as the new default in `addrman_tests` (the old default was to keep it _disabled_, to mirror `mainnet` and pre-change upstream behaviour).
* To ensure distinguishment _disabled_ works as expected, affected pre-backport tests were reintroduced with the `_nondiscriminate` suffix.
---
I would propose at some point to rename the flag to `ignore_port`/`suppress_port` (if not remove it altogether should the `mainnet` restriction be lifted) as discriminate (or distinguish) isn't immediately clear with if address entries will be discriminated/distinguished _using_ ports (i.e. considered) or ports will be discriminated _against_ (i.e. ignored).
## Breaking Changes
It's unclear if these backports result in serialization issues for older versions, as Dash Core technically supported address-port pairs since 0.12 and suppressed it on `mainnet` by setting zero-ing out the port ([source](19512988c6/src/addrman.cpp (L135-L138))), meaning even with port discrimination _disabled_, the serialization format should remain the same.
Regardless, following upstream backports, a new version has been introduced (v4) that marks the AddrMan format incompatible with older versions of Dash Core.
## 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
- [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:
UdjinM6:
utACK a93fec6f2d
PastaPastaPasta:
utACK a93fec6f2d
Tree-SHA512: 49b35af3e4eb660249ce9a65d5a539205d852e9c728da22dc88779f6b3b15c13cf91522896a313bfe2a91889fedf3b6b2cebdea12cc2bbe865ec3b85b6a5dfa8
6777ab73a2 Merge bitcoin-core/gui#682: Don't directly delete abandoned txes from GUI (Hennadii Stepanov)
6e1a8c1fdc Merge bitcoin/bitcoin#26628: RPC: Reject RPC requests with same named parameter specified multiple times (MarcoFalke)
7c28b01c78 Merge bitcoin/bitcoin#26666: refactor: Deleted unreachable code in httpserver.cpp (MarcoFalke)
478fe51ead Merge bitcoin/bitcoin#26100: doc: clarify that NetPermissionFlags::Implicit is only about whitelists (MarcoFalke)
69b19cbfc0 Merge bitcoin/bitcoin#26546: test: remove unused class `NodePongAdd1` (fanquake)
245df942c4 Merge bitcoin/bitcoin#26380: Revert "test: check importing wallets when blocks are pruned throw an error" (MacroFake)
3db3bd0d31 Merge bitcoin/bitcoin#24269: test: add functional test for `-discover` (Andrew Chow)
c72ef299da Merge bitcoin/bitcoin#25896: wallet: Log when Wallet::SetMinVersion sets a different minversion (Andrew Chow)
12b438cb46 Merge bitcoin/bitcoin#25925: doc: add `{import,list}descriptors` to list of descriptor RPCs (Andrew Chow)
73d64c48a9 Merge bitcoin/bitcoin#25738: depends: use a patch instead of sed in libxcb (fanquake)
1fae0c2bbb Merge bitcoin/bitcoin#25333: test: Fix out-of-range port collisions (MacroFake)
b75089667c Merge bitcoin/bitcoin#25231: ci: Install documented packages for "Win64" CI task (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
Trivial backports
## How Has This Been Tested?
Built; ran tests locally
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 6777ab73a2
Tree-SHA512: 60d68c8d0fb9875d0b2421cad97b46a9d4deb79e03ca011d66cc0595ed68bb7ad207f2fd95973a3b7a0b9bca2c2f0deaf0e1116f0312a4d9d315f009228fe485
9413ecdc66 fix: disable linter 'check-rpc-mapping.py' for composite commands (Konstantin Akimov)
f4bc19fb99 refactor: proper support for composite commands such as 'bls generate' (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
We have composite commands such as 'bls generate' that do not exist in bitcoin's implementation of rpc.
It doesn't let to backport yet bitcoin#18531 which enforced extra checks for arguments name (name of rpc and list arguments in rpc help and actual implementation must match).
## What was done?
This PR improves support of composite commands in Dash Core. New style of composite commands are applied for `bls` composite commands: `bls generate` and `bls fromsecret` as proof of concept.
Once this PR is merged, I will provide similar fixes for other "compose" rpc commands: `protx`, `masternode` (and everything else if any).
Beside better validation of arguments and command names, it improves suggest menu in Qt app (see a screenshot) for composite commands.
## How Has This Been Tested?
Run unit and functional tests. Also extra tests to conduct in qt app:
- check suggest for 'bls ....' and 'help bls ...'
- check output of next commands:
```
help bls
help bls generate
help bls from secret
bls
bls generate
bls generate 1
```
- also let's see that old-fashion composite commands are not broken:
```
help protx
help protx diff
protx diff 00000021c7604b9992254f9f1ed91de5d65eaade33c773abea63a7b0e93293ee 000000e44f9894838ebf768b464177cfce8859dcf92b0509f5c2fba774315996
```
## 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:
PastaPastaPasta:
utACK 9413ecdc66
Tree-SHA512: c8accd2f1ea1d2168d7a2bf25311c92b7839782fc300c587c7537880df5d3fbbf7e79cedb2c6fa88ab067b696994beaf7d92185ae3e64fe1a5f70ad9e8620bec
8c3ff7d52ae3314959e1e66da8718a3f0d30abaa test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca56382512df3084fce7353bf1e8b66cae61bc bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8cf27aa72ec2907342787e6fc9f94c50 rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e671e347e422d696d1cbdd9f82b2ce468 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa 🗂
kristapsk:
ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
32f8fda7d6 merge bitcoin#24991: allow startup with -onlynet=onion -listenonion=1 (Kittywhiskers Van Gogh)
e67ed92d3d merge bitcoin#25173: add coverage for unknown network in -onlynet (Kittywhiskers Van Gogh)
77efd36112 merge bitcoin#24687: Check an invalid -i2psam will raise an init error (Kittywhiskers Van Gogh)
fb1416f7cb merge bitcoin#24205: improve network reachability test coverage and safety (Kittywhiskers Van Gogh)
7cb7479829 merge bitcoin#24663: add links to doc/cjdns.md (Kittywhiskers Van Gogh)
c736ebf566 merge bitcoin#24555: create initial doc/cjdns.md for CJDNS how-to documentation (Kittywhiskers Van Gogh)
554bd24186 partial bitcoin#24468: improve -onlynet help and related tor/i2p documentation (Kittywhiskers Van Gogh)
5436b6a82d merge bitcoin#24165: extend inbound eviction protection by network to CJDNS peers (Kittywhiskers Van Gogh)
d52724d039 merge bitcoin#22834: respect -onlynet= when making outbound connections (Kittywhiskers Van Gogh)
f9d1a9a00d merge bitcoin#23077: Full CJDNS support (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on https://github.com/dashpay/dash/pull/6034
* Depends on https://github.com/dashpay/dash/pull/6035
* If `-proxy=` is given together with `-noonion` then the provided proxy will not be set as a proxy for reaching the Tor network. So it will not be possible to open manual connections to the Tor network for example with the `addnode` RPC. To mimic the old behavior use `-proxy=` together with `-onlynet=` listing all relevant networks except `onion`.
* [bitcoin#24165](https://github.com/bitcoin/bitcoin/pull/24165) has been backported _before_ [bitcoin#23758](https://github.com/bitcoin/bitcoin/pull/23758) and to account for this, minor changes were made in `src/test/net_peer_eviction_tests.cpp` (using `nTimeConnected` instead of `m_connected`). When backporting [bitcoin#23758](https://github.com/bitcoin/bitcoin/pull/23758), these changes will have to be reversed as they won't be covered by the cherry-pick diff.
* CJDNS support has been labelled as being introduced in Dash Core 21.0, in line with the milestone designation of the PR. Should `develop` be used for a new minor/patch release, `doc/cjdns.md` will have to be modified to reflect the correct version number.
## Breaking changes
No expected protocol or consensus 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 **(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:
PastaPastaPasta:
utACK 32f8fda7d6
Tree-SHA512: e23b22ca5edbe4c4abeab0bc07780303e68e7c4cc46b7697300b0837c5acd3a98649b6b03bd07a23c827bd85f64210173027b0b0eea31872c031fa4ed04eeb0c
40bdc8a6e4dcf7faf90f63b0912c18d72436e37f test: remove unused class `NodePongAdd1` (Sebastian Falbesoner)
Pull request description:
This class was introduced in commit fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 ("net: Use mockable time for ping/pong, add tests"), but actually never used.
ACKs for top commit:
stickies-v:
ACK 40bdc8a6e
Tree-SHA512: b5a6552e4f2e0b7e368a071cc53b9a8e6f5d1950565a9fda8eb1971a01d8be0541d066842723ef44174fe8189925fa36f2defb6d7bf8d104abc77de410cc4c13
e1eadaa72d6831d1d0a53ba97c215dc4cdb64436 Revert "test: check importing wallets when blocks are pruned throw an error" (Aurèle Oulès)
Pull request description:
The test doesn't pass (not detected by the normal CI, because it is an extended test):
```
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 480, in run_test
self.wallet_test()
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 361, in wallet_test
assert_raises_rpc_error(-4, "Importing wallets is disabled when blocks are pruned", self.nodes[2].importwallet, "abc")
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 145, in try_rpc
raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'Importing wallets is disabled when blocks are pruned'
error message: 'Only legacy wallets are supported by this command'.
```
So revert it for now, which will be done anyway in https://github.com/bitcoin/bitcoin/pull/24865/commits. (This commit is taken from there)
ACKs for top commit:
andrewtoth:
ACK e1eadaa72d6831d1d0a53ba97c215dc4cdb64436
Tree-SHA512: 10f556ea1aa97dc9cb64f91055977eecb9534f658170aabb4909c3e85ca6c20588c7a021219356fab678e0e2bec999d347facd00054f07a9445ad393e6353b4c
bff05bd7456d3634b0c83539293a753db6d76376 test: add functional test for -discover (brunoerg)
Pull request description:
This PR adds a functional test for `-discover`. It tests different scenarios where `localaddresses` should be empty or may contain the addresses. Obs: `localaddresses` is not always accurate, so it's not possible to ensure (100%) it will contain any addresses.
515200298b/src/init.cpp (L449)
Obs: See #24258 - It adds test coverage for this field but for nodes with proxy.
ACKs for top commit:
mzumsande:
Code review ACK bff05bd7456d3634b0c83539293a753db6d76376
achow101:
ACK bff05bd7456d3634b0c83539293a753db6d76376
rajarshimaitra:
tACK bff05bd7456d3634b0c83539293a753db6d76376
Tree-SHA512: 8782497c146bce1ba86fda6146f3847465d7069f2cb6b84f2afc8f3b43efa813442bffe7447e9ce02adee304100b60365409bf0e5d875dfb880038442feec2a6
fa7a711a30b707cbdee4435dd0a956bffb7aaccb test: Fix out-of-range port collisions (MacroFake)
Pull request description:
Otherwise the test will fail if two tests running in parallel use the same port. See https://github.com/bitcoin/bitcoin/pull/25096#discussion_r892558783 and https://github.com/bitcoin/bitcoin/pull/25312
ACKs for top commit:
dergoegge:
ACK fa7a711a30b707cbdee4435dd0a956bffb7aaccb - This gets rid of some rather arbitrary choices for ports in some of our functional tests that can cause port collisions across test runs, resulting in intermittent failures.
Tree-SHA512: ac73da8a498230b992ab12e1ee3c4ff3d868cd63c00d2c71537d156cb7c8f8be8598ec574646b17c5a44ae3ac5bb54bf29d300f054a36cec6f6ce8054a0da0a4
76279c1a37 Merge bitcoin/bitcoin#25149: refactor: Add thread safety annotation to `BanMan::SweepBanned()` (MacroFake)
6269c6f1db Merge bitcoin/bitcoin#25053: Guard `#include <config/bitcoin-config.h>` (fanquake)
d50f0b016f Merge bitcoin/bitcoin#24977: rpc: Explain active and internal in listdescriptors (fanquake)
3c44399d55 Merge bitcoin/bitcoin#24856: lint: Converting lint-assertions.sh to lint-assertions.py (laanwj)
e9f5b4b735 Merge bitcoin/bitcoin#24213: refactor: use Span in random.* (laanwj)
7e0474ac1c Merge bitcoin/bitcoin#24632: add `(none)` in -getinfo `Warnings:` if no warning returned (laanwj)
57e9b56bad Merge bitcoin/bitcoin#24145: mempool: Clear vTxHashes when mapTx is cleared (laanwj)
fe56d9b994 Merge bitcoin/bitcoin#24698: test: -peerblockfilters without -blockfilterindex raises an error (MarcoFalke)
3cabce645e Merge bitcoin/bitcoin#24472: fuzz: execute each file in dir without fuzz engine (MarcoFalke)
f5116a7d31 Merge bitcoin-core/gui#549: refactor: use std::chrono for formatDurationStr() helper (Hennadii Stepanov)
3fa8158510 Merge bitcoin/bitcoin#22317: doc: Highlight DNS requests part in tor.md (Andrew Chow)
72b62edd5a Merge bitcoin/bitcoin#23834: wallettool: Check that the dumpfile checksum is the correct size (laanwj)
ee9b3cdb0e Merge bitcoin/bitcoin#23979: test: wait for rather than assert presence of file in startupnotify test (MarcoFalke)
2ec5940399 Merge bitcoin/bitcoin#23532: test: add functional test for -startupnotify (MarcoFalke)
5a31be9608 Merge bitcoin/bitcoin#23812: test: fix intermittent failures in p2p_timeouts.py (MarcoFalke)
10828f5b3d Merge bitcoin/bitcoin#23733: fuzz: Move ISO8601 to one place (MarcoFalke)
7f39b5af41 Merge bitcoin/bitcoin#23635: test: Bump shellcheck version to 0.8.0 (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Trivial batch of backports
## What was done?
trivial backports
## How Has This Been Tested?
Unit tests ran; waiting on CI
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: a3f97003e6441468951b827b2c3ea607740e5b9d36b96c2f93e45e7fb4088ecf4d0a2b7038de050ca0e7d61379c364969f4a8caff98ec1cc69016f4114e64c6a
