This change unifies Homebrew packages workflow, and does not change
behavior.
Github-Pull: #20527
Rebased-From: c96d1f65a552712f8476269ad64a415717ead50d
Whenever both encodings are permitted, try both, and if only one succeeds,
return that one. Otherwise prefer the one for which the heuristic sanity
check passes. If that is the case for neither or for both, return the
extended-permitting deserialization.
Github-Pull: #20595
Rebased-From: 39c42c442044aef611d03ee7053d2dd6df63deb7
01b647b1a2 build: Avoid secp256k1.h include from system (Niklas Gögge)
Pull request description:
Backports #20469 to the 0.21 branch.
ACKs for top commit:
hebasto:
ACK 01b647b1a2, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: c098055b5e413be6f438d1d43e80c1943329ebb708531d8d82e72de402bddeb6f8b812303f9ae5a45abf62b3ff87fa909fbbf7fb56dca7959ecb9061febae4a1
and remove redundant units ("Must be at least 1.000 sat/vB sat/vB" -> "1.00 sat vB")
Github-Pull: #20426
Rebased-From: 9f08780dd7946b63476e9736745131db8e7f4e93
as the feeRate argument should soon be deprecated.
Also loosen one test (and a similar one) that caused a one-off CI failure with:
expected message
'Insufficient total fee 0.00000141, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)'
actual message
'Insufficient total fee 0.00000141, must be at least 0.00001712 (oldFee 0.00001007 + incrementalFee 0.00000705)'
Github-Pull: #20426
Rebased-From: 3f1e10b2b1cd11f7112fbad6355464bd4adbbc5c
A check to raise an error on zero-fee txns was mistakenly extended in commit
a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include
fundrawtransaction and walletcreatefundedpsbt.
This commit 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.
Github-Pull: #20426
Rebased-From: 1b3d7009280595108eb22ac1188bc43678
fa69c2c784 wallet: Do not treat default constructed types as None-type (MarcoFalke)
fac4e136fa refactor: Change pointer to reference because it can not be null (MarcoFalke)
Pull request description:
Github-Pull: #20410
Rebased-From: fac4e136fa
Github-Pull: #20410
Rebased-From: fa69c2c784
Top commit has no ACKs.
Tree-SHA512: 05c3fe29677710b57dcc482fd529b0ab79475519f60f9cfde19f956c4e2212d09b042af458ec4f1272c581360ce841b735dca4df144e0798b3ccf16547de9cd0
ab23a83400 Fix QPainter non-determinism on macOS (Andrew Chow)
Pull request description:
Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The
source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans
when compiling. The particular optimization that seems to be causing the
problems is that a temp variable is being added for spans->y. For some
reason, when it does this, it chooses different instructions to use when
making that variable. We bypass this problem by patching
qt_intersect_spans to always make and use this local variable.
Github-Pull: #20447
Rebased-From: 8f7d1b39efbe65ab2747c593cc3560d4a449a333
Tree-SHA512: 558da5c2bb0373e2a89f2c219170f802036e0e87cc8e808336b23d074152cb893007a440f46ec957156b0921355cd18502710f2d224f27bc26e934c50ebebc41
ACKs for top commit:
jonasschnelli:
codereview ACK ab23a83400
achow101:
ACK ab23a83400
Tree-SHA512: 10991fe2b5452b1393678c315281cfdca011e9bb2cd8094a002746e690890ace148ac2dbf39c5fbe5e7f4cd39eeebfa0a715c065cff150cf70e9733cb0ff32d6
b1f59d55d9 RPC/Wallet: unloadwallet: Clarify docs/error when both the RPC endpoint and wallet_name parameter specify a wallet (Luke Dashjr)
Pull request description:
Github-Pull: #20462
Rebased-From: b1f59d55d9
Top commit has no ACKs.
Tree-SHA512: 2ee0a8a280f56baf196a3a48a59620f297075d23898e6aa3b3e677cdde74826688614d27a477a1448306234c2109fa39083946f691ced10d8bbc53006730784e
Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The
source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans
when compiling. The particular optimization that seems to be causing the
problems is that a temp variable is being added for spans->y. For some
reason, when it does this, it chooses different instructions to use when
making that variable. We bypass this problem by patching
qt_intersect_spans to always make and use this local variable.
Github-Pull: #20447
Rebased-From: 8f7d1b39efbe65ab2747c593cc3560d4a449a333
Tree-SHA512: 558da5c2bb0373e2a89f2c219170f802036e0e87cc8e808336b23d074152cb893007a440f46ec957156b0921355cd18502710f2d224f27bc26e934c50ebebc41
7ffac12545 tests: shrink feature_taproot transfer of funds tx (Anthony Towns)
Pull request description:
Github-Pull: #20428
Rebased-From: 7ffac12545
Top commit has no ACKs.
Tree-SHA512: 4e6b37a44dca3e29d5168b7eb9238a7ce0bbb9b0924a21671537a7c534790fb6b05b1a30a404db434fade030b4f369adfc73694ef85d91884bb7349adddc5f6a
Of course, this one was in another place too.
Tree-SHA512: 87784829b1f700dcf5fd22daad0c920cfb25485ae17eff0b3e236513dc543c8643e568f39d418d43ea0eeb330fcac93ab2276cda8253ec6538d01e20d102d10c
It is unnecessary to upgrade to FEATURE_HD_SPLIT if this feature is
already supported by the wallet. Because upgrading to FEATURE_HD_SPLIT
actually requires upgrading to FEATURE_PRE_SPLIT_KEYPOOL, users would
accidentally be upgraded to FEATURE_PRE_SPLIT_KEYPOOL instead of nothing
being done.
Fixes the issue described at
https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
bb6441b7a4 qt: Pre-splitoff translations update (Wladimir J. van der Laan)
Pull request description:
0.21 split-off should be near now. Let's do one final translations update just before the split-off.
(Hopefully it won't take too long, but might want to keep this open to be the last thing merged)
ACKs for top commit:
hebasto:
ACK bb6441b7a4
MarcoFalke:
ACK bb6441b7a4 (checked that only changes are translation changes in `src/qt`)
Tree-SHA512: 3273246923d3020e1f7ae46cbb59f1ed45a35acb5e1582b55486c5723f5aa1e5809fe2fd87b1ac34d308eef2902e621d0ace97181a044262b2c8f002bf50daac
05e82d86b0 wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack)
9a670b4f07 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack)
be481b72e2 wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack)
449b730579 wallet: provide valid values if invalid estimate mode passed (Jon Atack)
6da3afbaee wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack)
173b5b5fe0 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack)
7f9835a05a wallet: remove fee rates from conf_target helps (Jon Atack)
b7994c01e9 wallet: add fee_rate unit warnings to bumpfee (Jon Atack)
410e471fa4 wallet: remove redundant bumpfee fee_rate checks (Jon Atack)
a0d4957473 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack)
e21212f01b wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack)
6112cf20d4 wallet: add CFeeRate ctor doxygen documentation (Jon Atack)
3f72791613 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 05e82d86b0
Sjors:
utACK 05e82d86b0
Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
ac64cec4ce gui: create wallet: add advanced section (Sjors Provoost)
c99d6f644a gui: create wallet: name placeholder (Sjors Provoost)
5bff82540b [gui] create wallet: smarter checkbox toggling (Sjors Provoost)
Pull request description:
Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of https://github.com/bitcoin/bitcoin/pull/15454 now all new users have to. I don't think it was user-friendly enough for that.
<img width="403" alt="Schermafbeelding 2020-09-18 om 09 41 44" src="https://user-images.githubusercontent.com/10217/93574129-52ef9680-f998-11ea-9a6f-31144f66d3bf.png">
This PR makes a few simple improvements so that new users don't have to think too much:
<img width="369" alt="Schermafbeelding 2020-10-15 om 16 45 22" src="https://user-images.githubusercontent.com/10217/96145959-0c914700-0f06-11eb-9526-cf447d841d7a.png">
It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo.
* wallet encryption is no longer checked by default, because such a change in the default needs a separate discussion (fwiw, I suspect it increases the number of users losing access to coins)
* watch-only and descriptor wallet stuff is moved to advanced, so new users know they can safely ignore these check boxes
* bonus: when you click on "disable private keys" it disables encrypt wallet and checks blank wallet
* label changes: see screenshot
* tooltip changes: see code diff
Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that.
_Update 2020-10-30_, dropped the new strings for now:
<img width="450" alt="Schermafbeelding 2020-10-30 om 11 26 55" src="https://user-images.githubusercontent.com/10217/97694591-1b99fc80-1aa3-11eb-8b85-e19f1ad5add4.png">
ACKs for top commit:
fjahr:
Tested ACK ac64cec4ce
jonatack:
re-ACK ac64cec4ce, per `git diff d393708 ac64cec` only change since my last review is improving the placeholder from "MyWallet" to "Wallet" and dropping the last commit. Tested creating a dozen wallets in signet with different combinations of options and then verifying/comparing their characteristics in the console with getwalletinfo. My remaining caveats are (1) the need for less user surprise by either (a) improving the user info or (b) with less auto-(un)selecting as mentioned in https://github.com/bitcoin-core/gui/pull/96#issuecomment-727017409 and (2) I prefer the "Encrypt private keys" and "Watch-only" wording and descriptions below over the current ones; hopefully these can be addressed in a follow-up.
hebasto:
re-ACK ac64cec4ce
ryanofsky:
Code review ACK ac64cec4ce. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit
Tree-SHA512: a25f84eb66ee4f99af441d73e33928df9d9cf592177398ef48f0037f5913699e47a162cf1301c83b34501546d43ff4ae12607fd078c5c03b92f573bf7604a9f2
b6121edf70 swapped "is" for "==" in literal comparison (Tyler Chambers)
Pull request description:
In Python 3.8+ literal comparisons using "is" instead of "==" produce a SyntaxWarning [source](https://docs.python.org/3.8/whatsnew/3.8.html#changes-in-python-behavior).
I checked the entire devtools directory, this seems to be the only occurrence.
This is a small fix, but removes the SyntaxWarning.
Fixes: #20338
ACKs for top commit:
hebasto:
re-ACK b6121edf70, only squashed since my [previous](https://github.com/bitcoin/bitcoin/pull/20346#pullrequestreview-525934568) review.
practicalswift:
re-ACK b6121edf70: patch still looks correct
theStack:
utACK b6121edf70
Tree-SHA512: 82a43495d6552fbaa3b02b58f0930b049d27aa937fe44b47714e3c059f844cc494de20674557371cbccf24fb8873ecb7376fb965ae326847eed2b855ed2d59c6
d355a302d9 Break circuit earlier (lontivero)
Pull request description:
Currently when parsing an onion v3 address the pubic key checksum is calculated in order to compare it with the received address checksum. However this step is not necessary if the address version byte is not 3, in which case the method can return with false immediately.
ACKs for top commit:
jonatack:
ACK d355a302d9
practicalswift:
ACK d355a302d9 -- patch looks correct
hebasto:
ACK d355a302d9, I have reviewed the code and it looks OK, I agree it can be merged.
sipa:
utACK d355a302d9
Tree-SHA512: 9e4506793b7f4a62ce8edc41a260a8c125ae81ed2f90cd850eb2a9214d323c446edc7586c7b0590dcbf3aed5be534718b77bb19c45b48f8f52553d32a3663a65
5f9c0b6360 wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08 test: Remove unused wallet.dat (MarcoFalke)
bf7635963c tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9dec test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc43485 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc62 wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f3 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842d 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 5f9c0b6360
jonatack:
ACK 5f9c0b6360, 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
20e491ddcb CI/Cirrus: Skip merge_base step for non-PRs (Luke Dashjr)
Pull request description:
CIRRUS_BASE_BRANCH is a PR-specific variable and undocumented on non-PR builds.
In practice (at the moment), it seems to be HEAD, which in private repositories can be pretty much anything, causing CI to fail if it can't be cleanly merged.
By checking CIRRUS_PR first, we can reliably do CI builds of branches outside PRs.
ACKs for top commit:
MarcoFalke:
review ACK 20e491ddcb
Tree-SHA512: 9fd8db2e19a3145f7dccfca107631b20df8c94d385f624e2bcef2fa18e38bf3e23c6c68fc8241decedbf1413bf69ca572cff75e1ccf82c09ac50443001ec5ae5
