fadf8b8182 refactor: Add and use PRUNE_TARGET_MANUAL constexpr (MarcoFalke)
fa9bd7be47 Move ::fImporting to BlockManager (MarcoFalke)
fa442b1377 Pass fImporting to ImportingNow helper class (MarcoFalke)
fa177d7b6b Move ::fPruneMode into BlockManager (MarcoFalke)
fa721f1cab Move ::nPruneTarget into BlockManager (MarcoFalke)
Pull request description:
It seems preferable to assign globals to a class (in this case `BlockManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
TheCharlatan:
Code review ACK fadf8b8182
achow101:
ACK fadf8b8182
dergoegge:
Code review ACK fadf8b8182
Tree-SHA512: d261b69257560c9f460bbe85944ca478d0390b498a5af514bafcb4f6444841e5ea58c2e8982f38c48685d6f649039234aec853a934e24ebf23e20d975991a5dc
The chainstatemanager m_options.chainparams member variable gets its
value from the global chainparams in init.cpp. This allows
validation.cpp to only include the the kernel chainparams file.
Moves chainparams code not using the ArgsManager to the kernel.
Subsequently use the kernel chainparams header now where possible in
order to further decouple chainparams call sites from gArgs.
This normalizes the behavior of initializing Main/Test/Sig/Reg
chainparams with RegTest/SigNet chainparams. These factory functions can
also easily be used from a context without an instantiated ArgsManager,
e.g. from libbitcoin kernel code, unlike the existing CreateChainParams
method.
RegTest chain params can now be initialized by configuring a
RegTestOptions struct, or with ArgsManager. This offers an interface for
creating RegTestChainParams without a gArgs object.
SigNet chain params can now be initialized by configuring a
SigNetOptions struct, or with ArgsManager. This offers an interface for
creating SigNetChainParams without a gArgs object.
Previously, we would make connections to peer from the netgroups to which
our MANUAL outbound connections belong.
However, they should be seen as regular connections from Addrman when it comes to netgroup diversity check, since the same rationale can be applied.
Note, this has nothing to do with how we connect to MANUAL connections:
we connect to them unconditionally.
127c637cf0 guix: pass --enable-initfini-array to release GCC (fanquake)
Pull request description:
This returns us to pre-Guix behaviour, where the compilers we were using to build releases, were configured with this option.
> [--enable-initfini-array](https://gcc.gnu.org/install/configure.html)
> Force the use of sections .init_array and .fini_array (instead of .init and .fini) for constructors and destructors. Option --disable-initfini-array has the opposite effect. If neither option is specified, the configure script will try to guess whether the .init_array and .fini_array sections are supported and, if they are, use them.
ACKs for top commit:
TheCharlatan:
ACK 127c637cf0
vincenzopalazzo:
utACK 127c637cf0
Tree-SHA512: fa61227054d52d4dfb4524af3888203a501f680661bdef00bb0970d4e8f7c96cf7f592686c4795be5a0debca267b8e564a4960859297c31f6b261c0729238382
05eeba2c5f [test] Add manual prune startup test case (dergoegge)
4517419628 [util] Avoid integer overflow in CheckDiskSpace (dergoegge)
Pull request description:
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` ([here](f7bdcfc83f/src/init.cpp (L1633-L1648))) because `nPruneTarget` is to the max `uint64_t` value.
```
node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
#0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
#1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
#2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43
#3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13
#4 0x7fcb7cbffd8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in
```
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file.
ACKs for top commit:
MarcoFalke:
ACK 05eeba2c5f🥝
john-moffett:
ACK 05eeba2c5f
Tree-SHA512: 1d8e6bcb49818139f04b5ab2cbef7f9b422bf0c38a804cd532b6bd0ba4c4fd07f959ba977e59896343f213086c8ecc48180f50d006638dc84649c66ec379d58a
error is a low-level function with a sole dependency on LogPrintf, which
is defined in logging.h
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.
This is a minimal extraction of a single function, but also the only use
of std::exception in util/system.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.
fa27cf4cc7 test: Default timeout factor to 4 under --valgrind (MarcoFalke)
Pull request description:
valgrind will incur a slowdown of at least 2, so increase the default timeout factor.
This should reduce the number of reported issues. See also https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455762739
ACKs for top commit:
fanquake:
ACK fa27cf4cc7 - I still see at least one actual issue when running the functional tests under `--valgrind` (outside the CI system), but will follow up separately with that. Increasing the timeout here seems fine.
Tree-SHA512: 4467559a3bfd98f5735f300f6ed54c68f951191d65a2b1294d71d72cc5d0864964de562d5dfa0a4855fc541ccb269a538b7aeb3d408d2d012a5369513397c395
763079a3f1 Squashed 'src/secp256k1/' changes from 21ffe4b22a9..bdf39000b9c (Pieter Wuille)
Pull request description:
This updates the libsecp256k1 subtree to [v0.3.0](https://github.com/bitcoin-core/secp256k1/releases/tag/v0.3.0). I don't believe there are code changes that are particularly important to Bitcoin Core, apart from the added CMake build system support.
ACKs for top commit:
jonasnick:
ACK e5c7fcb361
fanquake:
ACK e5c7fcb361
Tree-SHA512: eda42e44d6d4ae43e9fab8a15854e41c8d9e14b645945039dbc35402bec501d73caa5d293264bd03ec6a7fe4919b9a725560f1831a58a6364dc6edaf259145a0
fa3e9b420f refactor: Consistently use args over gArgs in init.cpp (MarcoFalke)
fa891120c8 refactor: Consistently use context args over gArgs in node/interfaces (MarcoFalke)
Pull request description:
Currently `node/interfaces.cpp` uses a mix of `gArgs` vs `m_context->args`. This is fine, because outside of tests those should be identical. However, it makes the code inconsistent and harder to use in tests.
Fix that by using `args` from the context consistently. Do the same in `init.cpp`, where `gArgs` and `args` are inconsistently used in the same scope or even line.
ACKs for top commit:
ryanofsky:
Code review ACK fa3e9b420f
Tree-SHA512: bcd52f176794ebb1ecb9e1922411f7b84d212ae13bad314a1961b85f3077f645fca71fb0124c0889ebfdd8b59a0903b99b9985b1a4fb8f152aa6d7f0126fe5c7
8fcbdadfad util: fix argsman dupe key error (willcl-ark)
Pull request description:
fixes#22638
Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when `settings.json` contains duplicate keys. This change has no effect on `bitcoind` because it treats a corrupt `settings.json` file as a hard error and doesn't attempt to modify it.
If we find a duplicate key and error, clear `values` before returning so that `WriteSettings()` will write an empty file, therefore clearing it.
This aligns with GUI behaviour added in 1ee6d0b.
The test added only checks that `values` is empty after a duplicate key is detected. This paves the way for the `abort` option in the GUI to properly clear `settings.json`, if the user selects the option, but the test does not currently check this entire mechanism (e.g. the file contents).
ACKs for top commit:
TheCharlatan:
Code review ACK 8fcbdadfad
ryanofsky:
Code review ACK 8fcbdadfad. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR:
Tree-SHA512: a5fd49b30ede0a24188623192825bccb952e427cc35f96ff9bfdc737361dcc35ac6480589ddf7f0ddeaebd34361bdaee31e7a91f2c0d857e4ff682614bb6bc04
da347de530 doc: update broken links (pablomartin4btc)
Pull request description:
References to `utilstrencodings` and `lint-locale-dependence.sh` where incorrect, updating them accordingly.
Also, adding another reference to util function [`LocaleIndependentAtoi`](https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L108-L118), which is related with the updated section of the guide:
```
// LocaleIndependentAtoi is provided for backwards compatibility reasons.
//
// New code should use ToIntegral or the ParseInt* functions
// which provide parse error feedback.
//
// The goal of LocaleIndependentAtoi is to replicate the defined behaviour of
// std::atoi as it behaves under the "C" locale, and remove some undefined
// behavior. If the parsed value is bigger than the integer type's maximum
// value, or smaller than the integer type's minimum value, std::atoi has
// undefined behavior, while this function returns the maximum or minimum
// values, respectively.
```
ACKs for top commit:
MarcoFalke:
lgtm ACK da347de530
Tree-SHA512: c8f4cd9cff1fb3ea367ac9dbe5aa45dc187fc60114f2e2106e02e0e17fea4ee34d6e0c408fe920c2d8765e06b4dc30c231f0454fa35469c4399e0cadbcd341ba
If `maxconnections=0`, it means our possible connections are
going to be manual (e.g via `addnode`). For this reason, we
can skip DNS seeds and set `listen` false.
54c4d03578 doc: Show how less noisy clang-tidy output can be achieved (TheCharlatan)
Pull request description:
Adds a paragraph to the clang-tidy section explaining how to de-noise its output. By default clang-tidy will print errors arrising from included headers in leveldb and other dependencies. By passing `--enable-suppress-external-warnings` flag to configure, errors arising from external dependencies are suppressed. Additional errors arrising from internal dependencies such as leveldb are suppressed by passing the `src/.bear-tidy-config` configuration file to bear. This file includes exclusionary rules for leveldb.
ACKs for top commit:
MarcoFalke:
utACK 54c4d03578
Tree-SHA512: c3dd8fb0600157582a38365a587e02e1d249fb246d6b8b4949a800fd05d3473dee49e2a4a556c60e51d6508feff810024e55fe09f5a0875f560fde30f3b6817c
We currently return "Insufficient funds" which doesn't really
describe what went wrong; the tx creation failed because of
a long-mempool-chain, not because of a lack of funds.
Also, return early from Coin Selection if the sum of the
discarded coins decreases the available balance below the
target amount.
5c938e74cf Use string interpolation for default value of -listen (ekzyis)
Pull request description:
This is a refactoring change. So I have read the following and will try to answer why this change should be accepted
> * Refactoring changes are only accepted if they are required for a feature or
bug fix or **_otherwise improve developer experience significantly_**. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
I have noticed in https://github.com/bitcoin/bitcoin/pull/26899#discussion_r1086731856 that the helper message for `-listen` does not use string interpolation.
That confused me and I wasn't sure what the reasons for that are. So it could be argued this confusion (by possibly many people in the past and in the future) may already be enough to accept this change.
However, not accepting this means that if `DEFAULT_LISTEN` is ever changed, this helper message will still use the old value (however unlikely that may be).
Therefore, this PR makes the helper message consistent with how other helper messages are implemented (using string interpolation) which leads to less confusion and prevents possibly wrong documentation in the future.
ACKs for top commit:
stratospher:
ACK 5c938e7.
vasild:
ACK 5c938e74cf
kristapsk:
cr utACK 5c938e74cf
Tree-SHA512: 8905f548ff399967966e67b29962821c28fd2620ff788c2b2bdfd088bbca75457dc63e933ad1985f93580508a65b91930fe4b2d97a2e0a7d83a3748b9ea2c26f
89cd20cbed test: add coverage for sigop limit policy (`-bytespersigop` setting) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-bytespersigop` option, which determines how pre-taproot signature operations (OP_CHECKSIG{VERIFY}, OP_CHECKMULTIGSIG{VERIFY}) affect fee handling calculations. The setting was introduced in PR #7081 for mitigating the [sigop spam attack](https://bitcointalk.org/index.php?topic=1166928.0); the initial implementation rejected txs exceeding the limit, but was changed in #8365 later to account for higher sizes in the mempool (i.e. exceeding the sigop limit is possible, but has to be compensated by higher fees).
For each combination of `-bytespersigop` setting and sigops count, the test first creates a P2WSH spending transaction with a witness script that puts sigops in a non-executing branch (OP_FALSE OP_IF OP_CHECKMULTISIG ... OP_CHECKSIG ... OP_ENDIF). This tx is then bumped up to reach exactly the _sig-op limit equivalent vsize_ by padding its datacarrier output. Based on that, increasing the tx's vsize should still reflect a vsize increase in the mempool, while a decrease of the tx's vsize should lead to the mempool treating the tx's vsize to be the _sig-op limit equivalent vsize_, since the limit was exceeded.
I assume that this parameter is almost never set explicitly by users (also it is not relevant for taproot spends), but it doesn't hurt to have a test for it. See also https://bitcoin.stackexchange.com/a/87958 for another explanation.
ACKs for top commit:
glozow:
light review ACK 89cd20cbed
MarcoFalke:
nice ACK 89cd20cbed📁
Tree-SHA512: 06998ce93bf9d5ce6143db2996a43f13990c415f97afe684227ad469349e73952bf4f6c871c1e6349e07606f4d45db64408848873a86a89481cdca5a134e5e60
faa671591f test: Use self.wait_until over wait_until_helper (MarcoFalke)
Pull request description:
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.
ACKs for top commit:
theStack:
Code-review ACK faa671591f
Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
fe329dc936 test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr)
cd761e6b2c rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr)
Pull request description:
These are additions to `getblockfrompeer` that I already [suggested on the original PR](https://github.com/bitcoin/bitcoin/pull/20295#pullrequestreview-817157738).
The two commits do the following:
1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node.
2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely.
ACKs for top commit:
Sjors:
re-utACK fe329dc936
MarcoFalke:
review ACK fe329dc936🍉
stratospher:
ACK fe329dc.
brunoerg:
re-ACK fe329dc936
Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac
fixes#22638
If we find a duplicate key and error, clear `values` before returning so that
WriteSettings will write an empty file, therefore clearing it.
This aligns with GUI behaviour added in 1ee6d0b.
3fa1185dda github: Switch to yaml issue templates (willcl-ark)
Pull request description:
The new YAML templates provide more flexibility and can be designed to extract more information from users when submitting issues, avoiding initial back-and-forth when reports do not include enough background information to begin with.
Key differences:
* YAML format
* Allows us to require responses to certain questions
* Not currently compatible with GitLab (.md only)
This does keep the "Blank Issue" option at the bottom.
Testing this must be done with the master branch of the repo, which is slightly annoying for this repo. I have therefore pushed this to my own fork so that you can see the new templates, along with how the output is rendered in newly-created issues:
[github.com/willcl-ark/bitcoin/issues](https://github.com/willcl-ark/bitcoin/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc)
I did make some minor changes to some of the template wording, but this change could also be a good time to add/remove additional questions.
This seems like a net-positive for me, setting aside the issue that if we ever migrated away from GitHub these might have to be ported back to *.md (or something else), but that seems easy-enough that this change would be worth it.
Curious to know what others think of this, and whether they would suggest adding any other questions to any of the templates as part of this update?
ACKs for top commit:
achow101:
ACK 3fa1185dda
glozow:
ACK 3fa1185dda
Tree-SHA512: ce7990cd5f951e3839bc54022ce9f4b0ff9ffb8b19754d657d79acf9118bbdc4aba196f872cd5511b81e03993e54dfe4fcb85e89deade024e5a65a336adb638b
4be57a5df1 gui: fix comments for BanTableModel and BanTablePriv::refreshBanlist() (Vasil Dimov)
a981af4e6f gui: use the stored CSubNet entry when unbanning (Vasil Dimov)
Pull request description:
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
ACKs for top commit:
furszy:
utACK 4be57a5d
mzumsande:
Tested ACK 4be57a5df1
Tree-SHA512: b783c18c9d676aa9486cff2d27039dd5c5ef3f1cc67e5056a2be68e35930926f368f26dacdf4f3d394a1f73e3e28f42dc8a6936cd1765c6e6e60695c7b4d78af
