It is part of the node library. Also, it won't be moved to the kernel
lib, as it will be pruned of ArgsManager.
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/mempool_args.cpp src/node/
git mv src/mempool_args.h src/node/
# Replacements
sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args)
sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args)
sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H)
-END VERIFY SCRIPT-
7878f97bf1 indexes, refactor: Remove CChainState use in index CommitInternal method (Ryan Ofsky)
ee3a079fab indexes, refactor: Remove CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
dc971be083 indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods (Ryan Ofsky)
bef4e405f3 indexes, refactor: Remove CBlockIndex* uses in index Init methods (Ryan Ofsky)
addb4f2af1 indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function (Ryan Ofsky)
33b4d48cfc indexes, refactor: Pass Chain interface instead of CChainState class to indexes (Ryan Ofsky)
a0b5b4ae5a interfaces, refactor: Add more block information to block connected notifications (Ryan Ofsky)
Pull request description:
Start transitioning index code away from using internal node types like `CBlockIndex` and `CChain` so index code is less coupled to node code and index code will later be able to stop locking cs_main and sync without having to deal with validationinterface race conditions, and so new indexes are easier to write and can run as plugins or separate processes.
This PR contains the first 7 commits from https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-1165625977 which have been split off for easier review. Previous review comments can be found in #24230
ACKs for top commit:
MarcoFalke:
ACK 7878f97bf1 though did not review the last commit 🤼
mzumsande:
Code Review ACK 7878f97bf1
Tree-SHA512: f84ac2eb6dca2c305566ddeb35ea14d0b71c00860c0fd752bbcf1a0188be833d8c2a6ac9d3ef6ab5b46fbd02d7a24cbb8f60cf12464cb8ba208e22287f709989
cb3e9a1e3f Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa30676541 Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8a LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b0 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e3756 mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741 test/mempool_persist: Test manual savemempool when -persistmempool=0 (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
This PR moves `{Dump,Load}Mempool` into its own `kernel/mempool_persist` module and introduces `ArgsManager` `node::` helpers in `node/mempool_persist_args`to remove the scattered calls to `GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)`.
More context can be gleaned from the commit messages.
-----
One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since `libbitcoinkernel` will include both validation and mempool, but perhaps something for the future.
ACKs for top commit:
glozow:
re ACK cb3e9a1e3f via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f🔒
ryanofsky:
Code review ACK cb3e9a1e3f
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
Also:
1. Have CChainState::LoadMempool and ::ThreadImport take in paths and
pass it through untouched to LoadMempool.
2. Make LoadMempool exit early if the load_path is empty.
3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass
in an empty path if mempool persistence is disabled.
m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.
-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@m_load_tried@g"
find_regex="\bIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@GetLoadTried@g"
find_regex="\bSetIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-
Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.
4c9666bd73 Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard)
aae66ab43d Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard)
3e27e31727 Introduce `mempoolfullrbf` node setting. (Antoine Riard)
Pull request description:
This is ready for review.
Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, ...) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0].
This PR implements a simple `fullrbf` setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays **false**, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one's application requirements, without arguing a change of the default behavior.
I [posted](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html) on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I'm proposing to express them on the future thread.
[0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html
[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html
Follow-up suggestions :
- soft-enable opt-in RBF in the wallet : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1154918789
- p2p discovery and additional outbound connection to full-rbf peers : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1156044401
- match the code between RPC, wallet and mempool about disregard of inherited signaling : #22698
ACKs for top commit:
instagibbs:
reACK 4c9666bd73
glozow:
ACK 4c9666bd73, a few nits which are non-blocking.
w0xlt:
ACK 4c9666bd73
Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
This new node policy setting enables to accept replaced-by-fee
transaction without inspection of the replaceability signaling
as described in BIP125 "explicit signaling".
If turns on, the node mempool accepts transaction replacement
as described in `policy/mempool-replacements.md`.
The default setting value is `false`, implying opt-in RBF
is enforced.
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_(ANCESTOR|DESCENDANT)_SIZE_LIMIT" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_KVB@g"
-END VERIFY SCRIPT-
Better to be explicit when it comes to time to avoid unintentional bugs.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MEMPOOL_EXPIRY" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_HOURS@g"
-END VERIFY SCRIPT-
Reviewers: Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.
We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).
1. We first set the default based on chainparams's
DefaultConsistencyChecks()
2. Then, we apply the ArgsManager option on top of that default
3. Finally, we clamp the result of that between 0 and 1 Million
With this patch, most CTxMemPool construction are along the lines of:
MemPoolOptions mempool_opts{...default overrides...};
ApplyArgsManOptions(argsman, mempool_opts);
...hard overrides...
CTxMemPool pool{mempool_opts};
This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.
For example:
- For users of libbitcoinkernel, where we eventually want to remove
ArgsManager, they simply won't call (or even know about)
ApplyArgsManOptions.
- See src/init.cpp to see how the check_ratio CTxMemPool option works
after this change.
A MemPoolOptionsForTest helper was also added and used by tests/fuzz
tests where a local CTxMemPool needed to be created.
The change in src/test/fuzz/tx_pool.cpp seemingly changes behaviour by
applying ArgsManager options on top of the CTxMemPool::Options defaults.
However, in future commits where we introduce flags like -maxmempool,
the call to ApplyArgsManOptions is actually what preserves the existing
behaviour. Previously, although it wasn't obvious, our CTxMemPool would
consult gArgs for flags like -maxmempool when it needed it, so it
already relied on ArgsManager information. This patchset just laid bare
the obfuscatory perils of globals.
[META] As this patchset progresses, we will move more and more
CTxMemPool-relevant options into MemPoolOptions and add their
ArgsMan-related logic to ApplyArgsManOptions.
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_MB@g"
-END VERIFY SCRIPT-
018d70b587 scripted-diff: Avoid incompatibility with CMake AUTOUIC feature (Hennadii Stepanov)
Pull request description:
Working on [migration](https://github.com/hebasto/bitcoin/pull/3) from Autotools to CMake build system, I found that our current code base needs to be adjusted.
CMake [allows](https://cmake.org/cmake/help/latest/prop_tgt/AUTOUIC.html) to
> handle the Qt `uic` code generator automatically
When using this feature, statements like `#include "ui_<ui_base>.h"` are processed in a special way.
The `node/ui_interface.h` unintentionally breaks this feature. Of course, it is possible to provide a list of source files to be excluded from `AUTOUIC`. But, unfortunately, this approach does not work for the `qt/sendcoinsdialog.cpp` source file, where there are both b71d37da2c/src/qt/sendcoinsdialog.cpp (L10) and b71d37da2c/src/qt/sendcoinsdialog.cpp (L24)
ACKs for top commit:
MarcoFalke:
cr ACK 018d70b587
ryanofsky:
Code review ACK 018d70b587
furszy:
Code review ACK 018d70b5
Tree-SHA512: 4fc83f2e5a82c8ab15c3c3d68f48b9863c47b96c0a66b6276b9b4dfc6063abffd73a16382acfe116553487b3ac697dbde2d9ada1b92010c5d8f8c6aa06f56428
ecff20db28 logging: use LogPrintfCategory rather than a manual category (Jon Atack)
eb8aab759f logging: add LogPrintfCategory to log unconditionally with category (Jon Atack)
Pull request description:
These are the next two commits from #25203.
- Add `LogPrintfCategory` to log unconditionally while prefixing the output with the passed category name. Add documentation and a unit test, and update the `lint-logs.py` and `lint-format-strings.py` scripts.
- Replace the log messages that manually print a category, with `LogPrintfCategory`. In upcoming commits, it will likely be used in many other cases, such as to replace `LogPrintf` where it makes sense.
ACKs for top commit:
klementtan:
Code Review ACK ecff20db28
laanwj:
Code review ACK ecff20db28
brunoerg:
ACK ecff20db28
Tree-SHA512: ad3a82835254f7606efcd14b88f3d9072f1eb9b25db1321ed38ef6a4ec60efd555d78f5e19d93736f2f8500251d06f8beee9d694a153f24bf5cce3590a2a45a5
ce893c0497 doc: Update developer notes (Anthony Towns)
d2852917ee sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497🐦
hebasto:
ACK ce893c0497
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
Here we update only the log messages that manually print a category.
In upcoming commits, LogPrintCategory will likely be used in many
other cases, such as to replace `LogPrintf` where it makes sense.
...instead of explicitly calling init::{Set,Unset}Globals.
Cool thing about this is that in both the testing and bitcoin-chainstate
codepaths, we no longer need to explicitly unset globals. The
kernel::Context goes out of scope and the globals are unset
"automatically".
Also construct kernel::Context outside of AppInitSanityChecks()
These checks were added in #4339, (see also #4081), to test
our back-compat stubs, however, those stubs no-longer exist (#22930),
meaning that these checks are now just testing some specific standard
library behaviour, without a particular rationale, or reason, compared
to any other standard library functions we use.
There has also been some discussion about the sanity checks in the
context of the libbitcoinkernel refactoring, see
https://github.com/bitcoin/bitcoin/pull/25065#discussion_r880668218.
Removing the checks removes the need to worry about atleast the glibcxx
checks.
Also remove the list of check from the doc in init.h, because it is
incomplete, and anyone who wants to know what checks are included can
look at the function.
f9fdcec7e9 settings: Add resetSettings() method (Ryan Ofsky)
77fabffef4 init: Remove Shutdown() node.args reset (Ryan Ofsky)
0e55bc6e7f 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 f9fdcec7e9
hebasto:
re-ACK f9fdcec7e9, only a function renamed since my recent [review](https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-979324357).
Tree-SHA512: 4cac853ee29be96d2ff38404165b9dfb7c622b2a9c99a15979596f3484ffde0da3d9c9c372677dff5119ca7cffa6383d81037fd9889a29cc9285882a8dc0c268
Since the removal of NODISCARD in 81d5af42f4,
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
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
[META] Although it seems like we don't need it for just one option,
we're going to introduce another member to this struct *in the
next commit*. In future patchsets for libbitcoinkernel decoupling
it from ArgsManager, even more members will be added here.
fa1b76aeb0 Do not call global Params() when chainman is in scope (MacroFake)
fa30234be8 Do not pass CChainParams& to PeerManager::make (MacroFake)
fafe5c0ca2 Do not pass CChainParams& to BlockAssembler constructor (MacroFake)
faf012b438 Do not pass Consensus::Params& to Chainstate helpers (MacroFake)
fa4ee53dca Do not pass time getter to Chainstate helpers (MacroFake)
Pull request description:
It seems confusing to pass chain params, consensus params, or a time function around when it is not needed.
Fix this by:
* Inlining the passed time getter function. I don't see a use case why this should be mockable.
* Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible.
ACKs for top commit:
promag:
Code review ACK fa1b76aeb0.
vincenzopalazzo:
ACK fa1b76aeb0
Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
1d4122dfef 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 1d4122dfef, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/24830#pullrequestreview-941255672).
Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53
This commit removes the `node.args = nullptr` assignment in the Shutdown()
function.
Clearing node.args there never made sense because it made the
Shutdown() function not idempotent, making it fragile and causing issues like
https://github.com/bitcoin/bitcoin/issues/23186.
The assignment also causes segfaults in GUI unit tests when a new
node().initParameterInteraction() call is added in OptionsModel to apply to Qt
settings (happens because AppTests calls Shutdown() which sets node.args to
null, and OptionTests runs after AppTests and then needs node.args not to be
null.)
bb5c24b120 validation: move g_versionbitscache into ChainstateManager (Anthony Towns)
eca22c726a test/versionbits: make versionbitscache a parameter (Anthony Towns)
d603f1d8a7 deploymentstatus: make versionbitscache a parameter (Anthony Towns)
78adef1753 refactor: use chainman instead of chainParams for DeploymentActive* (Anthony Towns)
deffe0df6c deploymentstatus: allow chainman in place of consensusParams (Anthony Towns)
eaa2e3f25c validation: move UpdateUncommittedBlockStructures and GenerateCoinbaseCommitment into ChainstateManager (Anthony Towns)
5c67e84d37 validation: replace ::Params() calls with chainstate/chainman member (Anthony Towns)
38860f93b6 validation: remove redundant CChainParams params from ChainstateManager methods (Anthony Towns)
69675ea4e7 validation: add CChainParams to ChainstateManager (Anthony Towns)
Pull request description:
Gives `ChainstateManager` a reference to the `CChainParams` its working on, and simplifies some of the functions that would otherwise take that as a parameter. Removes the `g_versionbitscache` global by moving it into `ChainstateManager`.
ACKs for top commit:
dongcarl:
reACK bb5c24b120
MarcoFalke:
review ACK bb5c24b120📙
Tree-SHA512: 3fa74905e5df561e3e74bb0b8fce6085c5311e6633e7d74c0fb0c82a907f5bbb1fd4ebc5d11d4f0b1c019bb51eabb9f6e4bcc4652a696d36a5878c807b85f121
ab1ea29ba1 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 ab1ea29ba1
Tree-SHA512: db5082a0e21783389f1be898ae73e097b31ab48cab1a2c0e29348a4adeb545d4098193aa72a547c6baa6e8205699aafec38d6a27b3d65522fb3246f91b4daae9
b42643c253 doc: update init.cpp -conf help text (josibake)
970b9987ad doc: update devtools, release-process readmes (josibake)
50635d27b4 build: include bitcoin.conf in build outputs (josibake)
6aac946f49 doc: update bitcoin-conf.md (Josiah Baker)
1c7e820ded script: add script to generate example bitcoin.conf (josibake)
b483084d86 doc: replace bitcoin.conf with placeholder file (josibake)
Pull request description:
create a script for parsing the output from `bitcoind --help` to create an example conf file for new users
## problem
per #10746 , `bitcoin.conf` not being put into the data directory during installation causes some confusion for users when running bitcoin. in the discussion on the issue, one proposed solution was to have an example config file and instruct users to `cp` it into their data directory after startup. in addition to #10746 , there have been other requests for a "skeleton config file" (https://github.com/bitcoin/bitcoin/issues/19641) to help users get started with configuring bitcoind.
the main issue with an example config file is that it creates a second source of truth regarding what options are available for configuring bitcoind. this means any changes to the options (including the addition or removal of options) would have to be updated for the command line and also updated in the example file.
this PR addresses this issue by providing a script to generate an example file directly from the `bitcoind --help` on-demand by running `contrib/devtools/gen-bitcoin-conf.sh`. this solution was originally proposed on #10746 and would also solve #19641 . this guarantees any changes made to the command-line options or the command-line options help would also be reflected in the example file after compiling and running the script.
the main purpose of this script is to generate a config file to be included with releases, same as `gen-manpages.sh`. this ensures every release also includes an up-to-date, full example config file for users to edit. the script is also available for users who compile from source for generating an example config for their compiled binary.
## special considerations
this removes the `bitcoin.conf` example file from the repo as it is now generated by this script. the original example file did contain extra text related to how to use certain options but going forward all option help docs should be moved into `init.cpp`
this also edits `init.cpp` to have the option help indicate that `-conf` is not usable from the config file. this is similar to how `-includeconf` 's help indicates it cannot be used from the command line
ACKs for top commit:
laanwj:
Tested and code review ACK b42643c253
Tree-SHA512: 4546e0cef92aa1398da553294ce4712d02e616dd72dcbe0b921af474e54f24750464ec813661f1283802472d1e8774e634dd1cc26fbf1f13286d3e0406c02c09
e3a06a3c6c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7 util: Refactor SysErrorString logic (laanwj)
e7f2f77756 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbf 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 e3a06a3c6c
Tree-SHA512: 20e71ebb9e979d4e1d8cafbb2e32e20c2a63f09115fe72cdde67c8f80ae98c531d286f935fd8a6e92a18b72607d7bd3e846b2d871d9691a6036b0676de8aaf25
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.
Fixes https://github.com/bitcoin/bitcoin/issues/22964
Previously, we used UnloadBlockIndex() in order to reset node.mempool
and node.chainman. However, that has proven to be fragile (see
https://github.com/bitcoin/bitcoin/issues/22964), and requires
UnloadBlockIndex and its callees to be updated manually for each member
that's introduced to the mempool and chainman classes.
In this commit, we stop using the UnloadBlockIndex function and we
simply reconstruct node.mempool and node.chainman.
Since PeerManager needs a valid reference to both node.mempool and
node.chainman, we also move PeerManager's construction via `::make` to
after the chainstate activation sequence is complete.
There are no more callers to UnloadBlockIndex after this commit, so it
and its sole callees can be pruned.
71c3f0356c move-only: Rename index + pruning functional test (Fabian Jahr)
de08932efa test: Update test for indices on pruned nodes (Fabian Jahr)
825d19839b Index: Allow coinstatsindex with pruning enabled (Fabian Jahr)
f08c9fb0c6 Index: Use prune locks for blockfilterindex (Fabian Jahr)
2561823531 blockstorage: Add prune locks to BlockManager (Fabian Jahr)
231fc7b035 refactor: Introduce GetFirstStoredBlock helper function (Fabian Jahr)
Pull request description:
# Motivation
The main motivation of this change and only behavior change noticeable by user is to allow running `coinstatsindex` on pruned nodes as has been requested [here for example](https://twitter.com/benthecarman/status/1388170854140452870?s=20).
# Background
`coinstatsindex` on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run `blockfilterindex`on pruned nodes was added in #15946 but it also added the `blockfilterindex` as a dependency to `validation` and it introduced two new circular dependencies. Enabling `coinstatsindex` on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.
Instead, this PR introduces a `m_prune_blockers` map to `BlockManager` as a flexible approach to block pruning. Entities like `blockfilterindex`, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.
# Alternative approach
Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:
- Usage of globals
- Blocks pruning with a start and a stop height
- Can persist blockers across restarts
- Blockers can be set/unset via RPCs
Personally, I don't think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.
ACKs for top commit:
mzumsande:
Code review ACK 71c3f0356c
ryanofsky:
Code review ACK 71c3f0356c. Changes since last review: just tweaking comments and asserts, and rebasing
w0xlt:
tACK 71c3f0356c on signet.
Tree-SHA512: de7efda08b44aa31013fbebc47a02cd2de32db170b570f9643e1f013fee0e8e7ca3068952d1acc6e5e74a70910735c5f263437981ad73df841ad945b52d36b71
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.
dac44fc06f init: disallow reindex-chainstate with optional indexes (Martin Zumsande)
62e14285f9 doc: Add note that -reindex will rebuild optional indexes (Martin Zumsande)
Pull request description:
When started together with `-reindex-chainstate`, currently coinstatsindex gets corrupted and the blockfilterindex flatfiles duplicated. See the OP of #24630 for more a more detailed explanation on why this happens.
This is an alternative to #24630 which does not wipe and rebuild the indexes but returns an `InitError` when they are activated, thus requiring the user to deactivate them temporarily until the `-reindex-chainstate` run is finished.
This also disallows `-reindex-chainstate` in combination with `-txindex`, which is not leading to corruption, but currently still rebuilds the index unnecessarily and unexpectedly.
As a long-term goal, it would be desirable to have the indexes tolerate `reindex-chainstate` by ignoring their `BlockConnected` notifications (there is discussion in #24630 about this) or possibly move `reindex-chainstate` option into a `bitcoin-chainstate` executable, which could also solve the problem. But these would be larger projects - until then, it might be better to disallow the interaction than having corrupted indexes.
The first commit adjusts the `-reindex` doc to mention that this option does rebuild all active indexes.
ACKs for top commit:
ryanofsky:
Code review ACK dac44fc06f. Just fixed IsArgSet call and edited error messages since last review
Tree-SHA512: c1abf7d350648ae227c3fd6c95d9a54c3bac9de70915275dea1c87cca6d9a76a056c0e306d95ef8cfe4df1f8525b418e0e7a4f52ded3be464041c0dc297f8930
36f814c0e8 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e63 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4580 [init] Add netgroupman to node.context (John Newbery)
9b3836710b [build] Add netgroup.cpp|h (John Newbery)
Pull request description:
The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.
This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.
ACKs for top commit:
mzumsande:
Code Review ACK 36f814c0e8
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c0e8, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e8
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
f0a2fb3c5d scripted-diff: Rename pindexBestHeader, fHavePruned (Carl Dong)
a401402125 Clear fHavePruned in BlockManager::Unload() (Carl Dong)
3308ecd3fc move-mostly: Make fHavePruned a BlockMan member (Carl Dong)
c96524113c Clear pindexBestHeader in ChainstateManager::Unload() (Carl Dong)
73eedaaacc style-only: Miscellaneous whitespace changes (Carl Dong)
0d567daf23 move-mostly: Make pindexBestHeader a ChainMan member (Carl Dong)
5d670173a3 validation: Load pindexBestHeader in ChainMan (Carl Dong)
Pull request description:
Split off from #22564 per Marco's suggestion: https://github.com/bitcoin/bitcoin/pull/22564#issuecomment-1100011503
This is basically the move-mostly parts of #22564. The overall intent is to move mutable globals manually reset by `::UnloadBlockIndex` into appropriate structs such that they are cleared at the appropriate times. Please read #22564's description for more rationale.
In summary , this PR moves:
1. `pindexBestHeader` -> `ChainstateManager::m_best_header`
2. `fHavePruned` -> `BlockManager::m_have_pruned`
ACKs for top commit:
ajtowns:
ACK f0a2fb3c5d -- code review only
MarcoFalke:
kirby ACK f0a2fb3c5d😋
Tree-SHA512: 8d161701af81af1ff42da1b22a6bef2f8626e8642146bc9c3b27f3a7cd24f4d691910a2392b188ae058fec0611a17304dd73f60da695f53832d327f73d2fc963
This is constructed before addrman and connman, and destructed afterwards.
netgroupman does not currently do anything, but will have functionality added in future commits.
Prevent -noproxy and -proxy=0 settings from interacting with -listen, -upnp,
and -natpmp settings.
These settings started being handled inconsistently in the `AppInitMain` and
`InitParameterInteraction` functions starting in commit
baf05075fa from #6272:
baf05075fa/src/init.cpp (L990-L991)baf05075fa/src/init.cpp (L687)
This commit changes both functions to handle proxy arguments the same way so
there are not side effects from specifying a proxy=0 setting.
fabdf9f870 Remove gui-only syscalls (MarcoFalke)
fa0c2aa826 init: Disable syscall sandbox in the bitcoin-qt process (MarcoFalke)
Pull request description:
It is basically impossible (and a bit out of scope) for us to maintain a sandbox for the qt library. I am not sure if it is possible to only sandbox a few threads in a process, but I doubt this will add no practical benefit anyway, so I am disabling the sandbox for the whole bitcoin-qt process.
See also https://github.com/bitcoin/bitcoin/pull/24690#issuecomment-1084372400
ACKs for top commit:
laanwj:
Code review ACK fabdf9f870
Tree-SHA512: 944ded03ee25f7dfd0bfeea9c3f97f575f2d470aa03b387b07f3e3bec5cb886e4aaa17e4a9fb359d3e670e6da69adc9111673d13e6561ec55b3161bb67dfe760
fa9112aac0 Remove utxo db upgrade code (MarcoFalke)
Pull request description:
It is not possible to upgrade Bitcoin Core pre-segwit (pre-0.13.1) to a recent version without a full IBD from scratch after commit 19a56d1519 (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0c (released in version 0.15), can upgrade to any version that is supported as of today.
This leaves the versions 0.13.1-0.14.x. Even though those versions are unsupported, some users with an existing datadir may want to upgrade to a recent version. However, it seems reasonable to simply ask them to `-reindex` to run a full IBD from scratch. This allows us to remove the utxo db upgrade code.
ACKs for top commit:
Sjors:
re-ACK fa9112aac0
laanwj:
Code review ACK fa9112aac0
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
58a14795b8 test: passing -onlynet=onion with -onion=0/-noonion raises expected init error (Jon Atack)
7000f66d36 test: passing -onlynet=onion without -proxy/-onion raises expected init error (Jon Atack)
8332e6e4cf test: passing invalid -onion raises expected init error (Jon Atack)
d5edb08708 test: passing invalid -proxy raises expected init error (Jon Atack)
bd57dcbaf2 test: hoist proxy out of 2 network loops in feature_proxy.py (Jon Atack)
afdf2de282 test: add CJDNS to LimitedAndReachable_Network unit tests (Jon Atack)
2b7a8180a9 net, init: assert each network reachability is true by default (Jon Atack)
Pull request description:
Adds missing network reachability test coverage and an assertion during init, noticed while reviewing #22834:
- assert during init that each network reachability is true by default
- add CJDNS to the `LimitedAndReachable_Network` unit tests
- hoist proxy out of two network loops in feature_proxy.py
- test that passing invalid `-proxy` raises expected init error
- test that passing invalid `-onion` raises expected init error
- test that passing `-onlynet=onion` without `-proxy` and `-onion` raises expected init error
- test that passing `-onlynet=onion` with `-onion=0` and with `-noonion` raises expected init error
ACKs for top commit:
vasild:
ACK 58a14795b8
brunoerg:
ACK 58a14795b8
dongcarl:
Code Review ACK 58a14795b8
Tree-SHA512: bdee6dd0c12bb63591ce7c9321fe77b509ab1265123054e774adc38a187746dddafe1627cbe89e990bcc78b45e194bfef8dc782710d5b217e2e2106ab0158827
60aa179d8f Use GetPathArg where possible (Pavol Rusnak)
5b946edd73 util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky)
687e655ae2 util: Add GetPathArg default path argument (Ryan Ofsky)
Pull request description:
Improve `ArgsManager::GetPathArg` method added in recent PR #24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for #24274 which is parsing more path options.
- Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values.
- Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated.
- Add unit tests for default and negated cases
- Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable.
ACKs for top commit:
w0xlt:
Tested ACK 60aa179 on Ubuntu 21.10
hebasto:
re-ACK 60aa179d8f
Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd
and harmonize them as follows
- s/outgoing/automatic outbound/
- s/Incoming/Inbound and manual/ (are not affected by this option.)
- s/only through network/only to network/
- s/this option. This option/this option. It/
- s/network types/networks/
and also pick up a few nits in doc/p2p-bad-ports.md
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
