Age | Commit message (Collapse) | Author |
|
Winsock properly
fd4c6a10f2285f16c5d0215eb56a3060441f3ef2 test: Setup networking globally (Hennadii Stepanov)
Pull request description:
On the master branch, when compiling without external signer support, the `bench_bitcoin.exe` does not initialize Winsock DLL that is required, for example, here: https://github.com/bitcoin/bitcoin/blob/459272d639b9547f68000d2b9a5a0d991d477de5/src/bench/addrman.cpp#L124
Moreover, Windows docs explicitly [state](https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsacleanup) that `WSAStartup` and `WSACleanup` must be balanced:
> There must be a call to `WSACleanup` for each successful call to `WSAStartup`. Only the final `WSACleanup` function call performs the actual cleanup. The preceding calls simply decrement an internal reference count in the WS2_32.DLL.
That is not the case for our unit tests because the `SetupNetworking()` call is a part of the `BasicTestingSetup` fixture and is invoked multiple times, while `~CNetCleanup()` is invoked once only, at the end of the test binary execution.
This PR fixes Winsock DLL initialization and termination.
More docs:
- https://learn.microsoft.com/en-us/windows/win32/winsock/initializing-winsock
- https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsastartup
- https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsacleanup
Fix https://github.com/bitcoin/bitcoin/issues/28940.
ACKs for top commit:
maflcko:
lgtm ACK fd4c6a10f2285f16c5d0215eb56a3060441f3ef2
Tree-SHA512: d360eaf776943f7f7a35ed5a5f9f3228d9e3d18eb824e5997cdc8eadddf466abe9f2da4910ee3bb86bf5411061e758259f7e1ec344f234ef7996f1bf8781dcda
|
|
compat/assumptions.h
fa1a38470697796a1a67397a815c8f8256f59224 Move compat.h include from system.h to system.cpp (MarcoFalke)
88887531b704f3943fdb33abbdd5378ecfeee14f Move compat/assumptions.h include to one place that actually needs it (MarcoFalke)
77774110f4dd591a71441851813d59c03c9e3c78 Remove __cplusplus from compat/assumptions.h (MarcoFalke)
faa3d4f1d8ecff444be53215d72e32d71d9ce138 Remove duplicate NDEBUG check from compat/assumptions.h (MarcoFalke)
Pull request description:
Generally, compile-time checks should be close to the code that use them. Especially, since `compat/assumptions.h` is only included in one place, where iwyu suggests to remove it.
Fix all issues:
* The `NDEBUG` check is used in `util/check`, so it is redundant in `compat/assumptions.h`.
* The `__cplusplus` check is redundant with `doc/dependencies.md` (see commit message).
* Add missing `// IWYU pragma: keep` to avoid removing the include by accident.
ACKs for top commit:
achow101:
ACK fa1a38470697796a1a67397a815c8f8256f59224
TheCharlatan:
re-ACK fa1a38470697796a1a67397a815c8f8256f59224
theuni:
ACK fa1a38470697796a1a67397a815c8f8256f59224
Tree-SHA512: f8b6db84be5d8844a2267345c0b1405fcbc39b8b5eeaa24db5b8412a74145fe44cf188b6b0c39cc2b062690ed37ca5b4662473484afe28dbec6469e79961389b
|
|
|
|
|
|
bbbbdb0cd57d75a06357d2811363d30a498f4499 ci: Add filesystem lint check (MarcoFalke)
fada2f91108a56cc5c447bd6b6fac411e4d5cdca refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)
Pull request description:
Using `std::filesystem` is problematic:
* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.
Fix all issues by removing use of it and adding a linter to avoid using it again in the future.
ACKs for top commit:
TheCharlatan:
ACK bbbbdb0cd57d75a06357d2811363d30a498f4499
fanquake:
ACK bbbbdb0cd57d75a06357d2811363d30a498f4499 🦀
Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
|
|
Needed by multiprocess support code to pass parsed configuration to a spawned process.
|
|
|
|
|
|
Also add the <IWYU pragma: keep> to avoid removing it by accident.
|
|
All code in this repo uses <util/fs.h>, except for a few lines. This is
confusing and potentially dangerous, if the safe <util/fs.h> wrappers
are not used.
|
|
Since script/standard only contains things that are used by the Solver
and its callers, rename the files to script/solver.
|
|
Should no-longer be needed post #27872. If it is, then
suppress-external-warnings should be fixed.
|
|
Any identifier starting with two _, or one _ followed by a capital letter is reserved for the compiler and thus must not be used. See: https://stackoverflow.com/a/228797/7130273
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s '__pushKV' 'pushKVEnd'
s '_EraseTx' 'EraseTxNoLock'
s '_Other' 'Other'
-END VERIFY SCRIPT-
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/namespace\ util/namespace\ common/g' src/common/settings.cpp src/common/settings.h
sed -i 's/util\:\:GetSetting/common\:\:GetSetting/g' $( git grep -l 'util\:\:GetSetting')
sed -i 's/util\:\:Setting/common\:\:Setting/g' $( git grep -l 'util\:\:Setting')
sed -i 's/util\:\:FindKey/common\:\:FindKey/g' $( git grep -l 'util\:\:FindKey')
sed -i 's/util\:\:ReadSettings/common\:\:ReadSettings/g' $( git grep -l 'util\:\:ReadSettings')
sed -i 's/util\:\:WriteSettings/common\:\:WriteSettings/g' $( git grep -l 'util\:\:WriteSettings')
-END VERIFY SCRIPT-
|
|
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from code that is not strictly required by it.
The settings code belongs into the common library and namespace, since
the kernel library should not depend on it. See doc/design/libraries.md
for more information on this rationale.
Changing the namespace of the moved functions is scripted in the
following commit.
|
|
interface_ui from validation.
7d3b35004b039f2bd606bb46a540de7babdbc41e refactor: Move system from util to common library (TheCharlatan)
7eee356c0a7fefd70c8de21689efa335f52a69ba refactor: Split util::AnyPtr into its own file (TheCharlatan)
44de325d95447498036479c3112ba741caf45bf6 refactor: Split util::insert into its own file (TheCharlatan)
9ec5da36b62276ae22e348f26f88aaf646357d6d refactor: Move ScheduleBatchPriority to its own file (TheCharlatan)
f871c69191dfe1331861ebcdbadb6bd47e45c8b1 kernel: Add warning method to notifications (TheCharlatan)
4452707edec91c7d7991f486dd41ef3edb4f7fbf kernel: Add progress method to notifications (TheCharlatan)
84d71457e7250ab25c0a11d1ad1c7657197ffd90 kernel: Add headerTip method to notifications (TheCharlatan)
447761c8228d58f948aae7e73ed079c028cacb97 kernel: Add notification interface (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this final step: https://github.com/bitcoin/bitcoin/pull/27419 https://github.com/bitcoin/bitcoin/pull/27254 https://github.com/bitcoin/bitcoin/pull/27238.
`interface_ui` defines functions for a more general node interface and has a dependency on `boost/signals2`. After applying the patches from this pull request, the kernel's reliance on boost is down to `boost::multiindex`.
The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated.
ACKs for top commit:
MarcoFalke:
re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e (no change) 🎋
stickies-v:
Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e
hebasto:
re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e, only last two commits dropped since my [recent](https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435394620) review.
Tree-SHA512: c8cfc698dc9d78e20191c444708f2d957501229abe95e5806106d1126fb9c5fbcee686fb55645658c0107ce71f10646f37a2fdf7fde16bbf22cbf1ac885dd08d
|
|
eefe56967b4eb4b5144325cde4f40fc1cbde3e65 bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1b732a04976fc70cbb0661f97bbbd99 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b02197ad92fded20f6ff83b364747297 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
|
|
Since the kernel library no longer depends on the system file, move it
to the common library instead in accordance to the diagram in
doc/design/libraries.md.
|
|
This is a follow up of https://github.com/bitcoin/bitcoin/pull/27491,
more concretely
https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847896,
for not using default cases (as per the style guide), and
https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188852707 and
https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188851857 for
avoiding dead code.
Also change chain name to chain type in docstrings
|
|
This commit effectively moves the definition of these constants
out of the chainparamsbase to their own file.
Using the ChainType enums provides better type safety compared to
passing around strings.
The commit is part of an ongoing effort to decouple the libbitcoinkernel
library from the ArgsManager and other functionality that should not be
part of the kernel library.
|
|
These are introduced for the next commit where the usage of the
ChainType is adopted throughout the code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
|
|
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.
The bad log print was reported and originally fixed in
https://github.com/bitcoin/bitcoin/pull/27303 by
Matthew Zipkin <pinheadmz@gmail.com>
This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
|
|
Show an error on startup if a bitcoin datadir that is being used contains a
`bitcoin.conf` file that is ignored. There are two cases where this could
happen:
- One case reported in
https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043
happens when a bitcoin.conf file in the default datadir (e.g.
$HOME/.bitcoin/bitcoin.conf) has a "datadir=/path" line that sets different
datadir containing a second bitcoin.conf file. Currently the second
bitcoin.conf file is ignored with no warning.
- Another way this could happen is if a -conf= command line argument points
to a configuration file with a "datadir=/path" line and that specified path
contains a bitcoin.conf file, which is currently ignored.
This change only adds an error message and doesn't change anything about way
settings are applied. It also doesn't trigger errors if there are redundant
-datadir or -conf settings pointing at the same configuration file, only if
they are pointing at different files and one file is being ignored.
|
|
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
|
|
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
|
|
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code
reading config files and creating the datadir.
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the
GUI error text has changed from "Error: Cannot parse configuration file:" to
"Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the
error text has changed from "Failed loading settings file" to "Settings
file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the
error text has changed from "Failed saving settings file" to "Settings file
could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read),
there is an normal error dialog showing "Error: filesystem error: status:
Permission denied [.../settings.json]", instead of an uncaught exception
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
|
|
These belong in libbitcoin_common.a, not libbitcoin_util.a, because they aren't
general-purpose utilities, they just contain common code that is used by both
the node and the wallet. Another reason to reason to not include these in
libbitcoin_util.a is to prevent them from being used by the kernel library.
|
|
|
|
Quoting ryanofsky: "util can be the library for things included in the kernel
which the kernel can depend on, and common can be the library for other code
that needs to be shared internally, but should not be part of the kernel or
shared externally."
|
|
|
|
|
|
|
|
fa5d2e678c809c26bd40d7e7c171529d3ffb5903 Remove unused char serialize (MarcoFalke)
fa24493d6394b3a477535f480664c9596f18e3c5 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217b725ada35107b4ad646d250228355c span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
sipa:
re-utACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
|
|
|
|
This switches .read() and .write() to take spans of bytes.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
-BEGIN VERIFY SCRIPT-
# Move to directory
mkdir src/common
git mv src/bloom.cpp src/common/
git mv src/bloom.h src/common/
# Replace occurrences
sed -i 's|\<bloom\.cpp\>|common/bloom.cpp|g' $(git grep -l 'bloom.cpp')
sed -i 's|\<bloom\.h\>|common/bloom.h|g' $(git grep -l 'bloom.h')
sed -i 's|BITCOIN_BLOOM_H|BITCOIN_COMMON_BLOOM_H|g' $(git grep -l 'BLOOM_H')
-END VERIFY SCRIPT-
|