13161ecf03 opt: Skip over barren combinations of tiny UTXOs (Murch)
b7672c7cdd opt: Skip checking max_weight separately (Murch)
1edd2baa37 opt: Cut if last addition was minimal weight (Murch)
5248e2a60d opt: Skip heavier UTXOs with same effective value (Murch)
9124c73742 opt: Tiebreak UTXOs by weight for CoinGrinder (Murch)
451be19dc1 opt: Skip evaluation of equivalent input sets (Murch)
407b1e3432 opt: Track remaining effective_value in lookahead (Murch)
5f84f3cc04 opt: Skip branches with worse weight (Murch)
d68bc74fb2 fuzz: Test optimality of CoinGrinder (Murch)
67df6c629a fuzz: Add CoinGrinder fuzz target (Murch)
1502231229 coinselection: Track whether CG completed (Murch)
7488acc646 test: Add coin_grinder_tests (Murch)
6cc9a46cd0 coinselection: Add CoinGrinder algorithm (Murch)
89d0956643 opt: Tie-break UTXO sort by waste for BnB (Murch)
aaee65823c 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 13161ecf03
sr-gi:
ACK [13161ec](13161ecf03)
sipa:
ACK 13161ecf03
Tree-SHA512: 895b08b2ebfd0b71127949b7dba27146a6d10700bf8590402b14f261e7b937f4e2e1b24ca46de440c35f19349043ed2eba4159dc2aa3edae57721384186dae40
The goal of the function is to erase the wallet transactions that
match the inputted hashes. There is no need to traverse the database,
reading record by record, to then perform single entry removals for
each of them.
To ensure consistency and improve performance, this change-set removes
all tx records within a single atomic db batch operation, as well as
it cleans up code, improves error handling and simplifies the
transactions removal process entirely.
This optimizes the removal of watch-only transactions during the wallet
migration process and the 'removeprunedfunds' RPC command.
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.
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.
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.
b58f009d95 test: check that mempool msgs lead to disconnect if uploadtarget is reached (Sebastian Falbesoner)
dd5cf38818 test: check for specific disconnect reasons in feature_maxuploadtarget.py (Sebastian Falbesoner)
73d7372115 test: verify `-maxuploadtarget` limit state via `getnettotals` RPC result (Sebastian Falbesoner)
Pull request description:
This PR improves existing and adds new test coverage for the `-maxuploadtarget` mechanism (feature_maxuploadtarget.py) in the following ways, one commit each:
* verify the uploadtarget state via the `getnettotals` RPC (`uploadtarget` result field):
160d23677a/src/rpc/net.cpp (L581-L582)
Note that reaching the total limit (`target_reached` == True) always implies that the historical blocks serving limits is also reached (`serve_historical_blocks` == False), i.e. it's impossible that both flags are set to True.
* check for peer's specific disconnect reason (in this case, `"historical block serving limit reached, disconnect peer"`):
160d23677a/src/net_processing.cpp (L2272-L2280)
* add a test for a peer disconnect if the uploadtarget is reached and a `mempool` message is received (if bloom filters are enabled):
160d23677a/src/net_processing.cpp (L4755-L4763)
Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is already covered in the functional test `p2p_nobloomfilter_messages.py`.
ACKs for top commit:
maflcko:
lgtm ACK b58f009d95
achow101:
ACK b58f009d95
sr-gi:
tACK [b58f009](b58f009d95)
Tree-SHA512: 7439134129695c9c3a7ddc5e39f2ed700f91e7c91f0b7a9e0a783f275c6aa2f9918529cbfd38bb37f9139184e05e0f0354ef3c3df56da310177ec1d6b48b43d0
5ca9b24da1 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 5ca9b24da1
edilmedeiros:
Tested ACK 5ca9b24da1
achow101:
ACK 5ca9b24da1
ryanofsky:
Tested ACK 5ca9b24da1.
Tree-SHA512: 470969d44585d7cc33ad038a16e791db9e2be8469f52ddf122c46f20776fad34e6a48f988861a132c42540158fed05f3cf66fcc3bea05708253daaa35af54339
cc87ee4c39 test: fix intermittent failure in rpc_setban.py --v2transport (Martin Zumsande)
Pull request description:
This test failed for me on master locally:
The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later `connect_nodes` call.
Also add the test with `--v2transport` to the test runner because banning with v2 seems like a useful thing to have test coverage for.
ACKs for top commit:
delta1:
tested ACK cc87ee4c39
epiccurious:
Concept ACK cc87ee4c39.
achow101:
ACK cc87ee4c39
stratospher:
tested ACK cc87ee4. nice find!
Tree-SHA512: ae234d9b771d9c9c11501ddd93c99cf93257c999de3da62280d4d51806cd246b289c10a5f41fa7d5651b2fb4fdaee753f5b2d6939a99f89d71aa012af4a4d231
Ensure we are checking sigop-adjusted virtual size by creating setups
and packages where sigop cost is larger than bip141 vsize.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
e064487ca2 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 e064487ca2
amitiuttarwar:
ACK e064487ca2
stratospher:
ACK e064487ca2. simple use of << instead of a loop, didn't observe any behaviour difference before and after.
Tree-SHA512: ff0a65155e47f65d2ce3cb5a3fd7a86efef1861181143df13a9d8e59cb16aee9be2f8801457bba8478b17fac47b015bff5cc656f6fac2ccc071ee7178a38d291
fab41697a5 Allow int8_t optimized vector serialization (MarcoFalke)
facaa14785 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 fab41697a5
achow101:
ACK fab41697a5
TheCharlatan:
ACK fab41697a5
Tree-SHA512: a3e20f375fd1d0e0dedb827a8ce528de1173ea69660c8c891ad1343a86b422072f6505096fca0d3f8af4442fbe1378a02e32d5974919d4e88ff06934d0258cba
86960cdb7f wallet: migration, batch addressbook records removal (furszy)
342c45f80e wallet: addressbook migration, batch db writes (furszy)
595bbe6e81 refactor: wallet, simplify addressbook migration (furszy)
d0943315b1 refactor: SetAddressBookWithDB, minimize number of map lookups (furszy)
bba4f8dcb5 refactor: SetAddrBookWithDB, signal only if write succeeded (furszy)
97b0753923 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 86960cdb7f
josibake:
reACK 86960cdb7f
Tree-SHA512: 10c941df3cd84fd8662b9c9ca6a1ed2c7402d38c677d2fc66b8b6c9edc6d73e827a5821487bbcacb5569d502934fa548fd10699e2ec45185f869e43174d8b2a1
cfcb9b1ecf test: wallet, coverage for concurrent db transactions (furszy)
548ecd1155 tests: Test for concurrent writes with db tx (Ava Chow)
395bcd2454 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 cfcb9b1ecf
furszy:
ACK cfcb9b1ecf
ryanofsky:
Code review ACK cfcb9b1ecf. 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.
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
a17fd33edd GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice (Luke Dashjr)
98e9ac5199 GUI: Use FontChoice type in OptionsModel settings abstraction (Luke Dashjr)
3a6757eed9 GUI: Load custom FontForMoney from QSettings (Luke Dashjr)
49eb97eff9 GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel (Luke Dashjr)
f2dfde80b8 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 a17fd33edd
hebasto:
ACK a17fd33edd, 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
facafa90f7 test: Fix CPartialMerkleTree.nTransactions signedness (MarcoFalke)
Pull request description:
It is unsigned in Bitcoin Core, so the tests should match it:
aa9231fafe/src/merkleblock.h (L59)
Large positive values, or "negative" values, are rejected anyway, but it still seems fine to fix this.
The bug was introduced when the code was written in d280617bf5.
(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)
ACKs for top commit:
theStack:
LGTM ACK facafa90f7
Empact:
ACK facafa90f7
Tree-SHA512: 35ac11bb5382dffe132bfae6097efc343ef6c06b1b4b1545130ca27b228ca6894679004862fee921b095172abaddbef5972c24d9bc195ce970f35643bd4a0f09
b14298c5bc 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 fa15861763, they are not used anymore. This PR removes them.
ACKs for top commit:
maflcko:
lgtm ACK b14298c5bc
epiccurious:
utACK b14298c5bc
ryanofsky:
Code review ACK b14298c5bc
Tree-SHA512: 164e6a66ba05e11176a0cf68db6257f0ac07459cf7aa01ec4302b303c156c205a68128373a0b8daba0a6dfbff990af7fa14465a6341a296312fb20ea778c7a8c
Sending multiple large messages is rather slow with the non-optimized python
implementation of ChaCha20.
Apart from the slowness, these tests would also run successfully with v2.
By adding to the test framework a wait until the v2 handshake
is completed, so that p2p_sendtxrcncl.py (which doesn't need
to be changed itself) doesnt't send out any other messages before that.
