aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2024-02-07Merge bitcoin-core/gui#497: Enable users to configure their monospace font ↵Hennadii Stepanov
specifically a17fd33edd1374145fd6986fbe352295355fde4f GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice (Luke Dashjr) 98e9ac51992b2332587d87f25351988bf4863238 GUI: Use FontChoice type in OptionsModel settings abstraction (Luke Dashjr) 3a6757eed9a24e91e7d800d8026cc3a5c4840141 GUI: Load custom FontForMoney from QSettings (Luke Dashjr) 49eb97eff96c2ec9e5a55d599f18b1866f83b115 GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel (Luke Dashjr) f2dfde80b85b202bece0b5b4c8f1c8777c1a660d GUI: Move "embedded font or not" decision into new OptionsModel::getFontForMoney method (Luke Dashjr) Pull request description: This replaces the overly-verbose radio-button font setting (which only allows embedded or autodetected from system) with a simple combobox providing both existing options as well as a custom option to allow the user to select any font of their choice/style. ACKs for top commit: pablomartin4btc: tested ACK a17fd33edd1374145fd6986fbe352295355fde4f hebasto: ACK a17fd33edd1374145fd6986fbe352295355fde4f, I have reviewed the code and tested it on Ubuntu 22.04. This is a UX improvement. https://github.com/bitcoin-core/gui/pull/497#issuecomment-1341222673 might be addressed in a follow-up. Tree-SHA512: 2f0a8bc1242a374c4b7dc6e34014400428b6d36063fa0b01c9f62a8bd6078adfbbca93d95c87e4ccb580d982fe10173e1d9a28bcec586591dd3f966c7b90fc5d
2024-02-07Merge bitcoin-core/gui#553: Change address / amount error backgroundHennadii Stepanov
fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 qt: change QLineEdit error background (w0xlt) Pull request description: This PR proposes a small change in QLineEdit when there is an error in the input. master | --- | ![image](https://user-images.githubusercontent.com/94266259/154762427-b816267e-ec70-4a8f-a7fb-1317ebacf1a4.png) PR | --- | ![image](https://user-images.githubusercontent.com/94266259/154761933-15eb3d81-ca81-4498-b8ec-cf1139ae2f8a.png) | This also shows good results when combined with other open PRs. #537 | --- | ![image](https://user-images.githubusercontent.com/94266259/154763411-6266a283-2d8a-4365-b3f2-a5cb545e773e.png) #533 | --- | ![image](https://user-images.githubusercontent.com/94266259/154765638-f38b13d6-a4f8-4b46-a470-f882668239f3.png) | ACKs for top commit: GBKS: ACK fe7c81e jarolrod: ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 shaavan: ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 Tree-SHA512: eccc53f42d11291944ccb96efdbe460cb10af857f1d4fa9b5348ddcb0796c82faf1cdad9040aae7a25c5d8f4007d6284eba868d7af14acf56280f6acae170b91
2024-02-06Merge bitcoin/bitcoin#29388: fuzz: remove unused `args` and `context` from ↵Ryan Ofsky
`FuzzedWallet` b14298c5bca395e5ed6a27fe1758c0d1f4b824ac fuzz: remove unused `args` and `context` from `FuzzedWallet` (brunoerg) Pull request description: `ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763df71e788849b587883b3c16bb12229, they are not used anymore. This PR removes them. ACKs for top commit: maflcko: lgtm ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac epiccurious: utACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac ryanofsky: Code review ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac Tree-SHA512: 164e6a66ba05e11176a0cf68db6257f0ac07459cf7aa01ec4302b303c156c205a68128373a0b8daba0a6dfbff990af7fa14465a6341a296312fb20ea778c7a8c
2024-02-06Merge bitcoin/bitcoin#28833: wallet: refactor: remove unused `SignatureData` ↵Ava Chow
instances in spkm's `FillPSBT` methods e2ad343f69af4f924b22dccf94a52b6431ef6e3c wallet: remove unused `SignatureData` instances in spkm's `FillPSBT` methods (Sebastian Falbesoner) Pull request description: These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used. ACKs for top commit: achow101: ACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c brunoerg: crACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c Tree-SHA512: f0cabcc000bcea6bc7d7ec9d3be2e2a8accbdbffbe35252250ea2305b65a5813fde2b8096fbdd2c7cccdf417ea285183dc325fc2d210d88bce62978ce642930e
2024-02-06Merge bitcoin/bitcoin#29375: wallet: remove unused 'accept_no_keys' arg from ↵Ava Chow
decryption process 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 wallet: remove unused 'accept_no_keys' arg from decryption process (furszy) Pull request description: Found it while reviewing other PR. Couldn't contain myself from cleaning it up. The wallet decryption process (`CheckDecryptionKey()` and `Unlock()`) contains an arg 'accept_no_keys,' introduced in #13926, that has never been used. Additionally, this also removes the unimplemented `SplitWalletPath` function. ACKs for top commit: delta1: ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 epiccurious: utACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452. achow101: ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 theStack: Code-review ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 Tree-SHA512: e0537c994be19ca0032551d8a64cf1755c8997e04d21ee0522b31de26ad90b9eb02a8b415448257b60bced484b9d2a23b37586e12dc5ff6e35bdd8ff2165c6bf
2024-02-06Merge bitcoin/bitcoin#29356: test: make v2transport arg in addconnection ↵glozow
mandatory and few cleanups e7fd70f4b6b163f4ad5b25b4da7fa79899245235 [test] make v2transport arg in addconnection mandatory and few cleanups (stratospher) Pull request description: - make `v2transport` argument in `addconnection` regression-testing only RPC mandatory. https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750 - previously it was an optional arg with default `false` value. - only place this RPC is used is in the [functional tests](https://github.com/bitcoin/bitcoin/blob/11b436a66af3ceaebb0f907878715f331516a0bc/test/functional/test_framework/test_node.py#L742) where we always pass the appropriate `v2transport` option to the RPC anyways. (and that too just for python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) interactions) - rename `v2_handshake()` to `_on_data_v2_handshake()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424 - more compact return statement in `wait_for_reconnect()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708 - assertion to check that empty version packets are received from `TestNode`. ACKs for top commit: glozow: ACK e7fd70f4b6 theStack: Code-review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235 mzumsande: Code Review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235 Tree-SHA512: e66e29baccd91e1e4398b91f7d45c5fc7c2841d77d8a6178734586017bf2be63496721649da91848dec71da605ee31664352407d5bb896e624cc693767c61a1f
2024-02-05fuzz: remove unused `args` and `context` from `FuzzedWallet`brunoerg
2024-02-05Merge bitcoin/bitcoin#29254: log: Don't use scientific notation in log messagesglozow
c819a83b4d83413a31323c1304664ee4c228ca29 Don't use scientific notation in log messages (Kristaps Kaupe) Pull request description: Don't see any benefits here, only harder to read for most of the users. Before: ``` 2024-01-16T13:11:36Z Dumped mempool: 8.165e-06s to copy, 0.00224268s to dump ``` After: ``` 2024-01-16T13:11:36Z Dumped mempool: 0.000s to copy, 0.002s to dump ``` ACKs for top commit: kristapsk: > > > > lgtm ACK [c819a83](https://github.com/bitcoin/bitcoin/commit/c819a83b4d83413a31323c1304664ee4c228ca29). can you update the PR description? glozow: lgtm ACK c819a83b4d83413a31323c1304664ee4c228ca29. can you update the PR description? Tree-SHA512: 0972e0a05934e1b014fdeca0c235065aa017ba9abf74b3018f514e4d8022ef02b7f042a07d3675144b51449492468aea6b5b0183233ad7f1bab887d18e3d06af
2024-02-05Merge bitcoin/bitcoin#29354: test: Assumeutxo with more than just coinbase ↵glozow
transactions fa5cd66f0a47d1b759c93d01524ee4558432c0cc test: Assumeutxo with more than just coinbase transactions (MarcoFalke) Pull request description: Currently the AU tests only check that loading a txout set with only coinbase outputs works. Fix that by adding other transactions. ACKs for top commit: jamesob: ACK https://github.com/bitcoin/bitcoin/pull/29354/commits/fa5cd66f0a47d1b759c93d01524ee4558432c0cc glozow: concept ACK fa5cd66f0a47d1b759c93d01524ee4558432c0cc Tree-SHA512: e090c41f73490ad72e36c478405bfd0716d46fbf5a131415095999da6503094a86689a179a84addae3562b760df64cdb67488f81692178c8ca8bf771b1e931ff
2024-02-03wallet: remove unused 'accept_no_keys' arg from decryption processfurszy
The wallet decryption process (CheckDecryptionKey() and Unlock()) contains an arg 'accept_no_keys,' introduced in #13926, that has never been used. Additionally, this also removes the unimplemented SplitWalletPath function.
2024-02-02Merge bitcoin/bitcoin#28868: wallet: Fix migration of wallets with txs that ↵Ryan Ofsky
have both spendable and watchonly outputs 4da76ca24725eb9ba8122317e04a6e1ee14ac846 test: Test migration of tx with both spendable and watchonly (Ava Chow) c62a8d03a862fb124b4f4b88efd61978e46605f8 wallet: Keep txs that belong to both watchonly and migrated wallets (Ava Chow) 71cb28ea8cb579ac04cefc47a57557c94080d1af test: Make sure that migration test does not rescan on reloading (Ava Chow) 78ba0e6748d2a519a96c41dea851e7c43b82f251 wallet: Reload the wallet if migration exited early (Ava Chow) 9332c7edda79a39bb729b71b6f8db6a9d37343bb wallet: Write bestblock to watchonly and solvable wallets (Ava Chow) Pull request description: A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both. I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen. The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits. ACKs for top commit: ryanofsky: Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage. furszy: Code review ACK 4da76ca2 Tree-SHA512: 5fc210cff16ca6720d7b2d0616d7e3f295c974147854abc704cf99a3bfaad17572ada084859e7a1b1ca94da647ad130303219678f429b7995f85e040236db35c
2024-02-02Merge bitcoin/bitcoin#29367: wallet: Set descriptors flag after migrating ↵Ryan Ofsky
blank wallets 3904123da954f9ebd905de4129aec9f9bab9efc7 tests: Test that descriptors flag is set for migrated blank wallets (Ava Chow) 072d506240f6c39387b2edd4421818cc914c0912 wallet: Make sure that the descriptors flag is set for blank wallets (Ava Chow) Pull request description: While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets. To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this. ACKs for top commit: epiccurious: Tested ACK 3904123da954f9ebd905de4129aec9f9bab9efc7. delta1: tested ACK 3904123da954f9ebd905de4129aec9f9bab9efc7 ryanofsky: Code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7 murchandamus: code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7 Tree-SHA512: 9f6fe9c1899ca387ab909b1bb6956cd6bc5acbf941686ddc6c061f9b1ceec2cc9d009ff472486fc86e963f6068f0e2f1ae96282e7c630193797a9734c4f424ab
2024-02-02Merge bitcoin/bitcoin#29361: refactor: Fix timedata includesAva Chow
fad0fafd5aca699cfab7673f8eb18211139aeb18 refactor: Fix timedata includes (MarcoFalke) Pull request description: Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it. ACKs for top commit: achow101: ACK fad0fafd5aca699cfab7673f8eb18211139aeb18 dergoegge: utACK fad0fafd5aca699cfab7673f8eb18211139aeb18 stickies-v: ACK fad0fafd5aca699cfab7673f8eb18211139aeb18 Tree-SHA512: 45e86f2eb90f0e37012bd83bf30259719e0e58ede18b31f51ca8a6f6d23e6ca4d060fc0f56f821a711cbdb45792b82cf780f5ae3226680d7a966471990f352bc
2024-02-01wallet: Make sure that the descriptors flag is set for blank walletsAva Chow
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-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
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-09 08:40:26 +0000