| Age | Commit message (Collapse) | Author |
|---|
|
13161ecf032b7a850686e5942c12222c8f3d0d52 opt: Skip over barren combinations of tiny UTXOs (Murch)
b7672c7cdd87acb105639f475744094b53cc9891 opt: Skip checking max_weight separately (Murch)
1edd2baa37a69ff69c2eaeceaad1028f1968cbab opt: Cut if last addition was minimal weight (Murch)
5248e2a60d243b3d5c77ecd9e4c335daca399a48 opt: Skip heavier UTXOs with same effective value (Murch)
9124c73742287b06dfe6e8a94be56ace25f07b2c opt: Tiebreak UTXOs by weight for CoinGrinder (Murch)
451be19dc10381122f705bbb2127b083f1d598c6 opt: Skip evaluation of equivalent input sets (Murch)
407b1e3432b77511900b77be84d5d7450352f462 opt: Track remaining effective_value in lookahead (Murch)
5f84f3cc043c5fb15072f5072fee752eaa01a2ec opt: Skip branches with worse weight (Murch)
d68bc74fb2e3ae4ae775ab544fe5b4ab46025abb fuzz: Test optimality of CoinGrinder (Murch)
67df6c629a2e9878b06c1903e90b67087eaa3688 fuzz: Add CoinGrinder fuzz target (Murch)
1502231229dbc32c94e80a2fc2959275c5d246ef coinselection: Track whether CG completed (Murch)
7488acc64685ae16e20b78e7ad018137f27fe404 test: Add coin_grinder_tests (Murch)
6cc9a46cd0f4ed80d4523bbef1508142e0c80d27 coinselection: Add CoinGrinder algorithm (Murch)
89d09566431f57034d9a7df32547ceb13d79c62c opt: Tie-break UTXO sort by waste for BnB (Murch)
aaee65823c6e620bef5cc96d8026567e64d822fe doc: Document max_weight on BnB (Murch)
Pull request description:
***Please refer to the [topic on Delving Bitcoin](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) discussing Gutter Guard/Coingrinder simulation results.***
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can significantly increase the fees
- Users are upset when fees are relatively large compared to the amount sent
- Some users struggle to maintain a sufficient count of UTXOs in their wallet
Approach
---
So far, Bitcoin Core has used a balanced approach to coin selection, where it will generate multiple input set candidates using various coin selection algorithms and pick the least wasteful among their results, but not explicitly minimize the input set weight. Under some circumstances, we _do_ want to minimize the weight of the input set. Sometimes changeless solutions require many or heavy inputs, and there is not always a changeless solution for Branch and Bound to find in the first place. This can cause expensive transactions unnecessarily. Given a wallet with sufficient funds, `CoinGrinder` will pick the minimal-waste input set for a transaction with a change output. The current implementation only runs `CoinGrinder` at feerates over 3×long-term-feerate-estimate (by default 30 ṩ/vB), which may be a decent compromise between our goal to reduce costs for the users, but still permit transactions at lower feerates to naturally reduce the wallet’s UTXO pool to curb bloat.
Trade-offs
---
Simulations for my thesis on coin selection ([see Section 6.3.2.1 [PDF]](https://murch.one/erhardt2016coinselection.pdf)) suggest that minimizing the input set for all transactions tends to grind a wallet’s UTXO pool to dust (pun intended): an approach selecting inputs per coin-age-priority (in effect similar to “largest first selection”) on average produced a UTXO pool with 15× the UTXO count as Bitcoin Core’s Knapsack-based Coin Selection then (in 2016). Therefore, I do not recommend running `CoinGrinder` under all circumstances, but only at extreme feerates or when we have another good reason to minimize the input set for other reasons. In the long-term, we should introduce additional metrics to score different input set candidates, e.g. on basis of their privacy and wallet health impact, to pick from all our coin selection results, but until then, we may want to limit use of `CoinGrinder` in other ways.
ACKs for top commit:
achow101:
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
sr-gi:
ACK [13161ec](https://github.com/bitcoin/bitcoin/pull/27877/commits/13161ecf032b7a850686e5942c12222c8f3d0d52)
sipa:
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
Tree-SHA512: 895b08b2ebfd0b71127949b7dba27146a6d10700bf8590402b14f261e7b937f4e2e1b24ca46de440c35f19349043ed2eba4159dc2aa3edae57721384186dae40
|
|
Given a lot of small amount UTXOs it is possible that the lookahead
indicates sufficient funds, but any combination of them would push us
beyond the current best_weight.
We can estimate a lower bound for the minimal necessary weight to reach
target from the maximal amount and minimal weight in the tail of the
UTXO pool: if adding a number of hypothetical UTXOs of this maximum
amount and minimum weight would not be able to beat `best_weight`, we
can SHIFT to the omission branch, and CUT if the last selected UTXO is
not heavier than the minimum weight of the remainder.
|
|
Initialize `best_selection_weight` as `max_weight` allows us to skip the
separate `max_weight` check on every loop.
|
|
In situations where we have UTXO groups of various weight, we can CUT
rather than SHIFT when we exceeded the max_weight or the best
selection’s weight while the last step was equal to the minimum weight
in the lookahead.
|
|
When two successive UTXOs differ in weight but match in effective value,
we can skip the second if the first is not selected, because all input
sets we can generate by swapping out a lighter UTXOs with a heavier UTXO
of matching effective value would be strictly worse.
|
|
|
|
When two successive UTXOs match in effective value and weight, we can
skip the second if the prior is not selected: adding it would create an
equivalent input set to a previously evaluated.
E.g. if we have three UTXOs with effective values {5, 3, 3} of the same
weight each, we want to evaluate
{5, _, _}, {5, 3, _}, {5, 3, 3}, {_, 3, _}, {_, 3, 3},
but skip {5, _, 3}, and {_, _, 3}, because the first 3 is not selected,
and we therefore do not need to evaluate the second 3 at the same
position in the input set.
If we reach the end of the branch, we must SHIFT the previously selected
UTXO group instead.
|
|
Introduces a dedicated data structure to track the total
effective_value available in the remaining UTXOs at each index of the
UTXO pool. In contrast to the approach in BnB, this allows us to
immediately jump to a lower index instead of visiting every UTXO to add
back their eff_value to the lookahead.
|
|
Once we exceed the weight of the current best selection, we can always
shift as adding more inputs can never yield a better solution.
|
|
Co-authored-by: Pieter Wuille <pieter@wuille.net>
|
|
|
|
CoinGrinder may not be able to exhaustively search all potentially
interesting combinations for large UTXO pools, so we keep track of
whether the search was terminated by the iteration limit.
|
|
|
|
CoinGrinder is a DFS-based coin selection algorithm that
deterministically finds the input set with the lowest weight creating a
change output.
|
|
5ca9b24da18e842e7a093dc44f6b222af73e92cf test: Add makefile target for running unit tests (TheCharlatan)
Pull request description:
`make check` runs a bunch of other subtree tests that exercise code that is hardly ever changed and have a comparatively long runtime. There seems to be no target for running just the unit tests, so add one.
Alternatively the secp256k1 tests could be removed from the `check-local` target, reducing its runtime. This was rejected before though in https://github.com/bitcoin/bitcoin/pull/20264.
ACKs for top commit:
delta1:
utACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf
edilmedeiros:
Tested ACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf
achow101:
ACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf
ryanofsky:
Tested ACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf.
Tree-SHA512: 470969d44585d7cc33ad038a16e791db9e2be8469f52ddf122c46f20776fad34e6a48f988861a132c42540158fed05f3cf66fcc3bea05708253daaa35af54339
|
|
`AddSingle`
e064487ca28c12ba774c2f43a3c7acbdb1a278c9 addrman, refactor: improve stochastic test in `AddSingle` (brunoerg)
Pull request description:
This PR changes this algorithm to be O(1) instead of O(n). Also, in the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`), this changes it. the change is relatively simple and does not cause conflicts.
ACKs for top commit:
achow101:
ACK e064487ca28c12ba774c2f43a3c7acbdb1a278c9
amitiuttarwar:
ACK e064487ca28c12ba774c2f43a3c7acbdb1a278c9
stratospher:
ACK e064487ca28c12ba774c2f43a3c7acbdb1a278c9. simple use of << instead of a loop, didn't observe any behaviour difference before and after.
Tree-SHA512: ff0a65155e47f65d2ce3cb5a3fd7a86efef1861181143df13a9d8e59cb16aee9be2f8801457bba8478b17fac47b015bff5cc656f6fac2ccc071ee7178a38d291
|
|
fab41697a5448ef2861f65795bd63a4ccdda6a40 Allow int8_t optimized vector serialization (MarcoFalke)
facaa14785e006c1af5a8b17b10e2722af8d054e Faster std::byte (pre)vector (un)serialize (MarcoFalke)
Pull request description:
Currently, large vectors of `std::byte` are (un)serialized byte-by-byte, which is slow. Fix this, by enabling the already existing optimization for them.
On my system this gives a 10x speedup for `./src/bench/bench_bitcoin --filter=PrevectorDeserializeTrivial`, when `std::byte` are used:
```diff
diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp
index 2524e215e4..76b16bc34e 100644
--- a/src/bench/prevector.cpp
+++ b/src/bench/prevector.cpp
@@ -17,7 +17,7 @@ struct nontrivial_t {
static_assert(!std::is_trivially_default_constructible<nontrivial_t>::value,
"expected nontrivial_t to not be trivially constructible");
-typedef unsigned char trivial_t;
+typedef std::byte trivial_t;
static_assert(std::is_trivially_default_constructible<trivial_t>::value,
"expected trivial_t to be trivially constructible");
```
However, the optimization does not cover `signed char`. Fix that as well.
ACKs for top commit:
sipa:
utACK fab41697a5448ef2861f65795bd63a4ccdda6a40
achow101:
ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
TheCharlatan:
ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
Tree-SHA512: a3e20f375fd1d0e0dedb827a8ce528de1173ea69660c8c891ad1343a86b422072f6505096fca0d3f8af4442fbe1378a02e32d5974919d4e88ff06934d0258cba
|
|
translation string freeze
71927b24e5aceecd8a07cdaeb916898d45486bea qt: Update translation source file (Hennadii Stepanov)
4d0b0bf225b50918ec54097e47c25368843d5476 qt: Bump Transifex slug for 27.x (Hennadii Stepanov)
42cbf561a7c5530261a9c2fd7dd355872110dbdc qt: Translation updates from Transifex (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md).
Required to open Transifex translations for v27.0 as it's scheduled in https://github.com/bitcoin/bitcoin/issues/29028.
The previous similar PR: https://github.com/bitcoin/bitcoin/pull/28383.
ACKs for top commit:
jarolrod:
ACK 71927b24e5aceecd8a07cdaeb916898d45486bea
johnny9:
ACK 71927b24e5aceecd8a07cdaeb916898d45486bea
Tree-SHA512: 9492ffc39518fc4e519cdf9bc558b1f17325b27f17e3bfba0c11e54af13c2d98ca08d9bad51880d0b577f855f95fd0c4bd8e35570336f16a5b154597737f3943
|
|
migration process
86960cdb7f75eaa2ae150914c54240d1d5ef96d1 wallet: migration, batch addressbook records removal (furszy)
342c45f80e32b0320829ce380b5854844cd74bc8 wallet: addressbook migration, batch db writes (furszy)
595bbe6e81885d35179aba6137dc63d0e652cc1f refactor: wallet, simplify addressbook migration (furszy)
d0943315b1d00905fe7f4513b2f3f47b88a99e8f refactor: SetAddressBookWithDB, minimize number of map lookups (furszy)
bba4f8dcb55de3ca4963711dc17882b43cb0bc4a refactor: SetAddrBookWithDB, signal only if write succeeded (furszy)
97b075392305becfbad4d497614478cff2d9237f wallet: clean redundancies in DelAddressBook (furszy)
Pull request description:
Commits decoupled from #28574, focused on the address book cloning process
Includes:
1) DB batch operations and flow simplification for the address book migration process.
2) Code improvements to `CWallet::DelAddressBook` and `Wallet::SetAddrBookWithDB` methods.
These changes will let us consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.
ACKs for top commit:
achow101:
ACK 86960cdb7f75eaa2ae150914c54240d1d5ef96d1
josibake:
reACK https://github.com/bitcoin/bitcoin/commit/86960cdb7f75eaa2ae150914c54240d1d5ef96d1
Tree-SHA512: 10c941df3cd84fd8662b9c9ca6a1ed2c7402d38c677d2fc66b8b6c9edc6d73e827a5821487bbcacb5569d502934fa548fd10699e2ec45185f869e43174d8b2a1
|
|
`SQLiteBatch`s
cfcb9b1ecf9be5267487713fa1e112ca09a1ae55 test: wallet, coverage for concurrent db transactions (furszy)
548ecd11553bea28bc79e6f9840836283e9c4e99 tests: Test for concurrent writes with db tx (Ava Chow)
395bcd245476d7c83abf2d82e31e90b6d0c432f3 sqlite: Ensure that only one SQLiteBatch is writing to db at a time (Ava Chow)
Pull request description:
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transaction is begun with one `SQLiteBatch`, any operations performed by another `SQLiteBatch` will also occur within the same transaction. Furthermore, those other `SQLiteBatch`s will not be expecting a transaction to be active, and will abort it once the `SQLiteBatch` is destructed. This is problematic as it will prevent some data from being written, and also cause the `SQLiteBatch` that opened the transaction in the first place to be in an unexpected state and throw an error.
To avoid this situation, we need to prevent the multiple batches from writing at the same time. To do so, I've implemented added a `CSemaphore` within `SQLiteDatabase` which will be used by any `SQLiteBatch` trying to do a write operation. `wait()` is called by `TxnBegin`, and at the beginning of `WriteKey`, `EraseKey`, and `ErasePrefix`. `post()` is called in `TxnCommit`, `TxnAbort` and at the end of `WriteKey`, `EraseKey`, and `ErasePrefix`. To avoid deadlocking on ` TxnBegin()` followed by a `WriteKey()`, `SQLiteBatch will now also track whether a transaction is in progress so that it knows whether to use the semaphore.
This issue is not a problem for BDB wallets since BDB uses WAL and provides transaction objects that must be used if an operation is to occur within a transaction. Specifically, we either pass a transaction pointer, or a nullptr, to all BDB operations, and this allows for concurrent transactions so it doesn't have this problem.
Fixes #29110
ACKs for top commit:
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/29112/commits/cfcb9b1ecf9be5267487713fa1e112ca09a1ae55
furszy:
ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55
ryanofsky:
Code review ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55. This looks great and I think it is ready for merge. Just holding off because josibake seemed ready to review https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190 and might have more feedback.
Tree-SHA512: 2dd5a8e76df52451a40e0b8a87c7139d68a0d8e1bf2ebc79168cc313e192dab87cfa4270ff17fea4f7b370060d3bc9b5d294d50f7e07994d9b5a69b40397c927
|
|
Instead of doing one db transaction per removed record,
we now batch all removals in a single db transaction.
Speeding up the process and preventing the wallet from entering
an inconsistent state when any of the intermediate writes fail.
|
|
Optimizing the process performance and consistency.
|
|
Same process written in a cleaner manner.
Removing code duplication.
|
|
|
|
|
|
1) Encode destination only once (instead of three).
2) Fail if the entry's linked data cannot be removed.
3) Don't remove entry from memory if db write fail.
4) Notify GUI only if removal succeeded
|
|
specifically
a17fd33edd1374145fd6986fbe352295355fde4f GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice (Luke Dashjr)
98e9ac51992b2332587d87f25351988bf4863238 GUI: Use FontChoice type in OptionsModel settings abstraction (Luke Dashjr)
3a6757eed9a24e91e7d800d8026cc3a5c4840141 GUI: Load custom FontForMoney from QSettings (Luke Dashjr)
49eb97eff96c2ec9e5a55d599f18b1866f83b115 GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel (Luke Dashjr)
f2dfde80b85b202bece0b5b4c8f1c8777c1a660d GUI: Move "embedded font or not" decision into new OptionsModel::getFontForMoney method (Luke Dashjr)
Pull request description:
This replaces the overly-verbose radio-button font setting (which only allows embedded or autodetected from system) with a simple combobox providing both existing options as well as a custom option to allow the user to select any font of their choice/style.
ACKs for top commit:
pablomartin4btc:
tested ACK a17fd33edd1374145fd6986fbe352295355fde4f
hebasto:
ACK a17fd33edd1374145fd6986fbe352295355fde4f, I have reviewed the code and tested it on Ubuntu 22.04. This is a UX improvement. https://github.com/bitcoin-core/gui/pull/497#issuecomment-1341222673 might be addressed in a follow-up.
Tree-SHA512: 2f0a8bc1242a374c4b7dc6e34014400428b6d36063fa0b01c9f62a8bd6078adfbbca93d95c87e4ccb580d982fe10173e1d9a28bcec586591dd3f966c7b90fc5d
|
|
fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 qt: change QLineEdit error background (w0xlt)
Pull request description:
This PR proposes a small change in QLineEdit when there is an error in the input.
master |
--- |
PR |
--- |
 |
