13d97608297bd56ed033d0e754d2e50447b02af0 test: load wallet, coverage for crypted keys (furszy)
373c99633ec7f20557db2734c49116ee4ad15423 refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy)
ee7a984f85015b610be4929b7c35cb501c1fbf7c refactor: unify test/util/wallet.h with wallet/test/util.h (furszy)
cc5a5e81217506ec6f9fff34056290f8f40a7396 wallet: bugfix, invalid crypted key "checksum_valid" set (furszy)
Pull request description:
At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.
Note:
The first commit fixes the issue, the two commits in the middle are cleanups so `DuplicateMockDatabase`
can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading
process.
Includes test coverage for the following scenarios:
1) "All ckeys checksums valid" test:
Loads an encrypted wallet with all the crypted keys with a valid checksum and
verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.
(we force a complete ckeys re-write if we find any missing crypted key checksum
during the wallet loading process)
2) "Missing checksum in one ckey" test:
Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
triggers a complete re-write of the crypted keys.
3) "Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
4) "Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.
ACKs for top commit:
achow101:
ACK 13d97608297bd56ed033d0e754d2e50447b02af0
aureleoules:
ACK 13d97608297bd56ed033d0e754d2e50447b02af0
Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b
fa2d01470a9f1a91f35ed8013635ac47dabd868b test: Use type-safe NodeSeconds for TestMemPoolEntryHelper (MacroFake)
Pull request description:
test-only refactor to drop the deprecated `GetTime` in favour of the type-safe alternative
ACKs for top commit:
aureleoules:
ACK fa2d01470a9f1a91f35ed8013635ac47dabd868b - verified that there is no behavior change
Tree-SHA512: 5b64dae19c7bba9e8d90377c85891bc86f60ffbe67ea28d5ed3bd38f6dc30d3fbfba00bf49a16792922bddf83a52c632b6e5e5d8ffe1619fd9bf63effc60d59a
files share the same purpose, and we shouldn't have wallet code
inside the test directory.
This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
c8dc0e3eaa9e0f956c5177bcb69632beb0d51770 refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e54402ed248ecf2bdfe54e8283d20fff refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770
glozow:
utACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
3fcb545ab26be3e785b5e5654be0bdc77099d827 bench: benchmark transaction creation process (furszy)
a8a75346d7e7247596c8a580d65ceaad49c97b97 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy)
f41712a734dc119f8a5e053a9cfa1f0411b5e8f1 wallet: simplify preset inputs selection target check (furszy)
5baedc33519661af9d19efcefd23dca8998d2547 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy)
295852f61998a025b0b28a0671e6e1cf0dc08d0d wallet: encapsulate pre-selected-inputs lookup into its own function (furszy)
37e7887cb4bfd7db6eb462ed0741c45aea22a990 wallet: skip manually selected coins from 'AvailableCoins' result (furszy)
94c0766b0cd1990c1399a745c88c2ba4c685d8d1 wallet: skip available coins fetch if "other inputs" are disallowed (furszy)
Pull request description:
#### # Context (Current Flow on Master)
In the transaction creation process, in order to select which coins the new transaction will spend,
we first obtain all the available coins known by the wallet, which means walking-through the
wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.
This coins vector is then provided to the Coin Selection process, which first checks if the user
has manually selected any input (which could be internal, aka known by the wallet, or external),
and if it does, it fetches them by searching each of them inside the wallet and/or inside the
Coin Control external tx data.
Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection
process walks-through the entire available coins vector once more just to erase coins that are
in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside
the same transaction).
#### # Process Workflow Changes
Now, a new method, `FetchCoins` will be responsible for:
1) Lookup the user pre-selected-inputs (which can be internal or external).
2) And, fetch the available coins in the wallet (excluding the already fetched ones).
Which will occur prior to the Coin Selection process. Which allows us to never include the
pre-selected-inputs inside the available coins vector in the first place, as well as doing other
nice improvements (written below).
So, Coin Selection can perform its main responsibility without mixing it with having to fetch
internal/external coins nor any slow and unneeded duplicate coins verification.
#### # Summarizing the Improvements:
1) If any pre-selected-input lookup fail, the process will return the error right away.
(before, the wallet was fetching all the wallet available coins, walking through the
entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)
2) The pre-selected-inputs lookup failure causes are properly described on the return error.
(before, we were returning an "Insufficient Funds" error for everything, even if the failure
was due a not solvable external input)
3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins
vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire
available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected).
Now, the available coins vector, which is built after the pre-selected-inputs fetching,
doesn’t include the already selected inputs in the first place.
4) **Faster transaction creation** for transactions that only use manually selected inputs.
We now will return early, as soon as we finish fetching the pre-selected-inputs and
not perform the resources expensive calculation of walking-through the entire wallet
txes map to obtain the available coins (coins that we will not use).
---------------------------
Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
#### Result on this PR (tip f6d0bb2d):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction`
vs
#### Result on master (tip 4a4289e2):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction`
The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 .
ACKs for top commit:
S3RK:
Code Review ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
achow101:
ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
aureleoules:
reACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
Goal 1:
Benchmark the transaction creation process for pre-selected-inputs only.
Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
Goal 2:
Benchmark the transaction creation process for pre-selected-inputs and coin selection.
-----------------------
Benchmark Setup:
1) Generates a 5k blockchain, loading the wallet with 5k transactions with two outputs each.
2) Fetch 4 random UTXO from the wallet's available coins and pre-select them as inputs inside CoinControl.
Benchmark (Goal 1):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=false` and
the manually selected coins.
Benchmark (Goal 2):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=true` and
the manually selected coins.
aaaa7bd0ba60aa7114810d4794940882d987c0aa iwyu: Add missing includes (MacroFake)
fa9ebec096ae185576a54aa80bd2a9e57f867ed4 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f988fa7f2ace2d67b1628211b7819df Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86288fa6666307a77c106c8f069ebdbe Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099453bf0882bce4f897f77eee5836e8b Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57cdeb91c8798d5c90ba9cc18085e99fb Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db555f7488c8df96db9fa88b793f897c Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa
ryanofsky:
Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba60aa7114810d4794940882d987c0aa
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).
bf9597606166323158bbf631137b82d41f39334f doc: add note about snapshot chainstate init (James O'Beirne)
e4d799528696c5ede38c257afaffd367917e0de8 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336d93a2dc88e4a61c49941887766bd72 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c08a00f1f407f1534853a5cddbbc0a23 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8f5971eb3c7b620aa7ad9b437cc515e test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215ed900145bd770525a341ba0ed9c027 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2c16a50d854f6f81428a68aa9180509 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247dad3982d54488bcdb3960470315290 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c2b271cb4683da2fb25fd295557f731 validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d159033106cc595cfa852695610bfe419c989c add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8bc5cdf4368ad55e827a873240535b28 init: add utxo snapshot detection (James O'Beirne)
f9f1735f139b6a1f1c7fea50717ff90dc4ba2bce validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100aaaa25c7558eeed8b5c536da99885f db: add StoragePath to CDBWrapper/CCoinsViewDB (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
---
Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.
This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven't yet enabled actually loading snapshots.
Don't be scared! There are some big move-only commits in here.
Accompanying changes include:
- moving the snapshot coinsdb directory from being called `chainstate_[base blockhash]` to `chainstate_snapshot`, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See [discussion here](https://github.com/bitcoin/bitcoin/pull/24232#discussion_r832762880).
- adding a simple fix in `FlushBlockFile()` that avoids a crash when attemping to flush to disk before `LoadBlockIndexDB()` is called, which happens when calling `MaybeRebalanceCaches()` during multiple chainstate init.
- improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.
ACKs for top commit:
naumenkogs:
utACK bf9597606166323158bbf631137b82d41f39334f
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf9597606166323158bbf631137b82d41f39334f. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests
fjahr:
utACK bf9597606166323158bbf631137b82d41f39334f
aureleoules:
ACK bf9597606166323158bbf631137b82d41f39334f
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
b527b549504672704a61f70d2565b9489aaaba91 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f76826056f53d971ac734b7ed49b39848d3 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac556791b5bb8aa118d4c1fed42c3fe45550c net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0aa51ff25f97bf21ce0cbc9e6b741cbd 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 b527b549504672704a61f70d2565b9489aaaba91 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 b527b549504672704a61f70d2565b9489aaaba91
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
d575a675cc884b1bebdb6197f2ef45c51788d4a3 net_processing: add thread safety annotation for m_highest_fast_announce (Anthony Towns)
0ae7987f68211729d87c9255889f4d9d1aa6a37f net_processing: add thread safety annotations for PeerManagerImpl members accessed only via the msgproc thread (Anthony Towns)
a66a7ccb822f0f1f554d27d04735b7fb585d3b71 net_processing: add thread safety annotations for Peer members accessed only via the msgproc thread (Anthony Towns)
bf12abe4542f2cf658516ea7e7fbbff6864c2208 net: drop cs_sendProcessing (Anthony Towns)
1e78f566d575a047a6f0b762bc79601e0208d103 net: add NetEventsInterface::g_msgproc_mutex (Anthony Towns)
Pull request description:
There are many cases where we assume message processing is single-threaded in order for how we access node-related memory to be safe. Add an explicit mutex that we can use to document this, which allows the compiler to catch any cases where we try to access that memory from other threads and break that assumption.
ACKs for top commit:
MarcoFalke:
review ACK d575a675cc884b1bebdb6197f2ef45c51788d4a3 📽
dergoegge:
Code review ACK d575a675cc884b1bebdb6197f2ef45c51788d4a3
w0xlt:
ACK d575a675cc
vasild:
ACK d575a675cc884b1bebdb6197f2ef45c51788d4a3 modulo the missing runtime checks
Tree-SHA512: b886d1aa4adf318ae64e32ccaf3d508dbb79d6eed3f1fa9d8b2ed96f3c72a3d38cd0f12e05826c9832a2a1302988adfd2b43ea9691aa844f37d8f5c37ff20e05
There are many cases where we assume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.
in TestingSetup(). This is used in the following commit to test
reinitializing chainstates after snapshot validation and cleanup.
Best reviewed with `git diff --color-moved=dimmed-zebra`.
This CreateAndActivateUTXOSnapshot parameter is necessary once we
perform snapshot completion within ABC, since the existing UpdateTip
test will fail because the IBD chain that has generated the snapshot
will exceed the base of the snapshot.
Being able to test snapshots being loaded into a mostly-uninitialized
datadir allows for more realistic unittest scenarios.
00eeb31c7660e2c28f189f77a6905dee946ef408 scripted-diff: rename CChainState -> Chainstate (James O'Beirne)
Pull request description:
Alright alright alright, I know: we hate refactors. We especially hate cosmetic refactors.
Nobody knows better than I that changing broad swaths of code out from under our already-abused collaborators, only to send a cascade of rebase bankruptcies, is annoying at best and sadistic at worst. And for a rename! The indignation!
But just for a second, imagine yourself. Programming `bitcoin/bitcoin`, on a sandy beach beneath a lapis lazuli sky. You go to type the name of what is probably the most commonly used data structure in the codebase, and you *only hit shift once*.
What could you do in such a world? You could do anything. [The only limit is yourself.](https://zombo.com/)
---
So maybe you like the idea of this patch but really don't want to deal with rebasing. You're in luck!
Here're the commands that will bail you out of rebase bankruptcy:
```sh
git rebase -i $(git merge-base HEAD master) \
-x 'sed -i "s/CChainState/Chainstate/g" $(git ls-files | grep -E ".*\.(py|cpp|h)$") && git commit --amend --no-edit'
# <commit changed?>
git add -u && git rebase --continue
```
---
~~Anyway I'm not sure how serious I am about this, but I figured it was worth proposing.~~ I have decided I am very serious about this.
Maybe we can have nice things every once in a while?
ACKs for top commit:
MarcoFalke:
cr ACK 00eeb31c7660e2c28f189f77a6905dee946ef408
hebasto:
ACK 00eeb31c7660e2c28f189f77a6905dee946ef408
glozow:
ACK 00eeb31c7660e2c28f189f77a6905dee946ef408, thanks for being the one to propose this
w0xlt:
ACK 00eeb31c76
Tree-SHA512: b828a99780614a9b74f7a9c347ce0687de6f8d75232840f5ffc26e02bbb25a3b1f5f9deabbe44f82ada01459586ee8452a3ee2da05d1b3c48558c8df6f49e1b1
377e9ccda469731d535829f184b70c73ed46b6ef scripted-diff: net: rename permissionFlags to permission_flags (Anthony Towns)
0a7fc428978c4db416fdcf9bf0b79de17d0558d7 net: make CNode::m_prefer_evict const (Anthony Towns)
d394156b99d6b9a99aedee78658310d169ca188d net: make CNode::m_permissionFlags const (Anthony Towns)
9dccc3328eeaf9cd66518d812c878def5d014e36 net: add CNodeOptions for optional CNode constructor params (Anthony Towns)
Pull request description:
Adds CNodeOptions to make it easier to add optional parameters to the CNode constructor, and makes prefer_evict and m_permissionFlags actually const.
ACKs for top commit:
naumenkogs:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef
jonatack:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef per `git range-diff 52dcb1d 2f3602b 377e9cc`
vasild:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef
ryanofsky:
Code review ACK 377e9ccda469731d535829f184b70c73ed46b6ef. Looks good and feel free to ignore suggestions!
Tree-SHA512: 06fd6748770bad75ec8c966fdb73b7534c10bd61838f6f1b36b3f3d6a438e58f6a7d0edb011977e5c118ed7ea85325fac35e10dde520fef249f7a780cf500a85
958048057087e6562b474f9028316c00ec03c2e4 Update debug logging section in the developer notes (Jon Atack)
1abaa31aa3d833caf2290d6c90f57f7f79d146c0 Update -debug and -debugexclude help docs for severity level logging (Jon Atack)
45f92821621a60891044f57c7a7bc4ab4c7d8a01 Create BCLog::Level::Trace log severity level (Jon Atack)
2a8712db4fb5d06f1a525a79bb0f793cb733aaa6 Unit test coverage for -loglevel configuration option (klementtan)
eb7bee5f84d41e35cb4296e01bea2aa5ac80a856 Create -loglevel configuration option (klementtan)
98a1f9c68744074f29fa5fa67514218b5ee9edc4 Unit test coverage for log severity levels (klementtan)
9c7507bf76e79da99766a69df939520ea0a125d1 Create BCLog::Logger::LogLevelsString() helper function (klementtan)
8fe3457dbb4146952b92fb9509bbe4e97dc1f05b Update LogAcceptCategory() and unit tests with log severity levels (klementtan)
c2797cfc602c5cdd899a7c11b37bb5711cebff38 Add BCLog::Logger::SetLogLevel()/SetCategoryLogLevel() for string inputs (klementtan)
f6c0cc03509255ffa4dfd6e2822fce840dd0b181 Add BCLog::Logger::m_category_log_levels data member and getter/setter (Jon Atack)
2978b387bffc226fb1eaca4d30f24a0deedb2a36 Add BCLog::Logger::m_log_level data member and getter/setter (Jon Atack)
f1379aeca9d3a8c4d3528de4d0af8298cb42fee4 Simplify BCLog::Level enum class and LogLevelToStr() function (Jon Atack)
Pull request description:
This is an updated version of https://github.com/bitcoin/bitcoin/pull/25287 and the next steps in parent PR #25203 implementing, with Klement Tan, user-configurable, per-category severity log levels based on an idea by John Newbery and refined in GitHub discussions by Wladimir Van der Laan and Marco Falke.
- simplify the `BCLog::Level` enum class and the `LogLevelToStr()` function and add documentation
- update the logging logic to filter logs by log level both globally and per-category
- add a hidden `-loglevel` help-debug config option to allow testing setting the global or per-category severity level on startup for logging categories enabled with the `-debug` configuration option or the logging RPC (Klement Tan)
- add a `trace` log severity level selectable by the user; the plan is for the current debug messages to become trace, LogPrint ones to become debug, and LogPrintf ones to become info, warning, or error
```
$ ./src/bitcoind -help-debug | grep -A10 loglevel
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC:
info, debug, trace (default=info); warning and error levels are
always logged. If <category>:<level> is supplied, the setting
will override the global one and may be specified multiple times
to set multiple category-specific levels. <category> can be:
addrman, bench, blockstorage, cmpctblock, coindb, estimatefee,
http, i2p, ipc, leveldb, libevent, lock, mempool, mempoolrej,
net, proxy, prune, qt, rand, reindex, rpc, selectcoins, tor,
util, validation, walletdb, zmq.
```
See the individual commit messages for details.
ACKs for top commit:
jonatack:
One final push per `git range-diff a5d5569 ce3c4c9 9580480` (should be trivial to re-ACK) to ensure this pull changes no default behavior in any way for users or the tests/CI in order to be completely v24 compatible, to update the unit test setup in general, and to update the debug logging section in the developer notes.
klementtan:
reACK 9580480570
1440000bytes:
reACK 9580480570
vasild:
ACK 958048057087e6562b474f9028316c00ec03c2e4
dunxen:
reACK 9580480
brunoerg:
reACK 958048057087e6562b474f9028316c00ec03c2e4
Tree-SHA512: 476a638e0581f40b5d058a9992691722e8b546471ec85e07cbc990798d1197fbffbd02e1b3d081b4978404e07a428378cdc8e159c0004b81f58be7fb01b7cba0
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
for verbose log messages for development or debugging only, as bitcoind may run
more slowly, that are more granular/frequent than the Debug log level, i.e. for
very high-frequency, low-level messages to be logged distinctly from
higher-level, less-frequent debug logging that could still be usable in production.
An example would be to log higher-level peer events (connection, disconnection,
misbehavior, eviction) as Debug, versus Trace for low-level, high-volume p2p
messages in the BCLog::NET category. This will enable the user to log only the
former without the latter, in order to focus on high-level peer management events.
With respect to the name, "trace" is suggested as the most granular level
in resources like the following:
- https://sematext.com/blog/logging-levels
- https://howtodoinjava.com/log4j2/logging-levels
Update the test framework and add test coverage.
- add a -loglevel=<level>|<category:level> config option to allow users
to set a global -loglevel and category-specific log levels. LogPrintLevel
messages with a higher severity level than -loglevel will not be printed
in the debug log.
- for now, this config option is debug-only during the migration to
severity-based logging
- update unit and functional tests
Co-authored-by: "Jon Atack <jon@atack.com>"
a23cca56c0a7f4a267915b4beba3af3454c51603 refactor: Replace BResult with util::Result (Ryan Ofsky)
Pull request description:
Rename `BResult` class to `util::Result` and update the class interface to be more compatible with `std::optional` and with a full-featured result class implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for this change is to update existing `BResult` usages now so they don't have to change later when more features are added in https://github.com/bitcoin/bitcoin/pull/25665.
This change makes the following improvements originally implemented in https://github.com/bitcoin/bitcoin/pull/25665:
- More explicit API. Drops potentially misleading `BResult` constructor that treats any bilingual string argument as an error. Adds `util::Error` constructor so it is never ambiguous when a result is being assigned an error or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same operators and methods as `std::optional`. `BResult` had a less familiar interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so naming reflects code organization and it is obvious where the class is coming from. Drops "B" from name because it is undocumented what it stands for (bilingual?)
- Has unit tests.
ACKs for top commit:
MarcoFalke:
ACK a23cca56c0a7f4a267915b4beba3af3454c51603 🏵
jonatack:
ACK a23cca56c0a7f4a267915b4beba3af3454c51603
Tree-SHA512: 2769791e08cd62f21d850aa13fa7afce4fb6875a9cedc39ad5025150dbc611c2ecfd7b3aba8b980a79fde7fbda13babdfa37340633c69b501b6e89727bad5b31
Also:
- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
no longer allude to maxsigcachesize being split evenly between the two
validation caches.
- Fix possible integer truncation and add a comment.
[META] I've kept the integer types as int64_t in order to not introduce
unintended behaviour changes, in the next commit we will make
them size_t.
Returning the approximate total size eliminates the need for
InitS*Cache() to do nElems*sizeof(uint256). The cuckoocache has a better
idea of this information.
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
It is part of the node library. Also, it won't be moved to the kernel
lib, as it will be pruned of ArgsManager.
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/mempool_args.cpp src/node/
git mv src/mempool_args.h src/node/
# Replacements
sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args)
sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args)
sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H)
-END VERIFY SCRIPT-
facc2fa7b8a218a0df6a19772e1641ea68dda2e3 Use AutoFile where possible (MacroFake)
6666803c897e4ad27b45cb74e3a9aa74a335f1bf streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8a218a0df6a19772e1641ea68dda2e3
fanquake:
ACK facc2fa7b8a218a0df6a19772e1641ea68dda2e3
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8eea1ad5be1a4bf8da63651e197a20359b2 [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe18dc5d81ce290876353555b51125127d1 [net processing] Remove CNode::nServices (John Newbery)
7d1c0369340cb752f0d78e24f4251595534bf5e9 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d51bfb6a34f1d5efccfb3d8786a51a4534 [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb528f7d7b33e2f2e443c5610a1551c7f099b [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47d5c09b59fd3153700751c74e63edc7d7e [net processing] Add m_our_services and m_their_services to Peer (John Newbery)
Pull request description:
Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`.
This is also a prerequisite for adding `PeerManager` unit tests (See #25515).
ACKs for top commit:
MarcoFalke:
ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 🔑
jnewbery:
utACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67
mzumsande:
Code Review ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67
Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
