a189d636184b1c28fa4a325b56c1fab8f44527b1 add release note for datacarriersize default change (Greg Sanders)
a141e1bf501bb2660f3a62083a65678250085e56 Add more OP_RETURN mempool acceptance functional tests (Peter Todd)
0b4048c73385166144d0b3e76beb9a2ac4cc1eca datacarrier: deprecate startup arguments for future removal (Greg Sanders)
63091b79e70b8e230a122fa6fb3dac91c80638e7 test: remove unnecessary -datacarriersize args from tests (Greg Sanders)
9f36962b07eff2369577a17c8adeaa0433697e1c policy: uncap datacarrier by default (Greg Sanders)
Pull request description:
Retains the `-datacarrier*` args, marks them as deprecated, and does not require another startup argument for multiple OP_RETURN outputs.
If a user has set `-datacarriersize` the value is "budgeted" across all seen OP_RETURN output scriptPubKeys. In other words the total script bytes stays the same, but can be spread across any number of outputs. This is done to not introduce an additional argument to support multiple outputs.
I do not advise people use the option with custom arguments and it is marked as deprecated to not mislead as a promise to offer it forever. The argument itself can be removed in some future release to clean up the code and minimize footguns for users.
Tree-SHA512: 3da2f1ef2f50884d4da7e50df2121bf175cb826edaa14ba7c3068a6d5b2a70beb426edc55d50338ee1d9686b9f74fdf9e10d30fb26a023a718dd82fa1e77b038
(cherry picked from commit 983cf90c00)
eb64d2486f Merge bitcoin/bitcoin#32473: Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts (merge-script)
Pull request description:
Backport of https://github.com/bitcoin/bitcoin/pull/32473
Additional cache index bit added for sighash combination with `SIGHASH_RANGEPROOF`
ACKs for top commit:
delta1:
ACK eb64d2486f
Tree-SHA512: 94014e726ff16473809672f3e01a8e4d11c4abd38b3ae3ae64b8b9ae61be2e3c246157dca5a3a3afc4b85cc8ec69e23181af8f11c36e0b43cfac2dcb3e2e8d70
The LogRatelimiter class implements a fixed window rate limiter. The
rate limiter allows a fixed amount of bytes to be consumed within a
fixed time window.
[log] Introduce source location type
The SourceLocation type stores the filename and line of a source code
location.
In a later commit we use this type as the key type in an unordered map
and set to keep track of rate limters for each location.
[config] Add -ratelimitlogging config option
The -ratelimitlogging can be used to enable/disable the rate limiting to
disk. Rate limiting is enabled by default.
[log] Add two new categories for unconditional logging
We create two new categories `UNCONDITIONAL_ALWAYS` and
`UNCONDITIONAL_RATE_LIMITED` that are always enabled by default.
LogPrintf now logs using the `UNCONDITIONAL_RATE_LIMITED` category which
will start to apply rate limiting in a later commit.
For some log locations it might be safe to allow more frequent logging
without rate limiting. These locations should use the
`UNCONDITIONAL_ALWAYS` category.
[validation] Exempt UpdateTipLog from rate limiting
UpdateTipLog logs everytime a new tip is activated. This occurs at an
increased frequency during IBD and should therefore be exempt from rate
limiting.
[log] Add rate limiting to LogPrintf
To mitigate disk filling attacks caused by unsafe usages of LogPrintf,
we rate limit LogPrintf by using the fixed window rate limiter
(BCLog::LogRatelimiter) introduced in an earlier commit.
The rate limiting logic is applied per source location instead of
globally. A source location is allowed to log up to 1 MiB per hour.
Source locations that violate the limit will have their logs supressed
for up to one hour.
[test util] Mark ~DebugLogHelper as noexcept(false)
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.
[test] Check for expected log rate limiting messages
[test] Test for expected file size changes when rate limiting is enabled
[test] Check that log rate limiting is disabled for exempt source locations
[test] Check that rate limiting can be disabled
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
83950275eddacac56c58a7a3648ed435a5593328 qa: unit test sighash caching (Antoine Poinsot)
b221aa80a081579b8d3b460e3403f7ac0daa7139 qa: simple differential fuzzing for sighash with/without caching (Antoine Poinsot)
92af9f74d74e76681f7d98f293eab226972137b4 script: (optimization) introduce sighash midstate caching (Pieter Wuille)
8f3ddb0bccebc930836b4a6745a7cf29b41eb302 script: (refactor) prepare for introducing sighash midstate cache (Pieter Wuille)
9014d4016ad9351cb59b587541895e55f5d589cc tests: add sighash caching tests to feature_taproot (Pieter Wuille)
Pull request description:
This introduces a per-txin cache for sighash midstate computation to the script interpreter for legacy (bare), P2SH, P2WSH, and (as collateral effect, but not actually useful) P2WPKH. This reduces the impact of certain types of quadratic hashing attacks that use standard transactions. It is not known to improve the situation for attacks involving non-standard transaction attacks.
The cache works by remembering for each of the 6 sighash modes a `(scriptCode, midstate)` tuple, which gives a midstate `CSHA256` object right before the appending of the sighash type itself (to permit all 256, rather than just the 6 ones that match the modes). The midstate is only reused if the `scriptCode` matches. This works because - within a single input - only the sighash type and the `scriptCode` affect the actual sighash used.
The PR implements two different approaches:
* The initial commits introduce the caching effect always, for both consensus and relay relation validation. Despite being primarily intended for improving the situation for standard transactions only, I chose this approach as the code paths are already largely common between the two, and this approach I believe involves fewer code changes than a more targetted approach, and furthermore, it should not hurt (it may even help common multisig cases slightly).
* The final commit changes the behavior to only using the cache for non-consensus script validation. I'm open to feedback about whether adding this commit is worth it.
Functional tests are included that construct contrived cases with many sighash types (standard and non-standard ones) and `OP_CODESEPARATOR`s in all script types (including P2TR, which isn't modified by this PR).
ACKs for top commit:
achow101:
ACK 83950275eddacac56c58a7a3648ed435a5593328
dergoegge:
Code review ACK 83950275eddacac56c58a7a3648ed435a5593328
darosior:
re-ACK 83950275eddacac56c58a7a3648ed435a5593328
Tree-SHA512: 65ae8635429a4d563b19969bac8128038ac2cbe01d9c9946abd4cac3c0780974d1e8b9aae9bb83f414e5d247a59f4a18fef5b37d93ad59ed41b6f11c3fe05af4
27aefac42505e9c083fa131d3d7edbec7803f3c0 validation: detect witness stripping without re-running Script checks (Antoine Poinsot)
2907b58834ab011f7dd0c42d323e440abd227c25 policy: introduce a helper to detect whether a transaction spends Segwit outputs (Antoine Poinsot)
eb073209db9efdbc2c94bc1f535a27ec6b20d954 qa: test witness stripping in p2p_segwit (Antoine Poinsot)
Pull request description:
Since it was introduced in 4eb515574e (#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 for every single input 3 times.
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 and 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 PR would always consider it witness stripped.
h/t AJ: this essentially implements a variant of https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3135258539.
ACKs for top commit:
sipa:
re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
Crypt-iQ:
re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
glozow:
reACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
Tree-SHA512: 70cf76b655b52bc8fa2759133315a3f11140844b6b80d9de3c95f592050978cc01a87bd2446e3a9c25cc872efea7659d6da3337b1a709511771fece206e9f149
16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/src/net_processing.cpp (L1662-L1683)))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes#30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3a
tdb3:
ACK 16bd283b3a
glozow:
ACK 16bd283b3a
dergoegge:
ACK 16bd283b3a
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
51f7668d31 addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31
stratospher:
ACK 51f7668d31. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.
Github-Pull: #25314
Rebased-From: 99b9e5f3a9
Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.
Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
Github-Pull: bitcoin/bitcoin#24498
Rebased-From: 84b0973e35
When we have an invalid program, use a zero CMR and create a valid
control block/taptweak. Otherwise we fail to hit the Simplicity logic at
all with bad programs.
#
# You are committing on CAMUS
#
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch 2024-12--simple-fuzz
# Changes to be committed:
# modified: src/test/fuzz/simplicity_tx.cpp
#
# Untracked files:
# reduced-corpus/
# run-fuzz-merge-dir-CPzq/
# run-fuzz-merge-dir-sscf/
#
The first fuzztest takes a Simplicity program and a transaction and
directly calls the Simplicity interpreter with some context cobbled
together from the transaction. It also tries messing with the budget
and computes AMRs to check that the AMR-check works, even though on
the blockchain AMRs are never used.
It also attempts mangling programs to directly fuzz the parser, type
inference and CMR checking.
THIS test, on the other hand, takes a transaction, looks for Simplicity
programs (or witnesses which look like Simplicity programs), computes
their CMRs to produce a correct corresponding scriptPubKey, creates
scriptchecks, and executes them. This should do an end-to-end coverage
of the whole Simplicity consensus logic, including all the new branches
in interpreter.cpp.
To produce seeds for this, I have a a local fuzz target which uses
rust-simplicity and rust-elements to produce programs, deep Taproot trees,
and transactions. I run this to get high coverage, then dump the
resulting complete transactions to disk, where they can be used as
seeds for this test.
This fuzz target takes its seeds in a simple and well-defined format: a
four-byte LE budget, then a transaction, Simplicity program and witness,
each prefixed by a four-byte LE length. The fuzz target extracts any
additional randomness it needs from the txid of the first input of the
transaction, since this data is not interpreted in any other way we
therefore won't confuse the fuzzer.
The reason for this design, rather than a more typical "just query the
fuzzer when you need stuff", is to make it possible to fairly easily
generate test vectors from sources other than this fuzz test. (For
example, I have an alternate target which uses Rust code to generate
well-formed Simplicity programs, which quickly gets high coverage at the
expense of being an unmaintainable mess.)
This commit includes a .c file with a small function to comute the AMR
of a program. This is needed to pass a correct AMR to the Simplicity
interpreter, to exercise all the AMR-checking paths. In practice this is
not really necessary; Elements passes NULL to disable these AMR checks.
We cannot fuzz RBF (or do anything mempool-related, really) without
chainparams. This has been true since 2019 at least. I suspect this fuzz
test has never really been run.
The main difference is that there is now explicit simplicity deallocation functions to go with the allocation functions in the API.
There are some minor changes to error message text.
The deserialization code is now automatically generated.
The are some other minor internal changes.
Compare the expected script error, parsed from JSON, with the actual
error that VerifyScript returns. If the JSON does not include an
expected error, then skip this check.
Take the existing function to parse script errors and extend it to parse
Elements errors. SCRIPT_ERR_ERROR_COUNT is not included because it is a
pseudo error.
