e9434ee03e Remove false positive GCC warning (Hennadii Stepanov)
Pull request description:
On master (f05c1ac444) GCC compiler fires a false positive `-Wmaybe-uninitialized`:
```
wallet/wallet.cpp: In static member function ‘static std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)’:
wallet/wallet.cpp:3913:27: warning: ‘*((void*)& time_first_key +8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Optional<int64_t> time_first_key;
^~~~~~~~~~~~~~
```
The same as #15292.
This PR leverages a workaround and removes the warning.
ACKs for top commit:
laanwj:
ACK e9434ee03e, removes the warning for me (gcc 7.4.0)
kristapsk:
ACK e9434ee03e
Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
d3bc184081 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f364 test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20 rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda4 test: remove getaddressinfo label tests (Jon Atack)
c7654af6f8 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc184081
laanwj:
ACK d3bc184081
meshcollider:
utACK d3bc184081
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
If fDecryptionThoroughlyChecked is false, after a key has been checked,
write (or rewrite) its checksum. This serves to upgrade wallets
and correct those which have the checksum corrupted but not the key.
Change fDecryptionThoroughlyChecked to default to true so that it can
latch to false when an invalid checksum is seen. Checksums may be invalid
if the wallet does not have checksums or if the wallet became corrupted.
It is safe to default fDecryptionThoroughlyChecked to true because any
existing wallet without a checksum will set it to false. Any new or
blank wallet where encrypted keys are added will then set this to true
when the first encrypted key is generated by virtue of CheckDecryptionKey
doing that during the initial Unlock prior to keys being added.
3f373659d7 Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c403 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3 Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59 refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e846 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee5 Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad94 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d7 (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d7
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
f41d589669 Document better -keypool as a look-ahead safety mechanism (Antoine Riard)
Pull request description:
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see https://github.com/bitcoin/bitcoin/pull/17681#issuecomment-563613452
ACKs for top commit:
ryanofsky:
Code review ACK f41d589669. Just "Warning:" prefix added since the last review
jonatack:
ACK f41d589669 code review and build/test. The added `Warning:` since last review is a good addition.
Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0
deaa6dd144 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa4 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes#17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough
This commit does not change behavior.
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
297e098557 Fix doxygen errors (Ben Woosley)
Pull request description:
These are all the remaining errors identified via -Werror=documentation, e.g.:
```
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
```
You can use this to run with `-Wdocumentation` yourself: #14920
ACKs for top commit:
laanwj:
ACK 297e098557
Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
2b1641492f wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Improve `CWallet:MarkDestinationsDirty` by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.
ACKs for top commit:
meshcollider:
re-utACK 2b1641492f
Tree-SHA512: 479dc2dde4b653b856e3d6a0c59a34fe33e963eb131a2d88552a8b30471b8725a087888fe5d7db6e4ee19b74072fe64441497f033be7d1931637f756e0d8fef5
6dd59d2e49 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e49
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e49
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
6fc554f591 wallet: Reset reused transactions cache (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17824)
`getbalances` is using the cache within `GetAvailableCredit` under certain conditions [here](35fff5be60/src/wallet/wallet.cpp (L1826)). For a wallet with `avoid_reuse` activated this can lead to inconsistent reporting of `used` transactions/balances between `getbalances` and `listunspent` as pointed out in #17603. When an address is reused before the first transaction is spending from this address, the cache is not updated even after the transaction is sent. This means the remaining outputs at the reused address are not showing up as `used` in `getbalances`.
With this change, any newly incoming transaction belonging to the wallet marks all the other outputs at the same address as dirty.
ACKs for top commit:
kallewoof:
Code review re-ACK 6fc554f591
promag:
ACK 6fc554f591.
achow101:
Re-ACK 6fc554f591
meshcollider:
Code review ACK 6fc554f591
Tree-SHA512: c4cad2c752176d16d77b4a4202291d20baddf9f27250896a40274d74a6945e0f6b34be04c2f9b1b2e756d3ac669b794969df8f82a98e0b16f10e92f276649ea2
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
If a destination is reused we mark the cache of the other transactions going to that destination dirty so they are not accidentally reported as trusted when the cache is hit.
6d6a7a8403 gui: Fix duplicate wallet showing up (João Barbosa)
81ea66c30e Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky)
Pull request description:
This PR includes 2 fixes:
- prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer;
- prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`.
Fixes#16937
ACKs for top commit:
fjahr:
code review ACK 6d6a7a8403
ryanofsky:
Code review ACK 6d6a7a8403. No changes since last ACK other than rebase due to #17070
kallewoof:
Code review ACK 6d6a7a8403
Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4
e1e1442f3e Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)
Pull request description:
Slight cleanup following https://github.com/bitcoin/bitcoin/pull/16944
This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.
ACKs for top commit:
achow101:
ACK e1e1442f3e
Sjors:
Code review ACK e1e1442f3e
meshcollider:
Code review ACK e1e1442f3e
Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
8925df86c4 doc: update release notes (Jon Atack)
8bb405bbad test: getaddressinfo labels purpose deprecation test (Jon Atack)
60aba1f2f1 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack)
7851f14ccf rpc: incorporate review feedback from PR 17283 (Jon Atack)
Pull request description:
This PR builds on #17283 (now merged) and is followed by #17585.
It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs.
before
```
"labels": [
{
"name": "DOUBLE SPEND",
"purpose": "receive"
}
```
after
```
"labels": [
"DOUBLE SPEND"
]
```
The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`.
For context, see:
- https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427)
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output.
Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those.
ACKs for top commit:
jnewbery:
reACK 8925df8
promag:
Code review ACK 8925df86c4.
meshcollider:
Code review ACK 8925df86c4
Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
091a876664 Test watchonly wallet bumpfee with PSBT return (Gregory Sanders)
e9b4f9419c bumpfee: Return PSBT when wallet has privkeys disabled (Gregory Sanders)
75a5e478b6 Change bumpfee to use watch-only funds for legacy watchonly wallets (Gregory Sanders)
Pull request description:
The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds.
ACKs for top commit:
achow101:
ACK 091a876664
Sjors:
Tested ACK 091a876664
meshcollider:
utACK 091a876664
Tree-SHA512: f7cf663e1af0b029e5c99eac88c5fdc3bc9e9a3841da8a608e8a9957e9bcf6a78864b8c2706fcaf78a480ffe11badd80c4fad29f97c0bb929e0470fafda5c22e
09502452bb IsUsedDestination should count any known single-key address (Gregory Sanders)
Pull request description:
This plugs the privacy leak detailed at https://github.com/bitcoin/bitcoin/issues/17605, at least for the single-key case.
ACKs for top commit:
meshcollider:
Code Review ACK 09502452bb
Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c
- change the value returned in the RPC getaddressinfo `labels` field to an array
of label name strings
- deprecate the previous behavior of returning a JSON hash structure containing
label `name` and address `purpose` key/value pairs
- update the relevant tests
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
6e77a7b65c keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34b28 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce23c keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)
Pull request description:
* The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
* An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`.
* Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool`
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.
See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174
ACKs for top commit:
instagibbs:
utACK 6e77a7b65c only change is slight elaboration on comment
ryanofsky:
Code review ACK 6e77a7b65c. Only the comment changed since my previous review.
Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f
Removes SetCrypted() and fUseCrypto as we don't need them anymore.
SetCrypted calls in LegacyScriptPubKeyMan are replaced with mapKeys.empty()
IsCrypted() is changed to just call HasEncryptionKeys()
886f1731be Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow)
386a994b85 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow)
ba41aa4969 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow)
65833a7407 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow)
9fcf8ce7ae Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow)
596f6460f9 Key pool: Move CanGetAddresses call (Andrew Chow)
Pull request description:
* The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't.
* `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan`
* In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved.
* A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug.
Split from #17261
ACKs for top commit:
ryanofsky:
Code review ACK 886f1731be. Only change is moving earlier fix to a better commit (same end result).
promag:
Code review ACK 886f1731be.
instagibbs:
code review re-ACK 886f1731be
Sjors:
Code review re-ACK 886f1731be
Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19
e7ad4a2f8c doc: rename wallet-tool references to bitcoin-wallet (Wilson Ccasihue S)
Pull request description:
Fix. text reference to executable bitcoin-wallet instead of wallet-tool, there is not a wallet-tool at bin/ folder.
ACKs for top commit:
fanquake:
ACK e7ad4a2f8c - thanks for following up.
Tree-SHA512: aed41b08947728a4ff3a97a62858ee7c86e2e5d57dcbbd0aab492dae3d8a548bb60541924e68cf3a0aa3d53d7db0012b489462b466919cd83f05b2aa88b7fff7
02afb0c550 Fix origfee return for bumpfee with feerate arg (Gregory Sanders)
Pull request description:
fixes https://github.com/bitcoin/bitcoin/issues/17642 and adds a simple test that would have caught it
ACKs for top commit:
achow101:
ACK 02afb0c550
Tree-SHA512: 303e392e05407f204dffe360689b5bb5dc77fd462dd0e489bc0b6c8f94f89ab7fe2bd8cb47e4dc6dc5c23a619826d15f3bf6b02b2c8e96402fbb51953c462e2d
In order for ScriptPubKeyMan to be generic and work with future
ScriptPubKeyMans, ScriptPubKeyMan::ReturnDestination is changed to
take a CTxDestination instead of a CPubKey. Since LegacyScriptPubKeyMan
still deals with keys internally, a new map m_reserved_key_to_index is
added in order to track the keypool indexes that have been reserved.
The CPubKey argument of KeepDestination is also removed so that it is
more generic. Instead of taking a CPubKey or a CTxDestination, we just use
the nIndex given to find the pubkey.
Addresses are determined by LegacyScriptPubKeyMan::GetReservedDestination
instead of ReserveDestination::GetReservedDestination as other ScriptPubKeyMan
implementations may construct addresses differently
This does not change behavior.
eadd1304c8 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow)
ff330badd4 Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow)
Pull request description:
#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally.
Apparently this particular case doesn't have a test, so I've added one as well.
ACKs for top commit:
Sjors:
ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test.
fanquake:
ACK eadd1304c8
instagibbs:
utACK eadd1304c8
Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e
33f5fc32e5 test: add rpc getaddressinfo labels test coverage (Jon Atack)
0f3539ac6d test: add listlabels test in wallet_labels.py (Jon Atack)
1388de8390 rpc: add getaddressinfo code documentation (Jon Atack)
2ee0cb3330 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack)
8d1ed0c263 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack)
5a0ed85070 rpc: improve getaddressinfo RPCHelpman content (Jon Atack)
70cda342cd rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack)
Pull request description:
This PR is a continuation of the work in https://github.com/bitcoin/bitcoin/pull/12892.
Main motivations:
- There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address.
- `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers.
Changes by order of commits:
- [x] improve/fix getaddressinfo RPCHelpman layout formatting
- [x] improve/fix getaddressinfo RPCHelpman content
- [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman
- [x] update getaddressinfo RPCExamples addresses to bech32
- [x] add getaddressinfo code docs
- [x] add a `listlabels` test assertion in wallet_labels.py
- [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests
Here are gists of the CLI help output:
[`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810)
[`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba)
It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._
ACKs for top commit:
fjahr:
Re-ACK 33f5fc32e5
jnewbery:
ACK 33f5fc32e5.
Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
An opportunistic TopUp is moved from LegacyScriptPubKeyMan::GetNewDestination
to CWallet::GetNewDestination. Another opportunistic TopUp is moved from
LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination)
to ReserveDestination::GetReservedDestination.
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always
be topped up before requesting Destinations from them as we cannot
always rely on future ScriptPubKeyMan implementaions topping up internally.
As such, it is also unnecessary to keep the TopUp calls in the
LegacyScriptPubKeyMan functions so they are moved.
This does not change behavior as TopUp calls are moved up the call stack.
This does not change behavior. This TopUp() is unnecessary as currently
m_spk_man calls TopUp further down the call stack inside
LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination)
By removing this here, we also prepare for future changes where CWallet
has multiple ScriptPubKeyMans instead of m_spk_man.
Call LegacyScriptPubKeyMan::CanGetAddresses directly instead of calling
CWallet::CanGetAddresses to only query the relevant key manager
This is a minor change in behavior: call now only happens if a new key needs to
be reserved, since if a key is already reserved it might fail unnecessarily.
This change also serves as a sanity check
https://github.com/bitcoin/bitcoin/pull/16341#discussion_r331238394
3c2c439dcd wallet: Make -walletdir network only (João Barbosa)
Pull request description:
With this PR `bitcoind -regtest` doesn't run if bitcoin.conf has
```
walletdir=/mnt/mydisk/wallets
```
But works with
```
[regtest]
walletdir=/mnt/mydisk/wallets
```
Doesn't change mainnet behavior.
Closes#15630.
ACKs for top commit:
ryanofsky:
ACK 3c2c439dcd
MarcoFalke:
ACK 3c2c439dcd🍈
meshcollider:
Tested ACK 3c2c439dcd
Tree-SHA512: 8ab3b2db5f3f9cab78b36baaf490c80f7330372cfd8f73fe6536c8fb4c6e55e09f62296feb70617075838b3bcd7101abebbef3b228b6c3dbd42ce8c7a5c372d9
6a2e6b0600 Remove out of date comments for CalculateMaximumSignedTxSize (Gregory Sanders)
Pull request description:
These paths can be hit for probably a number of reasons, and ISMINE spendability is not a requirement to call it.
For example: During watch-only transaction creation, previous transaction in wallet, pubkey imported, but not the witnessscript associated with the prevout.
In this case I think no/minimal comment is better than specific and soon to be out of date.
ACKs for top commit:
achow101:
ACK 6a2e6b0600
darosior:
ACK 6a2e6b0600
Tree-SHA512: ad4c26fd2409eb5aed19d67c19cb5479d226bd11e9298630309c4344f6562ace2e10c2850ebe22770331d71e91320a606e79619b9fe52dd478ce1f589a740122
3958295bc8 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa)
55295fba4c wallet: Lock address type in ReserveDestination (João Barbosa)
Pull request description:
Only mutates the wallet if the reserved key is kept.
First commit is a refactor that makes the address type a class member.
The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB.
ACKs for top commit:
achow101:
Re-ACK 3958295bc8
Sjors:
ACK 3958295bc8
ryanofsky:
Code review ACK 3958295bc8. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.)
meshcollider:
utACK 3958295bc8
Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f
c6dd565c88 [gui] watch-only wallet: copy PSBT to clipboard (Sjors Provoost)
39465d545d [wallet] add fillPSBT to interface (Sjors Provoost)
848f889208 [gui] send: include watch-only (Sjors Provoost)
40537f0909 [wallet] ListCoins: include watch-only for wallets without private keys (Sjors Provoost)
Pull request description:
For wallets with `WALLET_FLAG_DISABLE_PRIVATE_KEYS` this makes the watch-only balance available on the send screen (including coin selection). Instead of sending a transaction it generates a PSBT.
The user can take this PSBT and process it with [HWI](https://github.com/bitcoin-core/HWI) or put it an SD card for hardware wallets that support that.
The PSBT is copied to the clipboard. This was the easiest approach; we can add a dialog later to display it, as well as an option to save to disk.
ACKs for top commit:
instagibbs:
test and code review ACK c6dd565c88
meshcollider:
re-ACK c6dd565c88
Tree-SHA512: ebc3da0737e33b255ed926191b84569aedb6097d14868662bd5dce726ce3048e86e9a31eba987b10dffe1482b35c21ae1cd595c2caa4634bc4cf78a826a83852
d0dab897af Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow)
4b0c718f8f Accumulate result UniValue in SignTransaction (Andrew Chow)
Pull request description:
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.
Split from #17261
ACKs for top commit:
instagibbs:
utACK d0dab897af
ryanofsky:
Code review ACK d0dab897af. Thanks for the SignTransaction update. No other changes since last review
Sjors:
Code review ACK d0dab897af
promag:
Code review ACK d0dab897af.
meshcollider:
Code review ACK d0dab897af
Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70
b007efdf19 Allow BnB when subtract fee from outputs (Andrew Chow)
db15e71e79 Use BnB when preset inputs are selected (Andrew Chow)
Pull request description:
Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases.
Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too.
ACKs for top commit:
instagibbs:
reACK b007efdf19
Sjors:
re-ACK b007efdf19
Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
faffa7f0dc wallet: Avoid showing GUI popups on RPC errors (take 2) (MarcoFalke)
Pull request description:
Commit 8b0d82bb42 claims "This commit does not change behavior." However, it re-introduced the bug I tried to fix in #17070
ACKs for top commit:
ryanofsky:
Code review ACK faffa7f0dc
Tree-SHA512: 99987f80c76414dca40c7d76b2fe4ea853debbe3c49e7acdeab2596c726a2935c468f4484d49212e65ecc9c8b0d861c0c2b83c1ddfc07670540699199dbfecb0
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.
SignTransaction will be called multiple times in the future. Pass
it a result UniValue so that it can accumulate the results of multiple
SignTransaction passes.
fad1de66a2 wallet: Remove unused boost::this_thread::interruption_point (MarcoFalke)
Pull request description:
`BerkeleyEnvironment::Open` is only called from the main thread (init) or an http rpc thread, neither of which can be interrupted, so remove the useless interruption point.
`BerkeleyEnvironment{}` is only used in tests, which run in a single process/thread, so remove the useless interruption point.
ACKs for top commit:
laanwj:
ACK fad1de66a2
fanquake:
ACK fad1de66a2
Tree-SHA512: dacd8398e966e4a6ce5cf7d3ed821c9c267eff40b14c0635085441647cdb72d1642807f89355419f1710f814c7963e35a10d102d0b985c7198261dfc736256f8
0b75a7f068 wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd00e wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)
Pull request description:
This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
- 1st the recursive lock is removed and now it requires the lock to be held;
- 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.
ACKs for top commit:
achow101:
ACK 0b75a7f068
MarcoFalke:
ACK 0b75a7f068
ryanofsky:
Code review ACK 0b75a7f068. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?
Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
36b68de5b2 Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429c56 Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff03871ad Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de16f Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05e48 Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d3848e Add block_height field in struct Confirmation (Antoine Riard)
9700fcb47f Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3eff1 Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729e33 Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (Antoine Riard)
Pull request description:
Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.
I think it's ready for a first round of review before to get further.
- `BlockUntilSyncedToCurrent` : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.
~~- `AbandonTransaction` : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed~~ After #16624, we instead rely on Confirmation.
~~- `AddToWalletIfInvolvingMe`: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in `mapWallet` with a height set to zero so we remove check on block_hash.IsNull~~ Already done in #16624
ACKs for top commit:
jnewbery:
@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de5b2).
jkczyz:
> @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch ([36b68de](36b68de5b2)).
meshcollider:
utACK 36b68de5b2
ryanofsky:
Code review ACK 36b68de5b2. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment
jnewbery:
utACK 36b68de5b2
promag:
Code review ACK 36b68de5b2.
Tree-SHA512: 08b89a0bcc39f67c82a6cb6aee195e6a11697770c788ba737b90986b4893f44e90d1ab9ef87239ea3766508b7e24ea882b7199df41173ab27a3d000328c14644
fa4c6fa9b1 doc: Add documentation for new test/lib (MarcoFalke)
faec28252c scripted-diff: test: Move setup_common to test library (MarcoFalke)
Pull request description:
Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit.
Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes it. For example, setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui)
ACKs for top commit:
practicalswift:
ACK fa4c6fa9b1 -- diff looks correct and Travis is happy
jonatack:
ACK fa4c6fa9b1 with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review.
ryanofsky:
Code review ACK fa4c6fa9b1. I didn't realize `lib` was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two.
Tree-SHA512: 2e176df90c60578276e4a6dc83ff57ff59d8e666ecf30c5ceacb8c326725da91baa4cac3dfa7a2e1605f58122a3e3e27e4938ff33e3a0ce7ea53afffebbf57a4
We don't remove yet Chain locks as we need to preserve lock
order with CWallet one until swapping at once to avoid
deadlock failures (spotted by --enable-debug)
is exact match
In the next commit, we start using BlockConnected/BlockDisconnected
callbacks to establish tx depth, rather than querying the chain
directly.
Currently, BlockUntilSyncedToCurrentChain will return early if
the best block processed by the wallet is a descendant of the node'tip.
That means that in the case of a re-org, it won't wait for the
BlockDisconnected callbacks that have been enqueued during the re-org
but have not yet been triggered in the wallet.
Change BlockUntilSyncedToCurrentChain to only return early if the
wallet's m_last_block_processed matches the tip exactly. This ensures
that there are no BlockDisconnected or BlockConnected callbacks
in-flight.
At wallet loading, we rely on chain state querying to retrieve
height of txn, to do so we ensure that lock order is respected
between cs_main and cs_wallet.
If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as confirmation
height is not used by wallet-tool.
Reorder arguments and document Confirmation calls to avoid
ambiguity.
Fixes nits left from #16624
fa2c44c3cc test: Add ASSERT_DEBUG_LOG to unit test framework (MarcoFalke)
fa1936f57b logging: Add member for arbitrary print callbacks (MarcoFalke)
Pull request description:
Similar to `assert_debug_log` in the functional test framework
Top commit has no ACKs.
Tree-SHA512: aa9eaeca386b61d806867c04a33275f6eb4624fa5bf50f2928d16c83f5634bac96bcac46f9e8eda3b00b4251c5f12d7b01d6ffd84ba8e05c09eeec810cc31251
To do so we update CValidationInterface::BlockDisconnect to take a
CBlockIndex pointing to the block being disconnected.
This new parameter will be use in the following commit to establish
wallet height.
Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341194391
by Gregory Sanders <gsanders87@gmail.com>
Reason for keeping the `return true` `return false` verbosity is that more code
will be added after the ReserveKeyFromKeyPool() call before returning.
4671fc3d9e Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)
91f3073f08 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)
8f174ef112 Systematize style of IsTrusted single line if (Jeremy Rubin)
b49dcbedf7 update variable naming conventions for IsTrusted (Jeremy Rubin)
5ffe0d1449 Update comment in test/functional/wallet_balance.py (Jeremy Rubin)
a550c58267 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)
5dd7da4ccd Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)
595f09d6de Cache tx Trust per-call to avoid DoS (Jeremy Rubin)
dce032ce29 Make IsTrusted scan parents recursively (Jeremy Rubin)
Pull request description:
This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either.
This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected.
This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug.
The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change.
# Before `test_balance()`, we have had two nodes with a balance of 50
# each and then we:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 60 from node B to node A with fee 0.01
#
# Then we check the balances:
#
# 1) As is
# 2) With transaction 2 from above with 2x the fee
#
# Prior to #16766, in this situation, the node would immediately report
# a balance of 30 on node B as unconfirmed and trusted.
#
# After #16766, we show that balance as unconfirmed.
#
# The balance is indeed "trusted" and "confirmed" insofar as removing
# the mempool transactions would return at least that much money. But
# the algorithm after #16766 marks it as unconfirmed because the 'taint'
# tracking of transaction trust for summing balances doesn't consider
# which inputs belong to a user. In this case, the change output in
# question could be "destroyed" by replace the 1st transaction above.
#
# The post #16766 behavior is correct; we shouldn't be treating those
# funds as confirmed. If you want to rely on that specific UTXO existing
# which has given you that balance, you cannot, as a third party
# spending the other input would destroy that unconfirmed.
#
# For example, if the test transactions were:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 10 from node B to node A with fee 0.01
#
# Then our node would report a confirmed balance of 40 + 50 - 10 = 80
# BTC, which is more than would be available if transaction 1 were
# replaced.
The release notes have been updated to note the new behavior.
ACKs for top commit:
ariard:
Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR.
fjahr:
Code review ACK 4671fc3d9e
ryanofsky:
Code review ACK 4671fc3d9e. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good!
promag:
Code review ACK 4671fc3d9e.
Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
436ad43643 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)
Pull request description:
Closes#8752 by bringing back abandoned #10470.
This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.
For more context, #8757 was closed in favor of #10470.
ACKs for top commit:
instagibbs:
utACK 436ad43643
kallewoof:
utACK 436ad43643
jonatack:
I'm not qualifed to give an ACK here but 436ad43643 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:
Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
c98bd13e67 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas)
Pull request description:
- Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
- Add a linter to prevent future usage of assert being used in RPC code
ref https://github.com/bitcoin/bitcoin/pull/17192
ACKs for top commit:
practicalswift:
ACK c98bd13e67 -- diff looks correct
Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
d314e8a818 refactor: Replace all uses of boost::optional with our own Optional type (Wladimir J. van der Laan)
Pull request description:
Replace all uses of boost::optional with our own Optional type. Luckily, there aren't so many.
After this:
- `boost::optional` is no longer used directly (only through `Optional` which is an alias for it)
- `boost/optional.hpp` is only included in one place
ACKs for top commit:
MarcoFalke:
ACK d314e8a818
practicalswift:
ACK d314e8a818 -- diff looks correct + satisfying to see incremental progress towards the goal of a Boost free future :)
jtimon:
ACK d314e8a818
fanquake:
ACK d314e8a818
Tree-SHA512: b43e0017af81b07b5851377cd09624f114510ac5b9018d037664b58ad0fc8e893e30946b61f8f5e21e39125925bf9998a81f2226b468aab2df653ee57ed3213d
After this:
- `boost::optional` is no longer used directly (only through `Optional`
which is an alias for it)
- `boost/optional.hpp` is only included in one place
362ded410b Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b7 scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5 Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e scripted-diff: Rename InitInterfaces to NodeContext (Russell Yanofsky)
Pull request description:
This change is mainly a naming / organization change intended to simplify #10102. It:
- Renames struct InitInterfaces to struct NodeContext and moves it from
src/init.h to src/node/context.h. This is a cosmetic change intended to make
the point of the struct more obvious.
- Gets rid of BanMan and ConnMan globals making them NodeContext members
instead. Getting rid of these globals has been talked about in past as a way
to implement testing and simulations. Making them NodeContext members is a
way of keeping them accessible without the globals.
- Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This
better separates node and wallet rpc methods. Node RPC methods should have
access NodeContext, while wallet RPC methods should only have indirect access
to node functionality via interfaces::Chain.
- Adds NodeContext& references to interfaces::Chain class and the
interfaces::MakeChain() function. This is needed to access ConnMan and BanMan
instances without the globals.
- Gets rid of redundant Node and Chain instances in Qt tests. This is
needed due to the previous MakeChain change, and also makes test setup a
little more straightforward. More cleanup could be done in the future, but it
will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init
code.
ACKs for top commit:
laanwj:
ACK 362ded410b
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
f3b51eb935 Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)
Pull request description:
Using `data()` better communicates the intent here.
~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](https://github.com/bitcoin/bitcoin/pull/17281#discussion_r339742128).
ACKs for top commit:
fjahr:
Code review ACK f3b51eb
practicalswift:
ACK f3b51eb935 -- diff looks correct, `data()` more idiomatic
ryanofsky:
Code review ACK f3b51eb935. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this.
Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8
f201ba59ff Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes (Andrew Chow)
6702048f91 MOVEONLY: Move key handling code out of wallet to keyman file (Andrew Chow)
ab053ec6d1 Move wallet enums to walletutil.h (Andrew Chow)
Pull request description:
Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan.
First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden.
ACKs for top commit:
Sjors:
Code review ACK f201ba5.
promag:
Code review ACK f201ba59ff.
ryanofsky:
Code review ACK f201ba59ff
MarcoFalke:
ACK f201ba59ff
Tree-SHA512: bdc0d8595a06233fe003afcf968a38e0e8cc584a6a89c5bcd05309ac29dca852391802d46763ef81a108d146d0f40c79ea5438e87234ed12b4b8360c9aec94c0
c72906dcc1 refactor: Remove redundant c_str() calls in formatting (Wladimir J. van der Laan)
Pull request description:
Our formatter, tinyformat, *never* needs `c_str()` for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.
Remove redundant `c_str()` calls for:
- `strprintf`
- `LogPrintf`
- `tfm::format`
(also, combined with #17095, I think this improves logging in case of unexpected embedded NULL characters)
ACKs for top commit:
ryanofsky:
Code review ACK c72906dcc1. Easy to review with `git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee`
Tree-SHA512: 9e21e7bed8aaff59b8b8aa11571396ddc265fb29608c2545b1fcdbbb36d65b37eb361db6688dd36035eab0c110f8de255375cfda50df3d9d7708bc092f67fefc
Wallet code should use interfaces::Chain and not directly access to node state.
Add a g_rpc_chain replacement global for wallet code to use, and move
g_rpc_node definition to a libbitcoin_server source file so there are link
errors if wallet code tries to access it.
listsinceblock now checks that returned transactions are not
conflicting with any transactions that are filtered out by
the given blockhash
Co-Authored-By: Michael Chrostowski <michael.chrostowski@gmail.com>
Using `data()` better communicates the intent here.
Also, depending on how `c_str()` is implemented, this fixes undefined
behavior: The part of the string after the first NULL character might
have undefined contents.
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp
The easiest way to review this commit is to run:
git log -p -n1 --color-moved=dimmed_zebra
And check that everything is a move (other than includes and copyrights comments).
This commit is move-only and doesn't change code or affect behavior.
9e95931865 [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery)
Pull request description:
`CommitTransaction()` returns a bool to indicate success, but since commit
b3a7410 (#9302) it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function.
ACKs for top commit:
laanwj:
ACK 9e95931865
Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
cd68594dcd Only check the hash of transactions loaded from disk (Andrew Chow)
Pull request description:
It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.
ACKs for top commit:
MarcoFalke:
ACK cd68594dcd
laanwj:
ACK cd68594dcd
promag:
ACK cd68594dcd, AFAICT the check is not needed, hash comparison gives data integrity.
Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb
facec1c643 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)
Pull request description:
RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed.
```
gives me a GUI popup and no reason why loading the wallet failed.
After this pull request:
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core
ACKs for top commit:
laanwj:
Code review ACK facec1c643
Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
CommitTransaction returns a bool to indicate success, but since commit
b3a74100b8 it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
b96ed03962 [wallet] Remove pruning check for -rescan option (John Newbery)
eea462de9c [wallet] Remove package limit config access from wallet (John Newbery)
Pull request description:
Removes wallet access to `-limitancestorcount`, `-limitdescendantcount` and `-prune`:
- `-limitancestorcount` and `-limitdescendantcount` are now accessed with a method `getPackageLimits` in the `Chain` interface.
- `-prune` is not required. It was only used in wallet component initiation to prevent running `-rescan` when pruning was enabled. This check is not required.
Partially addresses #17137.
ACKs for top commit:
MarcoFalke:
Tested ACK b96ed03962
ryanofsky:
Code review ACK b96ed03962
promag:
Code review ACK b96ed03962.
ariard:
ACK b96ed03, check there isn't left anymore wallet access to node arguments.
Tree-SHA512: 90c8e3e083acbd37724f1bccf63dab642cf9ae95cc5e684872a67443ae048b4fdbf57b52ea47c5a1da6489fd277278fe2d9bbe95e17f3d4965a1a0fbdeb815bf
Prior to this PR, the wallet would not allow the `-rescan` option at
startup if pruning was enabled. This is unnecessarily restrictive. It
should be possible to rescan if pruning is enabled, as long as no blocks
have actually been pruned yet.
Remove the pruning check from WalletInit::ParameterInteraction(). If any
blocks have been pruned, that will be caught in CreateWalletFromFile().
The wallet should not be able to directly access global configuration
from the node. Remove access of "-limitancestorcount" and
"-limitdescendantcount".
a57a1d42d5 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)
Pull request description:
The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589aff) which is however static and only indirectly available via the public methods `AddWatchOnly()`, `LoadWatchOnly()` and `RemoveWatchOnly()`. Since the first of those methods also stores the addresses to the disk, the second, simpler one was chosen which only operates in memory.
ACKs for top commit:
Sjors:
ACK a57a1d4
instagibbs:
reACK a57a1d42d5
Sjors:
re-ACK a57a1d4
Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26
Fields involvesWatchonly, generated, walletconflicts were missing
in result description of listtransactions, listsinceblock,
gettransaction
Align getttransaction fields which were odd compare to other rpc
helpers
ea4cc3a7b3 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b3
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented
c812aba394 test bumpfee fee_rate argument (ezegom)
9f25de3d9e rpc bumpfee check fee_rate argument (ezegom)
88e5f997df rpc bumpfee: add fee_rate argument (ezegom)
1a4c791cf4 rpc bumpfee: move feerate estimation logic into separate method (ezegom)
Pull request description:
Taking over for https://github.com/bitcoin/bitcoin/pull/16492 which seems to have gone inactive.
Only minor commit cleanups, rebase, and some help text fixes on top of previous PR. Renamed `feeRate` to `fee_rate` to reflect updated guidelines.
ACKs for top commit:
Sjors:
Code review ACK c812aba
laanwj:
ACK c812aba394
Tree-SHA512: 5f7f51bd780a573ccef1ccd72b0faf3e5d143f6551060a667560c5163f7d9480e17e73775d1d7bcac0463f3b6b4328f0cff7b27e39483bddc42a530f4583ce30
addaf8af82 make sure to update the UI when deleting a transaction (Jonas Schnelli)
Pull request description:
`CWallet::ZapSelectTx` removes transactions from the internal model, but leaves the UI in the dark.
Adding a `NotifyTransactionChanged()` should avoid having invalid transactions in the GUI.
Fixes#16950
ACKs for top commit:
fanquake:
ACK addaf8af82 - tested that this fixes#16950
Sjors:
tACK addaf8a: tested with an unpruned wallet by calling `removeprunedfunds` on an RBF-replaced transaction. It neatly disappears from the UI.
kristapsk:
ACK addaf8af82 (tested both with and without this change)
Tree-SHA512: 65e8c690847f7499e82c9fef67b60d9aaa63c853732fe7fa7281da33054fcdcd9d24f5b86de71b0827728c25bac8efb7db445863f990304ebfee6fc450620c47
The motivation for this addition was to unit test the function
wallet.cpp:ExtractPubKey() (see recent change in commit
798a589aff) which is however static and only
indirectly available via the public methods AddWatchOnly(), LoadWatchOnly() and
RemoveWatchOnly(). Since the first of those methods also stores the addresses
to the disk, the second, simpler one was chosen which only operates in memory.
test: add missing wallet lock for test case WatchOnlyPubKeys
test: test case WatchOnlyPubKeys, suggested review changes by instagibbs
test: test case WatchOnlyPubKeys, suggested review changes by achow101
test: test case WatchOnlyPubKeys, s/isPubKeyFullyValid/is_pubkey_fully_valid
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.
However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.
This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.
It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction.
Update the RPC help, functional test, and release note.
This makes the RPC method consistent with other RPC methods that have a
'verbose' option.
Change the name of the return object from 'decoded' to details.
Update help text.
fa734603b7 wallet: Fix segmentation fault in CreateWalletFromFile (MarcoFalke)
fab3c34412 test: Print both messages on failure in assert_raises_message (MarcoFalke)
faa13539d5 wallet: Fix documentation around WalletParameterInteraction (MarcoFalke)
Pull request description:
Comes with a test to aid review. The test should fail without the fix to bitcoind
The following `CreateWalletFromFile` issues are fixed:
* `walletFile` refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
* `WalletParameterInteraction` was moved to `CreateWalletFromFile` and `WalletInit::ParameterInteraction` without updating the documentation
ACKs for top commit:
promag:
ACK fa734603b7.
darosior:
ACK fa734603b7
meshcollider:
LGTM, code-read ACK fa734603b7
Tree-SHA512: 2aceb63a3f25b90a840cfa08d37f5874aad4eb3df8c2ebf94e2ed18b55809b185e6920bdb345b988bff1fcea5e68a214fe06c361f7da2c01a3cc29e0cc421cb4
39034f1ee6 Refactor rawtransaction_util's SignTransaction to have previous tx parsing be separate (Andrew Chow)
Pull request description:
Currently the `SignTransaction` function has to handle both the actual signing and parsing of previous transaction data. This PR splits it so that `SignTransaction` only handles the signing itself and adds a `ParsePrevouts` function which handles parsing the prevtx information.
This allows for `SignTransaction` to just take any `SigningProvider`.
Split from #16341
ACKs for top commit:
MarcoFalke:
ACK 39034f1ee6
instagibbs:
utACK 39034f1ee6
ryanofsky:
utACK 39034f1ee6. No change since previously reviewed b49bbb939be92a67ff77c3f7bca5bb94dd141906, https://github.com/bitcoin/bitcoin/pull/16341#pullrequestreview-278610269 other than rebase with no conflicts.
Tree-SHA512: 09f7733e90691766bfb5cf0f20e913dbf270bd3b51abdcad966b24d110e562ed85fd3d0d1d7bbea61f903340060052ec73c4817b09aee0dc1f3916d781a9e40c
442a87cc0a Add a test wallet_reorgsrestore (Antoine Riard)
40ede992d9 Modify wallet tx status if has been reorged out (Antoine Riard)
7e89994133 Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
a31be09bfd Encapsulate tx status in a Confirmation struct (Antoine Riard)
Pull request description:
While working on #15931, I've tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of `hashBlock` and `nIndex` with magic value to determine tx confirmation, conflicted or abandoned state. It's hard to reason and error-prone. To solve that, we encapsulate these fields in a `TxConfirmation` struct and introduce a `TxState` member that we update accordingly at block connection/disconnection.
Following jnewbery [recommendation](https://github.com/bitcoin/bitcoin/pull/15931#discussion_r312576506), I've taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :
* https://github.com/bitcoin/bitcoin/issues/7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
* https://github.com/bitcoin/bitcoin/issues/8692 where we should cancel conflicted state of transactions chain smoothly
* `MarkConflicted` in `LoadToWallet` is likely useless if we track conflicts rights at block connection
Main changes of this PR to get right are tx update in `AddToWallet` and serialization/deserialization logic.
ACKs for top commit:
meshcollider:
Light re-Code Review ACK 442a87cc0a
ryanofsky:
utACK 442a87cc0a. Changes since last review are switching from `hasChain` to `LockChain` and removing chain lock in `WalletBatch::LoadWallet` that's redundant with the new lock still added in `CWallet::LoadWallet`, and fixing python test race condition.
Tree-SHA512: 029209e006de0240436817204e69e548c5665e2b0721b214510e7aba7eba130a5eab441d3a1ad95bd6426114dd27390492c77bf4560a9610009b32cd0a1f72f7
fa61365a13 wallet: Translate all initErrors in CreateWalletFromFile (MarcoFalke)
fa70d199d0 util: Make util/error bilingual_str (refactor) (MarcoFalke)
Pull request description:
The translations are going to close in three days (#15940), so I am submitting this as a standalone pull request.
Those changes are part of a bugfix #16661, which includes a test. The first change (the refactor) is required, the second commit is not. I am happy to drop it, if needed.
ACKs for top commit:
laanwj:
utACK fa61365a13
hebasto:
ACK fa61365a13, I have tested the code on Linux Mint 19.2.
Tree-SHA512: a7616cc38b9ffd301c6b915ea808a65815c3d97e9f57ec091772eb260e5cf0d75a13a6e4dfa3913e236833677c7929b9a748cb7d7a0e406d51749944b614e11b
9965940e35 doc: Add release note for the new gettransaction argument (darosior)
b8b3f0435a tests: Add a new functional test for gettransaction (darosior)
7f3bb247a8 gettransaction: add an argument to decode the transaction (darosior)
Pull request description:
This PR adds a new parameter to the `gettransaction` call : `decode`. If set to `true`, it will add a new `decoded` field to the response. This mimics the behavior of `getrawtransaction`'s `verbose` argument to avoid using 2 calls if we want to decode a wallet transaction (`gettransaction` then `decoderawtransaction`).
Fix#16181 .
ACKs for top commit:
meshcollider:
re-utACK 9965940e35
Tree-SHA512: bcb6b4bd252b3488d6afc77659c499c2ad99fd58661eb24b6a2e17014c74f22e47fde70e00fedb4f4754915786622ad02483b2cf2c4dea0ab0eb4ac8276dbeee
d9d8984270 wallet: Use wallet name instead of pointer on unload/release (João Barbosa)
Pull request description:
Fixes#16668. Wallet name is unique so it can be used instead of pointer.
ACKs for top commit:
meshcollider:
utACK d9d8984270
instagibbs:
utACK d9d8984270
ryanofsky:
utACK d9d8984270. Alternately I think it might be possible to use an intptr_t set instead of a string set to get around the undefined behavior described in the issue.
Tree-SHA512: eccd4d260cd4c02b52c30deeb32dbfd190a1151a5340eb3aa4ece0dc6ae3b3ed746ce5617336461f6f27c437c435629cd07d20beb1c5450f23b75edde6728598
This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction
Add a LockChain method to CWallet to know if we can lock or query
chain state safely.
At tx loading, we rely on chain to know if hashBlock of tx is still
in main chain. If not, we set its status to unconfirmed and reset
its hashBlock/nIndex.
If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as status is not
used by wallet-tool.
We take lock prematurely in CWallet::LoadWallet and CWallet::Verify
to ensure that lock order is respected between cs_main an cs_wallet.
The function ExtractPubKey() checks if a given script matches the P2PK pattern
(<PubKey> OP_CHECKSIG), extracts the PubKey and additionally checks if it is
cryptographically valid (full validation with ECC library via .IsFullyValid()).
Currently this is done manually in the following order:
1) check if first script OP is data push with valid PubKey length
(first part of pattern match), extract PubKey
2) create CPubKey object with extracted PubKey
3) fully validate public key
4) check if last script OP is OP_CHECKSIG
(second part of pattern match)
Using Solver, the pattern matching and PubKey extraction can be done via a
single step, leading to the following simplified order with shorter code:
1) check if given script matches P2PK pattern with Solver
(also contains valid PubKey length check), extracts Pubkey
2) create CPubKey object with extracted Pubkey
3) fully validate public key
Translated strings should not end up in the debug log, stderr, or
returned by an RPC.
Changing the util methods in util/error to return a bilingual_str paves
the way to achieve this goal in the long term.
We shouldn't rely on this sync call to get an accurate view of txn
state, if a tx conflicts with one in mapTx we are going to update
our wallet dependencies in AddToWalletIfInvolvingMe while conflicting
txn get connected. If it doesn't conflict with one of our dependencies
we are not going to track it anyway.
This is a cleanup, as this SyncTransaction is redundant with the
following one for confirmation which is triggering the MarkConflicted
logic. We keep the loop because set of conflicted txn isn't same as txn
included in block.
Instead of relying on combination of hashBlock and nIndex
values to manage tx in its lifecycle, we introduce 4
status : CONFIRMED, UNCONFIRMED, CONFLICTED, ABANDONED.
hashBlock and nIndex magic values should only be used at
serialization/deserialization for backward-compatibility.
At block disconnection, we know flag txn as UNCONFIRMED where
previously they kept their states until being override by a
block connection or abandontransaction call. This is a change
in behavior for which user may have to call abandon twice
if transaction is disconnected and not accepted back in the mempool.
We assert status transitioning right in AddToWallet. Doing so
flagged a misbehavior in ComputeTimeSmart unit test where same
tx is confirmed twice in different block. To avoid inconsistencies
we unconfirmed tx before new connection in different block. We
also remove a cs_main lock in test, as AddToWallet and its
callees don't rely on locked chain.
2dbfb37b40 Fix Char as Bool in interfaces (Jeremy Rubin)
Pull request description:
In a few places in src/wallet/wallet.h, we use a char when semantically we want a bool.
This is kind of an issue because it means we can unserialize the same transaction with different fFromMe flags (as differing chars) and evaluate the following section in wallet/wallet.cpp
```c++
if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe)
{
wtx.fFromMe = wtxIn.fFromMe;
fUpdated = true;
}
```
incorrectly (triggering an fUpdated where both fFromMe values represent true, via different chars).
I don't think this is a vulnerability, but it's just a little messy and unsemantic, and could lead to issues with stored wtxIns not being findable in a map by their hash.
The serialize/unserialize code for bool internally uses a char, so it should be safe to make this substitution.
NOTE: Technically, this is a behavior change -- I haven't checked too closely that nowhere is depending on storing information in this char. Theoretically, this could break something because after this change a tx unserialized with such a char would preserve it's value, but now it is converted to a ~true~ canonical bool.
ACKs for top commit:
achow101:
Code review ACK 2dbfb37b40
meshcollider:
Code review ACK 2dbfb37b40
Tree-SHA512: 8c0dc9cf672aa2276c694facbf50febe7456eaa8bf2bd2504f81a61052264b8b30cdb5326e1936893adc3d33504667aee3c7e207a194c71d87b3e7b5fe199c9d
Some failure conditions implicitly fail by failing some other check.
But the error messages are more helpful if they say explicitly what
actually caused the failure, so add those as failure conditions and
errors.
72eaab073b tests: functional watch-only wallet tests (William Casarin)
72ffbdc579 doc: add release note for include_watchonly default changes (William Casarin)
003a3c73c0 rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6c0b rpcwallet: default include_watchonly to true for watchonly wallets (William Casarin)
Pull request description:
Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an `include_watchonly` argument that needs to be explicitly set.
Wallets created with `createwallet` can have a `disable_private_keys` parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the `include_watchonly` parameter isn't set.
ACKs for top commit:
achow101:
Code review ACK 72eaab073b
Sjors:
ACK 72eaab073b
promag:
ACK 72eaab073b, code review only, didn't look closely to the test.
kallewoof:
ACK 72eaab073b
fanquake:
ACK 72eaab073b - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.
Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7
Restores the confirmed/conflicted tx check removed in
8753f5652b. There should be no external
behaviour change (these txs would not get accepted to the mempool
anyway), but not having the check in the wallet causes log spam.
Also adds a comment to ResentWalletTransactions() that
confirmed/conflicted tx check is done in SubmitMemoryPoolAndRelay().
RPC server starts in warmup mode, it can't
process yet calls, then follows connection manager
initialization and finally RPC server get out of
warmup mode. RPC calls shouldn't be able to get
P2P disabled errors because once we initialize
g_connman it's not unset until shutdown, after
RPC server has been stopped.
e6f649cb2c test: Make tests arg type specific (Hennadii Stepanov)
b70cc5d733 Revamp option negating policy (Hennadii Stepanov)
db08edb303 Replace IsArgKnown() with FlagsOfKnownArg() (Hennadii Stepanov)
dde80c272a Use ArgsManager::NETWORK_ONLY flag (Hennadii Stepanov)
9a12733508 Remove unused m_debug_only member from Arg struct (Hennadii Stepanov)
fb4b9f9e3b scripted-diff: Use ArgsManager::DEBUG_ONLY flag (Hennadii Stepanov)
1b4b9422ca scripted-diff: Use Flags enum in AddArg() (Hennadii Stepanov)
265c1b58d8 Add Flags enum to ArgsManager (Hennadii Stepanov)
e0d187dfeb Refactor InterpretNegatedOption() function (Hennadii Stepanov)
e0e18a1017 refactoring: Check IsArgKnown() early (Hennadii Stepanov)
Pull request description:
This PR adds the `Flags` enum to the `ArgsManager` class. Also the `m_flags` member is added to the `Arg` struct. Flags denote an allowed type of an arg value and special hints.
This PR is only a refactoring and does not change behavior.
ACKs for top commit:
jamesob:
ACK e6f649cb2c
MarcoFalke:
ACK e6f649cb2c thanks for adding types to the command line options
Tree-SHA512: b867f8a9cbce2d2473c293d534af662d8cd5be15060ff0682e97af678974bdaac35e8bc6328ccba32f105034bcd38f169b92a6fb67798667891ce14d5d2a2dea
fb62f128bb Tidy up BroadcastTransaction() (John Newbery)
b8eecf8e79 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f5652b Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c198 Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19b4b Add BroadcastTransaction utility usage in Chain interface (Antoine Riard)
Pull request description:
Remove CWalletTx::AcceptToMemoryPool
Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay
Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.
Obviously, working on implementing https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984 to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly
ACKs for top commit:
MarcoFalke:
re-ACK fb62f128bb
Sjors:
re-ACK fb62f128bb
Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f
d6b3640ac7 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b568 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698bc2 [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)
Pull request description:
The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes#15878
ACKs for top commit:
achow101:
Code Review ACK d6b3640ac7
l2a5b1:
re-ACK d6b3640
MarcoFalke:
ACK d6b3640ac7
Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
IsCoinBase check is already performed early by
AcceptToMemoryPoolWorker
GetDepthInMainChain check is already perfomed by
BroadcastTransaction
To avoid deadlock we MUST keep lock order in
ResendWalletTransactions and CommitTransaction,
even if we lock cs_main again further.
in BroadcastTransaction. Lock order will need
to be clean at once in a future refactoring
7a0c224289 Suppress output in test_bitcoin for expected errors (Gert-Jaap Glasbergen)
Pull request description:
Closes#15944
This adds two methods to noui, that allows temporarily suppressing (and then resuming) the output from `noui`. For situations where errors are expected, it's confusing for the test binary to output an error and then conclude with `No errors detected`.
It also uses this supress/reconnect in the tests that currently produce verbose errors when running `test_bitcoin`.
Output of `test_bitcoin` on current master:
```
gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_943311758/tempdir/path_does_not_exist" does not exist
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_643733972/tempdir/not_a_directory.dat" is not a directory
Error: Specified -walletdir "wallets" is a relative path
*** No errors detected
```
Output after this code is merged:
```
gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
*** No errors detected
```
ACKs for top commit:
l2a5b1:
ACK 7a0c224 - tested and reviewed.
laanwj:
ACK 7a0c224289
Tree-SHA512: c7881f7a431a065329360ffa9937ce4742694c646c90c019d3aff95dfd7fccbdcda9116c5762feb6dfd1108d14f9fb386e203b173c4bde9093afb2b8c977d13d
c5d3787367 Allow createwallet to take empty passwords to make unencrypted wallets (Andrew Chow)
Pull request description:
Allow createwallet to take the empty string as a password and interpret that as leaving the wallet unencrypted. Also warn when that happens.
This fixes a bug where it was not possible to use the `avoid_reuse` option for new unencrypted wallets without using named arguments.Thus this allows more `createwallet` options to be added that can be set on unencrypted wallets when using positional arguments.
ACKs for top commit:
jnewbery:
code review ACK c5d3787367
meshcollider:
re-utACK c5d3787367
ryanofsky:
utACK c5d3787367. Changes since last review are rebasing, concatenating warning strings to avoid discarding warnings, adding release notes, and choosing an unambiguous wallet name for the test.
Tree-SHA512: 146737a728dd614ba94d4b166b27e8c9e195badd1709ccab2315afe59176d9b493dfba9b61c3ed81090f059c7e464d709deb06d99451b9a3fff667f527d6f7c9
0b1f4b3c66 wallet: Drop unused OldKey (João Barbosa)
Pull request description:
Replaces #16494, `OldKey` (previously `CWalletKey`) was never serialized in the code history which means that unserialization support is not required, so remove the code entirely.
ACKs for top commit:
jnewbery:
ACK 0b1f4b3c66
laanwj:
ACK 0b1f4b3c66
fanquake:
ACK 0b1f4b3c66
Tree-SHA512: 92e9b2d6fc41f2765492d5d69d18fc4302c40ab44f28c8c30ca652c72767fbc484848c51a38ecf1f447849767a583c398784408bb5f64f9c86f9a5872b325ffc
80ba4241a6 extract min & max depth onto coin control (Amiti Uttarwar)
Pull request description:
- Refactor `AvailableCoins` to pull min & max depths from coin control.
- Add `m_max_depth` to coin control to support this.
- Addresses issue https://github.com/bitcoin/bitcoin/issues/15823, see thread for further details.
ACKs for top commit:
laanwj:
ACK 80ba4241a6
Tree-SHA512: 8f7c0aa90b3bc3667baf6741b1da2829f3919e1df92ae097d86c6b239f0c024eb410d7100e6251ea8fc49d022fb5a1214bf79b0f8b0014945b7784b2311647d1
CMerkleTx only exists as a base class for CWalletTx and for wallet file
serialization/deserialization. Move CMerkleTx methods into CWalletTx,
but leave class hierarchy and serialization logic in place.
fa6f22bf44 wallet: Rename CWalletKey to OldKey (MarcoFalke)
fa6dc7fa5f wallet: Enumerate walletdb keys (MarcoFalke)
Pull request description:
It is nice to see all the keys that exists in a single enum
Also, rename CWalletKey to OldKey and update the outdated documentation
ACKs for top commit:
laanwj:
ACK fa6f22bf44, I'm a big fan of this kind of change as it prevents typos, which can happen with 'magic' strings in the code.
promag:
ACK fa6f22bf44. @jnewbery suggestions are great followups, I think this is good enough.
meshcollider:
utACK fa6f22bf44
achow101:
Code review ACK fa6f22bf44
fanquake:
ACK fa6f22bf44 - I had a quick look over, definitely prefer this to strings floating around everywhere.
Tree-SHA512: 8ac3abd5a0d22dac1d77b8f97fe1e16c2608d650f3e9d6dd1df2fd5aeb35ef6643dfd4cd5c162404bb0100343c927d66df04dc695507ffc84a6c667e603acc54
35e60e790f Remove ReadVersion and WriteVersion (Andrew Chow)
b3d4f6c961 Log the actual wallet file version (Andrew Chow)
c88e87c3b2 Remove nFileVersion from CWalletScanState (Andrew Chow)
Pull request description:
The wallet file version is stored in the "minversion" record, not the "version" record. However "version" is no longer used anywhere except to record the highest versioned client which has opened a wallet file (which is currently only used to check whether this was most recently opened by a 0.4.0 or 0.5.0rc1 client which had a broken wallet encryption implementation). Furthermore, "version" was logged to the debug.log which is confusing because it is not the actual wallet file version.
This PR changes it so that this confusion largely no longer exists. The wallet file version logging is changed to use "minversion" and reading and writing the "version" record is no longer publicly exposed to prevent potential confusion about whether the actual file version is being read or written. Lastly, in the one place it is actually used, the variable name is changed from nFileVersion to last_client to better reflect what that record actually represents.
ACKs for top commit:
jb55:
ACK 35e60e7, I compiled locally as a quick sanity check.
ryanofsky:
utACK 35e60e790f. This code still pretty confusing, but a little simpler now. And the previous log statement was really misleading and useless compared to the new one here.
meshcollider:
Looks good, thanks! utACK 35e60e790f
Tree-SHA512: f782b2f215d07fbc9b806322bda8085445b81c02b65ca674a8c6a3e1de505a0abd050669afe0ead4778816144a1c18462e13930071cedb7227a058aeb39493f7
fa4a605a4c Remove wallet settings from chainparams (MarcoFalke)
Pull request description:
Feels a bit odd to have wallet setting in the chainparams, so remove them from there
ACKs for top commit:
promag:
ACK fa4a605a4c, missed s/2018/2019?
practicalswift:
utACK fa4a605a4c
darosior:
ACK fa4a605a4c
Tree-SHA512: 2b3a5ee85d36af290d7db80bed1339e3c684607f1ce61cc65c906726e9174e40325fb1f67a34d8780f2a61fa39a1785e7c3a1cef5b6d6c364f38db5300cdbe3a
2f7eb772f6 Add RPC bumpfee totalFee deprecation test (Jon Atack)
a92d9ce8cf deprecate totalFee argument in bumpfee RPC call (Gregory Sanders)
Pull request description:
totalFee argument is of questionable use, and should be removed in favor of feerate-based features.
I first moved IsDeprecatedRPCEnabled because `bitcoin-wallet` doesn't link `libbitcoin_server`.
ACKs for top commit:
ryanofsky:
utACK 2f7eb772f6. Only change since last review is leaving IsDeprecatedRPCEnabled in its happy home, and switching to rpcEnableDeprecated instead. (Thanks!)
jonatack:
ACK 2f7eb772f6. Built locally, manually tested rpc bumpfee, help output ([gist](https://gist.github.com/jonatack/863673eacc02f9da39ff6d6712f9d837)), all tests pass. Travis failures appears to be unrelated, the [bitcoin builds are green](https://bitcoinbuilds.org/index.php?build=121).
meshcollider:
Code Review ACK 2f7eb772f6
Tree-SHA512: c97465205ee59575df37894bcbb6c4ecf8858dd8fe9d89503f9342b226768c1dcb553153bc9eb3055f7bf5eb41573e48b8efa57e083cd255793cbe5280f0026a
40ad2f6a58 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow)
78941da5ba Optionally allow ImportScripts to set script creation timestamp (Andrew Chow)
94bf156f39 Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow)
a00d1e5ec5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow)
c6a8274247 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow)
fae7a5befd Log when an import is being skipped because we already have it (Andrew Chow)
ab28e31c95 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow)
Pull request description:
#15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet.
ACKs for top commit:
MarcoFalke:
ACK 40ad2f6a58 (checked that behavior changes are mentioned in the commit body)
ryanofsky:
utACK 40ad2f6a58. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit)
Sjors:
ACK 40ad2f6a5. Those extra tests also pass.
Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98
Also removes the now unused ImportAddress and ImportScript from rpcdump.cpp
Behavior changes:
* No errors will be thrown when the script or key already exists in the wallet.
* If the key or script is already in the wallet, their labels will be updated.
-BEGIN VERIFY SCRIPT-
sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h
sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h
sed -i 's/\b_("\([^"]\|\\"\)*")/&.translated/g' $(git grep --files-with-matches '\b_("' src)
echo Hard cases - multiline strings.
sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp
sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp
sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp
sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp
echo Special case.
sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py
-END VERIFY SCRIPT-
The "version" record that these functions read and write are not
used anywhere in the code except for one place. There is no reason
to expose these functions publicly. Furthermore, this avoids potential
confusion as developers may mistake these functions for actually
reading and writing the wallet version when they do not.
nFileVersion is not the actual file version and is not used except
in one place. So it is removed from CWalletScanState and changed so
that it is just read at the place it is needed. Furthermore, the
"version" record now only indicates the version of the highest
versioned client that has opened a wallet file so the variable
name is changed accordingly
The logic before would only include watchonly addresses if it was
explicitly set in the rpc argument.
This changes the logic like so:
If the include_watchonly argument is missing, check the
WALLET_FLAG_DISABLE_PRIVATE_KEYS flag to determine if we're working
with a watchonly wallet. If so, default include_watchonly to true.
If the include_watchonly argument is explicit set to false, we still
disable them from the listing. Although this would always return
nothing, it might be still useful in situations where you want to
explicitly filter out watchonly addresses regardless of what wallet
you are dealing with.
Signed-off-by: William Casarin <jb55@jb55.com>
e10e1e8db0 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders)
d9ff862f2d CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders)
Pull request description:
The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used.
Implementers such as myself may fail to complete this pattern, and could result in key re-use: https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393
Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned.
Those failure cases appear to be:
`CommitTransaction` failing to get the transaction into the mempool
Belt and suspenders check in `WalletModel::prepareTransaction`
Alternative to https://github.com/bitcoin/bitcoin/pull/15796
ACKs for top commit:
achow101:
ACK e10e1e8db0 Reviewed the diff
stevenroose:
utACK e10e1e8db0
meshcollider:
utACK e10e1e8db0
Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b
682a1d0f20 refactoring: remove mapBlockIndex global (James O'Beirne)
55d525ab90 refactoring: make pindexBestInvalid internal to validation.cpp (James O'Beirne)
4ed55dfcd7 refactoring: add block_index_candidates arg to LoadBlockIndex (James O'Beirne)
613c46fe9e refactoring: move block metadata structures into BlockManager (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
Under an assumeutxo model, we have multiple CChainState instances in use at once in order to support background validation. Currently, each CChainState instance has its own mapBlockIndex, a collection of linked block headers, in addition to a few other data structures that are related to maintenance of the block tree but not necessarily to any given chainstate.
In order to avoid duplicating this data across chainstates, this change moves chainstate-agnostic block metadata (and related behavior) into a class, `BlockManager`. Chainstates are parameterized with a reference to a blockmanager instance and in practice they share the same instance.
Most of this change is conceptually move-only, though the diff is somewhat muddled. The first commit can be reviewed slightly more easily with `--color-moved=dimmed_zebra`. Admittedly, that commit is pretty unwieldy; I tried to split it up after the fact with `git add --patch`, but that was difficult because of git's inability to split hunks past a certain point. Some of the moves also ended up being obscured when done over separate commits.
ACKs for top commit:
MarcoFalke:
ACK 682a1d0f20
ryanofsky:
utACK 682a1d0f20, only changes since last review were rebase and fixing conflict on a moved line
ariard:
utACK 682a1d0. Most of the changes are move-only, with main problem being to avoid creating circular dependencies between `BlockManager` and `CChainState`. Tested, comments are mostly nits, feel free to ignore them
Tree-SHA512: 738d8d06539ba53acf4bd2d48ae000473e645bbc4e63d798d55d247a4d5a4f781b73538ed590f6407be9ab402ea9d395570ea20bff0a4b9ce747bcc1600c5108
The internal bool was only to indicate whether the given label should
be applied as things that are internal should not have a label. To make
this clearer, we change internal to apply_label and invert its usage
so things that have labels set this to true in order to have their labels
applied.
93ce4a0b6f Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow)
8f5b81e6ed Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow)
37a79a4fcc Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow)
16f8096e91 Move KeyOriginInfo to its own header file (Andrew Chow)
d9becff4e1 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow)
a913e3f2fb Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow)
c7797ec655 Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow)
1b699a5083 Add HaveKey and HaveCScript to SigningProvider (Andrew Chow)
Pull request description:
This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is:
```
SigningProvider -> FillableSigningProvider -> CWallet
```
`SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`).
Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure.
ACKs for top commit:
Sjors:
re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.
instagibbs:
utACK 93ce4a0b6f
Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3
1aecdf2063 Move wallet creation out of the createwallet rpc into its own function (Andrew Chow)
Pull request description:
Moves the wallet creation logic from within the `createwallet` rpc and into its own function within wallet.cpp.
ACKs for top commit:
jnewbery:
ACK 1aecdf2063
MarcoFalke:
ACK 1aecdf2063
Sjors:
ACK 1aecdf2 with some suggestions for followup.
Tree-SHA512: 8d26d7ff48db4f8fac12408a5a294f788b7f50a72e7eb4008fb74ff14d7400eb3970f8038a19f989eff55198fc11c0cf86f52231c62b9015eb777132edc8ea88
