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
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
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
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)
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
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
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.
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.
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
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
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.
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
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
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
8e7f930828 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow)
33d13edd2b Replace CReserveKey with ReserveDestinatoin (Andrew Chow)
172213be5b Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow)
Pull request description:
The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool.
ACKs for top commit:
instagibbs:
re-utACK 8e7f930828
sipa:
ACK 8e7f930828. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.
laanwj:
ACK 8e7f930828
Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
Instead of having the same multiple lines of code everywhere
that new destinations are fetched, introduce GetNewDestination as
a member function of CWallet which does the key fetching, label
setting, script generation, and destination generation.
fabc57e07d test: Log to debug.log in all tests (MarcoFalke)
fa4a04a5a9 test: use common setup in gui tests (MarcoFalke)
fad3d2a624 test: Create data dir in BasicTestingSetup (MarcoFalke)
Pull request description:
This makes it easier to debug a frozen test or a test that failed. To debug a failed test, remove the line `fs::remove_all(m_path_root);`.
The pull is done in three commits:
* Create a datadir for every unit test once (and only once). This requires the `SetDataDir` function to go away.
* Use the common setup in the gui unit tests. Some of those tests are testing the init sequence, so we'd have to undo some of what the testing setup did.
* Log to the debug.log in all tests
ACKs for top commit:
laanwj:
ACK fabc57e07d
Tree-SHA512: 73444210b88172669e2cd22c2703a1e30e105185d2d5f03decbdedcfd09c64ed208d3716c59c8bebb0e44214cee5c8095e3e995d049e1572ee98f1017e413665
fa7e311e16 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c2aa scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729242 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)
Pull request description:
This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is
* that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
* that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.
ACKs for commit fa7e31:
promag:
utACK fa7e311e16.
jnewbery:
utACK fa7e311e16
Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8
