This is already checked by test/lint/lint-files.py
There is no need to reword all linters into the dev notes.
Also, allow scripts in Rust (there are already some).
This was added to the gitignore file itself in commit
fada115cbeaa8c0ca3d19178499079b66ee5f499. The past violations are
evidence that the note in the dev notes was missed anyway.
faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40 doc: Remove ParseInt mentions in documentation (MarcoFalke)
33332829333b589420f8038541d04ec6970f051d refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c36cb0accf87123ca4eb98f6219d55fb5e bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face2519fac9e840d52f0334d9079e664be7eb28 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf0b993c879ee8c516baa36339ff7b72406 bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a558ab15b5d8eeea5dd4c9c0d76350051b bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5fe38b81f1af6b343661b65e16b0de7c60 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac03788243847b799a3feaac56bc918fba9 rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb499dec79258b1857b404d72f93650503f4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c69192fcb48a9e04d52eeb07fff15655d0 test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123afa0ef752e8645bf695d121da66d8f1382b Reject + sign when checking -ipcfd (MarcoFalke)
fa479857ed234d54df31d33b60de14c6ffab3d6f Reject + sign in SplitHostPort (MarcoFalke)
fab4c2967d554ddbc15f732cea6cd190c547d89f net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652e68fc07fb6c9f3d9e34dc11d35f0cc1e1 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c45577dfbae67535e4965b5660288557d3631 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa980413257e2004a8d48a8be66c6d67f90b76ad refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7fc24212d85cdc7f52b317906b37a1a120 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)
Pull request description:
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:
* Useless, because the `+` sign was already rejected.
* Erroneous and inconsistent, when third party parsers reject it. (C.f. https://github.com/bitcoin/bitcoin/pull/32365)
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.
Fix all issues by removing the legacy int parsing.
ACKs for top commit:
stickies-v:
re-ACK faf55fc80b
brunoerg:
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
3a18075aedd7cff6f06b5fe10966d618b6378701 ci: Drop `-DENABLE_EXTERNAL_SIGNER=ON` configure option (Hennadii Stepanov)
719fa9f4ef6886c7c3be3b33d249b16f4e36a7e9 build: Re-enable external signer support for Windows (Hennadii Stepanov)
6e5fc2bf9b18cd492fe994dacb39182f601e9b86 test: Reintroduce Windows support in `system_tests/run_command` test (Hennadii Stepanov)
Pull request description:
This PR partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.h`.
ACKs for top commit:
Sjors:
ACK 3a18075aedd7cff6f06b5fe10966d618b6378701.
theStack:
Light ACK 3a18075aedd7cff6f06b5fe10966d618b6378701
laanwj:
Code review and lightly tested ACK 3a18075aedd7cff6f06b5fe10966d618b6378701
Tree-SHA512: 00d200685906e716750aae7cffa0794cca451653738ea590f50dfa28e1f3c5762a9be0ae0917aa0cf7436f00fe1e565236bff2853896530a5879466f7f45cb25
4b2cd0b41ff4800c8801f2c44883eaec60a035fa test: check that creating a wallet does not log version info (Ava Chow)
39a483c8e9dcfe8ec243fa72269e1df9e75059ab test: Check that the correct versions are logged on wallet load (Ava Chow)
359ecd3704993422eb53e3da2a7d0bea2f575ab0 walletdb: Log the wallet version after it has been read from disk (Ava Chow)
Pull request description:
The wallet's version (in the minversion record) needs to be logged only after we have read it from disk. Otherwise, we always log the lowest version number of 10500 which is incorrect. Furthermore, it doesn't make sense to log the last client version number if the record didn't exist. This is a regression caused by #26021.
The wallet file version logging is moved inside of `LoadMinVersion` so that it is logged after the record is read. It will also log unconditionally if a version is read so that the version number is reported even when there is an error. The last client logging is split into its own log line that will only occur if a last client record is read. The only situation where we expect no version numbers to be logged is when a wallet is being created.
A test is added in the second commit to check that the version number is correctly logged on loading. This commit can be cherrypicked to master to verify that it fails there. The last commit adds an additional check that creating a new wallet does not log any version info at all.
ACKs for top commit:
laanwj:
Code review ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
janb84:
ACK 4b2cd0b41f
furszy:
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
rkrux:
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
Tree-SHA512: b30c76f414d87be6c14b42d2d3c8794a91a7e8601501f4c24641d51ff2b5c5144776563baf41ca1c38415844740b760b19a3e5791f78013b39984dfedd3b1de7
fa58d6cdab000df288501db4a71487804b08ba4b ci: Move DEBUG=1 to centos task (MarcoFalke)
Pull request description:
The glibcxx debug mode has many bugs in prior gcc releases:
* https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890411766
* https://github.com/bitcoin/bitcoin/issues/31436#issuecomment-2530717875
* ...
Instead of working around all of them, just use the existing `ci_native_centos` task with gcc-14 to have it enabled. This also follows the logic of other sanitizers (tsan, asan, ubsan, msan, valgrind, ...) to generally prefer the latest version of the sanitizer for the latests features and bugfixes.
Fixes#32524.
Also, while touching the `ci_native_previous_releases`, increase g0 to g2, so that it is easier for developers to use gdb inside the CI without having to re-compile
ACKs for top commit:
hebasto:
ACK fa58d6cdab000df288501db4a71487804b08ba4b.
fanquake:
ACK fa58d6cdab000df288501db4a71487804b08ba4b
Tree-SHA512: 26f151887bc39e88353b4ba1f91e913b830d24eac258b7caa0027aa592595150d5e713ededa3ed15a1b84165a94b14b0bfe3eb2fb7314d261d972b63ce01af43
7193245cd66791216d4e586ca09302b26d4b7528 doc: remove For ... comments (fanquake)
1b9cdc933f6c11cb8593b5ad9ae3f4eb2c726859 net: drop win32 ifdef (fanquake)
19ba499b1f3884ea69c5f833f3eb797f90372aa5 init: cerrno is used on all platforms (fanquake)
Pull request description:
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs, if changed.
They are also wrong, i.e `DEFAULT_SCRIPTCHECK_THREADS` is not in
`validation.h`.
ACKs for top commit:
stickies-v:
re-ACK 7193245cd66791216d4e586ca09302b26d4b7528
fjahr:
ACK 7193245cd66791216d4e586ca09302b26d4b7528
willcl-ark:
reACK 7193245cd66791216d4e586ca09302b26d4b7528
Tree-SHA512: 6b5f83cd1df699356e1cbb78949f8d456b13ce288f0064138118cfb45b4c77e2d1945babe91598dffe9823ab07dfae36f4c3b61c586cf98baf16890bdf322b08
fa9198af55df74b0c19c9125d256ad4df83cf005 lint: Check for missing trailing newline (MarcoFalke)
fa2b2aa27c29fe810e296ef82126553b8f0d56e6 lint: Add archived notes to default excludes (MarcoFalke)
Pull request description:
A missing trailing newline is harmless, but a bit problematic:
* `git` shows a warning by default
* After another line is appended, the diff will be verbose and `git blame` will be wrong for the "untouched" line.
Fix the problems by just requiring what is already the default, see also 663a9cabf8/.editorconfig (L9) and 663a9cabf8/test/lint/test_runner/src/main.rs (L327)
ACKs for top commit:
l0rinc:
utACK fa9198af55df74b0c19c9125d256ad4df83cf005
fanquake:
ACK fa9198af55df74b0c19c9125d256ad4df83cf005
Tree-SHA512: d144eebdeee68fc3404aa4a66ecd5c130f907ed4b869bd300f6e9ed74d125561d1f4cdd6dd20d9e969471a7d007399f928f072d1c1f626275ca31f32bc23fdbc
In the dev notes, remove the whole section, because:
* ParseDouble was removed in commit
fa9d72a7947d2cff541794e21e0040c3c1d43b32
* The locale-dependent atoi is already checked by
test/lint/lint-locale-dependence.py
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
30a94b1ab9ae850d55cb9eb606a06890437bc75e test, wallet: Remove concurrent writes test (Ava Chow)
b44b7c03fef01e0b5db704e50762b3d16b3da69e wallet: Write best block record on unload (Ava Chow)
876a2585a8b69e12ac171a0d9ff5aab864067c42 wallet: Remove unnecessary database Close step on shutdown (Ava Chow)
98a1a5275c8c395fe47ff7f10109d75b06bc391d wallet: Remove chainStateFlushed (Ava Chow)
7fd3e1cf0c88553e0722048ce488f265883558e7 wallet, bench: Write a bestblock record in WalletMigration (Ava Chow)
6d3a8b195a826448c021dd189255ca41ba70cc5a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow)
7bacabb204b6c34f9545f0b37e2c66296ad2c0de wallet: Update best block record after block dis/connect (Ava Chow)
Pull request description:
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484
Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.
This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator.
To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again.
Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date.
ACKs for top commit:
rkrux:
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
mzumsande:
Code Review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
ryanofsky:
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029
fa982f14254433a969499e93c1c3f0db31dce6ab Use WitnessV0KeyHash in TestAddAddressesToSendBook (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32558
Fixes https://github.com/bitcoin-core/gui/issues/874
This fixes a bug introduced in commit fafee85358397289aa4c6b799d2603a5d89e83a2, which changed the type of the dummy address from `WitnessV0KeyHash` to `PKHash`. It was expected that this is fine, given that this is just a dummy address. However, the base58 characters can include the substring "io", leading to test failures later on.
Fix it by just using `WitnessV0KeyHash` again.
For reference, a passing test could look like:
```
Model contains 2 rows and 2 columns.
--- Model Data ---
Row 0 : "io - new A\tmxgkqJWAwfUwbgzZUsWrG1stKWV6fDn8YH"
Row 1 : "io - new B\tmhsxP2yrYDQiEncT8HzKxQSFSFJmUsudsP"
------------------
```
A failing test could look like:
```
Model contains 3 rows and 2 columns.
--- Model Data ---
Row 0 : "already here (s)\tmyDFZSKDQdPMMoSQgzkDtq2yioo8DA8qCX"
Row 1 : "io - new A\tmsAqQKjMrbxYRDhGXBBJ3yUEQxj5Bf5Njz"
Row 2 : "io - new B\tmtALQiit8dw33kznVfHDgE38ohfgz2Pchc"
------------------
FAIL! : AddressBookTests::addressBookTests() Compared values are not the same
Actual (table_view->model()->rowCount()): 3
Expected (2) : 2
Loc: [qt/test/addressbooktests.cpp(219)]
```
ACKs for top commit:
achow101:
ACK fa982f14254433a969499e93c1c3f0db31dce6ab
hebasto:
ACK fa982f14254433a969499e93c1c3f0db31dce6ab, I have reviewed the code along with the related changes from https://github.com/bitcoin/bitcoin/pull/32511.
Tree-SHA512: f55d7fe4193a0706e1a3ca1a2c0fbf2f04dc5b177699add00013ec56d64218ac85b80dad6e99f9fde26f4c9fca79f99e68ded057c5862364064404ac06b77176
e49a7274a2141dcb9e188bc4b45c2d7b928ccecd rpc: Avoid join-split roundtrip for user:pass for auth credentials (Vasil Dimov)
98ff38a6f1a8a1e214bd3905a2dcac31ae6c2f52 rpc: Perform HTTP user:pass split once in `RPCAuthorized` (laanwj)
879a17bcb1a5eab2ff1841ce5f3762dcccecb0ba rpc: Store all credentials hashed in memory (laanwj)
4ab9bedee9d86fdecaa4afbbf4214ca6c7d9a94e rpc: Undeprecate rpcuser/rpcpassword, change message to security warning (laanwj)
Pull request description:
This PR does two things:
### Undeprecate rpcuser/rpcpassword, change message to security warning
Back in 2015, in https://github.com/bitcoin/bitcoin/pull/7044, we added configuration option `rpcauth` for multiple RPC users. At the same time the old settings for single-user configuration `rpcuser` and `rpcpassword` were "soon" to be deprecated.
The main reason for this deprecation is that while `rpcpassword` stores the password in plain text, `rpcauth` stores a hash, so it doesn't appear in the configuration in plain text.
As the options are still in active use, actually removing them is expected to be a hassle to many, and it's not clear that is worth it. As for the security risk, in many kinds of setups (no wallet, containerized, single-user-single-application, local-only, etc) it is an unlikely point of escalation.
In the end, it is good to encourage secure practices, but it is the responsibility of the user. Log a clear warning but remove the deprecation notice (this is also the only place where the options appear as deprecated, they were never marked as such in the -help output).
<hr>
### Store all credentials hashed in memory
This gets rid of the special-casing of `strRPCUserColonPass` by hashing cookies as well as manually provided `-rpcuser`/`-rpcpassword` with a random salt before storing them.
Also take the opportunity to modernize the surrounding code a bit. There should be no end-user visible differences in behavior.
<hr>
Closes#29240.
ACKs for top commit:
1440000bytes:
utACK e49a7274a2
janb84:
reACK e49a7274a2
vasild:
ACK e49a7274a2141dcb9e188bc4b45c2d7b928ccecd
Tree-SHA512: 7162848ada4545bc07b5843d1ab6fb7e31fb26de8d6385464b7c166491cd122eac2ec5e70887c414fc136600482df8277dc0cc0541d7b7cf62c4f72e25bb6145
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
fa330a5e38a8e9937778eeb53f06390d943bc42c doc: Adjust stale MSVC bug url (MarcoFalke)
Pull request description:
The old url is stale, so use the current one. See https://github.com/bitcoin/bitcoin/pull/32552#issuecomment-2889188342
ACKs for top commit:
hebasto:
ACK fa330a5e38a8e9937778eeb53f06390d943bc42c.
Tree-SHA512: eb7813edb85f4bac06807bf2a35caf106a0faeffcbd0a4732c57edac07db00171f720888dbb7c35fae3fd515408ea7cb8e5dcbd3cf393caec23904cb800b8907
86de8c1668005304b2c630ca2ad4a8ca8e348e90 scripted-diff: test: remove 'descriptors=True' argument for `createwallet` calls (Sebastian Falbesoner)
Pull request description:
Descriptor wallets are already created by default [since v23.0](7710a31f0c/doc/release-notes/release-notes-23.0.md (L171)), but since the recent legacy wallet removal the `descriptors` parameter *must* be True for the `createwallet` RPC (see commit 9f04e02ffaee0fe64027dc56c7bea3885254321a), i.e. still passing it wouldn't contain any information for test readers anymore. So simply drop them in the functional tests in order to reduce code bloat. The only exception is calls to older versions, which happens in `wallet_backwards_compatibility.py` and is explicitly excluded in the scripted diff.
ACKs for top commit:
Sjors:
ACK 86de8c1668005304b2c630ca2ad4a8ca8e348e90
maflcko:
lgtm ACK 86de8c1668005304b2c630ca2ad4a8ca8e348e90
Tree-SHA512: 1acfae27bd960aeef9e1cf6e3f042752164a4d6869773c42df4c22c03dde0922993a3220fa14d52e75a0ff1f48c5194932b74a21427efbd496b0aaad7a2eafb2
Logging the wallet version before anything has been read from disk results
in the wrong version being logged.
Also split the last client version logging as it may not always be
present to be logged.
Descriptor wallets are already created by default since v23.0, but
since the recent legacy wallet removal this parameter *must* be True
(see commit 9f04e02ffaee0fe64027dc56c7bea3885254321a), i.e. still
passing it wouldn't contain any information for test readers
anymore. So simply drop them in the functional tests in order to
reduce code bloat.
-BEGIN VERIFY SCRIPT-
sed -i 's/, descriptors=True//g' $(git ls-files -- 'test/functional' ':(exclude)test/functional/wallet_backwards_compatibility.py')
sed -i '/descriptors=True,/d' ./test/functional/mempool_persist.py
-END VERIFY SCRIPT-
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
fa1f10a49e7c4f6377fbc7ae2f1520b38c86e5fa doc: Fix minor typos in rpc help (MarcoFalke)
fae840e94b79c427215c13e0fd5a9fcd71295ec9 rpc: Reject beginning newline in RPC docs (MarcoFalke)
fa414eda0834f6e8260855a01122b1bc1100922f scripted-diff: Remove unused leading newline in RPC docs (MarcoFalke)
Pull request description:
It is harmless, but newlines in the beginning read a bit odd ("nReturns"). So just require them to not be present.
The diff is large, but a trivial scripted-diff.
ACKs for top commit:
fanquake:
ACK fa1f10a49e7c4f6377fbc7ae2f1520b38c86e5fa
w0xlt:
ACK fa1f10a49e
Tree-SHA512: 5d2f9632f42ec1c02814d050f223941f436e5b0df426d7d6eb93fdd0ff118d57185af07b271dd73af63735dd17231125826c0c9ce0aad36bc8829c5b050a628c
7015052eba23368539dcd1a9b4217ce1cacd2999 build: remove Wsuggest-override suppression from leveldb build (fanquake)
e2c84b896fad37cbbf40c02f24c058b641e5bccc Squashed 'src/leveldb/' changes from 4188247086..113db4962b (fanquake)
Pull request description:
Pulls in
* https://github.com/bitcoin-core/leveldb-subtree/pull/51
Remove the related warning suppression.
ACKs for top commit:
l0rinc:
utACK 7015052eba23368539dcd1a9b4217ce1cacd2999
hebasto:
ACK 7015052eba23368539dcd1a9b4217ce1cacd2999, I've updated the `leveldb` subtree locally and got zero diff with this branch.
Tree-SHA512: 1ac7c8ecc9025086b429e12c22fc25f654eaf68fc9500b95341fb635cea12e7f80d69298cff120e8557a4f2f5809956a3b158cdb4db745cfa605c0df6f346423
When dealing with URI parts, it seems more consistent to use
corresponding SAFE_CHARS_URI mode in error messages.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
fa2be605fee42c1286de2ddefbec976dde2c35ba ci: Enable feature_init and wallet_reorgsrestore in valgrind task (MarcoFalke)
Pull request description:
The `fork()` isn't needed and in fact makes the forked process not react to signals (like kill or terminate), so just avoid it and run the valgrind process directly in the CI task.
Can be tested with something like:
`env -i HOME="$HOME" PATH="$PATH" USER="$USER" MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_valgrind.sh" ./ci/test_run_all.sh`
ACKs for top commit:
fanquake:
ACK fa2be605fee42c1286de2ddefbec976dde2c35ba - x86_64, aarch64
Tree-SHA512: 6293447d501191598c08f0cb9fcb4ed91e4cfec11255e702a926346ef8011d6ebc0ca12e9a1c14fa53541318c4e05dee5c96dfe965dcf4cf833c9392158ab883
