6c7788adf3 Avoid Simplicity header dependency propogation (Russell O'Connor)
Pull request description:
The problem with including <simplicity/elements/env.h> in interpreter.h is that now everyone who needs to include interpreter.h also needs access to Simplicity's header files too.
This commit breaks that dependency chain by using forward declarations. It will pay signifigant dividends when elements switches to CMake.
ACKs for top commit:
delta1:
ACK 6c7788a; built and tested locally
apoelstra:
ACK 6c7788adf3; successfully ran local tests
Tree-SHA512: 78d043a094f0a2bb51e0f103313ae568b42ec2852d8fe9a3fbbcad9c0fbf6829ef1347f83bfd361547ec165c01965e7342188a38d8faacfcf90ef70c5b7001f6
Changes tweakfedpegscript to return both p2wsh and p2shwsh version of
the tweaked address.
"address" has been removed from the result and replaced with "p2wsh" and
"p2shwsh".
In CreatePeginWitnessInner, the MerkleBlock is always serialized without
witness: PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS
In DecomposePeginWitness before this change, the MerkleBlock was
deserialized with witness: PROTOCOL_VERSION
This was only noticed as an issue in the pegin subsidy implementation,
in a failure in the feature_dynafed functional test. In the
test_transition_mempool_eject test case, the Merkle block proof is coming
from the same chain where we are creating a pegin.
See the comment: "hack: since we're not validating peg-ins in parent chain,
just make both the funding and claim tx on same chain (printing money)"
I haven't investigated enough to explain why this causes a
deserialization failure in this specific case, but presumably this change
is correct since we're always serializing without witness. Before this
DecomposePeginWitness was only used in src/psbt.cpp
The problem with including <simplicity/elements/env.h> in interpreter.h is that
now everyone who needs to include interpreter.h also needs access to
Simplicity's header files too.
This commit breaks that dependency chain by using forward declarations. It will
pay signifigant dividends when elements switches to CMake.
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
876dbdfb4702410dfd4037614dc9298a0c09c63e tests: drop expect_disconnect behaviour for tx relay (Anthony Towns)
b29ae9efdfeeff774e32ee433ce67d8ed8ecd49f validation: only check input scripts once (Anthony Towns)
266dd0e10d08c0bfde63205db15d6c210a021b90 net_processing: drop MaybePunishNodeForTx (Anthony Towns)
Pull request description:
Because we do not discourage nodes for transactions we consider non-standard, we don't get any DoS protection from this check in adversarial scenarios, so remove the check entirely both to simplify the code and reduce the risk of splitting the network due to changes in tx relay policy.
Then, because we no longer make use of the distinction between consensus and standardness failures during script validation, don't re-validate each script with only-consensus rules, reducing the cost to us of transactions that we won't relay.
ACKs for top commit:
achow101:
ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
darosior:
re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
sipa:
re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
glozow:
ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
Tree-SHA512: 8bb0395766dde54fc48f7077b80b88e35581aa6e3054d6d65735965147abefffa7348f0850bb3d46f6c2541fd384ecd40a00a57fa653adabff8a35582e2d1811
faa3e684118bffa7a98cf76eeeb59243219df900 test: Log KeyboardInterrupt as exception (MarcoFalke)
fa30b34026f76a5b8af997152fced2d281782e0d test: Do not pass tests on unhandled exceptions (MarcoFalke)
Pull request description:
Currently the functional tests are problematic, because they pass, even if they encounter an unhanded exception.
Fix this by handling all exceptions: Catch `BaseException` as fallback and mark it as failure.
Can be tested via:
```diff
diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
index da6e5d408f..ecc41fb041 100755
--- a/test/functional/wallet_disable.py
+++ b/test/functional/wallet_disable.py
@@ -19,6 +19,7 @@ class DisableWalletTest (BitcoinTestFramework):
self.wallet_names = []
def run_test (self):
+ import sys;sys.exit("fatal error")
# Make sure wallet is really disabled
assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
```
Previously, the test would pass. With this patch, it would fail.
ACKs for top commit:
enirox001:
Looks good to me—ACK faa3e68
stickies-v:
re-ACK faa3e684118bffa7a98cf76eeeb59243219df900
pablomartin4btc:
tACK faa3e684118bffa7a98cf76eeeb59243219df900
Tree-SHA512: 11ecd5201982e2c776e48d98834b17c15a415306a95524bc702daeba20a316aac797748e9592be8db575597804f149ee7ef104416037cc9e5891758625810e2d
f563ce90818d486d2a199439d2f6ba39cd106352 net: Do not apply whitelist permission to onion inbounds (Martin Zumsande)
Pull request description:
Tor inbound connections do not reveal the peer's actual network address. Do not apply whitelist permissions to them since address-based matching is ineffective.
ACKs for top commit:
darosior:
ACK f563ce90818d486d2a199439d2f6ba39cd106352
furszy:
ACK f563ce90818d486d2a199439d2f6ba39cd106352
vasild:
ACK f563ce90818d486d2a199439d2f6ba39cd106352
Tree-SHA512: 49ae70e382fc2f78b7073553fe649a6843a41214b2986ea7f77e285d02b7bd00fe0320a1b71d1aaca08713808fb14af058f0b1f19f19adb3a77b97cb9d3449ce
8b6264768030db1840041abeeaeefd6c227a2644 test: send duplicate blocktxn message in p2p_compactblocks.py (Eugene Siegel)
5e585a0fc4fd68dd7b4982054b34deae2e7aeb89 net: check for empty header before calling FillBlock (Eugene Siegel)
Pull request description:
This avoids an Assume crash if multiple blocktxn messages are received. The first call to `FillBlock` would make the header empty via `SetNull` and the call right before the second `FillBlock` would crash [here](689a321976/src/net_processing.cpp (L3333)) since `LookupBlockIndex` won't find anything. Fix that by checking for an empty header before the Assume.
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/33296/commits/8b6264768030db1840041abeeaeefd6c227a2644
fjahr:
tACK 8b6264768030db1840041abeeaeefd6c227a2644
achow101:
ACK 8b6264768030db1840041abeeaeefd6c227a2644
mzumsande:
Code Review ACK 8b6264768030db1840041abeeaeefd6c227a2644
Tree-SHA512: d43a6f652161d4f7e6137f207a3e95259fc51509279d20347b1698c91179c39c8fcb75d2668b13a6b220f478a03578573208a415804be1d8843acb057fa1a73a
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
9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6 node: cap -dbcache to 1GiB on 32-bit architectures (Antoine Poinsot)
2c43b6adebbfabb3c8dd82fe821ce0a5d6173b3b init: cap -maxmempool to 500 MB on 32-bit systems (Antoine Poinsot)
Pull request description:
32-bit architecture is limited to 4GiB of RAM, so it doesn't make sense to set a too high value. A too high value could cause an OOM unbeknownst to the user a while after startup as mempool / dbcache fills.
ACKs for top commit:
achow101:
ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
instagibbs:
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
dergoegge:
Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
glozow:
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
Tree-SHA512: cc7541b2c0040fc21a43916caec464dfb443af808f4e85deffa1187448ffff6edb0d69f9ebdb43915d145b8b4694d8465afe548f88da53ccebc9ce4b7c34b735
* test: add test for unblinded re/issuance greater than 21 million
Co-authored-by: Mihailo Milenkovic <mihailo.milenkovic84@gmail.com>
* fix: unblinded re/issuance for greater than 21 million
Co-authored-by: Mihailo Milenkovic <mihailo.milenkovic84@gmail.com>
* enable with param config
* change range check to policy with config option
* refactoring, additional error messages and test extension
---------
Co-authored-by: Mihailo Milenkovic <mihailo.milenkovic84@gmail.com>
Co-authored-by: Tom Trevethan <ttrevethan@blockstream.com>
