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.
b58f009d9585aab775998644de07e27e2a4a8045 test: check that mempool msgs lead to disconnect if uploadtarget is reached (Sebastian Falbesoner)
dd5cf38818d1e3f6cf583e2b242afd0da68b290a test: check for specific disconnect reasons in feature_maxuploadtarget.py (Sebastian Falbesoner)
73d737211536de5b834f21016c5549e52de11374 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 b58f009d9585aab775998644de07e27e2a4a8045
achow101:
ACK b58f009d9585aab775998644de07e27e2a4a8045
sr-gi:
tACK [b58f009](b58f009d95)
Tree-SHA512: 7439134129695c9c3a7ddc5e39f2ed700f91e7c91f0b7a9e0a783f275c6aa2f9918529cbfd38bb37f9139184e05e0f0354ef3c3df56da310177ec1d6b48b43d0
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
cc87ee4c3934028e78a59de509951ff7226ec80d 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 cc87ee4c3934028e78a59de509951ff7226ec80d
epiccurious:
Concept ACK cc87ee4c3934028e78a59de509951ff7226ec80d.
achow101:
ACK cc87ee4c3934028e78a59de509951ff7226ec80d
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>
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
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 86960cdb7f
Tree-SHA512: 10c941df3cd84fd8662b9c9ca6a1ed2c7402d38c677d2fc66b8b6c9edc6d73e827a5821487bbcacb5569d502934fa548fd10699e2ec45185f869e43174d8b2a1
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 cfcb9b1ecf
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.
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
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
facafa90f7a1eee452f9baf8a1b65a2edac0982b 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 d280617bf569f84457eaea546541dc74c67cd1e4.
(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)
ACKs for top commit:
theStack:
LGTM ACK facafa90f7a1eee452f9baf8a1b65a2edac0982b
Empact:
ACK facafa90f7a1eee452f9baf8a1b65a2edac0982b
Tree-SHA512: 35ac11bb5382dffe132bfae6097efc343ef6c06b1b4b1545130ca27b228ca6894679004862fee921b095172abaddbef5972c24d9bc195ce970f35643bd4a0f09
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
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.
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
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
There are occasions where a multi-statement tx is begun in one batch,
and a second batch is created which does a normal write (without a
multi-statement tx). These should not conflict with each other and all
of the data should end up being written to disk.
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.
