The committed state of an index should never
be ahead of the flushed chainstate. Otherwise, in the case
of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state would not be
available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next
ChainStateFlushed notification.
Github-Pull: #33212
Rebased-From: 01b95ac6f4
Let's say an attacker wants to use/exhaust the network's bandwidth, and
has the choice between renting resources from a commercial provider and
getting the network to "spam" itself it by sending unconfirmed
transactions. We'd like the latter to be more expensive than the former.
The bandwidth for relaying a transaction across the network is roughly
its serialized size (plus relay overhead) x number of nodes. A 1000vB
transaction is 1000-4000B serialized. With 100k nodes, that's 0.1-0.4GB
If the going rate for commercial services is 10c/GB, that's like 1-4c per kvB
of transaction data, so a 1000vB transaction should pay at least $0.04.
At a price of 120k USD/BTC, 100sat is about $0.12. This price allows us
to tolerate a large decrease in the conversion rate or increase in the
number of nodes.
Github-Pull: #33106
Rebased-From: 6da5de58ca
Use a virtual size of 1000 to keep precision when using a feerate
(which is rounded to the nearest satoshi per kvb) that isn't just an
integer.
Github-Pull: #33106
Rebased-From: 457cfb61b5
Back when we implemented coin age priority as a miner policy, miners
mempools might admit transactions paying very low fees, but then want to
set a higher fee for block inclusion. However, since coin age priority
was removed in v0.15, the block assembly policy is solely based on fees,
so we do not need to apply minimum feerate rules in multiple places. In
fact, the block assembly policy ignoring transactions that are added to
the mempool is likely undesirable as we waste resources accepting and
storing this transaction.
Instead, rely on mempool policy to enforce a minimum entry feerate to
the mempool (minrelaytxfee). Set the minimum block feerate to the
minimum non-zero amount (1sat/kvB) so it collects everything it finds in
mempool into the block.
Github-Pull: #33106
Rebased-From: 5f2df0ef78
Change time_window from 20s to 1h so Reset is not accidentally called
if the test takes a while.
Change num_lines from 1024 to 10 since LogRateLimiter is parameterized
and does not require logging 1MiB of data.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Github-Pull: #33211
Rebased-From: 5dda364c4b
Use -nologratelimit by default in functional tests if the bitcoind
version supports it.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Github-Pull: #33011
Rebased-From: 5c74a0b397
- Add helper functions and structs to improve readability and
reusability of test code
- Make tests more specific by comparing all produced log lines with
expected log lines instead of relying on approximations or proxies.
Github-Pull: #33011
Rebased-From: 9f3b017bcc
This allows us to safely and explicitly manage the dual dependency
on the limiter: one for the Logger, and one for the CScheduler.
Github-Pull: #33011
Rebased-From: 3d630c2544
In LogPrintStr_:
- remove an unnecessary BCLog since we are in the BCLog namespace.
- remove an unnecessary \n when rate limiting is triggered since
FormatLogStrInPlace will add it.
- move the ratelimit bool into an else if block.
- prefix all log lines with [*] when suppressions exist. Previously this
was only done if should_ratelimit was true.
In Reset:
- remove an unnecessary \n since FormatLogStrInPlace will add it.
- Change Level::Info to Level::Warning.
Github-Pull: #33011
Rebased-From: e8f9c37a3b
Clean up the noisy LogLimitStats and remove references to the time
window.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Github-Pull: #33011
Rebased-From: 3c7cae49b6
To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
friends, we rate-limit them by passing a should_ratelimit bool that
eventually makes its way to LogPrintStr which may call
LogRateLimiter::Consume. The rate limiting is accomplished by
adding a LogRateLimiter member to BCLog::Logger which tracks source
code locations for the given logging window.
Every hour, a source location can log up to 1MiB of data. Source
locations that exceed the limit will have their logs suppressed for the
rest of the window determined by m_limiter.
This change affects the public LogPrintLevel function if called with
a level >= BCLog::Level::Info.
The UpdateTipLog function has been changed to use the private LogPrintLevel_
macro with should_ratelimit set to false. This allows UpdateTipLog to log
during IBD without hitting the rate limit.
Note that on restart, a source location that was rate limited before the
restart will be able to log until it hits the rate limit again.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Github-Pull: #32604
Rebased-From: d541409a64
The std::source_location conveniently stores the file name, line number,
and function name of a source code location. We switch to using it instead
of the __func__ identifier and the __FILE__ and __LINE__ macros.
BufferedLog is changed to have a std::source_location member, replacing the
source_file, source_line, and logging_function members. As a result,
MemUsage no longer explicitly counts source_file or logging_function as the
std::source_location memory usage is included in the MallocUsage call.
This also changes the behavior of -logsourcelocations as std::source_location
includes the entire function signature. Because of this, the functional test
feature_config_args.py must be changed to no longer include the function
signature as the function signature can differ across platforms.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Github-Pull: #32604
Rebased-From: a6a35cc0c2
LogRateLimiter will be used to keep track of source locations and our
current time-based logging window. It contains an unordered_map and a
m_suppressions_active bool to track source locations. The map is keyed
by std::source_location, so a custom Hash function (SourceLocationHasher)
and custom KeyEqual function (SourceLocationEqual) is provided.
SourceLocationHasher uses CSipHasher(0,0) under the hood to get a
uniform distribution.
A public Reset method is provided so that a scheduler (e.g. the
"b-scheduler" thread) can periodically reset LogRateLimiter's state when
the time window has elapsed.
The LogRateLimiter::Consume method checks if we have enough available
bytes in our rate limiting budget to log an additional string. It
returns a Status enum that denotes the rate limiting status and can
be used by the caller to emit a warning, skip logging, etc.
The Status enum has three states:
- UNSUPPRESSED (logging was successful)
- NEWLY_SUPPRESSED (logging was succcesful, next log will be suppressed)
- STILL_SUPPRESSED (logging was unsuccessful)
LogLimitStats counts the available bytes left for logging per source
location for the current logging window. It does not track actual source
locations; it is used as a value in m_source_locations.
Also exposes a SuppressionsActive() method so the logger can use
that in a later commit to prefix [*] to logs whenenever suppressions
are active.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Github-Pull: #32604
Rebased-From: afb9e39ec5
We mark ~DebugLogHelper as noexcept(false) to be able to catch the
exception it throws. This lets us use it in test in combination with
BOOST_CHECK_THROW and BOOST_CHECK_NO_THROW to check that certain log
messages are (not) logged.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Github-Pull: #32604
Rebased-From: df7972a6cf
The getpeerinfo docs incorrectly specified the ping durations as
milliseconds. This was incorrectly changed in a3789c700b
(released in v25; master since Sept. 2022). The correct duration unit
is seconds.
Also, remove the documentation of the getpeerinfo RPC response from the
ping RPC since it's incomplete. Better to just reference the getpeerinfo
RPC and it's documenation for this.
Github-Pull: #33133
Rebased-From: 1252eeb997
The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should prevent the ability for someone to
broadcast a transaction through the p2p network that is not valid according to the new rules. This
is because if it was possible it would be a trivial DoS to potentially unupgraded miners after the
soft fork activates.
We do not know for sure whether users will activate the Consensus Cleanup. However if they do such
transactions must have been made non-standard long in advance, due to the time it takes for most
nodes on the network to upgrade. In addition this limit may only be run into by pathological
transactions which pad the Script with sigops but do not use actual signatures when spending, as
otherwise they would run into the standard transaction size limit.
Github-Pull: bitcoin/bitcoin#32521
Rebased-From: 5863315e33
32-bit architecture is limited to 4GiB, so it doesn't make sense to set a too
high value. Since this setting is performance critical, pick an arbitrary value
higher than for -maxmempool but still reasonable.
Github-Pull: #32530
Rebased-From: 9f8e7b0b3b
32-bit architecture is limited to 4GiB, so it doesn't make sense to set a too high value. 500 MB is
chosen as an arbitrary maximum value that seems reasonable.
Github-Pull: #32530
Rebased-From: 2c43b6adeb
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because 'disconnectBlock' provides 'TxStateInactive' without the "abandoned"
flag for coinbase transactions to 'SyncTransaction', while 'AddToWallet()' internally
modifies it to retain the abandoned state.
The flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
'AddToWallet' internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to 'SyncTransaction()', the state equality assertion fails and crashes the wallet.
Github-Pull: #31757
Rebased-From: 9ef429b6ae
This RPC lists all the descriptors present in the wallet, not only
the ones that were imported, but also the ones generated when a
new wallet is created.
It can be verified by creating a new wallet and calling the
`listdescriptors` RPC, which will contain 8 ranged descriptors that
are created for every new wallet.
Github-Pull: #32708
Rebased-From: b44514b876
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.
This PR adds a concise top-level description, improving documentation consistency and alerting users to the potential impacts of using the command.
Github-Pull: #32333
Rebased-From: 135a0f0aa7
Logging the wallet version before anything has been read from disk results
in the wrong version being logged.
Also split the last client version logging as it may not always be
present to be logged.
Github-Pull: #32553
Rebased-From: 359ecd3704
This commit fixes a couple command paths for interacting with the
test_bitcoin binary within the Unit Test documentation.
Github-Pull: #32389
Rebased-From: 6cbc28b8dd
The crash stems from the order of the shutdown procedure:
We first unset the client model, then destroy the wallet controller—but we leave
the internal wallet models ('m_wallets') untouched for a brief period. As a result,
there’s a point in time where views still have connected signals and access to
wallet models that are not connected to any wallet controller.
Now.. since the clientModel is only replaced with nullptr locally and not destroyed
yet, signals like numBlocksChanged can still emit. Thus, when wallet views receive
them, they see a non-null wallet model ptr, and proceed to call backend functions
from a model that is being torn down.
As the shutdown procedure begins by unsetting clientModel from all views. It’s safe
to ignore events when clientModel is nullptr.
Github-Pull: gui#864
Rebased-From: 71656bdfaa
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced an unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
Tried:
```
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
> grep -rnE --include='*.cpp' --include='*.h' '\b(\w+)\(([^)]*\b(\w+)\b[^)]*)\)\s*==\s*\3\.' .
```
./src/test/arith_uint256_tests.cpp:373: BOOST_CHECK(R1L.GetHex() == R1L.ToString());
./src/test/arith_uint256_tests.cpp:374: BOOST_CHECK(R2L.GetHex() == R2L.ToString());
./src/test/arith_uint256_tests.cpp:375: BOOST_CHECK(OneL.GetHex() == OneL.ToString());
./src/test/arith_uint256_tests.cpp:376: BOOST_CHECK(MaxL.GetHex() == MaxL.ToString());
./src/test/fuzz/cluster_linearize.cpp:565: assert(depgraph.FeeRate(best_anc.transactions) == best_anc.feerate);
./src/test/fuzz/cluster_linearize.cpp:646: assert(depgraph.FeeRate(found.transactions) == found.feerate);
./src/test/fuzz/cluster_linearize.cpp:765: assert(depgraph.FeeRate(chunk_info.transactions) == chunk_info.feerate);
./src/test/fuzz/base_encode_decode.cpp:95: assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
./src/test/fuzz/key.cpp:102: assert(pubkey.data() == pubkey.begin());
./src/test/skiplist_tests.cpp:42: BOOST_CHECK(vIndex[from].GetAncestor(0) == vIndex.data());
./src/script/signingprovider.cpp:535: ComputeTapbranchHash(node.sub[1]->hash, node.sub[1]->hash) == node.hash) {
./src/pubkey.h:78: return vch.size() > 0 && GetLen(vch[0]) == vch.size();
./src/cluster_linearize.h:881: Assume(elem.inc.feerate.IsEmpty() == elem.pot_feerate.IsEmpty());
```
Hodlinator deduced the UB on Windows in https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751723855
Github-Pull: #32141
Rebased-From: b1de59e896
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
In Base58 fuzz the two roundtrips are merged now, the new `decode_input` switches between a completely random input and a valid encoded one, to make sure the decoding passes more often.
The `max_ret_len` can also exceed the original length now and is being validated more thoroughly.
Github-Pull: #31917
Rebased-From: d5537c18a9
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
They seem to cause timeouts:
> Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode
The `encoded_string.empty()` check was corrected here to `decoded.empty()` to make sure the `(0, decoded.size() - 1)` range is always valid.
Github-Pull: #31917
Rebased-From: bad1433ef2
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
