aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2023-06-07Merge bitcoin/bitcoin#27810: fuzz: Partially revert #27780fanquake
71200ac390fe5c10f088cbe8053b010b515757b1 [fuzz] Only check duplicate coinbase script when block was valid (dergoegge) Pull request description: Partially revert #27780, because moving the duplicate coinbase check out of the `was_valid` branch leads to non-bug crashes in the fuzz target. For context and further explanation see: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59516 ACKs for top commit: MarcoFalke: nice lgtm ACK 71200ac390fe5c10f088cbe8053b010b515757b1 Tree-SHA512: 8c38e5ff9de6331016b9a0c5e435d007d46186151b04c09085f617bb31627a28ad56678066fe152372a3ad8656f026439e3e2f9ee61d7ef588072aef8124eaa3
2023-06-07Merge bitcoin/bitcoin#27501: mempool / rpc: add getprioritisedtransactions, ↵Andrew Chow
delete a mapDeltas entry when delta==0 67b7fecacd0489809690982c89ba2d0acdca938c [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow) c1061acb9d502cdf8c6996c818d9a8a281cbe40c [functional test] prioritisation is not removed during replacement and expiry (glozow) 0e5874f0b06114d9b077e0ff582915e4f83059e6 [functional test] getprioritisedtransactions RPC (glozow) 99f8046829f699ff2eace266aa8cea1d9f7cb65a [rpc] add getprioritisedtransactions (glozow) 9e9ca36c80013749faaf2aa777d52bd07d9d24ec [mempool] add GetPrioritisedTransactions (glozow) Pull request description: Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`. Motivation / Background - `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted. - Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen. - Since #8448, `mapDeltas` is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart. - Note the removal due to block/conflict is only done when `removeForBlock` is called, i.e. when the block is received. If you load a mempool.dat containing `mapDeltas` with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them. - Related: #4818 and #6464. - There is no way to query the node for not-in-mempool `mapDeltas`. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat. - Calling `prioritisetransaction` with an inverse value does not remove it from `mapDeltas`, it just sets the value to 0. It disappears on a restart (`LoadMempool` checks if delta is 0), but that might not happen for a while. Added together, if a user calls `prioritisetransaction` very regularly and not all those transactions get mined/conflicted, `mapDeltas` might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are. ACKs for top commit: achow101: ACK 67b7fecacd0489809690982c89ba2d0acdca938c theStack: Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c instagibbs: code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c ajtowns: ACK 67b7fecacd0489809690982c89ba2d0acdca938c code review only, some nits Tree-SHA512: 9df48b622ef27f33db1a2748f682bb3f16abe8172fcb7ac3c1a3e1654121ffb9b31aeaad5570c4162261f7e2ff5b5912ddc61a1b8beac0e9f346a86f5952260a
2023-06-05Merge bitcoin/bitcoin#27801: wallet: Add tracing for sqlite statementsfanquake
ff9d961bf38b24f8f931dcf66799cbc468e473df wallet: Add tracing for sqlite statements (Ryan Ofsky) Pull request description: I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options. ACKs for top commit: achow101: ACK ff9d961bf38b24f8f931dcf66799cbc468e473df kevkevinpal: ACK https://github.com/bitcoin/bitcoin/commit/ff9d961bf38b24f8f931dcf66799cbc468e473df theStack: ACK ff9d961bf38b24f8f931dcf66799cbc468e473df Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05
2023-06-03[fuzz] Only check duplicate coinbase script when block was validdergoegge
2023-06-02wallet: Add tracing for sqlite statementsRyan Ofsky
I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.
2023-06-02Merge bitcoin/bitcoin#27790: walletdb: Add PrefixCursorfanquake
ba616b932cb9e9adb7eb9f1826caa62ce422a22d wallet: Add GetPrefixCursor to DatabaseBatch (Andrew Chow) 1d858b055daeea363e0450f327672658548be4c6 walletdb: Handle when database keys are empty (Ryan Ofsky) 84b2f353bbefb9264284e7430863b2fa1d796d38 walletdb: Consistently clear key and value streams before writing (Andrew Chow) Pull request description: Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917 This PR adds a wallet database cursor that gives a view over all of the records beginning with the same prefix. ACKs for top commit: ryanofsky: Code review ACK ba616b932cb9e9adb7eb9f1826caa62ce422a22d. Just suggested changes since last review furszy: ACK ba616b93 Tree-SHA512: 38a61849f108d8003d28c599b1ad0421ac9beb3afe14c02f1253e7b4efc3d4eef483e32647a820fc6636bca3f9efeff9fe062b6b602e0cded69f21f8b26af544
2023-06-02Merge bitcoin/bitcoin#27256: refactor: rpc: Remove unnecessary uses of ↵fanquake
ParseNonRFCJSONValue() and rename it cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687 refactor: rpc: hide and rename ParseNonRFCJSONValue() (stickies-v) 6c8bde6d54d03224709dce54b8ba32b8c3e37ac7 test: move coverage on ParseNonRFCJSONValue() to UniValue::read() (stickies-v) Pull request description: Follow-up to https://github.com/bitcoin/bitcoin/pull/26612#issuecomment-1453623741. As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly: > IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. laanwj pointed this out in https://github.com/bitcoin/bitcoin/issues/9028#issuecomment-257885368 The implementation of `ParseNonRFCJSONValue()` was already [simplified in #26612](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-84c7a7f36362b9724c31e5dec9879b2f81eae0d0addbc9c0933c3558c577de65R259-R263) and [test coverage updated](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-fc0f86b6c3bb23b0e983bcf79d7546d1c9eaa15d6e4d8a7b03b5b85955f585abR292-R312) to ensure behaviour didn't change. To avoid code duplication, we keep the function to throw on invalid input data but rename it to `Parse()` and remove it from the header. The existing test coverage we had on `ParseNonRFCJSONValue()` is moved over to `UniValue::read()`. ACKs for top commit: ryanofsky: Code review ACK cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test Tree-SHA512: 89be959d2353af7ace0c1348ba1600b9ac1d3c7b3cf7f0b59f6e005b7fb9d695ce3e8720e1be3cf77fe7e318a4017c880df108928e7179ec50447583d13bc781
2023-06-02Merge bitcoin/bitcoin#27803: Fuzz: Mitigate timeout in CalculateTotalBumpFeesglozow
5d718f6913219d3ebe8394a17ddee81915e6f0ac Mitigate timeout in CalculateTotalBumpFees (Murch) Pull request description: The slow fuzz seed described in #27799 was just slower than expected, not an endless loop. Ensuring that every anscestor is only processed once speeds up the termination of the graph traversal. Fixes #27799 ACKs for top commit: glozow: ACK 5d718f6913219d3ebe8394a17ddee81915e6f0ac Tree-SHA512: f3c7cd2ef6716332136c75b43f6d54ce920be6f546a11bbf92b1fd65575607c42cc24b319691d86d0db038335636ba12b6387383a184f1589a8d71d1180f194f
2023-06-02Merge bitcoin/bitcoin#27800: streams: Drop confusing DataStream::Serialize ↵fanquake
method and << operator 5cd0717a54ce7a2065b29d90717aa2eb1c5e302d streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky) Pull request description: DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this PR just drops the DataStream::Serialize method. ACKs for top commit: furszy: lgtm ACK 5cd0717a MarcoFalke: lgtm ACK 5cd0717a54ce7a2065b29d90717aa2eb1c5e302d 🌙 Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2
2023-06-01Mitigate timeout in CalculateTotalBumpFeesMurch
The slow fuzz seed described in #27799 was just slower than expected, not an endless loop. Ensuring that every anscestor is only processed once speeds up the termination of the graph traversal. Fixes #27799
2023-06-01Merge bitcoin/bitcoin#26485: RPC: Accept options as named-only parametersAndrew Chow
2cd28e9fef5dd743bcd70025196ee311fcfdcae4 rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky) 95d7de0964620a3f7386a4adc5707559868abf84 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky) 96233146dd31c1d99fd1619be4449944623ef750 RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky) 702b56d2a8ce48bc3b66a2867d09fa11dcf12fc5 RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky) Pull request description: Allow RPC methods which take an `options` parameter (`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), to accept the options as named parameters, without the need for nested JSON objects. This makes it possible to make calls like: ```sh src/bitcoin-cli -named bumpfee txid fee_rate=10 ``` instead of ```sh src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}' ``` RPC help is also updated to show options as top level named arguments instead of as nested objects. <details><summary>diff</summary> <p> ```diff @@ -15,16 +15,17 @@ Arguments: 1. txid (string, required) The txid to be bumped -2. options (json object, optional) +2. options (json object, optional) Options object that can be used to pass named arguments, listed below. + +Named Arguments: - { - "conf_target": n, (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks +conf_target (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks - "fee_rate": amount, (numeric or string, optional, default=not set, fall back to wallet fee estimation) +fee_rate (numeric or string, optional, default=not set, fall back to wallet fee estimation) Specify a fee rate in sat/vB instead of relying on the built-in fee estimator. Must be at least 1.000 sat/vB higher than the current transaction fee rate. WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB. - "replaceable": bool, (boolean, optional, default=true) Whether the new transaction should still be +replaceable (boolean, optional, default=true) Whether the new transaction should still be marked bip-125 replaceable. If true, the sequence numbers in the transaction will be left unchanged from the original. If false, any input sequence numbers in the original transaction that were less than 0xfffffffe will be increased to 0xfffffffe @@ -32,11 +33,10 @@ still be replaceable in practice, for example if it has unconfirmed ancestors which are replaceable). - "estimate_mode": "str", (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive): +estimate_mode (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive): "unset" "economical" "conservative" - } Result: { (json object) ``` </p> </details> **Review suggestion:** To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation. ACKs for top commit: achow101: ACK 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436
2023-06-01wallet: Add GetPrefixCursor to DatabaseBatchAndrew Chow
In order to get records beginning with a prefix, we will need a cursor specifically for that prefix. So add a GetPrefixCursor function and DatabaseCursor classes for dealing with those prefixes. Tested on each supported db engine. 1) Write two different key->value elements to db. 2) Create a new prefix cursor and walk-through every returned element, verifying that it gets parsed properly. 3) Try to move the cursor outside the filtered range: expect failure and flag complete=true. Co-Authored-By: Ryan Ofsky <ryan@ofsky.org> Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
2023-06-01streams: Drop confusing DataStream::Serialize method and << operatorRyan Ofsky
DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this PR just drops the DataStream::Serialize method.
2023-05-31walletdb: Handle when database keys are emptyRyan Ofsky
2023-05-31walletdb: Consistently clear key and value streams before writingAndrew Chow
Before writing data to the output key and value streams, make sure they are cleared.
2023-05-31Merge bitcoin/bitcoin#27720: index: prevent race by calling 'CustomInit' ↵Andrew Chow
prior setting 'synced' flag 3126454dcfa1dd29bb66500d5f2b5261684d6c58 index: prevent race by calling 'CustomInit' prior setting 'synced' flag (furszy) Pull request description: Decoupled from #27607. Fixed a potential race condition in master (not possible so far) that could become an actual issue soon. Where the index's `CustomAppend` method could be called (from `BlockConnected`) before its `CustomInit` method, causing the index to try to update itself before it is initialized. This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events) before calling to the child class init function (`CustomInit`). So, for example, the block filter index could process a block before initialize the next filter position field and end up overwriting the first stored filter. This race was introduced in https://github.com/bitcoin/bitcoin/commit/bef4e405f3de2718dfee279a9abff4daf016da26 from https://github.com/bitcoin/bitcoin/pull/25494. ACKs for top commit: achow101: ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58 mzumsande: Code review ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58 TheCharlatan: Nice, ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58 Tree-SHA512: 7a53fed1d2035cb4c1f331d6ee9f92d499b6cbb618ea534c6440f5a45ff9b3ac4dcff5fb4b88937f45a0be249e3a9c6dc6c3ac77180f12ae25fc56856ba39735
2023-05-31Merge bitcoin/bitcoin#27657: doc: Remove unused NO_BLOOM_VERSION constantfanquake
facbcd3742313625137907276628267ad90eee01 doc: Remove unused NO_BLOOM_VERSION constant (MarcoFalke) Pull request description: This source code is the wrong place to document historic and now irrelevant details. Also, while touching the docs, clarify that the BIP 35 `mempool` message type is currently also guarded by the BIP 111 `NODE_BLOOM` flag, even though BIP 111 does not mention the `mempool` message type. ACKs for top commit: 0xB10C: ACK facbcd3 dergoegge: ACK facbcd3742313625137907276628267ad90eee01 Tree-SHA512: e69cf5cc2a4e6b061e8996bd9afc0c3e3c4e8174c086ecde425e0e9350b918f1c6612ce273ea1722d30e856049b83dada8782e7e96f676ae0112af85b22f868f
2023-05-31Merge bitcoin/bitcoin#27780: fuzz: Avoid timeout in utxo_total_supplyfanquake
fafb4da121b19ba1b7bd173e25651c64d1982fb4 fuzz: Avoid timeout in utxo_total_supply (MarcoFalke) Pull request description: Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1538252773. It can be checked that the fuzz target can still find the CVE, see https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1410594057 with a diff of: ```diff diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index f949655909..6f4cfb5f51 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -39,8 +39,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) // the underlying coins database. std::set<COutPoint> vInOutPoints; for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) ``` Also, fix a nit, see https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1186451948 ACKs for top commit: dergoegge: ACK fafb4da121b19ba1b7bd173e25651c64d1982fb4 Tree-SHA512: a28fe9cd6ebb4c9bed5a5b35be76c1c436a87586c8fc3b3c4c8559a4a77ac08098324370da421d794c99579882c0872b6b29415de47ade6a05a08504a3d494c4
2023-05-30fuzz: fix wallet notifications.cppfurszy
As the fuzzer test requires all blocks to be scanned by the wallet (because it is asserting the wallet balance at the end), we need to ensure that no blocks are skipped by the recently added wallet birth time functionality. This just means setting the chain accumulated time to the maximum value, so the wallet birth time is always below it, and the block is always processed.
2023-05-30Merge bitcoin/bitcoin#26261: p2p: cleanup `LookupIntern`, `Lookup` and ↵Andrew Chow
`LookupHost` 5c832c3820253affc65c0ed490e26e5b0a4d5c9b p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost` (brunoerg) 34bcdfc6a65de906c65edccdd96fe15219081cd2 p2p, refactor: return vector/optional<CService> in `Lookup` (brunoerg) 7799eb125b7a1146f8251be5d26df574236212a9 p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost` (brunoerg) 5c1774a563dcc237a88df69569cd94fe81e908f7 p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern` (brunoerg) Pull request description: Continuation of #26078. To improve readability instead of returning a bool and passing stuff by reference, this PR changes: - `LookupHost` to return `std::vector<CNetAddr>` - `LookupHost` to return `std::optional<CNetAddr>` - `Lookup` to return `std::vector<CService>` - `Lookup` to return `std::optional<CService>`. - `LookupIntern` to return `std::vector<CNetAddr>` As discussed in #26078, it would be better to avoid using `optional` in some cases, but for specific `Lookup` and `LookupHost` functions it's necessary to use `optional` to verify if they were able to catch some data from their overloaded function. ACKs for top commit: achow101: ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b stickies-v: re-ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b - just addressing two nits, no other changes theStack: re-ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b Tree-SHA512: ea346fdc54463999646269bd600cd4a1590ef958001d2f0fc2be608ca51e1b4365efccca76dd4972b023e12fcc6e67d226608b0df7beb901bdeadd19948df840
2023-05-30Merge bitcoin/bitcoin#27666: wallet, bench: Move commonly used functions to ↵fanquake
their own file and fix a bug 7379a54ec416c8c0a029cc41835a23d42cb6d800 bench: Remove incorrect LoadWallet call in WalletBalance (Andrew Chow) 846b2fe67ed76a678770d343153acedadfdacd0b tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h (Andrew Chow) c61d3f02f5122b38ea8bf0029aa9dfbbf38e10d0 tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper (Andrew Chow) Pull request description: I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner. The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`. The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues currently, it ends up causing issues in some PRs and branches that I'm working on. ACKs for top commit: Sjors: utACK 7379a54ec416c8c0a029cc41835a23d42cb6d800 furszy: ACK 7379a54 Tree-SHA512: 47773887a16c69ac7121c699d3446a8c399bd792a6a31714998b7b7a19fea179c6d3b29cb898b04397b2962c1b4120d57009352b8460b8283e188d4cb480c9ba
2023-05-30Merge bitcoin/bitcoin#27774: refactor: Add [[nodiscard]] where ignoring a ↵fanquake
Result return type is an error fa5680b7520c340952239c4e25ebe505d16c7c39 fix includes for touched header files (iwyu) (MarcoFalke) dddde27f6fbcff7cdb31f7138efc5d8363537b03 Add [[nodiscard]] where ignoring a Result return type is an error (MarcoFalke) Pull request description: Only add it for those where it is an error to ignore. Also, fix the gcc compile warning https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1564350880. Also, fix iwyu for touched header files. ACKs for top commit: TheCharlatan: ACK fa5680b7520c340952239c4e25ebe505d16c7c39 stickies-v: ACK fa5680b7520c340952239c4e25ebe505d16c7c39 Tree-SHA512: c3509103bfeae246e2cf565bc561fcd68d8118515bac5431ba5304c3a63c8254b9c4f40e268b6f6d6b79405675c5a960db9b4eb3bdd14aedca333dc1c9e76415
2023-05-30Merge bitcoin/bitcoin#27636: kernel: Remove util/system from kernel library, ↵fanquake
interface_ui from validation. 7d3b35004b039f2bd606bb46a540de7babdbc41e refactor: Move system from util to common library (TheCharlatan) 7eee356c0a7fefd70c8de21689efa335f52a69ba refactor: Split util::AnyPtr into its own file (TheCharlatan) 44de325d95447498036479c3112ba741caf45bf6 refactor: Split util::insert into its own file (TheCharlatan) 9ec5da36b62276ae22e348f26f88aaf646357d6d refactor: Move ScheduleBatchPriority to its own file (TheCharlatan) f871c69191dfe1331861ebcdbadb6bd47e45c8b1 kernel: Add warning method to notifications (TheCharlatan) 4452707edec91c7d7991f486dd41ef3edb4f7fbf kernel: Add progress method to notifications (TheCharlatan) 84d71457e7250ab25c0a11d1ad1c7657197ffd90 kernel: Add headerTip method to notifications (TheCharlatan) 447761c8228d58f948aae7e73ed079c028cacb97 kernel: Add notification interface (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". --- It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this final step: https://github.com/bitcoin/bitcoin/pull/27419 https://github.com/bitcoin/bitcoin/pull/27254 https://github.com/bitcoin/bitcoin/pull/27238. `interface_ui` defines functions for a more general node interface and has a dependency on `boost/signals2`. After applying the patches from this pull request, the kernel's reliance on boost is down to `boost::multiindex`. The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated. ACKs for top commit: MarcoFalke: re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e (no change) 🎋 stickies-v: Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e hebasto: re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e, only last two commits dropped since my [recent](https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435394620) review. Tree-SHA512: c8cfc698dc9d78e20191c444708f2d957501229abe95e5806106d1126fb9c5fbcee686fb55645658c0107ce71f10646f37a2fdf7fde16bbf22cbf1ac885dd08d
2023-05-30fuzz: Avoid timeout in utxo_total_supplyMarcoFalke
2023-05-30Merge bitcoin/bitcoin#27673: log: don't log total disk read time in ↵fanquake
ConnectTip bench bc862fad294fdb3e4232734497d0693a0da4d63a ConnectTip: don't log total disk read time in bench (Sjors Provoost) Pull request description: The " Load block from disk" log introduced in #24216 incorrectly assumed `num_blocks_total` would be greater than 0. This is not guaranteed until the `ConnectBlock` call right below it. The total and average metric is not very useful because it does not distinguish between blocks read from disk and those loaded from memory. So rather than fixing the divide by zero issue, we just drop the metric. Fixes #27635 ACKs for top commit: MarcoFalke: lgtm ACK bc862fad294fdb3e4232734497d0693a0da4d63a 🐓 willcl-ark: tACK bc862fad29 Tree-SHA512: ff52ff8a8a93f1c82071ca84c57ce5839e14271943393deac0aa5555d63383708777ed96e7226be6dd71b63ed07dc27bad1634ee848e88e4d0b95d511a8267e7
2023-05-29Merge bitcoin/bitcoin#27759: Fix `#include`s in `src/wallet`fanquake
1f97572b9c0d339a8497340e7066050aba9d7694 Fix `#include`s in `src/wallet` (Hennadii Stepanov) Pull request description: This PR is a minimum required changes to fix https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290. ACKs for top commit: MarcoFalke: lgtm ACK 1f97572b9c0d339a8497340e7066050aba9d7694 Tree-SHA512: de885210076d23f3394c42ca50e6ae2470c0ae6523399a2fa3ebb7c06383bdacef9c26166fa19747200396bed796c8772165e24416eb30ed8edd024e3394b2fe
2023-05-29fix includes for touched header files (iwyu)MarcoFalke
2023-05-29Add [[nodiscard]] where ignoring a Result return type is an errorMarcoFalke
2023-05-29Merge bitcoin/bitcoin#27765: test: Throw error when -signetchallenge is non-hexfanquake
fa6b11a55663e70369bfbbba5fccc55b33f2b310 test: Throw error when -signetchallenge is non-hex (MarcoFalke) Pull request description: Instead of silently parsing non-hex to an empty challenge, throw an error. Also, add missing includes while touching the file. ACKs for top commit: kevkevinpal: ACK [fa6b11a](https://github.com/bitcoin/bitcoin/pull/27765/commits/fa6b11a55663e70369bfbbba5fccc55b33f2b310) kallewoof: ACK fa6b11a TheCharlatan: Nice, ACK fa6b11a55663e70369bfbbba5fccc55b33f2b310 Tree-SHA512: 018ebbbf819ba7cdf0c6dd294fdfaa5ddb81b87058a8b9c57b96066d5b07e1656fd78f18e3cef375aebefa191fa515c2c70bc764880fa05f98f526334431a616
2023-05-27Merge bitcoin/bitcoin#27145: wallet: when a block is disconnected, update ↵Andrew Chow
transactions that are no longer conflicted 89df7987c2f1eea42454c2b0efc31a924fbfd3a8 Add wallets_conflicts (Antoine Riard) dced203162d45e542f187f8d0d07dab85c52cf28 wallet, tests: mark unconflicted txs as inactive (ishaanam) 096487c4dcfadebe5ca959927f5426cae1c304d5 wallet: introduce generic recursive tx state updating function (ishaanam) Pull request description: This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated. Second attempt at #17543 ACKs for top commit: achow101: ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8 rajarshimaitra: tACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8. glozow: ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8 furszy: Tested ACK 89df7987 Tree-SHA512: 3133b151477e8818302fac29e96de30cd64c09a8fe3a7612074a34ba1a332e69148162e6cb3f1074d9d9c9bab5ef9996967d325d8c4c99ba42b5fe3b66a60546
2023-05-27Merge bitcoin/bitcoin#27766: fuzz: Change LIMIT_TO_MESSAGE_TYPE from a ↵fanquake
compile-time to a run-time setting 1111c9ac97ca0f0afeb5df45bc0970b761c3c9ff fuzz: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time setting (MarcoFalke) Pull request description: The `process_message_${msg_type}` fuzz targets have many issues: * In a context where each fuzz target must be a separate binary, this bloats the storage requirements by the number of message types. * The qa-assets repo for fuzz inputs also bloats, because each input in the type specific folder (`./process_message_${msg_type}`) is accompanied by a similar input in the general folder (`./process_message`) or a in another specific folder. The size seems to be ~3GB for the sum of all folders vs 0.3GB for the general folder. * Handling of different folders for each message type and one general folder for all message types (and unknown message types) is undocumented and unclear. Cross-pollination is encouraged, I guess, but who does it? * It is unclear if the fuzz target has any value at all, given that any bug that is found here should also be found by the `process_messages` fuzz target, and historically always has been? So maybe it can even be removed completely in the future? * (minor nit): When adding a new message type, the message type has to be added to this fuzz target as well. Fix all issues by turning the compile-time setting into a run-time setting, thus removing the extra executables and fuzz folders. The same approach is also taken by the `rpc` fuzz target. If someone wants to limit their fuzzing to a specific message type, they can still do it. For example, ``` LIMIT_TO_MESSAGE_TYPE=inv FUZZ=process_message ./src/test/fuzz/fuzz ACKs for top commit: dergoegge: ACK 1111c9ac97ca0f0afeb5df45bc0970b761c3c9ff Tree-SHA512: 9495538d9bc83b24671a44e9311a4e82ce5b2fa89e431e42772dcfa19f675a9fe2dd8cd3d3b15b154c8b7f400e57ee08a976d37e55a5425778ced0ee85fb984c
2023-05-26Merge bitcoin/bitcoin#27469: wallet: improve IBD sync time by skipping block ↵Andrew Chow
scanning prior birth time 82bb7831fa6052620998c7eef47e48ed594248a8 wallet: skip block scan if block was created before wallet birthday (furszy) a082434d122754ec1a10e0e08a35bdb1f47989e6 refactor: single method to append new spkm to the wallet (furszy) Pull request description: During initial block download, the node's wallet(s) scans every arriving block looking for data that it owns. This process can be resource-intensive, as it involves sequentially scanning all transactions within each arriving block. To avoid wasting processing power, we can skip blocks that occurred before the wallet's creation time, since these blocks are guaranteed not to contain any relevant wallet data. This has direct implications (an speed improvement) on the underlying blockchain synchronization process as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain (activating the best chain to be more precise). Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are processed by the wallet(s). So, by skipping not relevant blocks in the wallet's IBD scanning process, we will also improve the chain synchronization time. ACKs for top commit: ishaanam: re-ACK 82bb7831fa6052620998c7eef47e48ed594248a8 achow101: re-ACK 82bb7831fa6052620998c7eef47e48ed594248a8 pinheadmz: ACK 82bb7831fa6052620998c7eef47e48ed594248a8 Tree-SHA512: 70158c9657f1fcc396badad2c4410b7b7f439466142640b31a9b1a8cea4555e45ea254e48043c9b27f783d5e4d24d91855f0d79d42f0484b8aa83cdbf3d6c50b
2023-05-26p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`brunoerg
2023-05-26p2p, refactor: return vector/optional<CService> in `Lookup`brunoerg
2023-05-26p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`brunoerg
2023-05-26p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern`brunoerg
2023-05-26Merge bitcoin/bitcoin#27761: p2p: Log addresses of stalling peersfanquake
fb02a3cd1a105bdf60ca39e1858e77685be88976 p2p: Log addresses of stalling peers (Martin Zumsande) Pull request description: This was suggested in #27705 by ArmchairCryptologist. It allows node operators that have the `-logips` option enabled to better identify potentially misbehaving peers and maybe ban them. This is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their `nodeId` to an address unless the very noisy `net` debugging is enabled. In case of outbound peers for which the address is potentially logged when establishing the connection, this just adds some convenience. ACKs for top commit: stratospher: tACK fb02a3c. jamesob: github ACK https://github.com/bitcoin/bitcoin/commit/fb02a3cd1a105bdf60ca39e1858e77685be88976 0xB10C: Untested ACK fb02a3cd1a105bdf60ca39e1858e77685be88976 instagibbs: utACK fb02a3cd1a105bdf60ca39e1858e77685be88976 Tree-SHA512: 2080f794c715bd36143405828b4b0e1be859095caf8f8a0c20dd2a4b64d192d78fee0fa350a2bb7c39848718332c4dd4d8edb2cc8d22095b65afe710591f7ccb
2023-05-26fuzz: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time settingMarcoFalke
2023-05-26Merge bitcoin/bitcoin#27302: init: Error if ignored bitcoin.conf file is foundfanquake
eefe56967b4eb4b5144325cde4f40fc1cbde3e65 bugfix: Fix incorrect debug.log config file path (Ryan Ofsky) 3746f00be1b732a04976fc70cbb0661f97bbbd99 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky) 398c3719b02197ad92fded20f6ff83b364747297 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky) Pull request description: Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen: - One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning. - Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored. This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored. ACKs for top commit: pinheadmz: re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65 willcl-ark: re-ACK eefe56967b TheCharlatan: ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65 Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
2023-05-26Merge bitcoin/bitcoin#25977: refactor: Replace ↵fanquake
`std::optional<bilingual_str>` with `util::Result` 8aa8f73adce72e1ae855b43413c1f65504423cb7 refactor: Replace std::optional<bilingual_str> with util::Result (Ryan Ofsky) 5f49cb1bc8e6ba0671c21bf6292d2d3de43fd001 util: Add void support to util::Result (MarcoFalke) Pull request description: Replace uses of `std::optional<bilingual_str>` with `util::Result` as suggested https://github.com/bitcoin/bitcoin/pull/25648#discussion_r936311768, https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192007516, https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194858242, https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1204047087 ACKs for top commit: MarcoFalke: very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏 TheCharlatan: ACK 8aa8f73adce72e1ae855b43413c1f65504423cb7 hebasto: ACK 8aa8f73adce72e1ae855b43413c1f65504423cb7, I have reviewed the code and it looks OK. Tree-SHA512: 6c2f218bc445184ce93ec2b907e61666a05f26f2c9447f69fcb504aa8291b0c693d913f659dfd19813a9e24615546c72cbe2ca419218fd867ff0694c4a1b6a30
2023-05-26p2p: Log addresses of stalling peersMartin Zumsande
This allows node operators that have the -logips option enabled to better identify potentially misbehaving peers and maybe ban them.
2023-05-25bench: Remove incorrect LoadWallet call in WalletBalanceAndrew Chow
The WalletBalance benchmarks would incorrectly call LoadWallet after the wallet has been setup. LoadWallet expects to be the first thing that is called and for the CWallet to be in a fresh state. When it is not, it results in bogus pointers which can cause segfaults during this benchmark.
2023-05-25tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.hAndrew Chow
This static address is usable for other wallet tests and benchmarks, so make it available to them.
2023-05-25tests, bench: Consolidate {Test,Bench}Un/LoadWallet helperAndrew Chow
The wallet tests and benchmarks both had helper functions for loading and unloading the wallet for the test that were almost identical. These functions are consolidated and reused.
2023-05-25test: Throw error when -signetchallenge is non-hexMarcoFalke
2023-05-25Fix `#include`s in `src/wallet`Hennadii Stepanov
2023-05-25wallet: skip block scan if block was created before wallet birthdayfurszy
To avoid wasting processing power, we can skip blocks that occurred before the wallet's creation time, since these blocks are guaranteed not to contain any relevant wallet data. This has direct implications (an speed improvement) on the underlying blockchain synchronization process as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain (activating the best chain to be more precise). Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are processed by the wallet(s).
2023-05-25refactor: single method to append new spkm to the walletfurszy
2023-05-25Merge bitcoin/bitcoin#27747: rpc: Use 'byte'/'bytes' for bech32(m) ↵fanquake
validation error message 3d0a5c37e9ccedfa4ecfaa48eeeca1ada5b4eec1 use 'byte'/'bytes' for bech32(m) validation error (Reese Russell) Pull request description: This PR rectifies a linguistic inconsistency found in merged PR [27727](https://github.com/bitcoin/bitcoin/pull/27727). It addresses the improper usage of the term 'byte' in error reports. As it stands, PR [27727](https://github.com/bitcoin/bitcoin/pull/27727) exclusively utilizes 'byte' in error messages, regardless of the context, as demonstrated below: Currently: ```Invalid Bech32 v0 address program size (16 byte), per BIP141``` This modification enhances the accuracy of error reporting in most scenarios users are likely to encounter by checking for a plural or singular number of bytes. This PR **16 Bytes program size error** : ``` ( "BC1QR508D6QEJXTDG4Y5R3ZARVARYV98GJ9P", "Invalid Bech32 v0 address program size (16 bytes), per BIP141", [], ) ``` **1 Byte program size error** ``` ( "bc1pw5dgrnzv", "Invalid Bech32 address program size (1 byte)", [] ), ``` Thank you ACKs for top commit: MarcoFalke: lgtm ACK 3d0a5c37e9ccedfa4ecfaa48eeeca1ada5b4eec1 Tree-SHA512: 55069194a6a33a37559cf14b59b6ac05b1160f57f14d1415aef8e76c916c7c7f343310916ae85b3fa895937802449c1dddb2f653340d0f39203f06aee10f6fce
2023-05-25use 'byte'/'bytes' for bech32(m) validation errorReese Russell
changed from std::string -> std::string_view applied snake case to byteStr -> byte_str
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-09 20:13:15 +0000