aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2024-05-20Merge bitcoin/bitcoin#30133: test: remove unneeded `-maxorphantx=1000` settingsglozow
8950053636cb38ed85fe2d58b53e5d0acb35c390 test: remove unneeded `-maxorphantx=1000` settings (Sebastian Falbesoner) Pull request description: It's unclear what the motivation for increasing the orphan pool is here, and it seems that this not needed at all. None of these tests involve orphan transactions explicitly, and if they would occur occasionally, there is no good reason to prefer a value of 1000 over the default of 100 (see DEFAULT_MAX_ORPHAN_TRANSACTIONS). ACKs for top commit: maflcko: utACK 8950053636cb38ed85fe2d58b53e5d0acb35c390 edilmedeiros: Tested ACK 8950053636cb38ed85fe2d58b53e5d0acb35c390 AngusP: tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390 glozow: ACK 8950053636cb38ed85fe2d58b53e5d0acb35c390 From skimming the tests, it appears that none of these need a larger `-maxorphantx`. Tree-SHA512: 81d4a4fb2ea92b97119f21cbc6c4b1240d863269932e6adf4982aead9726f20652523a4707add3ad38eb332d4452de41de6735265f22e62298f3b4b45de75a57
2024-05-20Merge bitcoin/bitcoin#30066: test: add conflicting topology test caseglozow
9365baa489e123d9bcaf986e4311d3fa3f1e3f88 test: add conflicting topology test case (Greg Sanders) Pull request description: We want to ensure that even if topologies that are acceptable are relaxed, like removing package-not-child-with-unconfirmed-parents, that we don't end up accepting packages we shouldn't. ACKs for top commit: glozow: reACK 9365baa489 rkrux: reACK [9365baa](https://github.com/bitcoin/bitcoin/pull/30066/commits/9365baa489e123d9bcaf986e4311d3fa3f1e3f88) Tree-SHA512: d58661064ca099ac0447c331a5020c74c0cdfe24259aa875592805bbd63de1bf23aa7ced9ff485fef90dc0602fcb997e631aaf1aa2e9805d2cf5f0e5c9b2f0e2
2024-05-17test: remove unneeded `-maxorphantx=1000` settingsSebastian Falbesoner
It's unclear what the motivation for increasing the orphan pool is, and it seems that this not needed at all. None of these tests involve orphan transactions explicitly, and if they would occur occasionally, there is no good reason to prefer a value of 1000 over the default of 100 (see DEFAULT_MAX_ORPHAN_TRANSACTIONS).
2024-05-17Merge bitcoin/bitcoin#29817: kernel: De-globalize fReindexAva Chow
b47bd959207e82555f07e028cc2246943d32d4c3 kernel: De-globalize fReindex (TheCharlatan) Pull request description: fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use. --- This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). ACKs for top commit: achow101: ACK b47bd959207e82555f07e028cc2246943d32d4c3 ryanofsky: Code review ACK b47bd959207e82555f07e028cc2246943d32d4c3. I rereviewed the whole PR, but the only change since last review was reverting the bugfix https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024 and make the change a pure refactoring. mzumsande: Code Review ACK b47bd959207e82555f07e028cc2246943d32d4c3 stickies-v: ACK b47bd959207e82555f07e028cc2246943d32d4c3 Tree-SHA512: f7399d01f93bc0c0c7428fe95d19b9d29b4ed00a4f1deabca78fb0c4fecb434ec971e890feecb105938b5247c926850b1b7b4a4a9caa333a061e40777d0c8463
2024-05-17Merge bitcoin/bitcoin#30048: crypto: add `NUMS_H` constAva Chow
9408a04e424cee0d226bde79171bd4954f9caeb0 tests, fuzz: use new NUMS_H const (josibake) b946f8a4c51be42e52d63a6d578158c0b2a6b7ed crypto: add NUMS_H const (josibake) Pull request description: Broken out from #28122 --- [BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#constructing-and-spending-taproot-outputs) defines a NUMS point `H` as *H = lift_x(0x50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0)* which is [constructed](https://github.com/ElementsProject/secp256k1-zkp/blob/11af7015de624b010424273be3d91f117f172c82/src/modules/rangeproof/main_impl.h#L16) by taking the hash of the standard uncompressed encoding of the [secp256k1](https://www.secg.org/sec2-v2.pdf) base point G as X coordinate." Add this as a constant so it can be used in our codebase. My primary motivation is BIP352 specifies a special case for when taproot spends use `H` as the internal key, but outside of BIP352 it seems generally useful to have `H` in the codebase, for testing or other use cases. ACKs for top commit: paplorinc: re-ACK 9408a04e424cee0d226bde79171bd4954f9caeb0 achow101: ACK 9408a04e424cee0d226bde79171bd4954f9caeb0 theStack: Code-review ACK 9408a04e424cee0d226bde79171bd4954f9caeb0 Tree-SHA512: ad84492f5d635c0cb05bd82546079ded7e5138e95361f20d8285a9ad6e69c10ee2cc3fe46e16b46ef03c4253c8bee1051911c6b91264c90c3b1ad33a824bff4b
2024-05-16Merge bitcoin/bitcoin#29975: blockstorage: Separate reindexing from saving ↵Ryan Ofsky
new blocks e41667b720372dae8438ea86e9819027e62b54e0 blockstorage: Don't move cursor backwards in UpdateBlockInfo (Ryan Ofsky) 17103637c6fa2dfcf5374ebb0cd715e540dd4ce1 blockstorage: Rename FindBlockPos and have it return a FlatFilePos (Martin Zumsande) d9e477c4dc39d9623ed66c35c06e28f94ae62ad5 validation, blockstorage: Separate code paths for reindex and saving new blocks (Martin Zumsande) 064859bbad6984a6ec85c744064abdf757807c58 blockstorage: split up FindBlockPos function (Martin Zumsande) fdae638e83522c28a1222e65c43d1cbca3e34cba doc: Improve doc for functions involved in saving blocks to disk (Martin Zumsande) 0d114e3cb20cb9e03fc9ba8daf3d03436b491742 blockstorage: Add Assume for fKnown / snapshot chainstate (Martin Zumsande) Pull request description: `SaveBlockToDisk` / `FindBlockPos` are used for two purposes, depending on whether they are called during reindexing (`dbp` set,  `fKnown = true`) or in the "normal" case when adding new blocks (`dbp == nullptr`,  `fKnown = false`). The actual tasks are quite different - In normal mode, preparations for saving a new block are made, which is then saved: find the correct position on disk (maybe skipping to a new blk file), check for available disk space, update the blockfile info db, save the block. - during reindex, most of this is not necessary (the block is already on disk after all), only the blockfile info needs to rebuilt because reindex wiped the leveldb it's saved in. Using one function with many conditional statements for this leads to code that is hard to read / understand and bug-prone: - many code paths in `FindBlockPos` are conditional on `fKnown` or `!fKnown` - It's not really clear what actually needs to be done during reindex (we don't need to "save a block to disk" or "find a block pos" as the function names suggest) - logic that should be applied to only one of the two modes is sometimes applied to both (see first commit, or #27039) #24858 and #27039 were recent bugs directly related to the differences between reindexing and normal mode, and in both cases the simple fix took a long time to be reviewed and merged. This PR proposes to clean this code up by splitting out the reindex logic into a separate function (`UpdateBlockInfo`) which will be called directly from validation. As a result, `SaveBlockToDisk` and `FindBlockPos` only need to cover the non-reindex logic. ACKs for top commit: paplorinc: ACK e41667b720372dae8438ea86e9819027e62b54e0 TheCharlatan: Re-ACK e41667b720372dae8438ea86e9819027e62b54e0 ryanofsky: Code review ACK e41667b720372dae8438ea86e9819027e62b54e0. Just improvements to comments since last review. Tree-SHA512: a14ff9a0facf6b1e3c1cd724a2d19a79a25d4b48de64398fdd172671532a472bc10a20cbb64ac3a3e55814dcc877d0597a3e1699cabc4f9d9a86b439b6eaba20
2024-05-16Merge bitcoin/bitcoin#27101: Support JSON-RPC 2.0 when requested by clientRyan Ofsky
cbc6c440e3811d342fa570713702900b3e3e75b9 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin) e7ee80dcf2b68684eae96070875ea13a60e3e7b0 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin) bf1a1f1662427fbf1a43bb951364eface469bdb7 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin) 466b90562f4785de74b548f7c4a256069e2aaf43 rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin) 2ca1460ae3a7217eaa8c5972515bf622bedadfce rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin) a64a2b77e09bff784a2635ba19ff4aa6582bb5a5 rpc: refactor single/batch requests (Matthew Zipkin) df6e3756d6feaf1856e7886820b70874209fd90b rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin) 09416f9ec445e4d6bb277400758083b0b4e8b174 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin) 4202c170da37a3203e05a9f39f303d7df19b6d81 test: refactor interface_rpc.py (Matthew Zipkin) Pull request description: Closes https://github.com/bitcoin/bitcoin/issues/2960 Bitcoin Core's JSONRPC server behaves with a special blend of 1.0, 1.1 and 2.0 behaviors. This introduces compliance issues with more strict clients. There are the major misbehaviors that I found: - returning non-200 HTTP codes for RPC errors like "Method not found" (this is not a server error or an HTTP error) - returning both `"error"` and `"result"` fields together in a response object. - different error-handling behavior for single and batched RPC requests (batches contain errors in the response but single requests will actually throw HTTP errors) https://github.com/bitcoin/bitcoin/pull/15495 added regression tests after a discussion in https://github.com/bitcoin/bitcoin/pull/15381 to kinda lock in our RPC behavior to preserve backwards compatibility. https://github.com/bitcoin/bitcoin/pull/12435 was an attempt to allow strict 2.0 compliance behind a flag, but was abandoned. The approach in this PR is not strict and preserves backwards compatibility in a familiar bitcoin-y way: all old behavior is preserved, but new rules are applied to clients that opt in. One of the rules in the [JSON RPC 2.0 spec](https://www.jsonrpc.org/specification#request_object) is that the kv pair `"jsonrpc": "2.0"` must be present in the request. Well, let's just use that to trigger strict 2.0 behavior! When that kv pair is included in a request object, the [response will adhere to strict JSON-RPC 2.0 rules](https://www.jsonrpc.org/specification#response_object), essentially: - always return HTTP 200 "OK" unless there really is a server error or malformed request - either return `"error"` OR `"result"` but never both - same behavior for single and batch requests If this is merged next steps can be: - Refactor bitcoin-cli to always use strict 2.0 - Refactor the python test framework to always use strict 2.0 for everything - Begin deprecation process for 1.0/1.1 behavior (?) If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit. ACKs for top commit: cbergqvist: re ACK cbc6c440e3811d342fa570713702900b3e3e75b9 ryanofsky: Code review ACK cbc6c440e3811d342fa570713702900b3e3e75b9. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments. tdb3: re ACK for cbc6c440e3811d342fa570713702900b3e3e75b9 Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36
2024-05-16kernel: De-globalize fReindexTheCharlatan
fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.
2024-05-16Merge bitcoin/bitcoin#30085: p2p: detect addnode cjdns peers in ↵merge-script
GetAddedNodeInfo() d0b047494c28381942c09d0cca45baa323bfcffc test: add GetAddedNodeInfo() CJDNS regression unit test (Jon Atack) 684da9707040ce25d95b2954eda50b811136d92c p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo() (Jon Atack) Pull request description: Addnode peers connected to us via the cjdns network are currently not detected by `CConnman::GetAddedNodeInfo()`, i.e. `fConnected` is always false. This causes the following issues: - RPC `getaddednodeinfo` incorrectly shows them as not connected - `CConnman::ThreadOpenAddedConnections()` continually retries to connect them Fix the issue and add a unit regression test. Extracted from #28248. Suggest running the test with: `./src/test/test_bitcoin -t net_peer_connection_tests -l test_suite` ACKs for top commit: mzumsande: utACK d0b047494c28381942c09d0cca45baa323bfcffc brunoerg: crACK d0b047494c28381942c09d0cca45baa323bfcffc pinheadmz: ACK d0b047494c28381942c09d0cca45baa323bfcffc Tree-SHA512: a4d81425f79558f5792585611f3fe8ab999b82144daeed5c3ec619861c69add934c2b2afdad24c8488a0ade94f5ce8112f5555d60a1ce913d4f5a1cf5dbba55a
2024-05-16Merge bitcoin/bitcoin#30097: crypto: disable asan for sha256_sse4 with clang ↵merge-script
and -O0 141df0a28810470e53fdbc6d32d3cb4020fe3ca1 crypto: disable asan for sha256_sse4 with clang and -O0 (Cory Fields) Pull request description: Clang is unable to compile the Transform function for that combination of options. Fixes #29801. ACKs for top commit: achow101: ACK 141df0a28810470e53fdbc6d32d3cb4020fe3ca1 Tree-SHA512: d74fdac5840ad7524edfde069fb43ae75c31146e90ecc58bbc7912ff57a02b068547431b1766afeed782272c0b93b0b41a286c1cf26ec55ce332d94ce917d810
2024-05-15Merge bitcoin/bitcoin#28929: serialization: Support for multiple parametersAva Chow
8d491ae9ecf1948ea29f67b50ca7259123f602aa serialization: Add ParamsStream GetStream() method (Ryan Ofsky) 951203bcc496c4415b7754cd764544659b76067f net: Simplify ParamsStream usage (Ryan Ofsky) e6794e475c84d9edca4a2876e2342cbb1d85f804 serialization: Accept multiple parameters in ParamsStream constructor (Ryan Ofsky) cb28849a88339c1e7ba03ffc7e38998339074e6e serialization: Reverse ParamsStream constructor order (Ryan Ofsky) 83436d14f06551026bcf5529df3b63b4e8a679fb serialization: Drop unnecessary ParamsStream references (Ryan Ofsky) 84502b755bcc35413ad466047893b5edf134c53f serialization: Drop references to GetVersion/GetType (Ryan Ofsky) f3a2b5237688e9f574444e793724664b00fb7f2a serialization: Support for multiple parameters (Ryan Ofsky) Pull request description: Currently it is only possible to attach one serialization parameter to a stream at a time. For example, it is not possible to set a parameter controlling the transaction format and a parameter controlling the address format at the same time because one parameter will override the other. This limitation is inconvenient for multiprocess code since it is not possible to create just one type of stream and serialize any object to it. Instead it is necessary to create different streams for different object types, which requires extra boilerplate and makes using the new parameter fields a lot more awkward than the older version and type fields. Fix this problem by allowing an unlimited number of serialization stream parameters to be set, and allowing them to be requested by type. Later parameters will still override earlier parameters, but only if they have the same type. For an example of different ways multiple parameters can be set, see the new [`with_params_multi`](https://github.com/bitcoin/bitcoin/blob/40f505583f4edeb2859aeb70da20c6374d331a4f/src/test/serialize_tests.cpp#L394-L410) unit test. This change requires replacing the `stream.GetParams()` method with a `stream.GetParams<T>()` method in order for serialization code to retrieve the desired parameters. The change is more verbose, but probably a good thing for readability because previously it could be difficult to know what type the `GetParams()` method would return, and now it is more obvious. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). ACKs for top commit: maflcko: ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa 🔵 sipa: utACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa TheCharlatan: ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa Tree-SHA512: 40b7041ee01c0372b1f86f7fd6f3b6af56ef24a6383f91ffcedd04d388e63407006457bf7ed056b0e37b4dec9ffd5ca006cb8192e488ea2c64678567e38d4647
2024-05-15Merge bitcoin-core/gui#722: wallet: Allow user to navigate options while ↵merge-script
encrypting at creation cccddc03f0c625daeac7158eb20c1508aea5df39 Wallet encrypt on create, allow to navigate options (Hernan Marino) Pull request description: This fixes https://github.com/bitcoin-core/gui/issues/394. It adds a "Go back" button to the "Confirm wallet encryption" window, allowing the users to change the password if they want to. It also adds a Cancel button to the "Wallet to be encrypted" window. Prior to this change users had no option to alter the password, and were forced to either go ahead with wallet creation or cancel the whole process. Also, at the final window, they were shown a warning but with no option to cancel. The new workflow for wallet encryption and creation is similar to the following: ![videoNavigation](https://user-images.githubusercontent.com/87907936/225705434-22d3c678-fa01-4079-ba10-ca5a0e8d3922.gif) ACKs for top commit: alfonsoromanz: Re-Tested ACK https://github.com/bitcoin-core/gui/commit/cccddc03f0c625daeac7158eb20c1508aea5df39 BrandonOdiwuor: re-Tested ACK cccddc03f0c625daeac7158eb20c1508aea5df39 hebasto: ACK cccddc03f0c625daeac7158eb20c1508aea5df39, tested on Ubuntu 24.04. Tree-SHA512: d2856d75f75acbd7d51ede62b4abd317f6ed6a9b890fe0b73b63b921b4b3d61b49477e35dc74466a056a9e8c0c1598df7601111d36c57ef18fdfdf0b18f503e6
2024-05-15Merge bitcoin/bitcoin#30000: p2p: index TxOrphanage by wtxid, allow entries ↵Ryan Ofsky
with same txid 0fb17bf61a40b73a2b81a18e70b3de180c917f22 [log] updates in TxOrphanage (glozow) b16da7eda76944719713be68b61f03d4acdd3e16 [functional test] attackers sending mutated orphans (glozow) 6675f6428d653bf7a53537bd773114f4fb5ba53f [unit test] TxOrphanage handling of same-txid-different-witness txns (glozow) 8923edfc1f12ebc6a074651c084ba7d249074799 [p2p] allow entries with the same txid in TxOrphanage (glozow) c31f148166f01a9167d82501a77823785d28a841 [refactor] TxOrphanage::EraseTx by wtxid (glozow) efcc5930175f31b685adb4627a038d9f0848eb1f [refactor] TxOrphanage::HaveTx only by wtxid (glozow) 7e475b9648bbee04f5825b922ba0399373eaa5a9 [p2p] don't query orphanage by txid (glozow) Pull request description: Part of #27463 in the "make orphan handling more robust" section. Currently the main map in `TxOrphanage` is indexed by txid; we do not allow 2 transactions with the same txid into TxOrphanage. This means that if we receive a transaction and want to store it in orphanage, we'll fail to do so if a same-txid-different-witness version of the tx already exists in the orphanage. The existing orphanage entry can stay until it expires 20 minutes later, or until we find that it is invalid. This means an attacker can try to block/delay us accepting an orphan transaction by sending a mutated version of the child ahead of time. See included test. Prior to #28970, we don't rely on the orphanage for anything and it would be relatively difficult to guess what transaction will go to a node's orphanage. After the parent(s) are accepted, if anybody sends us the correct transaction, we'll end up accepting it. However, this is a bit more painful for 1p1c: it's easier for an attacker to tell when a tx is going to hit a node's orphanage, and we need to store the correct orphan + receive the parent before we'll consider the package. If we start out with a bad orphan, we can't evict it until we receive the parent + try the 1p1c, and then we'll need to download the real child, put it in orphanage, download the parent again, and then retry 1p1c. ACKs for top commit: AngusP: ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22 itornaza: trACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22 instagibbs: ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22 theStack: ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22 sr-gi: crACK [0fb17bf](https://github.com/bitcoin/bitcoin/pull/30000/commits/0fb17bf61a40b73a2b81a18e70b3de180c917f22) stickies-v: ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22 Tree-SHA512: edcbac7287c628bc27036920c2d4e4f63ec65087fbac1de9319c4f541515d669fc4e5fdc30c8b9a248b720da42b89153d388e91c7bf5caf4bc5b3b931ded1f59
2024-05-15crypto: disable asan for sha256_sse4 with clang and -O0Cory Fields
Clang is unable to compile the Transform function for that combination of options.
2024-05-15Merge bitcoin/bitcoin#30060: ci: Roll clang in test-each-commit taskmerge-script
fa90ad23c0cb99bde305af156c978c066f7bacb8 ci: Roll test-each-commit Ubuntu (MarcoFalke) fa6c82dd9b0ef9687c28ddd6b57065d0ba7de85b ci: Remove clang version pin in test-each-commit (MarcoFalke) Pull request description: Needed for https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-2099704210 ACKs for top commit: hebasto: re-ACK fa90ad23c0cb99bde305af156c978c066f7bacb8. Tree-SHA512: 753a3a77d967f308b5b5dd0bc7ea9f3268fc93c5ac978da3d79b85461cb1e994c6ac481888dc472b9a08be45ad0fad66ad3fda241a8955f999b7c2cb2a2b1f58
2024-05-15ci: Roll test-each-commit UbuntuMarcoFalke
2024-05-15ci: Remove clang version pin in test-each-commitMarcoFalke
2024-05-15Merge bitcoin/bitcoin#30098: refactor: simplify `FormatSubVersion` using ↵merge-script
strprintf/Join 12d82817bf32396b58c8c65645012def606680b6 refactor: simplify `FormatSubVersion` using strprintf/Join (Sebastian Falbesoner) Pull request description: Rather than using std::ostringstream and manually joining the comments, use strprintf and our own `Join` helper. ACKs for top commit: maflcko: utACK 12d82817bf32396b58c8c65645012def606680b6 TheCharlatan: tACK 12d82817bf32396b58c8c65645012def606680b6 hebasto: ACK 12d82817bf32396b58c8c65645012def606680b6, I have reviewed the code and it looks OK. tdb3: ACK for 12d82817bf32396b58c8c65645012def606680b6. Tree-SHA512: b9b965c4416a4c0c8727e3c4b40da4be04b14067200220492e9bed4fa35c1907fb5cdec2a30052b9e762f71da3d3cf042f43c96ab1f2523df5bb9920b44ea2a5
2024-05-15Merge bitcoin/bitcoin#30074: contrib: use ENV flags in get_archmerge-script
b59a027d957a4cffd225a681e6c85f9ae7fd77f3 contrib: drop dead get_machine from test sym check (fanquake) e6aba463adeb88fc707342a12fef658f68b0a0ea contrib: use env_flags in get_arch (fanquake) Pull request description: This isn't an issue right now (because the get_arch check is simple), but becomes one as soon as we want to use `lld` for linking, and need LDFLAGS (otherwise we call `ld` and fail, see it's usage in #21778). So I've split this out for review. It also makes sense to use the same flags for all compilation in these checks. Also drops some dead code in test-symbol-check. ACKs for top commit: TheCharlatan: ACK b59a027d957a4cffd225a681e6c85f9ae7fd77f3 Tree-SHA512: d8afc4144815369aae63cf6dc6e983af46f208c7043d6ea5c9c811152649c256a8e67eb6864ea9d385d87b6b049fece07710a84b90da325da7fc3f05efcaacd6
2024-05-14Merge bitcoin/bitcoin#29086: refactor: Simply include CTxMemPool::Options in ↵Ava Chow
CTxMemPool directly rather than duplicating definition cc67d33fdac45357b593b1faff3d1735e5fe91ba refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition (Luke Dashjr) Pull request description: Instead of duplicating mempool options two places, just include the Options struct directly on the CTxMemPool ACKs for top commit: achow101: ACK cc67d33fdac45357b593b1faff3d1735e5fe91ba kristapsk: cr utACK cc67d33fdac45357b593b1faff3d1735e5fe91ba jonatack: ACK cc67d33fdac45357b593b1faff3d1735e5fe91ba Tree-SHA512: 9deb5ea6f85eeb1c7e04536cded65303b0ec459936a97e4f257aff2c50b0984a4ddbf69a4651f48455b9c80200a1fd24e9c74926874fdd9be436bbbe406251ce
2024-05-14blockstorage: Don't move cursor backwards in UpdateBlockInfoRyan Ofsky
Previously, it was possible to move the cursor back to an older file during reindex if blocks are enocuntered out of order during reindex. This would mean that MaxBlockfileNum() would be incorrect, and a wrong DB_LAST_BLOCK could be written to disk. This improves the logic by only ever moving the cursor forward (if possible) but not backwards. Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
2024-05-14blockstorage: Rename FindBlockPos and have it return a FlatFilePosMartin Zumsande
The new name reflects that it is no longer called with existing blocks for which the position is already known. Returning a FlatFilePos directly simplifies the interface.
2024-05-14validation, blockstorage: Separate code paths for reindex and saving new blocksMartin Zumsande
By calling SaveBlockToDisk only when we actually want to save a new block to disk. In the reindex case, we now call UpdateBlockInfo directly from validation. This commit doesn't change behavior.
2024-05-14blockstorage: split up FindBlockPos functionMartin Zumsande
FindBlockPos does different things depending on whether the block is known or not, as shown by the fact that much of the existing code is conditional on fKnown set or not. If the block position is known (during reindex) the function only updates the block info statistics. It doesn't actually find a block position in this case. This commit removes fKnown and splits up these two code paths by introducing a separate function for the reindex case when the block position is known. It doesn't change behavior.
2024-05-14doc: Improve doc for functions involved in saving blocks to diskMartin Zumsande
In particular, document the flat file positions expected and returned by functions better. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2024-05-14doc: add comments and release-notes for JSON-RPC 2.0Matthew Zipkin
2024-05-14rpc: JSON-RPC 2.0 should not respond to "notifications"Matthew Zipkin
For JSON-RPC 2.0 requests we need to distinguish between a missing "id" field and "id":null. This is accomplished by making the JSONRPCRequest id property a std::optional<UniValue> with a default value of UniValue::VNULL. A side-effect of this change for non-2.0 requests is that request which do not specify an "id" field will no longer return "id": null in the response.
2024-05-14Merge bitcoin/bitcoin#30083: kernel: Remove batchpriority from kernel libraryRyan Ofsky
d4b17c7d46ad8e2833ade99d5b4c9741c913e84d kernel: Remove batchpriority from kernel library (TheCharlatan) Pull request description: The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread. Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now freely choose their own scheduling policy. This PR is easier reviewed with `git diff --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra` --- This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). ACKs for top commit: maflcko: ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d 📭 ryanofsky: Code review ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, just added suggested comment since last review hebasto: ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK. Tree-SHA512: cafedecd9affad58ddd7f30f68bba71291ca951bb186ff4b2da04b7f21f0b26e5e3143846d032b9e391bd5ce6c7466b97aa3758d2a85ebd7353eb8b69139641a
2024-05-14rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requestsMatthew Zipkin
Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the RPC method failed but the HTTP request was otherwise valid. Batch requests already did not return HTTP errors previously.
2024-05-14rpc: Add "jsonrpc" field and drop null "result"/"error" fieldsMatthew Zipkin
Only for JSON-RPC 2.0 requests.
2024-05-14rpc: identify JSON-RPC 2.0 requestsMatthew Zipkin
2024-05-14test: add conflicting topology test caseGreg Sanders
We want to ensure that even if topologies that are acceptable are relaxed, like removing package-not-child-with-unconfirmed-parents, that we don't end up accepting packages we shouldn't.
2024-05-14tests, fuzz: use new NUMS_H constjosibake
2024-05-14[log] updates in TxOrphanageglozow
- Add elapsed time in "remove orphan" log - Add size in "stored orphan" log - grammar edit
2024-05-14[functional test] attackers sending mutated orphansglozow
2024-05-14[unit test] TxOrphanage handling of same-txid-different-witness txnsglozow
2024-05-14[p2p] allow entries with the same txid in TxOrphanageglozow
Index by wtxid instead of txid to allow entries with the same txid but different witnesses in orphanage. This prevents an attacker from blocking a transaction from entering the orphanage by sending a mutated version of it.
2024-05-14[refactor] TxOrphanage::EraseTx by wtxidglozow
No behavior change right now, as transactions in the orphanage are unique by txid. This makes the next commit easier to review.
2024-05-14[refactor] TxOrphanage::HaveTx only by wtxidglozow
2024-05-14[p2p] don't query orphanage by txidglozow
2024-05-14kernel: Remove batchpriority from kernel libraryTheCharlatan
The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread. Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now choose their own scheduling policy.
2024-05-14crypto: add NUMS_H constjosibake
2024-05-14Merge bitcoin/bitcoin#30078: depends: set AR & RANLIB for CMakemerge-script
019ad7327c397094d7648b55503bf5373b108a57 depends: set RANLIB for CMake (fanquake) 43cfb428cba04b8db98d4d0d56ffe28ad686e58c depends: set NM for CMake (fanquake) 1e4412b317f74dd64069309544fe73c95e2c10e7 depends: set AR for CMake (fanquake) Pull request description: Needed for #21778. Should be more correct in any case. ACKs for top commit: theuni: utACK 019ad7327c397094d7648b55503bf5373b108a57. I didn't test, but I tried this approach on a few deps and it seemed to work as expected. TheCharlatan: ACK 019ad7327c397094d7648b55503bf5373b108a57 Tree-SHA512: 78cc8981456f7476cafca0e40fcc569e474b92004c8024d1c4268b6aab53175074a06ab17ebded8d706bf0a7f77401642dd38bb7ce2e4b04abdcd149d3d69969
2024-05-13refactor: simplify `FormatSubVersion` using strprintf/JoinSebastian Falbesoner
Rather than using std::ostringstream and manually joining the comments, use strprintf and our own `Join` helper.
2024-05-13Merge bitcoin/bitcoin#28233: validation: don't clear cache on periodic ↵Ava Chow
flush: >2x block connection speed 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b validation: don't clear cache on periodic flush (Andrew Toth) Pull request description: Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit. Periodic flushes occur every 24 hours, which empties the cache and causes block connection to slow down. By keeping the cache through periodic flushes a node can run for several days with an increasingly hotter cache and connect blocks much more quickly. Now not only can setting a higher `dbcache` value be beneficial for IBD, it can also be beneficial for connecting blocks faster. To benchmark in real world usage, I spun up 6 identical `t2.small` AWS EC2 instances, all running in the same region in the same VPC. I configured 2 instances to run master, 2 instances to run the change in this PR, and 2 instances to run the change in this PR but with `dbcache=1000`. All instances had `prune=5000` and a 20 GB `gp2` `EBS` volume. A 7th EC2 instance in the same VPC ran master and connected only to some trusted nodes in the outside network. Each of the 6 nodes under test only connected directly to this 7th instance. I manually pruned as much as possible and uploaded the same `blocks`, `chainstate` and `mempool.dat` to all instances. I started all 6 peers simultaneously at block height `835245` and ran them for over a week until block `836534`. The results were much faster block connection times for this branch compared to master, and much faster for this branch with `dbcache=1000` compared to default `dbcache`. | branch |speed | |-----------:|----------:| | master 1 | 1995.49ms/blk | | master 2 | 2129.78ms/blk | | branch default dbcache 1 | 1189.65ms/blk | | branch default dbcache 2 | 1037.74ms/blk | | branch dbcache=1000 1 | 393.69ms/blk | | branch dbcache=1000 2 | 427.77ms/blk | The log files of all 6 instances are [here](https://gist.github.com/andrewtoth/03c95033e7581d5dbc5be028639a1a91). There is a lot of noise with the exact times of blocks being connected, so I plotted the rolling 20 block connect time averages. The large dots are the times where the cache is emptied. For the red master nodes, this happens every 24 hours. The blue branch nodes with default `dbcache` only filled up and emptied the caches once, which is seen in the middle. The green branch nodes with 1000 `dbcache` never emptied the cache. It is very clear from the chart that whenever the cache is emptied, connect block speed degrades significantly. ![plot](https://github.com/bitcoin/bitcoin/assets/237213/802cb28d-1ad4-47c3-a886-c5366b423eca) Also note that this still clears the cache for pruning flushes. Having frequent pruning flushes with a large cache that doesn't clear is less performant than the status quo https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-458657451. See https://github.com/bitcoin/bitcoin/pull/28280. ACKs for top commit: sipa: utACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b achow101: ACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b brunoerg: crACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b Tree-SHA512: 05dbc677bc309bbcf89c52a6c5e853e2816b0ef0b5ee3719b30696df315a0427e244bb82da9ad828ec0e7ea8764552f8affe14c0184b52adf1909f5d8c1b4f9e
2024-05-13Merge bitcoin/bitcoin#30094: rpc: move UniValue in blockToJSONRyan Ofsky
b77bad309e92f176f340598eec056eb7bff86f5f rpc: move UniValue in blockToJSON (willcl-ark) Pull request description: Fixes: #24542 Fixes: #30052 Without explicitly declaring the move, these `UniValues` get copied, causing increased memory usage. Fix this by explicitly moving the `UniValue` objects. Used by `rest_block` and `getblock` RPC. ACKs for top commit: maflcko: review ACK b77bad309e92f176f340598eec056eb7bff86f5f ismaelsadeeq: ACK b77bad309e92f176f340598eec056eb7bff86f5f TheCharlatan: ACK b77bad309e92f176f340598eec056eb7bff86f5f theuni: utACK b77bad309e92f176f340598eec056eb7bff86f5f hebasto: ACK b77bad309e92f176f340598eec056eb7bff86f5f, I have reviewed the code and it looks OK. BrandonOdiwuor: ACK b77bad309e92f176f340598eec056eb7bff86f5f Tree-SHA512: 767608331040f9cfe5c3568ed0e3c338920633472a1a50d4bbb47d1dc69d2bb11466d611f050ac8ad1a894b47fe1ea4d968cf34cbd44d4bb8d479fc5c7475f6d
2024-05-13Merge bitcoin/bitcoin#29974: fuzz: txorphan tests fixupsglozow
58594c7040241f2312b0b8735a8baf0412674613 fuzz: txorphan tests fixups (Sergi Delgado Segura) Pull request description: Motivated by https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576401327 Adds the following fixups in txorphan fuzz tests: - Don't bond the output count of the created orphans to the number of available coins - Allow duplicate inputs but don't store duplicate outpoints Most significantly, this gets rid of the `duplicate_input` flag altogether, making the test easier to reason about. Notice how, under normal conditions, duplicate inputs would be caught by `MemPoolAccept::PreChecks`, however, no validations checks are run on the test before adding data to the orphanage (neither were they before this patch) ## Rationale The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`). If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not. This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop. Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be. ACKs for top commit: maflcko: utACK 58594c7040241f2312b0b8735a8baf0412674613 glozow: ACK 58594c7 instagibbs: ACK 58594c7040241f2312b0b8735a8baf0412674613 Tree-SHA512: e97cc2a43e388f87b64d2e4e45f929dd5b0dd85d668dd693b75e4c9ceea734cd7645952385d428208d07b70e1aafbec84cc2ec264a2e07d36fc8ba3e97885a8d
2024-05-13Merge bitcoin/bitcoin#29994: doc: removed help text saying that peers may ↵glozow
not connect automatically 95897ff181c0757e445f0e066a2a590a0a0120d2 doc: removed help text saying that peers may not connect automatically (kevkevin) Pull request description: Introduced in https://github.com/bitcoin/bitcoin/pull/23542 and released in version 23.0 there has been significant time since this change (2 years). This should be removed as it is no longer relevant ACKs for top commit: stickies-v: ACK 95897ff181c0757e445f0e066a2a590a0a0120d2 tdb3: ACK for 95897ff181c0757e445f0e066a2a590a0a0120d2. vasild: ACK 95897ff181c0757e445f0e066a2a590a0a0120d2 jonatack: ACK 95897ff181c0757e445f0e066a2a590a0a0120d2 kristapsk: ACK 95897ff181c0757e445f0e066a2a590a0a0120d2. According to https://bitnodes.io/dashboard/#user-agents stats, most nodes on the network are v23+. Tree-SHA512: 9e35194f8a1e06f1447450af2ea30cdc43722665c2d2e4b7aa9a52afac5c1e83fed744742c836743a555cc180c90f9eebdc6637eba6190010d693eef9c5834f7
2024-05-13rpc: move UniValue in blockToJSONwillcl-ark
Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC.
2024-05-13depends: set RANLIB for CMakefanquake
generated by cgit v1.2.3 (git 2.26.2) at 2024-09-18 00:12:11 +0000