Age | Commit message (Collapse) | Author |
|
|
|
codespell in comments
f565b2836d5efeb6f7c16d0fac813b06fa4d41e4 Fixup option name in bench message (Ben Woosley)
bf209ac7a732394c3a54d6d1e3fb43f180ac1bb8 doc: Fix spelling errors identified by codespell in coments (Ben Woosley)
Pull request description:
From the output [here](https://cirrus-ci.com/task/5275612980969472?logs=lint#L849):
```
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
~~I left the 'nd' in miniscript_tests as-is, as it's valid miniscript,
and I'm wary of whitelisting it.~~
ACKs for top commit:
dunxen:
ACK f565b28
Tree-SHA512: 501a426c5f6f9761e2c8f980d5d955611428a827321888f53e0ae9526b0fecd43f9d1fa845fc70ae2489d77be6dc0b5b371dff55c5146f4b39ed874f4a1ea917
|
|
f9fdcec7e932843a91ddf7f377e00bd2a6efb82a settings: Add resetSettings() method (Ryan Ofsky)
77fabffef4ea840ee15c97061048fe8443d74658 init: Remove Shutdown() node.args reset (Ryan Ofsky)
0e55bc6e7fe439404dc56093a0949395dae51e6b settings: Add update/getPersistent/isIgnored methods (Ryan Ofsky)
Pull request description:
Add `interfaces::Node` `updateSetting`, `forceSetting`, `resetSettings`, `isSettingIgnored`, and `getPersistentSetting` methods so GUI is able to manipulate `settings.json` file and use and modify node settings.
(Originally this PR also contained GUI changes to unify bitcoin-qt and bitcoind persistent settings and call these methods, but the GUI commits have been dropped from this PR and moved to bitcoin-core/gui/pull/602)
ACKs for top commit:
vasild:
ACK f9fdcec7e932843a91ddf7f377e00bd2a6efb82a
hebasto:
re-ACK f9fdcec7e932843a91ddf7f377e00bd2a6efb82a, only a function renamed since my recent [review](https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-979324357).
Tree-SHA512: 4cac853ee29be96d2ff38404165b9dfb7c622b2a9c99a15979596f3484ffde0da3d9c9c372677dff5119ca7cffa6383d81037fd9889a29cc9285882a8dc0c268
|
|
From the output here:
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
https://cirrus-ci.com/task/5275612980969472?logs=lint#L849
I added 'nd' to the spelling.ignored-words.txt, as it's valid miniscript.
|
|
Since the removal of NODISCARD in 81d5af42f4dba5b68a597536cad7f61894dc22a3,
the only attributes def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
|
|
-BEGIN VERIFY SCRIPT-
sed -i -E -e '/^([a-z]+ )?Mutex [a-z]/ s/Mutex/GlobalMutex/' $(git grep -lE '^([a-z]+ )?Mutex [a-z]')
-END VERIFY SCRIPT-
|
|
facd1fb911abfc595a3484ee53397eff515d4c40 refactor: Use Span of std::byte in CExtKey::SetSeed (MarcoFalke)
fae1006019188700e0c497a63fc1550fe00ca8bb util: Add ParseHex<std::byte>() helper (MarcoFalke)
fabdf81983e2542d60542b80fb94ccb1acdd204a test: Add test for embedded null in hex string (MarcoFalke)
Pull request description:
This adds the hex->`std::byte` helper after the `std::byte`->hex helper was added in commit 9394964f6b9d1cf1220a4eca17ba18dc49ae876d
ACKs for top commit:
pk-b2:
ACK https://github.com/bitcoin/bitcoin/pull/23595/commits/facd1fb911abfc595a3484ee53397eff515d4c40
laanwj:
Code review ACK facd1fb911abfc595a3484ee53397eff515d4c40
Tree-SHA512: e2329fbdea2e580bd1618caab31f5d0e59c245a028e1236662858e621929818870b76ab6834f7ac6a46d7874dfec63f498380ad99da6efe4218f720a60e859be
|
|
1d4122dfefcb0a33c3d5bf7bbe2c7cd7e09d3764 init: Allow -proxy="" setting values (Ryan Ofsky)
Pull request description:
This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases.
The error was originally added in https://github.com/bitcoin/bitcoin/pull/20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well.
The motivation for this change is to prevent a GUI bug that happens with https://github.com/bitcoin/bitcoin/pull/15936, reported in https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.
ACKs for top commit:
hebasto:
re-ACK 1d4122dfefcb0a33c3d5bf7bbe2c7cd7e09d3764, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/24830#pullrequestreview-941255672).
Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53
|
|
fa305fd92c0a5a91831be3ccec0a5ef962a5fbcb Add mockable clock type and TicksSinceEpoch helper (MarcoFalke)
Pull request description:
This will be used primarily by the addr time refactor (https://github.com/bitcoin/bitcoin/pull/24697) to make addr relay time type safe. However, it can also be used in other places, and can be reviewed independently, so I split it up.
ACKs for top commit:
jonatack:
ACK fa305fd92c0a5a91831be3ccec0a5ef962a5fbcb per `git range-diff 7b3343f fa20781 fa305fd`
ajtowns:
ACK fa305fd92c0a5a91831be3ccec0a5ef962a5fbcb
Tree-SHA512: da00200126833c1f55b1b1e68f596eab5c9254e82b188ad17779c08ffd685e198a7c5270791b4b69a858dc6ba4e051fe0c8b445d203d356d0c884f6365ee1cfe
|
|
Allows the GUI to clear settings.json file and save settings.json.bak file when
GUI "Reset Options" button is pressed or -resetguisettings command line option
is used. (GUI code already backs up and resets the "guisettings.ini" file this
way, so this just makes the same behavior possible for "settings.json")
|
|
fa9af218780b7960d756db80c57222e5bf2137b1 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af218780b7960d756db80c57222e5bf2137b1
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
|
|
Add interfaces::Node methods to give GUI finer grained control over
settings.json file. Update method is used to write settings to the file,
getPersistent and isIgnored methods are used to find out about settings
file and command line option interactions.
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's|\<get_int64\>|getInt<int64_t>|g' $(git grep -l get_int ':(exclude)src/univalue')
sed -i 's|\<get_int\>|getInt<int>|g' $(git grep -l get_int ':(exclude)src/univalue')
-END VERIFY SCRIPT-
|
|
|
|
This will help to increase `Sock` usage and make more code mockable.
|
|
This will help to increase `Sock` usage and make more code mockable.
|
|
|
|
fab9e8a29c2cdeab6cf1ae7c1fc0e0a3af783b17 Remove unused GetTimeSeconds (MacroFake)
Pull request description:
Seems confusing to have this helper when it is possible to get the system time in a type-safe way by simply calling `std::chrono::system_clock::now` (C++11).
This patch replaces `GetTimeSeconds` and removes it:
* in `bitcoin-cli.cpp` by `system_clock`
* in `test/fuzz/fuzz.cpp` by `steady_clock`
ACKs for top commit:
laanwj:
Code review ACK fab9e8a29c2cdeab6cf1ae7c1fc0e0a3af783b17
naumenkogs:
ACK fab9e8a29c2cdeab6cf1ae7c1fc0e0a3af783b17
Tree-SHA512: 517e300b0baf271cfbeebd4a0838871acbea9360f9dd23572a751335c20c1ba261b1b5ee0aec8a36abd20c94fab83ce94f46042745279aca1f0ca2f885a03b6e
|
|
GetRandInt
ab1ea29ba1b8379a21fabd3dc859552c470a6421 refactor: make GetRand a template, remove GetRandInt (pasta)
Pull request description:
makes GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>()
ACKs for top commit:
laanwj:
Code review ACK ab1ea29ba1b8379a21fabd3dc859552c470a6421
Tree-SHA512: db5082a0e21783389f1be898ae73e097b31ab48cab1a2c0e29348a4adeb545d4098193aa72a547c6baa6e8205699aafec38d6a27b3d65522fb3246f91b4daae9
|
|
|
|
|
|
|
|
e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a8bc26e1612c771173325dbe49b3612 util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7b11b375042c3000d421fd93348c199 util: Refactor SysErrorString logic (laanwj)
e7f2f77756d33c6be9c8998a575b263ff2d39270 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbfbc39ebbc74ab1ed8c00edc12859373 util: Replace non-threadsafe strerror (laanwj)
Pull request description:
Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in #4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives (with code from `NetworkErrorString`) and replace all uses of `strerror` with this.
Edit: I've also added a commit that refactors the code so that buf[] is never read at all if the function fails, making some fragile-looking code unnecessary.
Edit2: from the linux manpage:
```
ATTRIBUTES
For an explanation of the terms used in this section, see attributes(7).
┌───────────────────┬───────────────┬─────────────────────────┐
│Interface │ Attribute │ Value │
├───────────────────┼───────────────┼─────────────────────────┤
│strerror() │ Thread safety │ MT-Unsafe race:strerror │
├───────────────────┼───────────────┼─────────────────────────┤
…
├───────────────────┼───────────────┼─────────────────────────┤
│strerror_r(), │ Thread safety │ MT-Safe │
│strerror_l() │ │ │
└───────────────────┴───────────────┴─────────────────────────┘
```
As the function can be called from any thread at any time, using a non-thread-safe function is unacceptable.
ACKs for top commit:
jonatack:
ACK e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c
Tree-SHA512: 20e71ebb9e979d4e1d8cafbb2e32e20c2a63f09115fe72cdde67c8f80ae98c531d286f935fd8a6e92a18b72607d7bd3e846b2d871d9691a6036b0676de8aaf25
|
|
5e61532e72c1021fda9c7b213bd9cf397cb3a802 util: optimizes HexStr (Martin Leitner-Ankerl)
4e2b99f72a90b956f3050095abed4949aff9b516 bench: Adds a benchmark for HexStr (Martin Leitner-Ankerl)
67c8411c37b483caa2fe3f7f4f40b68ed2a9bcf7 test: Adds a test for HexStr that checks all 256 bytes (Martin Leitner-Ankerl)
Pull request description:
In my benchmark, this rewrite improves runtime 27% (g++) to 46% (clang++) for the benchmark `HexStrBench`:
g++ 11.2.0
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.94 | 1,061,381,310.36 | 0.7% | 12.00 | 3.01 | 3.990 | 1.00 | 0.0% | 0.01 | `HexStrBench` master
| 0.68 | 1,465,366,544.25 | 1.7% | 6.00 | 2.16 | 2.778 | 1.00 | 0.0% | 0.01 | `HexStrBench` branch
clang++ 13.0.1
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.80 | 1,244,713,415.92 | 0.9% | 10.00 | 2.56 | 3.913 | 0.50 | 0.0% | 0.01 | `HexStrBench` master
| 0.43 | 2,324,188,940.72 | 0.2% | 4.00 | 1.37 | 2.914 | 0.25 | 0.0% | 0.01 | `HexStrBench` branch
Note that the idea for this change comes from denis2342 in #23364. This is a rewrite so no unaligned accesses occur.
Also, the lookup table is now calculated at compile time, which hopefully makes the code a bit easier to review.
ACKs for top commit:
laanwj:
Code review ACK 5e61532e72c1021fda9c7b213bd9cf397cb3a802
aureleoules:
tACK 5e61532e72c1021fda9c7b213bd9cf397cb3a802.
theStack:
ACK 5e61532e72c1021fda9c7b213bd9cf397cb3a802 🚤
Tree-SHA512: 40b53d5908332473ef24918d3a80ad1292b60566c02585fa548eb4c3189754971be5a70325f4968fce6d714df898b52d9357aba14d4753a8c70e6ffd273a2319
|
|
|
|
allowed by path append operators
f64aa9c411ad78259756a28756ec1eb8069b5ab4 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411ad78259756a28756ec1eb8069b5ab4
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
|
|
|
|
lint-assertions.py
fa82a1ed833fd749849fa19267207b63e338d84d lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (MacroFake)
Pull request description:
Follow up to commit b1c5991eebb916755be188f355ad36fe01a3f529. Also remove empty newline added in that commit.
ACKs for top commit:
fanquake:
ACK fa82a1ed833fd749849fa19267207b63e338d84d
Tree-SHA512: cf398eceb135672137183bfa19ee57a82553a3dbcbce74db954c6fcd79f9606092cc0d8217610fe6cd67b7ef2d4f01d90329f0f568516d9b14aa2cd0f0715478
|
|
Increase the error message buffer to 1024 as recommended in the manual
page (Thanks Jon Atack)
|
|
Deduplicate code and error checks by making sure `s` stays `nullptr`
in case of error. Return "Unknown error" instead of an empty string in
this case.
|
|
|
|
Some uses of non-threadsafe `strerror` have snuck into the code since
they were removed in #4152. Add a wrapper `SysErrorString` for
thread-safe strerror alternatives and replace all uses of `strerror`
with this.
|
|
|
|
Also, fix style in the corresponding function. The style change can be
reviewed with "--word-diff-regex=."
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's,ValidAsCString,ContainsNoNUL,g' $(git grep -l ValidAsCString)
-END VERIFY SCRIPT-
|
|
|
|
|
|
|
|
|
|
Base32/base64 are mechanisms for encoding binary data. That they'd
decode to a string is just bizarre. The fact that they'd do that
based on the type of input arguments even more so.
|
|
In addition, to make sure that no call site ignores the invalid
decoding status, make the pf_invalid argument mandatory.
|
|
|
|
|
|
|
|
|
|
This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>`
error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or
`settings.json` value is specified, and just makes bitcoin connect and listen
normally in these cases.
The error was originally added in https://github.com/bitcoin/bitcoin/pull/20003
to prevent a bare `-proxy` command line argument with no assignment from
clearing proxy settings. But it was implemented in an overbroad way breaking
empty `-proxy=` assignments as well.
The motivation for this change is to prevent a GUI bug that happens with
https://github.com/bitcoin/bitcoin/pull/15936, reported in
https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 by
vasild, that happens after a proxy setting is enabled and disabled in the GUI.
But this change also makes sense on its own to remove a potentially confusing
error message.
|
|
helper (boost::split replacement)
a62e84438d27ee6213219fe2c233e58814fcbb5d fuzz: add `SplitString` fuzz target (MarcoFalke)
4fad7e46d94a0fdee4ff917e81360d7ae6bd8110 test: add unit tests for `SplitString` helper (Kiminuo)
9cc8e876e412056ed22d364538f0da3d5d71946d refactor: introduce single-separator split helper `SplitString` (Sebastian Falbesoner)
Pull request description:
This PR adds a simple string split helper `SplitString` that takes use of the spanparsing `Split` function that was first introduced in #13697 (commit fe8a7dcd78cfeedc9a7c705e91384f793822912b). This enables to replace most calls to `boost::split`, in the cases where only a single separator character is used. Note that while previous attempts to replace `boost::split` were controversial (e.g. #13751), this one has a trivial implementation: it merely uses an internal helper (that is unit tested and in regular use with output descriptiors) and converts its result from spans to strings. As a drawback though, not all `boost::split` instances can be tackled.
As a possible optimization, one could return a vector of `std::string_view`s (available since C++17) instead of strings, to avoid copies. This would need more carefulness on the caller sites though, to avoid potential lifetime issues, and it's probably not worth it, considering that none of the places where strings are split are really performance-critical.
ACKs for top commit:
martinus:
Code review ACK a62e84438d27ee6213219fe2c233e58814fcbb5d. Ran all tests. I also like that with `boost::split` it was not obvious that the resulting container was cleared, and with `SplitString` API that's obvious.
Tree-SHA512: 10cb22619ebe46831b1f8e83584a89381a036b54c88701484ac00743e2a62cfe52c9f3ecdbb2d0815e536c99034558277cc263600ec3f3588b291c07eef8ed24
|
|
|
|
function and NONFATAL_UNREACHABLE macro
ee02c8bd9aedad8cfd3c2618035fe275da025fb9 util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès)
Pull request description:
This PR replaces the macro `CHECK_NONFATAL` with an identity function.
I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: https://github.com/bitcoin/bitcoin/pull/24804#discussion_r846182474.
Also adds `UNREACHABLE_NONFATAL` macro.
ACKs for top commit:
jonatack:
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9
MarcoFalke:
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2
|
|
|