aboutsummaryrefslogtreecommitdiff
path: root/src/test/util
AgeCommit message (Collapse)Author
2023-11-08Merge bitcoin/bitcoin#28155: net: improves addnode / m_added_nodes logicglozow
0420f99f429ce2382057e101859067f40de47be0 Create net_peer_connection unit tests (Jon Atack) 4b834f649921aceb44d3e0b5a2ffd7847903f9f7 Allow unit tests to access additional CConnman members (Jon Atack) 34b9ef443bc2655a85c8802edc5d5d48d792a286 net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura) 94e8882d820969ddc83f24f4cbe1515a886da4ea rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura) 2574b7e177ef045e64f1dd48cb000640ff5103d3 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura) Pull request description: ## Rationale Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis. ### Connecting to the same node more than once In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways: 1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses 2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it. An example to test this would be calling: ``` bitcoin-cli addnode "127.0.0.1:port" add bitcoin-cli addnode "localhost:port" add ``` And check how it allows us to perform both connections some times, and some times it fails. The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be: ``` bitcoin-cli addnode "127.0.0.1:port" add bitcoin-cli addnode "localhost:port" onetry ``` ### Adding the same peer with two different, yet equivalent, addresses The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks. Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`. This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable. ### Parametrize `CConnman::GetAddedNodeInfo` Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()` ACKs for top commit: jonatack: re-ACK 0420f99f429ce2382057e101859067f40de47be0 kashifs: > > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0) sr-gi: > > > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0) mzumsande: Tested ACK 0420f99f429ce2382057e101859067f40de47be0 Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
2023-11-08Merge bitcoin/bitcoin#28785: validation: return more helpful results for ↵fanquake
reconsiderable fee failures and skipped transactions 1147e00e59e47f27024ec96629993c66a3ce4ef0 [validation] change package-fee-too-low, return wtxid(s) and effective feerate (glozow) 10dd9f2441f4618321bfa2865449ac2223c572a0 [test] use CheckPackageMempoolAcceptResult in previous tests (glozow) 3979f1afcbef5fdd3fad56312573a6733a7d78a4 [validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN (glozow) 5c786a026aee434363ad54f4346211d0e2c5a38d [refactor] use Wtxid for m_wtxids_fee_calculations (glozow) Pull request description: Split off from #26711 (suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253). This is part of #27463. - Add 2 new TxValidationResults - `TX_RECONSIDERABLE` helps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished from `TX_MEMPOOL_POLICY` so that we re-validate a transaction if and only if it is eligible for package CPFP. In the future, we will have a separate cache for reconsiderable rejects so these transactions don't go in `m_recent_rejects`. - `TX_UNKNOWN` helps us communicate that we aborted package validation and didn't finish looking at this transaction: it's not valid but it's also not invalid (i.e. don't cache it as a rejected tx) - Return effective feerate and the wtxids of transactions used to calculate that effective feerate when the error is `TX_SINGLE_FAILURE`. Previously, we would only provide this information if the transaction passed. Now that we have package validation, it's much more helpful to the caller to know how the failing feerate was calculated. This can also be used to improve our submitpackage RPC result (which is currently a bit unhelpful when things fail). - Use the newly added `CheckPackageMempoolAcceptResult` for existing package validation tests. This increases test coverage and helps test the changes made in this PR. ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/28785/commits/1147e00e59e47f27024ec96629993c66a3ce4ef0 achow101: ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0 murchandamus: reACK 1147e00e59e47f27024ec96629993c66a3ce4ef0 ismaelsadeeq: ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0 Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
2023-11-07Merge bitcoin/bitcoin#28224: shutdown: Destroy kernel last, make test ↵Andrew Chow
shutdown order consistent c1144f0076339c775f41d4b5fcfdc72191440d96 tests: Reset node context members on ~BasicTestingSetup (TheCharlatan) 9759af17ff7c28eb909cb73f62b78c90851ab74d shutdown: Destroy kernel last (TheCharlatan) Pull request description: The destruction/resetting of node context members in the tests should roughly follow the behavior of the `Shutdown` function in `init.cpp`. This was originally requested by MarcoFalke in this [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r890161249) in response to the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) introducing the `kernel::Context`. ACKs for top commit: maflcko: ACK c1144f0076339c775f41d4b5fcfdc72191440d96 🗣 achow101: ACK c1144f0076339c775f41d4b5fcfdc72191440d96 ryanofsky: Code review ACK c1144f0076339c775f41d4b5fcfdc72191440d96. No code changes since last review, just updated commits and descriptions Tree-SHA512: 819bb85ff82a5c6c60e429674d5684f3692fe9062500d00a87b361cc59e6bda145be21b5a4466dee6791faed910cbde4d26baab325bf6daa1813af13a63588ff
2023-11-07[validation] change package-fee-too-low, return wtxid(s) and effective feerateglozow
With subpackage evaluation and de-duplication, it's not always the entire package that is used in CheckFeerate. To be more helpful to the caller, specify which transactions were included in the evaluation and what the feerate was. Instead of PCKG_POLICY (which is supposed to be for package-wide errors), use PCKG_TX.
2023-11-03test: bugfix CheckPackageMempoolAcceptResult return all error stringsGreg Sanders
2023-11-03Merge bitcoin/bitcoin#28758: refactors for subpackage evaluationfanquake
b5a60abe8783852f5b31bc1e63b5836530410e65 MOVEONLY: CleanupTemporaryCoins into its own function (glozow) 10c0a8678cd28e7f0715e6cfa3e651903e4ad4aa [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow) 6ff647a7e0d85040a6033047c5cf84f8f22b1c65 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow) da9aceba217bbded6909f06144eaa1e1a4ebcb69 [refactor] move package checks into helper functions (glozow) Pull request description: This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253. - Split package sanitization in policy/packages.h into helper functions - Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597) - Rename `CheckPackage` to `IsPackageWellFormed` - Improve the `CreateValidTransaction` unit test utility to: - Configure the target feerate and return the fee paid - Signal BIP125 on transactions to enable RBF tests - Allow the specification of multiple inputs and outputs - Move `CleanupTemporaryCoins` into its own function to be reused later without duplication ACKs for top commit: dergoegge: Code review ACK b5a60abe8783852f5b31bc1e63b5836530410e65 instagibbs: ACK b5a60abe8783852f5b31bc1e63b5836530410e65 Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
2023-11-02[test util] CheckPackageMempoolAcceptResult for sanity-checking resultsglozow
2023-11-01[test util] CreateValidTransaction multi-in/out, configurable feerate, ↵glozow
signal BIP125 Support the creation of a transaction with multiple specified inputs or outputs. Also accept a target feerate and return the fee paid. Also, signal BIP125 by default - a subsequent commit needs to RBF something. Co-authored-by: Andrew Chow <achow101@gmail.com>
2023-10-30Allow unit tests to access additional CConnman membersJon Atack
that are otherwise private: - CConnman::m_nodes - CConnman::ConnectNodes() - CConnman::AlreadyConnectedToAddress() and update the #include headers per iwyu.
2023-10-24tests: Reset node context members on ~BasicTestingSetupTheCharlatan
The destruction/resetting of node context members in the tests should roughly follow the behaviour of the Shutdown function in `init.cpp`.
2023-10-12tidy: modernize-use-emplaceMarcoFalke
2023-10-05Merge bitcoin/bitcoin#28558: Make PeerManager own a FastRandomContextfanquake
4cafe9f176e93ebb6c38abb12140e8d8be005cbf [test] Make PeerManager's rng deterministic in tests (dergoegge) fecec3e1c661ba273470ecc5ef12d4c070b53050 [net processing] FeeFilterRounder doesn't own a FastRandomContext (dergoegge) 47520ed209d9341702a0fb6006bee6f63f7da42e [net processing] Make fee filter rounder non-global (dergoegge) 77506f4ac6b3a3d7396a3a6101345019e05b3b10 [net processing] Addr shuffle uses PeerManager's rng (dergoegge) a648dd79e5ebfdb627d0221b1207862efb664dfc [net processing] PushAddress uses PeerManager's rng (dergoegge) 87c706713e5d1c78bad943a42bf7c69047d28ea5 [net processing] PeerManager holds a FastRandomContext (dergoegge) Pull request description: This lets us avoid some non-determinism in tests (also see #28537). ACKs for top commit: MarcoFalke: re-ACK 4cafe9f176e93ebb6c38abb12140e8d8be005cbf 🕗 glozow: concept && light code review ACK 4cafe9f176e93ebb6c38abb12140e8d8be005cbf Tree-SHA512: 3c18700773d0bc547ccb6442c41567e6f26b0b50fab5b79620da417ec91b9c0ae1395d15258da3aa4a91447b8ce560145dd135e39fbbd0610749e528e665b111
2023-10-04[test] Make PeerManager's rng deterministic in testsdergoegge
2023-10-03Merge bitcoin/bitcoin#26312: Remove Sock::Get() and Sock::Sock()Ryan Ofsky
7df450836969b81e98322c9a09c08b35d1095a25 test: improve sock_tests/move_assignment (Vasil Dimov) 5086a99b84367a45706af7197da1016dd966e6d9 net: remove Sock default constructor, it's not necessary (Vasil Dimov) 7829272f7826511241defd34954e6040ea963f07 net: remove now unnecessary Sock::Get() (Vasil Dimov) 944b21b70ae490a5a746bcc1810a5074d74e9d34 net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov) aeac68d036e3cff57ce155f1a904d77f98b357d4 net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov) 5ac1a51ee5a57da59f1ff1986b7d9054484d3c80 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov) Pull request description: _This is a piece of #21878, chopped off to ease review._ Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary. The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it. ACKs for top commit: ajtowns: ACK 7df450836969b81e98322c9a09c08b35d1095a25 jonatack: ACK 7df450836969b81e98322c9a09c08b35d1095a25 Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
2023-10-03Make `CCheckQueue` destructor stop worker threadsHennadii Stepanov
2023-10-03Move global `scriptcheckqueue` into `ChainstateManager` classHennadii Stepanov
2023-10-02net: advertise NODE_P2P_V2 if CLI arg -v2transport is onPieter Wuille
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
2023-09-30validation: do not activate snapshot if behind active chainJames O'Beirne
Most easily reviewed with git show --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2023-09-30validation: pass ChainstateRole for validationinterface callsJames O'Beirne
This allows consumers to decide how to handle events from background or assumedvalid chainstates.
2023-09-15Merge bitcoin/bitcoin#28473: refactor: Serialization parameter cleanupsfanquake
fb6a2ab63e310d8b600352ef41aab6dafccfbff0 scripted-diff: use SER_PARAMS_OPFUNC (Anthony Towns) 5e5c8f86b60a8018e8801fb44bbe56ce97d9deef serialize: add SER_PARAMS_OPFUNC (Anthony Towns) 33203f59b482bddfe0bbe7d497cb8731ce8334a4 serialize: specify type for ParamsWrapper not ref (Anthony Towns) bf147bfffa1afb11721f30e83eec1fa829f64d5f serialize: move ser_action functions out of global namespace (Anthony Towns) Pull request description: Cleanups after #25284: * ser_action namespacing - https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1316189977 * make reference implicit - https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1316277030 * function notation - https://github.com/bitcoin/bitcoin/pull/25284#issuecomment-1710714821 ACKs for top commit: MarcoFalke: lgtm ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 💨 TheCharlatan: ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 Tree-SHA512: aacca2ee9cfec360ade6b394606e13d1dfe05bc29c5fbdd48a4e6992bd420312d4ed0d32218d95c560646af326e9977728dc2e759990636298e326947f6f9526
2023-09-14scripted-diff: use SER_PARAMS_OPFUNCAnthony Towns
-BEGIN VERIFY SCRIPT- sed -i 's/WithParams(\(CAddress::V[12]_[A-Z]*\) *, */\1(/g' $(git grep -l 'WithParams' src/) sed -i 's/WithParams(\(CNetAddr::V[12]\) *, */\1(/g' $(git grep -l 'WithParams' src/) sed -i 's@\(CNetAddr::V1.CService{}.*\) //@\1 //@' src/test/util/net.cpp -END VERIFY SCRIPT-
2023-09-12[refactor] Add CChainParams member to CConnmanTheCharlatan
This is done in preparation to the next commit, but has the nice effect of removing one further data structure relying on the global `Params()`.
2023-09-07net: add have_next_message argument to Transport::GetBytesToSend()Pieter Wuille
Before this commit, there are only two possibly outcomes for the "more" prediction in Transport::GetBytesToSend(): * true: the transport itself has more to send, so the answer is certainly yes. * false: the transport has nothing further to send, but if vSendMsg has more message(s) left, that still will result in more wire bytes after the next SetMessageToSend(). For the BIP324 v2 transport, there will arguably be a third state: * definitely not: the transport has nothing further to send, but even if vSendMsg has more messages left, they can't be sent (right now). This happens before the handshake is complete. To implement this, we move the entire decision logic to the Transport, by adding a boolean to GetBytesToSend(), called have_next_message, which informs the transport whether more messages are available. The return values are still true and false, but they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".
2023-09-07Merge bitcoin/bitcoin#25284: net: Use serialization parameters for CAddress ↵fanquake
serialization fa626af3edbe8d98b2de91dd71729ceef90389fb Remove unused legacy CHashVerifier (MarcoFalke) fafa3fc5a62702da72991497e3270034eb9159c0 test: add tests that exercise WithParams() (MarcoFalke) fac81affb527132945773a5315bd27fec61ec52f Use serialization parameters for CAddress serialization (MarcoFalke) faec591d64e40ba7ec7656cbfdda1a05953bde13 Support for serialization parameters (MarcoFalke) fac42e9d35f6ba046999b2e3a757ab720c51b6bb Rename CSerAction* to Action* (MarcoFalke) aaaa3fa9477eef9ea72e4a501d130c57b47b470a Replace READWRITEAS macro with AsBase wrapping function (MarcoFalke) Pull request description: It seems confusing that picking a wrong value for `ADDRV2_FORMAT` could have effects on consensus. (See the docstring of `ADDRV2_FORMAT`). Fix this by implementing https://github.com/bitcoin/bitcoin/issues/19477#issuecomment-1147421608 . This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327 ACKs for top commit: TheCharlatan: ACK fa626af3edbe8d98b2de91dd71729ceef90389fb ajtowns: ACK fa626af3edbe8d98b2de91dd71729ceef90389fb Tree-SHA512: 229d379da27308890de212b1fd2b85dac13f3f768413cb56a4b0c2da708f28344d04356ffd75bfcbaa4cabf0b6cc363c4f812a8f1648cff9e436811498278318
2023-09-05Merge bitcoin/bitcoin#28195: blockstorage: Drop legacy -txindex checkfanquake
fae405556d56f6f13ce57f69a06b9ec1e825422b scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke) faf63039cce40f5cf8dea5a1d24945773c3433a1 Fixup style of moved code (MarcoFalke) fa65111b99627289fd47dcfaa5197e0f09b8a50e move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke) fa8685597e7302fc136f21b6dd3a4b187fa8e251 index: Drop legacy -txindex check (MarcoFalke) fa69148a0a26c5054dbccdceeac8e117bf449275 scripted-diff: Use blocks_path where possible (MarcoFalke) Pull request description: The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check. Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242) ACKs for top commit: TheCharlatan: ACK fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251. stickies-v: ACK fae405556d56f6f13ce57f69a06b9ec1e825422b Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
2023-09-05Use serialization parameters for CAddress serializationMarcoFalke
This also cleans up the addrman (de)serialization code paths to only allow `Disk` serialization. Some unit tests previously forced a `Network` serialization, which does not make sense, because Bitcoin Core in production will always `Disk` serialize. This cleanup idea was suggested by Pieter Wuille and implemented by Anthony Towns. Co-authored-by: Pieter Wuille <pieter@wuille.net> Co-authored-by: Anthony Towns <aj@erisian.com.au>
2023-08-25net: remove Sock default constructor, it's not necessaryVasil Dimov
2023-08-23net: move message conversion to wire bytes from PushMessage to SocketSendDataPieter Wuille
This furthers transport abstraction by removing the assumption that a message can always immediately be converted to wire bytes. This assumption does not hold for the v2 transport proposed by BIP324, as no messages can be sent before the handshake completes. This is done by only keeping (complete) CSerializedNetMsg objects in vSendMsg, rather than the resulting bytes (for header and payload) that need to be sent. In SocketSendData, these objects are handed to the transport as permitted by it, and sending out the bytes the transport tells us to send. This also removes the nSendOffset member variable in CNode, as keeping track of how much has been sent is now a responsability of the transport. This is not a pure refactor, and has the following effects even for the current v1 transport: * Checksum calculation now happens in SocketSendData rather than PushMessage. For non-optimistic-send messages, that means this computation now happens in the network thread rather than the message handler thread (generally a good thing, as the message handler thread is more of a computational bottleneck). * Checksum calculation now happens while holding the cs_vSend lock. This is technically unnecessary for the v1 transport, as messages are encoded independent from one another, but is untenable for the v2 transport anyway. * Statistics updates about per-message sent bytes now happen when those bytes are actually handed to the OS, rather than at PushMessage time.
2023-08-23net: abstract sending side of transport serialization furtherPieter Wuille
This makes the sending side of P2P transports mirror the receiver side: caller provides message (consisting of type and payload) to be sent, and then asks what bytes must be sent. Once the message has been fully sent, a new message can be provided. This removes the assumption that P2P serialization of messages follows a strict structure of header (a function of type and payload), followed by (unmodified) payload, and instead lets transports decide the structure themselves. It also removes the assumption that a message must always be sent at once, or that no bytes are even sent on the wire when there is no message. This opens the door for supporting traffic shaping mechanisms in the future.
2023-08-23refactor: merge transport serializer and deserializer into Transport classPieter Wuille
This allows state that is shared between both directions to be encapsulated into a single object. Specifically the v2 transport protocol introduced by BIP324 has sending state (the encryption keys) that depends on received messages (the DH key exchange). Having a single object for both means it can hide logic from callers related to that key exchange and other interactions.
2023-08-22Merge bitcoin/bitcoin#28200: refactor: Remove unused includes from wallet.cppfanquake
fa6286891fa4164510e4fbf4bc214ce3033b2d1b Remove unused includes from wallet.cpp (MarcoFalke) fa8fdbe22932a4717d2bc4060269da9bff228728 Remove unused includes from blockfilter.h (MarcoFalke) fad8c36aa9011c3f7b1183f8380577e16a2167a6 move-only: Create src/kernel/mempool_removal_reason.h (MarcoFalke) fa5760880094c4e4238249f6d1837cd74383cc3a Remove unused includes from txmempool.h (MarcoFalke) Pull request description: This makes compilation of wallet.cpp use a few % less memory and time, locally. Created in the context of https://github.com/bitcoin/bitcoin/issues/28109, but I don't think it is enough to actually fix this problem. ACKs for top commit: hebasto: ACK fa6286891fa4164510e4fbf4bc214ce3033b2d1b, I have reviewed the code and it looks OK. Tree-SHA512: 06f1120af2a8ef3368dbd9ae747acda88ace2507bd261bcc10341d476a0b3d71c8485377ea6c108b47df3e4c13b7f75a15f486bafa6a8466303168dde16ebbc8
2023-08-18assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ↵Ryan Ofsky
ChainstateManager This change makes IsInitialBlockDownload and NotifyHeaderTip functions no longer tied to individual Chainstate objects. It makes them work with the ChainstateManager object instead so code is simpler and it is no longer possible to call them incorrectly with an inactive Chainstate. This change also makes m_cached_finished_ibd caching easier to reason about, because now there is only one cached value instead of two (for background and snapshot chainstates) so the cached IBD state now no longer gets reset when a snapshot is loaded. There should be no change in behavior because these functions were always called on the active ChainState objects. These changes were discussed previously https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905 and https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792 as possible followups for that PR.
2023-08-17Remove unused includes from blockfilter.hMarcoFalke
This removes unused includes, primitives/block found manually, and the others by iwyu: blockfilter.h should remove these lines: - #include <serialize.h> // lines 16-16 - #include <undo.h> // lines 18-18
2023-08-17Merge bitcoin/bitcoin#28244: Break up script/standard.{h/cpp}fanquake
91d924ede1b421df31c895f4f43359e453a09ca5 Rename script/standard.{cpp/h} to script/solver.{cpp/h} (Andrew Chow) bacdb2e208531124e85ed2d4ea2a4b508fbb5088 Clean up script/standard.{h/cpp} includes (Andrew Chow) f3c9078b4cddec5581e52de5c216ae53984ec130 Clean up things that include script/standard.h (Andrew Chow) 8bbe257bac751859a272ddf52dc0328c1b5a1ede MOVEONLY: Move datacarrier defaults to policy.h (Andrew Chow) 7a172c76d2361fc3cdf6345590e26c79a7821672 Move CTxDestination to its own file (Andrew Chow) 145f36ec81e79d2e391847520364c2420ef0e0e8 Move Taproot{SpendData/Builder} to signingprovider.{h/cpp} (Andrew Chow) 86ea8bed5473f400f7a93fcc455393a574a2f319 Move CScriptID to script.{h/cpp} (Andrew Chow) b81ebff0d99c45c071b999796b8ae3f0f2517b22 Remove ScriptHash from CScriptID constructor (Andrew Chow) cba69dda3da0e4fa39cff5ce4dc81d1242fe651b Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/policy.h (Anthony Towns) Pull request description: Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything. * `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}` * `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to `SigningProvider` as these are used only during signing. * `CScriptID` is moved to `script/script.h` to be next to `CScript`. * `MANDATORY_SCRIPT_VERIFY_FLAGS` is moved to `interpreter.h` * The parameters `DEFAULT_ACCEPT_DATACARRIER` and `MAX_OP_RETURN_RELAY` are moved to `policy.h` * `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above moves ACKs for top commit: Sjors: ACK 91d924ede1b421df31c895f4f43359e453a09ca5 ajtowns: ACK 91d924ede1b421df31c895f4f43359e453a09ca5 MarcoFalke: ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇 murchandamus: ACK 91d924ede1b421df31c895f4f43359e453a09ca5 darosior: Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5. theStack: Code-review ACK 91d924ede1b421df31c895f4f43359e453a09ca5 Tree-SHA512: d347439890c652081f6a303d99b2bde6c371c96e7f4127c5db469764a17d39981f19884679ba883e28b733fde6142351dd8288c7bc61c379b7eefe7fa7acca1a
2023-08-17Merge bitcoin/bitcoin#27675: p2p: Drop m_recently_announced_invs bloom filterfanquake
fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 mempool_entry: improve struct packing (Anthony Towns) 1a118062fbc4ec8f645f4ec4298d869a869c3344 net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns) 6fa49937e488d0924044786c76b42324b659f351 test: Check tx from disconnected block is immediately requestable (glozow) e4ffabbffacc4b890d393aafcc8286916ef887d8 net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns) 6ec1809d33bfc42b80cb6f35625dccd56be8d507 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns) a70beafdb22564043dc24fc98133fdadbaf77d8a validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns) 1e9684f39fba909b3501e9402d5b61f4bf744ff2 mempool_entry: add mempool entry sequence number (Anthony Towns) Pull request description: This PR replaces the `m_recently_announced_invs` bloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally). The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. We likewise consider the BIP 133 feefilter and BIP 37 bloom filters to be bandwidth optimisations here, and treat transactions as requestable if they would have been announced without those filters. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of `m_recently_announced_invs`, `m_last_mempool_req` and `UNCONDITIONAL_RELAY_DELAY` and associated logic. ACKs for top commit: naumenkogs: ACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 amitiuttarwar: review ACK fb02ba3c5f5 glozow: reACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 Tree-SHA512: cbba5ee04c86df26b6057f3654c00a2b45ec94d354f4f157a769cecdaa0b509edaac02b3128afba39b023e82473fc5e28c915a787f84457ffe66638c6ac9c2d4
2023-08-14Clean up things that include script/standard.hAndrew Chow
Remove standard.h from files that don't use anything in it, and include it in files that do.
2023-08-07Merge bitcoin/bitcoin#28186: kernel: Prune leveldb headersfanquake
d8f1222ac50f089a0af29eaf8ce0555bad8366ef refactor: Correct dbwrapper key naming (TheCharlatan) be8f159ac59b9e700cbd3314ed71ebf39bd5b67a build: Remove leveldb from BITCOIN_INCLUDES (TheCharlatan) c95b37d641b1eed4a62d55ca5342a6ed8c7a1ce7 refactor: Move CDBWrapper leveldb members to their own context struct (TheCharlatan) c534a615e93452a5f509aaf5f68c600391a98d6a refactor: Split dbwrapper CDBWrapper::EstimateSize implementation (TheCharlatan) 586448888b72f7c87db4dcd30fc4e4044afae13b refactor: Move HandleError to dbwrapper implementation (TheCharlatan) dede0eef7adb7413f62f5abd68cac8e01635ba4a refactor: Split dbwrapper CDBWrapper::Exists implementation (TheCharlatan) a5c2eb57484314b04ec94523d14e0ef0c6c46d4f refactor: Fix logging.h includes (TheCharlatan) 84058e0eed9c05bc30984b39131e88ad1425628f refactor: Split dbwrapper CDBWrapper::Read implementation (TheCharlatan) e4af2408f2ac59788567b6fc8cb3a68fc43da9fe refactor: Pimpl leveldb::Iterator for CDBIterator (TheCharlatan) ef941ff1281e76308c3e746e592375bec023e9e4 refactor: Split dbwrapper CDBIterator::GetValue implementation (TheCharlatan) b7a1ab5cb4e60230f62c94efb3a10d07c9af4883 refactor: Split dbwrapper CDBIterator::GetKey implementation (TheCharlatan) d7437908cdf242626263ba9d5541addcddadc594 refactor: Split dbwrapper CDBIterator::Seek implementation (TheCharlatan) ea8135de7e617259cda3fc7b1c8e7569d454fd57 refactor: Pimpl leveldb::batch for CDBBatch (TheCharlatan) b9870c920dc475ec759eaf7339ea42aecba92138 refactor: Split dbwrapper CDBatch::Erase implementation (TheCharlatan) 532ee812a499e13b123af6b8415d8de1f3804f0f refactor: Split dbwrapper CDBBatch::Write implementation (TheCharlatan) afc534df9adbf5599b286b5dc3531a4b9ac2d056 refactor: Wrap DestroyDB in dbwrapper helper (TheCharlatan) Pull request description: Leveldb headers are currently included in the `dbwrapper.h` file and thus available to many of Bitcoin Core's source files. However, leveldb-specific functionality should be abstracted by the `dbwrapper` and does not need to be available to the rest of the code. Having leveldb included in a widely-used header such as `dbwrapper.h` bloats the entire project's header tree. The `dbwrapper` is a key component of the libbitcoinkernel library. Future users of this library would not want to contend with having the leveldb headers exposed and potentially polluting their project's namespace. For these reasons, the leveldb headers are removed from the `dbwrapper` by moving leveldb-specific code to the implementation file and creating a [pimpl](https://en.cppreference.com/w/cpp/language/pimpl) where leveldb member variables are indispensable. As a final step, the leveldb include flags are removed from the `BITCOIN_INCLUDES` and moved to places where the dbwrapper is compiled. --- This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), and more specifically its stage 1 step 3 "Decouple most non-consensus headers from libbitcoinkernel". ACKs for top commit: stickies-v: re-ACK https://github.com/bitcoin/bitcoin/commit/d8f1222ac50f089a0af29eaf8ce0555bad8366ef MarcoFalke: ACK d8f1222ac50f089a0af29eaf8ce0555bad8366ef 🔠 Tree-SHA512: 0f58309be165af0162e648233451cd80fda88726fc10c0da7bfe4ec2ffa9afe63fbf7ffae9493698d3f39653b4ad870c372eee652ecc90ab1c29d86c387070f3
2023-08-07Merge bitcoin/bitcoin#26296: ci: Integrate `bitcoin-tidy` clang-tidy pluginfanquake
1c976c691cc4b20f43071aabf36c7afed1571057 tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake) 7de23cceb8ac13fcc709453ef0fa14fb93c460b0 refactor: fix unterminated LogPrintf()s (fanquake) 0a1029aa2946787bfe91a84de882c2dd0de076e9 lint: remove /* Continued */ markers from codebase (fanquake) 910007995d8603ffc466878856227153a638caff lint: remove lint-logs.py (fanquake) d86a83d6b8587b0971e66c6910af23dd8c042969 lint: drop DIR_IWYU global (fanquake) Pull request description: Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job. The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter. ACKs for top commit: TheCharlatan: ACK 1c976c691cc4b20f43071aabf36c7afed1571057 theuni: ACK 1c976c691cc4b20f43071aabf36c7afed1571057 MarcoFalke: re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 👠 Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8
2023-08-06Merge bitcoin/bitcoin#27213: p2p: Diversify automatic outbound connections ↵fanquake
with respect to networks 1b52d16d07be3b5d968157913f04d9cd1e2d3678 p2p: network-specific management of outbound connections (Martin Zumsande) 65cff00ceea48ac8a887ffea79aedb4251aa097f test: Add test for outbound protection by network (Martin Zumsande) 034f61f83b9348664d868933dbbfd8f9f8882168 p2p: Protect extra full outbound peers by network (Martin Zumsande) 654d9bc27647fb3797001472e2464dededb45d3f p2p: Introduce data struct to track connection counts by network (Amiti Uttarwar) Pull request description: This is joint work with mzumsande. This is a proposal to diversify outbound connections with respect to reachable networks. The existing logic evaluates peers for connection based purely on the frequency of available addresses in `AddrMan`. This PR adds logic to automatically connect to alternate reachable networks and adds eviction logic that protects one existing connection to each network. For instance, if `AddrMan` is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won't establish any automatic outbound connections to Tor, even if we're capable of doing so. For smaller networks like CJDNS, this is even more of an issue and often requires adding manual peers to ensure regularly being connected to the network. Connecting to multiple networks improves resistance to eclipse attacks for individual nodes. It also benefits the entire p2p network by increasing partition resistance and privacy in general. The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration that bitcoind cannot detect. Once an additional peer is added and we have one more outbound connection than we want, outbound eviction ensures that peers are protected if they are the only ones for their network. Manual connections are also taken into account: If a user already establishes manual connections to a trusted peer from a network, there is no longer a need to make extra efforts to ensure we also have an automatic connection to it (although this may of course happen by random selection). ACKs for top commit: naumenkogs: ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678 vasild: ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678 Tree-SHA512: 5616c038a5fbb868d4c46c5963cfd53e4599feee25db04b0e18da426d77d22e0994dc4e1da0b810f5b457f424ebbed3db1704f371aa6cad002b3565b20170ec0
2023-08-05refactor: Fix logging.h includesTheCharlatan
These were uncovered as missing by the next commit.
2023-08-03p2p: Introduce data struct to track connection counts by networkAmiti Uttarwar
Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and MANUAL connections. Unused until next commit. Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
2023-08-03refactor: fix unterminated LogPrintf()sfanquake
2023-08-03mempool_entry: add mempool entry sequence numberAnthony Towns
2023-08-02scripted-diff: Rename CBlockTreeDB -> BlockTreeDBMarcoFalke
-BEGIN VERIFY SCRIPT- sed -i 's|CBlockTreeDB|BlockTreeDB|g' $( git grep -l CBlockTreeDB ) -END VERIFY SCRIPT-
2023-08-01move-only: Move CBlockTreeDB to node/blockstorageMarcoFalke
The block index (CBlockTreeDB) is required to write and read blocks, so move it to blockstorage. This allows to drop the txdb.h include from `node/blockstorage.h`. Can be reviewed with: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
2023-07-31Merge bitcoin/bitcoin#27746: Rework validation logic for assumeutxoRyan Ofsky
a733dd79e29068ad1e0532ac42a45188a040a7b9 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar) d4a11abb1972b54f0babdccfbb2fde97ab885933 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar) 3556b850221bc0e597d7dd749d4d47ab58dc8083 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar) 0ce805b632dcb98944a931f758f76f530f5ce5f2 Documentation improvements for assumeutxo (Ryan Ofsky) 768690b7ce551cd403f8e2a099372915f6022ad4 Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar) d43a1f1a2fa35d377c7a9ad7ab92d1ae325bde3d Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar) d0d40ea9a6478d81d7531b7cfc52a8bdaa0883d6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar) 3cfc75366e6596942cbc84f354f42dfd7fc5c073 test: Clear block index flags when testing snapshots (Suhas Daftuar) 272fbc370c4e133d31d9f1d34e327cc265c5fad2 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar) 10c05710ce1602d932037f72dc6c4bbc3f6f34ba Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar) 471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar) 1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (Suhas Daftuar) fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (Suhas Daftuar) Pull request description: This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation. This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates. Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well. ACKs for top commit: achow101: ACK a733dd79e29068ad1e0532ac42a45188a040a7b9 jamesob: reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic)) Sjors: Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. ryanofsky: Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge. Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
2023-07-24[net processing] Introduce PeerManager optionsdergoegge
2023-07-19Merge bitcoin/bitcoin#27425: test: move remaining rand code from ↵fanquake
util/setup_common to util/random 1cd45d4e08c3dfd1d6423620c79169f1404ac12b test: move random.h include header from setup_common.h to cpp (Jon Atack) 1b246fdd145a95f5da479159f5e8eaf5a76bdc3a test: move remaining random test util code from setup_common to random (jonatack) Pull request description: and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140 by glozow (thanks!) ACKs for top commit: MarcoFalke: lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂 Tree-SHA512: 6ce63d9103ba9b04eebbd8ad02fe9aa79e356296533404034a1ae88e9b7ca0bc9a5c51fd754b71cf4e7b55b18bcd4d5474b2d588edee3851e3b3ce0e4d309a93
2023-07-14test: Clear block index flags when testing snapshotsSuhas Daftuar
When simulating a snapshot, remove the HAVE_DATA status for blocks below the snapshot height, to simulate never having downloaded them at all. This makes tests more realistic (and more closely match what will happen when using assumeutxo).
2023-07-11Merge bitcoin/bitcoin#28044: test: indexes, fix on error infinite loopRyan Ofsky
89ba8905f5c68ae29412f9c4010314c5a113c234 test: indexes, fix on error infinite loop (furszy) Pull request description: Coming from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703, I thought that we were going to fix it there but seems that got merged without it for some reason. As index sync failures trigger a shutdown request without notifying `BaseIndex::BlockUntilSyncedToCurrentChain` in any way, we also need to check whether a shutdown was requested or not inside 'IndexWaitSynced'. Otherwise, any error inside the index sync process will hang the test forever. ACKs for top commit: MarcoFalke: lgtm ACK 89ba8905f5c68ae29412f9c4010314c5a113c234 jamesob: ACK 89ba890 ryanofsky: Code review ACK 89ba8905f5c68ae29412f9c4010314c5a113c234. Just comment update since last review Tree-SHA512: 1f6daf34e51d3fbc802799bfa4ac0ef0d8f774db5f9e2f5d35df18a77679778475c94efc3da1fb723ebaf3583e4075e4a5cbe4a5104ad0c50e2b32076e247b29
generated by cgit v1.2.3 (git 2.26.2) at 2025-03-16 21:59:16 +0000