dc3ec74d67abc85e8f724648f93efdd097e6f783 Add rescan removal release note (Samuel Dobson)
bccd1d942d971e70e7a0f4f5628e1b74b3ac15e0 Remove -rescan startup parameter (Samuel Dobson)
f963b0fa8cdd5223feb828c5faf6c57bc4107c8a Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson)
6c006495ef07f163d0734ec35d3cd1589a4aae9d Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson)
Pull request description:
Remove the `-rescan` startup parameter.
Rescans can be run with the `rescanblockchain` RPC.
Rescans are still done on wallet-load if needed due to corruption, for example.
ACKs for top commit:
achow101:
ACK dc3ec74d67abc85e8f724648f93efdd097e6f783
laanwj:
re-ACK dc3ec74d67abc85e8f724648f93efdd097e6f783
Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c
90be29c5b52e68b5de8a3282cd83172fbf9acf1b wallet: enable SQLite extended result codes (Sebastian Falbesoner)
Pull request description:
With this change, we get more fine-grained error messages if something goes wrong in the course of communicating with the SQLite database. To pick some random examples, the error codes SQLITE_IOERR_NOMEM, SQLITE_IOERR_CORRUPTFS or SQLITE_IOERR_FSYNC are way more specific than just a plain SQLITE_IOERR, and the corresponding error messages generated by sqlite3_errstr() will hence give a better hint to the user (or also to the developers, if an error report is sent) what the cause for a failure is.
See the SQLite documentation
https://www.sqlite.org/c3ref/extended_result_codes.htmlhttps://www.sqlite.org/c3ref/c_abort_rollback.html
> In its default configuration, SQLite API routines return one of 30 integer result codes. However, experience has shown that many of these result codes are too coarse-grained. They do not provide as much information about problems as programmers might like. In an effort to address this, newer versions of SQLite (version 3.3.8 2006-10-09 and later) include support for additional result codes that provide more detailed information about errors.
ACKs for top commit:
Sjors:
utACK 90be29c
achow101:
ACK 90be29c5b52e68b5de8a3282cd83172fbf9acf1b
laanwj:
Code review ACK 90be29c5b52e68b5de8a3282cd83172fbf9acf1b
Tree-SHA512: 2b7a60860c206f2b5f8ff9d4a7698efdee897c9ad024621b8fd165b841c20746d9780da3cf46aaf448a777e229a5b3cdf3a4792e8ef82cda9c5d46e354a9a598
240ea294d5e899a906f213f039b21e94c24d6018 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21810bf1c3bdce0ef2212c1ad6597bcd test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e71b9df10dfac7f32f92997938f7e7aa wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5e899a906f213f039b21e94c24d6018 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5e899a906f213f039b21e94c24d6018
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
43cd6b8af9d613ca033800c5cd8524c3f77e13ec doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz)
2b1fdc2c6ce1d0b0e51a3f107e23443c142d57af refactor: minor styling, prefer snake case and same line if (Michael Dietz)
d64deac7b823a0eba97ab3a3686054eefe330d3c refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz)
8721638daa8502c7f8de5ae24a9393d7290a2ce5 rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz)
Pull request description:
Resolves#21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.
`-deprecatedrpc=addresses` was initially added in this PR #20286 (which resolved the original issue #20102).
Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)
ACKs for top commit:
MarcoFalke:
re-ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec 🐉
meshcollider:
Code review ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec
jonatack:
ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit
Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
7e3ee4cdd0f60a2f549ba030fe96b90d61c036c5 GUI: Ask user to unlock wallet before signing psbt (Samuel Dobson)
0f3acecf3372f9da143590bb17d8444564e083f4 Add test that walletprocesspsbt requires unlocked wallet when signing (Samuel Dobson)
0e895212bb571ae0de5580adfd8ee9b3c2137e24 Ensure wallet is unlocked before signing in walletprocesspsbt (Samuel Dobson)
Pull request description:
If signing a PSBT, we need to ensure the wallet is unlocked.
Fixes#22874, fixesbitcoin-core/gui#312
ACKs for top commit:
achow101:
ACK 7e3ee4cdd0f60a2f549ba030fe96b90d61c036c5
lsilva01:
Code Review ACK 7e3ee4cdd0
benthecarman:
ACK 7e3ee4cdd0f60a2f549ba030fe96b90d61c036c5
Tree-SHA512: 6726a873582747900ab454ea21153a92be86808a4c1517dc2856b389876a2da9e8df1ffa9b567b6bd017038342c3544ecf5ca3c97744e7debe0a5ee65563687d
efcaefc7b5ffe0495e7c809032342ee5ca4841be test: Add remaining scenarios of 0 waste (rajarshimaitra)
Pull request description:
As per the [review club](https://bitcoincore.reviews/22009) discussion on #22009 , it was observed that there were other two fee scenarios in which selection waste could be zero.
These are:
- (LTF - Fee) == Change Cost
- (LTF - Fee) == Excess
Even though these are obvious by the definition of waste metric, adding tests for them can be helpful in explaining its behavior
to new readers of the code base, along with pinning the behavior for future.
This PR adds those two cases to waste calculation unit test.
Also let me know if I am missing more scenarios.
ACKs for top commit:
jonatack:
Tested re-ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be
achow101:
ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be
meshcollider:
ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be
Tree-SHA512: 13fe3e2c0ea7bb58d34e16c32908b84705130dec16382ff941e5e60ca5b379f9c5811b33f36c4c72d7a98cfbb6af2f196d0a69e96989afa4b9e49893eaadd7cb
With this change, we get more fine-grained error messages if something
goes wrong in the course of communicating with the SQLite database. To
pick some random examples, the error codes SQLITE_IOERR_NOMEM,
SQLITE_IOERR_CORRUPTFS or SQLITE_IOERR_FSYNC are way more specific than just a
plain SQLITE_IOERR, and the corresponding error messages generated by
sqlite3_errstr() will hence give a better hint to the user (or also to the
developers, if an error report is sent) what the cause for a failure is.
93b9800fec588fc0c258e754405db1e9f89cfb81 scripted-diff: Rename overloaded int GetArg to GetIntArg (Russell Yanofsky)
Pull request description:
This is meant to improve readability of code and remove guesswork needed to determine argument types and migrate to [typed arguments (#22978)](https://github.com/bitcoin/bitcoin/issues/22978) by having distinctly named `GetArg` `GetArgs` `GetBoolArg` and `GetIntArg` methods.
---
This commit was originally part of #22766 and had some review discussion there. But it was [wisely suggested](https://github.com/bitcoin/bitcoin/pull/22766#issuecomment-910001542) to be split off to make that PR smaller.
ACKs for top commit:
hebasto:
ACK 93b9800fec588fc0c258e754405db1e9f89cfb81.
MarcoFalke:
re-ACK 93b9800fec588fc0c258e754405db1e9f89cfb81 📨
Tree-SHA512: e034bd938b2c8fbadd90bcd52213a61161965dfddf18c2cb0d68816ecf2438cde8afee6fb7e3418f5c6b35c208338da9deb99e4e418dbf68ca34552e0916a625
d2eccacd188959c5aed661d0d5b39c119e7c7a32 doc: Clarify that change_cost cannot be negative in GetSelectionWaste (benthecarman)
Pull request description:
We assert that the `change_cost` must be positive so we should document it in the function's docs
4f5ad43b1e/src/wallet/coinselection.cpp (L361)
ACKs for top commit:
jonatack:
ACK d2eccacd188959c5aed661d0d5b39c119e7c7a32
jarolrod:
ACK d2eccacd188959c5aed661d0d5b39c119e7c7a32
shaavan:
ACK d2eccacd188959c5aed661d0d5b39c119e7c7a32
Tree-SHA512: 089bc41d7e212b811455527ce2ac83301eae0edc03933dcbd5229b22078827395c8e3bcfda0ba029b7c60705db3119b4f80e372842a6bd8dae195dbaa0b52833
Before this patch, the log might be corrupted by other threads logging
at the same time. For example, another RPC thread:
[httpworker.1] [default wallet] keypool reserve 1296
[httpworker.1] SelectCoins() best subset: Received a POST request for / from 127.0.0.1:53732
[httpworker.3] ThreadRPCServer method=getnetworkinfo user=__cookie__
[httpworker.1] 0.78125 0.1953125 0.02441406 0.00610351 0.00305175 0.00152587 total 1.01025417
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
d96b000e94d72d041689c5c47e374df2ebc0e011 Make GUI UTXO lock/unlock persistent (Samuel Dobson)
077154fe698f5556ad6e26ef49c9024c2f07ff68 Add release note for lockunspent change (Samuel Dobson)
719ae927dcdb60c0f9902fa79796256035228c4e Update lockunspent tests for lock persistence (Samuel Dobson)
f13fc16295c19a156f2974d2d73fba56d52fc161 Allow lockunspent to store the lock in the wallet DB (Samuel Dobson)
c52789365e5dbcb25aa5f1775de4d318da79e5a7 Allow locked UTXOs to be store in the wallet database (Samuel Dobson)
Pull request description:
Addresses and closes#22368
As per that issue (and its predecessor #14907), there seems to be some interest in allowing unspent outputs to be locked persistently. This PR does so by adding a flag to lockunspent to store the change in the wallet database. Defaults to false, so there is no change in default behaviour.
Edit: GUI commit changes default behaviour. UTXOs locked/unlocked via the GUI are now persistent.
ACKs for top commit:
achow101:
ACK d96b000e94d72d041689c5c47e374df2ebc0e011
kristapsk:
ACK d96b000e94d72d041689c5c47e374df2ebc0e011
lsilva01:
Tested ACK d96b000e94 on Ubuntu 20.04
prayank23:
ACK d96b000e94
Tree-SHA512: 957a5bbfe7f763036796906ccb1598feb6c14c5975838be1ba24a198840bf59e83233165cb112cebae909b6b25bf27275a4d7fa425923ef6c788ff671d7a89a8
There are two more cases where waste can be 0, when:
- (Fee - LTF) == -Change Cost
- (Fee - LTF) == -Excess
Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.
98cf19ca32785c991628324c313e01349c2986af wallet: refactor: avoid duplicate lookup on `mapValue["timesmart"]` (Sebastian Falbesoner)
973d8ba93d0fa00bed4569287e32696300036ab8 wallet: refactor: inline function WriteOrderPos() (Sebastian Falbesoner)
65ed198295e58cf1bc339aa17349b83490872f70 wallet: refactor: inline function ReadOrderPos() (Sebastian Falbesoner)
Pull request description:
The functions `ReadOrderPos` and `WriteOrderPos` have been introduced in commit 9c7722b7c5ce49130bd978b932f73b629ce5cebe in 2012. Since accounts have been removed in #13825 (commit c9c32e6b844fc79467b7e24c6c916142a0d08484), they are only called at one place in `CWalletTx::{Serialize,Unserialize}` and thus can be directly inlined instead. Additionally, this PR aims to avoids duplicate lookups on the map `mapValue` (affects keys "n" and "timesmart").
ACKs for top commit:
laanwj:
Code review ACK 98cf19ca32785c991628324c313e01349c2986af
achow101:
Code Review ACK 98cf19ca32785c991628324c313e01349c2986af
Tree-SHA512: 8af63c174c79e589bd713f04e8e40caba9f93ec2978c805427cac50d48049808a8c23ff5eea9ef589c9bd79fc66087f43ff5ab28e3cda51dd03f37c0164e2e4c
e4709c7b56612553fb7cbf16ef2d5099c5b732d0 Start using init makeNode, makeChain, etc methods (Russell Yanofsky)
Pull request description:
Use `interfaces::Init::make*` methods instead of `interfaces::Make*` functions, so interfaces can be constructed differently in different executable without having to change any code. (So for example `bitcoin-gui` can make an `interfaces::Node` pointer that communicates with a `bitcoin-node` subprocess, while `bitcoin-qt` can make an `interfaces::Node` pointer that controls node code in the same process.)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
jamesob:
reACK e4709c7b56
achow101:
ACK e4709c7b56612553fb7cbf16ef2d5099c5b732d0
benthecarman:
utACK e4709c7b56612553fb7cbf16ef2d5099c5b732d0
Tree-SHA512: 580c1979dbb2ef444157c8e53041e70d15ddeee77e5cbdb34f70b6d228cc2d2fe3843825f172da84e506200c58f7e0932f7cd4c006bb5058c1f4e43259394834
Since accounts were removed in commit c9c32e6b844fc79467b7e24c6c916142a0d08484,
this function is only called at one place and thus can be as well inlined.
Since accounts were removed in commit c9c32e6b844fc79467b7e24c6c916142a0d08484,
this function is only called at one place and thus can be as well inlined. Also,
avoid a duplicate lookup by using the find() method and dereference, instead of
calling count() and operator[].
fa57fa1a2e764792b873998ddf38db2ac061dcb6 Enable clang-tidy bugprone-argument-comment and fix violations (MarcoFalke)
Pull request description:
Named arguments can be dangerous when they are wrong, because they are not enforced by the compiler. Currently there are only minor typos, no actual bugs.
Fix the typos and add the `.clang-tidy` file to make it easier to find them in the future.
ACKs for top commit:
practicalswift:
cr ACK fa57fa1a2e764792b873998ddf38db2ac061dcb6
fanquake:
ACK fa57fa1a2e764792b873998ddf38db2ac061dcb6
Tree-SHA512: b66f01e0a1e77e56ed8454002176df660cc2cc0947a90785aa33cc5b8003a1f99fd8b2f8f89f2a0bf180ff2c42c031d69e669d127bb557b879c17975275a220b
3633b667ffca5a715d9fb27e977515c1e24f600a Use SelectCoinsSRD if it has less waste (Andrew Chow)
8bf789b4b4b26082aea1d91c4d7aa8b01aedfdcf Add SelectCoinsSRD function (Andrew Chow)
2ad3b5d2ad03f781f564ee697ef11e18b2edcea3 tests: wallet_basic lock needed unspents (Andrew Chow)
b77885f13e480304085c654f1b948b20bba63452 tests: wallet_txn explicilty specify inputs (Andrew Chow)
59ba7d2861359f19fa740da9daad1a199409583f tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow)
a165bfbe44b4db3a40b8d1ba8095035367c932a7 tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow)
df765a484d84702f066e127813fa45a7d440a957 tests: rpc_fundrawtx lock to UTXO types (Andrew Chow)
Pull request description:
To ease in the use of SRD as our fallback mechanism, this PR adds it as a secondary fallback algorithm in addition to the knapsack solver. Since #22009, the solution with the least waste will be chosen. This pattern is continued with SRD simply being another solution whose waste is compared.
ACKs for top commit:
glozow:
reACK 3633b66 via `git range-diff 981b9d1...3633b66`, thanks for taking the suggestions
laanwj:
Concept and code review ACK 3633b667ffca5a715d9fb27e977515c1e24f600a
Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92
Try to find a solution with SelectCoinsSRD. If we do have one, add it to
the list of solutions from which we choose the one with the least waste
as the solution to use.
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
86beee05795216738f51fa744539336503c26fd9 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c291a316298e54e73426c765e61c129 tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e05cd7b403f87aae4c4d42e5aea810b Add waste metric calculation function (Andrew Chow)
935b3ddf72aa390087684e03166c707f5b173434 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f904efe38dacd662d919aba74f066b1dc tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa7d335f3043227f843cbb9d8ba1507b tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b47463d98f860167d4e0b7e4ebb3926b59c Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee05795216738f51fa744539336503c26fd9
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
In order to change the KnapsackSolver tests to call KnapsackSolver, we
need KnapsackGroupOutputs to create the OutputGroups filtered with the
filter criteria.
