e11cdc9303 logging: Add log severity level to net.cpp (klementtan)
a8290649a6 logging: Add severity level to logs. (klementtan)
Pull request description:
**Overview**: This PR introduces a new macro, `LogPrintLevel`, that allows developers to add logs with the severity level. Additionally, it will also print the log category if it is specified.
Sample log:
```
2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XX.XX.XXX.XXX:YYYYY lastseen=2.7hrs
```
**Motivation**: This feature was suggested in #20576 and I believe that it will bring the following benefits:
* Allow for easier filtering of logs in `debug.log`
* Can be extended to allow users to select the minimum level of logs they would like to view (not in the scope of this PR)
**Details**:
* New log format. `... [category:level]...`. ie:
* Do not print category if `category == NONE`
* Do not print level if `level == NONE`
* If `category == NONE` and `level == NONE`, do not print any fields (current behaviour)
* Previous logging functions:
* `LogPrintf`: no changes in log as it calls `LogPrintf_` with `category = NONE` and `level = NONE`
* `LogPrint`: prints additional `[category]` field as it calls `LogPrintf_` with `category = category` and `level = NONE`
* `net.cpp`: As a proof of concept, updated logs with obvious severity (ie prefixed with `Warning/Error:..`) to use the new logging with severity.
**Testing**:
* Compiling and running `bitcoind` with this PR should instantly display logs with the category name (ie `net/tor/...`)
* Grepping for `net:debug` in `debug.log` should display the updated logs with severity level:
<details>
<summary>Code</summary>
```
$ grep "net:debug" debug.log
2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XXX:YYY lastseen=2.7hrs
2022-03-04T16:41:16Z [opencon] [net:debug] trying connection XXX:YYY lastseen=16.9hrs
2022-03-04T16:41:17Z [opencon] [net:debug] trying connection XXX:YYY lastseen=93.2hrs
2022-03-04T16:41:18Z [opencon] [net:debug] trying connection XXX:YYY lastseen=2.7hrs
```
</details>
ACKs for top commit:
laanwj:
Code review and lightly tested ACK e11cdc9303
Tree-SHA512: 89a8c86667ccc0688e5acfdbd399aac1f5bec9f978a160e40b0210b0d9b8fdc338479583fc5bd2e2bc785821363f174f578d52136d228e8f638a20abbf0a568f
664a14ba7c coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f100687566 kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f04 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c9 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6 coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b8 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993 kernel: Remove unnecessary blockfilter{index,}.cpp (Carl Dong)
Pull request description:
Part of: #24303
Depends on: #24322
The `GetUTXOStats` function has 2 codepaths:
- One which queries the `CoinStatsIndex` for the UTXO hash
- One which actually performs the hashing
For `libbitcoinkernel`, the only place where we call `GetUTXOStats` is in `PopulateAndValidateSnapshots`, which uses the `SHA256D` hash, and is therefore unable to use the `CoinStatsIndex` since that only provides `MuHash` hashes. Not that I think indices necessarily belong in `libbitcoinkernel` anyway.
This PR separates these 2 aforementioned codepaths of `GetUTXOStats`, uses the hashing codepath in `PopulateAndValidateSnapshots`, and removes the need to link in `index/coinstatsindex.cpp` and `node/coinstats.cpp`.
-----
Logistically, this PR:
- Extracts out the `index_requested` and `hash_type` members of `CoinStats`, which served as "in-params" to `GetUTXOStats` embedded within the `CoinStats` struct. This allows `CoinStats` to only consist of "out-param" members, and be returned by `GetUTXOStats` without needing to be an "in-out" param
- Introduce the purely virtual `UTXOHashers` class, with 3 implementations: `SHA256DHasher`, `MuHashHasher`, and `NullHasher`. These replace the existing template-based polymorphism.
- Split `GetUTXOStats` into:
- `CalculateUTXOStatsWithHasher(UTXOHasher&, ...)`, and
- `LookupUTXOStatsWithIndex(CoinStatsIndex&, ...)`
- Use `CalculateUTXOStatsWithHasher` directly where appropriate (`src/validation.cpp` and `src/fuzz`)
- Move `GetUTXOStats` to `rpc/blockchain`, which is the only place that depends on `GetUTXOStats`'s weird fallback behaviour
- Move `LookupUTXOStatsWithIndex` to `index/coinstatsindex`
Code organization:
- `src/`
- `kernel/` → only contains the hashing codepath
- `coinstats.cpp` → hashing codepath implementations
- `coinstats.h` → header for `kernel/coinstats.cpp`
- `index/` → only contains the index codepath
- `coinstatsindex.cpp` → index codepath implementations
- `coinstatsindex.h`
- `validation.cpp` → only uses the hashing codepath
- `rpc/blockchain.cpp` → uses both the hashing and index codepath, old `GetUTXOStats` fallback logic moved here as static
- `test/fuzz/coins_view.cpp` → only uses the hashing codepath
TODOs:
- [x] Commit messages could be fleshed out more
Would love any feedback!
ACKs for top commit:
laanwj:
Code review ACK 664a14ba7c
Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73
The indexing codepath logic in node/coinstats.cpp is simple enough to be
moved into CoinStatsIndex::LookUpStats, avoiding an additional layer of
function calls. Callers are modified accordingly.
Also, add 2 missed BOOST_CHECKs to the coinstatsindex_initial_sync unit
test.
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
Currently, CCoinsStats is a struct with both in-params and out-params
where the hash_type and index_requested members are the only in-params.
This change removes CCoinsStats' hash_type in-param member and adds it
to the relevant functions instead.
[META] In subsequent commits, all of CCoinsStats' members which serve as
in-params will be moved out so as to make CCoinsStats a pure
out-param struct.
In the GetUTXOStats fuzz case, GetUTXOStats is always called with a
CCoinsViewCache. Which is guaranteed to throw a std::logic_error when
its ::Cursor() method is called on the first line of GetUTXOStats.
In the fuzz case, we basically catch this logic error and declare
victory if we caught it.
There is no point to fuzzing this deterministic logic.
Confirmed with IWYU that the node/coinstats.h #include is no longer
necessary.
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
facd1fb911 refactor: Use Span of std::byte in CExtKey::SetSeed (MarcoFalke)
fae1006019 util: Add ParseHex<std::byte>() helper (MarcoFalke)
fabdf81983 test: Add test for embedded null in hex string (MarcoFalke)
Pull request description:
This adds the hex->`std::byte` helper after the `std::byte`->hex helper was added in commit 9394964f6b
ACKs for top commit:
pk-b2:
ACK facd1fb911
laanwj:
Code review ACK facd1fb911
Tree-SHA512: e2329fbdea2e580bd1618caab31f5d0e59c245a028e1236662858e621929818870b76ab6834f7ac6a46d7874dfec63f498380ad99da6efe4218f720a60e859be
fa305fd92c Add mockable clock type and TicksSinceEpoch helper (MarcoFalke)
Pull request description:
This will be used primarily by the addr time refactor (https://github.com/bitcoin/bitcoin/pull/24697) to make addr relay time type safe. However, it can also be used in other places, and can be reviewed independently, so I split it up.
ACKs for top commit:
jonatack:
ACK fa305fd92c per `git range-diff 7b3343f fa20781 fa305fd`
ajtowns:
ACK fa305fd92c
Tree-SHA512: da00200126833c1f55b1b1e68f596eab5c9254e82b188ad17779c08ffd685e198a7c5270791b4b69a858dc6ba4e051fe0c8b445d203d356d0c884f6365ee1cfe
fa9af21878 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af21878
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
bf6526f4a0 [test] Remove segwit argument from build_block_on_tip() (John Newbery)
c65bf50b44 Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery)
Pull request description:
This implements two of the suggestions from code reviews of PR 20799:
- Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor
- Remove segwit argument from build_block_on_tip()
ACKs for top commit:
dergoegge:
Code review ACK bf6526f4a0
naumenkogs:
ACK bf6526f4a0
Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
All uses of CBlockHeaderAndShortTxIDs in the product code are
constructed with fUseWTXID=true, so remove the parameter.
There is one use of the CBlockHeaderAndShortTxIDs constructor with
fUseWTXID=false in the unit tests. This is used to construct a
CBlockHeaderAndShortTxIDs for a block with only the coinbase
transaction, so setting fUseWTXID to true or false makes no difference.
Suggested in https://github.com/bitcoin/bitcoin/pull/20799#pullrequestreview-963480278
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
fab9e8a29c Remove unused GetTimeSeconds (MacroFake)
Pull request description:
Seems confusing to have this helper when it is possible to get the system time in a type-safe way by simply calling `std::chrono::system_clock::now` (C++11).
This patch replaces `GetTimeSeconds` and removes it:
* in `bitcoin-cli.cpp` by `system_clock`
* in `test/fuzz/fuzz.cpp` by `steady_clock`
ACKs for top commit:
laanwj:
Code review ACK fab9e8a29c
naumenkogs:
ACK fab9e8a29c
Tree-SHA512: 517e300b0baf271cfbeebd4a0838871acbea9360f9dd23572a751335c20c1ba261b1b5ee0aec8a36abd20c94fab83ce94f46042745279aca1f0ca2f885a03b6e
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
308dd2e93e Sanity assert GetAncestor() != nullptr where appropriate (Adam Jonas)
Pull request description:
Re-opening #17232. I have rebased the PR and addressed jonatack's nit suggestions.
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate.
In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time.
In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.
Co-Authored-By: Adam Jonas <jonas@chaincode.com>
Co-Authored-By: danra <danra@users.noreply.github.com>
ACKs for top commit:
jonatack:
ACK 308dd2e93e
Tree-SHA512: 5bfdaab1499607ae2c3cd3e2e9e8c37850bfd0e327e680f4e36c81f9c6d98a543af78ecfac1ab0e06325d264412615a04d52005875780c7db2a4d81bd2d2259a
fac6cfc50f refactor: Change * to & in MutableTransactionSignatureCreator (MarcoFalke)
Pull request description:
The `MutableTransactionSignatureCreator` constructor takes in a pointer to a mutable transaction. This is problematic for several reasons:
* It would be undefined behaviour to pass in a nullptr because for signature creation, the memory of the mutable transaction is accessed
* No caller currently passes in a nullptr, so passing a reference as a pointer is confusing
Fix all issues by replacing `*` with `&` in `MutableTransactionSignatureCreator`
ACKs for top commit:
theStack:
Code-review ACK fac6cfc50f
jonatack:
ACK fac6cfc50f
Tree-SHA512: d84296b030bd4fa2709e5adbfe43a5f8377d218957d844af69a819893252af671df7f00004f5ba601a0bd70f3c1c2e58c4f00e75684da663f28432bb5c89fb86
e4303c337c [unit test] prioritisation in mining (glozow)
7a8d60676b [miner] bug fix: update for parent inclusion using modified fee (glozow)
0f9a44461c MOVEONLY: group miner tests into MinerTestingSetup functions (glozow)
Pull request description:
Came up while reviewing #24364, where some of us incorrectly assumed that we use the same fee deduction in `CTxMemPoolModifiedEntry::nModFeesWithAncestors` when first constructing an entry and in `update_for_parent_inclusion`.
Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a `CTxMemPoolModifiedEntry` for it, subtracting the ancestor's modified fees from `nModFeesWithAncestors`. If another ancestor is included, we update it again, but use the ancestor's _base_ fees instead.
I can't explain why we use `GetFee` in one place and `GetModifiedFee` in the other, but I'm quite certain we should be using the same one for both.
And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the `prioritsetransaction` helpstring and implement it in `CTxMemPool::mapDeltas`, not as a quirk in the mining code?
Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit.
ACKs for top commit:
ccdle12:
tested ACK e4303c3
MarcoFalke:
ACK e4303c337c🚗
Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate.
In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time.
In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.
Co-Authored-By: Aurèle Oulès <aurele@oules.com>
Co-Authored-By: danra <danra@users.noreply.github.com>
f849e63bad fuzz: SplitString with multiple separators (Martin Leitner-Ankerl)
d1a9850102 http: replace boost::split with SplitString (Martin Leitner-Ankerl)
0d7efcdf75 core_read: Replace boost::split with SplitString (Martin Leitner-Ankerl)
b7ab9db545 Extend Split to work with multiple separators (Martin Leitner-Ankerl)
Pull request description:
As a followup of #22953, this removes the remaining occurrences of `boost::split` and replaces them with our own `SplitString`. To be able to do so, this extends the function `spanparsing::Split` to work with multiple separators. Finally this removes 3 more files from `lint-includes.py`.
ACKs for top commit:
theStack:
Code-review ACK f849e63bad
Tree-SHA512: f37d4dbe11cab2046e646045b0f018a75f978d521443a2c5001512737a1370e22b09247d5db0e5c9e4153229a4e2d66731903c1bba3713711c4cae8cedcc775d
fa4652ce59 Pass lifetimebound reference to SingleThreadedSchedulerClient (MacroFake)
Pull request description:
Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference.
All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines.
ACKs for top commit:
pk-b2:
ACK fa4652ce59
jonatack:
Code review ACK fa4652ce59 rebased to master, debug build, unit tests
vincenzopalazzo:
ACK fa4652ce59
Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
f64aa9c411 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
e5485e8e4b test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee5090 validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack)
Pull request description:
along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench).
A swap must not fail; when a class has a swap member function, it should be declared noexcept.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail
ACKs for top commit:
pk-b2:
ACK e5485e8e4b
w0xlt:
ACK e5485e8e4b
Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
778343a379 scripted-diff: Rename PeerManagerImpl members (dergoegge)
91c339243e [net processing] Move nHighestFastAnnounce into PeerManagerImpl (dergoegge)
10b83e2aa3 [net processing] Move block cache state into PeerManagerImpl (dergoegge)
a4c55a93ef [net processing] Inline and simplify UpdatePreferredDownload (dergoegge)
490c08f96a [net processing] Move nPreferredDownload into PeerManagerImpl (dergoegge)
a292df283a [net processing] Move mapNodeState into PeerManagerImpl (dergoegge)
37ecaf3e7a [net processing] Move CNodeState declaration above PeerManagerImpl (dergoegge)
Pull request description:
This PR moves the remaining net processing globals into `PeerManagerImpl`. This will make testing the peer manager in isolation easier and also acts as a code clean up.
ACKs for top commit:
jnewbery:
Code review ACK 778343a379
MarcoFalke:
ACK 778343a379 🗒
Tree-SHA512: 4f22105d1de37b94c3ef349f38784a30cf8d450d394a6a7849e5bd78940a71e3edbffa3d25e8efb35d7f698fd255f199de7bd4c33e23af5621a6e4e67ed43cb5
