aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2023-02-23util: Remove duplicate includeAndrew Chow
Duplicate `#include <utility>` is upsetting the linter.
2023-02-23Merge bitcoin/bitcoin#27073: Convert ArgsManager::GetDataDir to a read-only ↵Andrew Chow
function 64c105442ce8c10900ea6fbecdbcfebe42f2d387 util: make GetDataDir read-only & create datadir.. (willcl-ark) 56e370fbb9413260723c598048392219b1055ad0 util: add ArgsManager datadir helper functions (willcl-ark) Pull request description: Fixes #20070 Currently `ArgsManager::GetDataDir()` ensures it will always return a datadir by creating one if necessary. The function is shared between `bitcoind` `bitcoin-qt` and `bitcoin-cli` which results in the undesirable behaviour described in #20070. This PR splits out the part of the function which creates directories and adds it as a standalone function, only called as part of `bitcoind` and `bitcoin-qt` init, but not `bitcoin-cli`. `ReadConfigFiles`' behavior is changed to use the absolute path of the config file in error and warning messages instead of a relative path. This was inadvertantly the form being tested [here](https://github.com/bitcoin/bitcoin/blob/73966f75f67fb797163f0a766292a79d4b2c1b70/test/functional/feature_config_args.py#L287), whilst we were _not_ testing that a relative path was returned by the message even though we passed a relative path in as argument. ACKs for top commit: achow101: ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387 hebasto: re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387, only comments have been adjusted as requsted since my previous [review](https://github.com/bitcoin/bitcoin/pull/27073#pullrequestreview-1307435890). TheCharlatan: Re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387 ryanofsky: Code review ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387. Only comment changes since last review Tree-SHA512: b129501346071ad62551c9714492b21536d0558a94117d97218e255ef4e948d00df899a4bc2788faea27d3b1f20fc6136ef9d03e6a08498d926d9ad8688d6c96
2023-02-23Merge bitcoin/bitcoin#16195: util: Use void* throughout support/lockedpool.hAndrew Chow
f36d1d5b8934aac60d3097047ecedeb58bae2185 Use void* throughout support/lockedpool.h (Jeffrey Czyz) Pull request description: Replace uses of char* with void* in Arena's member variables. Instead, cast to char* where needed in the implementation. Certain compiler environments disallow std::hash<char*> specializations to prevent hashing the pointer's value instead of the string contents. Thus, compilation fails when std::unordered_map is keyed by char*. Explicitly using void* is a workaround in such environments. For consistency, void* is used throughout all member variables similarly to the public interface. Changes to this code are covered by src/test/allocator_tests.cpp. ACKs for top commit: achow101: ACK f36d1d5b8934aac60d3097047ecedeb58bae2185 theStack: Code-review ACK f36d1d5b8934aac60d3097047ecedeb58bae2185 jonatack: ACK f36d1d5b8934aac60d3097047ecedeb58bae2185 review, debug build, unit tests, checked clang 15 raises "error: arithmetic on a pointer to void" without the conversions here from the generic void* pointer back to char* Tree-SHA512: f9074e6d29ef78c795a512a6e00e9b591e2ff34165d09b73eae9eef25098c59e543c194346fcd4e83185a39c430d43744b6f7f9d1728a132843c67bd27ea5189
2023-02-23Merge bitcoin/bitcoin#25943: rpc: Add a parameter to sendrawtransaction ↵Andrew Chow
which sets a maximum value for unspendable outputs. 7013da07fbcddb04abae9759767a9419ab90444c Add release note for PR#25943 (David Gumberg) 04f270b4358417fc2827b9f91717816062b1864e Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg) Pull request description: This PR adds a user configurable, zero by default parameter — `maxburnamount` — to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes #25899. As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions: 1. Those that begin with `OP_RETURN` - (datacarriers) 2. Those whose lengths exceed the script limit. 3. Those that contain invalid opcodes. The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool. I see two legitimate use cases for this override: 1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain. 2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about. ACKs for top commit: glozow: reACK 7013da07fbcddb04abae9759767a9419ab90444c achow101: re-ACK 7013da07fbcddb04abae9759767a9419ab90444c Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
2023-02-23Merge bitcoin/bitcoin#27124: docs: add ramdisk guide for running tests on OSXfanquake
2f84ad7b9e62dd710940c2f265b65973b94864d7 docs: add ramdisk guide for running tests on OSX (Matthew Zipkin) Pull request description: Using a ramdisk on OSX sped up the test suite by about 5x (using default `jobs=4`) on my M1 macbook pro running macOS Monterey 12.3.1. This PR adds the relevant OSX commands following the Linux directions. Default: ``` 8204 s (accumulated) Runtime: 2104 s ``` following commands from the PR: ``` 1606 s (accumulated) Runtime: 421 s ``` ramdisk + `jobs=32`: ``` 2090 s (accumulated) Runtime: 85 s ``` ACKs for top commit: jonatack: ACK 2f84ad7b9e62dd710940c2f265b65973b94864d7 willcl-ark: ACK 2f84ad7b9e62dd710940c2f265b65973b94864d7 brunoerg: utACK 2f84ad7b9e62dd710940c2f265b65973b94864d7 Tree-SHA512: 37a9903c8ac2cbfaa91e7e73fc96ef65042ff4b15763d452af7b8615255adf03429ad01cf85265a99dd569290c1d69c05a393d616868c05c190b60b053820786
2023-02-23util: make GetDataDir read-only & create datadir..willcl-ark
.. only in bitcoind and bitcoin-qt This changes behaviour of GetConfigFilePath which now always returns the absolute path of the provided -conf argument.
2023-02-23util: add ArgsManager datadir helper functionswillcl-ark
* Add ArgsManager::EnsureDataDir() Creates data directory if it doesn't exist * Add ArgsManager::GetConfigFilePath() Return config file path (read-only)
2023-02-22Merge bitcoin/bitcoin#25574: validation: Improve error handling when ↵Andrew Chow
VerifyDB dosn't finish successfully 0af16e7134459e0820ab95d751093876c1ec4c6d doc: add release note for #25574 (Martin Zumsande) 57ef2a4812f443b2d734f43cebf3ef5038da83f2 validation: report if pruning prevents completion of verification (Martin Zumsande) 0c7785bb2540b69564104767d38342704230cbc2 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande) d6f781f1cfcbc2c2ad5ee289a0642ed00386d013 validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande) 6360b5302d2675788de5c4a28ea77d823f6d809e validation: Change return value of VerifyDB to enum type (Martin Zumsande) Pull request description: `VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways: - The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache - During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check. This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown. Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged). ACKs for top commit: achow101: ACK 0af16e7134459e0820ab95d751093876c1ec4c6d ryanofsky: Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups john-moffett: ACK 0af16e7134459e0820ab95d751093876c1ec4c6d MarcoFalke: lgtm re-ACK 0af16e7134459e0820ab95d751093876c1ec4c6d 🎚 Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
2023-02-22Merge bitcoin/bitcoin#27143: test: Replace 0xC0 constantfanquake
c3b4b5a142b204ceeca4e9b1ca1e2ff41ddd1308 test: Replace 0xC0 constant (roconnor-blockstream) Pull request description: Instead it should be the named constant `LEAF_VERSION_TAPSCRIPT`. ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/27143/commits/c3b4b5a142b204ceeca4e9b1ca1e2ff41ddd1308 theStack: ACK c3b4b5a142b204ceeca4e9b1ca1e2ff41ddd1308 Tree-SHA512: c00be584ea2d0e7c01bf5620da0da1f37e5b5298ef95df48d91d137c8c542f5d91be158d45392cf2ba8874bf27bd12924e2eed395773b49d091e3028de3356a2
2023-02-22docs: add ramdisk guide for running tests on OSXMatthew Zipkin
2023-02-22Merge bitcoin/bitcoin#27068: wallet: SecureString to allow null charactersAndrew Chow
4bbf5ddd44bde15b328be131922123eaa3212a7e Detailed error message for passphrases with null chars (John Moffett) b4bdabc2238750a1f6e72cb1403f8b770fc4f365 doc: Release notes for 27068 (John Moffett) 4b1205ba37d6737722d2087696b1a054a852286a Test case for passphrases with null characters (John Moffett) 00a0861181cc7f4771ac2690ca6be5731c30b005 Pass all characters to SecureString including nulls (John Moffett) Pull request description: `SecureString` is a `std::string` specialization with a secure allocator. However, in practice it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security. Instead of assigning to `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and still doesn't make any extraneous copies in memory. Note to reviewers, the following all compile identically in recent `GCC` (x86-64 and ARM64) with `-O2` (and `-std=c++17`): ```C++ std::string orig_string; std::cin >> orig_string; SecureString s; s.reserve(100); // The following all compile identically s = orig_string; s = std::string_view{orig_string}; s.assign(std::string_view{orig_string}); s.assign(orig_string.data(), orig_string.size()); ``` So it's largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory. Something like `SecureString s{orig_string};` is still invalid and probably unwanted in our case, since it'd get treated as a short string and optimized away from the secure allocator. I presume that's the reason for the `reserve()` calls. Fixes #27067. ACKs for top commit: achow101: re-ACK 4bbf5ddd44bde15b328be131922123eaa3212a7e stickies-v: re-ACK [4bbf5dd](https://github.com/bitcoin/bitcoin/commit/4bbf5ddd44bde15b328be131922123eaa3212a7e) furszy: utACK 4bbf5ddd Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7
2023-02-22Merge bitcoin/bitcoin#27144: kernel: add missing includefanquake
49d01f32c9cc4de4fcd0d1f235e2c62e4acfc7a2 kernel: add missing include (Cory Fields) Pull request description: This syncs the cs_main definition/declaration. Noticed when experimenting with the external visibility of `cs_main`. Specifically, this is needed for the following to work as intended: ```c++ __attribute__ ((visibility ("default"))) extern RecursiveMutex cs_main; ``` ACKs for top commit: fanquake: ACK 49d01f32c9cc4de4fcd0d1f235e2c62e4acfc7a2 Tree-SHA512: ea0dbcf81959566f949d76c7dcd1e33de53e613519500c863bfb0ac8209665b1c12cff2daa7890d03b76debc4d046339ee7b3231adb71b128e9d5a8fa3132b6c
2023-02-22Merge bitcoin/bitcoin#26837: I2P network optimizationsfanquake
3c1de032de01e551992975eb374465300a655f44 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov) 801b405f85b413631427c2d8cc1f8447309ea5d8 i2p: lower the number of tunnels for transient sessions (Vasil Dimov) b906b64eb76643feaede1da5987a0c4d466c581b i2p: reuse created I2P sessions if not used (Vasil Dimov) Pull request description: * Reuse an I2P transient session instead of discarding it if we failed to connect to the desired peer. This means we never used the generated address (destination), whose creation is not cheap. This does not mean that we will use the same address for more than one peer. * Lower the number of tunnels for transient sessions. * Explicitly specify the number of tunnels for persistent sessions instead of relying on the defaults which differ between I2P routers. This way we get consistent behavior with all routers. Alleviates: https://github.com/bitcoin/bitcoin/issues/26754 (I have not tested this with i2pd, yet) ACKs for top commit: jonatack: ACK 3c1de032de01e551992975eb374465300a655f44 mzumsande: Light ACK 3c1de032de01e551992975eb374465300a655f44 Tree-SHA512: 477b4b9a5755e6a9a46bc0f7b268fa419dff4414e25445c750ae913f7552d9e2313f2aca4e3b70067b8390c2d0c2d68ec459f331765e939fc84139e454031cd4
2023-02-22Merge bitcoin/bitcoin#27137: test: Raise PRNG seed log to INFOfanquake
4d84eaec82e7b5a450d47cd30e5936a717035f77 Raise PRNG seed log to INFO. (roconnor-blockstream) Pull request description: Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log (stdout/stderr) of the failed build. For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test. By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build. ACKs for top commit: MarcoFalke: lgtm ACK 4d84eaec82e7b5a450d47cd30e5936a717035f77 theStack: ACK 4d84eaec82e7b5a450d47cd30e5936a717035f77 Tree-SHA512: 3ccb4a4e7639a3babc3b2a6456a6d0bffc090da34e4545b317f7bfbed4e9950d1b38ea5b2a90c37ccb49b3454bdeff03a6aaf86770b9c4dd14b26320aba50b94
2023-02-22Merge bitcoin/bitcoin#26595: wallet: be able to specify a wallet name and ↵fanquake
passphrase to migratewallet 9486509be65f09174a0cb50a337cac58a0c09de4 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) aaf02b5721a8b5d3d9280dc3146fa5e44ea671b6 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) 7fd125b27d48e410509f3009e2eb9fa5cd6729dd wallet: Be able to unlock the wallet for migration (Andrew Chow) 6bdbc5ff590de18dfb47c31190baad879f68fef7 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) dbfa34540372033d95036a02b7025ddd33f540aa wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) Pull request description: `migratewallet` currently operates on wallets that are already loaded, however this is not necessarily required, and in the future, not possible once the legacy wallet is removed. So we need to also be able to give the wallet name to migrate. Additionally, the passphrase is required when migrating a wallet. Since a wallet may not be loaded when we migrate, and as we currently unload wallets when migrating, we need the passphrase to be given to `migratewallet` in order to migrate encrypted wallets. Fixes #27048 ACKs for top commit: john-moffett: reACK 9486509be65f09174a0cb50a337cac58a0c09de4 pinheadmz: ACK 9486509be65f09174a0cb50a337cac58a0c09de4 furszy: ACK 9486509b Tree-SHA512: 35e2ba69a148e129a41e20d7fb99c4cab7947b1b7e7c362f4fd06ff8ac6e79e476e07207e063ba5b80e1a33e2343f4b4f1d72d7930ce80c34571c130d2f5cff4
2023-02-22kernel: add missing includeCory Fields
This syncs the cs_main definition/declaration. Noticed when experimenting with the external visibility of cs_main.
2023-02-22test: Replace 0xC0 constantroconnor-blockstream
Instead it should be the named constant `LEAF_VERSION_TAPSCRIPT`.
2023-02-22Merge bitcoin/bitcoin#27117: fuzz: avoid redundant dup key checks when ↵fanquake
creating Miniscript nodes c1b7bd047f47dcd3eb6897adfaf9a55594deff5d fuzz: avoid redundant dup key checks when creating Miniscript nodes (Antoine Poinsot) Pull request description: I thought i had done that already in #24149, but it must have slipped through the rebase. It's a 2x speed improvement against the existing corpora and will probably be much more as we extend them with larger nodes. ACKs for top commit: sipa: ACK c1b7bd047f47dcd3eb6897adfaf9a55594deff5d Tree-SHA512: 9e6ceb6254183964b6c5538e21ba6321df95a68acb343a15a6ecfef5c51a1980d2627df5aeef9aef1db41656e18cc4f3bc96e6f24314d12fa60368b04a350001
2023-02-22Merge bitcoin/bitcoin#25867: lint: enable E722 do not use bare exceptfanquake
61bb4e783b3acc62b121a228f6b14c2462e23315 lint: enable E722 do not use bare except (Leonardo Lazzaro) Pull request description: Improve test code and enable E722 lint check. If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:). Reference: https://peps.python.org/pep-0008/#programming-recommendations ACKs for top commit: MarcoFalke: lgtm ACK 61bb4e783b3acc62b121a228f6b14c2462e23315 Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
2023-02-21wallet, rpc: Update migratewallet help text for encrypted walletsAndrew Chow
2023-02-21tests: Tests for migrating wallets by name, and providing passphraseAndrew Chow
2023-02-21Detailed error message for passphrases with null charsJohn Moffett
Since users may have thought the null characters in their passphrases were actually evaluated prior to this change, they may be surprised to learn that their passphrases no longer work. Give them feedback to explain how to remedy the issue.
2023-02-21doc: Release notes for 27068John Moffett
To reflect the change in behavior.
2023-02-21Test case for passphrases with null charactersJohn Moffett
Add a functional test to make sure the system properly accepts passphrases with null characters.
2023-02-21Pass all characters to SecureString including nullsJohn Moffett
`SecureString` is a `std::string` specialization with a secure allocator. However, it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected behavior. For instance, if a user enters a passphrase with an embedded null character (which is possible through Qt and the JSON-RPC), it will ignore any characters after the null, giving the user a false sense of security. Instead of assigning `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and doesn't make any extraneous copies in memory.
2023-02-21Merge bitcoin/bitcoin#26347: wallet: ensure the wallet is unlocked when ↵Andrew Chow
needed for rescanning 6a5b348f2e526f048d0b448b01f6c4ab608569af test: test rescanning encrypted wallets (ishaanam) 493b813e171a389a8b6750b4f2e42e8363a0267e wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam) 66a86ebabb26a055ca92af846bfa39dbd2f9f722 wallet: keep track of when the passphrase is needed when rescanning (ishaanam) Pull request description: Wallet passphrases are needed to top up the keypool of encrypted wallets during a rescan. The following RPCs need the passphrase when rescanning: - `importdescriptors` - `rescanblockchain` The following RPCs use the information about whether or not the passphrase is being used to ensure that full rescans are able to take place (meaning the following RPCs should not be able to run if a rescan requiring the wallet to be unlocked is taking place): - `walletlock` - `encryptwallet` - `walletpassphrasechange` `m_relock_mutex` is also introduced so that the passphrase is not deleted from memory when the timeout provided in `walletpassphrase` is up and the wallet is still rescanning. Fixes #25702, #11249 Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions. ACKs for top commit: achow101: ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af hernanmarino: ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af furszy: Tested ACK 6a5b348f Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5
2023-02-21Merge bitcoin/bitcoin#27122: script: BIP341 txdata cannot be precomputed ↵Andrew Chow
without spent outputs 95f12de92505522a32ba58acd5251c69e602d160 BIP341 txdata cannot be precomputed without spent outputs (Pieter Wuille) Pull request description: In `PrecomputedTransactionData::Init`, if `force` is set to `true`, `m_bip341_taproot_ready` is always set to true, suggesting that all its BIP341-relevant members (including `m_spent_amounts_single_hash`) are correct. If however no `spent` array of spent previous `CTxOut`s is provided, some of these members will be incorrect. This option was introduced in #21365. That doesn't actually hurt, as without prevout data, it's fundamentally impossible to generate correct BIP341 signatures anyway, and https://github.com/bitcoin/bitcoin/blob/f722a9bd132222d9d5cd503b5af25c905b205cdb/src/script/sign.cpp#L71 should prevent the logic from being used anyway. Still, don't set `m_bip341_taproot_ready` variable when we clearly don't have enough data to compute it. Discovered by Russell O'Connor. ACKs for top commit: ajtowns: ACK 95f12de92505522a32ba58acd5251c69e602d160 achow101: ACK 95f12de92505522a32ba58acd5251c69e602d160 instagibbs: ACK 95f12de92505522a32ba58acd5251c69e602d160 Tree-SHA512: 90acd2bfa50a7a0bde75a15a9f6c1f5c40f48fb5b870b1bbc4082777e24a482c8282463ef7d1245e53201dbcb5c196ef0386352f8e380e68cdf00c2111633b77
2023-02-21Raise PRNG seed log to INFO.roconnor-blockstream
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log of the failed build. For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test. By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
2023-02-20Add release note for PR#25943David Gumberg
Co-authored-by: glozow <gloriajzhao@gmail.com>
2023-02-20Add test for unspendable transactions and parameter 'maxburnamount' to ↵David Gumberg
sendrawtransaction. 'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.
2023-02-20Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX ↵fanquake
with avoidpartialspends 14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) Pull request description: Closes https://github.com/bitcoin/bitcoin/issues/27051 When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain. If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected. I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582 ACKs for top commit: achow101: ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
2023-02-20Merge bitcoin/bitcoin#27127: rpc: fix successful broadcast count in ↵fanquake
`submitpackage` error msg 7554b1fd663fe2010edb0e8a93ab85a6cb10a323 rpc: fix successful broadcast count in `submitpackage` error msg (Sebastian Falbesoner) Pull request description: If a `submitpackage` RPC call errors due to any of the individual tx broadcasts failing, the returned error message is supposed to contain the number of successful broadcasts so far: https://github.com/bitcoin/bitcoin/blob/4395b7f0845d2dca60f3b4e007ef5770ce8e2aa9/src/rpc/mempool.cpp#L848-L849 Right now this is wrongly always shown as zero. Fix this by adding the missing increment of the counter. While touching that area, the variable is also renamed to better reflect its purpose (s/num_submitted/num_broadcast/; the submission has already happened at that point) and named arguments for the `BroadcastTransaction` call are added. (Note that the error should be really rare, as all txs have already been submitted succesfully to the mempool. IIUC this code-path could only hit if somehow a tx is being removed from the mempool between `ProcessNewPackage` and the `BroadcastTransaction` calls, e.g. if a new block is received which confirms any of the package's txs.) ACKs for top commit: glozow: utACK 7554b1fd663fe2010edb0e8a93ab85a6cb10a323, thanks! Tree-SHA512: e362e93b443109888e28d6facf6f52e67928e8baaa936e355bfdd324074302c4832e2fa0bd8745309a45eb729866d0513b928ac618ccc9432b7befc3aa2aac66
2023-02-20Merge bitcoin/bitcoin#27113: rpc: Use a FlatSigningProvider in decodescript ↵fanquake
to allow inferring descriptors for scripts larger than 520 bytes 73ec4b2a8347c796b9aadc1f2576b286c469f9e7 tests: decodescript can infer descriptors for scripts >520 bytes (Andrew Chow) 7cc78223710679c6e7fd40b762798a1f5ca4938e rpc: Use FlatSigningProvider in decodescript (Andrew Chow) Pull request description: `FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded. Fixes #27111 ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/27113/commits/73ec4b2a8347c796b9aadc1f2576b286c469f9e7 Tree-SHA512: c0e6d21025e2da864471989ac94c54e127d05459b9b048f34a0da8d76d8e372d5472a2e667ba2db74d6286e3e6faa55486ffa9232a068b519afa676394031d5a
2023-02-20Merge bitcoin/bitcoin#27027: build: use _FORTIFY_SOURCE=3fanquake
4faa4e37a6511c6ada303ef7929ac99c7462f083 build: use _FORTIFY_SOURCE=3 (fanquake) Pull request description: [glibc 2.33](https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html) introduced a new fortification level, `_FORTIFY_SOURCE=3`. It improves the coverage of cases where `_FORTIFY_SOURCE` can use `_chk` functions. For example, using GCC 13 and glibc 2.36 (Fedora Rawhide), compiling master: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 33 ``` vs this branch: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __memset_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 61 ``` Usage of level 3 requires LLVM/Clang 9+, or GCC 12+. Older compilers/glibc will still use _FORTIFY_SOURCE=2. For example, in the glibc we currently use for Linux release builds (2.24), `__USE_FORTIFY_LEVEL` is determined using the following: ```c #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 # if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0 # warning _FORTIFY_SOURCE requires compiling with optimization (-O) # elif !__GNUC_PREREQ (4, 1) # warning _FORTIFY_SOURCE requires GCC 4.1 or later # elif _FORTIFY_SOURCE > 1 # define __USE_FORTIFY_LEVEL 2 # else # define __USE_FORTIFY_LEVEL 1 # endif #endif #ifndef __USE_FORTIFY_LEVEL # define __USE_FORTIFY_LEVEL 0 #endif ``` so any value > 1 will turn on `_FORTIFY_SOURCE=2`. This value detection logic has become slightly more complex in later versions of glibc. https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source ACKs for top commit: theuni: ACK 4faa4e37a6511c6ada303ef7929ac99c7462f083. After playing with this quite a bit I didn't observe any noticeable pitfalls. Tree-SHA512: e84ba49e3872c29fed1e2aea237b0d6bdff0d1274fa3297e2e08317cb62004396ee97b1cd6addb7c8b582498f3fa857a6d84c8e8f5ca97791b93985b47ff7faa
2023-02-20Merge bitcoin/bitcoin#27128: test: fix intermittent issue in ↵fanquake
`p2p_disconnect_ban` 1819564c2130d4d8537ca433c6688b56c769fb79 test: fix intermittent issue in `p2p_disconnect_ban` (brunoerg) Pull request description: Fixes #26808 When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection. ACKs for top commit: MarcoFalke: lgtm ACK 1819564c2130d4d8537ca433c6688b56c769fb79 Tree-SHA512: 53a386fc38e2faa6f6da3536e76857ff4b6f55e2590d73fe857b3fe5d0f3ff92c5c7e4abd50ab4be250cb2106a4d14ad95d4809ea60c6e00ed3ac0e71255b0b0
2023-02-20Merge bitcoin/bitcoin#25950: test: fix test abort for high timeout values ↵fanquake
(and `--timeout-factor 0`) 14302a4802e2dbb41f5189de88f99ddd5dda7736 test: fix test abort for high timeout values (and `--timeout-factor 0`) (Sebastian Falbesoner) Pull request description: On master, the functional tests's option `--timeout-factor 0` (which according to the test docs and parameter description should disable the RPC timeouts) currently fails, same as high values like `--timeout-factor 999999`: ``` $ ./test/functional/wallet_basic.py --timeout-factor 0 2022-08-29T01:26:39.561000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f24yxzp5 2022-08-29T01:26:40.262000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/honey/bitcoin/test/functional/test_framework/test_framework.py", line 549, in start_nodes node.wait_for_rpc_connection() File "/home/honey/bitcoin/test/functional/test_framework/test_node.py", line 234, in wait_for_rpc_connection rpc.getblockcount() File "/home/honey/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__ response, status = self._request('POST', self.__url.path, postdata.encode('utf-8')) File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request self.__conn.request(method, path, postdata, headers) File "/usr/local/lib/python3.9/http/client.py", line 1285, in request self._send_request(method, url, body, headers, encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1331, in _send_request self.endheaders(body, encode_chunked=encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1280, in endheaders self._send_output(message_body, encode_chunked=encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1040, in _send_output self.send(msg) File "/usr/local/lib/python3.9/http/client.py", line 980, in send self.connect() File "/usr/local/lib/python3.9/http/client.py", line 946, in connect self.sock = self._create_connection( File "/usr/local/lib/python3.9/socket.py", line 844, in create_connection raise err File "/usr/local/lib/python3.9/socket.py", line 832, in create_connection sock.connect(sa) OSError: [Errno 22] Invalid argument ``` This is caused by a high timeout value that Python's HTTP(S) client library can't cope with. Fix this by clamping down the connection's set timeout value in AuthProxy. The change can easily be tested by running an arbitrary test with `--timeout-factor 0` on master (should fail), on this PR (should pass) and on this PR with the clamping value increased by 1 (should fail). // EDIT: The behaviour was observed on OpenBSD 7.1 and Python 3.9.12. ACKs for top commit: MarcoFalke: lgtm ACK 14302a4802e2dbb41f5189de88f99ddd5dda7736 Tree-SHA512: 6469e8ac699f1bb7dea11d5fb8b3ae54d895bb908570587c5631144cd41fe980ca0b1e6d0b7bfa07983307cba15fb26ae92e6766375672bf5be838d8e5422dbc
2023-02-20test: fix intermittent issue in `p2p_disconnect_ban`brunoerg
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
2023-02-20Merge bitcoin/bitcoin#26883: src/node/miner cleanups, follow-ups for #26695glozow
6a5e88e5cf06a6b410486cc36aba7afece0d9da9 miner: don't re-apply default Options value if argument is unset (stickies-v) ea72c3d9d594b2ea9b3397e64efd08f8563cb400 refactor: avoid duplicating BlockAssembler::Options members (stickies-v) cba749a9b7a6cd24e8887bddeb0430a1ebc783da refactor: rename local gArgs to args (stickies-v) Pull request description: Two follow-ups for #26695, both refactoring and no observed (*) behaviour change: - Rename `gArgs` to `args` because it's not actually a global - Add `BlockAssembler::Options` as a (private) member to `BlockAssembler` to avoid having to assign all the options individually, essentially duplicating them Reduces LoC and makes the code more readable, in my opinion. --- (*) as [pointed out by ajtowns](https://github.com/bitcoin/bitcoin/pull/26883#discussion_r1068247937), this PR changes the interface of `ApplyArgsManOptions()`, making this not a pure refactoring PR. In practice, `ApplyArgsManOptions()` is never called in such a way that this leads to observed behaviour change. Regardless, I've carved out the potential behaviour change into a separate commit and would be okay with dropping it, should it turn out to be controversial. ACKs for top commit: glozow: ACK 6a5e88e5cf TheCharlatan: Light code review ACK 6a5e88e5cf06a6b410486cc36aba7afece0d9da9 Tree-SHA512: 15c30442ff0e070b1a58dc4c9615550d619ce35b4a2596b2c0a9d790259bbf987cab708f7cbb1057a8cf8b4c3226f3ad981282d3499ac442094806492a5f68ce
2023-02-20rpc: fix successful broadcast count in `submitpackage` error msgSebastian Falbesoner
If a `submitpackage` RPC call errors due to any of the individual tx broadcasts failing, the returned error message is supposed to contain the number of successful broadcasts so far. Right now this is wrongly always shown as zero. Fix this by adding the missing counting. (Note though that the error should be really rare, as all txs have already been submitted succesfully to the mempool.)
2023-02-19Merge bitcoin/bitcoin#26814: refactor: remove windows-only compat.h usage in ↵fanquake
random 621cfb77227b5a240d66547947f73130f0c51f44 random: consolidate WIN32 #ifdefs (fanquake) 75ec6275e6780b9ed18e271e6b24bef46d1af96d random: remove compat.h include (fanquake) 4dc12816ace11eceee05c1ad24dd925f420a0bda random: use int for MAX_TRIES (fanquake) Pull request description: This change is related to removing the use of `compat.h` as a miscellaneous catch-all for unclear/platform specific includes. Somewhat prompted by IWYU-related discussion here: https://github.com/bitcoin/bitcoin/pull/26763/files#r1058861693. The only reason `compat.h` is required in random.cpp for Windows (note the `#ifdef WIN32`), is for `ssize_t` and an "indirect" inclusion of `windows.h`. I say indirect, because `windows.h` isn't actually included in compat.h either, it's dragged in as a side-effect of other windows includes there, i.e `winsock2.h`. Remove this coupling by replacing `ssize_t` with int, just including `windows.h` and removing compat.h. ACKs for top commit: hebasto: re-ACK 621cfb77227b5a240d66547947f73130f0c51f44, rebased only since my [recent](https://github.com/bitcoin/bitcoin/pull/26814#pullrequestreview-1237312144) review. Verified with: john-moffett: ACK 621cfb77227b5a240d66547947f73130f0c51f44 Tree-SHA512: 31e1ed2e7ff7daf6c3ee72e6a908def52f7addf8305ba371c5032f1927cbb8ef5d302785e8de42b5c04a123052f04688cc9fd80decceb04738b5d9153f3d32d7
2023-02-18lint: enable E722 do not use bare exceptLeonardo Lazzaro
2023-02-17test: fix test abort for high timeout values (and `--timeout-factor 0`)Sebastian Falbesoner
2023-02-17Merge bitcoin/bitcoin#26940: test: create random and coins utils, add amount ↵Andrew Chow
helper, dedupe add_coin 4275195606e6f42466d9a8ef766b3035833df4d5 De-duplicate add_coin methods to a test util helper (Jon Atack) 9d92c3d7f42c18939a9a6aa1ee185f1c958360a0 Create InsecureRandMoneyAmount() test util helper (Jon Atack) 81f5ade2a324167c03c5ce765a26bd42ed652723 Move random test util code from setup_common to random (Jon Atack) Pull request description: - Move random test utilities from `setup_common` to a new `random` file, as many tests don't use this code. - Create a helper to generate semi-random CAmounts up to `MONEY_RANGE` rather than only uint32, and use the helper in the unit tests. - De-duplicate a shared `add_coin` method by extracting it to a `coins` test utility. ACKs for top commit: pinheadmz: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 achow101: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 john-moffett: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 Tree-SHA512: 3ed974251149c7417f935ef2f8865aa0dcc33b281b47522b0f96f1979dff94bb8527957f098fe4d210f40d715c00f29512f2ffe189097102229023b7284a3a27
2023-02-17Merge bitcoin/bitcoin#25862: refactor, kernel: Remove gArgs accesses from ↵Andrew Chow
dbwrapper and txdb aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f refactor, validation: Add ChainstateManagerOpts db options (Ryan Ofsky) 0352258148c51572426666d337c7b28d0033376c refactor, txdb: Use DBParams struct in CBlockTreeDB (Ryan Ofsky) c00fa1a7341d3f47f992e0beb043da655cbca777 refactor, txdb: Add CoinsViewOptions struct (Ryan Ofsky) 2eaeded37f3a07c35eef38d9a80c1e5fbd4d41ee refactor, dbwrapper: Add DBParams and DBOptions structs (Ryan Ofsky) Pull request description: Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules. This PR does not change behavior in any way. It is a simpler alternative to #25623 because the only thing it does is remove `gArgs` references from kernel code. It avoids other unnecessary changes like adding options to the kernel API (they can be added separately later). ACKs for top commit: TheCharlatan: Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f achow101: ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f furszy: diff ACK aadd7c5b Tree-SHA512: 46dfd5d99ab3110492e7bba97a87122c831b8344caaf7dd2ebdb6e0ad6aa9174d4d1832d6f3a7465eda9294fe50defaa3c000afbbddc4e72838687df09a63ffd
2023-02-17BIP341 txdata cannot be precomputed without spent outputsPieter Wuille
2023-02-17Merge bitcoin/bitcoin#20018: p2p: ProcessAddrFetch(-seednode) is unnecessary ↵Andrew Chow
if -connect is specified 2555a3950f0304b7af7609c1e6c696993c50ac72 p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta) Pull request description: If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted. Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`. Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`. If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration. ACKs for top commit: mzumsande: Code Review ACK 2555a3950f0304b7af7609c1e6c696993c50ac72 achow101: ACK 2555a3950f0304b7af7609c1e6c696993c50ac72 vasild: ACK 2555a3950f0304b7af7609c1e6c696993c50ac72 Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
2023-02-17Merge bitcoin/bitcoin#25619: net: avoid overriding non-virtual ToString() in ↵Andrew Chow
CService and use better naming c9d548c91fb12fba516dee896f1f97692cfa2104 net: remove CService::ToStringPort() (Vasil Dimov) fd4f0f41e915d99c9b0eac1afd21c5628222e368 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov) 96c791dd20fea54c17d224000dee677bc158f66a net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov) 944a9de08a00f8273e73cd28b40e46cc0eb0bad1 net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov) 043b9de59aec88ae5e29daac7dc2a8b51a9414ce scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov) Pull request description: Before this PR we had the somewhat confusing combination of methods: `CNetAddr::ToStringIP()` `CNetAddr::ToString()` (duplicate of the above) `CService::ToStringIPPort()` `CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`) `CService::ToStringPort()` Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396). "IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP". Change the above to: `CNetAddr::ToStringAddr()` `CService::ToStringAddrPort()` The changes touch a lot of files, but are mostly mechanical. ACKs for top commit: sipa: utACK c9d548c91fb12fba516dee896f1f97692cfa2104 achow101: ACK c9d548c91fb12fba516dee896f1f97692cfa2104 jonatack: re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests LarryRuane: ACK c9d548c91fb12fba516dee896f1f97692cfa2104 Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
2023-02-17Merge bitcoin/bitcoin#26889: refactor: wallet, remove global 'ArgsManager' ↵Andrew Chow
dependency 52f4d567d69425dfd514489079db80483024a80d refactor: remove <util/system.h> include from wallet.h (furszy) 6c9b342c306b9e17024762c4ba8f1c64e9810ee2 refactor: wallet, remove global 'ArgsManager' access (furszy) d8f5fc446216258a68e256076c889ec23471855f wallet: set '-walletnotify' script instead of access global args manager (furszy) 3477a28dd3b4bc6c1993554c5ce589d69fa86070 wallet: set keypool_size instead of access global args manager (furszy) Pull request description: Structurally, the wallet class shouldn't access the global `ArgsManager` class, its internal behavior shouldn't be coupled to a global command line args parsing object. So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the "-walletnotify" script. And cleans up the, now unneeded, wallet `ArgsManager` ref member. Extra note: In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header including `util/system.h`. ACKs for top commit: achow101: ACK 52f4d567d69425dfd514489079db80483024a80d TheCharlatan: Re-ACK 52f4d567d69425dfd514489079db80483024a80d hebasto: re-ACK 52f4d567d69425dfd514489079db80483024a80d Tree-SHA512: 0cffd99b4dd4864bf618aa45aeaabbef2b6441d27b6dbb03489c4e013330877682ff17b418d07aa25fbe1040bdf2c67d7559bdeb84128c5437bf0e6247719016
2023-02-17random: consolidate WIN32 #ifdefsfanquake
Order includes Remove // for xyz comments
2023-02-17random: remove compat.h includefanquake
We no-longer need ssize_t. Add windows.h, which was being indirectly included via compat.h. It isn't actually included in compat.h itself, but was being included as a side-effect of other includes, like winsock2.h.
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-09 20:07:25 +0000