69cf5d4eeb [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
OutputGroup will handle the fee and effective value computations
inside of Insert. It now needs to take the effective feerate and long
term feerates as arguments to its constructor.
Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.
Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.
This commit adds a unified stream which can be used to closely track mempool:
1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)
The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.
These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.
Although loadwallet() in rpcwallet.cpp assumes LoadWallet() always
assign some value to the 'status', but LoadWallet() does not do so
in some situation.
This fixes above and prevends loadwallet() returns ambiguous error code.
f1ee37319a wallet: Reload previously loaded wallets on GUI startup (Andrew Chow)
Pull request description:
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.
ACKs for top commit:
jonasschnelli:
Tested ACK f1ee37319a - works as expected. Wallets loaded via bitcoin-cli (in `-server` mode) or through the RPC console won't be loaded on startup but wallets loaded via the GUI menu will.
kristapsk:
ACK f1ee37319a, I have tested the code.
Tree-SHA512: f5b44aa763cf761d919015c5fbc0600b72434aa71e3b57007fd7530a29c3da1a9a0c98c4f22cb6cdffba61150a31170056a7d4737625e7b76f6958f3d584da8c
Checks are now consolidated in MakeBerkeleyDatabase function instead of
happening in higher level code.
This commit does not change behavior except for error messages which now
include more complete information.
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.
3340dbadd3 Remove -zapwallettxes (Andrew Chow)
Pull request description:
It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.
Alternative to #19700
ACKs for top commit:
meshcollider:
utACK 3340dbadd3
fanquake:
ACK 3340dbadd3 - remaining manpage references will get cleaned up pre-release.
Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.
6d1f51343c [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost)
Pull request description:
When using `fundrawtransaction` and `walletcreatefundedpsbt` with `lockUnspents`, it would only lock automatically selected coins, not manually selected coins. That doesn't make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction.
Note that when creating a transaction, manually selected coins are automatic "unlocked" (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with `lockUnspents`) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling `lockunspent` and the subsequent spending RPC.
See #7518 for historical background.
ACKs for top commit:
meshcollider:
Code review ACK 6d1f51343c
fjahr:
Code review ACK 6d1f51343c
Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d
772ea4844c wallet: Avoid recursive lock in IsTrusted (João Barbosa)
819f10f671 wallet, refactor: Immutable CWalletTx::pwallet (João Barbosa)
Pull request description:
This change moves `CWalletTx::IsTrusted` to `CWallet` in order to have TSAN. So now `CWallet::IsTrusted` requires `cs_wallet` and the recursive lock no longer happens.
Motivated by https://github.com/bitcoin/bitcoin/pull/19289/files#r473308226.
ACKs for top commit:
meshcollider:
utACK 772ea4844c
hebasto:
ACK 772ea4844c, reviewed and tested on Linux Mint 20 (x86_64).
Tree-SHA512: 702ffd928b2f42a8b90de398790649a5fd04e1ac3877558da928e94cdeb19134883f06c3a73a6826c11c912facf199173375a70200737e164ccaea1bec515b2a
24bf17602c gui refactor: Inline SplashScreen::ConnectWallet (Russell Yanofsky)
e4f4350471 refactor: Move wallet methods out of chain.h and node.h (Russell Yanofsky)
b266b3e0bf refactor: Create interfaces earlier during initialization (Russell Yanofsky)
Pull request description:
Add WalletClient interface so node interface is cleaner and don't need wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate global wallet variables like ::vpwallets in #19101, because createWallet(), loadWallet(), getWallets(), etc methods called by the GUI need a way to get a reference to the list of open wallets if it is no longer a global variable.
ACKs for top commit:
promag:
Code review ACK 24bf17602c.
MarcoFalke:
ACK 24bf17602c🐚
Tree-SHA512: a70d3776cd6723093db8912028c50075ec5fa0a48b961cb1a945f922658f5363754f8380dbb8378ed128c8c858913024f8264740905b8121a35c0d63bfaed7cf
Add WalletClient interface so node interface is cleaner and don't need
wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate
global wallet variables like ::vpwallets, because createWallet(), loadWallet(),
getWallets(), etc methods called by the GUI need a way to get a reference to
the list of open wallets if it is no longer a global variable.
Also tweaks splash screen registration for load wallet events to be delayed
until after wallet client is created.
7f13dfb587 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf69 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587
achow101:
ACK 7f13dfb587
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
9adc2f80fc Refactor OutputGroups to handle effective values, fees, and filtering (Andrew Chow)
7d07e864b8 Use real value when calculating OutputGroup value (Andrew Chow)
Pull request description:
Currently, the effective values and filtering for positive effective values is done outside of the OutputGroup. We should instead have functions in Outputgroup to do this and call those for each OutputGroup. So this PR does that.
This makes future changes for effective values in coin selection much easier.
ACKs for top commit:
instagibbs:
reACK 9adc2f80fc
fjahr:
re-ACK 9adc2f80fc
meshcollider:
Light code review ACK 9adc2f80fc
Tree-SHA512: 7445c94b7295b45bcd83a6f8a5c8f6961a89453fcc856335192d4b4a66aec7724513616b04e5111588ab208c89b311055399d6279cd9c4ce452aefb85f04b64a
e7448d6680 wallet: Don't override signing errors (Fabian Jahr)
Pull request description:
While reviewing #17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one.
Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](c7b4968552/src/script/sign.cpp (L481)), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed.
On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach.
ACKs for top commit:
achow101:
ACK e7448d6680
meshcollider:
Code review ACK e7448d6680
Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23
Instead of having callers set the fees, effective values, and filtering
of outputs, do these within OutputGroups themselves as member functions.
m_fee and m_long_term_fee is added to OutputGroup to track the fees of
the OutputGroup.
3a9aba21a4 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow)
d9cd095b59 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow)
0122fbab4c Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow)
Pull request description:
`SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet.
`AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet.
`LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk.
ACKs for top commit:
jnewbery:
Code review ACK 3a9aba21a4
ryanofsky:
Code review ACK 3a9aba21a4. Only changes since last review tweaks making m_wallet_flags updates more safe
meshcollider:
utACK 3a9aba21a4
Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
9c59f9c285 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 9c59f9c285
ryanofsky:
Code review ACK 9c59f9c285. Only change since last review tweaking the for loop as suggested
jonatack:
ACK 9c59f9c285 tested rebased on current master b33136b6ba and the new unit test does indeed fail without the change.
meshcollider:
utACK 9c59f9c285
Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
d8e9ca66d1 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)
Pull request description:
The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.
`BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.
`BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.
Part of #18971
ACKs for top commit:
MarcoFalke:
re-ACK d8e9ca66d1 only change is test fixup 🤞
promag:
Code review ACK d8e9ca66d1, good stuff.
Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a
84d295e513 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
4600479058 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da rpc: show both UTXOs in decodepsbt (Andrew Chow)
Pull request description:
Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition.
Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.
Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.
As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs.
ACKs for top commit:
Sjors:
utACK 84d295e513 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)
ryanofsky:
Code review re-ACK 84d295e513. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to
meshcollider:
utACK 84d295e513
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
fa927ff884 Enable Wswitch for OutputType (MarcoFalke)
faddad71f6 Remove confusing OutputType::CHANGE_AUTO (MarcoFalke)
fa2eb38352 interfaces: Remove unused getDefaultChangeType (MarcoFalke)
Pull request description:
`OutputType::CHANGE_AUTO` is problematic for several reasons:
* An output that is not change must never be described by `CHANGE_AUTO`. Simply allowing that option makes the code confusing and review harder than it needs to be.
* To make review even harder, `CHANGE_AUTO` requires `-Wswitch` to be disabled for `OutputType`
Fix both issues by removing `CHANGE_AUTO` and then enabling `-Wswitch` for `OutputType`
ACKs for top commit:
promag:
Code review ACK fa927ff884.
laanwj:
Code review ACK fa927ff884
Tree-SHA512: 24fd809757aa343866c94dafe9a7130b50cda4f77c97666d407f99b813f75b115a7d8e688a6bc2a737e87cba64ddd4e43f2b3c5538fd35fabb5845807bb39134
9b009fae6e qa: Test concurrent wallet loading (João Barbosa)
b9971ae585 wallet: Handle concurrent wallet loading (João Barbosa)
Pull request description:
This PR handles concurrent wallet loading.
This can be tested by running in parallel the following script a couple of times:
```sh
for i in {1..10}
do
src/bitcoin-cli -regtest loadwallet foo
src/bitcoin-cli -regtest unloadwallet foo
done
```
Eventually the error occurs:
```
error code: -4
error message:
Wallet already being loading.
```
For reference, loading and already loaded wallet gives:
```
error code: -4
error message:
Wallet file verification failed. Error loading wallet w1. Duplicate -wallet filename specified.
```
Fixes#19232.
ACKs for top commit:
MarcoFalke:
Concept ACK 9b009fae6e I have not reviewed the code
hebasto:
ACK 9b009fae6e, tested on Linux Mint 20 (x86_64):
ryanofsky:
Code review good-but-not-ideal ACK 9b009fae6e
Tree-SHA512: 0ccd77b03c0926e4c4e51efb31e193b93cb4b9ffe8bac6bb018f7344c55dfd939b873b8cf5e657dca73e6202eb75aa672de2acb787cc133184b0b3b51e47b972
931dd47608 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29 [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b733 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd47608
jb55:
ACK 931dd47608
achow101:
ACK 931dd47608
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
e5327f947c [rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost)
79804fe24b [rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost)
Pull request description:
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`.
Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`.
This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection.
ACKs for top commit:
achow101:
ACK e5327f947c
meshcollider:
utACK e5327f947c
Tree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb
In FillPSBT, optionally report the number of inputs we successfully
signed, as an out parameter. If "sign" is false, instead report the
number of inputs for which GetSigningProvider does not return nullptr.
(This is a potentially overbroad estimate of inputs we could sign.)
44cc75f80e wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm)
Pull request description:
This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.
ACKs for top commit:
Sjors:
utACK 44cc75f80e (rebased)
fjahr:
re-ACK 44cc75f80e
Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
7eaf86d3bf trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd and 7e89994133 from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bf
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bf🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
84ae0578b6 Add release notes about salvage changes (Andrew Chow)
ea337f2d03 Move RecoverKeysOnlyFilter into RecoverDataBaseFile (Andrew Chow)
9ea2d258b4 Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h} (Andrew Chow)
b426c7764d Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter standalone (Andrew Chow)
2741774214 Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter (Andrew Chow)
ced95d0e43 Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover (Andrew Chow)
07250b8dce walletdb: remove fAggressive from Salvage (Andrew Chow)
8ebcbc85c6 walletdb: don't automatically salvage when corruption is detected (Andrew Chow)
d321046f4b wallet: remove -salvagewallet (Andrew Chow)
cdd955e580 Add basic test for bitcoin-wallet salvage (Andrew Chow)
c87770915b wallettool: Add a salvage command (Andrew Chow)
Pull request description:
Removes the `-salvagewallet` startup option and adds a `salvage` command to the `bitcoin-wallet` tool. As such, `-salvagewallet` is removed. Additionally, the automatic salvage that is done if the wallet file fails to load is removed.
Lastly the salvage code entirely is moved out entirely into `bitcoin-wallet` from `walletdb.{cpp/h}` and `db.{cpp/h}`.
ACKs for top commit:
jonatack:
ACK 84ae0578b6 feedback taken, and compared to my previous review, the bitcoin-wallet salvage command now seems to run and it exits without raising. The new test passes at both 9454105 and 84ae057 so as a sanity check I'd agree there is room for improvement, if possible.
MarcoFalke:
re-ACK 84ae0578b6🏉
Empact:
Code Review ACK 84ae0578b6
ryanofsky:
Code review ACK 84ae0578b6. Lot of small changes since previous review: added verify step before salvage, added basic test in new commit, removed unused scanstate variable and warnings parameter, tweaked various comments and strings, moved fsuccess variable declaration
meshcollider:
Concept / light code review ACK 84ae0578b6
Tree-SHA512: 05be116b56ecade1c58faca1728c8fe4b78f0a082dbc2544a3f7507dd155f1f4f39070bd1fe90053444384337bc48b97149df5c1010230d78f8ecc08e69d93af
ca2a09640f Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1140 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8f13 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d95c docs: Add release notes for descriptor wallets (Andrew Chow)
Pull request description:
Some docs and cleanup following #16528.
* Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
* Adds a warning to `createwallet` that descriptor wallets are experimental.
* Removed unused `SetCrypted` as suggestioned: https://github.com/bitcoin/bitcoin/pull/16528#discussion_r415300916
* Removed `m_address_type` as mentioned in https://github.com/bitcoin/bitcoin/pull/18782#issuecomment-620167077
ACKs for top commit:
Sjors:
tACK ca2a09640f
instagibbs:
utACK ca2a09640f
meshcollider:
utACK ca2a09640f
Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two `CWallet::BlockConnected` updates:
a31be09bfd and
7e89994133 from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard <ariard@student.42.fr>
fa47cf9d95 wallet: Fix typo in assert that is compile-time true (MarcoFalke)
Pull request description:
Commit 92bcd70808 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`.
However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
ACKs for top commit:
Sjors:
utACK fa47cf9d95
Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
28b112e9bd Get rid of BindWallet (Russell Yanofsky)
d002f9d15d Disable CWalletTx copy constructor (Russell Yanofsky)
65b9d8f8dd Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky)
bd2fbc7cdb Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky)
2b9cba2065 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 28b112e9bd
Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef
m_address_type was used for two things:
1. Determine the type of descriptor to generate during
SetupDescriptorGeneration
2. Sanity check during GetNewDestination.
There is no need to have this variable to accomplish those things.
1. Add a argument to SetupDescriptorGeneration indicating the address
type to use
2. Use Descriptor::GetOutputType for the sanity check.
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.
Disable copying of CWalletTx objects to prevent bugs where instances get copied
in and out of the mapWallet map and fields are updated in the wrong copy.
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.
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
Since the mempool unbroadcast mechanism handles the reattempts for initial
broadcast, the wallet rebroadcast attempts can be much less frequent
(previously ~1/30 min)
When a CWallet doesn't have a ScriptPubKeyMan for the requested type
in GetNewDestination, give a meaningful error. Also handle this in
Qt which did not do anything with errors.
FillPSBT will add our own scripts to the PSBT if those inputs are ours.
If an input also lists pubkeys that we happen to know the private keys
for, we will sign those inputs too.
