aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2024-02-01wallet: Keep txs that belong to both watchonly and migrated walletsAva Chow
It is possible for a transaction that has an output that belongs to the mgirated wallet, and another output that belongs to the watchonly wallet. Such transaction should appear in both wallets during migration.
2024-02-01wallet: Reload the wallet if migration exited earlyAva Chow
Migration will unload loaded wallets prior to beginning. It will then perform some checks which may exit early. Such unloaded wallets should be reloaded prior to exiting.
2024-02-01wallet: Write bestblock to watchonly and solvable walletsAva Chow
When migrating, we should also be writing the bestblock record to the watchonly and solvable wallets to avoid rescanning on the reload as that can be slow.
2024-02-01Merge bitcoin/bitcoin#29275: refactor: Fix prevector iterator concept issuesfanquake
fad74bbbd0ab4425573f182ebde1b31a99e80547 refactor: Mark prevector iterator with std::contiguous_iterator_tag (MarcoFalke) fab8a01048fc6cfcee7e89884a5af31385189c63 refactor: Fix binary operator+ for prevector iterators (MarcoFalke) fa44a60b2bb5cb91bc411e5b625fc81bd84befff refactor: Fix constness for prevector iterators (MarcoFalke) facaa66b49ef7eeefe1d57c1bfc1bbe5b3011661 refactor: Add missing default constructor to prevector iterators (MarcoFalke) Pull request description: Currently prevector iterators have many issues: * Forward iterators (and stronger) must be default constructible (https://eel.is/c++draft/forward.iterators#1.2). Otherwise, some functions can not be instantiated, like `std::minmax_element`. * Various `const` issues with random access iterators. For example, a `const iterator` is different from a `const_iterator`, because the first one holds a mutable reference and must also return it without `const`. Also, `operator+` must be callable regardless of the iterator object's `const`-ness. * When adding an offset to random access iterators, both `x+n` and `n+x` must be specified, see https://eel.is/c++draft/random.access.iterators#tab:randomaccessiterator Fix all issues. Also, upgrade the `std::random_access_iterator_tag` (C++17) to `std::contiguous_iterator_tag` (C++20) ACKs for top commit: TheCharlatan: ACK fad74bbbd0ab4425573f182ebde1b31a99e80547 stickies-v: ACK fad74bbbd0ab4425573f182ebde1b31a99e80547 willcl-ark: ACK fad74bbbd0ab4425573f182ebde1b31a99e80547 Tree-SHA512: b1ca778a31602af94b323b8feaf993833ec78be09f1d438a68335485a4ba97f52125fdd977ffb9541b89f8d45be0105076aa07b5726936133519aae832556e0b
2024-02-01refactor: Fix timedata includesMarcoFalke
2024-01-31Merge bitcoin/bitcoin#26859: fuzz: extend ConsumeNetAddr() to return I2P and ↵Ava Chow
CJDNS addresses b851c5385d0a0acec4493be1561cea285065d5dc fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses (Vasil Dimov) Pull request description: In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`. ACKs for top commit: achow101: ACK b851c5385d0a0acec4493be1561cea285065d5dc mzumsande: ACK b851c5385d0a0acec4493be1561cea285065d5dc brunoerg: utACK b851c5385d0a0acec4493be1561cea285065d5dc Tree-SHA512: 9905acff0e996f30ddac0c14e5ee9e1db926c7751472c06d6441111304242b563f7c942b162b209d80e8fb65a97249792eef9ae0a96100419565bf7f59f59676
2024-01-31Merge bitcoin/bitcoin#29301: init: settings, do not load auto-generated ↵Ava Chow
warning msg 987a1b51eeb72c7fcb085470817a4fe85fcc3c7c init: settings, do not load auto-generated warning msg (furszy) Pull request description: Fixes https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1907071391. The settings warning message is meant to be used only to discourage users from modifying the file manually. Therefore, there is no need to keep it in memory. ACKs for top commit: achow101: ACK 987a1b51eeb72c7fcb085470817a4fe85fcc3c7c ryanofsky: Code review ACK 987a1b51eeb72c7fcb085470817a4fe85fcc3c7c. Seems like a clean, simple fix Tree-SHA512: 3f2bdcf4b4a9cadb396dcff9b43155211eeed018527a07356970a341d139ad18edbd1a4d369377c8907b8ec1f19ee2ab8aacf85a887379e6d57a8a6db2403d51
2024-01-31Merge bitcoin/bitcoin#28976: wallet: Fix migration of blank walletsRyan Ofsky
c11c404281d2d0e22967e30e16c0733db84f4eee tests: Test migration of blank wallets (Andrew Chow) 563b2a60d6a371f26474410397da435547e58786 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow) b1d2c771d43b802db12768e620588ed179e92b29 wallet: Check for descriptors flag before migration (Andrew Chow) 8c127ff1edb6b9a607bf1ad247893347252a38e3 wallet: Skip key and script migration for blank wallets (Andrew Chow) Pull request description: Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets. Fixes the issue mentioned in https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110 ACKs for top commit: furszy: reACK c11c404281d2d0e22967e30e16c0733db84f4eee. CI failure is unrelated. ryanofsky: Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4
2024-01-31Merge bitcoin/bitcoin#28956: Nuke adjusted time from validation (attempt 2)Ava Chow
ff9039f6ea876bab2c40a06a93e0dd087f445fa2 Remove GetAdjustedTime (dergoegge) Pull request description: This picks up parts of #25908. The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains. ACKs for top commit: naumenkogs: ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 achow101: ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 maflcko: lgtm ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 🤽 stickies-v: ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
2024-01-31Merge bitcoin/bitcoin#29347: net: enable v2transport by defaultAva Chow
0bef1042ce6c459acb1de965cbccd98867a417f1 net: enable v2transport by default (Pieter Wuille) Pull request description: This enables BIP324's v2 transport by default (see #27634): * Inbound connections will auto-sense whether v1 or v2 is in use. * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure. * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure. It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node. ACKs for top commit: stratospher: ACK 0bef104. josibake: reACK https://github.com/bitcoin/bitcoin/pull/29347/commits/0bef1042ce6c459acb1de965cbccd98867a417f1 achow101: ACK 0bef1042ce6c459acb1de965cbccd98867a417f1 naumenkogs: ACK 0bef1042ce6c459acb1de965cbccd98867a417f1 theStack: ACK 0bef1042ce6c459acb1de965cbccd98867a417f1 willcl-ark: crACK 0bef1042ce6c459acb1de965cbccd98867a417f1 BrandonOdiwuor: utACK 0bef1042ce6c459acb1de965cbccd98867a417f1 pablomartin4btc: re ACK 0bef1042ce6c459acb1de965cbccd98867a417f1 kristapsk: utACK 0bef1042ce6c459acb1de965cbccd98867a417f1 Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
2024-01-31Merge bitcoin/bitcoin#29253: wallet: guard against dangling to-be-reverted ↵Ava Chow
db transactions b298242c8d495c36072415e1b95eaa7bf485a38a test: sqlite, add coverage for dangling to-be-reverted db txns (furszy) fc0e747192e98e779c5f31f2df808f62b3fdd071 sqlite: guard against dangling to-be-reverted db transactions (furszy) 472d2ca98170049e0edec830e2d11c5ef23740a4 sqlite: introduce HasActiveTxn method (furszy) dca874e838c2599bd24433675b291168f8e7b055 sqlite: add ability to interrupt statements (furszy) fdf9f66909a354a95f4b7c5f092f0e9fbe1baa7c test: wallet db, exercise deadlock after write failure (furszy) Pull request description: Discovered while was reviewing #29112, specifically https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931. If the db handler that initiated the database transaction is destroyed, the ongoing transaction cannot be left dangling when the db txn fails to abort. It must be forcefully reverted; otherwise, any subsequent db handler executing a write operation will dump the dangling, to-be-reverted transaction data to disk. This not only breaks the isolation property but also results in the improper storage of incomplete information on disk, impacting the wallet consistency. This PR fixes the issue by resetting the db connection, automatically rolling back the transaction (per https://www.sqlite.org/c3ref/close.html) when the handler object is being destroyed and the txn abortion failed. Testing Notes Can verify the failure by reverting the fix e5217fea and running the test. It will fail without e5217fea and pass with it. ACKs for top commit: achow101: ACK b298242c8d495c36072415e1b95eaa7bf485a38a ryanofsky: Code review ACK b298242c8d495c36072415e1b95eaa7bf485a38a. Just fix for exec result codes and comment update since last review. Tree-SHA512: 44ba0323ab21440e79e9d7791bc1c56a8873c8bd3e8f6a85641b91576e1293011fa8032d8ae5b0580f3fb7a949356f7b9676693d7ceffa617aaad9f6569993eb
2024-01-31Don't use scientific notation in log messagesKristaps Kaupe
2024-01-31[test] make v2transport arg in addconnection mandatory and few cleanupsstratospher
`TestNode::add_outbound_p2p_connection()` is the only place where addconnection test-only RPC is used. here, we always pass the appropriate v2transport option to addconnection RPC. currently the v2transport option for addconnection RPC is optional. so simply make the v2transport option mandatory instead.
2024-01-31Merge bitcoin/bitcoin#28170: p2p: adaptive connections services flagsAva Chow
27f260aa6e04f82dad78e9a06d58927546143a27 net: remove now unused global 'g_initial_block_download_completed' (furszy) aff7d92b1500e2478ce36a7e86ae47df47dda178 test: add coverage for peerman adaptive connections service flags (furszy) 6ed53602ac7c565273b5722de167cb2569a0e381 net: peer manager, dynamically adjust desirable services flag (furszy) 9f36e591c551ec2e58a6496334541bfdae8fdfe5 net: move state dependent peer services flags (furszy) f9ac96b8d6f4eba23c88f302b22a2c676e351263 net: decouple state independent service flags from desirable ones (furszy) 97df4e38879d2644aeec34c1eef241fed627333e net: store best block tip time inside PeerManager (furszy) Pull request description: Derived from #28120 discussion. By relocating the peer desirable services flags into the peer manager, we allow the connections acceptance process to handle post-IBD potential stalling scenarios. The peer manager will be able to dynamically adjust the services flags based on the node's proximity to the tip (back and forth). Allowing the node to recover from the following post-IBD scenario: Suppose the node has successfully synced the chain, but later experienced dropped connections and remained inactive for a duration longer than the limited peers threshold (the timeframe within which limited peers can provide blocks). In such cases, upon reconnecting to the network, the node might only establish connections with limited peers, filling up all available outbound slots. Resulting in an inability to synchronize the chain (because limited peers will not provide blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold). ACKs for top commit: achow101: ACK 27f260aa6e04f82dad78e9a06d58927546143a27 vasild: ACK 27f260aa6e04f82dad78e9a06d58927546143a27 naumenkogs: ACK 27f260aa6e04f82dad78e9a06d58927546143a27 mzumsande: Light Code Review ACK 27f260aa6e04f82dad78e9a06d58927546143a27 andrewtoth: ACK 27f260aa6e04f82dad78e9a06d58927546143a27 Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953
2024-01-31test: Assumeutxo with more than just coinbase transactionsMarcoFalke
2024-01-30test: sqlite, add coverage for dangling to-be-reverted db txnsfurszy
2024-01-30sqlite: guard against dangling to-be-reverted db transactionsfurszy
If the handler that initiated the database transaction is destroyed, the ongoing transaction cannot be left dangling when the db txn fails to abort. It must be forcefully reversed; otherwise, any subsequent db handler executing a write operation will dump the dangling, to-be-reverted transaction data to disk. This not only breaks the database isolation property but also results in the improper storage of incomplete information on disk, impacting the wallet consistency.
2024-01-30sqlite: introduce HasActiveTxn methodfurszy
Util function to clean up code and let us verify, in the following-up commit, that dangling, to-be-reverted db transactions cannot occur anymore.
2024-01-30sqlite: add ability to interrupt statementsfurszy
By encapsulating sqlite3_exec into its own standalone method and introducing the 'SQliteExecHandler' class, we enable the ability to test db statements execution failures within the unit test framework. This is used in the following-up commit to exercise a deadlock and improve our wallet db error handling code. Moreover, the future encapsulation of other sqlite functions within this class will contribute to minimize the impact of any future API changes.
2024-01-30Merge bitcoin/bitcoin#29308: doc: update `BroadcastTransaction` commentglozow
31cce4a1bdbb48f57996615ee6c686e456cc0bea doc: update `BroadcastTransaction` comment (ismaelsadeeq) Pull request description: `BroadcastTransaction` is also called by `submitpackage` RPC. All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926 It's not maintainable to list all the callers of a function. ACKs for top commit: stickies-v: ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea kristapsk: ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea naumenkogs: ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
2024-01-30Merge bitcoin/bitcoin#29299: validation: fix misleading checkblockindex commentsglozow
9819db4ccaa03519a78d4d9ecce9f89f5be669e5 validation: move nChainTx assert down in CheckBlockIndex (Martin Zumsande) 033477dba65151b1863214606d5a49686ab6f559 doc: fix checkblockindex comments (Martin Zumsande) Pull request description: The two assumptions there were described as test-only, which has led to confusion whether they should exist. However, they are necessary in general, as the changed comment explains - without them, the check would fail everywhere where it is enabled. The second commit moves this assert down to the other checks. Closes #29261 ACKs for top commit: maflcko: ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5 🌦 naumenkogs: ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5 ryanofsky: Code review ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5. Thanks for figuring this issue out and fixing it. Would suggest changing pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed. Tree-SHA512: 3f77791253eb0c97f8153dd8ae1c567f43f6387ea7a53efea94817463c672a4e11d548aa7eff62235346ff0713ff4d6fe08f9ec50d0c30a1e6b6d27b9918b419
2024-01-29net: enable v2transport by defaultPieter Wuille
2024-01-29Merge bitcoin/bitcoin#24748: test/BIP324: functional tests for v2 P2P encryptionAva Chow
bc9283c4415a932ec1eeb70ca2aa4399c80437b3 [test] Add functional test to test early key response behaviour in BIP 324 (stratospher) ffe6a56d75c0b47d0729e4e0b7225a827b43ad89 [test] Check whether v2 TestNode performs downgrading (stratospher) ba737358a37438c18f0fba723eab10ccfd9aae9b [test] Add functional tests to test v2 P2P behaviour (stratospher) 4115cf995647d1a513caecb54a4ff3f51927aa8e [test] Ignore BIP324 decoy messages (stratospher) 8c054aa04d33b247744b3747cd5bf3005a013e90 [test] Allow inbound and outbound connections supporting v2 P2P protocol (stratospher) 382894c3acd2dbf3e4198814f547c75b6fb17706 [test] Reconnect using v1 P2P when v2 P2P terminates due to magic byte mismatch (stratospher) a94e350ac0e5b65ef23a84b05fb10d1204c98c97 [test] Build v2 P2P messages (stratospher) bb7bffed799dc5ad8b606768164fce46d4cbf9d0 [test] Use lock for sending P2P messages in test framework (stratospher) 5b91fb14aba7d7fe45c9ac364526815bec742356 [test] Read v2 P2P messages (stratospher) 05bddb20f5cc9036fd680500bde8ece70dbf0646 [test] Perform initial v2 handshake (stratospher) a049d1bd08c8cdb3b693520f24f8a82572dcaab1 [test] Introduce EncryptedP2PState object in P2PConnection (stratospher) b89fa59e715a185d9fa7fce089dad4273d3b1532 [test] Construct class to handle v2 P2P protocol functions (stratospher) 8d6c848a48530893ca40be5c1285541b3e7a94f3 [test] Move MAGIC_BYTES to messages.py (stratospher) 595ad4b16880ae1f23463ca9985381c8eae945d8 [test/crypto] Add ECDH (stratospher) 4487b8051797173c7ab432e75efa370afb03b529 [rpc/net] Allow v2 p2p support in addconnection (stratospher) Pull request description: This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same. ### commits overview 1. introduces a new class `EncryptedP2PState` to store the keys, functions for performing the initial v2 handshake and encryption/decryption. 3. this class is used by `P2PConnection` in inbound/outbound connections to perform the initial v2 handshake before the v1 version handshake. Only after the initial v2 handshake is performed do application layer P2P messages(version, verack etc..) get exchanged. (in a v2 connection) - `v2_state` is the object of class `EncryptedP2PState` in `P2PConnection` used to store its keys, session-id etc. - a node [advertising](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#advertising-to-support-v2-p2p) support for v2 P2P is different from a node actually [supporting v2 P2P](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#supporting-v2-p2p) (differ when false advertisement of services occur) - introduce a boolean variable `supports_v2_p2p` in `P2PConnection` to denote if it supports v2 P2P. - introduce a boolean variable `advertises_v2_p2p` to denote whether `P2PConnection` which mimics peer behaviour advertises V2 P2P support. Default option is `False`. - In the test framework, you can create Inbound and Outbound connections to `TestNode` 1. During **Inbound Connections**, `P2PConnection` is the initiator [`TestNode` <--------- `P2PConnection`] - Case 1: - if the `TestNode` advertises/signals v2 P2P support (means `self.nodes[i]` set up with `"-v2transport=1"`), different behaviour will be exhibited based on whether: 1. `P2PConnection` supports v2 P2P 2. `P2PConnection` does not support v2 P2P - In a real world scenario, the initiator node would intrinsically know if they support v2 P2P based on whatever code they choose to run. However, in the test scenario where we mimic peer behaviour, we have no way of knowing if `P2PConnection` should support v2 P2P or not. So `supports_v2_p2p` boolean variable is used as an option to enable support for v2 P2P in `P2PConnection`. - Since the `TestNode` advertises v2 P2P support (using "-v2transport=1"), our initiator `P2PConnection` would send: 1. (if the `P2PConnection` supports v2 P2P) ellswift + garbage bytes to initiate the connection 2. (if the `P2PConnection` does not support v2 P2P) version message to initiate the connection - Case 2: - if the `TestNode` doesn't signal v2 P2P support; `P2PConnection` being the initiator would send version message to initiate a connection. 2. During **Outbound Connections** [TestNode --------> P2PConnection] - initiator `TestNode` would send: - (if the `P2PConnection` advertises v2 P2P) ellswift + garbage bytes to initiate the connection - (if the `P2PConnection` advertises v2 P2P) version message to initiate the connection - Suppose `P2PConnection` advertises v2 P2P support when it actually doesn't support v2 P2P (false advertisement scenario) - `TestNode` sends ellswift + garbage bytes - `P2PConnection` receives but can't process it and disconnects. - `TestNode` then tries using v1 P2P and sends version message - `P2PConnection` receives/processes this successfully and they communicate on v1 P2P 4. the encrypted P2P messages follow a different format - 3 byte length + 1-13 byte message_type + payload + 16 byte MAC 5. includes support for testing decoy messages and v2 connection downgrade(using false advertisement - when a v2 node makes an outbound connection to a node which doesn't support v2 but is advertised as v2 by some malicious intermediary) ### run the tests * functional test - `test/functional/p2p_v2_encrypted.py` `test/functional/p2p_v2_earlykeyresponse.py` I'm also super grateful to @ dhruv for his really valuable feedback on this branch. Also written a more elaborate explanation here - https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md ACKs for top commit: naumenkogs: ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3 mzumsande: Code Review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3 theStack: Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3 glozow: ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3 Tree-SHA512: 9b54ed27e925e1775e0e0d35e959cdbf2a9a1aab7bcf5d027e66f8b59780bdd0458a7a4311ddc7dd67657a4a2a2cd5034ead75524420d58a83f642a8304c9811
2024-01-29doc: update `BroadcastTransaction` commentismaelsadeeq
BroadcastTransaction is also called by submitpackage RPC. It's not maintainable to list all the callers of a function.
2024-01-26Merge bitcoin/bitcoin#29180: crypto: remove use of BUILD_BITCOIN_INTERNAL ↵Ava Chow
macro in sha256 bbf218d06164b7247f5e9df5ba143383022fbf74 crypto: remove sha256_sse4 from the base crypto helper lib (Cory Fields) 4dbd0475d8c16ed10c309bf6badc17f2d2eaaa69 crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 (Cory Fields) Pull request description: Replace it with a more explicit `DISABLE_OPTIMIZED_SHA256` and clean up some. The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity. Also remove the `BUILD_BITCOIN_INTERNAL` define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports. Removing the define should have the effect of enabling sha256 optimizations for the kernel. ACKs for top commit: TheCharlatan: Re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74 hebasto: re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74 Tree-SHA512: 7c17592bb2d3e671779f96903cb36887c5785408213bffbda1ae37b66e6bcfaffaefd0c1bf2d1a407060cd377e3d4881cde3a73c429a1aacb677f370314a066a
2024-01-26Merge bitcoin-core/gui#789: Avoid non-self-contained Windows headerHennadii Stepanov
8023640a71a10ec54a6a8e6b95a29d07f7be218d qt: Avoid non-self-contained Windows header (Hennadii Stepanov) Pull request description: Using the `windows.h` header guarantees correctness regardless of the content of other headers. For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager. Related to https://github.com/hebasto/bitcoin/pull/77. ACKs for top commit: theuni: ACK 8023640a71a10ec54a6a8e6b95a29d07f7be218d. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless. Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
2024-01-26[rpc] return full string for package_msg and package-errorglozow
2024-01-25Merge bitcoin/bitcoin#29315: refactor: Compile unreachable walletdb codeAva Chow
fa3373d3adbace7e4665cf391363319a55a09a96 refactor: Compile unreachable code (MarcoFalke) Pull request description: When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916 ACKs for top commit: achow101: ACK fa3373d3adbace7e4665cf391363319a55a09a96 ryanofsky: Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking. Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e
2024-01-25Merge bitcoin/bitcoin#20827: During IBD, prune as much as possible until we ↵Ava Chow
get close to where we will eventually keep blocks d298ff8b62b2624ed390c8a2f905c888ffc956ff During IBD, prune as much as possible until we get close to where we will eventually keep blocks (Luke Dashjr) Pull request description: This should reduce pruning flushes even more, speeding up IBD with pruning on systems that have a sufficient dbcache. Assumes 1 MB per block between tip and best header chain. Simply adds this to the buffer pruning is trying to leave available, which results in pruning almost everything up until we get close to where we need to be keeping blocks. ACKs for top commit: andrewtoth: ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff fjahr: utACK d298ff8b62b2624ed390c8a2f905c888ffc956ff achow101: ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff Tree-SHA512: 2a482376bfb177e2ba7c2f0bb0b58b02efdb38b34755a18d1fc3e869df5959c85b6f1009e1386fa8b89c4f90d520383e36bd3e21dec221042315134efb1a455b
2024-01-25refactor: Compile unreachable codeMarcoFalke
When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916 Can be reviewed with --ignore-all-space
2024-01-25qt: Avoid non-self-contained Windows headerHennadii Stepanov
Using the `windows.h` header guarantees correctness regardless of the content of other headers. For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.
2024-01-25Merge bitcoin/bitcoin#29205: build: always set `-g -O2` in `CORE_CXXFLAGS`fanquake
00c1e2aa4496b5f038ae5199dbd16d8313766533 build: fix optimisation flags used for --coverage (fanquake) 1dc2c9b385f8345c588449848149b8e470653afc ci: cleanup C*FLAG usage in Valgrind jobs (fanquake) 6cc2a38c1388b696e9c28a08c6bd9c93da4fa6b8 build: add sanitizer flags to configure output (fanquake) 08cd5aca18f0774258c7c459773b9e8b386d48ef build: always set -g -O2 in CORE_CXXFLAGS (fanquake) Pull request description: Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override (if unset), and always set the flags we want (which are the same as the Autoconf defaults). Removes the need for duplicate code to clear (if not overridden) `CXXFLAGS`. Fixes cases of "missing" `-O2`. i.e this PR when running a Valgrind CI job with changes here: ```bash CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bti -Werror -fsanitize=fuzzer -gdwarf-4 ``` Fixes configure output to reflect actual compilation flag ordering, so it's useful. Note that if we do still end up with a duplicate "-g -O2" when compiling, that has no effect, and I don't really thinks it's something worth trying to optimize. ACKs for top commit: TheCharlatan: lgtm ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533 hebasto: ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`. theuni: ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533 Tree-SHA512: cf6c7acf813ba10b198561e83eb72e9b2532a39cb1767c452d031e82921dcd42a47b129735b24c4e36131fd0c8fe7457f7cae870c1e011cdfdd430bdc4d4912b
2024-01-23wallet: clarify replaced_by_txid and replaces_txid in help outputmarco
2024-01-23init: settings, do not load auto-generated warning msgfurszy
The settings warning message is meant to be used only to discourage users from modifying the file manually. Therefore, there is no need to keep it in memory.
2024-01-23validation: move nChainTx assert down in CheckBlockIndexMartin Zumsande
There is a designated section meant for the actual consistency checks, marked by a comment.
2024-01-23doc: fix checkblockindex commentsMartin Zumsande
These exceptions are not related to situations specific to tests, but are required in general: Without the first check CheckBlockindex could fail for blocks where we only know the header. Without the second, it could fail when blocks are received out of order.
2024-01-23Merge bitcoin/bitcoin#28560: wallet, rpc: `FundTransaction` refactorAva Chow
18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake) 5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake) 47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake) f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake) 6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake) 435fe5cd96599c518e26efe444c9d94d1277996b test: add tests for fundrawtx and sendmany rpcs (josibake) Pull request description: ## Motivation The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`. As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO. I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments. ACKs for top commit: S3RK: reACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d josibake: > According to my `range-diff` nothing changed. reACK [18ad1b9](https://github.com/bitcoin/bitcoin/commit/18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d) achow101: ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
2024-01-23Merge bitcoin/bitcoin#28921: multiprocess: Add basic type conversion hooksAva Chow
6acec6b9ff02b91de132bb1575d75908a8a2d27b multiprocess: Add type conversion code for UniValue types (Ryan Ofsky) 0cc74fce72e0c79849109ee5d7afe707991b3512 multiprocess: Add type conversion code for serializable types (Ryan Ofsky) 4aaee239211a5287fbc361c0eb158b105ae8c8db test: add ipc test to test multiprocess type conversion code (Ryan Ofsky) Pull request description: Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly. The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). ACKs for top commit: achow101: ACK 6acec6b9ff02b91de132bb1575d75908a8a2d27b dergoegge: reACK 6acec6b9ff02b91de132bb1575d75908a8a2d27b Tree-SHA512: 5d2cbc5215d488b876d34420adf91205dabf09b736183dcc85aa86255e3804c2bac5bab6792dacd585ef99a1d92cf29c8afb3eb65e4d953abc7ffe41994340c6
2024-01-23Merge bitcoin/bitcoin#29144: init: handle empty settings file gracefullyAva Chow
e9014042a6bed8c16cc9a31fc35cb709d4b3c766 settings: add auto-generated warning msg for editing the file manually (furszy) 966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 init: improve corrupted/empty settings file error msg (furszy) Pull request description: Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144). Improving a confusing situation reported by users who did not understand why a settings parsing error occurred when the file was empty and did not know how to solve it. Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content. In both scenarios, the 'Unable to parse settings file' error does not help the user move forward. ACKs for top commit: achow101: ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766 hebasto: re-ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. ryanofsky: Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. Just whitespace formatting changes and shortening a test string literal since last review shaavan: Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766 Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
2024-01-23Merge bitcoin/bitcoin#28774: wallet: avoid returning a reference to ↵Ava Chow
vMasterKey after releasing the mutex that guards it 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov) Pull request description: `CWallet::GetEncryptionKey()` would return a reference to the internal `CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe. Returning a copy would be a shorter solution, but could have security implications of the master key remaining somewhere in the memory even after `CWallet::Lock()` (the current calls to `CWallet::GetEncryptionKey()` are safe, but that is not future proof). So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)` change the `GetEncryptionKey()` method to provide the encryption key to a given callback: `m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })` This silences the following (clang 18): ``` wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return] 3520 | return vMasterKey; | ^ ``` --- _Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510 was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_ Avoid this unsafe pattern from `ArgsManager` and `CWallet`: ```cpp class A { Mutex mutex; Foo member GUARDED_BY(mutex); const Foo& Get() { LOCK(mutex); return member; } // callers of `Get()` will have access to `member` without owning the mutex. ``` ACKs for top commit: achow101: ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b ryanofsky: Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple. furszy: ACK 32a9f13c Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1
2024-01-23Merge bitcoin/bitcoin#29272: wallet: fix coin selection tracing to return -1 ↵Ava Chow
when no change pos d55fdb1a495190e213b1b5127f5d91e4a409765e Move TRACEx parameters to seperate lines (Richard Myers) 2d58629ee63eebc760e2a9226afcd0c46d3ec2bd wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers) Pull request description: This is a bugfix for from when [optional was introduced](https://github.com/bitcoin/bitcoin/pull/25273/commits/758501b71391136c33b525b1a0109b990d4f463e) for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0. I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change. You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](https://github.com/achow101/coin-selection-simulation/issues/16). You can also run the `interface_usdt_coinselection.py` test without the changes to `wallet/spend.cpp`. ACKs for top commit: 0xB10C: ACK d55fdb1a495190e213b1b5127f5d91e4a409765e achow101: ACK d55fdb1a495190e213b1b5127f5d91e4a409765e murchandamus: ACK d55fdb1a495190e213b1b5127f5d91e4a409765e Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
2024-01-23Merge bitcoin/bitcoin#29291: Add test for negative transaction version w/ ↵fanquake
CSV to tx_valid.json 97181decf5726aab6c5cd01b3e1964072f2531ff Add test for negative transaction version w/ CSV to tx_valid.json (Chris Stewart) Pull request description: This PR adds a static test vector corresponding to the bug found in various implementations of the bitcoin protocol discovered by dergoegge For more information see: https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455 ACKs for top commit: darosior: ACK 97181decf5726aab6c5cd01b3e1964072f2531ff dergoegge: ACK 97181decf5726aab6c5cd01b3e1964072f2531ff Tree-SHA512: 92bbcd3cd10a569757b4de91e1b2bcfebc2b75ddb0160be36d8e512a6fa4623cced1aba93bd1cc044962cd2b10e1d184ef109ccdfe3cfcf85cf4b9585d80d115
2024-01-23[rpc/net] Allow v2 p2p support in addconnectionstratospher
This test-only RPC is required when a TestNode initiates an outbound v2 p2p connection. Add a new arg `v2transport` so that the node can attempt v2 connections.
2024-01-23net: remove now unused global 'g_initial_block_download_completed'furszy
2024-01-23test: add coverage for peerman adaptive connections service flagsfurszy
2024-01-23net: peer manager, dynamically adjust desirable services flagfurszy
Introduces functionality to detect when limited peers connections are desirable or not. Ensuring that the new connections desirable services flags stay relevant throughout the software's lifecycle. (Unlike the previous approach, where once the validation IBD flag was set, the desirable services flags remained constant forever). This will let us recover from stalling scenarios where the node had successfully synced, but subsequently dropped connections and remained inactive for a duration longer than the limited peers threshold (the timeframe within which limited peers can provide blocks). Then, upon reconnection to the network, the node may end up only establishing connections with limited peers, leading to an inability to synchronize the chain. This also fixes a possible limited peers threshold violation during IBD, when the user configures `-maxtipage` further than the BIP159's limits. This rule violation could lead to sync delays and, in the worst-case scenario, trigger the same post-IBD stalling scenario (mentioned above) but during IBD.
2024-01-23fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addressesVasil Dimov
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
2024-01-22settings: add auto-generated warning msg for editing the file manuallyfurszy
Hopefully, refraining users from modifying the file unless they are certain about the potential consequences. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2024-01-22init: improve corrupted/empty settings file error msgfurszy
The preceding "Unable to parse settings file" message lacked the necessary detail and guidance for users on what steps to take next in order to resolve the startup error. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2024-01-22wallet: remove unused `SignatureData` instances in spkm's `FillPSBT` methodsSebastian Falbesoner
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed.
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-09 09:54:22 +0000