Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a
stripped witness relies on running the Script checks 3 times. In the worst case, this consists in
running Script validation 3 times for every single input.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's
wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with
txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so
doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot,
P2WPKH, P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus).
For undefined program types, Script validation is always going to fail regardless of the witness (by
standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation.
However this might lead to more "false positives" (cases where we return witness stripping for an
otherwise invalid transaction) than the existing implementation. For instance a transaction with one
P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing
implementation would treat it as consensus invalid while the implementation in this commit would
always consider it witness stripped.
Github-Pull: #33105
Rebased-From: 27aefac42505e9c083fa131d3d7edbec7803f3c0
We will use this helper in later commits to detect witness stripping without having
to execute every input Script three times in a row.
Github-Pull: #33105
Rebased-From: 2907b58834ab011f7dd0c42d323e440abd227c25
A stripped witness is detected as a special case in mempool acceptance to make sure we do not add
the wtxid (which is =txid since witness is stripped) to the reject filter. This is because it may
interfere with 1p1c parent relay which currently uses orphan reconciliation (and originally it was
until wtxid-relay was widely adopted on the network.
This commit adds a test for this special case in the p2p_segwit function test, both when spending
a native segwit output and when spending a p2sh-wrapped segwit output.
Thanks to Eugene Siegel for pointing out the p2sh-wrapped detection did not have test coverage by
finding a bug in a related patch of mine.
Github-Pull: #33105
Rebased-From: eb073209db9efdbc2c94bc1f535a27ec6b20d954
084c95a18c9978c0a047ffe219a9eef8ab327ea6 doc: update manual pages for v29.1 (fanquake)
37d115c67eafec68128649651a7e921c3e2635ce build: bump version to v29.1 final (fanquake)
b0d88bcc501bf0fc06e64c2f0a295b99a1101073 doc: finalise release notes for 29.1 (fanquake)
99ab2e70e782bf5ca753ad636f69642da6054283 ci: return to using dash in CentOS job (fanquake)
6448ebb5a7c942949a70ffc4a1d2a93338fac130 doc: Remove wrong and redundant doxygen tag (MarcoFalke)
Pull request description:
Backports:
* #33236
* #33261
Since `rc2`, #33212 was also backported in #33251.
ACKs for top commit:
glozow:
ACK 084c95a18c9978c0a047ffe219a9eef8ab327ea6
willcl-ark:
ACK 084c95a18c9978c0a047ffe219a9eef8ab327ea6
Tree-SHA512: 0698e5b2d12f7328bf5af8dbbd92b0049de401c0a4af27fda2209f9aab35d827c5ac65eb9268aa1fae241e3adf0d3dd89324bb288655ead8af2b5584aae1f6d2
fcac8022d839572f5d8781096eec14ca7ea2e0dd test: index with an unclean restart after a reorg (Martin Zumsande)
16b1710d97464f134a526634a412a4b1b6cc8639 index: don't commit state in BaseIndex::Rewind (Martin Zumsande)
Pull request description:
Backports #33212 to 29.x
ACKs for top commit:
achow101:
ACK fcac8022d839572f5d8781096eec14ca7ea2e0dd
stickies-v:
ACK fcac8022d8
mzumsande:
Code Review ACK fcac8022d839572f5d8781096eec14ca7ea2e0dd
Tree-SHA512: eeb9213f03bbb1d48c3ccb12121a6e475f436895d314b5171007e7e4ee457c74b312fa7f0d1808d6221dc22b192700a93ea21c4e9e04689da7dde7e1f79e9569
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: 01b95ac6f496e24e525b2fc9d69ee8b543da65ff
0034dcfba9dc599449e7569ed1b30e58d4f4434f [doc] man pages for 29.1rc2 (glozow)
eb1574af0c6cba4918957d704c5c0f17235a10b5 [build] bump version to 29.1rc2 (glozow)
f9f1ca5445fc216c770b583c1db999aaf910f96f [doc] update release notes (glozow)
9dd7efc8c3fc30cd65ae18a8a91e292c71fe7c16 [policy] lower default minrelaytxfee and incrementalrelayfee to 100sat/kvB (glozow)
bbdab3ef7b7e8f3da4e571f35692bc3264c384db [prep/test] make wallet_fundrawtransaction's minrelaytxfee assumption explicit (glozow)
da30ca0efadd3861016f6435636d9b399da65162 [prep/util] help MockMempoolMinFee handle more precise feerates (glozow)
a0ae3fc8a764121b17e11e3a99330e73c0e44c2d [prep/test] replace magic number 1000 with respective feerate vars (glozow)
1c1970fb45896dc5fa7b16370408c34964ed4c19 [miner] lower default -blockmintxfee to 1sat/kvB (glozow)
3a7e093f948571e058db31dd971dc628d9729232 [doc] assert that default min relay feerate and incremental are the same (glozow)
567c3ee3cb937de09d3fe8b7e82e431993fac7c7 [test] explicitly check default -minrelaytxfee and -incrementalrelayfee (glozow)
6b5396c4b1b3561f79e0c4881dad5eb0ca7b44ae [test] RBF rule 4 for various incrementalrelayfee settings (glozow)
03da7aff996eaaddce04448f8461d4f3d15d2a4a [test] check bypass of minrelay for various minrelaytxfee settings (glozow)
4e3cfa660d1ab1552881f2eedb5123ab7d0fe0b0 [test] check miner doesn't select 0fee transactions (glozow)
Pull request description:
Backports #33106 and includes final changes for 29.1rc2. Based on current network conditions (in which nodes rejecting 0.1-1sat/vB are missing many transactions), it is recommended to change these policy settings.
I did not include #32750 because it causes #33177 and I don't foresee any problems; it was just a nice to have.
For reviewers: the backport is unclean but fairly straightforward. I just had to adapt a test that is no longer in master (#32973) and include `-datacarriersize` in order to pad transaction size (#32406).
ACKs for top commit:
dergoegge:
utACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f
marcofleon:
ACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f
murchandamus:
crACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f
brunoerg:
crACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f
Tree-SHA512: 1b7540ac3fec5b15cf36926dbf633054f14549d76aa445a2bf042b5667e8637db4f9c21c869af25a0c3f8c7cca6c585d17896d2f7e95a6264c1ff59817446694
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: 6da5de58cabc4133c379baa50845e30e5bc6b3e4
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: 457cfb61b5323a13218b3cfb5a6a6d8b3a7c5f7f
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: 5f2df0ef78be7b24798d0983c9b962740608f1f4
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: 5dda364c4b1965da586db7b81de8be90b6919414
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: 5c74a0b397cb3db94761bad78801eed4544155b9
- 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: 9f3b017bcc067bba1d1682a5d4e65b5450dc10c4
This ensures log tests behave consistently when other tests modify
the log category mask.
Github-Pull: #33011
Rebased-From: 350193e5e2efabb3eb66197b91869b946ec5428c
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: 3d630c2544e19480268426cda245796d4ce34ac3
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: e8f9c37a3b4c9c88baddb556c4b33a4cbba1f614
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: 3c7cae49b692bb6bf5cae5ee23479091bed0b8be
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: d541409a64c60d127ff912dad9dea949d45dbd8c
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: a6a35cc0c23d0d529bfeb2f40d83d61f15ca7b40
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: afb9e39ec5552e598a5febaa81820d5509b7c5d2
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: df7972a6cfd919b972bcbba07de85f7797898529
The getpeerinfo docs incorrectly specified the ping durations as
milliseconds. This was incorrectly changed in a3789c700b5a43efd4b366b4241ae840d63f2349
(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: 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff
