| Age | Commit message (Collapse) | Author |
|---|
|
node::Warnings and remove globals
260f8da71a35232d859d7705861fc1a88bfbbe81 refactor: remove warnings globals (stickies-v)
9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f node: update uiInterface whenever warnings updated (stickies-v)
b071ad9770b7ae7fc718dcbfdc8f62dffbf6cfee introduce and use the generalized `node::Warnings` interface (stickies-v)
20e616f86444d00712ac7eb840666e2b0378af4a move-only: move warnings from common to node (stickies-v)
bed29c481aebeb2b0160450c63c03cc68fb89bc6 refactor: remove unnecessary AppendWarning helper function (stickies-v)
Pull request description:
This PR:
- moves warnings from common to the node library and into the node namespace (as suggested in https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1570069541)
- generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface.
- removes warnings.cpp from the kernel library
- removes warning globals
- adds testing for the warning logic
Behaviour change introduced:
- the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning
- the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn't be, e.g. when `node::AbortNode()` is called, or for the `LARGE_WORK_INVALID_CHAIN` warning
Some discussion points:
- ~is `const std::string& id` the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.~ _edit: updated approach to use `node::Warning` and `kernel::Warning` enums._
ACKs for top commit:
achow101:
ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
ryanofsky:
Code review ACK 260f8da71a35232d859d7705861fc1a88bfbbe81. Only change since last review was rebasing
TheCharlatan:
Re-ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588
|
|
fae3a1f0065064d80ab4c0375a9eaeb666c5dd55 log: use error level for critical log messages (MarcoFalke)
Pull request description:
This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
ACKs for top commit:
stickies-v:
re-ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55, I'm ~0 on the latest force push as `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);` so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up.
kevkevinpal:
ACK [fae3a1f](https://github.com/bitcoin/bitcoin/pull/30255/commits/fae3a1f0065064d80ab4c0375a9eaeb666c5dd55)
achow101:
ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55
Tree-SHA512: 3f99fd25d5a204d570a42d8fb2b450439aad7685692f9594cc813d97253c4df172a6ff3cf818959bfcf25dfcf8ee9a9c9ccc6028fcfcecdb47591e18c77ef246
|
|
|
|
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
|
|
5bc2077e8f592442b089affdf0b5795fbc053bb8 validation: allow to specify frequency for -checkblockindex (Martin Zumsande)
d5a631b9597e5029a5048d9b8ad84ea4536bbac0 validation: improve performance of CheckBlockIndex (Martin Zumsande)
32c80413fdb063199f3bee719c4651bd63f05fce bench: add benchmark for checkblockindex (Martin Zumsande)
Pull request description:
`CheckBlockIndex() ` are consistency checks that are currently enabled by default on regtest.
The function is rather slow, which is annoying if you
* attempt to run it on other networks, especially if not fully synced
* want to generate a long chain on regtest and see block generation slow down because you forgot to disable `-checkblockindex` or don't know it existed.
One reason why it's slow is that in order to be able to traverse the block tree depth-first from genesis, it inserts pointers to all block indices into a `std::multimap` - for which inserts and lookups become slow once there are hundred thousands of entries.
However, typically the block index is mostly chain-like with just a few forks so a multimap isn't really needed for the most part. This PR suggests to store the block indices of the chain ending in the best header in a vector instead, and store only the rest of the indices in a multimap. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed.
This adds a bit of complication to make sure each block is visited (note that there are asserts that check it), making sure that the two containers are traversed correctly, but it speeds up the function considerably:
On master, a single invocation of `CheckBlockIndex` takes ~1.4s on mainnet for me (4.9s on testnet which has >2.4 million blocks).
With this branch, the runtime goes down to ~0.27s (0.85s on testnet).This is a speedup by a factor ~5.
ACKs for top commit:
achow101:
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
furszy:
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
ryanofsky:
Code review ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8. Just added suggested assert and simplification since last review
Tree-SHA512: 6b9c3e3e5069d6152b45a09040f962380d114851ff0f9ff1771cf8cad7bb4fa0ba25cd787ceaa3dfa5241fb249748e2ee6987af0ccb24b786a5301b2836f8487
|
|
09ef322acc0a88a9e119f74923399598984c68f6 [[refactor]] Check CTxMemPool options in constructor (TheCharlatan)
Pull request description:
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
ACKs for top commit:
stickies-v:
re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
achow101:
ACK 09ef322acc0a88a9e119f74923399598984c68f6
ryanofsky:
Code review ACK 09ef322acc0a88a9e119f74923399598984c68f6. Just fuzz test error checking fix and updated comment since last review
Tree-SHA512: eb3361411c2db70be17f912e3b14d9cb9c60fb0697a1eded952c3b7e8675b7d783780d45c52e091931d1d80fe0f0280cee98dd57a3100def13af20259d9d1b9e
|
|
As per doc/developer-notes#logging, LogError should be used for
severe problems that require the node to shut down.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
|
|
This is a just a mechanical change, renaming and inverting the meaning
of the indexing variable.
"m_blockfiles_indexed" is a more straightforward name for this variable
because this variable just indicates whether or not
<datadir>/blocks/blk?????.dat files have been indexed in the
<datadir>/blocks/index LevelDB database. The name "m_reindexing" was
more confusing, it could be true even if -reindex was not specified, and
false when it was specified. Also, the previous name unnecessarily
required thinking about the whole reindexing process just to understand
simple checks in validation code about whether blocks were indexed.
The motivation for this change is to follow up on previous commits,
moving away from having multiple variables called "reindex" internally,
and instead naming variables individually after what they do and
represent.
|
|
Before this change continuing a reindex without the -reindex flag set
would leave the block and coins db intact, but discard the data of the
optional indexes. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, and potentially
leading to longer startup times.
When initially running a reindex, both the block index and any further
activated indexes are wiped. On an index's Init(), both the best block
stored by the index and the chain's tip are null. An index's m_synced
member is therefore true. This means that it will process blocks through
validation events while the reindex is running.
Currently, if the reindex is continued without the user re-specifying
the reindex flag, the block index is preserved but further index data is
wiped. This leads to the stored best block being null, but the chain tip
existing. The m_synced member will be set to false. The index will not
process blocks through the validation interface, but instead use the
background sync once the reindex is completed.
If the index is preserved (this change) after a restart its best block
may potentially match the chain tip. The m_synced member will be set to
true and the index can process validation events during the rest of the
reindex.
|
|
Drop confusing kernel options:
BlockManagerOpts::reindex
ChainstateLoadOptions::reindex
ChainstateLoadOptions::reindex_chainstate
Replacing them with more straightforward options:
ChainstateLoadOptions::wipe_block_tree_db
ChainstateLoadOptions::wipe_chainstate_db
Having two options called "reindex" which did slightly different things
was needlessly confusing (one option wiped the block tree database, and
the other caused block files to be rescanned). Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.
|
|
Reverts a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3
"kernel: De-globalize fReindex". The change leads to a GUI user being
prompted to re-index on a chainstate loading failure more than once as
well as the node actually not reindexing if the user chooses to. Fix
this by setting the reindexing option instead of the atomic, which can
be safely re-used to indicate that a reindex should be attempted.
The bug specifically is caused by the chainman, and thus the blockman
and its m_reindexing atomic being destroyed on every iteration of
the for loop.
The reindex option for ChainstateLoadOptions is currently also set in a
confusing way. By using the reindex atomic, it is not obvious in which
scenario it is true or false.
The atomic is controlled by both the user passing the -reindex option,
the user chosing to reindex if something went wrong during chainstate
loading when running the gui, and by reading the reindexing flag from
the block tree database in LoadBlockIndexDB. In practice this read is
done through the chainstate module's CompleteChainstateInitialization's
call to LoadBlockIndex. Since this is only done after the reindex option
is set already, it does not have an effect on it.
Make this clear by using the reindex option from the blockman opts which
is only controlled by the user.
|
|
This ensures that the tests run the same checks on the mempool options
that the init code also applies.
|
|
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
|
|
Move enum and message formatting functions to a common/messages header where
they should be more discoverable, and also out of the util library, so they
will not be a dependency of the kernel
The are no changes in behavior and no changes to the moved code.
|
|
fReindex is one of the last remaining globals exposed by the kernel
library, so move it into the blockstorage class to reduce the amount of
global mutable state and make the kernel library a bit less awkward to
use.
|
|
d4b17c7d46ad8e2833ade99d5b4c9741c913e84d kernel: Remove batchpriority from kernel library (TheCharlatan)
Pull request description:
The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread.
Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now freely choose their own scheduling policy.
This PR is easier reviewed with `git diff --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
maflcko:
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d 📭
ryanofsky:
Code review ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, just added suggested comment since last review
hebasto:
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK.
Tree-SHA512: cafedecd9affad58ddd7f30f68bba71291ca951bb186ff4b2da04b7f21f0b26e5e3143846d032b9e391bd5ce6c7466b97aa3758d2a85ebd7353eb8b69139641a
|
|
The current usage of ScheduleBatchPriority is not transparent. Once the
thread scheduling is changed, it remains unchanged for the remainder of
the thread's lifetime. So move the call from `ImportBlocks` to the init
code where it is clearer that its effect lasts for the entire lifetime
of the thread.
Users of the kernel library might not expect `ImportBlocks` to have an
influence on the thread it is called in. Particularly since it is only a
compile time option and cannot be controlled at runtime. With this patch
users of the kernel library can now choose their own scheduling policy.
|
|
not connect automatically
95897ff181c0757e445f0e066a2a590a0a0120d2 doc: removed help text saying that peers may not connect automatically (kevkevin)
Pull request description:
Introduced in https://github.com/bitcoin/bitcoin/pull/23542 and released in version 23.0 there has been significant time since this change (2 years).
This should be removed as it is no longer relevant
ACKs for top commit:
stickies-v:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2
tdb3:
ACK for 95897ff181c0757e445f0e066a2a590a0a0120d2.
vasild:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2
jonatack:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2
kristapsk:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2. According to https://bitnodes.io/dashboard/#user-agents stats, most nodes on the network are v23+.
Tree-SHA512: 9e35194f8a1e06f1447450af2ea30cdc43722665c2d2e4b7aa9a52afac5c1e83fed744742c836743a555cc180c90f9eebdc6637eba6190010d693eef9c5834f7
|
|
The key module's functionality is not used by the kernel library, but
currently kernel users are still required to initialize the key module's
`secp256k1_context_sign` global as part of the `kernel::Context` through
`ECC_Start`.
|
|
keep to bitcoin-config.h includes
fa09451f8e6799682d7e7c863f25334fd1c7dce3 Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba8267dea11a3eb03d5cf8b51dbb99be5d scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke)
Pull request description:
The `bitcoin-config.h` includes have issues:
* The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711
* Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853 .
ACKs for top commit:
achow101:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
TheCharlatan:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
hebasto:
re-ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535) (`timedata.cpp` removed in https://github.com/bitcoin/bitcoin/pull/29623).
Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
|
|
|
|
e504b1fa1fa4d014b329dea81bfdf1aa55db238f test: Add test case for spending bare multisig (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/29113
ACKs for top commit:
ajtowns:
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f ; LGTM and just checking the 1-of-3 case seems fine
maflcko:
utACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
achow101:
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
willcl-ark:
reACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
Tree-SHA512: 641a12599efa34e1a3eb65b125318df326628fef3e6886410ea9e63a044664fad7bcad46d1d6f41ddc59630746b9963cedb569c2682b5940b32b9225883da8f2
|
|
-BEGIN VERIFY SCRIPT-
perl -0777 -pi -e 's/#if defined\(HAVE_CONFIG_H\)\n#include <config\/bitcoin-config.h>.*\n#endif.*\n/#include <config\/bitcoin-config.h> \/\/ IWYU pragma: keep\n/g' $( git grep -l '#include <config/bitcoin-config.h>' )
-END VERIFY SCRIPT-
|
|
both are provided
82f41d76f1c6ad38290917dad5499ffbe6b3974d Added seednode prioritization message to help output (tdb3)
3120a4678ab2a71a381e847688f44068749cfa97 Gives seednode priority over dnsseed if both are provided (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #27577
If both `seednode` and `dnsseed` are provided, the node will start a race between them in order to fetch data to feed the `addrman`.
This PR gives priority to `seednode` over `dnsseed` so if some nodes are provided as seeds, they can be tried before defaulting to the `dnsseeds`
ACKs for top commit:
davidgumberg:
untested reACK https://github.com/bitcoin/bitcoin/commit/82f41d76f1c6ad38290917dad5499ffbe6b3974d
itornaza:
tested re-ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d
achow101:
ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d
cbergqvist:
ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d
Tree-SHA512: 4e39e10a7449af6cd9b8f9f6878f846b94bca11baf89ff2d4fbcd4f28293978a6ed71a3a86cea36d49eca891314c834e32af93f37a09c2cc698a878f84d31c62
|
|
c6be144c4b774a03a8bcab5a165768cf81e9b06b Remove timedata (stickies-v)
92e72b5d0d49aa395e626c238bc28aba8e4c3d44 [net processing] Move IgnoresIncomingTxs to PeerManagerInfo (dergoegge)
7d9c3ec622d73a98d07ab3cee78751718982a5bc [net processing] Introduce PeerManagerInfo (dergoegge)
ee178dfcc1175e0af8163216c9c024f4bfc97965 Add TimeOffsets helper class (stickies-v)
55361a15d1aa6984051441bce88112000688fb43 [net processing] Use std::chrono for type-safe time offsets (stickies-v)
038fd979effb54ee76ce1b7cf078e920c652326a [net processing] Move nTimeOffset to net_processing (dergoegge)
Pull request description:
[An earlier approach](https://github.com/bitcoin/bitcoin/commits/1d226ae1f984c5c808f5c24c431b959cdefa692e/) in #28956 involved simplifying and refactoring the network-adjusted time calculation logic, but this was eventually [left out](https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1904214370) of the PR to make it easier for reviewers to focus on consensus logic changes.
Since network-adjusted time is now only used for warning/informational purposes, cleaning up the logic (building on @dergoegge's approach in #28956) should be quite straightforward and uncontroversial. The main changes are:
- Previously, we would only calculate the time offset from the first 199 outbound peers that we connected to. This limitation is now removed, and we have a proper rolling calculation. I've reduced the set to 50 outbound peers, which seems plenty.
- Previously, we would automatically use the network-adjusted time if the difference was < 70 mins, and warn the user if the difference was larger than that. Since there is no longer any automated time adjustment, I've changed the warning threshold to ~~20~~ 10 minutes (which is an arbitrary number).
- Previously, a warning would only be raised once, and then never again until node restart. This behaviour is now updated to 1) warn to log for every new outbound peer for as long as we appear out of sync, 2) have the RPC warning toggled on/off whenever we go in/out of sync, and 3) have the GUI warn whenever we are out of sync (again), but limited to 1 messagebox per 60 minutes
- no more globals
- remove the `-maxtimeadjustment` startup arg
Closes #4521
ACKs for top commit:
sr-gi:
Re-ACK [c6be144](https://github.com/bitcoin/bitcoin/pull/29623/commits/c6be144c4b774a03a8bcab5a165768cf81e9b06b)
achow101:
reACK c6be144c4b774a03a8bcab5a165768cf81e9b06b
dergoegge:
utACK c6be144c4b774a03a8bcab5a165768cf81e9b06b
Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96
|
|
This help text was introduced about two years ago and now a significant
portion of users are using version 23.0 and higher
|
|
This makes it similar to -checkaddrman and -checkmempool, which
also allow to run the check occasionally instead of always / never.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
|
`util::Result` objects are aggregates that can hold multiple fields with
different information. Currently Result objects can only hold a success value
of an arbitrary type or a single bilingual_str error message. In followup PR
https://github.com/bitcoin/bitcoin/pull/25722, Result objects may be able to
hold both success and failure values of different types, plus error and warning
messages.
Having a Result::operator= assignment operator that completely erases all
existing Result information before assigning new information is potentially
dangerous in this case. For example, code that looks like it is assigning a
warning value could erase previously-assigned success or failure values.
Conversely, code that looks like it is just assigning a success or failure
value could erase previously assigned error and warning messages.
To prevent potential bugs like this, disable Result::operator= assignment
operator.
It is possible in the future we may want to re-enable operator= in limited
cases (such as when implicit conversions are not used) or add a Replace() or
Reset() method that mimicks default operator= behavior. Followup PR
https://github.com/bitcoin/bitcoin/pull/25722 also adds a Result::Update()
method providing another way to update an existing Result object.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
|
|
Co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
|
|
|
|
92f88a962908c49dde99c03a4608e63e4a6eec71 doc: fixup NAT-PMP help doc (fanquake)
02395edca9e99454388ae9b91ee174fbccc38021 init: remove redundant upnp #ifdef (fanquake)
Pull request description:
This is a very belated followup to #26896 (which removed the configure options for setting the upnp and natpmp runtime default) and corrects the `-help` docs for `-upnp` and `-natpmp`.
ACKs for top commit:
davidgumberg:
ACK https://github.com/bitcoin/bitcoin/commit/92f88a962908c49dde99c03a4608e63e4a6eec71
hernanmarino:
ACK 92f88a962908c49dde99c03a4608e63e4a6eec71
Tree-SHA512: 795dc8a8703bf322b5831d845de85f2428ee0dd45d3064b48ff47d147147381af26c0a9d00c596db12009b254763844b209989daf4e7470d20e8a1753b640966
|
|
With the introduction and usage of TimeOffsets, there is no more need
for this file. Remove it.
|
|
See https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1469388717
|
|
The extra `bilingual_str` argument of the fatal error notifications and
`node::AbortNode()` is often unused and when used usually contains the
same string as the message argument. It also seems to be confusing,
since it is not consistently used for errors requiring user action. For
example some assumeutxo fatal errors require the user to do something,
but are not translated.
So simplify the fatal error and abort node interfaces by only passing a
translated string. This slightly changes the fatal errors displayed to
the user.
Also de-duplicate the abort error log since it is repeated in noui.cpp.
|
|
operations by caching last header
99afb9d15a08d2f46739f4d2b66c63dbabd7a44e refactor: init, simplify index shutdown code (furszy)
0faafb57f8298547949cbc0044ee9e925ed887ba index: decrease ThreadSync cs_main contention (furszy)
f1469eb45469672046c5793b44863f606736c853 index: cache last block filter header (furszy)
a6756ecdb2f1ac960433412807aa377d1ee80d05 index: blockfilter, decouple header lookup into its own function (furszy)
331f044e3b49223cedd16803d123c0da9d91d6a2 index: blockfilter, decouple Write into its own function (furszy)
bcbd7eb8d40fbbd0e58c61acef087d65f2047036 bench: basic block filter index initial sync (furszy)
Pull request description:
Work decoupled from #26966 per request.
The aim is to remove an unnecessary disk read operation that currently takes place with every new arriving block (or scanned block during background sync). Instead of reading the last filter header from disk merely to access its hash for constructing the next filter, this work caches it, occupying just 32 more bytes in memory.
Also, reduces `cs_main` lock contention during the index initial sync process. And, simplifies the indexes initialization and shutdown procedure.
Testing Note:
To compare the changes, added a pretty basic benchmark in the second commit. Alternatively, could also test the changes by timing the block filter sync from scratch on any network; start the node with `-blockfilterindex` and monitor the logs until the syncing process finish.
Local Benchmark Results:
*Master (c252a0fc0f4dc7d262b971a5e7ff01508159193b):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 132,042,516.60 | 7.57 | 0.3% | 7.79 | `BlockFilterIndexSync`
*PR (43a212cfdac6c64e82b601c664443d022f191520):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 126,915,841.60 | 7.88 | 0.6% | 7.51 | `BlockFilterIndexSync`
ACKs for top commit:
Sjors:
re-ACK 99afb9d15a08d2f46739f4d2b66c63dbabd7a44e
achow101:
ACK 99afb9d15a08d2f46739f4d2b66c63dbabd7a44e
TheCharlatan:
Re-ACK 99afb9d15a08d2f46739f4d2b66c63dbabd7a44e
andrewtoth:
ACK 99afb9d15a08d2f46739f4d2b66c63dbabd7a44e
Tree-SHA512: 927daadd68f4ee1ca781a89519539b895f5185a76ebaf525fbc246ea8dcf40d44a82def00ac34b188640802844b312270067f1b33e65a2479e06be9169c616de
|
|
-onion
567cec9a05e1261e955535f734826b12341684b6 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe51928911daf484ae07deb52a7ff0bcb2526ae test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d01630b44fa71321ea7ad68d5f9fbb7aefb init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142eba77dcf1acd4984e437759f65e237a gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd1d8c1db0a9c8b663dab3e3c2f0f030 i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec09fe0b002815a7e48710e530620ae2 net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182fb5ad9bcd41540b19382376114d6ee proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc44eaf4f59912c1accfc0ce5d61933a netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548effa6cd9a4a5413b690c2fd85da4ef65 net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6fd5c74b7b9bf0ce69876430746a53b1 netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d318d06818aa75a9ebe3db864197f0bc6 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51de205cc69b1a58647c65c04fa6c6362 configure: test for unix domain sockets (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`
I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):
`tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`
`bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`
Prep work for this feature includes:
- Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
- Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)
Future work:
- Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
- Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
- Enable UNIX sockets on windows where supported
- Update Network Proxies dialog in GUI to support UNIX sockets
ACKs for top commit:
Sjors:
re-ACK 567cec9a05e1261e955535f734826b12341684b6
tdb3:
re ACK for 567cec9a05e1261e955535f734826b12341684b6.
achow101:
ACK 567cec9a05e1261e955535f734826b12341684b6
vasild:
ACK 567cec9a05e1261e955535f734826b12341684b6
Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
|
|
rest_block by reading raw block
e710cefd5701cd33d1e55034b3e37cea78582733 rest: read raw block in rest_block and deserialize for json (Andrew Toth)
95ce0783a6dab325038a64d6c529c9e7816e3072 rpc: read raw block in getblock and deserialize for verbosity > 0 (Andrew Toth)
0865ab8712429761bc69f09d93760f8c6081c99c test: check more details on zmq raw block response (Andrew Toth)
38265cc14e7d646bf27882329d374d42167eb49f zmq: read raw block with ReadRawBlockFromDisk (Andrew Toth)
da338aada7943c392013c36c542af621fbc6edd1 blockstorage: check nPos in ReadRawBlockFromDisk before seeking back (Andrew Toth)
Pull request description:
For the `getblock` endpoint with `verbosity=0`, the `rest_block` REST endpoint for `bin` and `hex`, and zmq `NotifyBlock` we don't have to deserialize the block since we're just sending the raw data. This PR uses `ReadRawBlockFromDisk` instead of `ReadBlockFromDisk` to serve these requests, and only deserializes for `verbosity > 0` and `json` REST requests. See benchmarks in https://github.com/bitcoin/bitcoin/pull/26684.
Benchmarked using ApacheBench. Requesting block 750,000 in binary 10k times on a single core (set `-rest=1` in config):
`ab -n 10000 -c 1 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"`
On master, mean time 15ms.
On this branch, mean time 1ms.
For RPC
```
echo '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 0]}' > /tmp/data.json
ab -p /tmp/data.json -n 1000 -c 1 -A user:password "http://127.0.0.1:8332/"
```
On master, mean time 32ms
On this branch, mean time 13ms
ACKs for top commit:
achow101:
re-ACK e710cefd5701cd33d1e55034b3e37cea78582733
Tree-SHA512: 4cea13c7b563b2139d041b1fdcfdb793c8cc688654ae08db07e7ee6b875c5e582b8185db3ae603abbfb06d2164724f29205774620b48c493726b991999af289e
|
|
0a533613fb44207053796fd01a9f4b523a3153d4 docs: add release notes for #27114 (brunoerg)
e6b8f19de9a6d1c477d0bbda18d17794cd81a6f4 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854cc86deb747caea5283c17cf51b6a983 test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d1749f43d7b314aa2784a06af78440170 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c5e14cbe75256eba170e0867f95f360 net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5cc9a0ab1a06a60d09f1b7e1039018e net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb62d987b3e1c3b8bfad7083f0f774b2 net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)
Pull request description:
Revives #17167. It allows whitelisting manual connections. Fixes #9923
Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
- Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
- https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
- https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
- Force-relay/mempool permissions for a node you intentionally connected to.
ACKs for top commit:
achow101:
ACK 0a533613fb44207053796fd01a9f4b523a3153d4
sr-gi:
re-ACK [0a53361](https://github.com/bitcoin/bitcoin/pull/27114/commits/0a533613fb44207053796fd01a9f4b523a3153d4)
pinheadmz:
ACK 0a533613fb44207053796fd01a9f4b523a3153d4
Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
|
|
|
|
|
|
functional tests
2cc8ca19f4185490f30a49516c890b2289fbab71 [test] Use deterministic addrman in addrman info tests (stratospher)
a8978661093500df8d8dcf2a9c90837afacd0aab [test] Restart a node with empty addrman (stratospher)
71c19915c0c716d6f8a539dd92b8ad41e8c447ee [test] Use deterministic addrman in addpeeraddress test (stratospher)
7b868e6b678502e86571976d696c0e3cb72c0884 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher)
69e091f3e1f65b12cf1804e7d39651f55a01827a [init] Create deterministic addrman in tests using -test=addrman (stratospher)
be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac [init] Remove -addrmantest command line arg (stratospher)
802e6e128bba5ffa6d4ec53ff45acccb7cb28f21 [init] Add new command line arg for use only in functional tests (stratospher)
Pull request description:
An address is placed in a `[bucket,position]` in the addrman table (new table or tried table) using the `addpeeraddress` RPC. This `[bucket,position]` is calculated using `nKey`(and other metrics) for the addrman which is chosen randomly during every run.
Supposing there are 2 addresses to be placed in an addrman table. During every test run, a different `[bucket,position]` would be calculated for each address.These calculated `[bucket,position]` could even be the same for the 2 addresses in some test runs and result in collisions in the addrman. We wouldn't be able to predict when the collisions are going to happen because we can't predict the `nKey` value which is chosen at random. This can cause flaky tests.
Because of these non deterministic collisions, we are limited in what we can do to test addrman functionality. Currently in our tests don't add a second address to prevent these collisions from happening - we only keep 1 address in the new table and 1 address in the tried table. See https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1091145647, https://github.com/bitcoin/bitcoin/pull/23084, [#22831(comment)](https://github.com/bitcoin/bitcoin/pull/22831/files#r708302639).
This PR lets us create a deterministic addrman with fixed `nKey` so that we can know the `[bucket,position]` collisions beforehand, safely add more addresses in an addrman table and write more extensive tests.
ACKs for top commit:
amitiuttarwar:
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
achow101:
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
0xB10C:
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
mzumsande:
Code Review ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189
|
|
|
|
|
|
connections
|
|
|
|
By defining a virtual interface class for the scheduler client, users of
the kernel can now define their own event consuming infrastructure,
without having to spawn threads or rely on the scheduler design.
Removing CScheduler also allows removing the thread and
exception modules from the kernel library.
|
|
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s 'CMainSignals' 'ValidationSignals'
s 'MainSignalsImpl' 'ValidationSignalsImpl'
-END VERIFY SCRIPT-
|
|
|
|
|
