From 52a797bfe5ced4329f2272be417c35730ec8839f Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 22 Feb 2022 15:21:11 +0200 Subject: [PATCH 001/111] doc: Avoid ADL for function calls --- doc/developer-notes.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index bfb64093e1..4cee70863b 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -105,6 +105,28 @@ code. - `nullptr` is preferred over `NULL` or `(void*)0`. - `static_assert` is preferred over `assert` where possible. Generally; compile-time checking is preferred over run-time checking. +For function calls a namespace should be specified explicitly, unless such functions have been declared within it. +Otherwise, [argument-dependent lookup](https://en.cppreference.com/w/cpp/language/adl), also known as ADL, could be +triggered that makes code harder to maintain and reason about: +```c++ +#include + +namespace fs { +class path : public std::filesystem::path +{ +}; +// The intention is to disallow this function. +bool exists(const fs::path& p) = delete; +} // namespace fs + +int main() +{ + //fs::path p; // error + std::filesystem::path p; // compiled + exists(p); // ADL being used for unqualified name lookup +} +``` + Block style example: ```c++ int g_count = 0; From ef6dbe6863d92710fd2da7781e5b2aac87578751 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 26 Jan 2022 18:29:56 -0500 Subject: [PATCH 002/111] Respond to getheaders if we have sufficient chainwork Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes getting restarted during that time. To make things simpler to reason about, just use nMinimumChainWork as our anti-DoS threshold; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed). --- src/net_processing.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 273cb4fccb..ac6b4654f1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3189,9 +3189,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } + if (fImporting || fReindex) { + LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d while importing/reindexing\n", pfrom.GetId()); + return; + } + LOCK(cs_main); - if (m_chainman.ActiveChainstate().IsInitialBlockDownload() && !pfrom.HasPermission(NetPermissionFlags::Download)) { - LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom.GetId()); + + // Note that if we were to be on a chain that forks from the checkpointed + // chain, then serving those headers to a peer that has seen the + // checkpointed chain would cause that peer to disconnect us. Requiring + // that our chainwork exceed nMinimumChainWork is a protection against + // being fed a bogus chain when we started up for the first time and + // getting partitioned off the honest network for serving that chain to + // others. + if (m_chainman.ActiveTip() == nullptr || + (m_chainman.ActiveTip()->nChainWork < nMinimumChainWork && !pfrom.HasPermission(NetPermissionFlags::Download))) { + LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work\n", pfrom.GetId()); return; } From a35f963edf1a14ee572abea106fb0147575e4694 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 26 Jan 2022 19:18:27 -0500 Subject: [PATCH 003/111] Add test for getheaders behavior Expect responses to a getheaders iff the node has a chain with more than nMinimumChainWork --- test/functional/feature_minchainwork.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/functional/feature_minchainwork.py b/test/functional/feature_minchainwork.py index 489a729cfc..f6432ed20e 100755 --- a/test/functional/feature_minchainwork.py +++ b/test/functional/feature_minchainwork.py @@ -17,6 +17,7 @@ import time +from test_framework.p2p import P2PInterface, msg_getheaders from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal @@ -41,6 +42,9 @@ def setup_network(self): for i in range(self.num_nodes-1): self.connect_nodes(i+1, i) + # Set clock of node2 2 days ahead, to keep it in IBD during this test. + self.nodes[2].setmocktime(int(time.time()) + 48*60*60) + def run_test(self): # Start building a chain on node0. node2 shouldn't be able to sync until node1's # minchainwork is exceeded @@ -71,6 +75,15 @@ def run_test(self): assert self.nodes[1].getbestblockhash() != self.nodes[0].getbestblockhash() assert_equal(self.nodes[2].getblockcount(), starting_blockcount) + self.log.info("Check that getheaders requests to node2 are ignored") + peer = self.nodes[2].add_p2p_connection(P2PInterface()) + msg = msg_getheaders() + msg.locator.vHave = [int(self.nodes[2].getbestblockhash(), 16)] + msg.hashstop = 0 + peer.send_and_ping(msg) + time.sleep(5) + assert "headers" not in peer.last_message + self.log.info("Generating one more block") self.generate(self.nodes[0], 1) @@ -85,5 +98,13 @@ def run_test(self): self.sync_all() self.log.info(f"Blockcounts: {[n.getblockcount() for n in self.nodes]}") + self.log.info("Test that getheaders requests to node2 are not ignored") + peer.send_and_ping(msg) + assert "headers" in peer.last_message + + # Verify that node2 is in fact still in IBD (otherwise this test may + # not be exercising the logic we want!) + assert_equal(self.nodes[2].getblockchaininfo()['initialblockdownload'], True) + if __name__ == '__main__': MinimumChainWorkTest().main() From 318655c3951d69adfa0aaf9f7b5064172678494f Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 23 Feb 2022 00:09:43 +0100 Subject: [PATCH 004/111] Add missing references to signet in the release process --- doc/release-process.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/release-process.md b/doc/release-process.md index 5a74f72b6e..bad0e8cd72 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -301,13 +301,14 @@ Both variables are used as a guideline for how much space the user needs on thei Note that all values should be taken from a **fully synced** node and have an overhead of 5-10% added on top of its base value. To calculate `m_assumed_blockchain_size`: -- For `mainnet` -> Take the size of the data directory, excluding `/regtest` and `/testnet3` directories. +- For `mainnet` -> Take the size of the data directory, excluding the `/testnet3`, `/signet`, and `/regtest` directories. - For `testnet` -> Take the size of the `/testnet3` directory. - +- For `signet` -> Take the size of the `/signet` directory. To calculate `m_assumed_chain_state_size`: - For `mainnet` -> Take the size of the `/chainstate` directory. - For `testnet` -> Take the size of the `/testnet3/chainstate` directory. +- For `signet` -> Take the size of the `/signet/chainstate` directory. Notes: - When taking the size for `m_assumed_blockchain_size`, there's no need to exclude the `/chainstate` directory since it's a guideline value and an overhead will be added anyway. From b4d2d74767ed0441dbc3a789cad2334eaac54290 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 23 Feb 2022 00:11:52 +0100 Subject: [PATCH 005/111] Release process: specify blockchain/chain_state units, reduce repetition --- doc/release-process.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/doc/release-process.md b/doc/release-process.md index bad0e8cd72..b0f7ab02db 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -300,15 +300,15 @@ cat "$VERSION"/*/all.SHA256SUMS.asc > SHA256SUMS.asc Both variables are used as a guideline for how much space the user needs on their drive in total, not just strictly for the blockchain. Note that all values should be taken from a **fully synced** node and have an overhead of 5-10% added on top of its base value. -To calculate `m_assumed_blockchain_size`: -- For `mainnet` -> Take the size of the data directory, excluding the `/testnet3`, `/signet`, and `/regtest` directories. -- For `testnet` -> Take the size of the `/testnet3` directory. -- For `signet` -> Take the size of the `/signet` directory. - -To calculate `m_assumed_chain_state_size`: -- For `mainnet` -> Take the size of the `/chainstate` directory. -- For `testnet` -> Take the size of the `/testnet3/chainstate` directory. -- For `signet` -> Take the size of the `/signet/chainstate` directory. +To calculate `m_assumed_blockchain_size`, take the size in GiB of these directories: +- For `mainnet` -> the data directory, excluding the `/testnet3`, `/signet`, and `/regtest` directories +- For `testnet` -> `/testnet3` +- For `signet` -> `/signet` + +To calculate `m_assumed_chain_state_size`, take the size in GiB of these directories: +- For `mainnet` -> `/chainstate` +- For `testnet` -> `/testnet3/chainstate` +- For `signet` -> `/signet/chainstate` Notes: - When taking the size for `m_assumed_blockchain_size`, there's no need to exclude the `/chainstate` directory since it's a guideline value and an overhead will be added anyway. From e538eada7c559362f43051f18d38b3d3c3be2dc8 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Mon, 28 Feb 2022 17:39:15 +0100 Subject: [PATCH 006/111] Release process: exclude huge files for mainnet m_assumed_blockchain_size --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index b0f7ab02db..1031c6c9c1 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -301,7 +301,7 @@ Both variables are used as a guideline for how much space the user needs on thei Note that all values should be taken from a **fully synced** node and have an overhead of 5-10% added on top of its base value. To calculate `m_assumed_blockchain_size`, take the size in GiB of these directories: -- For `mainnet` -> the data directory, excluding the `/testnet3`, `/signet`, and `/regtest` directories +- For `mainnet` -> the data directory, excluding the `/testnet3`, `/signet`, and `/regtest` directories and any overly large files, e.g. a huge `debug.log` - For `testnet` -> `/testnet3` - For `signet` -> `/signet` From e8f844888f3663d0cbb8765271d1ee5fea4af579 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 23 Feb 2022 21:41:01 +0100 Subject: [PATCH 007/111] Clarify release process overhead note to be more actionable --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index 1031c6c9c1..f5f343a6a3 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -312,4 +312,4 @@ To calculate `m_assumed_chain_state_size`, take the size in GiB of these directo Notes: - When taking the size for `m_assumed_blockchain_size`, there's no need to exclude the `/chainstate` directory since it's a guideline value and an overhead will be added anyway. -- The expected overhead for growth may change over time, so it may not be the same value as last release; pay attention to that when changing the variables. +- The expected overhead for growth may change over time. Consider whether the percentage needs to be changed in response; if so, update it here in this section. From 584147682ad670cd3be99006af5d643549bba3bc Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 23 Feb 2022 21:19:10 +0100 Subject: [PATCH 008/111] Reorganize release process chainparams section to reduce repetition --- doc/release-process.md | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/doc/release-process.md b/doc/release-process.md index f5f343a6a3..b640084b46 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -28,14 +28,17 @@ Release Process #### Before branch-off * Update hardcoded [seeds](/contrib/seeds/README.md), see [this pull request](https://github.com/bitcoin/bitcoin/pull/7415) for an example. -* Update [`src/chainparams.cpp`](/src/chainparams.cpp) m_assumed_blockchain_size and m_assumed_chain_state_size with the current size plus some overhead (see [this](#how-to-calculate-assumed-blockchain-and-chain-state-size) for information on how to calculate them). -* Update [`src/chainparams.cpp`](/src/chainparams.cpp) chainTxData with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC, see - [this pull request](https://github.com/bitcoin/bitcoin/pull/20263) for an example. Reviewers can verify the results by running `getchaintxstats ` with the `window_block_count` and `window_final_block_hash` from your output. -* Update `src/chainparams.cpp` nMinimumChainWork and defaultAssumeValid (and the block height comment) with information from the `getblockheader` (and `getblockhash`) RPCs. - - The selected value must not be orphaned so it may be useful to set the value two blocks back from the tip. - - Testnet should be set some tens of thousands back from the tip due to reorgs there. - - This update should be reviewed with a reindex-chainstate with assumevalid=0 to catch any defect - that causes rejection of blocks in the past history. +* Update the following variables in [`src/chainparams.cpp`](/src/chainparams.cpp): + - `m_assumed_blockchain_size` and `m_assumed_chain_state_size` with the current size plus some overhead (see + [this](#how-to-calculate-assumed-blockchain-and-chain-state-size) for information on how to calculate them). + - `chainTxData` with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC, see + [this pull request](https://github.com/bitcoin/bitcoin/pull/20263) for an example. Reviewers can verify the results by running + `getchaintxstats ` with the `window_block_count` and `window_final_block_hash` from your output. + - `nMinimumChainWork` and `defaultAssumeValid` (and the block height comment) with information from the `getblockheader` (and `getblockhash`) RPCs. + - The selected value must not be orphaned so it may be useful to set the value two blocks back from the tip. + - Testnet should be set some tens of thousands back from the tip due to reorgs there. + - This update should be reviewed with a reindex-chainstate with assumevalid=0 to catch any defect + that causes rejection of blocks in the past history. - Clear the release notes and move them to the wiki (see "Write the release notes" below). - Translations on Transifex - Create [a new resource](https://www.transifex.com/bitcoin/bitcoin/content/) named after the major version with the slug `[bitcoin.qt-translation-x]`, where `RRR` is the major branch number padded with zeros. Use `src/qt/locale/bitcoin_en.xlf` to create it. From fe048f7f7cd15597f24ca219a0077c46d088ca30 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Mon, 28 Feb 2022 15:07:48 +0100 Subject: [PATCH 009/111] Specify in release process which chains need to be updated --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index b640084b46..ddd1b9266b 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -28,7 +28,7 @@ Release Process #### Before branch-off * Update hardcoded [seeds](/contrib/seeds/README.md), see [this pull request](https://github.com/bitcoin/bitcoin/pull/7415) for an example. -* Update the following variables in [`src/chainparams.cpp`](/src/chainparams.cpp): +* Update the following variables in [`src/chainparams.cpp`](/src/chainparams.cpp) for mainnet, testnet, and signet: - `m_assumed_blockchain_size` and `m_assumed_chain_state_size` with the current size plus some overhead (see [this](#how-to-calculate-assumed-blockchain-and-chain-state-size) for information on how to calculate them). - `chainTxData` with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC, see From 415345d5475854ae34056594ac0bc464d739d3af Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 24 Feb 2022 22:58:53 +0100 Subject: [PATCH 010/111] Release process: use 4096 blocks and getbestblockhash for getchaintxstats --- doc/release-process.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index ddd1b9266b..680aee9e85 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -31,7 +31,8 @@ Release Process * Update the following variables in [`src/chainparams.cpp`](/src/chainparams.cpp) for mainnet, testnet, and signet: - `m_assumed_blockchain_size` and `m_assumed_chain_state_size` with the current size plus some overhead (see [this](#how-to-calculate-assumed-blockchain-and-chain-state-size) for information on how to calculate them). - - `chainTxData` with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC, see + - `chainTxData` with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC with an + `nBlocks` of 4096 (28 days) and a `bestblockhash` of RPC `getbestblockhash`; see [this pull request](https://github.com/bitcoin/bitcoin/pull/20263) for an example. Reviewers can verify the results by running `getchaintxstats ` with the `window_block_count` and `window_final_block_hash` from your output. - `nMinimumChainWork` and `defaultAssumeValid` (and the block height comment) with information from the `getblockheader` (and `getblockhash`) RPCs. From 74743ad90590708b46a8bc8cb9cefedb66471306 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Mon, 28 Feb 2022 14:52:39 +0100 Subject: [PATCH 011/111] Clarify in release process how to update defaultAssumeValid/nMinimumChainWork --- doc/release-process.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/doc/release-process.md b/doc/release-process.md index 680aee9e85..79df1074a4 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -31,15 +31,17 @@ Release Process * Update the following variables in [`src/chainparams.cpp`](/src/chainparams.cpp) for mainnet, testnet, and signet: - `m_assumed_blockchain_size` and `m_assumed_chain_state_size` with the current size plus some overhead (see [this](#how-to-calculate-assumed-blockchain-and-chain-state-size) for information on how to calculate them). + - The following updates should be reviewed with `reindex-chainstate` and `assumevalid=0` to catch any defect + that causes rejection of blocks in the past history. - `chainTxData` with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC with an `nBlocks` of 4096 (28 days) and a `bestblockhash` of RPC `getbestblockhash`; see [this pull request](https://github.com/bitcoin/bitcoin/pull/20263) for an example. Reviewers can verify the results by running `getchaintxstats ` with the `window_block_count` and `window_final_block_hash` from your output. - - `nMinimumChainWork` and `defaultAssumeValid` (and the block height comment) with information from the `getblockheader` (and `getblockhash`) RPCs. - - The selected value must not be orphaned so it may be useful to set the value two blocks back from the tip. - - Testnet should be set some tens of thousands back from the tip due to reorgs there. - - This update should be reviewed with a reindex-chainstate with assumevalid=0 to catch any defect - that causes rejection of blocks in the past history. + - `defaultAssumeValid` with the output of RPC `getblockhash` using the `height` of `window_final_block_height` above + (and update the block height comment with that height), taking into account the following: + - On mainnet, the selected value must not be orphaned, so it may be useful to set the height two blocks back from the tip. + - Testnet should be set with a height some tens of thousands back from the tip, due to reorgs there. + - `nMinimumChainWork` with the "chainwork" value of RPC `getblockheader` using the same height as that selected for the previous step. - Clear the release notes and move them to the wiki (see "Write the release notes" below). - Translations on Transifex - Create [a new resource](https://www.transifex.com/bitcoin/bitcoin/content/) named after the major version with the slug `[bitcoin.qt-translation-x]`, where `RRR` is the major branch number padded with zeros. Use `src/qt/locale/bitcoin_en.xlf` to create it. From c5f65db0f03b52bc4525acae944173829290ce6f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 9 Apr 2022 17:03:30 +0200 Subject: [PATCH 012/111] miniscript: remove a workaround for a GCC 4.8 bug Co-authored-by: Pieter Wuille --- src/script/miniscript.cpp | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp index 019f02f159..a64216f61d 100644 --- a/src/script/miniscript.cpp +++ b/src/script/miniscript.cpp @@ -17,21 +17,19 @@ Type SanitizeType(Type e) { int num_types = (e << "K"_mst) + (e << "V"_mst) + (e << "B"_mst) + (e << "W"_mst); if (num_types == 0) return ""_mst; // No valid type, don't care about the rest assert(num_types == 1); // K, V, B, W all conflict with each other - bool ok = // Work around a GCC 4.8 bug that breaks user-defined literals in macro calls. - (!(e << "z"_mst) || !(e << "o"_mst)) && // z conflicts with o - (!(e << "n"_mst) || !(e << "z"_mst)) && // n conflicts with z - (!(e << "n"_mst) || !(e << "W"_mst)) && // n conflicts with W - (!(e << "V"_mst) || !(e << "d"_mst)) && // V conflicts with d - (!(e << "K"_mst) || (e << "u"_mst)) && // K implies u - (!(e << "V"_mst) || !(e << "u"_mst)) && // V conflicts with u - (!(e << "e"_mst) || !(e << "f"_mst)) && // e conflicts with f - (!(e << "e"_mst) || (e << "d"_mst)) && // e implies d - (!(e << "V"_mst) || !(e << "e"_mst)) && // V conflicts with e - (!(e << "d"_mst) || !(e << "f"_mst)) && // d conflicts with f - (!(e << "V"_mst) || (e << "f"_mst)) && // V implies f - (!(e << "K"_mst) || (e << "s"_mst)) && // K implies s - (!(e << "z"_mst) || (e << "m"_mst)); // z implies m - assert(ok); + assert(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o + assert(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z + assert(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W + assert(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d + assert(!(e << "K"_mst) || (e << "u"_mst)); // K implies u + assert(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u + assert(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f + assert(!(e << "e"_mst) || (e << "d"_mst)); // e implies d + assert(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e + assert(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f + assert(!(e << "V"_mst) || (e << "f"_mst)); // V implies f + assert(!(e << "K"_mst) || (e << "s"_mst)); // K implies s + assert(!(e << "z"_mst) || (e << "m"_mst)); // z implies m return e; } From 5922c662c08a061b3b3d5ac34a31f9f9d4640d47 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 15 Apr 2022 13:38:05 +0200 Subject: [PATCH 013/111] scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' The 'Fragment' type was previously named 'Nodetype'. For clarity, name the variables the same. -BEGIN VERIFY SCRIPT- sed -i 's/nodetype/fragment/g' src/script/miniscript.* -END VERIFY SCRIPT- Co-authored-by: Pieter Wuille --- src/script/miniscript.cpp | 38 +++++++++++----------- src/script/miniscript.h | 68 +++++++++++++++++++-------------------- 2 files changed, 53 insertions(+), 53 deletions(-) diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp index a64216f61d..c4dcd58cf7 100644 --- a/src/script/miniscript.cpp +++ b/src/script/miniscript.cpp @@ -33,51 +33,51 @@ Type SanitizeType(Type e) { return e; } -Type ComputeType(Fragment nodetype, Type x, Type y, Type z, const std::vector& sub_types, uint32_t k, size_t data_size, size_t n_subs, size_t n_keys) { +Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector& sub_types, uint32_t k, size_t data_size, size_t n_subs, size_t n_keys) { // Sanity check on data - if (nodetype == Fragment::SHA256 || nodetype == Fragment::HASH256) { + if (fragment == Fragment::SHA256 || fragment == Fragment::HASH256) { assert(data_size == 32); - } else if (nodetype == Fragment::RIPEMD160 || nodetype == Fragment::HASH160) { + } else if (fragment == Fragment::RIPEMD160 || fragment == Fragment::HASH160) { assert(data_size == 20); } else { assert(data_size == 0); } // Sanity check on k - if (nodetype == Fragment::OLDER || nodetype == Fragment::AFTER) { + if (fragment == Fragment::OLDER || fragment == Fragment::AFTER) { assert(k >= 1 && k < 0x80000000UL); - } else if (nodetype == Fragment::MULTI) { + } else if (fragment == Fragment::MULTI) { assert(k >= 1 && k <= n_keys); - } else if (nodetype == Fragment::THRESH) { + } else if (fragment == Fragment::THRESH) { assert(k >= 1 && k <= n_subs); } else { assert(k == 0); } // Sanity check on subs - if (nodetype == Fragment::AND_V || nodetype == Fragment::AND_B || nodetype == Fragment::OR_B || - nodetype == Fragment::OR_C || nodetype == Fragment::OR_I || nodetype == Fragment::OR_D) { + if (fragment == Fragment::AND_V || fragment == Fragment::AND_B || fragment == Fragment::OR_B || + fragment == Fragment::OR_C || fragment == Fragment::OR_I || fragment == Fragment::OR_D) { assert(n_subs == 2); - } else if (nodetype == Fragment::ANDOR) { + } else if (fragment == Fragment::ANDOR) { assert(n_subs == 3); - } else if (nodetype == Fragment::WRAP_A || nodetype == Fragment::WRAP_S || nodetype == Fragment::WRAP_C || - nodetype == Fragment::WRAP_D || nodetype == Fragment::WRAP_V || nodetype == Fragment::WRAP_J || - nodetype == Fragment::WRAP_N) { + } else if (fragment == Fragment::WRAP_A || fragment == Fragment::WRAP_S || fragment == Fragment::WRAP_C || + fragment == Fragment::WRAP_D || fragment == Fragment::WRAP_V || fragment == Fragment::WRAP_J || + fragment == Fragment::WRAP_N) { assert(n_subs == 1); - } else if (nodetype != Fragment::THRESH) { + } else if (fragment != Fragment::THRESH) { assert(n_subs == 0); } // Sanity check on keys - if (nodetype == Fragment::PK_K || nodetype == Fragment::PK_H) { + if (fragment == Fragment::PK_K || fragment == Fragment::PK_H) { assert(n_keys == 1); - } else if (nodetype == Fragment::MULTI) { + } else if (fragment == Fragment::MULTI) { assert(n_keys >= 1 && n_keys <= 20); } else { assert(n_keys == 0); } - // Below is the per-nodetype logic for computing the expression types. + // Below is the per-fragment logic for computing the expression types. // It heavily relies on Type's << operator (where "X << a_mst" means // "X has all properties listed in a"). - switch (nodetype) { + switch (fragment) { case Fragment::PK_K: return "Konudemsxk"_mst; case Fragment::PK_H: return "Knudemsxk"_mst; case Fragment::OLDER: return @@ -248,8 +248,8 @@ Type ComputeType(Fragment nodetype, Type x, Type y, Type z, const std::vector& sub_types, uint32_t k, size_t data_size, size_t n_subs, size_t n_keys); +Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector& sub_types, uint32_t k, size_t data_size, size_t n_subs, size_t n_keys); //! Helper function for Node::CalcScriptLen. -size_t ComputeScriptLen(Fragment nodetype, Type sub0typ, size_t subsize, uint32_t k, size_t n_subs, size_t n_keys); +size_t ComputeScriptLen(Fragment fragment, Type sub0typ, size_t subsize, uint32_t k, size_t n_subs, size_t n_keys); //! A helper sanitizer/checker for the output of CalcType. Type SanitizeType(Type x); @@ -279,7 +279,7 @@ struct StackSize { template struct Node { //! What node type this node is. - const Fragment nodetype; + const Fragment fragment; //! The k parameter (time for OLDER/AFTER, threshold for THRESH(_M)) const uint32_t k = 0; //! The keys used by this expression (only for PK_K/PK_H/MULTI) @@ -306,7 +306,7 @@ struct Node { subsize += sub->ScriptSize(); } Type sub0type = subs.size() > 0 ? subs[0]->GetType() : ""_mst; - return internal::ComputeScriptLen(nodetype, sub0type, subsize, k, subs.size(), keys.size()); + return internal::ComputeScriptLen(fragment, sub0type, subsize, k, subs.size(), keys.size()); } /* Apply a recursive algorithm to a Miniscript tree, without actual recursive calls. @@ -414,7 +414,7 @@ struct Node { // THRESH has a variable number of subexpressions std::vector sub_types; - if (nodetype == Fragment::THRESH) { + if (fragment == Fragment::THRESH) { for (const auto& sub : subs) sub_types.push_back(sub->GetType()); } // All other nodes than THRESH can be computed just from the types of the 0-3 subexpressions. @@ -422,7 +422,7 @@ struct Node { Type y = subs.size() > 1 ? subs[1]->GetType() : ""_mst; Type z = subs.size() > 2 ? subs[2]->GetType() : ""_mst; - return SanitizeType(ComputeType(nodetype, x, y, z, sub_types, k, data.size(), subs.size(), keys.size())); + return SanitizeType(ComputeType(fragment, x, y, z, sub_types, k, data.size(), subs.size(), keys.size())); } public: @@ -434,17 +434,17 @@ struct Node { // by an OP_VERIFY (which may need to be combined with the last script opcode). auto downfn = [](bool verify, const Node& node, size_t index) { // For WRAP_V, the subexpression is certainly followed by OP_VERIFY. - if (node.nodetype == Fragment::WRAP_V) return true; + if (node.fragment == Fragment::WRAP_V) return true; // The subexpression of WRAP_S, and the last subexpression of AND_V // inherit the followed-by-OP_VERIFY property from the parent. - if (node.nodetype == Fragment::WRAP_S || - (node.nodetype == Fragment::AND_V && index == 1)) return verify; + if (node.fragment == Fragment::WRAP_S || + (node.fragment == Fragment::AND_V && index == 1)) return verify; return false; }; // The upward function computes for a node, given its followed-by-OP_VERIFY status // and the CScripts of its child nodes, the CScript of the node. auto upfn = [&ctx](bool verify, const Node& node, Span subs) -> CScript { - switch (node.nodetype) { + switch (node.fragment) { case Fragment::PK_K: return BuildScript(ctx.ToPKBytes(node.keys[0])); case Fragment::PK_H: return BuildScript(OP_DUP, OP_HASH160, ctx.ToPKHBytes(node.keys[0]), OP_EQUALVERIFY); case Fragment::OLDER: return BuildScript(node.k, OP_CHECKSEQUENCEVERIFY); @@ -502,30 +502,30 @@ struct Node { // the TreeEvalMaybe algorithm. The State is a boolean: whether the parent node is a // wrapper. If so, non-wrapper expressions must be prefixed with a ":". auto downfn = [](bool, const Node& node, size_t) { - return (node.nodetype == Fragment::WRAP_A || node.nodetype == Fragment::WRAP_S || - node.nodetype == Fragment::WRAP_D || node.nodetype == Fragment::WRAP_V || - node.nodetype == Fragment::WRAP_J || node.nodetype == Fragment::WRAP_N || - node.nodetype == Fragment::WRAP_C || - (node.nodetype == Fragment::AND_V && node.subs[1]->nodetype == Fragment::JUST_1) || - (node.nodetype == Fragment::OR_I && node.subs[0]->nodetype == Fragment::JUST_0) || - (node.nodetype == Fragment::OR_I && node.subs[1]->nodetype == Fragment::JUST_0)); + return (node.fragment == Fragment::WRAP_A || node.fragment == Fragment::WRAP_S || + node.fragment == Fragment::WRAP_D || node.fragment == Fragment::WRAP_V || + node.fragment == Fragment::WRAP_J || node.fragment == Fragment::WRAP_N || + node.fragment == Fragment::WRAP_C || + (node.fragment == Fragment::AND_V && node.subs[1]->fragment == Fragment::JUST_1) || + (node.fragment == Fragment::OR_I && node.subs[0]->fragment == Fragment::JUST_0) || + (node.fragment == Fragment::OR_I && node.subs[1]->fragment == Fragment::JUST_0)); }; // The upward function computes for a node, given whether its parent is a wrapper, // and the string representations of its child nodes, the string representation of the node. auto upfn = [&ctx](bool wrapped, const Node& node, Span subs) -> std::optional { std::string ret = wrapped ? ":" : ""; - switch (node.nodetype) { + switch (node.fragment) { case Fragment::WRAP_A: return "a" + std::move(subs[0]); case Fragment::WRAP_S: return "s" + std::move(subs[0]); case Fragment::WRAP_C: - if (node.subs[0]->nodetype == Fragment::PK_K) { + if (node.subs[0]->fragment == Fragment::PK_K) { // pk(K) is syntactic sugar for c:pk_k(K) std::string key_str; if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {}; return std::move(ret) + "pk(" + std::move(key_str) + ")"; } - if (node.subs[0]->nodetype == Fragment::PK_H) { + if (node.subs[0]->fragment == Fragment::PK_H) { // pkh(K) is syntactic sugar for c:pk_h(K) std::string key_str; if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {}; @@ -538,15 +538,15 @@ struct Node { case Fragment::WRAP_N: return "n" + std::move(subs[0]); case Fragment::AND_V: // t:X is syntactic sugar for and_v(X,1). - if (node.subs[1]->nodetype == Fragment::JUST_1) return "t" + std::move(subs[0]); + if (node.subs[1]->fragment == Fragment::JUST_1) return "t" + std::move(subs[0]); break; case Fragment::OR_I: - if (node.subs[0]->nodetype == Fragment::JUST_0) return "l" + std::move(subs[1]); - if (node.subs[1]->nodetype == Fragment::JUST_0) return "u" + std::move(subs[0]); + if (node.subs[0]->fragment == Fragment::JUST_0) return "l" + std::move(subs[1]); + if (node.subs[1]->fragment == Fragment::JUST_0) return "u" + std::move(subs[0]); break; default: break; } - switch (node.nodetype) { + switch (node.fragment) { case Fragment::PK_K: { std::string key_str; if (!ctx.ToString(node.keys[0], key_str)) return {}; @@ -573,7 +573,7 @@ struct Node { case Fragment::OR_I: return std::move(ret) + "or_i(" + std::move(subs[0]) + "," + std::move(subs[1]) + ")"; case Fragment::ANDOR: // and_n(X,Y) is syntactic sugar for andor(X,Y,0). - if (node.subs[2]->nodetype == Fragment::JUST_0) return std::move(ret) + "and_n(" + std::move(subs[0]) + "," + std::move(subs[1]) + ")"; + if (node.subs[2]->fragment == Fragment::JUST_0) return std::move(ret) + "and_n(" + std::move(subs[0]) + "," + std::move(subs[1]) + ")"; return std::move(ret) + "andor(" + std::move(subs[0]) + "," + std::move(subs[1]) + "," + std::move(subs[2]) + ")"; case Fragment::MULTI: { auto str = std::move(ret) + "multi(" + ::ToString(node.k); @@ -602,7 +602,7 @@ struct Node { } internal::Ops CalcOps() const { - switch (nodetype) { + switch (fragment) { case Fragment::JUST_1: return {0, 0, {}}; case Fragment::JUST_0: return {0, {}, 0}; case Fragment::PK_K: return {0, 0, 0}; @@ -676,7 +676,7 @@ struct Node { } internal::StackSize CalcStackSize() const { - switch (nodetype) { + switch (fragment) { case Fragment::JUST_0: return {{}, 0}; case Fragment::JUST_1: case Fragment::OLDER: @@ -767,7 +767,7 @@ struct Node { //! Equality testing. bool operator==(const Node& arg) const { - if (nodetype != arg.nodetype) return false; + if (fragment != arg.fragment) return false; if (k != arg.k) return false; if (data != arg.data) return false; if (keys != arg.keys) return false; @@ -781,12 +781,12 @@ struct Node { } // Constructors with various argument combinations. - Node(Fragment nt, std::vector> sub, std::vector arg, uint32_t val = 0) : nodetype(nt), k(val), data(std::move(arg)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} - Node(Fragment nt, std::vector arg, uint32_t val = 0) : nodetype(nt), k(val), data(std::move(arg)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} - Node(Fragment nt, std::vector> sub, std::vector key, uint32_t val = 0) : nodetype(nt), k(val), keys(std::move(key)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} - Node(Fragment nt, std::vector key, uint32_t val = 0) : nodetype(nt), k(val), keys(std::move(key)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} - Node(Fragment nt, std::vector> sub, uint32_t val = 0) : nodetype(nt), k(val), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} - Node(Fragment nt, uint32_t val = 0) : nodetype(nt), k(val), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(Fragment nt, std::vector> sub, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(Fragment nt, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(Fragment nt, std::vector> sub, std::vector key, uint32_t val = 0) : fragment(nt), k(val), keys(std::move(key)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(Fragment nt, std::vector key, uint32_t val = 0) : fragment(nt), k(val), keys(std::move(key)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(Fragment nt, std::vector> sub, uint32_t val = 0) : fragment(nt), k(val), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + Node(Fragment nt, uint32_t val = 0) : fragment(nt), k(val), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} }; namespace internal { From 1ab8d89fd1bdb3c0f2a506b4a10df6c23ba21c48 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 9 Apr 2022 17:38:28 +0200 Subject: [PATCH 014/111] miniscript: make equality operator non-recursive Co-authored-by: Pieter Wuille --- src/script/miniscript.h | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 96a7f21dc5..270ce06ba0 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -408,6 +408,26 @@ struct Node { )); } + /** Compare two miniscript subtrees, using a non-recursive algorithm. */ + friend int Compare(const Node& node1, const Node& node2) + { + std::vector&, const Node&>> queue; + queue.emplace_back(node1, node2); + while (!queue.empty()) { + const auto& [a, b] = queue.back(); + queue.pop_back(); + if (std::tie(a.fragment, a.k, a.keys, a.data) < std::tie(b.fragment, b.k, b.keys, b.data)) return -1; + if (std::tie(b.fragment, b.k, b.keys, b.data) < std::tie(a.fragment, a.k, a.keys, a.data)) return 1; + if (a.subs.size() < b.subs.size()) return -1; + if (b.subs.size() < a.subs.size()) return 1; + size_t n = a.subs.size(); + for (size_t i = 0; i < n; ++i) { + queue.emplace_back(*a.subs[n - 1 - i], *b.subs[n - 1 - i]); + } + } + return 0; + } + //! Compute the type for this miniscript. Type CalcType() const { using namespace internal; @@ -765,20 +785,7 @@ struct Node { bool IsSaneTopLevel() const { return IsValidTopLevel() && IsSane() && NeedsSignature(); } //! Equality testing. - bool operator==(const Node& arg) const - { - if (fragment != arg.fragment) return false; - if (k != arg.k) return false; - if (data != arg.data) return false; - if (keys != arg.keys) return false; - if (subs.size() != arg.subs.size()) return false; - for (size_t i = 0; i < subs.size(); ++i) { - if (!(*subs[i] == *arg.subs[i])) return false; - } - assert(scriptlen == arg.scriptlen); - assert(typ == arg.typ); - return true; - } + bool operator==(const Node& arg) const { return Compare(*this, arg) == 0; } // Constructors with various argument combinations. Node(Fragment nt, std::vector> sub, std::vector arg, uint32_t val = 0) : fragment(nt), k(val), data(std::move(arg)), subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} From ed45ee3882e69266d550b56ff69388e071f0ad1b Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 11 Apr 2022 14:07:25 +0200 Subject: [PATCH 015/111] miniscript: use optional instead of bool/outarg Co-authored-by: Pieter Wuille --- src/script/miniscript.cpp | 25 +++--- src/script/miniscript.h | 117 ++++++++++++++-------------- src/test/fuzz/miniscript_decode.cpp | 27 ++++--- src/test/miniscript_tests.cpp | 19 ++--- 4 files changed, 92 insertions(+), 96 deletions(-) diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp index c4dcd58cf7..b91152671f 100644 --- a/src/script/miniscript.cpp +++ b/src/script/miniscript.cpp @@ -281,16 +281,15 @@ size_t ComputeScriptLen(Fragment fragment, Type sub0typ, size_t subsize, uint32_ return 0; } -bool DecomposeScript(const CScript& script, std::vector>>& out) +std::optional>>> DecomposeScript(const CScript& script) { - out.clear(); + std::vector>> out; CScript::const_iterator it = script.begin(), itend = script.end(); while (it != itend) { std::vector push_data; opcodetype opcode; if (!script.GetOp(it, opcode, push_data)) { - out.clear(); - return false; + return {}; } else if (opcode >= OP_1 && opcode <= OP_16) { // Deal with OP_n (GetOp does not turn them into pushes). push_data.assign(1, CScript::DecodeOP_N(opcode)); @@ -307,30 +306,28 @@ bool DecomposeScript(const CScript& script, std::vector()); opcode = OP_VERIFY; } else if (IsPushdataOp(opcode)) { - if (!CheckMinimalPush(push_data, opcode)) return false; + if (!CheckMinimalPush(push_data, opcode)) return {}; } else if (it != itend && (opcode == OP_CHECKSIG || opcode == OP_CHECKMULTISIG || opcode == OP_EQUAL) && (*it == OP_VERIFY)) { // Rule out non minimal VERIFY sequences - return false; + return {}; } out.emplace_back(opcode, std::move(push_data)); } std::reverse(out.begin(), out.end()); - return true; + return out; } -bool ParseScriptNumber(const std::pair>& in, int64_t& k) { +std::optional ParseScriptNumber(const std::pair>& in) { if (in.first == OP_0) { - k = 0; - return true; + return 0; } if (!in.second.empty()) { - if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return false; + if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return {}; try { - k = CScriptNum(in.second, true).GetInt64(); - return true; + return CScriptNum(in.second, true).GetInt64(); } catch(const scriptnum_error&) {} } - return false; + return {}; } int FindNextChar(Span sp, const char m) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 270ce06ba0..20aeb3c01b 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -517,7 +517,7 @@ struct Node { } template - bool ToString(const CTx& ctx, std::string& ret) const { + std::optional ToString(const CTx& ctx) const { // To construct the std::string representation for a Miniscript object, we use // the TreeEvalMaybe algorithm. The State is a boolean: whether the parent node is a // wrapper. If so, non-wrapper expressions must be prefixed with a ":". @@ -541,15 +541,15 @@ struct Node { case Fragment::WRAP_C: if (node.subs[0]->fragment == Fragment::PK_K) { // pk(K) is syntactic sugar for c:pk_k(K) - std::string key_str; - if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {}; - return std::move(ret) + "pk(" + std::move(key_str) + ")"; + auto key_str = ctx.ToString(node.subs[0]->keys[0]); + if (!key_str) return {}; + return std::move(ret) + "pk(" + std::move(*key_str) + ")"; } if (node.subs[0]->fragment == Fragment::PK_H) { // pkh(K) is syntactic sugar for c:pk_h(K) - std::string key_str; - if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {}; - return std::move(ret) + "pkh(" + std::move(key_str) + ")"; + auto key_str = ctx.ToString(node.subs[0]->keys[0]); + if (!key_str) return {}; + return std::move(ret) + "pkh(" + std::move(*key_str) + ")"; } return "c" + std::move(subs[0]); case Fragment::WRAP_D: return "d" + std::move(subs[0]); @@ -568,14 +568,14 @@ struct Node { } switch (node.fragment) { case Fragment::PK_K: { - std::string key_str; - if (!ctx.ToString(node.keys[0], key_str)) return {}; - return std::move(ret) + "pk_k(" + std::move(key_str) + ")"; + auto key_str = ctx.ToString(node.keys[0]); + if (!key_str) return {}; + return std::move(ret) + "pk_k(" + std::move(*key_str) + ")"; } case Fragment::PK_H: { - std::string key_str; - if (!ctx.ToString(node.keys[0], key_str)) return {}; - return std::move(ret) + "pk_h(" + std::move(key_str) + ")"; + auto key_str = ctx.ToString(node.keys[0]); + if (!key_str) return {}; + return std::move(ret) + "pk_h(" + std::move(*key_str) + ")"; } case Fragment::AFTER: return std::move(ret) + "after(" + ::ToString(node.k) + ")"; case Fragment::OLDER: return std::move(ret) + "older(" + ::ToString(node.k) + ")"; @@ -598,9 +598,9 @@ struct Node { case Fragment::MULTI: { auto str = std::move(ret) + "multi(" + ::ToString(node.k); for (const auto& key : node.keys) { - std::string key_str; - if (!ctx.ToString(key, key_str)) return {}; - str += "," + std::move(key_str); + auto key_str = ctx.ToString(key); + if (!key_str) return {}; + str += "," + std::move(*key_str); } return std::move(str) + ")"; } @@ -616,9 +616,7 @@ struct Node { return ""; // Should never be reached. }; - auto res = TreeEvalMaybe(false, downfn, upfn); - if (res.has_value()) ret = std::move(*res); - return res.has_value(); + return TreeEvalMaybe(false, downfn, upfn); } internal::Ops CalcOps() const { @@ -858,11 +856,11 @@ int FindNextChar(Span in, const char m); template std::optional> ParseKeyEnd(Span in, const Ctx& ctx) { - Key key; int key_size = FindNextChar(in, ')'); if (key_size < 1) return {}; - if (!ctx.FromString(in.begin(), in.begin() + key_size, key)) return {}; - return {{std::move(key), key_size}}; + auto key = ctx.FromString(in.begin(), in.begin() + key_size); + if (!key) return {}; + return {{std::move(*key), key_size}}; } /** Parse a hex string ending at the end of the fragment's text representation. */ @@ -1029,12 +1027,12 @@ inline NodeRef Parse(Span in, const Ctx& ctx) // Get keys std::vector keys; while (next_comma != -1) { - Key key; next_comma = FindNextChar(in, ','); int key_length = (next_comma == -1) ? FindNextChar(in, ')') : next_comma; if (key_length < 1) return {}; - if (!ctx.FromString(in.begin(), in.begin() + key_length, key)) return {}; - keys.push_back(std::move(key)); + auto key = ctx.FromString(in.begin(), in.begin() + key_length); + if (!key) return {}; + keys.push_back(std::move(*key)); in = in.subspan(key_length + 1); } if (keys.size() < 1 || keys.size() > 20) return {}; @@ -1207,10 +1205,10 @@ inline NodeRef Parse(Span in, const Ctx& ctx) * and OP_EQUALVERIFY are decomposed into OP_CHECKSIG, OP_CHECKMULTISIG, OP_EQUAL * respectively, plus OP_VERIFY. */ -bool DecomposeScript(const CScript& script, std::vector>>& out); +std::optional>>> DecomposeScript(const CScript& script); /** Determine whether the passed pair (created by DecomposeScript) is pushing a number. */ -bool ParseScriptNumber(const std::pair>& in, int64_t& k); +std::optional ParseScriptNumber(const std::pair>& in); enum class DecodeContext { /** A single expression of type B, K, or V. Specifically, this can't be an @@ -1317,34 +1315,35 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) } // Public keys if (in[0].second.size() == 33) { - Key key; - if (!ctx.FromPKBytes(in[0].second.begin(), in[0].second.end(), key)) return {}; + auto key = ctx.FromPKBytes(in[0].second.begin(), in[0].second.end()); + if (!key) return {}; ++in; - constructed.push_back(MakeNodeRef(Fragment::PK_K, Vector(std::move(key)))); + constructed.push_back(MakeNodeRef(Fragment::PK_K, Vector(std::move(*key)))); break; } if (last - in >= 5 && in[0].first == OP_VERIFY && in[1].first == OP_EQUAL && in[3].first == OP_HASH160 && in[4].first == OP_DUP && in[2].second.size() == 20) { - Key key; - if (!ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end(), key)) return {}; + auto key = ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end()); + if (!key) return {}; in += 5; - constructed.push_back(MakeNodeRef(Fragment::PK_H, Vector(std::move(key)))); + constructed.push_back(MakeNodeRef(Fragment::PK_H, Vector(std::move(*key)))); break; } // Time locks - if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && ParseScriptNumber(in[1], k)) { + std::optional num; + if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && (num = ParseScriptNumber(in[1]))) { in += 2; - if (k < 1 || k > 0x7FFFFFFFL) return {}; - constructed.push_back(MakeNodeRef(Fragment::OLDER, k)); + if (*num < 1 || *num > 0x7FFFFFFFL) return {}; + constructed.push_back(MakeNodeRef(Fragment::OLDER, *num)); break; } - if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && ParseScriptNumber(in[1], k)) { + if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) { in += 2; - if (k < 1 || k > 0x7FFFFFFFL) return {}; - constructed.push_back(MakeNodeRef(Fragment::AFTER, k)); + if (num < 1 || num > 0x7FFFFFFFL) return {}; + constructed.push_back(MakeNodeRef(Fragment::AFTER, *num)); break; } // Hashes - if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && ParseScriptNumber(in[5], k) && k == 32 && in[6].first == OP_SIZE) { + if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && (num = ParseScriptNumber(in[5])) && num == 32 && in[6].first == OP_SIZE) { if (in[2].first == OP_SHA256 && in[1].second.size() == 32) { constructed.push_back(MakeNodeRef(Fragment::SHA256, in[1].second)); in += 7; @@ -1366,20 +1365,20 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) // Multi if (last - in >= 3 && in[0].first == OP_CHECKMULTISIG) { std::vector keys; - if (!ParseScriptNumber(in[1], n)) return {}; - if (last - in < 3 + n) return {}; - if (n < 1 || n > 20) return {}; - for (int i = 0; i < n; ++i) { - Key key; + const auto n = ParseScriptNumber(in[1]); + if (!n || last - in < 3 + *n) return {}; + if (*n < 1 || *n > 20) return {}; + for (int i = 0; i < *n; ++i) { if (in[2 + i].second.size() != 33) return {}; - if (!ctx.FromPKBytes(in[2 + i].second.begin(), in[2 + i].second.end(), key)) return {}; - keys.push_back(std::move(key)); + auto key = ctx.FromPKBytes(in[2 + i].second.begin(), in[2 + i].second.end()); + if (!key) return {}; + keys.push_back(std::move(*key)); } - if (!ParseScriptNumber(in[2 + n], k)) return {}; - if (k < 1 || k > n) return {}; - in += 3 + n; + const auto k = ParseScriptNumber(in[2 + *n]); + if (!k || *k < 1 || *k > *n) return {}; + in += 3 + *n; std::reverse(keys.begin(), keys.end()); - constructed.push_back(MakeNodeRef(Fragment::MULTI, std::move(keys), k)); + constructed.push_back(MakeNodeRef(Fragment::MULTI, std::move(keys), *k)); break; } /** In the following wrappers, we only need to push SINGLE_BKV_EXPR rather @@ -1407,10 +1406,10 @@ inline NodeRef DecodeScript(I& in, I last, const Ctx& ctx) break; } // Thresh - if (last - in >= 3 && in[0].first == OP_EQUAL && ParseScriptNumber(in[1], k)) { - if (k < 1) return {}; + if (last - in >= 3 && in[0].first == OP_EQUAL && (num = ParseScriptNumber(in[1]))) { + if (*num < 1) return {}; in += 2; - to_parse.emplace_back(DecodeContext::THRESH_W, 0, k); + to_parse.emplace_back(DecodeContext::THRESH_W, 0, *num); break; } // OP_ENDIF can be WRAP_J, WRAP_D, ANDOR, OR_C, OR_D, or OR_I @@ -1645,12 +1644,12 @@ inline NodeRef FromString(const std::string& str, const Ctx& template inline NodeRef FromScript(const CScript& script, const Ctx& ctx) { using namespace internal; - std::vector>> decomposed; - if (!DecomposeScript(script, decomposed)) return {}; - auto it = decomposed.begin(); - auto ret = DecodeScript(it, decomposed.end(), ctx); + auto decomposed = DecomposeScript(script); + if (!decomposed) return {}; + auto it = decomposed->begin(); + auto ret = DecodeScript(it, decomposed->end(), ctx); if (!ret) return {}; - if (it != decomposed.end()) return {}; + if (it != decomposed->end()) return {}; return ret; } diff --git a/src/test/fuzz/miniscript_decode.cpp b/src/test/fuzz/miniscript_decode.cpp index 4cc0a1be8f..1e2378cb17 100644 --- a/src/test/fuzz/miniscript_decode.cpp +++ b/src/test/fuzz/miniscript_decode.cpp @@ -21,9 +21,8 @@ using miniscript::operator""_mst; struct Converter { typedef CPubKey Key; - bool ToString(const Key& key, std::string& ret) const { - ret = HexStr(key); - return true; + std::optional ToString(const Key& key) const { + return HexStr(key); } const std::vector ToPKBytes(const Key& key) const { return {key.begin(), key.end()}; @@ -34,20 +33,21 @@ struct Converter { } template - bool FromString(I first, I last, Key& key) const { + std::optional FromString(I first, I last) const { const auto bytes = ParseHex(std::string(first, last)); - key.Set(bytes.begin(), bytes.end()); - return key.IsValid(); + Key key{bytes.begin(), bytes.end()}; + if (key.IsValid()) return key; + return {}; } template - bool FromPKBytes(I first, I last, CPubKey& key) const { - key.Set(first, last); - return key.IsValid(); + std::optional FromPKBytes(I first, I last) const { + Key key{first, last}; + if (key.IsValid()) return key; + return {}; } template - bool FromPKHBytes(I first, I last, CPubKey& key) const { - assert(last - first == 20); - return false; + std::optional FromPKHBytes(I first, I last) const { + return {}; } }; @@ -63,8 +63,7 @@ FUZZ_TARGET(miniscript_decode) if (!ms) return; // We can roundtrip it to its string representation. - std::string ms_str; - assert(ms->ToString(CONVERTER, ms_str)); + std::string ms_str = *ms->ToString(CONVERTER); assert(*miniscript::FromString(ms_str, CONVERTER) == *ms); // The Script representation must roundtrip since we parsed it this way the first time. const CScript ms_script = ms->ToScript(CONVERTER); diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 930582ea24..212525537a 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -84,27 +84,28 @@ struct KeyConverter { //! Parse a public key from a range of hex characters. template - bool FromString(I first, I last, CPubKey& key) const { + std::optional FromString(I first, I last) const { auto bytes = ParseHex(std::string(first, last)); - key.Set(bytes.begin(), bytes.end()); - return key.IsValid(); + Key key{bytes.begin(), bytes.end()}; + if (key.IsValid()) return key; + return {}; } template - bool FromPKBytes(I first, I last, CPubKey& key) const { - key.Set(first, last); - return key.IsValid(); + std::optional FromPKBytes(I first, I last) const { + Key key{first, last}; + if (key.IsValid()) return key; + return {}; } template - bool FromPKHBytes(I first, I last, CPubKey& key) const { + std::optional FromPKHBytes(I first, I last) const { assert(last - first == 20); CKeyID keyid; std::copy(first, last, keyid.begin()); auto it = g_testdata->pkmap.find(keyid); assert(it != g_testdata->pkmap.end()); - key = it->second; - return true; + return it->second; } }; From a0f064dc1474a048e236bfff12f4def3aa11daf3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 6 Dec 2021 13:32:09 +0100 Subject: [PATCH 016/111] miniscript: introduce a CheckTimeLocksMix helper This helps to have finer-grained descriptor parsing errors. --- src/script/miniscript.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 20aeb3c01b..54eda4b286 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -776,8 +776,11 @@ struct Node { //! Check whether this script always needs a signature. bool NeedsSignature() const { return GetType() << "s"_mst; } + //! Check whether there is no satisfaction path that contains both timelocks and heightlocks + bool CheckTimeLocksMix() const { return GetType() << "k"_mst; } + //! Do all sanity checks. - bool IsSane() const { return IsValid() && GetType() << "mk"_mst && CheckOpsLimit() && CheckStackSize(); } + bool IsSane() const { return IsValid() && IsNonMalleable() && CheckTimeLocksMix() && CheckOpsLimit() && CheckStackSize(); } //! Check whether this node is safe as a script on its own. bool IsSaneTopLevel() const { return IsValidTopLevel() && IsSane() && NeedsSignature(); } From 5cea85f12cba5dcfe3a298eddfa711f582adffac Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 12 Apr 2022 19:26:12 +0200 Subject: [PATCH 017/111] miniscript: split ValidSatisfactions from IsSane This makes IsSane clearer. It is useful to differentiate between 'potential non-malleable satisfactions are valid' and 'such satisfactions exist' for testing. Co-authored-by: Pieter Wuille --- src/script/miniscript.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 54eda4b286..4774308920 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -779,8 +779,11 @@ struct Node { //! Check whether there is no satisfaction path that contains both timelocks and heightlocks bool CheckTimeLocksMix() const { return GetType() << "k"_mst; } - //! Do all sanity checks. - bool IsSane() const { return IsValid() && IsNonMalleable() && CheckTimeLocksMix() && CheckOpsLimit() && CheckStackSize(); } + //! Whether successful non-malleable satisfactions are guaranteed to be valid. + bool ValidSatisfactions() const { return IsValid() && CheckOpsLimit() && CheckStackSize(); } + + //! Whether the apparent policy of this node matches its script semantics. + bool IsSane() const { return ValidSatisfactions() && IsNonMalleable() && CheckTimeLocksMix(); } //! Check whether this node is safe as a script on its own. bool IsSaneTopLevel() const { return IsValidTopLevel() && IsSane() && NeedsSignature(); } From 7eb70f0ac0a54adabc566e2b93bbf6b2beb54a79 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 12 Apr 2022 19:28:12 +0200 Subject: [PATCH 018/111] miniscript: tiny doc fixups Co-authored-by: Pieter Wuille --- src/script/miniscript.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 4774308920..6b306b21db 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -40,7 +40,7 @@ namespace miniscript { * - For example: older(n) = OP_CHECKSEQUENCEVERIFY. * - "V" Verify: * - Takes its inputs from the top of the stack. - * - When satisfactied, pushes nothing. + * - When satisfied, pushes nothing. * - Cannot be dissatisfied. * - This can be obtained by adding an OP_VERIFY to a B, modifying the last opcode * of a B to its -VERIFY version (only for OP_CHECKSIG, OP_CHECKSIGVERIFY @@ -329,6 +329,8 @@ struct Node { * computes the result of the node. If std::nullopt is returned by upfn, * TreeEvalMaybe() immediately returns std::nullopt. * The return value of TreeEvalMaybe is the result of the root node. + * + * Result type cannot be bool due to the std::vector specialization. */ template std::optional TreeEvalMaybe(State root_state, DownFn downfn, UpFn upfn) const @@ -858,7 +860,7 @@ enum class ParseContext { int FindNextChar(Span in, const char m); -/** Parse a key string ending with a ')' or ','. */ +/** Parse a key string ending at the end of the fragment's text representation. */ template std::optional> ParseKeyEnd(Span in, const Ctx& ctx) { From be34d5077b2fede7404de7706362f5858c443525 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 13 Apr 2022 14:11:59 +0200 Subject: [PATCH 019/111] fuzz: rename and improve the Miniscript Script roundtrip target Parse also key hashes using the Key type. Make this target the first of the 4 Miniscript fuzz targets in a single `miniscript` file. Co-authored-by: Pieter Wuille --- src/Makefile.test.include | 2 +- src/test/fuzz/miniscript.cpp | 70 ++++++++++++++++++++++++++++ src/test/fuzz/miniscript_decode.cpp | 71 ----------------------------- 3 files changed, 71 insertions(+), 72 deletions(-) create mode 100644 src/test/fuzz/miniscript.cpp delete mode 100644 src/test/fuzz/miniscript_decode.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 02a3f9ae7d..781313e42d 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -269,7 +269,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/locale.cpp \ test/fuzz/merkleblock.cpp \ test/fuzz/message.cpp \ - test/fuzz/miniscript_decode.cpp \ + test/fuzz/miniscript.cpp \ test/fuzz/minisketch.cpp \ test/fuzz/muhash.cpp \ test/fuzz/multiplication_overflow.cpp \ diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp new file mode 100644 index 0000000000..0788f37e55 --- /dev/null +++ b/src/test/fuzz/miniscript.cpp @@ -0,0 +1,70 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include