aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2024-02-15[refactor] Make signals optional in mempool and chainmanTheCharlatan
This is done in preparation for the next two commits, where the CMainSignals are de-globalized. This avoids adding new constructor arguments to the ChainstateManager and CTxMemPool classes over the next two commits. This could also allow future tests that are only interested in the internal behaviour of the classes to forgo instantiating the signals.
2024-02-09Merge bitcoin/bitcoin#28948: v3 transaction policy for anti-pinningAva Chow
29029df5c700e6940c712028303761d91ae15847 [doc] v3 signaling in mempool-replacements.md (glozow) e643ea795e4b6fea4a6bbb3d72870ee6a4c836b1 [fuzz] v3 transactions and sigop-adjusted vsize (glozow) 1fd16b5c62f54c7f4c60122acd65d852f63d1e8b [functional test] v3 transaction submission (glozow) 27c8786ba918a42c860e6a50eaee9fdf56d7c646 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke) 9a1fea55b29fe025355b06b45e3d77d192acc635 [policy/validation] allow v3 transactions with certain restrictions (glozow) eb8d5a2e7d939dd3ee683486e98702079e0dfcc0 [policy] add v3 policy rules (glozow) 9a29d470fbb62bbb27d517efeafe46ff03c25f54 [rpc] return full string for package_msg and package-error (glozow) 158623b8e0726dff7eae4288138f1710e727db9c [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow) Pull request description: See #27463 for overall package relay tracking. Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340 Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418 Rationale: - There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2] - Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution. V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2. Immediate benefits: - You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later. - Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction. This also enables some other cool things (again see #27463 for overall roadmap): - Ephemeral Anchors - Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees. - We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use. - We can switch to a cluster-based mempool [5] (#27677 #28676), which removes CPFP carve out [6]. [1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html [2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward. [3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html [4]: Original PR #25038 also contains a lot of the discussion [5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7 [6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12 ACKs for top commit: sdaftuar: ACK 29029df5c700e6940c712028303761d91ae15847 achow101: ACK 29029df5c700e6940c712028303761d91ae15847 instagibbs: ACK 29029df5c700e6940c712028303761d91ae15847 modulo that Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
2024-02-09Merge bitcoin/bitcoin#27877: wallet: Add CoinGrinder coin selection algorithmAva Chow
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
2024-02-09opt: Skip over barren combinations of tiny UTXOsMurch
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.
2024-02-09opt: Skip checking max_weight separatelyMurch
Initialize `best_selection_weight` as `max_weight` allows us to skip the separate `max_weight` check on every loop.
2024-02-09opt: Cut if last addition was minimal weightMurch
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.
2024-02-09opt: Skip heavier UTXOs with same effective valueMurch
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.
2024-02-09opt: Tiebreak UTXOs by weight for CoinGrinderMurch
2024-02-09opt: Skip evaluation of equivalent input setsMurch
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.
2024-02-09opt: Track remaining effective_value in lookaheadMurch
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.
2024-02-09opt: Skip branches with worse weightMurch
Once we exceed the weight of the current best selection, we can always shift as adding more inputs can never yield a better solution.
2024-02-09fuzz: Test optimality of CoinGrinderMurch
Co-authored-by: Pieter Wuille <pieter@wuille.net>
2024-02-09fuzz: Add CoinGrinder fuzz targetMurch
2024-02-09coinselection: Track whether CG completedMurch
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.
2024-02-09test: Add coin_grinder_testsMurch
2024-02-09coinselection: Add CoinGrinder algorithmMurch
CoinGrinder is a DFS-based coin selection algorithm that deterministically finds the input set with the lowest weight creating a change output.
2024-02-08Merge bitcoin/bitcoin#28996: test: maxuploadtarget: check for mempool msg ↵Ava Chow
disconnect if limit is reached, improve existing test coverage 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): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/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"`): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/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): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/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](https://github.com/bitcoin/bitcoin/commit/b58f009d9585aab775998644de07e27e2a4a8045) Tree-SHA512: 7439134129695c9c3a7ddc5e39f2ed700f91e7c91f0b7a9e0a783f275c6aa2f9918529cbfd38bb37f9139184e05e0f0354ef3c3df56da310177ec1d6b48b43d0
2024-02-08Merge bitcoin/bitcoin#29377: test: Add makefile target for running unit testsAva Chow
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
2024-02-08Merge bitcoin/bitcoin#29372: test: fix intermittent failure in ↵Ava Chow
`rpc_setban.py --v2transport`, run it in CI 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
2024-02-08[doc] v3 signaling in mempool-replacements.mdglozow
2024-02-08[fuzz] v3 transactions and sigop-adjusted vsizeglozow
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>
2024-02-08[functional test] v3 transaction submissionglozow
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com> Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
2024-02-08test framework: Add and use option for tx-version in MiniWallet methodsMarcoFalke
2024-02-08[policy/validation] allow v3 transactions with certain restrictionsglozow
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
2024-02-08[policy] add v3 policy rulesglozow
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
2024-02-08Merge bitcoin/bitcoin#27319: addrman, refactor: improve stochastic test in ↵Ava Chow
`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
2024-02-08Merge bitcoin/bitcoin#29114: util: Faster std::byte (pre)vector (un)serializeAva Chow
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
2024-02-08Merge bitcoin/bitcoin#29397: release: Update translations for v27.0 soft ↵Hennadii Stepanov
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
2024-02-08Merge bitcoin/bitcoin#26836: wallet: batch and simplify addressbook ↵Ava Chow
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
2024-02-07Merge bitcoin/bitcoin#29112: sqlite: Disallow writing from multiple ↵Ryan Ofsky
`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
2024-02-07wallet: migration, batch addressbook records removalfurszy
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.
2024-02-07wallet: addressbook migration, batch db writesfurszy
Optimizing the process performance and consistency.
2024-02-07refactor: wallet, simplify addressbook migrationfurszy
Same process written in a cleaner manner. Removing code duplication.
2024-02-07refactor: SetAddressBookWithDB, minimize number of map lookupsfurszy
2024-02-07refactor: SetAddrBookWithDB, signal only if write succeededfurszy
2024-02-07wallet: clean redundancies in DelAddressBookfurszy
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
2024-02-07Merge bitcoin-core/gui#497: Enable users to configure their monospace font ↵Hennadii Stepanov
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
2024-02-07Merge bitcoin-core/gui#553: Change address / amount error backgroundHennadii Stepanov
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 | --- | ![image](https://user-images.githubusercontent.com/94266259/154762427-b816267e-ec70-4a8f-a7fb-1317ebacf1a4.png) PR | --- | ![image](https://user-images.githubusercontent.com/94266259/154761933-15eb3d81-ca81-4498-b8ec-cf1139ae2f8a.png) | This also shows good results when combined with other open PRs. #537 | --- | ![image](https://user-images.githubusercontent.com/94266259/154763411-6266a283-2d8a-4365-b3f2-a5cb545e773e.png) #533 | --- | ![image](https://user-images.githubusercontent.com/94266259/154765638-f38b13d6-a4f8-4b46-a470-f882668239f3.png) | ACKs for top commit: GBKS: ACK fe7c81e jarolrod: ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 shaavan: ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 Tree-SHA512: eccc53f42d11291944ccb96efdbe460cb10af857f1d4fa9b5348ddcb0796c82faf1cdad9040aae7a25c5d8f4007d6284eba868d7af14acf56280f6acae170b91
2024-02-07Merge bitcoin/bitcoin#29363: test: Fix CPartialMerkleTree.nTransactions ↵fanquake
signedness facafa90f7a1eee452f9baf8a1b65a2edac0982b test: Fix CPartialMerkleTree.nTransactions signedness (MarcoFalke) Pull request description: It is unsigned in Bitcoin Core, so the tests should match it: https://github.com/bitcoin/bitcoin/blob/aa9231fafe45513134ec8953a217cda07446fae8/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
2024-02-07qt: Update translation source fileHennadii Stepanov
The diff is generated by executing `make -C src translate`.
2024-02-07qt: Bump Transifex slug for 27.xHennadii Stepanov
2024-02-07qt: Translation updates from TransifexHennadii Stepanov
The diff is generated by executing the `update-translations.py` script.
2024-02-06Merge bitcoin/bitcoin#29388: fuzz: remove unused `args` and `context` from ↵Ryan Ofsky
`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
2024-02-06Merge bitcoin/bitcoin#28833: wallet: refactor: remove unused `SignatureData` ↵Ava Chow
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
2024-02-06Merge bitcoin/bitcoin#29375: wallet: remove unused 'accept_no_keys' arg from ↵Ava Chow
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
2024-02-06test: wallet, coverage for concurrent db transactionsfurszy
Verifying that a database handler can't commit/abort changes occurring in a different database handler.
2024-02-06tests: Test for concurrent writes with db txAva Chow
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.
2024-02-06sqlite: Ensure that only one SQLiteBatch is writing to db at a timeAva Chow
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.
2024-02-06Merge bitcoin/bitcoin#29356: test: make v2transport arg in addconnection ↵glozow
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
2024-02-06Merge bitcoin/bitcoin#29353: test: p2p: adhere to typical VERSION message ↵glozow
protocol flow c340503b67585b631909584f1acaa327f5af25d3 test: p2p: adhere to typical VERSION message protocol flow (Sebastian Falbesoner) 7ddfc28309e6b70c273112a93f01e518409ceaa0 test: p2p: process post-v2-handshake data immediately (Sebastian Falbesoner) b198b9c2ce28463f7c6a2b4cf08d64138c8509ee test: p2p: introduce helper for sending prepared VERSION message (Sebastian Falbesoner) Pull request description: This PR addresses a quirk in the test framework's p2p implementation regarding the version handshake protocol: Currently, the VERSION message is sent immediately after an inbound connection (i.e. TestNode outbound connection) is made. This doesn't follow the usual protocol flow where the initiator sends a version first, the responder processes that and only then responds with its own version message. Change that accordingly by only sending immediate VERSION message for outbound connections (or after v2 handshake for v2 connections, respectively), and sending out VERSION message as response for incoming VERSION messages (i.e. in the function `on_version`) for inbound connections. I first stumbled upon this issue through reading comment https://mirror.b10c.me/bitcoin-bitcoin/24748/#discussion_r1465420112 (see last paragraph) and recently again in the course of working on a v2-followup for #29279, where this causes issues for TestNode outbound connections that disconnect *before* sending out their own version message. Note that these changes lead to slightly more code in some functional tests that override the `on_version` method, as the version reply has to be sent explicitly now, but I think is less confusing and reflects better what is actually happening. ACKs for top commit: epiccurious: utACK c340503b67585b631909584f1acaa327f5af25d3 stratospher: tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we'd want real node behaviour! mzumsande: ACK c340503b67585b631909584f1acaa327f5af25d3 sr-gi: tACK https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3 Tree-SHA512: 63eac287d3e1c87a01852bfd9f0530363354bbb642280298673b9c8817056356373adf348955c4e92af95c7c6efa8cc515cee2892e9f077bfbe1bce8e97ad082
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-09 09:52:30 +0000