aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2022-12-12net: remove CNetAddr::ToString() and use ToStringAddr() insteadVasil Dimov
Both methods do the same thing, so simplify to having just one. Further, `CService` inherits `CNetAddr` and `CService::ToString()` overrides `CNetAddr::ToString()` but the latter is not virtual which may be confusing. Avoid such a confusion by not having non-virtual methods with the same names in inheritance.
2022-12-12scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]()Vasil Dimov
"IP" stands for "Internet Protocol". "IP address" is sometimes shortened to just "IP" or "address". However, Tor or I2P addresses are not "IP addresses", nor "IPs". Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or I2P addresses: `CService::ToStringIPPort()` -> `CService::ToStringAddrPort()` `CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()` -BEGIN VERIFY SCRIPT- sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src) sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src) -END VERIFY SCRIPT-
2022-12-12Merge bitcoin/bitcoin#26199: p2p: Don't self-advertise during version processingMarcoFalke
956c67059caf3807b3ceacdd5de1caaae538f009 refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande) 3c43d9db1e0f84279b08a93afd730caeece061a9 p2p: Don't self-advertise during VERSION processing (Gleb Naumenko) Pull request description: This picks up the last commit from #19843. Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer. This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after the version handshake is finished. There are a couple of differences: 1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones. 2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable. 3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`. Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`. ACKs for top commit: stratospher: ACK 956c670 naumenkogs: ACK 956c67059caf3807b3ceacdd5de1caaae538f009 amitiuttarwar: reACK 956c67059c Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
2022-12-10Merge bitcoin/bitcoin#26666: refactor: Deleted unreachable code in ↵MarcoFalke
httpserver.cpp 8f5c560e11a9f3c7c5877f444caa605bdc296e81 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos) 7fd3b9491b0e553aa7d60a68bda82968a79da8f5 refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos) Pull request description: Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes. Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570) ACKs for top commit: stickies-v: re-ACK [8f5c560](https://github.com/bitcoin/bitcoin/commit/8f5c560e11a9f3c7c5877f444caa605bdc296e81) Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
2022-12-10Merge bitcoin/bitcoin#26673: univalue: Remove confusing getBool methodfanquake
293849a26088a16f6187dfdc36bef1d4d83ebb9d univalue: Remove confusing getBool method (Ryan Ofsky) Pull request description: Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception. The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate. These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR. ACKs for top commit: justinpickering: ACK https://github.com/bitcoin/bitcoin/pull/26673/commits/293849a26088a16f6187dfdc36bef1d4d83ebb9d sipa: utACK 293849a26088a16f6187dfdc36bef1d4d83ebb9d w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/26673/commits/293849a26088a16f6187dfdc36bef1d4d83ebb9d hebasto: ACK 293849a26088a16f6187dfdc36bef1d4d83ebb9d, also verified that the removed `getBool` method is not mentioned in any docs: furszy: ACK 293849a2 Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
2022-12-10Merge bitcoin/bitcoin#26213: rpc: Strict type checking for RPC boolean ↵fanquake
parameters fa0153e609caf61a59efb0779e754861edc1684d refactor: Replace isTrue with get_bool (MarcoFalke) fa2cc5d1d66aa00e828d1bb65b9923f76fbdf4e1 bugfix: Strict type checking for RPC boolean parameters (MarcoFalke) Pull request description: ACKs for top commit: ryanofsky: Code review ACK fa0153e609caf61a59efb0779e754861edc1684d furszy: Code ACK fa0153e6 Tree-SHA512: b221f823c69d90c94447fd491071ff3659cfd512872b495ebc3e711f50633351974102c9ef7e50fa4a393c4131d349adea8fd41cc9d66f1f31e1f5e7a5f78757
2022-12-09refactor: Refactored RequestMethodString function to follow developer notesJoaoAJMatos
Removed the default case in the switch statement in order to comply with the Developer Notes
2022-12-09refactor: Deleted unreachable code in httpserver.cppJoaoAJMatos
Removed all break statements from both RequestMethodString and GetRequestMethod functions as they were unreachable
2022-12-09univalue: Remove confusing getBool methodRyan Ofsky
Drop UniValue::getBool method because it is easy to confuse with the UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool, getBool doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exceptions. The getBool method is also redundant because it is an alias for isTrue. There were only 5 getBool() calls in the codebase, so this commit replaces them with isTrue() or get_bool() calls as appropriate. These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
2022-12-08Merge bitcoin/bitcoin#26378: refactor: Pass reference to last header, not ↵MarcoFalke
pointer fa579f306363ed03c1138121415b67b9b36b4d53 refactor: Pass reference to last header, not pointer (MacroFake) Pull request description: It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders. Passing a reference makes the code easier to read and less brittle. ACKs for top commit: john-moffett: ACK fa579f3 aureleoules: ACK fa579f306363ed03c1138121415b67b9b36b4d53 Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
2022-12-08Merge bitcoin/bitcoin#26513: Make static nLastFlush and nLastWrite ↵fanquake
Chainstate members 07dfbb5bb8115c680621f361c65d9cde2f8c52f2 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès) Pull request description: Fixes #22189. The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables. ACKs for top commit: jamesob: ACK 07dfbb5bb8115c680621f361c65d9cde2f8c52f2 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a)) theStack: Concept and code-review ACK 07dfbb5bb8115c680621f361c65d9cde2f8c52f2 Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
2022-12-08Merge bitcoin/bitcoin#26308: rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 ↵MarcoFalke
times as many requests per second d7f61e7d5909da7227c9e34be06ce9eb872ba074 rpc: reduce LOCK(cs_main) scope in gettxoutproof (Andrew Toth) 4d92b5aabaf371225cdc37a7b114adc040bf4da5 rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstats (Andrew Toth) efd82aec8a2dd0fca8f2597c3f84cefe057d1243 rpc: reduce LOCK(cs_main) scope in blockToJSON (Andrew Toth) f00808e932c1f67bc76702a3f05778074b10025c rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblock (Andrew Toth) 7d253c943f44612431be894b198ffb49ff988fff zmq: remove LOCK(cs_main) from NotifyBlock (Andrew Toth) c75e3d2772b00acc3850f72a8cb733a0345aa773 rest: reduce LOCK(cs_main) scope in rest_block (Andrew Toth) Pull request description: Picking up from #21006. After commit https://github.com/bitcoin/bitcoin/commit/ccd8ef65f93ed82a87cee634660bed3ac17d9eb5 it is no longer required to hold `cs_main` when calling `ReadBlockFromDisk`. This can be verified in `master` at https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L755. Same can be seen for `UndoReadFromDisk` https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L485. The first commit moves `ReadBlockFromDisk` outside the lock scope in `rest_block`, where we can see a huge performance improvement when fetching blocks with multiple threads. My test setup, on an Intel i7 with 8 cores (16 threads): 1. Start a fully synced bitcoind, with this `bitcoin.conf`: ``` rest=1 rpcthreads=16 rpcworkqueue=64 rpcuser=user rpcpassword=password ``` 2. Run ApacheBench: 10000 requests, 16 parallel threads, fetching block nr. 750000 in binary: ``` ab -n 10000 -c 16 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin" ``` Time per request (mean) 183 ms on master 30 ms this branch So this can process 6.1 times as many requests, and saturates all the cores instead of keeping them partly idle waiting in the lock. With 8 threads the mean times were 90 ms on master and 19 ms on this branch, a speedup of 4.7x. Big thanks to martinus for finding this and the original PR. The second commit is from a suggestion on the original PR by jonatack to remove the unnecessary `LOCK(cs_main)` in the zmq notifier's `NotifyBlock`. I also found that this approach could be applied to rpcs `getblock` (including `verbosity=3`), `getblockstats`, and `gettxoutproof` with similar very good results. The above benchmarks steps need to be modified slightly for RPC. Run the following ApacheBench command with different request data in a file named `data.json`: ``` ab -p data.json -n 10000 -c 16 -A user:password "http://127.0.0.1:8332/" ``` For `getblock`, use the following in `data.json`: ``` {"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e"]} ``` master - 184 ms mean request time branch - 28 ms mean request time For `getblock` with verbosity level 3, use the following in `data.json`: ``` {"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 3]} ``` This verbosity level fetches an undo file from disk, so it benefits from this approach as well. However, a lot of time is spent serializing to JSON so the performance gain is not as severe. master - 818 ms mean request time branch - 505 ms mean request time For `getblockstats`, use the following in `data.json`: ``` {"jsonrpc": "1.0", "id": "curltest", "method": "getblockstats", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", ["minfeerate","avgfeerate"]]} ``` This request used a lock on reading both a block and undo file, so the results are very good. master - 244 ms mean request time branch - 28 ms mean request time ACKs for top commit: MarcoFalke: re-ACK d7f61e7d5909da7227c9e34be06ce9eb872ba074 💫 hebasto: ACK d7f61e7d5909da7227c9e34be06ce9eb872ba074, I have reviewed the code and it looks OK. Did not make benchmarking though. Tree-SHA512: 305ac945b4571c5f47646d4f0e78180d7a3d40b2f70ee43e4b3e00c96a465f6d0b9c750b8e85c89ed833e557e2cdb5896743f07ef90e4e53d4ad85452b545886
2022-12-07Merge bitcoin/bitcoin#25934: wallet, rpc: add `label` to `listsinceblock`Andrew Chow
4e362c2b7269ae0426010850c605e5c1d0d58234 doc: add release note for 25934 (brunoerg) fe488b4c4b7aa07fb83d528e2942ef914fd188c0 test: add coverage for `label` in `listsinceblock` (brunoerg) 722e9a418d078ed34aedd1ca55c1ae104f29a7d3 wallet, rpc: add `label` to `listsinceblock` (brunoerg) 852891ff98cffd37a74b9cb96394f43b2e6ca30e refactor, wallet: use optional for `label` in `ListTransactions` (brunoerg) Pull request description: This PR adds `label` parameter to `listsinceblock` to be able to fetch all incoming transactions having the specified label since a specific block. It's possible to use it in `listtransactions`, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. `getreceivedbylabel` only returns the total amount received, not the txs info. `listreceivedbylabel` doesn't list all the informations about the transactions and it's not possible to fetch since a block. ACKs for top commit: achow101: ACK 4e362c2b7269ae0426010850c605e5c1d0d58234 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25934/commits/4e362c2b7269ae0426010850c605e5c1d0d58234 aureleoules: ACK 4e362c2b7269ae0426010850c605e5c1d0d58234 Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737
2022-12-07refactor: Replace isTrue with get_boolMarcoFalke
This makes the code more robust, see previous commit. In general replacing isTrue with get_bool is not equivalent because get_bool can throw exceptions, but in this case, exceptions won't happen because of RPCTypeCheck() and isNull() checks in the preceding code.
2022-12-07bugfix: Strict type checking for RPC boolean parametersMarcoFalke
2022-12-07Merge bitcoin/bitcoin#26298: refactor: Move src/interfaces/*.cpp files to ↵fanquake
libbitcoin_common.a b19c4124b3c9a15fe3baf8792c55eb26eca51c0f refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky) dd6e8bd71c6025a51d88000caf28121ec00499db build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake) 82e272a109281f750909d1feade784c778d8b592 refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a (Ryan Ofsky) Pull request description: These belong in `libbitcoin_common.a`, not `libbitcoin_util.a`, because they aren't general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in `libbitcoin_util.a` is to prevent them from being used by the kernel library. Also rename ambiguous `MakeHandler` functions to `MakeCleanupHandler` and `MakeSignalHandler`. Cleanup function handler was introduced after boost signals handler, so original naming didn't make much sense. This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes. This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the _util_ library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase. ACKs for top commit: hebasto: ACK b19c4124b3c9a15fe3baf8792c55eb26eca51c0f Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
2022-12-07Merge bitcoin/bitcoin#26645: util: Include full version id in bug reportsMarcoFalke
fa825bd227b9d2bace896a2d29b5ce78bbd4e59c util: Include full version id in bug reports (MarcoFalke) Pull request description: This will show the unique id of the full source code when the bug occurred, which can help debugging ACKs for top commit: 1440000bytes: utACK https://github.com/bitcoin/bitcoin/commit/fa825bd227b9d2bace896a2d29b5ce78bbd4e59c theStack: ACK fa825bd227b9d2bace896a2d29b5ce78bbd4e59c john-moffett: ACK fa825bd227b9d2bace896a2d29b5ce78bbd4e59c Tree-SHA512: a7a775718f5f9796b5cffafbb3ace8adb5c163414ec584a57143157fc9dfb86f799e3b9c8365fcb831ee1e9eafc59d699d1653d772c68392de421b3de74dcd61
2022-12-06rpc: reduce LOCK(cs_main) scope in gettxoutproofAndrew Toth
2022-12-06rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstatsAndrew Toth
2022-12-06rpc: reduce LOCK(cs_main) scope in blockToJSON Andrew Toth
2022-12-06rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblockAndrew Toth
2022-12-06zmq: remove LOCK(cs_main) from NotifyBlockAndrew Toth
2022-12-06rest: reduce LOCK(cs_main) scope in rest_blockAndrew Toth
2022-12-06Merge bitcoin-core/gui#683: doc: Drop no longer relevant commentHennadii Stepanov
5d332da2cfdc67d69a165119c6ff598e29b28afa doc: Drop no longer relevant comment (Hennadii Stepanov) Pull request description: The comment was introduced in 4cf3411056f6a59fc5fe07784b6b6a512d76b046, and since 7e4bd19785ff9120b7242ff9f15231868aaf7db4 it has been no longer relevant. ACKs for top commit: jarolrod: ACK 5d332da2cfdc67d69a165119c6ff598e29b28afa Tree-SHA512: 6d32561336993b1ff7d7c524d090ac52aefb40078ed706ca4c6d5026cc3f63244c49c0e00e45ff192ba0e9f1527faf63249aa18bc8aa677b9e053d387e0f4027
2022-12-06wallet, rpc: add `label` to `listsinceblock`brunoerg
2022-12-06refactor, wallet: use optional for `label` in `ListTransactions`brunoerg
2022-12-06Merge bitcoin/bitcoin#26609: refactor: Move `txmempool_entry.h` --> ↵MarcoFalke
`kernel/mempool_entry.h` 38941a703e079709bb465ad1fcde50e11350f8f1 refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov) Pull request description: This PR addresses the https://github.com/bitcoin/bitcoin/pull/17786#discussion_r1027818360: > why not move it to the right place, that is to `kernel/txmempool_entry.h`? ACKs for top commit: MarcoFalke: review ACK 38941a703e079709bb465ad1fcde50e11350f8f1 📊 Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
2022-12-06Merge bitcoin/bitcoin#25729: wallet: Check max transaction weight in ↵Andrew Chow
CoinSelection c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 test: Check max transaction weight in CoinSelection (Aurèle Oulès) 6b563cae92957dc30dc35103a7c321fdb0115ef3 wallet: Check max tx weight in coin selector (Aurèle Oulès) Pull request description: This PR is an attempt to fix #5782. I have added 4 test scenarios, 3 of them provided here https://github.com/bitcoin/bitcoin/issues/5782#issuecomment-73819058 (slightly modified to use a segwit wallet). Here are my benchmarks : ## PR | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,466,341.00 | 681.97 | 0.6% | 11,176,762.00 | 3,358,752.00 | 3.328 | 1,897,839.00 | 0.3% | 0.02 | `CoinSelection` ## Master | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,526,029.00 | 655.30 | 0.5% | 11,142,188.00 | 3,499,200.00 | 3.184 | 1,994,156.00 | 0.2% | 0.02 | `CoinSelection` ACKs for top commit: achow101: reACK c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25729/commits/c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 furszy: diff ACK c7c7ee9d Tree-SHA512: ef0b28576ff845174651ba494aa9adee234c96e6f886d0e032eceb7050296e45b099dda0039d1dfb9944469f067627b2101f3ff855c70353cf39d1fc7ee81828
2022-12-06Merge bitcoin/bitcoin#26238: clang-tidy: fixup named argument commentsMarcoFalke
203886c443c4ad76f8a1dba740a286e383e55206 Fixup clang-tidy named argument comments (fanquake) Pull request description: Fix comments so they are checked/consistent. Fix incorrect comments. ACKs for top commit: hebasto: ACK 203886c443c4ad76f8a1dba740a286e383e55206, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: e1257840f91fe39842e2b19299c1633604697b8584fe44b1977ada33cdde5433c877ed0b669fa334e20b04971dc89cd47d58b2783b6f7004521f01d05a1245da
2022-12-06util: Include full version id in bug reportsMarcoFalke
2022-12-06Merge bitcoin/bitcoin#26611: wallet: Change coin selection fee assert to errorMarcoFalke
3eb041f014870954db564369a4be4bd0dea48fbe wallet: Change coin selection fee assert to error (Andrew Chow) c6e7f224c119f47af250a9e0c5b185cb98b30c4c util: Add StrFormatInternalBug and STR_INTERNAL_BUG (MarcoFalke) Pull request description: Returning an error instead of asserting for the low fee check will be better as it does not crash the node and instructs users to report the bug. ACKs for top commit: S3RK: ACK 3eb041f014870954db564369a4be4bd0dea48fbe aureleoules: ACK 3eb041f014870954db564369a4be4bd0dea48fbe furszy: ACK 3eb041f0 Tree-SHA512: 118c13d7cdfce492080edd4cb12e6d960695377b978c7573f9c58b6d918664afd0e8e591eed0605d08ac756fa8eceed456349de5f3a025174069abf369bb5a5f
2022-12-05Merge bitcoin/bitcoin#19888: rpc, test: Improve getblockstats for unspendablesAndrew Chow
d885bb2f6ea34cd21dacfebe763a07dbb389c1bd test: Test exclusion of OP_RETURN from getblockstats (Fabian Jahr) ba9d288b2468f047e5a8e4637dd8749e247ff547 test: Fix getblockstats test data generator (Fabian Jahr) 2ca5a496c2f3cbcc63ea15fa05c1658e7f527bbc rpc: Improve getblockstats (Fabian Jahr) cb94db119f4643f49da63520d64efc99fb0c0795 validation, index: Add unspendable coinbase helper functions (Fabian Jahr) Pull request description: Fixes #19885 The genesis block does not have undo data saved to disk so the RPC errored because of that. ACKs for top commit: achow101: ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd aureleoules: ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd stickies-v: ACK d885bb2f6 Tree-SHA512: f37bda736ed605b7a41a81eeb4bfbb5d2b8518f847819e5d6a090548a61caf1455623e15165d72589ab3f4478252b00e7b624f9313ad6708cac06dd5edb62e9a
2022-12-05Merge bitcoin/bitcoin#26462: wallet: fix crash on loading descriptor wallet ↵Andrew Chow
containing legacy key type entries 3198e4239e848bbb119e3638677aa9bcf8353ca6 test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner) 349ed2a0eed3aaaf199ead93057c97730869c3a3 wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner) Pull request description: Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details): ``` $ ./src/bitcoin-cli createwallet crashme $ ./src/bitcoin-cli unloadwallet crashme $ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat SQLite version 3.38.2 2022-03-26 13:51:10 Enter ".help" for usage hints. sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00'); $ ./src/bitcoin-cli loadwallet crashme --- bitcoind output: --- 2022-11-06T13:51:01Z Using SQLite Version 3.38.2 2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme 2022-11-06T13:51:01Z init message: Loading wallet… 2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900 Segmentation fault (core dumped) ``` Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: https://github.com/bitcoin/bitcoin/blob/50422b770a40f5fa964201d1e99fd6b5dc1653ca/src/wallet/walletdb.cpp#L589-L594 ~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~ ~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~ This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading. ACKs for top commit: achow101: ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6 aureleoules: ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6 Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
2022-12-05test: Check max transaction weight in CoinSelectionAurèle Oulès
Co-authored-by: Andrew Chow <github@achow101.com>
2022-12-05wallet: Check max tx weight in coin selectorAurèle Oulès
Co-authored-by: Andrew Chow <github@achow101.com>
2022-12-05wallet: Change coin selection fee assert to errorAndrew Chow
Returning an error instead of asserting for the low fee check will be better as it does not crash the node and instructs users to report the bug.
2022-12-05Merge bitcoin/bitcoin#26560: wallet: bugfix, invalid CoinsResult cached ↵Andrew Chow
total amount 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3 refactor: make CoinsResult total amounts members private (furszy) 3282fad59908da328f8323e1213344fe58ccf69e wallet: add assert to SelectionResult::Merge for safety (S3RK) c4e3b7d6a154e82cdb902fd7bcb7b725aebde5ea wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy) cac2725fd0f5baeb741dfe079a87332784c2adc7 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy) cf793846978a8783c23b66ba6b4f3f30e83ff3eb test: Coin Selection, duplicated preset inputs selection (furszy) 341ba7ffd8cdb56b4cde1f251768c3d2c2a9b4e9 test: wallet, coverage for CoinsResult::Erase function (furszy) f930aefff9690a1e830d897d0a8c53f4219ae4a8 wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) Pull request description: This comes with #26559. Solving few bugs inside the wallet's transaction creation process and adding test coverage for them. Plus, making use of the `CoinsResult::total_amount` cached value inside the Coin Selection process to return early if we don't have enough funds to cover the target amount. ### Bugs 1) The `CoinsResult::Erase` method removes only one output from the available coins vector (there is a [loop break](https://github.com/bitcoin/bitcoin/blob/c1061be14a515b0ed4f4d646fcd0378c62e6ded3/src/wallet/spend.cpp#L112) that should have never been there) and not all the preset inputs. Which on master is not a problem, because since [#25685](https://github.com/bitcoin/bitcoin/pull/25685) we are no longer using the method. But, it's a bug on v24 (check [#26559](https://github.com/bitcoin/bitcoin/pull/26559)). This method it's being fixed and not removed because I'm later using it to solve another bug inside this PR. 2) As we update the total cached amount of the `CoinsResult` object inside `AvailableCoins` and we don't use such function inside the coin selection tests (we manually load up the `CoinsResult` object), there is a discrepancy between the outputs that we add/erase and the total amount cached value. ### Improvements * This makes use of the `CoinsResult` total amount field to early return with an "Insufficient funds" error inside Coin Selection if the tx target amount is greater than the sum of all the wallet available coins plus the preset inputs amounts (we don't need to perform the entire coin selection process if we already know that there aren't enough funds inside our wallet). ### Test Coverage 1) Adds test coverage for the duplicated preset input selection bug that we have in v24. Where the wallet invalidly selects the preset inputs twice during the Coin Selection process. Which ends up with a "good" Coin Selection result that does not cover the total tx target amount. Which, alone, crashes the wallet due an insane fee. But.. to make it worst, adding the subtract fee from output functionality to this mix ends up with the wallet by-passing the "insane" fee assertion, decreasing the output amount to fulfill the insane fee, and.. sadly, broadcasting the tx to the network. 2) Adds test coverage for the `CoinsResult::Erase` method. ------------------------------------ TO DO: * [ ] Update [#26559 ](https://github.com/bitcoin/bitcoin/pull/26559) description. ACKs for top commit: achow101: ACK 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3 glozow: ACK 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there. josibake: ACK [7362f8e](https://github.com/bitcoin/bitcoin/pull/26560/commits/7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3) Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
2022-12-05Fixup clang-tidy named argument commentsfanquake
Fix comments so they are checked/consistent. Fix incorrect arguments.
2022-12-05Merge bitcoin/bitcoin#24226: rpc: warn that nodes ignore requests for old ↵MarcoFalke
stale blocks f39d9269ebbcffe70d02674c67c4f53783b63005 rpc: warn that nodes ignore requests for old stale blocks (Sjors Provoost) Pull request description: Adds warning to RPC help that `getblockfrompeer` is of little use for stale blocks that are more than a month old. This is an anti-fingerprinting measure. See `BlockRequestAllowed` in `net_processing`. It's been in Bitcoin Core since 2014, introduced in #2910 and later improved to not rely on checkpoints. Older and alternative clients might still serve these blocks, so not throwing an error. Allowing whitelisted nodes to fetch these blocks anyway might be nice. ACKs for top commit: fjahr: Code review ACK f39d9269ebbcffe70d02674c67c4f53783b63005 Tree-SHA512: db88f9f7521289640c5e629c840dda1c2c3ab70d458e9e7136c60fbaeb02acfb36dc093502d83d4c098c331e22aab81bf8f4c4961d805e3bde0f8f3cfe68d968
2022-12-05Merge bitcoin/bitcoin#26624: refactor: Rename local variable to distinguish ↵glozow
it from type alias 1984db1d5037646d7ded23d9f26a42524b7e3519 refactor: Rename local variable to distinguish it from type alias (Hennadii Stepanov) Pull request description: The `txiter` type alias is declared in the `txmempool.h`: https://github.com/bitcoin/bitcoin/blob/9e59d21fbe5746b220f35b0a5a735198c3e6dcdb/src/txmempool.h#L406 ACKs for top commit: stickies-v: ACK 1984db1d5 vasild: ACK 1984db1d5037646d7ded23d9f26a42524b7e3519 jarolrod: ACK 1984db1d5037646d7ded23d9f26a42524b7e3519 Tree-SHA512: 127bfb62627e2d79d8cdb0bd0ac11b3737568c3631b54b2d1e37984f673a1f60edf7bc102a269f7eb40e4bb124b910b924a89475c6a6ea978b2171219fa30685
2022-12-02doc: Drop no longer relevant commentHennadii Stepanov
The comment was introduced in 4cf3411056f6a59fc5fe07784b6b6a512d76b046, and since 7e4bd19785ff9120b7242ff9f15231868aaf7db4 it has been no longer relevant.
2022-12-02refactor: make CoinsResult total amounts members privatefurszy
2022-12-02wallet: add assert to SelectionResult::Merge for safetyS3RK
2022-12-02wallet: SelectCoins, return early if wallet's UTXOs cannot cover the targetfurszy
The CoinsResult class will now count the raw total amount and the effective total amount internally (inside the 'CoinsResult::Add' and 'CoinsResult::Erase' methods). So there is no discrepancy between what we add/erase and the total values. (which is what was happening on the coinselector_test because the 'CoinsResult' object is manually created there, and we were not keeping the total amount in sync with the outputs being added/removed).
2022-12-02test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of ↵furszy
direct member access Aside from the cleanup, this solves a bug in the following-up commit. Because, in these tests, we are manually adding/erasing outputs from the CoinsResult object but never updating the internal total amount field.
2022-12-02test: Coin Selection, duplicated preset inputs selectionfurszy
This exercises the bug inside CoinsResult::Erase that ends up on (1) a wallet crash or (2) a created and broadcasted tx that contains a reduced recipient's amount. This is covered by making the wallet selects the preset inputs twice during the coin selection process. Making the wallet think that the selection process result covers the entire tx target when it does not. It's actually creating a tx that sends more coins than what inputs are covering for. Which, combined with the SFFO option, makes the wallet incorrectly reduce the recipient's amount by the difference between the original target and the wrongly counted inputs. Which means, a created and relayed tx sending less coins to the destination than what the user inputted.
2022-12-02test: wallet, coverage for CoinsResult::Erase functionfurszy
2022-12-02Merge bitcoin/bitcoin#26569: p2p: Ensure transaction announcements are only ↵fanquake
queued for fully connected peers 8f2dac54096c20afd8fd12c21a2ee5465fea085e [test] Add p2p_tx_privacy.py (dergoegge) ce63fca13e9b500e9f687d80a457175ac967a371 [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge) 845e3a34c49abcc634b5a10ccdd6b10fb4fcf449 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge) Pull request description: `TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake). Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node. ACKs for top commit: MarcoFalke: ACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e 🔝 jnewbery: utACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9
2022-12-02refactor: Rename local variable to distinguish it from type aliasHennadii Stepanov
The `txiter` type alias is declared in the `txmempool.h`.
2022-12-01util: Add StrFormatInternalBug and STR_INTERNAL_BUGMarcoFalke
generated by cgit v1.2.3 (git 2.26.2) at 2024-09-19 09:02:48 +0000