aboutsummaryrefslogtreecommitdiff
path: root/src/wallet/wallet.cpp
AgeCommit message (Collapse)Author
2022-12-07Merge bitcoin/bitcoin#26298: refactor: Move src/interfaces/*.cpp files to ↵fanquake
libbitcoin_common.a b19c4124b3c9a15fe3baf8792c55eb26eca51c0f refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky) dd6e8bd71c6025a51d88000caf28121ec00499db build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake) 82e272a109281f750909d1feade784c778d8b592 refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a (Ryan Ofsky) Pull request description: These belong in `libbitcoin_common.a`, not `libbitcoin_util.a`, because they aren't general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in `libbitcoin_util.a` is to prevent them from being used by the kernel library. Also rename ambiguous `MakeHandler` functions to `MakeCleanupHandler` and `MakeSignalHandler`. Cleanup function handler was introduced after boost signals handler, so original naming didn't make much sense. This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes. This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the _util_ library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase. ACKs for top commit: hebasto: ACK b19c4124b3c9a15fe3baf8792c55eb26eca51c0f Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
2022-12-06Merge bitcoin/bitcoin#26238: clang-tidy: fixup named argument commentsMarcoFalke
203886c443c4ad76f8a1dba740a286e383e55206 Fixup clang-tidy named argument comments (fanquake) Pull request description: Fix comments so they are checked/consistent. Fix incorrect comments. ACKs for top commit: hebasto: ACK 203886c443c4ad76f8a1dba740a286e383e55206, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: e1257840f91fe39842e2b19299c1633604697b8584fe44b1977ada33cdde5433c877ed0b669fa334e20b04971dc89cd47d58b2783b6f7004521f01d05a1245da
2022-12-05Merge bitcoin/bitcoin#26462: wallet: fix crash on loading descriptor wallet ↵Andrew Chow
containing legacy key type entries 3198e4239e848bbb119e3638677aa9bcf8353ca6 test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner) 349ed2a0eed3aaaf199ead93057c97730869c3a3 wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner) Pull request description: Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details): ``` $ ./src/bitcoin-cli createwallet crashme $ ./src/bitcoin-cli unloadwallet crashme $ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat SQLite version 3.38.2 2022-03-26 13:51:10 Enter ".help" for usage hints. sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00'); $ ./src/bitcoin-cli loadwallet crashme --- bitcoind output: --- 2022-11-06T13:51:01Z Using SQLite Version 3.38.2 2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme 2022-11-06T13:51:01Z init message: Loading wallet… 2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900 Segmentation fault (core dumped) ``` Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: https://github.com/bitcoin/bitcoin/blob/50422b770a40f5fa964201d1e99fd6b5dc1653ca/src/wallet/walletdb.cpp#L589-L594 ~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~ ~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~ This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading. ACKs for top commit: achow101: ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6 aureleoules: ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6 Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
2022-12-05Fixup clang-tidy named argument commentsfanquake
Fix comments so they are checked/consistent. Fix incorrect arguments.
2022-12-01Merge bitcoin/bitcoin#26594: wallet: Avoid a segfault in migratewallet ↵fanquake
failure cleanup 5e65a216d1fd00c447757736d4f2899d235e731a wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow) 88afc73ae0c67a4482ecd3d77eb2a8fd2673f82d tests: Test for migrating encrypted wallets (Andrew Chow) 86ef7b3c7be84e4183098f448c77ecc9ea7367ab wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow) Pull request description: When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault. This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached. This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue. ACKs for top commit: S3RK: reACK 5e65a216d1fd00c447757736d4f2899d235e731a stickies-v: ACK [5e65a21](https://github.com/bitcoin/bitcoin/commit/5e65a216d1fd00c447757736d4f2899d235e731a) furszy: diff ACK 5e65a21 Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
2022-11-29wallet: Avoid null pointer deref when cleaning up migratewalletAndrew Chow
If migratewallet fails, we do a cleanup which removes the watchonly and solvables wallets if they were created. However, if they were not, their pointers are nullptr and we don't check for that, which causes a segfault during the cleanup. So check that they aren't nullptr before cleaning them up.
2022-11-29refactor: Rename ambiguous interfaces::MakeHandler functionsRyan Ofsky
2022-11-08wallet: throw error if legacy entries are present on loading descriptor walletsSebastian Falbesoner
In the wallet key-value-loading routine, most legacy type entries require a LegacyScriptPubKeyMan instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. Fix this by throwing an error if if the wallet flags indicate that we have a descriptor wallet and there is a legacy entry found.
2022-10-26Merge bitcoin/bitcoin#25957: wallet: fast rescan with BIP157 block filters ↵Andrew Chow
for descriptor wallets 0582932260e7de4e8aba01d63e7c8a9ddb9c3685 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner) ca48a4694f73e5be8f971ae482ebc2cce4caef44 rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner) 3449880b499d54bfbcf6caeed52851ce55259ed7 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner) 935c6c4b234bbb0565cda6f58ee298048856acae wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner) 70b35139040a2351c845a1cec1dafd2fbcd16e93 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner) c051026586fb269584bcba41de8a4a90280f5a7e wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner) 845279132b494f03b84d689c666fdcfad37f5a42 wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner) 088e38d3bbea9694b319bc34e0d2e70d210c38b4 add chain interface methods for using BIP 157 block filters (Sebastian Falbesoner) Pull request description: ## Description This PR is another take of using BIP 157 block filters (enabled by `-blockfilterindex=1`) for faster wallet rescans and is a modern revival of #15845. For reviewers new to this topic I can highly recommend to read the corresponding PR review club (https://bitcoincore.reviews/15845). The basic idea is to skip blocks for deeper inspection (i.e. looking at every single tx for matches) if our block filter doesn't match any of the block's spent or created UTXOs are relevant for our wallet. Note that there can be false-positives (see https://bitcoincore.reviews/15845#l-199 for a PR review club discussion about false-positive rates), but no false-negatives, i.e. it is safe to skip blocks if the filter doesn't match; if the filter *does* match even though there are no wallet-relevant txs in the block, no harm is done, only a little more time is spent extra. In contrast to #15845, this solution only supports descriptor wallets, which are way more widespread now than back in the time >3 years ago. With that approach, we don't have to ever derive the relevant scriptPubKeys ourselves from keys before populating the filter, and can instead shift the full responsibility to that to the `DescriptorScriptPubKeyMan` which already takes care of that automatically. Compared to legacy wallets, the `IsMine` logic for descriptor wallets is as trivial as checking if a scriptPubKey is included in the ScriptPubKeyMan's set of scriptPubKeys (`m_map_script_pub_keys`): https://github.com/bitcoin/bitcoin/blob/e191fac4f3c37820f0618f72f0a8e8b524531ab8/src/wallet/scriptpubkeyman.cpp#L1703-L1710 One of the unaddressed issues of #15845 was that [the filter was only created once outside the loop](https://github.com/bitcoin/bitcoin/pull/15845#discussion_r343265997) and as such didn't take into account possible top-ups that have happened. This is solved here by keeping a state of ranged `DescriptorScriptPubKeyMan`'s descriptor end ranges and check at each iteration whether that range has increased since last time. If yes, we update the filter with all scriptPubKeys that have been added since the last filter update with a range index equal or higher than the last end range. Note that finding new scriptPubKeys could be made more efficient than linearly iterating through the whole `m_script_pub_keys` map (e.g. by introducing a bidirectional map), but this would mean introducing additional complexity and state and it's probably not worth it at this time, considering that the performance gain is already significant. Output scripts from non-ranged `DescriptorScriptPubKeyMan`s (i.e. ones with a fixed set of output scripts that is never extended) are added only once when the filter is created first. ## Benchmark results Obviously, the speed-up indirectly correlates with the wallet tx frequency in the scanned range: the more blocks contain wallet-related transactions, the less blocks can be skipped due to block filter detection. In a [simple benchmark](https://github.com/theStack/bitcoin/blob/fast_rescan_functional_test_benchmark/test/functional/pr25957_benchmark.py), a regtest chain with 1008 blocks (corresponding to 1 week) is mined with 20000 scriptPubKeys contained (25 txs * 800 outputs) each. The blocks each have a weight of ~2500000 WUs and hence are about 62.5% full. A global constant `WALLET_TX_BLOCK_FREQUENCY` defines how often wallet-related txs are included in a block. The created descriptor wallet (default setting of `keypool=1000`, we have 8*1000 = 8000 scriptPubKeys at the start) is backuped via the `backupwallet` RPC before the mining starts and imported via `restorewallet` RPC after. The measured time for taking this import process (which involves a rescan) once with block filters (`-blockfilterindex=1`) and once without block filters (`-blockfilterindex=0`) yield the relevant result numbers for the benchmark. The following table lists the results, sorted from worst-case (all blocks contain wallte-relevant txs, 0% can be skipped) to best-case (no blocks contain walltet-relevant txs, 100% can be skipped) where the frequencies have been picked arbitrarily: wallet-related tx frequency; 1 tx per... | ratio of irrelevant blocks | w/o filters | with filters | speed gain --------------------------------------------|-----------------------------|-------------|--------------|------------- ~ 10 minutes (every block) | 0% | 56.806s | 63.554s | ~0.9x ~ 20 minutes (every 2nd block) | 50% (1/2) | 58.896s | 36.076s | ~1.6x ~ 30 minutes (every 3rd block) | 66.67% (2/3) | 56.781s | 25.430s | ~2.2x ~ 1 hour (every 6th block) | 83.33% (5/6) | 58.193s | 15.786s | ~3.7x ~ 6 hours (every 36th block) | 97.22% (35/36) | 57.500s | 6.935s | ~8.3x ~ 1 day (every 144th block) | 99.31% (143/144) | 68.881s | 6.107s | ~11.3x (no txs) | 100% | 58.529s | 5.630s | ~10.4x Since even the (rather unrealistic) worst-case scenario of having wallet-related txs in _every_ block of the rescan range obviously doesn't take significantly longer, I'd argue it's reasonable to always take advantage of block filters if they are available and there's no need to provide an option for the user. Feedback about the general approach (but also about details like naming, where I struggled a lot) would be greatly appreciated. Thanks fly out to furszy for discussing this subject and patiently answering basic question about descriptor wallets! ACKs for top commit: achow101: ACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685 Sjors: re-utACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685 aureleoules: ACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685 - minor changes, documentation and updated test since last review w0xlt: re-ACK https://github.com/bitcoin/bitcoin/pull/25957/commits/0582932260e7de4e8aba01d63e7c8a9ddb9c3685 Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
2022-10-25wallet: fast rescan: show log message for every non-skipped blockSebastian Falbesoner
For that purpose, a new logging category BCLog::SCAN is introduced.
2022-10-25wallet: take use of `FastWalletRescanFilter`Sebastian Falbesoner
Can be reviewed with `--ignore-all-space`.
2022-10-25wallet: add `FastWalletRescanFilter` class for speeding up rescansSebastian Falbesoner
This only supports wallet descriptors right now.
2022-10-24Merge bitcoin/bitcoin#26302: refactor: Use type-safe time point for ↵fanquake
CWallet::m_next_resend fa51cc965110e14661c848364a29c493287673be refactor: Use type-safe time point for CWallet::m_next_resend (MacroFake) Pull request description: `GetTime` is not type-safe, thus deprecated, see https://github.com/bitcoin/bitcoin/blob/75cbbfa279685f70d9f6fa71432df00862ffa865/src/util/time.h#L62-L70 ACKs for top commit: shaavan: Code Review ACK fa51cc965110e14661c848364a29c493287673be aureleoules: ACK fa51cc965110e14661c848364a29c493287673be Tree-SHA512: 030de10070518580763ea75079442e2f934c54d3083be3ebe35e7f1bc6db2096745bb46d95aa1e6efe29ced30a048acfe5cd999178e6787b7647dfbec5ecb444
2022-10-13Merge bitcoin/bitcoin#25526: wallet: avoid double keypool TopUp() call on ↵Andrew Chow
descriptor wallets bfb9b94ebefdb95ac7656836975b3d5afc428744 wallet: remove duplicate descriptor type check in GetNewDestination (furszy) 76b982a4a5328c1357dbc5361317f682db160876 wallet: remove unused `nAccountingEntryNumber` field (furszy) 599ff5adfc7e1227c6d97d861d0715aee57611dd wallet: avoid double TopUp() calls on descriptor wallets (furszy) Pull request description: Found it while was digging over a `getnewaddress` timeout on the functional test suite. ### Context: We are calling `TopUp()` twice in the following flows for descriptor wallets: A) `CWallet::GetNewDestination`: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again. B) `CWallet::GetReservedDestination`: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again). ### Changes: Move `TopUp()` responsibility from the wallet class to each scriptpubkeyman. So each spkm can decide to call it or not after perform the basic checks for the new destination request. Aside from that, remove the unused `nAccountingEntryNumber` wallet field. And a duplicated descriptor type check in `GetNewDestination` ACKs for top commit: aureleoules: re-ACK bfb9b94ebefdb95ac7656836975b3d5afc428744. achow101: ACK bfb9b94ebefdb95ac7656836975b3d5afc428744 theStack: Code-review ACK bfb9b94ebefdb95ac7656836975b3d5afc428744 Tree-SHA512: 3ab73f37729e50d6c6a4434f676855bc1fb404619d63c03e5b06ce61c292c09c59d64cb1aa3bd9277b06f26988956991d62c90f9d835884f41ed500b43a12058
2022-10-13refactor: Use type-safe time point for CWallet::m_next_resendMacroFake
2022-10-13Merge bitcoin/bitcoin#26205: wallet: #25768 follow upsfanquake
b01682a812f0841170657708ef0e896b904fcd77 refactor: revert m_next_resend to not be std::atomic (stickies-v) 9245f456705b285e2d9afcc01a6155e1b3f92fad wallet: only update m_next_resend when actually resending (stickies-v) 7fbde8af5c06694eecd4ce601109bd826a54bd6f refactor: carve out tx resend timer logic into ShouldResend (stickies-v) 01f3534632d18c772901fb6ce22f6394eae96799 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v) c6e8e11fb030ef406752761530421a9e2f0f5d4f wallet: fix capitalization in docstring (stickies-v) Pull request description: This PR addresses the outstanding comments/issues from #25768: - capitalization [typo](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958572522) in docstring - remove [unused locks](https://github.com/bitcoin/bitcoin/commit/01f3534632d18c772901fb6ce22f6394eae96799) that we previously needed for `ReacceptWalletTransactions()` - before #25768, only `ResendWalletTransactions()` would reset `m_next_resend` (formerly called `nNextResend`). By unifying it with `ReacceptWalletTransactions()` into `ResubmitWalletTransactions()`, the number of callsites that would reset the `m_next_resend` timer increased - since `m_next_resend` is only used in case of `relay=true` (formerly `ResendWalletTransactions()`), this is unintuitive - it leads to [unexpected behaviour](https://github.com/bitcoin/bitcoin/pull/25768#issuecomment-1252619427) such as transactions potentially never being rebroadcasted. - it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r962828563) - since #25768, we relied on an earlier call of `ResubmitWalletTransactions(relay=false, force=true)` to initialize `m_next_resend()`, I think we can more elegantly do that by just providing `m_next_resend` with a default value - just to highlight: this commit introduces behaviour change Note: the `if (!fBroadcastTransactions)` in `CWallet:ShouldResend()` is duplicated on purpose, since it potentially avoids the slightly more expensive `if (!chain().isReadyToBroadcast())` check afterwards. I don't have a strong view on it, so happy to remove that additional check to reduce the diff, too. ACKs for top commit: aureleoules: ACK b01682a812f0841170657708ef0e896b904fcd77 achow101: ACK b01682a812f0841170657708ef0e896b904fcd77 Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c
2022-10-10Merge bitcoin/bitcoin#26282: wallet: have prune error take precedence over ↵fanquake
assumedvalid 1c36bafc5f7db268546dcc86c793071a7e9d35e0 wallet: have prune error take precedence over assumedvalid (James O'Beirne) Pull request description: Fixes https://github.com/bitcoin/bitcoin/pull/23997#discussion_r891412739. From Russ Yanofsky: > Agree with all of Marco's points here and think this should be updated > > If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" ACKs for top commit: MarcoFalke: ACK 1c36bafc5f7db268546dcc86c793071a7e9d35e0 aureleoules: ACK 1c36bafc5f7db268546dcc86c793071a7e9d35e0 Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc
2022-10-09Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.hDimitris Tsapakidis
Fix comment typos: sigature -> signature ponter -> pointer it's key -> its key
2022-10-07wallet: have prune error take precedence over assumedvalidJames O'Beirne
From Russ Yanofsky: "Agree with all of Marco's points here and think this should be updated If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" Co-authored-by: MacroFake <MarcoFalke@gmail.com> Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
2022-09-30wallet: only update m_next_resend when actually resendingstickies-v
We only want to relay our resubmitted transactions once every 12-36h. By separating the timer update logic out of ResubmitWalletTransactions and into MaybeResendWalletTxs we avoid non-relay calls (previously in the separate ReacceptWalletTransactions function) from resetting that timer.
2022-09-30refactor: carve out tx resend timer logic into ShouldResendstickies-v
Moves the logic of whether or not transactions should actually be resent out of the function that's resending them. This reduces responsibilities of ResubmitWalletTransactions and allows carving out the updating of m_next_resend in a future commit.
2022-09-29refactor: remove unused locks for ResubmitWalletTransactionsstickies-v
ReacceptWalletTransactions is replaced by ResubmitWalletTransactions which already handles acquiring the necessary locks internally.
2022-09-29wallet: fix capitalization in docstringstickies-v
2022-09-24Merge bitcoin/bitcoin#26130: Bugfix: Wallet: Lock cs_wallet for SignMessageMacroFake
a60d9eb9e6b6a272a3fca8981d89a55955dced55 Bugfix: Wallet: Lock cs_wallet for SignMessage (Luke Dashjr) Pull request description: cs_desc_main is typically locked within scope of a cs_wallet lock, but: CWallet::IsLocked locks cs_wallet ...called from DescriptorScriptPubKeyMan::GetKeys ...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first ------------- Note this is currently only an issue for the GUI (which lacks sufficient testing apparently), but can be reproduced by #26082 (CI fails as a result) ACKs for top commit: achow101: ACK a60d9eb9e6b6a272a3fca8981d89a55955dced55 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/26130/commits/a60d9eb9e6b6a272a3fca8981d89a55955dced55 Tree-SHA512: 60f6959b0ceaf4d9339ba1a47154734034b637c41b1f9e26748a2dbbc3a2a95fc3696019103c55ae70c91d910ba8f3d7f4e27d263030eb60b689f290c4d82ea9
2022-09-20Fix nNextResend data race in ResubmitWalletTransactionsMacroFake
2022-09-20Bugfix: Wallet: Lock cs_wallet for SignMessageLuke Dashjr
cs_desc_main is typically locked within scope of a cs_wallet lock, but: CWallet::IsLocked locks cs_wallet ...called from DescriptorScriptPubKeyMan::GetKeys ...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first
2022-09-19Merge bitcoin/bitcoin#26005: Wallet: Fix error handling (copy_file failure ↵fanquake
in RestoreWallet, and in general via interfaces) c3e536555aa3a7db773170671da1256a2ace2094 Bugfix: Wallet: Return util::Error rather than non-error nullptr when CreateWallet/LoadWallet/RestoreWallet fail (Luke Dashjr) 335ff98c8a64eda38a2a2334102bd253f108c253 Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure exceptions become returned errors and incomplete wallet directory is removed (Luke Dashjr) Pull request description: Bug 1: `copy_file` can throw exceptions, but `RestoreWallet` is expected to return a nullptr with a populated `errors` parameter. This is fixed by wrapping `copy_file` and `LoadWallet` (for good measure) in a `try` block, and converting any exceptions to the intended return style. Bug 2: `util::Result` turns what would have been a `false` unique_ptr into a `true` nullptr result, which leads to nullptr dereferences in at least the 3 cases of wallet creation/loading/restoring. This is fixed by keeping the pointer as a plain `std::unique_ptr` until actually returning it (ie, after the nullptr check). Fixes https://github.com/bitcoin-core/gui/issues/661 ACKs for top commit: achow101: ACK c3e536555aa3a7db773170671da1256a2ace2094 Tree-SHA512: 4291b3dbbb147acea2e63a704324c9371bc16ecb4237f8753729b0b0a6e55c9758ad61bfe8bd432fd7b0bae95d8b63a9831e61ac8b8d5c0197b550a2e0f4a105
2022-09-16Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure ↵Luke Dashjr
exceptions become returned errors and incomplete wallet directory is removed
2022-09-16Merge bitcoin/bitcoin#25499: Use steady clock for all millis bench loggingfanquake
fa521c960337a65d4ce12cd1ef009c652ffe57e6 Use steady clock for all millis bench logging (MacroFake) Pull request description: Currently `GetTimeMillis` is used for bench logging in milliseconds integral precision. Replace it to use a steady clock that is type-safe and steady. Microsecond or float precision can be done in a follow-up. ACKs for top commit: fanquake: ACK fa521c960337a65d4ce12cd1ef009c652ffe57e6 - started making the same change. Tree-SHA512: 86a810e496fc663f815acb8771a6c770331593715cde85370226685bc50c13e8e987e3c5efd0b4e48b36ebd2372255357b709204bac750d41e94a9f7d9897fa6
2022-09-15test: Add missing syncwithvalidationinterfacequeueMacroFake
2022-09-14test/doc: Remove unused syncwithvalidationinterfacequeueMacroFake
See https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958562071 Also fix doc typo from https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958571943
2022-09-09wallet: bugfix, load wallet with an unknown descriptor cause fatal errorfurszy
If the descriptor entry is unrecognized/corrupt, the unserialization fails and `LoadWallet` instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized/corrupted descriptor are scanned, a fatal error is thrown.
2022-09-05Merge bitcoin/bitcoin#25768: wallet: Properly rebroadcast unconfirmed ↵glozow
transaction chains 3405f3eed5cf841b23a569b64a376c2e5b5026cd test: Test that an unconfirmed not-in-mempool chain is rebroadcast (Andrew Chow) 10d91c5abe9ed7dcc237c9d52c588e7d26e162a4 wallet: Deduplicate Resend and ReacceptWalletTransactions (Andrew Chow) Pull request description: Currently `ResendWalletTransactions` (used for normal rebroadcasts) will attempt to rebroadcast all of the transactions in the wallet in the order they are stored in `mapWallet`. This ends up being random as `mapWallet` is a `std::unordered_map`. However `ReacceptWalletTransactions` (used for adding to the mempool on loading) first sorts the txs by wallet insertion order, then submits them. The result is that `ResendWalletTranactions` will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid. This PR resolves this issue by combining `ReacceptWalletTransactions` and `ResendWalletTransactions` into a new `ResubmitWalletTransactions` so that the iteration code and basic checks are shared. A test has also been added that checks that such transaction chains are rebroadcast correctly. ACKs for top commit: naumenkogs: utACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd 1440000bytes: reACK https://github.com/bitcoin/bitcoin/pull/25768/commits/3405f3eed5cf841b23a569b64a376c2e5b5026cd furszy: Late code review ACK 3405f3ee stickies-v: ACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd Tree-SHA512: 1240d9690ecc2ae8d476286b79e2386f537a90c41dd2b8b8a5a9c2a917aa3af85d6aee019fbbb05e772985a2b197e2788305586d9d5dac78ccba1ee5aa31d77a
2022-09-01Merge bitcoin/bitcoin#19602: wallet: Migrate legacy wallets to descriptor ↵Andrew Chow
wallets 53e7ed075c49f853cc845afc7b2f058cabad0cb0 doc: Release notes and other docs for migration (Andrew Chow) 9c44bfe244f35f08ba576d8b979a90dcd68d2c77 Test migratewallet (Andrew Chow) 0b26e7cdf2659fd8b54d21fd2bd749f9f3e87af8 descriptors: addr() and raw() should return false for ToPrivateString (Andrew Chow) 31764c3f872f4f01b48d50585f86e97c41554954 Add migratewallet RPC (Andrew Chow) 0bf7b38bff422e7413bcd3dc0abe2568dd918ddc Implement MigrateLegacyToDescriptor (Andrew Chow) e7b16f925ae5b117e8b74ce814b63e19b19b50f4 Implement MigrateToSQLite (Andrew Chow) 5b62f095e790a0d4e2a70ece89465b64fc68358a wallet: Refactor SetupDescSPKMs to take CExtKey (Andrew Chow) 22401f17e026ead4bc3fe96967eec56a719a4f75 Implement LegacyScriptPubKeyMan::DeleteRecords (Andrew Chow) 35f428fae68ad974abdce0fa905148f620a9443c Implement LegacyScriptPubKeyMan::MigrateToDescriptor (Andrew Chow) ea1ab390e4dac128e3a37d4884528c3f4128ed83 scriptpubkeyman: Implement GetScriptPubKeys in Legacy (Andrew Chow) e664af29760527e75cd7e290be5f102b6d29ebee Apply label to all scriptPubKeys of imported combo() (Andrew Chow) Pull request description: This PR adds a new `migratewallet` RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from `getnewaddress` or `getrawchangeaddress`. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active `ScriptPubKeyMan`s. For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active `ScriptPubKeyMan`s. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor. There are also tests. ACKs for top commit: Sjors: tACK 53e7ed075c49f853cc845afc7b2f058cabad0cb0 w0xlt: reACK https://github.com/bitcoin/bitcoin/commit/53e7ed075c49f853cc845afc7b2f058cabad0cb0 Tree-SHA512: c0c003694ca2e17064922d08e8464278d314e970efb7df874b4fe04ec5d124c7206409ca701c65c099d17779ab2136ae63f1da2a9dba39b45f6d62cf93b5c60a
2022-08-29Implement MigrateLegacyToDescriptorAndrew Chow
2022-08-29Implement MigrateToSQLiteAndrew Chow
2022-08-29wallet: Deduplicate Resend and ReacceptWalletTransactionsAndrew Chow
Both of these functions do almost the exact same thing. They can be deduplicated so that their behavior matches except for the filtering aspect. As this function will now always be called on wallet loading, nNextResend will also always be initialized, so wallet_resendwallettransactions.py is updated to account for that. This also resolves a bug where ResendWalletTransactions would fail to rebroadcast txs in insertion order thereby potentially rebroadcasting a child transaction before its parent and causing the child to not actually get rebroadcast. Also names the combined function to ResubmitWalletTransactions as the function just submits the transactions to the mempool rather than doing any sending by itself.
2022-08-26wallet: Refactor SetupDescSPKMs to take CExtKeyAndrew Chow
Refactors SetupDescSPKMs so that the DescSPKM loops are in their own function. This allows us to call it later during migration with a key that was already generated.
2022-08-25Apply label to all scriptPubKeys of imported combo()Andrew Chow
2022-08-25Wallet::SetMinVersion - Log the new minversionAli Sherief
2022-08-19Merge bitcoin/bitcoin#25784: Wallet: Document expectations for ↵Andrew Chow
AddWalletFlags (now InitWalletFlags) correctly 0cb6d2aec63aec76a517b8da621a3c53ab432632 Bugfix: Wallet: Document expectations for AddWalletFlags (now InitWalletFlags) correctly (Luke Dashjr) Pull request description: Includes some slight refactoring (return type changed, current status checked) ACKs for top commit: achow101: ACK 0cb6d2aec63aec76a517b8da621a3c53ab432632 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25784/commits/0cb6d2aec63aec76a517b8da621a3c53ab432632 ryanofsky: Code review ACK 0cb6d2aec63aec76a517b8da621a3c53ab432632. This is a clarifying change, and should prevent the InitWalletFlags method being called incorrectly. I left a comment suggestion, but feel free to ignore it. Tree-SHA512: fa18e9471b5e89d35cbc01526e6d4dbe4eee8faa9646847248909af1751b33014a6f9a42fe70a1331c0d73adea79008b8fc3ae2b51a641eba3e36d5c631327f6
2022-08-19Merge bitcoin/bitcoin#25869: wallet: remove UNKNOWN type from OUTPUT_TYPES arrayAndrew Chow
5b4fdbbff527aef8288edb3bf21b478de1061221 wallet: remove UNKNOWN type from OUTPUT_TYPES array (furszy) Pull request description: Fixing https://github.com/bitcoin/bitcoin/pull/25734#discussion_r949502998 -> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50329 The `OUTPUT_TYPES` array contain the known active output types only. And it's solely used to create/walk-through the active spkms. So, no need to add the `UNKNOWN` type here. ACKs for top commit: achow101: ACK 5b4fdbbff527aef8288edb3bf21b478de1061221 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25869/commits/5b4fdbbff527aef8288edb3bf21b478de1061221 LarryRuane: ACK 5b4fdbbff527aef8288edb3bf21b478de1061221 Tree-SHA512: dee2dc362a1b0c777555e5ee4d355a3351340591d0096f74e8c3a25f374cb2d9aef26145977ff4dd0f8cc940da9464eb5541eb2895bc19f8cbd6bb6d292ab9a9
2022-08-19Merge bitcoin/bitcoin#25707: refactor: Make const references to avoid ↵MacroFake
unnecessarily copying objects and enable two clang-tidy checks ae7ae36d311a869b3bda41d29dc0e47fade77d28 tidy: Enable two clang-tidy checks (Aurèle Oulès) 081b0e53e3adca7ea57d23e5fcd9db4b86415a72 refactor: Make const refs vars where applicable (Aurèle Oulès) Pull request description: I added const references to some variables to avoid unnecessarily copying objects. Also added two clang-tidy checks : [performance-for-range-copy](https://releases.llvm.org/11.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-for-range-copy.html) and [performance-unnecessary-copy-initialization](https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.html). ACKs for top commit: vasild: ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28 MarcoFalke: review ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28 Tree-SHA512: f6ac6b0cd0eee1e0c34d2f186484bc0f7ec6071451cccb33fa88a67d93d92b304e2fac378b88f087e94657745bca4e966dbc443759587400eb01b1f3061fde8c
2022-08-19Merge bitcoin/bitcoin#25679: wallet: Correctly identify external inputs that ↵fanquake
are also in the wallet ef8e2a5b09dea73de8d57e6b976d77edbde5f8ff tests: Test that external inputs of txs in wallet is handled correctly (Andrew Chow) eb879634db2116b23e884dab21318743f974f1f3 wallet: Try estimating input size with external data if wallet fails (Andrew Chow) a537d7aaa069bc216aeab381bbc4d312b5ffedf1 wallet: SelectExternal actually external inputs (Andrew Chow) f2d00bfe1a32a11c0d88e8c1d3bae6a6b01db15e wallet: Add CWallet::IsMine(COutPoint) (Andrew Chow) Pull request description: if a transaction is being funded that has an external input, and that input's parent is also in the wallet, we will fail to detect that and fail to fund the transaction. In order to correctly detect such inputs, we need to be doing `IsMine` on all specified inputs in order to use `Select` and `SelectExternal` correctly. Additionally `SelectCoins` needs to call `CalculateMaximumSignedInputSize` with the correct parameters which depends on whether the wallet is able to solve for the input. Because there are some situations where the wallet could find an external input to belong to it (e.g. watching an address - unable to solve, but will be ISMINE_WATCHONLY), instead of switching which `CalculateMaximumSignedInputSize` to use, we should call the one that uses the wallet, and if that fails, try again with the one that uses external solving data. Also adds a test for this case. ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/25679/commits/ef8e2a5b09dea73de8d57e6b976d77edbde5f8ff furszy: ACK ef8e2a5b ishaanam: reACK ef8e2a5b09dea73de8d57e6b976d77edbde5f8ff Tree-SHA512: a43c4aefeed4605f33a36ce87ebb916e2c153fea6d415b02c9a89275e84a7e3bf12840b33c296d2d2bde46350390da48d9262f9567338e3f21d5936aae4caa1e
2022-08-18wallet: remove UNKNOWN type from OUTPUT_TYPES arrayfurszy
This array contains the known active output types only. And it's solely used to create/walk-through the active spkms.
2022-08-17Merge bitcoin/bitcoin#24678: Prevent wallet unload on GetWalletForJSONRPCRequestfanquake
f59959e3818692c5b3c2dfa51c14e515085e940f wallet: Prevent wallet unload on GetWalletForJSONRPCRequest (João Barbosa) Pull request description: Don't extend shared ownership of all wallets to `GetWalletForJSONRPCRequest` scope. ACKs for top commit: achow101: ACK f59959e3818692c5b3c2dfa51c14e515085e940f shaavan: Code Review ACK f59959e3818692c5b3c2dfa51c14e515085e940f theStack: Concept and code-review ACK f59959e3818692c5b3c2dfa51c14e515085e940f Tree-SHA512: 7c0294098b5c32acaab8cc6fcf17a581d580ad1a557ba0602a9506074ac035815739afb4a25b3e61be9132535c7fc3ec7ef5137c1dfc9d4078f13663d508ef55
2022-08-16wallet: Add CWallet::IsMine(COutPoint)Andrew Chow
It is useful to have an IsMine function that can take an outpoint.
2022-08-16Merge bitcoin/bitcoin#25734: wallet, refactor: #24584 follow-upsAndrew Chow
8cd21bb2799d37ed00dc9d0490bb5f5f1375932b refactor: improve readability for AttemptSelection (josibake) f47ff717611182da27461e29b3c23933eb22fbce test: only run test for descriptor wallets (josibake) 0760ce0b9e646b6c86f4cc890c6ab78103a242ab test: add missing BOOST_ASSERT (josibake) db09aec9378c5e8cc49c866fa50bfcb6c567d66c wallet: switch to new shuffle, erase, push_back (josibake) b6b50b0f2b055d81c5d4ff9e21dd88cdc9a88ccb scripted-diff: Uppercase function names (josibake) 3f27a2adce12c6b0e7b43ba7c024331657bcf335 refactor: add new helper methods (josibake) f5649db9d5e984ba7f376ccfd5b0a627f5c42402 refactor: add UNKNOWN OutputType (josibake) Pull request description: This PR is to address follow-ups for #24584, specifically: * Remove redundant, hard-to-read code by adding a new `OutputType` and adding shuffle, erase, and push_back methods for `CoinsResult` * Add missing `BOOST_ASSERT` to unit test * Ensure functional test only runs if using descriptor wallets * Improve readability of `AttemptSelection` by removing triple-nested if statement Note for reviewers: commit `refactor: add new helper methods` should throw an "unused function warning"; the function is used in the next commit. Also, commit `wallet: switch to new shuffle, erase, push_back` will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility. ACKs for top commit: achow101: ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b aureleoules: ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b. LarryRuane: Concept, code review ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b furszy: utACK 8cd21bb2. Left a small, non-blocking, comment. Tree-SHA512: a1bbc5962833e3df4f01a4895d8bd748cc4c608c3f296fd94e8afd8797b8d2e94e7bd44d598bd76fa5c9f5536864f396fcd097348fa0bb190a49a86b0917d60e
2022-08-16Merge bitcoin/bitcoin#25504: RPC: allow to track coins by parent descriptorsAndrew Chow
a6b0c1fcc06485ecd320727fa7534a51a20608c1 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot) 0fd2d144540b720626fc065a3cef5188831b5ee2 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot) 55f98d087efd2609d808c082d5770306cc489409 rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot) b724476158a7dfeef9edfda3f519dfd6f93202a8 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot) 55a82eaf91d252a04a0cc8ad7d948d956c6cb24f wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot) Pull request description: Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends). It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit. I'll add this to the output of `listunspent` too if this gets a few Concept ACKs. ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/25504/commits/a6b0c1fcc06485ecd320727fa7534a51a20608c1 achow101: re-ACK a6b0c1fcc06485ecd320727fa7534a51a20608c1 Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
2022-08-12wallet: avoid double TopUp() calls on descriptor walletsfurszy
Move TopUp() responsibility from the wallet class to each scriptpubkeyman. So each spkm can decide to call it or not after perform the basic checks for the new destination request. Reason: We were calling it twice in the following flows for descriptor wallets: A) CWallet::GetNewDestination: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again. B) CWallet::GetReservedDestination: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again).
generated by cgit v1.2.3 (git 2.26.2) at 2024-10-12 06:18:09 +0000