Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from
scratch on the next load.
With this change, progress is saved every 60 seconds.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Instead of accepting a `vCoins` reference that is cleared at the beginning of the method.
Note:
This new struct, down the commits line, will contain other `AvailableCoins` useful results.
Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
049003fe68a4183f6f20da16f58f10079d1e02df coinselection: Remove COutput operators == and != (Andrew Chow)
f6c39c6adb6cbf9c87f04d3d667701905ef5c0a0 coinselection: Remove CInputCoin (Andrew Chow)
70f31f1a81710aa59e95770de9a84bf58cbce1e8 coinselection: Use COutput instead of CInputCoin (Andrew Chow)
14fbb57b79c664090f6a4e60d7bdfc9759ff4307 coinselection: Add effective value and fees to COutput (Andrew Chow)
f0821230b8de2eec21a869d1edf9e2b9f502de25 moveonly: move COutput to coinselection.h (Andrew Chow)
42e974e15c6deba1d9395a4da9341c9ebec6e8e5 wallet: Remove CWallet and CWalletTx from COutput's constructor (Andrew Chow)
14d04d5ad15ae56df56edee7ca9a202b52037889 wallet: Replace CWalletTx in COutput with COutPoint and CTxOut (Andrew Chow)
0ba4d1916e26e2a5d603edcdb7625463989d25b6 wallet: Provide input bytes to COutput (Andrew Chow)
d51f27d3bb0d6e3ca55bcd23ce53e4fe413a9360 wallet: Store whether a COutput is from the wallet (Andrew Chow)
b799814bbd53736b79495072f3c9e05989a465e8 wallet: Store tx time in COutput (Andrew Chow)
46022953ee2e8113167bafd1fd48a383a578b13c wallet: Remove use_max_sig default value (Andrew Chow)
10379f007fd2c18f4cd24d0a0783d6d929f45556 scripted-diff: Rename COutput member variables (Andrew Chow)
c7c64db41e1718584aa2f30ff27f60ab0966de62 wallet: cleanup COutput constructor (Andrew Chow)
Pull request description:
While working on coin selection code, it occurred to me that `CInputCoin` is really a subset of `COutput` and the conversion of a `COutput` to a `CInputCoin` does not appear to be all that useful. So this PR adds fields that are present in `CInputCoin` to `COutput` and replaces the usage of `CInputCoin` with `COutput`.
`COutput` is also moved to coinselection.h. As part of this move, the usage of `CWalletTx` is removed from `COutput`. It is instead replaced by storing a `COutPoint` and the `CTxOut` rather than the entire `CWalletTx` as coin selection does not really need the full `CWalletTx`. The `CWalletTx` was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to `COutput`'s constructor.
ACKs for top commit:
ryanofsky:
Code review ACK 049003fe68a4183f6f20da16f58f10079d1e02df, just adding comments and removing == operators since last review
w0xlt:
reACK 049003f
Xekyo:
reACK 049003fe68a4183f6f20da16f58f10079d1e02df
Tree-SHA512: 048b4cd620a0415e1d9fe8597257ee4bc64656566e1d28a9bdd147d6d72dc87c3f34a3339fa9ab6acf42c388df7901fc4ee900ccaabc3de790ffad162b544c15
39b1763730177cd7d6a32fd9321da640b0d65e0e Replace use of `ArgsManager` with `DatabaseOptions` (Kiminuo)
Pull request description:
Contributes to #21005.
The goal of this PR is to remove `gArgs` from database classes (i.e. `bdb.h` and `sqlite.h`) so that they can be tested without relying on `gArgs` in tests.
Notes:
* My goal is to enable unit-testing without relying on `gArgs` as much as possible. Global variables are hard to reason about which in turn makes it slightly harder to contribute to this codebase. When the compiler does the heavy lifting for us and allows us only to construct an object (or call a method) with valid parameters, we may also save some time in code reviews. The cost for this is passing an argument which is not for free but the cost is very miniscule compared to benefits, I think.
* GUI code is an exception because it seems fine to have `gArgs` there so I don't plan to make changes in `src/qt` folder, for example.
* My approach to removal of `gArgs` uses is moving from lower levels to upper ones and pass `ArgsManager` as an argument as needed. The approach is very similar to what #20158.
ACKs for top commit:
achow101:
ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e
ryanofsky:
Code review ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e. Just the two small ReadDatabaseArgs and Berkeley open changes that were discussed since the last review
Tree-SHA512: aa066b314db593e46c18698fe8cdd500f558b405dc04e4a9a3ff57b52b5b3a81a6cb090e0e661785d1d02c1bf18958c1f4cd715ff233aab63381e3f80960622d
Instead of having a pointer to the CWalletTx in COutput, we can just
store the COutPoint and the CTxOut as those are the only things we need
from the CWalletTx. Other things CWalletTx used to provide were time and
fIsFromMe but these are also being stored by COutput.
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
A change to BlockManager::LookupBlockIndex:
- Previously, it was a const member function returning a non-const CBlockIndex*
- Now, there's are const and non-const versions of
BlockManager::LookupBlockIndex returning a CBlockIndex with the same
const-ness as the member function:
(e.g. const CBlockIndex* LookupBlockIndex(...) const)
See next commit for some weirdness that this eliminates.
The range based for-loops are modernize (using auto + destructuring) in
a future commit.
3ee6d0788ec1b90f7c39c9644dba4011f7cf5db4 test: add more wallet conflicts assertions (S3RK)
3b98bf9c43ece060d57d7ae31624d4a8220de266 Revert "Add to spends only transcations from me" (S3RK)
Pull request description:
This reverts commit d04566415e16ae685af066384f346dff522c068f from #22929.
This commit was based on invalid assumption that `mapTxSpends` should contain only outgoing txs and broke wallet conflicts feature.
ACKs for top commit:
achow101:
ACK 3ee6d0788ec1b90f7c39c9644dba4011f7cf5db4
Tree-SHA512: bf5a77ced6bac57d5eb85771d9189c53e1edc295d179ed5a1bdce18e365794a9101b4cecf35387b27f67260db3b47f7214e7876e490494529b748cceeb95632d
This removes the ability to set an offset in the SpanReader constructor,
as the current code is broken. All call sites use pos=0, so it is actually
unused. If future call sites need it, SpanReader{a, b, c, d} is equivalent
to SpanReader{a, b, c.subspan(d)}.
It also removes the ability to deserialize from SpanReader directly from
the constructor. This too is unused, and can be more idiomatically
simulated using (SpanReader{a, b, c} >> x >> y >> z) instead of
SpanReader{a, b, c, x, y, z}.
2c35a93b3cc19dc71d5664f9f61c24a04f419e35 Generalize/simplify VectorReader into SpanReader (Pieter Wuille)
Pull request description:
Originally written for #21590 (safegcd-based MuHash inverses), but then found a better way that removed the need for it, so I'm submitting it independently.
ACKs for top commit:
MarcoFalke:
re-ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35 🖨
shaavan:
ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35
Tree-SHA512: 959e3251e0cfe20e13a50639b617c9dc2a561d613a0884d983c93d15dacb6d2305d760aa933d18ba055cef8a1651a344bcb6b3f93051ecf26d3f2efc5779efa4
3d71d16d1eb4173c70d4c294559fc2365e189856 test: listtranscations with externally generated addresses (S3RK)
d04566415e16ae685af066384f346dff522c068f Add to spends only transcations from me (S3RK)
9f3a622b1cea37e452560f2f82d8e82d3b48a73a Automatically add labels to detected receiving addresses (S3RK)
c1b99c088c54eb101c0a28a67237965576ccf5ad Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses (S3RK)
03840c20640685295a65ed8c82456e877f668b9b Add CWallet::IsInternalScriptPubKeyMan (S3RK)
456e350926adde5dabdbc85fc0f017fb29bdadb3 wallet: resolve ambiguity of two ScriptPubKey managers providing same script (S3RK)
Pull request description:
This PR fixes certain use-cases when **send-to-self** transactions are missing from `listtransactions` output.
1. When a receiving address is generated externally to the wallet
(e.g. same wallet running on two nodes, or by 3rd party from xpub)
2. When restoring backup with lost metadata, but keypool gap is not exceeded yet
When the block is connected or tx added to mempool we already mark used keys. This PR extends this logic to determine whether the destination is a receiving one and if yes add it to the address book with empty label.
Works both for legacy and descriptors wallets.
- For legacy it uses the internal flag from the keypool entry. Caveat: because we don't know which script type would be used we add all possible destinations for such keys.
- For descriptor wallets it uses internal flag for the script pub key manager. Caveat: it only works for active descriptors.
fixes#19856fixes#20293
ACKs for top commit:
laanwj:
Code review ACK 3d71d16d1eb4173c70d4c294559fc2365e189856
Tree-SHA512: 03fafd5548ead0c4ffe9ebcc9eb2849f1d2fa7270fda4166419b86877d4e57dcf04460e465fbb9c90b42031f3c05d1b83f1b67a9f82c2a42980825ed1e7b52e6
f13a22a631efe01e1fbae4ae78a4901d14ebda3c wallet: Call load handlers without cs_wallet locked (João Barbosa)
Pull request description:
Don't have `cs_wallet` locked while calling each `context.wallet_load_fns`. A load handler can always lock `cs_wallet` if needed.
The lock was added in 1c7e25db0c to satisfy TSAN. With 44c430ffac most of the code requiring the lock is in `CWallet::AttachChain`. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
ACKs for top commit:
meshcollider:
re-utACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c
ryanofsky:
Code review ACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c. Only change since last review is adding a lock order comment
jonatack:
ACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c
Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62
Current CWalletTx state representation makes it possible to set
inconsistent states that won't be handled correctly by wallet sync code
or serialized & deserialized back into the same form.
For example, it is possible to call setConflicted without setting a
conflicting block hash, or setConfirmed with no transaction index. And
it's possible update individual m_confirm and fInMempool data fields
without setting an overall consistent state that can be serialized and
handled correctly.
Fix this without changing behavior by using std::variant, instead of an
enum and collection of fields, to represent sync state, so state
tracking code is safer and more legible.
This is a first step to fixing state tracking bugs
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
by adding an extra margin of safety that can prevent new bugs from being
introduced as existing bugs are fixed.
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
For the generic wallet tests, make DescriptorScriptPubKeyMans. There are
still some wallet tests that test legacy wallet things. Those remain
unchanged.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
92993aa5cf37995e65e68dfd6f129ecaf418e01c Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b828a557f8262d9dc14ff7a03f813f7 Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51115454c6a699be9492024f7b46c2b4 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
meshcollider:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
No change in behavior. This just moves some code from the ListCoins test
setup to a reusable util function, so it can be reused in a new test in
the next commit.
f5ba424cd44619d9b9be88b8593d69a7ba96db26 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky)
62252c95e5aa55f33a5ef22292d5d8161fcb892a interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky)
985430d9b2e183c1f59a34472e413a8d00a7e6da test: Add gui test for wallet receive requests (Russell Yanofsky)
Pull request description:
Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.
There are no changes in behavior.
ACKs for top commit:
jarolrod:
tACK f5ba424cd44619d9b9be88b8593d69a7ba96db26
laanwj:
Code review ACK f5ba424cd44619d9b9be88b8593d69a7ba96db26
Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0
