| Age | Commit message (Collapse) | Author |
|---|
|
CWallet::CreateWalletFromFile() was removed in
8b5e7297c02f3100a9cb27bfe206e3fc617ec173 but these references remain.
|
|
|
|
[META] This is a pure refactor commit.
Move PruneBlockFile to BlockManager because:
1. PruneOneBlockFile only acts on BlockManager
2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a
reference to the larger ChainstateManager, just a reference to
BlockManager is enough. See following commits.
|
|
No changes in behavior
|
|
This removes a source of complexity and indirection that makes it harder to
understand path checking code. Path checks will be simplified in upcoming
commits.
There is no change in behavior in this commit other than a slightly more
descriptive error message in `loadwallet` if the default "" wallet can't be
found. (The error message is improved more in upcoming commit "wallet: Remove
path checking code from loadwallet RPC".)
|
|
Enable the GUI to also use the load_on_startup feature.
Wallets loaded in the GUI always have load_on_startup=true.
When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet
module and called from within LoadWallet, RemoveWallet, and
Createwallet. This change does not actually touch the GUI code but
rather the wallet functions that are shared between the GUI and RPC.
|
|
|
|
|
|
1554b54d47d7e24ce2491f57d24e56d38ceb7649 Static asserts for consistency of fee defaults. (Daniel Kraft)
Pull request description:
This adds `static_assert`'s that ensure that the default values given for fee levels in the wallet (minimum fee and incremental feerate increase) are at least as high as the corresponding levels configured in the core node policy. Since the core policy values are enforced by the network, it makes sense for the wallet to be conservative and above (or at least not below) this.
ACKs for top commit:
laanwj:
code review ACK 1554b54d47d7e24ce2491f57d24e56d38ceb7649, these assumptions seem straightforward
Tree-SHA512: 50e5adf082f467062334377f82a3ee75bcfd436afc65bd0eb33c8d0549d6d90fd1f48c31f60cabe523eb59be9efa8ae0879e9e09cd51ca9c1bd466631ce03cf4
|
|
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
|
|
9c59f9c285303659ee1beed7555bbb322e6e6981 Fix ZapSelectTx to sync wallet spends (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
ACKs for top commit:
achow101:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981
ryanofsky:
Code review ACK 9c59f9c285303659ee1beed7555bbb322e6e6981. Only change since last review tweaking the for loop as suggested
jonatack:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981 tested rebased on current master b33136b6ba9887f7d and the new unit test does indeed fail without the change.
meshcollider:
utACK 9c59f9c285303659ee1beed7555bbb322e6e6981
Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
|
|
fab80fef61ddd4afeff6e497c7e76bffcd05e8a4 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c811d99200453b0936219c473f514b0 scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701adba1cb48535cac25fd43c742a82e40d util: Add Assert identity function (MarcoFalke)
fa457fbd3387661e1973a8f4e5cc2def79e0c625 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61ddd4afeff6e497c7e76bffcd05e8a4.
ryanofsky:
Code review ACK fab80fef61ddd4afeff6e497c7e76bffcd05e8a4
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
|
|
|
|
This adds static asserts that ensure that the default values given for
fee levels in the wallet (minimum fee and incremental feerate increase)
are at least as high as the corresponding levels configured in the
core node policy.
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e 's/WalletDatabase::Create(/CreateWalletDatabase(/g' `git grep -l "WalletDatabase::Create("`
sed -i -e 's/WalletDatabase::CreateDummy(/CreateDummyWalletDatabase(/g' `git grep -l "WalletDatabase::CreateDummy("`
sed -i -e 's/WalletDatabase::CreateMock(/CreateMockWalletDatabase(/g' `git grep -l "WalletDatabase::CreateMock("`
-END VERIFY SCRIPT-
|
|
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i -e 's/EnsureChainman\((m?_?node)\)\./Assert(\1.chainman)->/g' $(git grep -l EnsureChainman)
-END VERIFY SCRIPT-
|
|
|
|
|
|
This commit does not change behavior
|
|
|
|
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
|
|
28b112e9bd3fd1181c0720306051ba7efca8b436 Get rid of BindWallet (Russell Yanofsky)
d002f9d15d938e78360ad906f2d74a249c7e923e Disable CWalletTx copy constructor (Russell Yanofsky)
65b9d8f8ddb5a838454efc8bdd6576f0deb65f6d Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky)
bd2fbc7cdbec46400341209f4cb7e69e5b2cee19 Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky)
2b9cba206594bfbcefcef0c88a0bf793819643bd Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky)
Pull request description:
This is a pure refactoring, no behavior is changing.
Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged.
Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.
This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.
ACKs for top commit:
MarcoFalke:
Anyway, re-ACK 28b112e9bd3fd1181c0720306051ba7efca8b436
Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef
|
|
Also, mark feebumper bilingual_str as Untranslated
They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.
|
|
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
|
|
Instead of AddToWallet taking a temporary CWalletTx object and then potentially
merging it with a pre-existing CWalletTx, have it take a callback so callers
can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with
saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to
CWalletTx entries without having to make temporary objects and then worry about
how they will be merged.
This is a pure refactoring, no behavior is changing.
|
|
This change is intended to make the bitcoin node and its rpc, network
and gui interfaces more responsive while the wallet is in use. Currently
because the node's cs_main mutex is always locked before the wallet's
cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked
while the wallet does relatively slow things like creating and listing
transactions.
This commit only remmove chain lock tacking in wallet code, and invert
lock order from cs_main, cs_wallet to cs_wallet, cs_main.
must happen at once to avoid any deadlock. Previous commit were only
removing Chain::Lock methods to Chain interface and enforcing they
take cs_main.
Remove LockChain method from CWallet and Chain::Lock interface.
|
|
Remove findPruned and findFork, no more used after 17954.
|
|
Instead of calling getHeight, we rely on CWallet::m_last_block
processed_height where it's possible.
|
|
Add unit test calling CreateWalletFromFile, which isn't currently called from
other unit tests, with some basic checks to make sure it rescans and registers
for notifications correctly.
Motivation for this change was to try to write a test that would fail without
the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8
from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:
https://github.com/bitcoin/bitcoin/blob/ef8c6ca60767cac589d98ca57ee33179608ccda8/src/wallet/wallet.cpp#L3978-L3986
However, writing a full test for the race condition that call prevents isn't
possible without the locking changes from #16426. So this PR just adds as much
test coverage as is possible now.
This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719,
since it detects the stale notifications.transactionAddedToMempool notifications
that PR eliminates.
|
|
WalletDescriptor is a Descriptor with other wallet metadata
|
|
fa60afc4fb957875bab1c8982d9d9e4999a3814c wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet (MarcoFalke)
Pull request description:
dumpwallet includes the block hash in the output, so this method depends on the chainstate. According to the developer notes https://github.com/bitcoin/bitcoin/blame/e84a5f000493fe39adb2a5f22b43c3848dcd0a4f/doc/developer-notes.md#L1095 it must include a `BlockUntilSyncedToCurrentChain`.
This is a minor fix and does not need backport, I think.
It fixes test failures such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/675487097#L2657 , which can only happen in master because the test was not backported.
ACKs for top commit:
promag:
Code review ACK fa60afc4fb957875bab1c8982d9d9e4999a3814c.
ryanofsky:
Code review ACK fa60afc4fb957875bab1c8982d9d9e4999a3814c
meshcollider:
utACK fa60afc4fb957875bab1c8982d9d9e4999a3814c
Tree-SHA512: 8df70b06b226b2cdf880dec9264adb72d66fd81b09b404fd1665a79e5f5236d26122eebf15df00fe71ee292b5c91b2dc23a0a42b2aa50a8d690604b23832723f
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
48973402d8bccb673eaeb68b7aa86faa39d3cb8a wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes (Russell Yanofsky)
e958ff9ab5607da2cd321f29fc785a6d359e44f4 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction (Russell Yanofsky)
c0d07dc4cba7634cde4e8bf586557772f3248a42 wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions (Russell Yanofsky)
1be8ff280c78c30baabae9429c53c0bebb89c44d wallet: Avoid use of Chain::Lock in rescanblockchain (Russell Yanofsky)
3cb85ac594f115db99f96b0a0f4bfdcd69ef0590 wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime (Russell Yanofsky)
f7ba881bc669451a60fedac58a449794702a3e23 wallet: Avoid use of Chain::Lock in listsinceblock (Russell Yanofsky)
bc96a9bfc61afdb696fb92cb644ed5fc3d1793f1 wallet: Avoid use of Chain::Lock in importmulti (Russell Yanofsky)
25a9fcf9e53bfa94e8f8b19a4abfda0f444f6b2a wallet: Avoid use of Chain::Lock in importwallet and dumpwallet (Russell Yanofsky)
c1694ce6bb7e19a8722d5583cd85ad17da40bb67 wallet: Avoid use of Chain::Lock in importprunedfunds (Russell Yanofsky)
ade5f87971211bc67753f14a0d49e020142efc7c wallet refactor: Avoid use of Chain::Lock in qt wallettests (Russell Yanofsky)
f6da44ccce4cfff53433e665305a6fe0a01364e4 wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances (Russell Yanofsky)
bf30cd4922ea62577d7bf63f5029e8be62665d45 refactor: Add interfaces::FoundBlock class to selectively return block data (Russell Yanofsky)
Pull request description:
This is a set of changes updating wallet code to make fewer calls to `Chain::Lock` methods, so the `Chain::Lock` class will be easier to remove in #16426 with fewer code changes and small changes to behavior.
ACKs for top commit:
MarcoFalke:
re-ACK 48973402d8, only change is fixing bug 📀
fjahr:
re-ACK 48973402d8bccb673eaeb68b7aa86faa39d3cb8a, reviewed rebase and changes since last review, built and ran tests locally
ariard:
Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in `findCommonAncestor`
Tree-SHA512: cfd2f559f976b6faaa032794c40c9659191d5597b013abcb6c7968d36b2abb2b14d4e596f8ed8b9a077e96522365261299a241a939b3111eaf729ba0c3ef519b
|
|
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change affects behavior in a few small ways.
- If there's no max_height specified, percentage progress is measured ending at
wallet last processed block instead of node tip
- More consistent error reporting: Early check to see if start_block is on the
active chain is removed, so start_block is always read and the triggers an
error if it's unavailable
|
|
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case it may use a more accurate rescan
time.
|
|
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case it will use more accurate backup and
rescan timestamps.
|
|
I'd previously attempted to create a specialized lock for ChainstateManager,
but it turns out that because that lock would be required for functions like
ChainActive() and ChainstateActive(), it created irreconcilable lock inversions
since those functions are used so broadly throughout the codebase.
Instead, I'm just using cs_main to protect the contents of g_chainman.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
|
|
In CWallet::LoadWallet, use this to detect and empty wallet with no keys
This commit does not change behavior.
|
|
This commit only affects locking behavior and doesn't have other changes.
|
|
0b75a7f0680d16a41043864a897470324917b1e8 wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd00eb032a19d142026e4d019944192da19 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 0b75a7f0680d16a41043864a897470324917b1e8
MarcoFalke:
ACK 0b75a7f0680d16a41043864a897470324917b1e8
ryanofsky:
Code review ACK 0b75a7f0680d16a41043864a897470324917b1e8. 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
|
|
36b68de5b2938722911db900ca299f7008780d01 Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429c56c85fa16c309be0b2bca9c25fdd3e1a Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff03871add000f8b4d8f82aeb168eed2fc9dc5f Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de16feee097a88e99d2ecdd4d84beb4f915 Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05e48fb53d4b62c59060424a0fea71d0aab Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d3848e09abf571e5308185275296127efca4 Add block_height field in struct Confirmation (Antoine Riard)
9700fcb47feca9d78e005b8d18b41148c8f6b25f Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3eff15b9b5bdc951f1e274f00d581f63bce Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729e33f76092bd8cfa06d1a5e0a066436f76 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 (36b68de5b2938722911db900ca299f7008780d01).
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](https://github.com/bitcoin/bitcoin/commit/36b68de5b2938722911db900ca299f7008780d01)).
meshcollider:
utACK 36b68de5b2938722911db900ca299f7008780d01
ryanofsky:
Code review ACK 36b68de5b2938722911db900ca299f7008780d01. 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 36b68de5b2938722911db900ca299f7008780d01
promag:
Code review ACK 36b68de5b2938722911db900ca299f7008780d01.
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
|
|
-BEGIN VERIFY SCRIPT-
# Move files
for f in $(git ls-files src/test/lib/); do git mv $f src/test/util/; done
git mv src/test/setup_common.cpp src/test/util/
git mv src/test/setup_common.h src/test/util/
# Replace Windows paths
sed -i -e 's|\\setup_common|\\util\\setup_common|g' $(git grep -l '\\setup_common')
sed -i -e 's|src\\test\\lib\\|src\\test\\util\\|g' build_msvc/test_bitcoin/test_bitcoin.vcxproj
# Everything else
sed -i -e 's|/setup_common|/util/setup_common|g' $(git grep -l 'setup_common')
sed -i -e 's|test/lib/|test/util/|g' $(git grep -l 'test/lib/')
# Fix include guard
sed -i -e 's|BITCOIN_TEST_SETUP_COMMON_H|BITCOIN_TEST_UTIL_SETUP_COMMON_H|g' ./src/test/util/setup_common.h
sed -i -e 's|BITCOIN_TEST_LIB_|BITCOIN_TEST_UTIL_|g' $(git grep -l 'BITCOIN_TEST_LIB_')
-END VERIFY SCRIPT-
|
|
|
|
At BlockConnected/BlockDisconnected, we rely on height of block
itself to know current height of wallet
|
|
|
|
362ded410b8cb1104b7ef31ff8488fec4824a7d5 Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b751a3e6b3b9f6fb7961c442877fb65a scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5e42feaf7bfa5f41e80292aaa73cd7d Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b71c9d91399c31b043237091be2e5e7 MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e6765b185bd55f4c541f9e27aeea29d 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 362ded410b8cb1104b7ef31ff8488fec4824a7d5
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
|
