aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2023-11-06[refactor] use Wtxid for m_wtxids_fee_calculationsglozow
2023-11-03test: bugfix CheckPackageMempoolAcceptResult return all error stringsGreg Sanders
2023-11-03Merge bitcoin/bitcoin#28762: MiniMiner changes for package linearizationAndrew Chow
d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9 [test] MiniMiner::Linearize and manual construction (glozow) dfd6a3788c35be121eba1ad84f20effadcb7e7dc [refactor] unify fee amounts in miniminer_tests (glozow) f4b1b24a3bcd0199a6d2e828ad46033e1368336e [MiniMiner] track inclusion order and add Linearize() function (glozow) 004075963f12f17c3c399d81e3b48d6a8151aebd [test] add case for MiniMiner working with negative fee txns (glozow) fe6332c0badf07e99044b00084fc6b07f735a051 [MiniMiner] make target_feerate optional (glozow) 5a83f55c96661a886dd6f5231920b2f730cf6773 [MiniMiner] allow manual construction with non-mempool txns (glozow) e3b2e630b219ca15fe0b2640ca422712c86ac33d [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow) 4aa98b79b266cd526efa577762b0bcfccbdeda11 [lint] update expected boost includes (glozow) Pull request description: This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253. - Allow using `MiniMiner` on transactions that aren't in the mempool. - Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected." - Add clarification for how this is different from `target_feerate=0` (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133) - Track the order in which transactions are included in the template to get the "linearization order" of the transactions. - Tests Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there. ACKs for top commit: TheCharlatan: ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9 kevkevinpal: reACK [d9cc99d](https://github.com/bitcoin/bitcoin/pull/28762/commits/d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9) achow101: re-ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9 Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
2023-11-03Merge bitcoin/bitcoin#28758: refactors for subpackage evaluationfanquake
b5a60abe8783852f5b31bc1e63b5836530410e65 MOVEONLY: CleanupTemporaryCoins into its own function (glozow) 10c0a8678cd28e7f0715e6cfa3e651903e4ad4aa [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow) 6ff647a7e0d85040a6033047c5cf84f8f22b1c65 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow) da9aceba217bbded6909f06144eaa1e1a4ebcb69 [refactor] move package checks into helper functions (glozow) Pull request description: This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253. - Split package sanitization in policy/packages.h into helper functions - Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597) - Rename `CheckPackage` to `IsPackageWellFormed` - Improve the `CreateValidTransaction` unit test utility to: - Configure the target feerate and return the fee paid - Signal BIP125 on transactions to enable RBF tests - Allow the specification of multiple inputs and outputs - Move `CleanupTemporaryCoins` into its own function to be reused later without duplication ACKs for top commit: dergoegge: Code review ACK b5a60abe8783852f5b31bc1e63b5836530410e65 instagibbs: ACK b5a60abe8783852f5b31bc1e63b5836530410e65 Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
2023-11-03Merge bitcoin/bitcoin#28764: Fuzz: Check individual and package transaction ↵glozow
invariants fcb3069fa307942cf7f3edabcda1be96d615c91f Use CheckPackageMempoolAcceptResult for package evaluation fuzzing (Greg Sanders) 34088d6c9ed4ed99bb6b7fc83795da01ec9f3c97 [test util] CheckPackageMempoolAcceptResult for sanity-checking results (glozow) 651fa404e454e31f8e9d830aa292eb3b456b54fb fuzz: tx_pool checks ATMP result invariants (Greg Sanders) Pull request description: Poached from https://github.com/bitcoin/bitcoin/pull/26711 since that PR is being split apart, and modified to match current behavior. ACKs for top commit: glozow: reACK fcb3069fa307942cf7f3edabcda1be96d615c91f, only whitespace changes dergoegge: ACK fcb3069fa307942cf7f3edabcda1be96d615c91f Tree-SHA512: abd687e526d8dfc8d65b3a873ece8ca35fdcbd6b0f7b93da6a723ef4e47cf85612de819e6f2b8631bdf897e1aba27cdd86f89b7bd85fc3356e74be275dcdf8cc
2023-11-03[test] MiniMiner::Linearize and manual constructionglozow
2023-11-03[refactor] unify fee amounts in miniminer_testsglozow
Name {low,med,high}_fee constants for reuse across file.
2023-11-03[MiniMiner] track inclusion order and add Linearize() functionglozow
Sometimes we are just interested in the order in which transactions would be included in a block (we want to "linearize" the transactions). Track and store this information. This doesn't change any of the bump fee calculations.
2023-11-03[test] add case for MiniMiner working with negative fee txnsglozow
2023-11-03[MiniMiner] make target_feerate optionalglozow
Add an option to keep building the template regardless of feerate. We can't just use target_feerate=0 because it's possible for transactions to have negative modified feerates. No behavior change for users that pass in a target_feerate.
2023-11-03[MiniMiner] allow manual construction with non-mempool txnsglozow
This is primarily intended for linearizing a package of transactions prior to submitting them to mempool. Note that, if this ctor is used, bump fees will not be calculated because we haven't instructed MiniMiner which outpoints for which we want bump fees to be calculated.
2023-11-03[refactor] change MiniMinerMempoolEntry ctor to take values, update includesglozow
No behavior change. All we are doing is copying out these values before passing them into the ctor instead of within the ctor. This makes it possible to use the MiniMiner algorithms to analyze transactions that haven't been submitted to the mempool yet. It also iwyu's the mini_miner includes.
2023-11-02Merge bitcoin/bitcoin#28172: refactor: use string_view for passing string ↵Andrew Chow
literals to Parse{Hash,Hex} bb91131d545d986aab81c4bb13676c4520169259 doc: remove out-of-date external link in src/util/strencodings.h (Jon Atack) 7d494a48ddf4248ef3b1753b6e7f2eeab3a8ecb7 refactor: use string_view to pass string literals to Parse{Hash,Hex} (Jon Atack) Pull request description: as `string_view` is optimized to be trivially copiable, whereas the current code creates a `std::string` copy at each call. These utility methods are called by quite a few RPCs and tests, as well as by each other. ``` $ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l 61 ``` Also remove an out-of-date external link. ACKs for top commit: jonatack: Rebased per `git range-diff c9273f6 b94581a bb91131` for an include header from the merge of https://github.com/bitcoin/bitcoin/pull/28230. Should be trivial to re-ACK. maflcko: lgtm ACK bb91131d545d986aab81c4bb13676c4520169259 ns-xvrn: ACK https://github.com/bitcoin/bitcoin/commit/bb91131d545d986aab81c4bb13676c4520169259 achow101: ACK bb91131d545d986aab81c4bb13676c4520169259 brunoerg: crACK bb91131d545d986aab81c4bb13676c4520169259 Tree-SHA512: 9734fe022c9e43fd93c23a917770d332dbbd3132c80a234059714c32faa6469391e59349954749fc86c4ef0b18d5fd99bf8f4b7b82d9f799943799c1253272ae
2023-11-02Merge bitcoin/bitcoin#24097: Replace RecursiveMutex m_cs_banned with Mutex, ↵Andrew Chow
and rename it 37d150d8c5ffcb2bddcd99951a739e97571194c7 refactor: Add more negative `!m_banned_mutex` thread safety annotations (Hennadii Stepanov) 0fb29087080a4e60d7c709ff5edf14e830ef3a69 refactor: replace RecursiveMutex m_banned_mutex with Mutex (w0xlt) 784c316f9cb664c9577cbfed1873bae573efd1b4 scripted-diff: rename m_cs_banned -> m_banned_mutex (w0xlt) 46709c5f27bf6cbc8eba1298b04bd079da2cdded refactor: Get rid of `BanMan::SetBannedSetDirty()` (Hennadii Stepanov) d88c0d8440cf640ef4f2c7a40b8b8b31bfd38f23 refactor: Get rid of `BanMan::BannedSetIsDirty()` (Hennadii Stepanov) Pull request description: This PR is an alternative to bitcoin/bitcoin#24092. Last two commit have been cherry-picked from the latter. ACKs for top commit: maflcko: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 🎾 achow101: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 theStack: Code-review ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 vasild: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 Tree-SHA512: 5e9d40101a09af6e0645a6ede67432ea68631a1b960f9e6af0ad07415ca7718a30fcc1aad5182d1d5265dc54c26aba2008fc9973840255c09adbab8fedf10075
2023-11-02Use CheckPackageMempoolAcceptResult for package evaluation fuzzingGreg Sanders
2023-11-02[test util] CheckPackageMempoolAcceptResult for sanity-checking resultsglozow
2023-11-02Merge bitcoin/bitcoin#21161: Fee estimation: extend bucket ranges consistentlyglozow
a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Fee estimation: extend bucket ranges consistently (Anthony Towns) Pull request description: When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target. Fixes #20725 ACKs for top commit: ismaelsadeeq: Code review ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d glozow: btw what I meant by [this](https://github.com/bitcoin/bitcoin/pull/21161#pullrequestreview-1350258467) was ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d jonatack: Initial ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Tree-SHA512: 0edf4e56717c4ab8d4ab0bc0f1d7ab36a13b99de12f689e55c9142c6b81691367ffd8df2e8260c5e14335310b1a51770c6c22995db31109976239befcb558ef8
2023-11-02Merge bitcoin/bitcoin#28530: tests, bug fix: DisconnectedBlockTransactions ↵glozow
rewrite followups 9b3da70bd06b45482e7211aa95637a72bd115553 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow) b2d04479647af64ad7cf5ebfb6175251b2f6b72e bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v) f4254e209801d6a790b5f0c251c0b32154a4e3cc assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq) 29eb219c1247993378fce06c8f71aab20736c237 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq) 81dfeddea70ae5feeaf79062585c2ff9f33c0ca3 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq) Pull request description: This PR is a follow-up to fix review comments and a bugfix from #28385 The PR - Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes. - Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`. - `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`. - Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`. * When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](https://github.com/bitcoin/bitcoin/blob/6e721c923c87abdb8d99674093d08d8c17bc52c2/src/core_memusage.h#L32) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block. * This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](https://github.com/bitcoin/bitcoin/blob/6e721c923c87abdb8d99674093d08d8c17bc52c2/src/core_memusage.h#L67) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage. * see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452) - Added test for DisconnectedBlockTransactions memory limit. ACKs for top commit: stickies-v: ACK 9b3da70bd06b45482e7211aa95637a72bd115553 - nice work! BrandonOdiwuor: re ACK 9b3da70bd06b45482e7211aa95637a72bd115553 glozow: ACK 9b3da70bd06b45482e7211aa95637a72bd115553 Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
2023-11-01MOVEONLY: CleanupTemporaryCoins into its own functionglozow
Avoid duplicate code. This will be used at the end of every AcceptSubPackage and after PreChecks loop in AcceptPackage.
2023-11-01[test util] CreateValidTransaction multi-in/out, configurable feerate, ↵glozow
signal BIP125 Support the creation of a transaction with multiple specified inputs or outputs. Also accept a target feerate and return the fee paid. Also, signal BIP125 by default - a subsequent commit needs to RBF something. Co-authored-by: Andrew Chow <achow101@gmail.com>
2023-11-01scripted-diff: rename CheckPackage to IsWellFormedPackageglozow
-BEGIN VERIFY SCRIPT- sed -i 's/CheckPackage(/IsWellFormedPackage(/g' $(git grep -l CheckPackage) -END VERIFY SCRIPT-
2023-11-01[refactor] move package checks into helper functionsglozow
This allows IsSorted() and IsConsistent() to be used by themselves. IsSorted() with a precomputed set is used so that we don't create this set multiple times.
2023-11-01refactor: Remove unused circular include dependency from validation.cppMarcoFalke
2023-11-01Merge bitcoin/bitcoin#28729: addrman: log AS only when using asmapfanquake
02a4f1a3859ed7e865641b35ca1bc9ce711e696f addrman: log AS only when using asmap (brunoerg) Pull request description: This PR changes the log to just print the ASN when using asmap, same logic presented in other logs: https://github.com/bitcoin/bitcoin/blob/afa081a39bde77d3959aa35b6e6c75a2fe988d68/src/net_processing.cpp#L3552-L3556 https://github.com/bitcoin/bitcoin/blob/afa081a39bde77d3959aa35b6e6c75a2fe988d68/src/net_processing.cpp#L3598-L3604 ACKs for top commit: naumenkogs: ACK 02a4f1a3859ed7e865641b35ca1bc9ce711e696f mzumsande: Code Review ACK 02a4f1a3859ed7e865641b35ca1bc9ce711e696f Tree-SHA512: adad5904ab163660d47554b32dc2dc3dfdff8dd64b94e5320ad11706381264d1e338654fa8239430eed4ccbebc8f6670698b4278895794055c37fc4bcefe71bc
2023-11-01Merge bitcoin-core/gui#774: Fix crash on selecting "Mask values" in ↵Hennadii Stepanov
transaction view e26e665f9f64a962dd56053be817cc953e714847 gui: fix crash on selecting "Mask values" in transaction view (Sebastian Falbesoner) Pull request description: This PR fixes a crash bug that can be caused with the following steps: - change to the "Transactions" view - right-click on an arbitrary transaction -> "Show transaction details" - close the transaction detail window again - select menu item "Settings" -> "Mask values" The problem is that the list of opened dialogs, tracked in the member variable `m_opened_dialogs` (introduced in https://github.com/bitcoin-core/gui/pull/708, commit 4492de1be11f4131811f504a037342c78f480e20), is only ever appended with newly opened transaction detail dialog pointers, but never removed. This leads to dangling pointers in the list, and if the "Mask values" menu item is selected, a crash is caused in the course of trying to close the opened transaction detail dialogs (see `closeOpenedDialogs()` method). Fix this by removing a pointer of the list if the corresponding widget is destroyed. ACKs for top commit: achow101: ACK e26e665f9f64a962dd56053be817cc953e714847 pablomartin4btc: tACK e26e665f9f64a962dd56053be817cc953e714847 furszy: utACK e26e665f9 hebasto: ACK e26e665f9f64a962dd56053be817cc953e714847, tested on Ubuntu 22.04. Tree-SHA512: 37885c22abae0ab065b4878bae46fd362f41b09609d081fd59e26bb05474f427b98771ee73f5480526afaef04e016c5ba62c956e0e85a57b6a0f44a905b68a83
2023-10-31fuzz: tx_pool checks ATMP result invariantsGreg Sanders
2023-10-31Merge bitcoin/bitcoin#28503: refactor: Remove WithParams serialization ↵fanquake
helper, use SER_PARAMS_OPFUNC 99990194ce26af89308fab5ad0b1c8c26e45f80c Remove WithParams serialization helper (MarcoFalke) ffffb4af83a47979a0ecc84247bc1167abc2fbf6 scripted-diff: Use ser params operator (MarcoFalke) fae9054793ff2a15db1a645cce3df749e0de2f39 test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp (MarcoFalke) Pull request description: Every serialization parameter struct already has the `SER_PARAMS_OPFUNC`, except for one in the tests. For consistency, and to remove verbose code, convert the test to `SER_PARAMS_OPFUNC`, and use it everywhere, then remove the `WithParams` helper. ACKs for top commit: ajtowns: reACK 99990194ce26af89308fab5ad0b1c8c26e45f80c TheCharlatan: Re-ACK 99990194ce26af89308fab5ad0b1c8c26e45f80c Tree-SHA512: be9cae4225a502486fe8d552aaf4b2cd2904a9f73cce9d931c6b7c757594ff1982fcc2c30d00d012cd12b0a9531fd609f8bcd7c94b811e965ac087eb8a3589d3
2023-10-31gui: fix crash on selecting "Mask values" in transaction viewSebastian Falbesoner
This commits fixes a crash bug that can be caused with the following steps: - change to the "Transactions" view - right-click on an arbitrary transaction -> "Show transaction details" - close the transaction detail window again - select "Settings" -> "Mask values" The problem is that the list of opened dialogs, tracked in the member variable `m_opened_dialogs`, is only ever appended with newly opened transaction detail dialog pointers, but never removed. This leads to dangling pointers in the list, and if the "Mask values" menu item is selected, a crash is caused in the course of trying to close the opened transaction detail dialogs (see `closeOpenedDialogs()` method). Fix this by removing the pointer from the list if the corresponding widget is destroyed.
2023-10-30addrman: log AS only when using asmapbrunoerg
2023-10-30Merge bitcoin/bitcoin#28741: refactor: Fix bugprone-string-constructor warningfanquake
fa56067a8f56701cbda95595592e74934af7d1cd refactor: Fix bugprone-string-constructor warning (MarcoFalke) Pull request description: String literals in C++ have a trailing null character, so the current code is fine to rely on that implicitly. However, * the sqlite documentation explicitly mentions the null character * code readers may wonder if the code is intentional * clang-tidy warns about the code via `bugprone-string-constructor` Address the points by putting the null character into the code and enable the clang-tidy `bugprone-string-constructor` check. ACKs for top commit: stickies-v: ACK fa56067a8f56701cbda95595592e74934af7d1cd Tree-SHA512: da519184d792a885a8151ffc44c8da5781f5aaae12ef768a187cc6d9e542ca8952aebc2ec6c1a05f673f29a86ef44902ee96e7b491af7b4705ad38e14624882e
2023-10-30refactor: Remove unused gcc-9 workaround in txrequestMarcoFalke
2023-10-30Bump g++ minimum supported version to 10MarcoFalke
Also, enable -Werror=maybe-uninitialized in ci/test/00_setup_env_native_qt5.sh
2023-10-30refactor: Fix bugprone-string-constructor warningMarcoFalke
2023-10-30Merge bitcoin/bitcoin#28737: doc: Fix bugprone-lambda-function-name errorsfanquake
faa769db5a4c16fd171e9a39c33e245db4e7c134 Fix bugprone-lambda-function-name errors (MarcoFalke) Pull request description: Inside a lambda, `__func__` will evaluate to something like `"operator()"`. Fix this by either removing it, or by using the real name. https://clang.llvm.org/extra/clang-tidy/checks/bugprone/lambda-function-name.html ACKs for top commit: TheCharlatan: ACK faa769db5a4c16fd171e9a39c33e245db4e7c134 darosior: utACK faa769db5a4c16fd171e9a39c33e245db4e7c134 Tree-SHA512: 0b562bd4ebd7f46ca3ebabeee67851ad30bd522fa57e5010e833b163664e51f5df645ff9ca35d22c3479fb27d9267d4e5d0d417d42729bf3ccf80d7944970e4e
2023-10-30Merge bitcoin/bitcoin#28695: net: Sanity check private keys received from ↵fanquake
SAM proxy 5cf4d266d9b1e7bd9394e7581398de5bc540ae99 [test] Test i2p private key constraints (Vasil Dimov) cf70a8d56510a5f07eff0fd773184cae14b2dcc9 [net] Check i2p private key constraints (dergoegge) Pull request description: Not sanity checking can lead to crashes or worse: ``` ==1715589==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6140000055c2 at pc 0x5622ed66e7ad bp 0x7ffee547a2c0 sp 0x7ffee547a2b8 READ of size 2 at 0x6140000055c2 thread T0 (b-test) #0 0x5622ed66e7ac in memcpy include/bits/string_fortified.h:29:10 #1 0x5622ed66e7ac in i2p::sam::Session::MyDestination() const src/i2p.cpp:362:5 #2 0x5622ed662e46 in i2p::sam::Session::CreateIfNotCreatedAlready() src/i2p.cpp:414:40 #3 0x5622ed6619f2 in i2p::sam::Session::Listen(i2p::Connection&) src/i2p.cpp:143:9 ``` ACKs for top commit: maflcko: code lgtm ACK 5cf4d266d9b1e7bd9394e7581398de5bc540ae99 stickies-v: re-ACK 5cf4d266d9b1e7bd9394e7581398de5bc540ae99 vasild: ACK 5cf4d266d9b1e7bd9394e7581398de5bc540ae99 Tree-SHA512: 3de3bd396538fa619de67957b9c8a58011ab911f0f51097c387e730c13908278b7322aa3357051fb245a20b15bef34b0e9fadcb1eff8ad751139d2aa634c78ad
2023-10-30Remove WithParams serialization helperMarcoFalke
2023-10-30[test] Test i2p private key constraintsVasil Dimov
2023-10-29Merge bitcoin/bitcoin#28698: assumeutxo, blockstorage: Prevent core dump on ↵fanquake
invalid hash 811067ca1cbbd4a697791cbe3ecd4bee19fe6193 test: add coverage for snapshot chainstate not matching AssumeUTXO parameters (pablomartin4btc) 4a5be10b928d4ed33d223972537c1cb79163e79c assumeutxo, blockstorage: prevent core dump on invalid hash (pablomartin4btc) Pull request description: While reviewing #27596 (ran `loadtxoutset` in `mainnet` before `m_assumeutxo_data` is empty as [currently](https://github.com/jamesob/bitcoin/blob/434495a8c1496ca23fe35b84499f3daf668d76b8/src/kernel/chainparams.cpp#L175-L177) in master - back to 1b1d711), got a `core dumped`, so it seems there's a potential issue if new releases ever remove snapshot details or a semi-experienced user performs a `loadtxoutset` on a different "customised" binary version (not sure if this is a real use case). ``` 2023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000 node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed. Aborted (core dumped) ``` <details> <summary>This is also happening before IBD is completed (<code>background validation</code> still being performed as it can be seen in rpc <code>getchainstates</code>)</summary> ``` /src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates { "headers": 813097, "chainstates": [ { "blocks": 368249, "bestblockhash": "00000000000000000b7a08224a1cb00d337100ba7a46c03d04b2c2d8964efc37", "difficulty": 52278304845.59168, "verificationprogress": 0.086288278873286, "coins_db_cache_bytes": 7969177, "coins_tip_cache_bytes": 14908338995, "validated": true }, { "blocks": 813097, "bestblockhash": "0000000000000000000270c9fdce7b17db64cca91f90106964b58e33a4d91089", "difficulty": 61030681983175.59, "verificationprogress": 0.999997140098457, "coins_db_cache_bytes": 419430, "coins_tip_cache_bytes": 784649420, "snapshot_blockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054", "validated": false } ] } ``` </details> <details> <summary>Steps to reproduce the core dump error and its output:</summary> 1. Perform a `loadtxoutset` in `mainnet` on compiled `bitcoind` adding the block hash from Sjors's [commit](https://github.com/Sjors/bitcoin/commit/24deb2022b822f22fba9fcbee201e37a83225eb2). 2. Once step 1 finishes, remove the added code from step 1 and compile again or just compile `master` without any changes on top. 3. Run `bitcoind`, soon it'll crash with: ``` 2023-10-18T17:42:52Z [init] init message: Loading block index… 2023-10-18T17:42:52Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures. 2023-10-18T17:42:52Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64 2023-10-18T17:42:52Z [init] Prune configured to target 3000 MiB on disk for block and undo files. 2023-10-18T17:42:52Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading 2023-10-18T17:42:52Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null) 2023-10-18T17:42:52Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index 2023-10-18T17:42:52Z [init] Opened LevelDB successfully 2023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000 node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed. Aborted (core dumped) ``` </details> <details> <summary>After original change, error message output:</summary> ``` 2023-10-20T15:49:12Z [init] init message: Loading block index… 2023-10-20T15:49:12Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures. 2023-10-20T15:49:12Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64 2023-10-20T15:49:12Z [init] Prune configured to target 3000 MiB on disk for block and undo files. 2023-10-20T15:49:12Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading 2023-10-20T15:49:12Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null) 2023-10-20T15:49:12Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index 2023-10-20T15:49:12Z [init] Opened LevelDB successfully 2023-10-20T15:49:12Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000 2023-10-20T15:49:13Z [init] *** Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'. 2023-10-20T15:49:13Z [init] Error: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'. Error: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'. 2023-10-20T15:49:13Z [init] Shutdown requested. Exiting. 2023-10-20T15:49:13Z [init] Shutdown: In progress... 2023-10-20T15:49:13Z [scheduler] scheduler thread exit 2023-10-20T15:49:13Z [shutoff] Flushed fee estimates to fee_estimates.dat. 2023-10-20T15:49:13Z [shutoff] Shutdown: done ``` </details> <details> <summary>Alternative on error handling using <code>return error()</code> instead of <code>return FatalError()</code> used in this PR, which produces a different output and perhaps confusing:</summary> ``` 2023-10-20T21:45:58Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000 2023-10-20T21:45:59Z [init] ERROR: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'. 2023-10-20T21:45:59Z [init] : Error loading block database. Please restart with -reindex or -reindex-chainstate to recover. : Error loading block database. Please restart with -reindex or -reindex-chainstate to recover. 2023-10-20T21:45:59Z [init] Aborted block database rebuild. Exiting. 2023-10-20T21:45:59Z [init] Shutdown: In progress... 2023-10-20T21:45:59Z [scheduler] scheduler thread exit 2023-10-20T21:45:59Z [shutoff] Flushed fee estimates to fee_estimates.dat. 2023-10-20T21:45:59Z [shutoff] Shutdown: done ``` </details> <details> <summary>Current state (including ryanofsky <a href="https://github.com/bitcoin/bitcoin/pull/28698#discussion_r1368635965">suggestion</a>), after code change, error message output:</summary> ``` 2023-10-25T02:29:57Z [init] Using obfuscation key for /home/pablo/.test_utxo_2/regtest/blocks/index: 0000000000000000 2023-10-25T02:29:57Z [init] *** Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'. 2023-10-25T02:29:57Z [init] Error: A fatal internal error occurred, see debug.log for details Error: A fatal internal error occurred, see debug.log for details 2023-10-25T02:29:57Z [init] Shutdown requested. Exiting. 2023-10-25T02:29:57Z [init] Shutdown: In progress... 2023-10-25T02:29:57Z [scheduler] scheduler thread exit 2023-10-25T02:29:57Z [shutoff] Flushed fee estimates to fee_estimates.dat. 2023-10-25T02:29:57Z [shutoff] Shutdown: done ``` </details> ACKs for top commit: naumenkogs: ACK 811067ca1cbbd4a697791cbe3ecd4bee19fe6193 theStack: ACK 811067ca1cbbd4a697791cbe3ecd4bee19fe6193 ryanofsky: Code review ACK 811067ca1cbbd4a697791cbe3ecd4bee19fe6193. Tree-SHA512: cfc137b0a4f638b99fd7dac2c35cc729ef71ae1166a2a8960a91055ec90841cb33aed589834012cfe0e157937e2a76a88d1020ea1df2bc98e1114eb1fc8eaae4
2023-10-27refactor: Add LIFETIMEBOUND to all (w)txid gettersMarcoFalke
Then, use the compiler warnings to create copies only where needed. Also, fix iwyu includes while touching the includes.
2023-10-26Merge bitcoin/bitcoin#27116: doc: clarify that LOCK() internally checks ↵Andrew Chow
whether the mutex is held 91d08889218e06631f43a3dab0bae576aa46e43c sync: unpublish LocksHeld() which is used only in sync.cpp (Vasil Dimov) 3df37e0c78c3d5139c963a74eda56c331355ef72 doc: clarify that LOCK() does AssertLockNotHeld() internally (Vasil Dimov) Pull request description: Constructs like ```cpp AssertLockNotHeld(m); LOCK(m); ``` are equivalent to (almost, modulo some logging differences, see below) ```cpp LOCK(m); ``` for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. Requests to do the former keep coming during review process. `developer-notes.md` explicitly states "Combine annotations in function declarations with run-time asserts in function definitions", but that seems to be too strong or unclear. `LOCK()` is also a run-time assert in this case. Also remove `LocksHeld()` from the public interface in `sync.h` since it is only used in `sync.cpp`. ACKs for top commit: achow101: ACK 91d08889218e06631f43a3dab0bae576aa46e43c hebasto: ACK 91d08889218e06631f43a3dab0bae576aa46e43c, I have reviewed the code and it looks OK. Tree-SHA512: c4b7ef2c0bfeb28d1c4f55f497810f629873137e02f5a92137c02cb1ff603ac76473dcd2171e594491494a5cb87b8c0c803e06b86f190d4acb231791e28e802d
2023-10-26Merge bitcoin/bitcoin#26078: p2p: return `CSubNet` in `LookupSubNet`Andrew Chow
fb3e812277041f239b97b88689a5076796d75b9b p2p: return `CSubNet` in `LookupSubNet` (brunoerg) Pull request description: Analyzing the usage of `LookupSubNet`, noticed that most cases uses check if the subnet is valid by calling `subnet.IsValid()`, and the boolean returned by `LookupSubNet` hasn't been used so much, see: https://github.com/bitcoin/bitcoin/blob/29d540b7ada890dd588c4825d40c27c5e6f20061/src/httpserver.cpp#L172-L174 https://github.com/bitcoin/bitcoin/blob/29d540b7ada890dd588c4825d40c27c5e6f20061/src/net_permissions.cpp#L114-L116 It makes sense to return `CSubNet` instead of `bool`. ACKs for top commit: achow101: ACK fb3e812277041f239b97b88689a5076796d75b9b vasild: ACK fb3e812277041f239b97b88689a5076796d75b9b theStack: Code-review ACK fb3e812277041f239b97b88689a5076796d75b9b stickies-v: Concept ACK, but Approach ~0 (for now). Reviewed the code (fb3e812277041f239b97b88689a5076796d75b9b) and it all looks good to me. Tree-SHA512: ba50d6bd5d58dfdbe1ce1faebd80dd8cf8c92ac53ef33519860b83399afffab482d5658cb6921b849d7a3df6d5cea911412850e08f3f4e27f7af510fbde4b254
2023-10-26Merge bitcoin/bitcoin#28107: util: Type-safe transaction identifiersAndrew Chow
940a49978c70453e1aaf2c4a0bcb382872b844a5 Use type-safe txid types in orphanage (dergoegge) ed70e6501648466b9ca91a39b83775363e9a726d Introduce types for txids & wtxids (dergoegge) cdb14d79e809bf7d1612b21b554a9fcfb2ab1c91 [net processing] Use HasWitness over comparing (w)txids (dergoegge) Pull request description: We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected. This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs. (Only the orphanage is converted to use these types in this PR) ACKs for top commit: achow101: ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 stickies-v: ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 hebasto: ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5, I have reviewed the code and it looks OK. instagibbs: re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 BrandonOdiwuor: re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 glozow: reACK 940a49978c Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
2023-10-26[net] Check i2p private key constraintsdergoegge
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
2023-10-26Merge bitcoin/bitcoin#28728: wallet: [bugfix] Mark CNoDestination and ↵Andrew Chow
PubKeyDestination constructor explicit 1111475b41698260cda0f25a96c051fd18d66129 bugfix: Mark CNoDestination and PubKeyDestination constructor explicit (MarcoFalke) fa5ccc4137fdd14a75a6fc860b8ff6fc455cb55d iwyu: Export prevector.h from script.h (MarcoFalke) Pull request description: It seems confusing to allow any script, even one with a corresponding address, to silently convert to `CNoDestination`. Make the converstion `explicit` in the code, and fix any bugs that were previously introduced. In a follow-up, the class can be renamed, or the documentation can be updated to better reflect what the code does. ACKs for top commit: josibake: ACK https://github.com/bitcoin/bitcoin/commit/1111475b41698260cda0f25a96c051fd18d66129 achow101: ACK 1111475b41698260cda0f25a96c051fd18d66129 furszy: Code review ACK 1111475 Tree-SHA512: d8b5f54d0cd8649a31e227ef164bb13e5b81ee9820f1976fd70c7a0de6841fba72d549c2f63e351c8cdda37dceb4763eca203e1c8ef385f46d9da6f1855c39ec
2023-10-26Fix bugprone-lambda-function-name errorsMarcoFalke
Can be reviewed with --color-moved=dimmed-zebra
2023-10-25bugfix: Mark CNoDestination and PubKeyDestination constructor explicitMarcoFalke
This should fix the bug reported in https://github.com/bitcoin/bitcoin/pull/28246#discussion_r1371640502, which caused the GUI to not detect the destination type of recipients, thus picking the wrong change destination type. Also, add missing lifetimebound attribute to a getter method.
2023-10-25Merge bitcoin-core/gui#771: Avoid error-prone leading whitespace in ↵Hennadii Stepanov
translatable strings 856325fac17465d102da621f1282b6d8ed02f679 lint: Add `lint-qt-translation.py` (Hennadii Stepanov) 294a018bf5106b03af39a2a8cfa4d5f2ebf6912b qt: Avoid error prone leading spaces in translatable strings (Hennadii Stepanov) d8298e7f069f961fc077ceacff2c332d58734688 qt, refactor: Drop superfluous type conversions (Hennadii Stepanov) Pull request description: While working on the GUI translation via Transifex web interface, I found it error-prone to have leading whitespace in translatable strings. This is because it is very easy to unintentionally drop them in translations unnoticed. Fixed all current cases. Added a linter to prevent similar cases in the future. ACKs for top commit: furszy: utACK 856325f Tree-SHA512: b1ca5effb2db6649e1e99382de79acf3a9f81cc9dad434db5623338489e597897e8addd60c1ab3dcc7506ae62753a7a4ad5a41d7a865f8fcdf94348b54baa7e7
2023-10-25Merge bitcoin-core/gui#742: Exit and show error if unrecognized command line ↵Hennadii Stepanov
args are present 51e4dc49f5335b5bae6c14606d1cc653a08a96b1 gui: Show error if unrecognized command line args are present (John Moffett) Pull request description: Fixes https://github.com/bitcoin-core/gui/issues/741 Starting bitcoin-qt with non-hyphen ("-") arguments causes it to silently ignore any later valid options. For instance, invoking `bitcoin-qt -server=1 foo -regtest` on a fresh install will run `mainnet` instead of `regtest`. This change makes the client exit with an error message if any such "loose" arguments are encountered. This mirrors how `bitcoind` handles it: https://github.com/bitcoin/bitcoin/blob/c6287faae4c0e705a9258a340dfcf548906f12af/src/bitcoind.cpp#L127-L132 However, BIP-21 `bitcoin:` payment URIs are still allowed, but only if they're not followed by any additional options. ACKs for top commit: maflcko: lgtm ACK 51e4dc49f5335b5bae6c14606d1cc653a08a96b1 hernanmarino: tested ACK 51e4dc49f5335b5bae6c14606d1cc653a08a96b1 pablomartin4btc: tACK 51e4dc49f5335b5bae6c14606d1cc653a08a96b1 hebasto: ACK 51e4dc49f5335b5bae6c14606d1cc653a08a96b1, I have reviewed the code and it looks OK. Tree-SHA512: 3997a7a9a747314f13e118aee63e8679e00ed832d9c6f115559a4c39c9c4091572207c60e362cb4c19fc8da980d4b0b040050aa70c5ef84a855cb7e3568bbf13
2023-10-25iwyu: Export prevector.h from script.hMarcoFalke
This should cut some include bloat and seems fine to do, because prevector exists primarily to represent scripts. Also, add missing includes to script.h and addresstype.h
2023-10-24assumeutxo, blockstorage: prevent core dump on invalid hashpablomartin4btc
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-10 01:25:30 +0000