This also shows good results when combined with other open PRs.
#537 |
--- |
#533 |
--- |
 |
ACKs for top commit:
GBKS:
ACK fe7c81e
jarolrod:
ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25
shaavan:
ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25
Tree-SHA512: eccc53f42d11291944ccb96efdbe460cb10af857f1d4fa9b5348ddcb0796c82faf1cdad9040aae7a25c5d8f4007d6284eba868d7af14acf56280f6acae170b91
|
|
The diff is generated by executing `make -C src translate`.
|
|
The diff is generated by executing the `update-translations.py` script.
|
|
`FuzzedWallet`
b14298c5bca395e5ed6a27fe1758c0d1f4b824ac fuzz: remove unused `args` and `context` from `FuzzedWallet` (brunoerg)
Pull request description:
`ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763df71e788849b587883b3c16bb12229, they are not used anymore. This PR removes them.
ACKs for top commit:
maflcko:
lgtm ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
epiccurious:
utACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
ryanofsky:
Code review ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
Tree-SHA512: 164e6a66ba05e11176a0cf68db6257f0ac07459cf7aa01ec4302b303c156c205a68128373a0b8daba0a6dfbff990af7fa14465a6341a296312fb20ea778c7a8c
|
|
instances in spkm's `FillPSBT` methods
e2ad343f69af4f924b22dccf94a52b6431ef6e3c wallet: remove unused `SignatureData` instances in spkm's `FillPSBT` methods (Sebastian Falbesoner)
Pull request description:
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.
ACKs for top commit:
achow101:
ACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c
brunoerg:
crACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c
Tree-SHA512: f0cabcc000bcea6bc7d7ec9d3be2e2a8accbdbffbe35252250ea2305b65a5813fde2b8096fbdd2c7cccdf417ea285183dc325fc2d210d88bce62978ce642930e
|
|
decryption process
2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 wallet: remove unused 'accept_no_keys' arg from decryption process (furszy)
Pull request description:
Found it while reviewing other PR. Couldn't contain myself from cleaning it up.
The wallet decryption process (`CheckDecryptionKey()` and `Unlock()`)
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented `SplitWalletPath`
function.
ACKs for top commit:
delta1:
ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452
epiccurious:
utACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452.
achow101:
ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452
theStack:
Code-review ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452
Tree-SHA512: e0537c994be19ca0032551d8a64cf1755c8997e04d21ee0522b31de26ad90b9eb02a8b415448257b60bced484b9d2a23b37586e12dc5ff6e35bdd8ff2165c6bf
|
|
Verifying that a database handler can't commit/abort changes
occurring in a different database handler.
|
|
A SQLiteBatch need to wait for any other batch to finish writing before
it can begin writing, otherwise db txn state may be incorrectly
modified. To enforce this, each SQLiteDatabase has a semaphore which
acts as a lock and is acquired by a batch when it begins a write, erase,
or a transaction, and is released by it when it is done.
To avoid deadlocking on itself for writing during a transaction,
SQLiteBatch also keeps track of whether it has begun a transaction.
|
|
mandatory and few cleanups
e7fd70f4b6b163f4ad5b25b4da7fa79899245235 [test] make v2transport arg in addconnection mandatory and few cleanups (stratospher)
Pull request description:
- make `v2transport` argument in `addconnection` regression-testing only RPC mandatory. https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750
- previously it was an optional arg with default `false` value.
- only place this RPC is used is in the [functional tests](https://github.com/bitcoin/bitcoin/blob/11b436a66af3ceaebb0f907878715f331516a0bc/test/functional/test_framework/test_node.py#L742) where we always pass the appropriate `v2transport` option to the RPC anyways. (and that too just for python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) interactions)
- rename `v2_handshake()` to `_on_data_v2_handshake()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424
- more compact return statement in `wait_for_reconnect()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708
- assertion to check that empty version packets are received from `TestNode`.
ACKs for top commit:
glozow:
ACK e7fd70f4b6
theStack:
Code-review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235
mzumsande:
Code Review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235
Tree-SHA512: e66e29baccd91e1e4398b91f7d45c5fc7c2841d77d8a6178734586017bf2be63496721649da91848dec71da605ee31664352407d5bb896e624cc693767c61a1f
|
|
|
|
c819a83b4d83413a31323c1304664ee4c228ca29 Don't use scientific notation in log messages (Kristaps Kaupe)
Pull request description:
Don't see any benefits here, only harder to read for most of the users.
Before:
```
2024-01-16T13:11:36Z Dumped mempool: 8.165e-06s to copy, 0.00224268s to dump
```
After:
```
2024-01-16T13:11:36Z Dumped mempool: 0.000s to copy, 0.002s to dump
```
ACKs for top commit:
kristapsk:
> > > > lgtm ACK [c819a83](https://github.com/bitcoin/bitcoin/commit/c819a83b4d83413a31323c1304664ee4c228ca29). can you update the PR description?
glozow:
lgtm ACK c819a83b4d83413a31323c1304664ee4c228ca29. can you update the PR description?
Tree-SHA512: 0972e0a05934e1b014fdeca0c235065aa017ba9abf74b3018f514e4d8022ef02b7f042a07d3675144b51449492468aea6b5b0183233ad7f1bab887d18e3d06af
|
|
transactions
fa5cd66f0a47d1b759c93d01524ee4558432c0cc test: Assumeutxo with more than just coinbase transactions (MarcoFalke)
Pull request description:
Currently the AU tests only check that loading a txout set with only coinbase outputs works.
Fix that by adding other transactions.
ACKs for top commit:
jamesob:
ACK https://github.com/bitcoin/bitcoin/pull/29354/commits/fa5cd66f0a47d1b759c93d01524ee4558432c0cc
glozow:
concept ACK fa5cd66f0a47d1b759c93d01524ee4558432c0cc
Tree-SHA512: e090c41f73490ad72e36c478405bfd0716d46fbf5a131415095999da6503094a86689a179a84addae3562b760df64cdb67488f81692178c8ca8bf771b1e931ff
|
|
make check runs a bunch of other subtree tests that exercise code that
is hardly ever changed and have a comparatively long runtime. There
seems to be no target for running just the unit tests, so add one.
|
|
The wallet decryption process (CheckDecryptionKey() and Unlock())
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented SplitWalletPath
function.
|
|
have both spendable and watchonly outputs
4da76ca24725eb9ba8122317e04a6e1ee14ac846 test: Test migration of tx with both spendable and watchonly (Ava Chow)
c62a8d03a862fb124b4f4b88efd61978e46605f8 wallet: Keep txs that belong to both watchonly and migrated wallets (Ava Chow)
71cb28ea8cb579ac04cefc47a57557c94080d1af test: Make sure that migration test does not rescan on reloading (Ava Chow)
78ba0e6748d2a519a96c41dea851e7c43b82f251 wallet: Reload the wallet if migration exited early (Ava Chow)
9332c7edda79a39bb729b71b6f8db6a9d37343bb wallet: Write bestblock to watchonly and solvable wallets (Ava Chow)
Pull request description:
A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both.
I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen.
The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits.
ACKs for top commit:
ryanofsky:
Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.
furszy:
Code review ACK 4da76ca2
Tree-SHA512: 5fc210cff16ca6720d7b2d0616d7e3f295c974147854abc704cf99a3bfaad17572ada084859e7a1b1ca94da647ad130303219678f429b7995f85e040236db35c
|
|
blank wallets
3904123da954f9ebd905de4129aec9f9bab9efc7 tests: Test that descriptors flag is set for migrated blank wallets (Ava Chow)
072d506240f6c39387b2edd4421818cc914c0912 wallet: Make sure that the descriptors flag is set for blank wallets (Ava Chow)
Pull request description:
While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets.
To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this.
ACKs for top commit:
epiccurious:
Tested ACK 3904123da954f9ebd905de4129aec9f9bab9efc7.
delta1:
tested ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
ryanofsky:
Code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
murchandamus:
code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
Tree-SHA512: 9f6fe9c1899ca387ab909b1bb6956cd6bc5acbf941686ddc6c061f9b1ceec2cc9d009ff472486fc86e963f6068f0e2f1ae96282e7c630193797a9734c4f424ab
|
|
fad0fafd5aca699cfab7673f8eb18211139aeb18 refactor: Fix timedata includes (MarcoFalke)
Pull request description:
Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it.
ACKs for top commit:
achow101:
ACK fad0fafd5aca699cfab7673f8eb18211139aeb18
dergoegge:
utACK fad0fafd5aca699cfab7673f8eb18211139aeb18
stickies-v:
ACK fad0fafd5aca699cfab7673f8eb18211139aeb18
Tree-SHA512: 45e86f2eb90f0e37012bd83bf30259719e0e58ede18b31f51ca8a6f6d23e6ca4d060fc0f56f821a711cbdb45792b82cf780f5ae3226680d7a966471990f352bc
|
|
|
|
It is possible for a transaction that has an output that belongs to the
mgirated wallet, and another output that belongs to the watchonly
wallet. Such transaction should appear in both wallets during migration.
|
|
Migration will unload loaded wallets prior to beginning. It will then
perform some checks which may exit early. Such unloaded wallets should
be reloaded prior to exiting.
|
|
When migrating, we should also be writing the bestblock record to the
watchonly and solvable wallets to avoid rescanning on the reload as that
can be slow.
|
|
fad74bbbd0ab4425573f182ebde1b31a99e80547 refactor: Mark prevector iterator with std::contiguous_iterator_tag (MarcoFalke)
fab8a01048fc6cfcee7e89884a5af31385189c63 refactor: Fix binary operator+ for prevector iterators (MarcoFalke)
fa44a60b2bb5cb91bc411e5b625fc81bd84befff refactor: Fix constness for prevector iterators (MarcoFalke)
facaa66b49ef7eeefe1d57c1bfc1bbe5b3011661 refactor: Add missing default constructor to prevector iterators (MarcoFalke)
Pull request description:
Currently prevector iterators have many issues:
* Forward iterators (and stronger) must be default constructible (https://eel.is/c++draft/forward.iterators#1.2). Otherwise, some functions can not be instantiated, like `std::minmax_element`.
* Various `const` issues with random access iterators. For example, a `const iterator` is different from a `const_iterator`, because the first one holds a mutable reference and must also return it without `const`. Also, `operator+` must be callable regardless of the iterator object's `const`-ness.
* When adding an offset to random access iterators, both `x+n` and `n+x` must be specified, see https://eel.is/c++draft/random.access.iterators#tab:randomaccessiterator
Fix all issues.
Also, upgrade the `std::random_access_iterator_tag` (C++17) to `std::contiguous_iterator_tag` (C++20)
ACKs for top commit:
TheCharlatan:
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547
stickies-v:
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547
willcl-ark:
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547
Tree-SHA512: b1ca778a31602af94b323b8feaf993833ec78be09f1d438a68335485a4ba97f52125fdd977ffb9541b89f8d45be0105076aa07b5726936133519aae832556e0b
|
|
|
