34c80473a8 Merge #19877: [test] clarify rpc_net & p2p_disconnect_ban functional tests (Wladimir J. van der Laan)
e42412924f Merge #19770: RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (MarcoFalke)
f96966b7ea Merge #20043: doc: Add 19501 release notes (fanquake)
6a164eaea9 Merge #19501: send* RPCs in the wallet returns the "fee reason" (MarcoFalke)
b6c8d852e3 Merge #19725: [RPC] Add connection type to getpeerinfo, improve logs (MarcoFalke)
f86263b180 Merge #18202: refactor: consolidate sendmany and sendtoaddress code (Samuel Dobson)
fab41fd3c5 partial Merge #18878: test: Add test for conflicted wallet tx notifications (Wladimir J. van der Laan)
db5bd34ee8 Merge #19202: log: remove deprecated `db` log category (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v21
## What was done?
- bitcoin/bitcoin#19202
- partial bitcoin/bitcoin#18878
- bitcoin/bitcoin#18202
- bitcoin/bitcoin#19725
- bitcoin/bitcoin#19501
- bitcoin/bitcoin#20043
- bitcoin/bitcoin#19770
- bitcoin/bitcoin#19877
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
- (RPC) The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
field will be fully removed in the next major release. It can be accessed
with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
it is recommended to instead use the `connection_type` field (it will return
`manual` when addnode is true)
- (Settings) The `sendtoaddress` and `sendmany` RPCs accept an optional `verbose=True`
argument to also return the fee reason about the sent tx.
- (Settings) The `-debug=db` logging category, which was deprecated in v0.18 and replaced by
`-debug=walletdb` to distinguish it from `coindb`, has been removed.
- (RPC) To make RPC `sendtoaddress` more consistent with `sendmany` the following error
`sendtoaddress` codes were changed from `-4` to `-6`:
- Insufficient funds
- Fee estimation failed
- Transaction has too long of a mempool chain
## 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 34c80473a8
Tree-SHA512: 725a103e04c9c7d44a79da6f3f54e7745c7fb98ec906e7228ae16f7662d568e48c015c855902ff8485f2908f0f71815e769ca394cf6c3ca2e5fd920dd39cca74
47ff5098ad5ea2c20ea387f99940a7cde6c80789 [test] Clarify setup of node topology. (Amiti Uttarwar)
0672522aedd3760c30b8740c7e9487f00bf9dfeb [move-only, test]: Match test order with run order (Amiti Uttarwar)
Pull request description:
small improvements to clarify logic in the functional tests
1. have test logic in `rpc_net.py` match run order of the test
2. remove `connect_nodes` calls that are redundant with the automatic test setup executed by the test framework
Noticed when I was trying to debug a test for #19725. Small changes but imo very helpful, because they initially confused me.
ACKs for top commit:
laanwj:
ACK 47ff5098ad5ea2c20ea387f99940a7cde6c80789
Tree-SHA512: 2843da2c0b4f06b2600b3adb97900a62be7bb2228770abd67d86f2a65c58079af22c7c20957474a98c17da85f40a958a6f05cb8198aa0c56a58adc1c31100492
5b57dc5458800e56b4dddfeb32a1813804a62b0f RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr)
d681a28219d3876a2b6e3cd2fb0d92963674903e RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr)
Pull request description:
If we were going to continue support for "whitelisted", we should have probably made it true if any permission flag was set, rather than only if "default permissions" were used.
This corrects the description, and deprecates it.
ACKs for top commit:
laanwj:
ACK 5b57dc5458800e56b4dddfeb32a1813804a62b0f
Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
fa710a6d67b2de64bde90def77c70d0a052f9030 doc: Add 19501 release notes (MarcoFalke)
faf60dee34ae3dbe8e103a2c1b0679f13df6a921 doc: Remove double-whitespace from help string, other whitespace fixups (MarcoFalke)
Pull request description:
Adds release notes and fixes up some whitespace nits for the touched RPCs
ACKs for top commit:
fanquake:
ACK fa710a6d67b2de64bde90def77c70d0a052f9030
laanwj:
Code review ACK fa710a6d67b2de64bde90def77c70d0a052f9030
Tree-SHA512: b84a96386a9a8ed69f464c7dffdd600cf9a8b33a06120798b141b300991baed369ab91ae48df6446e89e1d62534ccd8ae721454e7a19b48900b317e9192afc47
69cf5d4eeb73f7d685e915fc17af64634d88a4a2 [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e20d56acf7246008b7832efde68ab21 [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb73f7d685e915fc17af64634d88a4a2
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar)
50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
sipa:
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
promag:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
08fc6f6cfc3b06fd170452a766696d7b833113fa [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost)
Pull request description:
I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination.
Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this.
Salvaged from #18201.
ACKs for top commit:
fjahr:
Code review ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
jonatack:
ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
meshcollider:
Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
bde72a41fe merge bitcoin#23324: print peer counts for all reachable networks in -netinfo (Kittywhiskers Van Gogh)
4b245441a0 merge bitcoin#22959: Display all proxies in -getinfo (Kittywhiskers Van Gogh)
30b0fcf4a6 merge bitcoin#22544: drop torv2; torv3 becomes onion per GetNetworkName() (Kittywhiskers Van Gogh)
b6ca36edda merge bitcoin#22547: Add progress bar for -getinfo (Kittywhiskers Van Gogh)
1f89bfd176 merge bitcoin#21832: Implement human readable -getinfo (Kittywhiskers Van Gogh)
2200b78a15 merge bitcoin#20877: user help and argument parsing improvements (Kittywhiskers Van Gogh)
bd934c71eb partial bitcoin#20764: cli -netinfo peer connections dashboard updates (Kittywhiskers Van Gogh)
b2d865633f merge bitcoin#21261: update inbound eviction protection for multiple networks, add I2P peers (Kittywhiskers Van Gogh)
0b16b50fcb cli: fix loop counter comparison in `ProcessReply` (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/6035
* Dependency for https://github.com/dashpay/dash/pull/6031
* In [dash#5904](https://github.com/dashpay/dash/pull/5904) ([bitcoin#21595](https://github.com/bitcoin/bitcoin/pull/21595)), one of the loops in `ProcessReply` is supposed to iterate `rows.size()` times (which at the time was hardcoded to `3`), the backport erroneously set the value to `m_networks.size()` (which also evaluated to `3`) as part of increasing `m_networks.size()` usage.
As this pull request includes [bitcoin#23324](https://github.com/bitcoin/bitcoin/pull/23324), which changes it over to `rows.size()`, the above has been corrected in a separate commit for documentation purposes.
* `-addrinfo` output
* `-getinfo` output (diamonds are due to rendering limitations of my terminal and are not indicative of the symbols used)
* `-netinfo` output
## Breaking Changes
* CLI `-addrinfo` now returns a single field for the number of `onion` addresses known to the node instead of separate `torv2` and `torv3` fields, as support for TorV2 addresses was removed from Dash Core in 18.0.
* `-getinfo` has been updated to return data in a user-friendly format that also reduces vertical space.
## 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
ACKs for top commit:
PastaPastaPasta:
utACK bde72a41fe
Tree-SHA512: 921cb45b7e243a321a32c835eb23d5ba8df610ff234a548a9051436a2c21845ce70097fb9a9bb812b77b04373f9f0a9f90264168d97b08da1890be06bfd9f99c
5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f93979b274a37de013502b05d25fad8 net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d9072b7d5a36a6491784bdeb9247e91fb0 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450dde6a54bd785504c923adfb45c7060 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
naumenkogs:
ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
Backport notice:
we don't have bumpfee feature, so, only some part of code is backported
f963a680515eda66429b3d1537a7baf281ab9283 test: Add test for conflicted wallet tx notifications (Russell Yanofsky)
Pull request description:
Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions
https://github.com/bitcoin/bitcoin/pull/9240 - accidental break
https://github.com/bitcoin/bitcoin/issues/9479 - bug report
https://github.com/bitcoin/bitcoin/pull/9371 - fix
https://github.com/bitcoin/bitcoin/pull/16624 - accidental break
https://github.com/bitcoin/bitcoin/issues/18325 - bug report
https://github.com/bitcoin/bitcoin/pull/18600 - potential fix
ACKs for top commit:
laanwj:
ACK f963a680515eda66429b3d1537a7baf281ab9283
jonatack:
re-ACK f963a680515eda66429b3d1537a7baf281ab9283
MarcoFalke:
ACK f963a680515eda66429b3d1537a7baf281ab9283
Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07
fixup dashify of feature_notifications
5fe8100ff36fed6d50c2a25b028f57b25af3504c Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. (lsilva01)
ae23faba6fc5cabc896f1175456d1018576f912d Add a new RPC command: restorewallet (lsilva01)
Pull request description:
As far as I know, there is no command to restore the wallet from a backup file.
The only way to do this is to replace the `wallet.dat` of a newly created wallet with the backup file, which is hardly an intuitive way.
This PR implements the `restorewallet` RPC command which restores the wallet from the backup file.
To test:
First create a backup file:
`$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak`
Then restore it in another wallet:
`$ bitcoin-cli restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak`
ACKs for top commit:
achow101:
re-ACK 5fe8100ff36fed6d50c2a25b028f57b25af3504c
prayank23:
tACK 5fe8100ff3
meshcollider:
utACK 5fe8100ff36fed6d50c2a25b028f57b25af3504c
Tree-SHA512: 9639df4d8ad32f255f5b868320dc69878bd9aceb3b471b49dfad500b67681e2d354292b5410982fbf18e25a44ed0c06fd4a0dd010e82807c2e00ff32e84047a1
09205b33aa74e385caa2803aa6febc18ad1efa32 net: Clarify message header validation errors (W. J. van der Laan)
955eee76803c098978cf0bbc7f1f6d3c230544e2 net: Sanitize message type for logging (W. J. van der Laan)
Pull request description:
- Use `SanitizeString` when logging message errors to make sure that the message type is sanitized. I have checked all logging in `net.cpp`.
- For the `MESSAGESTART` error don't inspect and log header details at all: receiving invalid start bytes makes it likely that the packet isn't even formatted as valid P2P message. Logging the four unexpected start bytes (as hex) should be enough.
- Update `p2p_invalid_messages.py` test to check this.
- Improve error messages in a second commit.
Issue reported by gmaxwell.
ACKs for top commit:
MarcoFalke:
re-ACK 09205b33aa74e385caa2803aa6febc18ad1efa32 only change is log message fixup 🔂
practicalswift:
re-ACK 09205b33aa74e385caa2803aa6febc18ad1efa32
Tree-SHA512: 8fe5326af135cfcf39ea953d9074a8c966b9b85a810b06a2c45b8a745cf115de4f321e72fc769709d6bbecfc5953aab83176db6735b04c0bc6796f59272cadce
754e802274e9373ad7e1dccb710acf74ded6e7fb test: check rejected future block later accepted (Luke Dashjr)
Pull request description:
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
This PR re-opens https://github.com/bitcoin/bitcoin/pull/17872 which went stale and addresses the nits raised by reviewers there.
ACKs for top commit:
MarcoFalke:
review ACK 754e802274e9373ad7e1dccb710acf74ded6e7fb
Tree-SHA512: a2bbc8fffb523cf2831e1ecb05f20868e30106a38cc2e369e4973fa549cca06675a668df16f76c49cc4ce3a22925404255e5c53c4232d63ba1b9fca878509aa0
ef5e9304cd407adab1563f24215da1b582274c20 test: update logging and docstring in rpc_blockchain.py (Jon Atack)
d548dc71e4849f638fccaea6be86ac4fa5304f01 test: replace magic values by constants in rpc_blockchain.py (Jon Atack)
78c361086fc0bf27612e8142bd33e05e37a36af6 test: assert on mediantime in getblockheader and getblockchaininfo (Jon Atack)
0a9129c588ab016eb0453b40a0cae918ca4aa6a2 test: assert on the value of getblockchaininfo#time (Jon Atack)
Pull request description:
Follow-up to #22407 improving test coverage per https://github.com/bitcoin/bitcoin/pull/22407#pullrequestreview-702077013.
ACKs for top commit:
tryphe:
untested ACK ef5e9304cd407adab1563f24215da1b582274c20
Tree-SHA512: f746d56f430331bc6a2ea7ecd27b21b06275927966aacf1f1127d8d5fdfd930583cabe72e23df3adb2e005da904fc05dc573b8e5eaa2f86e0e193b89a17a5734
a006d7d73019b8cf4d68626c019c3d69729dda69 test: add logging to wallet_listtransactions (Sebastian Falbesoner)
47915b118720c6e2b2ec9f599f25848041b42b99 test: remove unneeded/redundant code in wallet_listtransactions (Sebastian Falbesoner)
fb6c6a7938cb7c4808ad88d23bfc2b7408407b12 test: speedup wallet_listtransactions by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
This PR improves the test `wallet_listtransactions.py` in three ways:
* speeds up runtime by a factor of 2-3x by using the good ol' immediate tx relay trick (`-whitelist=noban@127.0.0.1`)
* removes unneeded/redundant code
* adds log messages, mostly by turning comments into `self.log.info(...)` calls
ACKs for top commit:
jonatack:
ACK a006d7d73019b8cf4d68626c019c3d69729dda69
kristapsk:
ACK a006d7d73019b8cf4d68626c019c3d69729dda69
Tree-SHA512: a91a19f5ebc4d05f0b96c5419683c4c57ac0ef44b64eeb8dd550bd72296fd3a2857a3ba83f755fe4b0b3bd06439973f226070b5d0ce2dee58344dae78cb50290
ba7e17e073f833eccd4c7c111ae9058c3f123371 rpc, test: document {previous,next}blockhash as optional (Sebastian Falbesoner)
Pull request description:
This PR updates the result help of the following RPCs w.r.t. the `previousblockhash` and `nextblockhash` fields:
- getblockheader
- getblock
Also adds trivial tests on genesis block (should not contain "previousblockhash") and best block (should not contain "nextblockhash").
Top commit has no ACKs.
Tree-SHA512: ef42c5c773fc436e1b4a67be14e2532e800e1e30e45e54a57431c6abb714d2c069c70d40ea4012d549293b823a1973b3f569484b3273679683b28ed40abf46bb
a33dcb3283 fix: CheckWalletOwnsScript/CheckWalletOwnsKey to use wallet instead of SPK (Konstantin Akimov)
b2ede8bfee feat: update list of tests that still doesn't support descriptor wallets (Konstantin Akimov)
838d06f2fd feat: enable descriptor wallets for more tests (Konstantin Akimov)
5ab108c982 feat: implementation of RPC 'protx register' for descriptor wallets (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Many rpc such as `protx register` 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)
```
for all functional tests that uses Masternodes/evo nodes.
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## What was done?
Some direct usages of LegacyScriptPubKeyMan refactored to use CWallet's functionality.
There are still 4 functional tests that doesn't work for descriptor wallets:
- feature_dip3_deterministicmns.py (no rpc `protx updateregistar`)
- feature_governance.py: no rpc for `governance votemany` and `governance votealias`
- interface_zmq_dash.py (see governance)
That's part I of changes, other changes are not PR-ready yet, WIP.
## How Has This Been Tested?
Firstly, the flag `--legacy-wallets` are removed for many functional tests.
Secondly, the flag `--descriptors` is inverted in default value:
```
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 585a6a74d6..9ad5fd1daa 100755
--- 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
else:
# If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter
# It still needs to exist and be None in order for tests to work however.
```
## Breaking Changes
N/A, descriptor wallets have not been publicly 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
- [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 a33dcb3283
Tree-SHA512: 24e22ac91e30a3804d1ac9af864eb9d073208bbb6d1f3326c7f0438f3ccce2b553aa46450989e48cb5c6e0d838ff1c88c6522f195e7aa2bd89342710f3ecef77
e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886b60644d103a5eb310691c003a39328 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2f2e139fbc63e2f37fb33ea4efae088 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a83f3e94bcb78b6aa463c5570c1e737e Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8ba69e2cc82c617cdf406ab79df58825 wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e5434da90d2cf680f23e8c7b7164c9f945 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b834077f102724415e0fada6ee8b2def2 descriptors: Cache last hardened xpub (Andrew Chow)
cacc3910989c4f3d7afa530dbab042461426abce Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75cd03c8f0a8cfadb47e0fbcabe3c5e59 Refactor Cache merging and writing (Andrew Chow)
976b53b085d681645fd3a008fe382de85647e29f Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
3eb6f8b2e61c24a22ea9396d86672307845f35eb wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb56bf5d455154b0498b1f58f77d20ed wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e357159c7154f69f28cb5587c5ca20d6594 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce88696a3216fc38b7d393906b733e8b1 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788cb0862aafbb116fd37936cbed6a431 wallet: refactor GetClosestWalletFeature() (Jon Atack)
Pull request description:
This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.
This PR fixes 4 upgradewallet issues:
- this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
- it returns nothing in the absence of an RPC error, which isn't reassuring for users
- it returns the same thing both in the case of a successful upgrade and when no upgrade took place
- the error message object is currently dead code
This PR fixes the above and provides:
...user feedback to not silently return without upgrading
```
{
"wallet_name": "disable private keys",
"previous_version": 169900,
"current_version": 169900,
"result": "Already at latest version. Wallet version unchanged."
}
```
...better feedback after successfully upgrading
```
{
"wallet_name": "watch-only",
"previous_version": 159900,
"current_version": 169900,
"result": "Wallet upgraded successfully from version 159900 to version 169900."
}
```
...helpful error responses
```
{
"wallet_name": "blank",
"previous_version": 169900,
"current_version": 169900,
"error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
}
{
"wallet_name": "blank",
"previous_version": 130000,
"current_version": 130000,
"error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
}
```
updated help:
```
upgradewallet ( version )
Upgrade the wallet. Upgrades to the latest version if no version number is specified.
New keys may be generated and a new wallet backup will need to be made.
Arguments:
1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"previous_version" : n, (numeric) Version of wallet before this operation
"current_version" : n, (numeric) Version of wallet after this operation
"result" : "str", (string, optional) Description of result, if no error
"error" : "str" (string, optional) Error message (if there is one)
}
```
ACKs for top commit:
achow101:
ACK 3eb6f8b
MarcoFalke:
review ACK 3eb6f8b2e61c24a22ea9396d86672307845f35eb 🛡
Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
5f9c0b6360215636cfa62a70d3a70f1feb3977ab wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08215feba53ead27096ac7fda34acb3c test: Remove unused wallet.dat (MarcoFalke)
bf7635963c03203e7189ddaa56c6b086a0108cbf tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9decc3e855ee4b0bbf9e61121c8e9904e5 test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc434854f881330771a93a1280ac67b1d3549 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19be65b0dd23df1df571c71428c2bc32 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c995e832e643f605d35a7aa112837e6 wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc6258c258e9f4411c50630ec4a552341b wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f34dedf75b063b962845fa8eca604514 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842df489f1b8d68e67a234788966218184 wallet: Add utility method for CanSupportFeature (Andrew Chow)
Pull request description:
This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.
The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.
`CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.
`nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.
Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.
Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too.
Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict.
ACKs for top commit:
MarcoFalke:
approach ACK 5f9c0b6360
laanwj:
Code review ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab
jonatack:
ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`
Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
The key difference between bitcoin's and dash's implementation that sethdseed
does not update existing seed for wallet. Seed can be set only once.
It behave similarly to `upgradetohd` rpc, but since v20.1 all wallets are HD and
the name `upgradetohd` is not relevant more.
That are:
- feature_dip3_deterministicmns.py
- interface_zmq_dash.py
- feature_governance.py
- wallet_upgradetohd.py (as expected to be implemented for legacy-only wallets)
- p2p_timeouts.py (why? can not understand it)
This partially reverts commit b20f812674.
ccd976dd3dbb8f991dc1203ada2043f1736be5a4 test: use 327 fewer blocks in feature_nulldummy (Jon Atack)
68c280f19732fb96bc29113ce9c8007d0101868c test, refactor: abstract the feature_nulldummy blockheight values (Jon Atack)
Pull request description:
The resolved timeout issue seen in the CI can be reproduced locally by running `test/functional/feature_nulldummy.py --valgrind --loglevel=debug`
Speeds up the normal test runtime for me from 3.8 to 2.2 seconds (debug build). Thanks to Marco Falke for the approach suggestion.
ACKs for top commit:
AnthonyRonning:
reACK ccd976dd3dbb8f991dc1203ada2043f1736be5a4 - ran a few times with the rest of the tests and still passing for me with just the fewer block change.
MarcoFalke:
review ACK ccd976dd3dbb8f991dc1203ada2043f1736be5a4 🏝
Tree-SHA512: 38339dca4276d1972e3a5a5ee436da64e9e58fd3b50b186e34b07ade9523ac4c03f6c3869c5f2a59c23b43c44f87e712f8297a01a8d83202049c081e3eeb4445
ffe33dfbd4c3b11e3475b022b6c1dd077613de79 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2c2ce5fea84cf8681013f85a444e7ea versionbits: simplify state transitions (Anthony Towns)
55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48040dc7eae46bc7941db48d98a669fd fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c1702c8ae259610fe55050ba212e21e tests: test versionbits delayed activation (Anthony Towns)
73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa205eee5c3b99343988adcb8d320460 tests: clean up versionbits test (Anthony Towns)
593274445004506c921d5d851361aefb3434d744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a4760c0c0f784029849cb5d21ee088abb tests: pull ComputeBlockVersion test into its own function (Anthony Towns)
Pull request description:
BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
Edge cases are tested by fuzzing added in #21380.
ACKs for top commit:
instagibbs:
tACK ffe33dfbd4
jnewbery:
utACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
MarcoFalke:
review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
achow101:
re-ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
gmaxwell:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
benthecarman:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
Sjors:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
jonatack:
Initial approach ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
ariard:
Code Review ACK ffe33df
Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672 validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b24a89aba884cb18c13fa67ccca339ee8c validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)
Pull request description:
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
See: https://github.com/bitcoin/bitcoin/pull/19806#discussion_r612165410
ACKs for top commit:
Sjors:
utACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672
ryanofsky:
Code review ACK 91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
10a006e626 test: disable ipv6 tests for now (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Our CI nodes on Amazon have issues running tests using ipv6.
## What was done?
Pretend we don't have ipv6 when we run tests for now
## How Has This Been Tested?
See CI results for this PR.
## Breaking Changes
n/a but we'll have ipv6 not being tested for some time.
## 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:
kwvg:
ACK 10a006e626
PastaPastaPasta:
utACK 10a006e
Tree-SHA512: 75da180b916a5a99b1363f152494cbdd1031c7daf2d5eb370ca74547dee694ad04db3e9c677f0f738b9dbb15760d685a6b43b032dde642a4c1d49fff671e2aac
68ace23fa3bc01baa734ddf2c0963acae1c75ed1 test: convert docs into type annotations in test_framework/test_node.py (fanquake)
8bfcba36db974326d258c610456bd55cf5818b1e test: convert docs into type annotations in test_framework/wallet.py (fanquake)
b043ca8e8b65199061ebe4bbed2200504dfc6ce9 test: convert docs into type annotations in test_framework/util.py (fanquake)
Pull request description:
Rather than having function types exist as documentation, make them type annotations, which enables more `mypy` checking.
ACKs for top commit:
instagibbs:
utACK 68ace23fa3
Tree-SHA512: b705f26b48baabde07b9b2c0a8d547b4dcca291b16eaf5ac70462bb3a1f9e9c2783d93a9d4290889d0cbb3f7db3671446754411a1f498b265d336f6ff33478df
7a681d61b0d98a310fbb1b8e095ab8fbc5d5741c Add sync_blocks in wallet_orphanedreward.py. (Daniel Kraft)
Pull request description:
Add an explicit `sync_blocks` call in `wallet_orphanedreward.py`, which was missing and could lead to intermittent failures of the test due to race conditions.
This will presumably fix#22181.
ACKs for top commit:
MarcoFalke:
review ACK 7a681d61b0d98a310fbb1b8e095ab8fbc5d5741c
Tree-SHA512: bb226c31bf3f2e7c52beb829d7b67496e5b38781245db5f9184e3f28c93ac3aa4d21fcf5bf3055e79d384cfd0ed916e79dccb3d77486e86fe1fedb5e35f894ad
44dab423eb88dbf854d22f2991e79c828ffac0f2 qa: Test default include_mempool value of gettxout (João Barbosa)
Pull request description:
With the following diff the functional test would pass. Fix by testing the default value.
```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1142,7 +1142,7 @@ static RPCHelpMan gettxout()
uint256 hash(ParseHashV(request.params[0], "txid"));
int n = request.params[1].get_int();
COutPoint out(hash, n);
- bool fMempool = true;
+ bool fMempool = false;
if (!request.params[2].isNull())
fMempool = request.params[2].get_bool();
```
ACKs for top commit:
MarcoFalke:
cr ACK 44dab423eb88dbf854d22f2991e79c828ffac0f2
Tree-SHA512: 14db21b29d6b2c01d1d1278e18a0cf35d6ae566e33e45515d1fe2983dda94ad1ff6065c217601d283f9515cae39b57e981b62ac71ec2002de5359bd8a9e3efa9
ceefab5226 fix: feature_backwards compatible works now with as expected if no bdb compiled (Konstantin Akimov)
b20f812674 fix: follow-up fixes for functional tests used protx (Konstantin Akimov)
655146d5e7 Merge #21302: wallet: createwallet examples for descriptor wallets (W. J. van der Laan)
99a8b60393 Merge #21063: wallet, rpc: update listdescriptors response format (fanquake)
6ee2c7cc59 Merge #21277: wallet: listdescriptors uses normalized descriptor form (Wladimir J. van der Laan)
8bacdbf71f Merge #19136: wallet: add parent_desc to getaddressinfo (Samuel Dobson)
f567de007a chore: release notes for 5965 with wallet tool improvements (Konstantin Akimov)
0daf360edf chore: add TODO to implement mnemonic for descriptor wallets (Konstantin Akimov)
5016294307 chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test" (Konstantin Akimov)
ef7ce87c1b fix: remove workarounds introduced due to missing bitcoin#20267 (bdb is not compiled) (Konstantin Akimov)
06b2d85bb4 partial Merge #20267: Disable and fix tests for when BDB is not compiled (Wladimir J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/59
## Extra notes
This commit `chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test"` is not directly connected to descriptor wallets, but added to this PR due to conflicts with 20267
## What was done?
It steadily improves support of descriptor wallets in Dash core.
Done backports and related fixes:
- partial bitcoin/bitcoin#20267
- bitcoin/bitcoin#19136
- bitcoin/bitcoin#21277
- bitcoin/bitcoin#21063
- bitcoin/bitcoin#21302
Beside backports and related fixes, this PR includes release notes for previous batch of backports for descriptor wallets support #5965
## 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
- [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
Top commit has no ACKs.
Tree-SHA512: f4b2033f8c4fa1d0f72cfc31378909703b3ae8f44748989ff00c3e71311ac80ac37837137133c7e4a166823a941ed7df10efa09c89f5b213f3c8ede7d3d6e8f4
