fa1b4e5c32 Use steady clock in FlushStateToDisk (MarcoFalke)
1111e2f8b4 Use steady clock in SeedStrengthen and FindBestImplementation (MarcoFalke)
Pull request description:
There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing `SeedStrengthen`.
Fix this by using steady clock.
Do the same in `FindBestImplementation`, which shouldn't be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
Do the same in `FlushStateToDisk`, which should make the flushes more steady, if the system clock is adjusted by a large offset.
ACKs for top commit:
john-moffett:
ACK fa1b4e5c32
willcl-ark:
ACK fa1b4e5c3
Tree-SHA512: cc625e796b186accd53222bd64eb57d0512bc7e588312d254349b542bbc5e5daac348ff2b3b3f7dc5ae0bbbae2ec11fdbf3022cf2164211633765a4b0108e83e
802cc1ef53 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c671 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd7 Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
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.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef53
TheCharlatan:
ACK 802cc1ef53
achow101:
ACK 802cc1ef53
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
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
Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.
9a9d5da11f refactor: Stop using gArgs global in system.cpp (Ryan Ofsky)
b20b34f5b3 refactor: Use new GetConfigFilePath function (Ryan Ofsky)
Pull request description:
Most of the code in `util/system.cpp` that was hardcoded to use the global `ArgsManager` instance `gArgs` has been changed to stop using it (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a few hardcoded references to `gArgs` remain. This commit removes the last ones so these functions aren't reading or writing global state.
Noticed these `gArgs` references while reviewing #27073
ACKs for top commit:
achow101:
ACK 9a9d5da11f
stickies-v:
ACK 9a9d5da11
willcl-ark:
tACK 9a9d5da11
Tree-SHA512: 2c74b0d5fc83e9ed2ec6562eb26ec735512f75db8876a11a5d5f04e6cdbe0cd8beec19894091aa2cbf29319194d2429ccbf8036f5520ecc394f6fe89a0079a7b
Since #27073, the behaviour of GetDataDir changed to only return
the datadir path, but not create it. This also changed the behaviour
of GetDataDirNet and GetDataDirBase but the docs do not yet reflect
that.
Most of the code in util/system.cpp that was hardcoded to use the global
ArgsManager instance `gArgs` has been changed to work with explicit ArgsManager
instances (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a
few hardcoded references to `gArgs` remain. This commit removes the last ones
so these functions aren't reading or writing global state.
faab273e06 util: Return empty vector on invalid hex encoding (MarcoFalke)
fa3549a77b test: Add hex parse unit tests (MarcoFalke)
Pull request description:
Seems a bit confusing to happily accept random bytes and pretend they are hex encoded strings.
ACKs for top commit:
stickies-v:
re-ACK faab273e06
Tree-SHA512: a808135f744f50aece03d4bf5a71481c7bdca1fcdd0d5b113abdb0c8b382bf81cafee6d17c239041fb49b59f4e19970f24a475378e7f711c3a47d6438de2bdab
64c105442c util: make GetDataDir read-only & create datadir.. (willcl-ark)
56e370fbb9 util: add ArgsManager datadir helper functions (willcl-ark)
Pull request description:
Fixes #20070
Currently `ArgsManager::GetDataDir()` ensures it will always return a datadir by creating one if necessary. The function is shared between `bitcoind` `bitcoin-qt` and `bitcoin-cli` which results in the undesirable behaviour described in #20070.
This PR splits out the part of the function which creates directories and adds it as a standalone function, only called as part of `bitcoind` and `bitcoin-qt` init, but not `bitcoin-cli`.
`ReadConfigFiles`' behavior is changed to use the absolute path of the config file in error and warning messages instead of a relative path.
This was inadvertantly the form being tested [here](73966f75f6/test/functional/feature_config_args.py (L287)), whilst we were _not_ testing that a relative path was returned by the message even though we passed a relative path in as argument.
ACKs for top commit:
achow101:
ACK 64c105442c
hebasto:
re-ACK 64c105442c, only comments have been adjusted as requsted since my previous [review](https://github.com/bitcoin/bitcoin/pull/27073#pullrequestreview-1307435890).
TheCharlatan:
Re-ACK 64c105442c
ryanofsky:
Code review ACK 64c105442c. Only comment changes since last review
Tree-SHA512: b129501346071ad62551c9714492b21536d0558a94117d97218e255ef4e948d00df899a4bc2788faea27d3b1f20fc6136ef9d03e6a08498d926d9ad8688d6c96
7013da07fb Add release note for PR#25943 (David Gumberg)
04f270b435 Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg)
Pull request description:
This PR adds a user configurable, zero by default parameter — `maxburnamount` — to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes #25899.
As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions:
1. Those that begin with `OP_RETURN` - (datacarriers)
2. Those whose lengths exceed the script limit.
3. Those that contain invalid opcodes.
The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool.
I see two legitimate use cases for this override:
1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain.
2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
ACKs for top commit:
glozow:
reACK 7013da07fb
achow101:
re-ACK 7013da07fb
Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
.. only in bitcoind and bitcoin-qt
This changes behaviour of GetConfigFilePath which now always returns the
absolute path of the provided -conf argument.
561848aaf2 Exercise non-DIRTY spent coins in caches in fuzz test (Pieter Wuille)
59e6828bb5 Add deterministic mode to CCoinsViewCache (Pieter Wuille)
b0ff310840 Add CCoinsViewCache::SanityCheck() and use it in fuzz test (Pieter Wuille)
3c9cea1340 Add simulation-based CCoinsViewCache fuzzer (Pieter Wuille)
Pull request description:
The fuzzer goes through a sequence of operations that get applied to both a real stack of `CCoinsViewCache` objects, and to simulation data, comparing the two at the end.
ACKs for top commit:
jamesob:
re-ACK 561848aaf2
dergoegge:
Code review ACK 561848aaf2
Tree-SHA512: 68634f251fdb39436b128ecba093f651bff12ac11508dc9885253e57fd21efd44edf3b22b0f821c228175ec507df7d46c7f9f5404fc1eb8187fdbd136a5d5ee2
b89530483d util: move threadinterrupt into util (fanquake)
Pull request description:
Alongside thread and threadnames. It's part of libbitcoin_util.
ACKs for top commit:
ryanofsky:
Code review ACK b89530483d. No changes since last review other than rebase
theuni:
ACK b89530483d.
Tree-SHA512: 0421f4d1881ec295272446804b27d16bf63e6b62b272f8bb52bfecde9ae6605e8109ed16294690d3e3ce4b15cc5e7c4046f99442df73adb10bdf069d3fb165aa
fa3ea81c3e refactor: Add LIFETIMEBOUND / -Wdangling-gsl to Assert() (MacroFake)
Pull request description:
Currently compiles clean, but I think it may still be useful.
Can be tested by adding an `&`:
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 5766fff92d..300c1ec60f 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -125,7 +125,7 @@ BOOST_AUTO_TEST_CASE(util_check)
// Check -Wdangling-gsl does not trigger when copying the int. (It would
// trigger on "const int&")
- const int nine{*Assert(std::optional<int>{9})};
+ const int& nine{*Assert(std::optional<int>{9})};
BOOST_CHECK_EQUAL(9, nine);
}
```
Output:
```
test/util_tests.cpp:128:29: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
const int& nine{*Assert(std::optional<int>{9})};
^~~~~~~~~~~~~~~~~~~~~
./util/check.h:75:50: note: expanded from macro 'Assert'
#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
^~~
1 warning generated.
ACKs for top commit:
jonatack:
ACK fa3ea81c3e
theuni:
ACK fa3ea81c3e
Tree-SHA512: 17dea4d75f2ee2bf6e1b6a6f6d8f439711c777df0390574e8d8edb6ac9ee807a135341e4439050bd6a15ecc4097a1ba9a7ab15d27541ebf70a4e081fa6871877
deba6fe315 test: update feature_config_args.py (josibake)
2e3826cbcd util: warn if reindex is used in conf (josibake)
5e744f4238 util: disallow setting conf in bitcoin.conf (josibake)
Pull request description:
In help from `bitcoind -h` it specifes that `conf` can only be used from the commandline. However, if `conf` is set in a `bitcoin.conf` file, there is no error and from reading the logs it seems as if the `conf=<other file>` is being used, despite it being ignored. To recreate, you can setup a `bitcoin.conf` file in the default directory, add `conf=<some other file>.conf` and in the separate config file set whichever config value you want and verify that it is being ignored. alternatively, if you set `includeconf=<some other file>.conf` , your config in `<some other file>` will be picked up.
This PR fixes this by having the node error when reading the config file if `conf=` is set.
Additionally, it was mentioned in a recent [PR review club](https://bitcoincore.reviews/24858) that if `reindex=1` is set in the config file, the node will reindex on every startup, which is undesirable:
```irc
17:14 <larryruane> michaelfolkson: Reindex is requested by the user (node operator) as a configuration option (command line or in the config file, tho you probably would never put it in the file, or else it would reindex on every startup!)
```
This PR also has a commit to warn if `reindex=1` is set in the config file.
ACKs for top commit:
hebasto:
ACK deba6fe315, tested on Ubuntu 22.04.
aureleoules:
tACK deba6fe315
ryanofsky:
Code review ACK deba6fe315.
Tree-SHA512: 619fd0aa14e98af1166d6beb92651f5ba3f10d38b8ee132957f094f19c3a37313d9f4d7be2e4019f3fc9a2ca5fa42d03eb539ad820e27efec7ee58a26eb520b1
b527b54950 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f7682 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac55679 net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0a moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp} (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
* convert standalone `IsSelectableSocket()` to `Sock::IsSelectable()`
* convert standalone `SetSocketNonBlocking()` to `Sock::SetNonBlocking()`
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
jonatack:
ACK b527b54950 review/debug build/unit tests at each commit, cross-referenced the changes with `man select` and `man errno`, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
dergoegge:
Code review ACK b527b54950
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
04526787b5 Validate `port` options (amadeuszpawlik)
f8387c4234 Validate port value in `SplitHostPort` (amadeuszpawlik)
Pull request description:
Validate `port`-options, so that invalid values are rejected early in the startup.
Ports are `uint16_t`s, which effectively limits a port's value to <=65535. As discussed in https://github.com/bitcoin/bitcoin/pull/24116 and https://github.com/bitcoin/bitcoin/pull/24344, port "0" is considered invalid too.
Proposed in https://github.com/bitcoin/bitcoin/issues/21893#issuecomment-835784223
The `SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)` now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc,
ACKs for top commit:
luke-jr:
utACK 04526787b5
ryanofsky:
Code review ACK 04526787b5. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.
Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d
fabf1cdb20 Use steady clock for bench logging (MacroFake)
faed342a23 scripted-diff: Rename time symbols (MacroFake)
Pull request description:
Instead of using `0.001` and similar constants to "convert" an int64_t to milliseconds, use the type-safe `Ticks<>` helper. Also, use steady clock instead of system clock, since the durations are used for benchmarking.
ACKs for top commit:
fanquake:
ACK fabf1cdb20 - validation bench output still looks sane.
Tree-SHA512: e6525b5fdad6045ca500c56014897d7428ad288aaf375933d3b5939feddf257f6910d562eb66ebcde9186bef9a604ee8d763a318253838318d59df2a285be7c2
43b8777dc3 refactor: move run_command from util to common (Cory Fields)
192325a77d kernel: move RunCommandParseJSON to its own file (Cory Fields)
Pull request description:
Because libbitcoinkernel does not include this new object, this has the side-effect of eliminating its unnecessary `boost::process` dependency.
This leaves libbitcoinkernel with 3 remaining boost dependencies:
- `boost::date_time` for `util/time.cpp`, which I'll separate out next. Exactly like this PR.
- `boost::signals2` for which I have a POC re-implementation here: https://github.com/theuni/bitcoin/commits/replace-boost-signals
- `boost::multi_index` which I'm not sure about yet.
ACKs for top commit:
ryanofsky:
Code review ACK 43b8777dc3. Could consider squashing the two commits, so the code just moves once instead of twice.
fanquake:
ACK 43b8777dc3
Tree-SHA512: f2a46cac34aaadfb8a1442316152ad354f6990021b82c78d80cae9fd43cd026209ffd62132eaa99d5d0f8cf34e996b6737d318a9d9a3f1d2ff8d17d697abf26d
Help from `bitcoind -h` states that conf can only be used from the commandline.
However, if conf is set in a bitcoin.conf file, it is ignored but there is no error.
Show an error to user if conf is set in a .conf file and prompt them to use
`includeconf` if they wish to specify additional config files.
Adds `IsConfSupported` function to allow for easily adding conf options
to disallow or throw warnings for.
Check `port` options for invalid values (ports are parsed as uint16, so
in practice values >65535 are invalid; port 0 is undefined and therefore
considered invalid too). This allows for an early rejection of faulty
values and an supplying an informative message to the user.
Splits tests in `feature_proxy.py` to cover both invalid `hostname`
and `port` values.
Adds a release-note as previously valid `-port` and `-rpcport` values
can now result in errors.
Forward the validation of the port from `ParseUInt16(...)`.
Consider port 0 as invalid.
Add suitable test for the `SplitHostPort` function.
Add doxygen description to the `SplitHostPort` function.
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."
Since it is now a string_view instead of a const char*, update the
name to reflect that the variable is no longer a "Pointer to
String, Zero-terminated" (psz).
-BEGIN VERIFY SCRIPT-
sed -i s/pszThread/thread_name/ $(git grep -l pszThread src)
-END VERIFY SCRIPT-
fa875349e2 Fix iwyu (MacroFake)
faad673716 Fix issues when calling std::move(const&) (MacroFake)
Pull request description:
Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways:
* Remove the `const`, or
* Remove the `std::move`
ACKs for top commit:
ryanofsky:
Code review ACK fa875349e2. Looks good. Good for univalue to support c++11 move optimizations
Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
3add234546 ui: show header pre-synchronization progress (Pieter Wuille)
738421c50f Emit NotifyHeaderTip signals for pre-synchronization progress (Pieter Wuille)
376086fc5a Make validation interface capable of signalling header presync (Pieter Wuille)
93eae27031 Test large reorgs with headerssync logic (Suhas Daftuar)
355547334f Track headers presync progress and log it (Pieter Wuille)
03712dddfb Expose HeadersSyncState::m_current_height in getpeerinfo() (Suhas Daftuar)
150a5486db Test headers sync using minchainwork threshold (Suhas Daftuar)
0b6aa826b5 Add unit test for HeadersSyncState (Suhas Daftuar)
83c6a0c524 Reduce spurious messages during headers sync (Suhas Daftuar)
ed6cddd98e Require callers of AcceptBlockHeader() to perform anti-dos checks (Suhas Daftuar)
551a8d957c Utilize anti-DoS headers download strategy (Suhas Daftuar)
ed470940cd Add functions to construct locators without CChain (Pieter Wuille)
84852bb6bb Add bitdeque, an std::deque<bool> analogue that does bit packing. (Pieter Wuille)
1d4cfa4272 Add function to validate difficulty changes (Suhas Daftuar)
Pull request description:
New nodes starting up for the first time lack protection against DoS from low-difficulty headers. While checkpoints serve as our protection against headers that fork from the main chain below the known checkpointed values, this protection only applies to nodes that have been able to download the honest chain to the checkpointed heights.
We can protect all nodes from DoS from low-difficulty headers by adopting a different strategy: before we commit to storing a header in permanent storage, first verify that the header is part of a chain that has sufficiently high work (either `nMinimumChainWork`, or something comparable to our tip). This means that we will download headers from a given peer twice: once to verify the work on the chain, and a second time when permanently storing the headers.
The p2p protocol doesn't provide an easy way for us to ensure that we receive the same headers during the second download of peer's headers chain. To ensure that a peer doesn't (say) give us the main chain in phase 1 to trick us into permanently storing an alternate, low-work chain in phase 2, we store commitments to the headers during our first download, which we validate in the second download.
Some parameters must be chosen for commitment size/frequency in phase 1, and validation of commitments in phase 2. In this PR, those parameters are chosen to both (a) minimize the per-peer memory usage that an attacker could utilize, and (b) bound the expected amount of permanent memory that an attacker could get us to use to be well-below the memory growth that we'd get from the honest chain (where we expect 1 new block header every 10 minutes).
After this PR, we should be able to remove checkpoints from our code, which is a nice philosophical change for us to make as well, as there has been confusion over the years about the role checkpoints play in Bitcoin's consensus algorithm.
Thanks to Pieter Wuille for collaborating on this design.
ACKs for top commit:
Sjors:
re-tACK 3add234546
mzumsande:
re-ACK 3add234546
sipa:
re-ACK 3add234546
glozow:
ACK 3add234546
Tree-SHA512: e7789d65f62f72141b8899eb4a2fb3d0621278394d2d7adaa004675250118f89a4e4cb42777fe56649d744ec445ad95141e10f6def65f0a58b7b35b2e654a875
