From 1c4b9cbe906507295d8b7d52855de1441ad411dd Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Mon, 7 Aug 2023 10:15:26 -0400 Subject: [PATCH 1/8] bench: add readblock benchmark --- src/Makefile.bench.include | 1 + src/bench/readblock.cpp | 53 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 src/bench/readblock.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 28b779a5a88e6..27046b3c11d6f 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -46,6 +46,7 @@ bench_bench_bitcoin_SOURCES = \ bench/poly1305.cpp \ bench/pool.cpp \ bench/prevector.cpp \ + bench/readblock.cpp \ bench/rollingbloom.cpp \ bench/rpc_blockchain.cpp \ bench/rpc_mempool.cpp \ diff --git a/src/bench/readblock.cpp b/src/bench/readblock.cpp new file mode 100644 index 0000000000000..0545c6b0170fa --- /dev/null +++ b/src/bench/readblock.cpp @@ -0,0 +1,53 @@ +// Copyright (c) 2023 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 +#include +#include +#include +#include + +static FlatFilePos WriteBlockToDisk(ChainstateManager& chainman) +{ + DataStream stream{benchmark::data::block413567}; + CBlock block; + stream >> TX_WITH_WITNESS(block); + + return chainman.m_blockman.SaveBlockToDisk(block, 0, nullptr); +} + +static void ReadBlockFromDiskTest(benchmark::Bench& bench) +{ + const auto testing_setup{MakeNoLogFileContext(ChainType::MAIN)}; + ChainstateManager& chainman{*testing_setup->m_node.chainman}; + + CBlock block; + const auto pos{WriteBlockToDisk(chainman)}; + + bench.run([&] { + const auto success{chainman.m_blockman.ReadBlockFromDisk(block, pos)}; + assert(success); + }); +} + +static void ReadRawBlockFromDiskTest(benchmark::Bench& bench) +{ + const auto testing_setup{MakeNoLogFileContext(ChainType::MAIN)}; + ChainstateManager& chainman{*testing_setup->m_node.chainman}; + + std::vector block_data; + const auto pos{WriteBlockToDisk(chainman)}; + + bench.run([&] { + const auto success{chainman.m_blockman.ReadRawBlockFromDisk(block_data, pos)}; + assert(success); + }); +} + +BENCHMARK(ReadBlockFromDiskTest, benchmark::PriorityLevel::HIGH); +BENCHMARK(ReadRawBlockFromDiskTest, benchmark::PriorityLevel::HIGH); From 562664d26374331d291b97e2e2f7fca1f0fd467b Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 6 Dec 2023 13:55:38 +0100 Subject: [PATCH 2/8] test: wait for fee estimator to catch up before estimating fees --- src/test/policyestimator_tests.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 13ec89663ae11..75cdfb90dc46a 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -112,6 +112,9 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) } } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + std::vector origFeeEst; // Highest feerate is 10*baseRate and gets in all blocks, // second highest feerate is 9*baseRate and gets in 9/10 blocks = 90%, From e03d6f7ed534f423f58236866f8e83beee1871e1 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Wed, 13 Dec 2023 10:02:34 -0300 Subject: [PATCH 3/8] fuzz: set `m_fallback_fee`/`m_fee_mode` in `wallet_fees` target --- src/wallet/test/fuzz/fees.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/wallet/test/fuzz/fees.cpp b/src/wallet/test/fuzz/fees.cpp index 2f7892dc0ac9c..c2e785651ad4a 100644 --- a/src/wallet/test/fuzz/fees.cpp +++ b/src/wallet/test/fuzz/fees.cpp @@ -36,6 +36,10 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup) wallet.SetLastBlockProcessed(chainstate->m_chain.Height(), chainstate->m_chain.Tip()->GetBlockHash()); } + if (fuzzed_data_provider.ConsumeBool()) { + wallet.m_fallback_fee = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; + } + if (fuzzed_data_provider.ConsumeBool()) { wallet.m_discard_rate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; } @@ -58,6 +62,9 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup) if (fuzzed_data_provider.ConsumeBool()) { coin_control.m_confirm_target = fuzzed_data_provider.ConsumeIntegralInRange(0, 999'000); } + if (fuzzed_data_provider.ConsumeBool()) { + coin_control.m_fee_mode = fuzzed_data_provider.ConsumeBool() ? FeeEstimateMode::CONSERVATIVE : FeeEstimateMode::ECONOMICAL; + } FeeCalculation fee_calculation; FeeCalculation* maybe_fee_calculation{fuzzed_data_provider.ConsumeBool() ? nullptr : &fee_calculation}; From 91dc48c14825a9075a57c1eefda202b83b6346ba Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 Nov 2023 15:33:50 -0500 Subject: [PATCH 4/8] doc: Add multiprocess design doc Also split up existing multiprocess documentation into design and usage sections --- doc/design/multiprocess.md | 310 ++++++++++++++++++++++++++++++------- doc/multiprocess.md | 34 ++++ 2 files changed, 285 insertions(+), 59 deletions(-) create mode 100644 doc/multiprocess.md diff --git a/doc/design/multiprocess.md b/doc/design/multiprocess.md index 45681d12dea49..636d78d905c45 100644 --- a/doc/design/multiprocess.md +++ b/doc/design/multiprocess.md @@ -1,72 +1,264 @@ -# Multiprocess Bitcoin +# Multiprocess Bitcoin Design Document -On unix systems, the `--enable-multiprocess` build option can be passed to `./configure` to build new `bitcoin-node`, `bitcoin-wallet`, and `bitcoin-gui` executables alongside existing `bitcoind` and `bitcoin-qt` executables. +Guide to the design and architecture of the Bitcoin Core multiprocess feature -`bitcoin-node` is a drop-in replacement for `bitcoind`, and `bitcoin-gui` is a drop-in replacement for `bitcoin-qt`, and there are no differences in use or external behavior between the new and old executables. But internally (after [#10102](https://github.com/bitcoin/bitcoin/pull/10102)), `bitcoin-gui` will spawn a `bitcoin-node` process to run P2P and RPC code, communicating with it across a socket pair, and `bitcoin-node` will spawn `bitcoin-wallet` to run wallet code, also communicating over a socket pair. This will let node, wallet, and GUI code run in separate address spaces for better isolation, and allow future improvements like being able to start and stop components independently on different machines and environments. +_This document describes the design of the multiprocess feature. For usage information, see the top-level [multiprocess.md](../multiprocess.md) file._ -## Next steps +## Table of contents -Specific next steps after [#10102](https://github.com/bitcoin/bitcoin/pull/10102) will be: +- [Introduction](#introduction) +- [Current Architecture](#current-architecture) +- [Proposed Architecture](#proposed-architecture) +- [Component Overview: Navigating the IPC Framework](#component-overview-navigating-the-ipc-framework) +- [Design Considerations](#design-considerations) + - [Selection of Cap’n Proto](#selection-of-capn-proto) + - [Hiding IPC](#hiding-ipc) + - [Interface Definition Maintenance](#interface-definition-maintenance) + - [Interface Stability](#interface-stability) +- [Security Considerations](#security-considerations) +- [Example Use Cases and Flows](#example-use-cases-and-flows) + - [Retrieving a Block Hash](#retrieving-a-block-hash) +- [Future Enhancements](#future-enhancements) +- [Conclusion](#conclusion) +- [Appendices](#appendices) + - [Glossary of Terms](#glossary-of-terms) + - [References](#references) +- [Acknowledgements](#acknowledgements) -- [ ] Adding `-ipcbind` and `-ipcconnect` options to `bitcoin-node`, `bitcoin-wallet`, and `bitcoin-gui` executables so they can listen and connect to TCP ports and unix socket paths. This will allow separate processes to be started and stopped any time and connect to each other. -- [ ] Adding `-server` and `-rpcbind` options to the `bitcoin-wallet` executable so wallet processes can handle RPC requests directly without going through the node. -- [ ] Supporting windows, not just unix systems. The existing socket code is already cross-platform, so the only windows-specific code that needs to be written is code spawning a process and passing a socket descriptor. This can be implemented with `CreateProcess` and `WSADuplicateSocket`. Example: https://memset.wordpress.com/2010/10/13/win32-api-passing-socket-with-ipc-method/. -- [ ] Adding sandbox features, restricting subprocess access to resources and data. See [https://eklitzke.org/multiprocess-bitcoin](https://eklitzke.org/multiprocess-bitcoin). +## Introduction -## Debugging +The Bitcoin Core software has historically employed a monolithic architecture. The existing design has integrated functionality like P2P network operations, wallet management, and a GUI into a single executable. While effective, it has limitations in flexibility, security, and scalability. This project introduces changes that transition Bitcoin Core to a more modular architecture. It aims to enhance security, improve usability, and facilitate maintenance and development of the software in the long run. -The `-debug=ipc` command line option can be used to see requests and responses between processes. +## Current Architecture -## Installation +The current system features two primary executables: `bitcoind` and `bitcoin-qt`. `bitcoind` combines a Bitcoin P2P node with an integrated JSON-RPC server, wallet, and indexes. `bitcoin-qt` extends this by incorporating a Qt-based GUI. This monolithic structure, although robust, presents challenges such as limited operational flexibility and increased security risks due to the tight integration of components. -The multiprocess feature requires [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) as dependencies. A simple way to get starting using it without installing these dependencies manually is to use the [depends system](../../depends) with the `MULTIPROCESS=1` [dependency option](../../depends#dependency-options) passed to make: +## Proposed Architecture +The new architecture divides the existing code into three specialized executables: + +- `bitcoin-node`: Manages the P2P node, indexes, and JSON-RPC server. +- `bitcoin-wallet`: Handles all wallet functionality. +- `bitcoin-gui`: Provides a standalone Qt-based GUI. + +This modular approach is designed to enhance security through component isolation and improve usability by allowing independent operation of each module. This allows for new use-cases, such as running the node on a dedicated machine and operating wallets and GUIs on separate machines with the flexibility to start and stop them as needed. + +This subdivision could be extended in the future. For example, indexes could be removed from the `bitcoin-node` executable and run in separate executables. And JSON-RPC servers could be added to wallet and index executables, so they can listen and respond to RPC requests on their own ports, without needing to forward RPC requests through `bitcoin-node`. + +
+ +```mermaid +flowchart LR + node[bitcoin-node] -- listens on --> socket["<datadir>/node.sock"] + wallet[bitcoin-wallet] -- connects to --> socket + gui[bitcoin-gui] -- connects to --> socket +``` + +
+Processes and socket connection. +
+ +## Component Overview: Navigating the IPC Framework + +This section describes the major components of the Inter-Process Communication (IPC) framework covering the relevant source files, generated files, tools, and libraries. + +### Abstract C++ Classes in [`src/interfaces/`](../../src/interfaces/) +- The foundation of the IPC implementation lies in the abstract C++ classes within the [`src/interfaces/`](../../src/interfaces/) directory. These classes define pure virtual methods that code in [`src/node/`](../../src/node/), [`src/wallet/`](../../src/wallet/), and [`src/qt/`](../../src/qt/) directories call to interact with each other. +- Each abstract class in this directory represents a distinct interface that the different modules (node, wallet, GUI) implement and use for cross-process communication. +- The classes are written following conventions described in [Internal Interface + Guidelines](../developer-notes.md#internal-interface-guidelines) to ensure + compatibility with Cap'n Proto. + +### Cap’n Proto Files in [`src/ipc/capnp/`](../../src/ipc/capnp/) +- Corresponding to each abstract class, there are `.capnp` files within the [`src/ipc/capnp/`](../../src/ipc/capnp/) directory. These files are used as input to the `mpgen` tool (described below) to generate C++ code. +- These Cap’n Proto files ([learn more about Cap'n Proto RPC](https://capnproto.org/rpc.html)) define the structure and format of messages that are exchanged over IPC. They serve as blueprints for generating C++ code that bridges the gap between high-level C++ interfaces and low-level socket communication. + +### The `mpgen` Code Generation Tool +- A central component of the IPC framework is the `mpgen` tool which is part the [`libmultiprocess` project](https://github.com/chaincodelabs/libmultiprocess). This tool takes the `.capnp` files as input and generates C++ code. +- The generated code handles IPC communication, translating interface calls into socket reads and writes. + +### C++ Client Subclasses in Generated Code +- In the generated code, we have C++ client subclasses that inherit from the abstract classes in [`src/interfaces/`](../../src/interfaces/). These subclasses are the workhorses of the IPC mechanism. +- They implement all the methods of the interface, marshalling arguments into a structured format, sending them as requests to the IPC server via a UNIX socket, and handling the responses. +- These subclasses effectively mask the complexity of IPC, presenting a familiar C++ interface to developers. +- Internally, the client subclasses generated by the `mpgen` tool wrap [client classes generated by Cap'n Proto](https://capnproto.org/cxxrpc.html#clients), and use them to send IPC requests. + +### C++ Server Classes in Generated Code +- On the server side, corresponding generated C++ classes receive IPC requests. These server classes are responsible for unmarshalling method arguments, invoking the corresponding methods in the local [`src/interfaces/`](../../src/interfaces/) objects, and creating the IPC response. +- The server classes ensure that return values (including output argument values and thrown exceptions) are marshalled and sent back to the client, completing the communication cycle. +- Internally, the server subclasses generated by the `mpgen` tool inherit from [server classes generated by Cap'n Proto](https://capnproto.org/cxxrpc.html#servers), and use them to process IPC requests. + +### The `libmultiprocess` Runtime Library +- **Core Functionality**: The `libmultiprocess` runtime library's primary function is to instantiate the generated client and server classes as needed. +- **Bootstrapping IPC Connections**: It provides functions for starting new IPC connections, specifically binding generated client and server classes for an initial `interfaces::Init` interface (defined in [`src/interfaces/init.h`](../../src/interfaces/init.h)) to a UNIX socket. This initial interface has methods returning other interfaces that different Bitcoin Core modules use to communicate after the bootstrapping phase. +- **Asynchronous I/O and Thread Management**: The library is also responsible for managing I/O and threading. Particularly, it ensures that IPC requests never block each other and that new threads on either side of a connection can always make client calls. It also manages worker threads on the server side of calls, ensuring that calls from the same client thread always execute on the same server thread (to avoid locking issues and support nested callbacks). + +### Type Hooks in [`src/ipc/capnp/*-types.h`](../../src/ipc/capnp/) +- **Custom Type Conversions**: In [`src/ipc/capnp/*-types.h`](../../src/ipc/capnp/), function overloads of two `libmultiprocess` C++ functions, `mp::CustomReadField` and `mp::CustomBuildFields`, are defined. These overloads are used for customizing the conversion of specific C++ types to and from Cap’n Proto types. +- **Handling Special Cases**: The `mpgen` tool and `libmultiprocess` library can convert most C++ types to and from Cap’n Proto types automatically, including interface types, primitive C++ types, standard C++ types like `std::vector`, `std::set`, `std::map`, `std::tuple`, and `std::function`, as well as simple C++ structs that consist of aforementioned types and whose fields correspond 1:1 with Cap’n Proto struct fields. For other types, `*-types.h` files provide custom code to convert between C++ and Cap’n Proto data representations. + +### Protocol-Agnostic IPC Code in [`src/ipc/`](../../src/ipc/) +- **Broad Applicability**: Unlike the Cap’n Proto-specific code in [`src/ipc/capnp/`](../../src/ipc/capnp/), the code in the [`src/ipc/`](../../src/ipc/) directory is protocol-agnostic. This enables potential support for other protocols, such as gRPC or a custom protocol in the future. +- **Process Management and Socket Operations**: The main purpose of this component is to provide functions for spawning new processes and creating and connecting to UNIX sockets. +- **ipc::Exception Class**: This code also defines an `ipc::Exception` class which is thrown from the generated C++ client class methods when there is an unexpected IPC error, such as a disconnection. + +
+ +```mermaid +flowchart TD + capnpFile[ipc/capnp/chain.capnp] -->|Input to| mpgenTool([mpgen Tool]) + mpgenTool -->|Generates| proxyTypesH[ipc/capnp/chain.capnp.proxy-types.h] + mpgenTool --> proxyClientCpp[ipc/capnp/chain.capnp.proxy-client.c++] + mpgenTool --> proxyServerCpp[ipc/capnp/chain.capnp.proxy-server.c++] + proxyTypesH -.->|Includes| interfaces/chain.h + proxyClientCpp -.-> interfaces/chain.h + proxyServerCpp -.-> interfaces/chain.h ``` -cd -make -C depends NO_QT=1 MULTIPROCESS=1 -CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure -make -src/bitcoin-node -regtest -printtoconsole -debug=ipc -BITCOIND=bitcoin-node test/functional/test_runner.py + +
+Diagram showing generated source files and includes. +
+ +## Design Considerations + +### Selection of Cap’n Proto +The choice to use [Cap’n Proto](https://capnproto.org/) for IPC was primarily influenced by its support for passing object references and managing object lifetimes, which would have to be implemented manually with a framework that only supported plain requests and responses like [gRPC](https://grpc.io/). The support is especially helpful for passing callback objects like `std::function` and enabling bidirectional calls between processes. + +The choice to use an RPC framework at all instead of a custom protocol was necessitated by the size of Bitcoin Core internal interfaces which consist of around 150 methods that pass complex data structures and are called in complicated ways (in parallel, and from callbacks that can be nested and stored). Writing a custom protocol to wrap these complicated interfaces would be a lot more work, akin to writing a new RPC framework. + +### Hiding IPC + +The IPC mechanism is deliberately isolated from the rest of the codebase so less code has to be concerned with IPC. + +Building Bitcoin Core with IPC support is optional, and node, wallet, and GUI code can be compiled to either run in the same process or separate processes. The build system also ensures Cap’n Proto library headers can only be used within the [`src/ipc/capnp/`](../../src/ipc/capnp/) directory, not in other parts of the codebase. + +The libmultiprocess runtime is designed to place as few constraints as possible on IPC interfaces and to make IPC calls act like normal function calls. Method arguments, return values, and exceptions are automatically serialized and sent between processes. Object references and `std::function` arguments are tracked to allow invoked code to call back into invoking code at any time. And there is a 1:1 threading model where every client thread has a corresponding server thread responsible for executing incoming calls from that thread (there can be multiple calls from the same thread due to callbacks) without blocking, and holding the same thread-local variables and locks so behavior is the same whether IPC is used or not. + +### Interface Definition Maintenance + +The choice to maintain interface definitions and C++ type mappings as `.capnp` files in the [`src/ipc/capnp/`](../../src/ipc/capnp/) was mostly done for convenience, and probably something that could be improved in the future. + +In the current design, class names, method names, and parameter names are duplicated between C++ interfaces in [`src/interfaces/`](../../src/interfaces/) and Cap’n Proto files in [`src/ipc/capnp/`](../../src/ipc/capnp/). While this keeps C++ interface headers simple and free of references to IPC, it is a maintenance burden because it means inconsistencies between C++ declarations and Cap’n Proto declarations will result in compile errors. (Static type checking ensures these are not runtime errors.) + +An alternate approach could use custom [C++ Attributes](https://en.cppreference.com/w/cpp/language/attributes) embedded in interface declarations to automatically generate `.capnp` files from C++ headers. This has not been pursued because parsing C++ headers is more complicated than parsing Cap’n Proto interface definitions, especially portably on multiple platforms. + +In the meantime, the developer guide [Internal interface guidelines](developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors. + +### Interface Stability + +The currently defined IPC interfaces are unstable, and can change freely with no backwards compatibility. The decision to allow this stems from the recognition that our current interfaces are still evolving and not yet ideal for external use. As these interfaces mature and become more refined, there may be an opportunity to declare them stable and use Cap’n Proto's support for protocol evolution ([Cap'n Proto - Evolving Your Protocol](https://capnproto.org/language.html#evolving-your-protocol)) to allow them to be extended while remaining backwards compatible. This could allow different versions of node, GUI, and wallet binaries to interoperate, and potentially open doors for external tools to utilize these interfaces, such as creating custom indexes through a stable indexing interface. However, for now, the priority is to improve the interfaces internally. Given their current state and the advantages of using JSON-RPC for most common tasks, it's more practical to focus on internal development rather than external applicability. + +## Security Considerations + +The integration of [Cap’n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) into the Bitcoin Core architecture increases its potential attack surface. Cap’n Proto, being a complex and substantial new dependency, introduces potential sources of vulnerability, particularly through the creation of new UNIX sockets. The inclusion of libmultiprocess, while a smaller external dependency, also contributes to this risk. However, plans are underway to incorporate libmultiprocess as a git subtree, aligning it more closely with the project's well-reviewed internal libraries. While adopting these multiprocess features does introduce some risk, it's worth noting that they can be disabled, allowing builds without these new dependencies. This flexibility ensures that users can balance functionality with security considerations as needed. + +## Example Use Cases and Flows + +### Retrieving a Block Hash + +Let’s walk through an example where the `bitcoin-wallet` process requests the hash of a block at a specific height from the `bitcoin-node` process. This example demonstrates the practical application of the IPC mechanism, specifically the interplay between C++ method calls and Cap’n Proto-generated RPC calls. + +
+ +```mermaid +sequenceDiagram + box "bitcoin-wallet process" + participant WalletCode as Wallet code + participant ChainClient as Generated Chain client class
ProxyClient + end + box "bitcoin-node process" + participant ChainServer as Generated Chain server class
ProxyServer + participant LocalChain as Chain object
node::ChainImpl + end + + WalletCode->>ChainClient: getBlockHash(height) + ChainClient->>ChainServer: Send RPC getBlockHash request + ChainServer->>LocalChain: getBlockHash(height) + LocalChain->>ChainServer: Return block hash + ChainServer->>ChainClient: Send response with block hash + ChainClient->>WalletCode: Return block hash ``` -The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option). - -Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `./configure --enable-multiprocess` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess/blob/master/doc/install.md) section of the libmultiprocess readme for install steps. See [build-unix.md](../build-unix.md) and [build-osx.md](../build-osx.md) for information about installing dependencies in general. - -## IPC implementation details - -Cross process Node, Wallet, and Chain interfaces are defined in -[`src/interfaces/`](../../src/interfaces/). These are C++ classes which follow -[conventions](../developer-notes.md#internal-interface-guidelines), like passing -serializable arguments so they can be called from different processes, and -making methods pure virtual so they can have proxy implementations that forward -calls between processes. - -When Wallet, Node, and Chain code is running in the same process, calling any -interface method invokes the implementation directly. When code is running in -different processes, calling an interface method invokes a proxy interface -implementation that communicates with a remote process and invokes the real -implementation in the remote process. The -[libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) code -generation tool internally generates proxy client classes and proxy server -classes for this purpose that are thin wrappers around Cap'n Proto -[client](https://capnproto.org/cxxrpc.html#clients) and -[server](https://capnproto.org/cxxrpc.html#servers) classes, which handle the -actual serialization and socket communication. - -As much as possible, calls between processes are meant to work the same as -calls within a single process without adding limitations or requiring extra -implementation effort. Processes communicate with each other by calling regular -[C++ interface methods](../../src/interfaces/README.md). Method arguments and -return values are automatically serialized and sent between processes. Object -references and `std::function` arguments are automatically tracked and mapped -to allow invoked code to call back into invoking code at any time, and there is -a 1:1 threading model where any thread invoking a method in another process has -a corresponding thread in the invoked process responsible for executing all -method calls from the source thread, without blocking I/O or holding up another -call, and using the same thread local variables, locks, and callbacks between -calls. The forwarding, tracking, and threading is implemented inside the -[libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library -which has the design goal of making calls between processes look like calls in -the same process to the extent possible. +
+Chain::getBlockHash call diagram +
+ +1. **Initiation in bitcoin-wallet** + - The wallet process calls the `getBlockHash` method on a `Chain` object. This method is defined as a virtual method in [`src/interfaces/chain.h`](../../src/interfaces/chain.h). + +2. **Translation to Cap’n Proto RPC** + - The `Chain::getBlockHash` virtual method is overridden by the `Chain` [client subclass](#c-client-subclasses-in-generated-code) to translate the method call into a Cap’n Proto RPC call. + - The client subclass is automatically generated by the `mpgen` tool from the [`chain.capnp`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc/src/ipc/capnp/chain.capnp) file in [`src/ipc/capnp/`](../../src/ipc/capnp/). + +3. **Request Preparation and Dispatch** + - The `getBlockHash` method of the generated `Chain` client subclass in `bitcoin-wallet` populates a Cap’n Proto request with the `height` parameter, sends it to `bitcoin-node` process, and waits for a response. + +4. **Handling in bitcoin-node** + - Upon receiving the request, the Cap'n Proto dispatching code in the `bitcoin-node` process calls the `getBlockHash` method of the `Chain` [server class](#c-server-classes-in-generated-code). + - The server class is automatically generated by the `mpgen` tool from the [`chain.capnp`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc/src/ipc/capnp/chain.capnp) file in [`src/ipc/capnp/`](../../src/ipc/capnp/). + - The `getBlockHash` method of the generated `Chain` server subclass in `bitcoin-wallet` receives a Cap’n Proto request object with the `height` parameter, and calls the `getBlockHash` method on its local `Chain` object with the provided `height`. + - When the call returns, it encapsulates the return value in a Cap’n Proto response, which it sends back to the `bitcoin-wallet` process, + +5. **Response and Return** + - The `getBlockHash` method of the generated `Chain` client subclass in `bitcoin-wallet` which sent the request now receives the response. + - It extracts the block hash value from the response, and returns it to the original caller. + +## Future Enhancements + +Further improvements are possible such as: + +- Separating indexes from `bitcoin-node`, and running indexing code in separate processes (see [indexes: Stop using node internal types #24230](https://github.com/bitcoin/bitcoin/pull/24230)). +- Enabling wallet processes to listen for JSON-RPC requests on their own ports instead of needing the node process to listen and forward requests to them. +- Automatically generating `.capnp` files from C++ interface definitions (see [Interface Definition Maintenance](#interface-definition-maintenance)). +- Simplifying and stabilizing interfaces (see [Interface Stability](#interface-stability)). +- Adding sandbox features, restricting subprocess access to resources and data (see [https://eklitzke.org/multiprocess-bitcoin](https://eklitzke.org/multiprocess-bitcoin)). +- Using Cap'n Proto's support for [other languages](https://capnproto.org/otherlang.html), such as [Rust](https://github.com/capnproto/capnproto-rust), to allow code written in other languages to call Bitcoin Core C++ code, and vice versa (see [How to rustify libmultiprocess? #56](https://github.com/chaincodelabs/libmultiprocess/issues/56)). + +## Conclusion + +This modularization represents an advancement in Bitcoin Core's architecture, offering enhanced security, flexibility, and maintainability. The project invites collaboration and feedback from the community. + +## Appendices + +### Glossary of Terms + +- **abstract class**: A class in C++ that consists of virtual functions. In the Bitcoin Core project, they define interfaces for inter-component communication. + +- **asynchronous I/O**: A form of input/output processing that allows a program to continue other operations while a transmission is in progress. + +- **Cap’n Proto**: A high-performance data serialization and RPC library, chosen for its support for object references and bidirectional communication. + +- **Cap’n Proto interface**: A set of methods defined in Cap’n Proto to facilitate structured communication between different software components. + +- **Cap’n Proto struct**: A structured data format used in Cap’n Proto, similar to structs in C++, for organizing and transporting data across different processes. + +- **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code)) + +- **IPC (inter-process communication)**: Mechanisms that enable processes to exchange requests and data. + +- **ipc::Exception class**: A class within Bitcoin Core's protocol-agnostic IPC code that is thrown by client class methods when there is an IPC error. + +- **libmultiprocess**: A custom library and code generation tool used for creating IPC interfaces and managing IPC connections. + +- **marshalling**: Transforming an object’s memory representation for transmission. + +- **mpgen tool**: A tool within the `libmultiprocess` suite that generates C++ code from Cap’n Proto files, facilitating IPC. + +- **protocol-agnostic code**: Generic IPC code in [`src/ipc/`](../../src/ipc/) that does not rely on Cap’n Proto and could be used with other protocols. Distinct from code in [`src/ipc/capnp/`](../../src/ipc/capnp/) which relies on Cap’n Proto. + +- **RPC (remote procedure call)**: A protocol that enables a program to request a service from another program in a different address space or network. Bitcoin Core uses [JSON-RPC](https://en.wikipedia.org/wiki/JSON-RPC) for RPC. + +- **server class (in generated code)**: A C++ class generated from a Cap’n Proto interface which handles requests sent by a _client class_ in another process. The request handled by calling a local Bitcoin Core interface method, and the return values (if any) are sent back in a response. (see also: [components section](#c-server-classes-in-generated-code)) + +- **unix socket**: Communication endpoint which is a filesystem path, used for exchanging data between processes running on the same host. + +- **virtual method**: A function or method whose behavior can be overridden within an inheriting class by a function with the same signature. + +## References + +- **Cap’n Proto RPC protocol description**: https://capnproto.org/rpc.html +- **libmultiprocess project page**: https://github.com/chaincodelabs/libmultiprocess + +## Acknowledgements + +This design doc was written by @ryanofsky, who is grateful to all the reviewers who gave feedback and tested [multiprocess PRs](https://github.com/bitcoin/bitcoin/pull/28722), and everyone else who's helped with this project. Particular thanks to @ariard who deeply reviewed IPC code and improved the design of the IPC library and initialization process. @jnewbery who championed the early refactoring PRs and helped guide them through development and review. @sjors who has reviewed and repeatedly tested multiprocess code, reporting many issues and helping debug them. @hebasto, @fanquake, and @maflcko who made significant improvements to the build system and fixed countless build issues. @vasild and @jamesob who were brave contributors to the libmultiprocess library. And Chaincode Labs for making this work possible. Also thanks to ChatGPT, who actually wrote most of this document (not @ryanofsky). diff --git a/doc/multiprocess.md b/doc/multiprocess.md new file mode 100644 index 0000000000000..7ba89b3ff56a2 --- /dev/null +++ b/doc/multiprocess.md @@ -0,0 +1,34 @@ +# Multiprocess Bitcoin + +_This document describes usage of the multiprocess feature. For design information, see the [design/multiprocess.md](design/multiprocess.md) file._ + +## Build Option + +On unix systems, the `--enable-multiprocess` build option can be passed to `./configure` to build new `bitcoin-node`, `bitcoin-wallet`, and `bitcoin-gui` executables alongside existing `bitcoind` and `bitcoin-qt` executables. + +## Debugging + +The `-debug=ipc` command line option can be used to see requests and responses between processes. + +## Installation + +The multiprocess feature requires [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) as dependencies. A simple way to get started using it without installing these dependencies manually is to use the [depends system](../depends) with the `MULTIPROCESS=1` [dependency option](../depends#dependency-options) passed to make: + +``` +cd +make -C depends NO_QT=1 MULTIPROCESS=1 +CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure +make +src/bitcoin-node -regtest -printtoconsole -debug=ipc +BITCOIND=bitcoin-node test/functional/test_runner.py +``` + +The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option). + +Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `./configure --enable-multiprocess` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess/blob/master/doc/install.md) section of the libmultiprocess readme for install steps. See [build-unix.md](build-unix.md) and [build-osx.md](build-osx.md) for information about installing dependencies in general. + +## Usage + +`bitcoin-node` is a drop-in replacement for `bitcoind`, and `bitcoin-gui` is a drop-in replacement for `bitcoin-qt`, and there are no differences in use or external behavior between the new and old executables. But internally after [#10102](https://github.com/bitcoin/bitcoin/pull/10102), `bitcoin-gui` will spawn a `bitcoin-node` process to run P2P and RPC code, communicating with it across a socket pair, and `bitcoin-node` will spawn `bitcoin-wallet` to run wallet code, also communicating over a socket pair. This will let node, wallet, and GUI code run in separate address spaces for better isolation, and allow future improvements like being able to start and stop components independently on different machines and environments. +[#19460](https://github.com/bitcoin/bitcoin/pull/19460) also adds a new `bitcoin-node` `-ipcbind` option and an `bitcoind-wallet` `-ipcconnect` option to allow new wallet processes to connect to an existing node process. +And [#19461](https://github.com/bitcoin/bitcoin/pull/19461) adds a new `bitcoin-gui` `-ipcconnect` option to allow new GUI processes to connect to an existing node process. From fa1d49542e4b69a5d8b1177ffe4207f051a468bb Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 12 Sep 2023 03:35:40 +0200 Subject: [PATCH 5/8] refactor: share and use `GenerateRandomKey` helper Making the `GenerateRandomKey` helper available to other modules via key.{h.cpp} allows us to create random private keys directly at instantiation of CKey, in contrast to the two-step process of creating the instance and then having to call `MakeNewKey(...)`. --- src/bench/ellswift.cpp | 4 +--- src/key.cpp | 10 ++++++-- src/key.h | 2 ++ src/net.cpp | 8 +------ src/qt/test/addressbooktests.cpp | 3 +-- src/test/blockfilter_index_tests.cpp | 5 ++-- src/test/compress_tests.cpp | 9 +++---- src/test/script_standard_tests.cpp | 12 ++++------ src/test/script_tests.cpp | 19 +++++++-------- src/test/sigopcount_tests.cpp | 6 ++--- src/test/transaction_tests.cpp | 28 ++++++++++------------ src/test/txpackage_tests.cpp | 36 ++++++++++------------------ src/wallet/scriptpubkeyman.cpp | 3 +-- src/wallet/test/ismine_tests.cpp | 3 +-- src/wallet/test/wallet_tests.cpp | 12 ++++------ src/wallet/wallet.cpp | 3 +-- 16 files changed, 63 insertions(+), 100 deletions(-) diff --git a/src/bench/ellswift.cpp b/src/bench/ellswift.cpp index f0348421b38ee..82e8dec9826fc 100644 --- a/src/bench/ellswift.cpp +++ b/src/bench/ellswift.cpp @@ -11,9 +11,7 @@ static void EllSwiftCreate(benchmark::Bench& bench) { ECC_Start(); - CKey key; - key.MakeNewKey(true); - + CKey key = GenerateRandomKey(); uint256 entropy = GetRandHash(); bench.batch(1).unit("pubkey").run([&] { diff --git a/src/key.cpp b/src/key.cpp index 0f283ca3e3839..512790252af74 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -369,6 +369,13 @@ ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, c return output; } +CKey GenerateRandomKey(bool compressed) noexcept +{ + CKey key; + key.MakeNewKey(/*fCompressed=*/compressed); + return key; +} + bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const { if (nDepth == std::numeric_limits::max()) return false; out.nDepth = nDepth + 1; @@ -420,8 +427,7 @@ void CExtKey::Decode(const unsigned char code[BIP32_EXTKEY_SIZE]) { } bool ECC_InitSanityCheck() { - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); CPubKey pubkey = key.GetPubKey(); return key.VerifyPubKey(pubkey); } diff --git a/src/key.h b/src/key.h index 785059da0218c..9cac716ec86b1 100644 --- a/src/key.h +++ b/src/key.h @@ -205,6 +205,8 @@ class CKey bool initiating) const; }; +CKey GenerateRandomKey(bool compressed = true) noexcept; + struct CExtKey { unsigned char nDepth; unsigned char vchFingerprint[4]; diff --git a/src/net.cpp b/src/net.cpp index 102d81579f9e8..41f5323d910eb 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -938,13 +939,6 @@ class V2MessageMap const V2MessageMap V2_MESSAGE_MAP; -CKey GenerateRandomKey() noexcept -{ - CKey key; - key.MakeNewKey(/*fCompressed=*/true); - return key; -} - std::vector GenerateRandomGarbage() noexcept { std::vector ret; diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index f17a6b7f74bab..f7d66f316e1cd 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -85,8 +85,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) } auto build_address = [&wallet]() { - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); CTxDestination dest(GetDestinationForKey( key.GetPubKey(), wallet->m_default_address_type)); diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index a9009948eee41..d44d84af93239 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -162,9 +162,8 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup) LOCK(cs_main); tip = m_node.chainman->ActiveChain().Tip(); } - CKey coinbase_key_A, coinbase_key_B; - coinbase_key_A.MakeNewKey(true); - coinbase_key_B.MakeNewKey(true); + CKey coinbase_key_A = GenerateRandomKey(); + CKey coinbase_key_B = GenerateRandomKey(); CScript coinbase_script_pub_key_A = GetScriptForDestination(PKHash(coinbase_key_A.GetPubKey())); CScript coinbase_script_pub_key_B = GetScriptForDestination(PKHash(coinbase_key_B.GetPubKey())); std::vector> chainA, chainB; diff --git a/src/test/compress_tests.cpp b/src/test/compress_tests.cpp index b56629ae40d70..264b47b07cd56 100644 --- a/src/test/compress_tests.cpp +++ b/src/test/compress_tests.cpp @@ -65,8 +65,7 @@ BOOST_AUTO_TEST_CASE(compress_amounts) BOOST_AUTO_TEST_CASE(compress_script_to_ckey_id) { // case CKeyID - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); CPubKey pubkey = key.GetPubKey(); CScript script = CScript() << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; @@ -101,8 +100,7 @@ BOOST_AUTO_TEST_CASE(compress_script_to_cscript_id) BOOST_AUTO_TEST_CASE(compress_script_to_compressed_pubkey_id) { - CKey key; - key.MakeNewKey(true); // case compressed PubKeyID + CKey key = GenerateRandomKey(); // case compressed PubKeyID CScript script = CScript() << ToByteVector(key.GetPubKey()) << OP_CHECKSIG; // COMPRESSED_PUBLIC_KEY_SIZE (33) BOOST_CHECK_EQUAL(script.size(), 35U); @@ -119,8 +117,7 @@ BOOST_AUTO_TEST_CASE(compress_script_to_compressed_pubkey_id) BOOST_AUTO_TEST_CASE(compress_script_to_uncompressed_pubkey_id) { - CKey key; - key.MakeNewKey(false); // case uncompressed PubKeyID + CKey key = GenerateRandomKey(/*compressed=*/false); // case uncompressed PubKeyID CScript script = CScript() << ToByteVector(key.GetPubKey()) << OP_CHECKSIG; // PUBLIC_KEY_SIZE (65) BOOST_CHECK_EQUAL(script.size(), 67U); // 1 char code + 65 char pubkey + OP_CHECKSIG diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp index 58bdb37b7c8c7..29e2d4a569325 100644 --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -136,10 +136,8 @@ BOOST_AUTO_TEST_CASE(script_standard_Solver_success) BOOST_AUTO_TEST_CASE(script_standard_Solver_failure) { - CKey key; - CPubKey pubkey; - key.MakeNewKey(true); - pubkey = key.GetPubKey(); + CKey key = GenerateRandomKey(); + CPubKey pubkey = key.GetPubKey(); CScript s; std::vector > solutions; @@ -192,10 +190,8 @@ BOOST_AUTO_TEST_CASE(script_standard_Solver_failure) BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination) { - CKey key; - CPubKey pubkey; - key.MakeNewKey(true); - pubkey = key.GetPubKey(); + CKey key = GenerateRandomKey(); + CPubKey pubkey = key.GetPubKey(); CScript s; CTxDestination address; diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp index e988ce2194f15..1f674408b2c66 100644 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -1051,10 +1051,9 @@ sign_multisig(const CScript& scriptPubKey, const CKey& key, const CTransaction& BOOST_AUTO_TEST_CASE(script_CHECKMULTISIG12) { ScriptError err; - CKey key1, key2, key3; - key1.MakeNewKey(true); - key2.MakeNewKey(false); - key3.MakeNewKey(true); + CKey key1 = GenerateRandomKey(); + CKey key2 = GenerateRandomKey(/*compressed=*/false); + CKey key3 = GenerateRandomKey(); CScript scriptPubKey12; scriptPubKey12 << OP_1 << ToByteVector(key1.GetPubKey()) << ToByteVector(key2.GetPubKey()) << OP_2 << OP_CHECKMULTISIG; @@ -1081,11 +1080,10 @@ BOOST_AUTO_TEST_CASE(script_CHECKMULTISIG12) BOOST_AUTO_TEST_CASE(script_CHECKMULTISIG23) { ScriptError err; - CKey key1, key2, key3, key4; - key1.MakeNewKey(true); - key2.MakeNewKey(false); - key3.MakeNewKey(true); - key4.MakeNewKey(false); + CKey key1 = GenerateRandomKey(); + CKey key2 = GenerateRandomKey(/*compressed=*/false); + CKey key3 = GenerateRandomKey(); + CKey key4 = GenerateRandomKey(/*compressed=*/false); CScript scriptPubKey23; scriptPubKey23 << OP_2 << ToByteVector(key1.GetPubKey()) << ToByteVector(key2.GetPubKey()) << ToByteVector(key3.GetPubKey()) << OP_3 << OP_CHECKMULTISIG; @@ -1165,8 +1163,7 @@ BOOST_AUTO_TEST_CASE(script_combineSigs) std::vector pubkeys; for (int i = 0; i < 3; i++) { - CKey key; - key.MakeNewKey(i%2 == 1); + CKey key = GenerateRandomKey(/*compressed=*/i%2 == 1); keys.push_back(key); pubkeys.push_back(key.GetPubKey()); BOOST_CHECK(keystore.AddKey(key)); diff --git a/src/test/sigopcount_tests.cpp b/src/test/sigopcount_tests.cpp index c0bed50e1d261..2081acdf4da85 100644 --- a/src/test/sigopcount_tests.cpp +++ b/src/test/sigopcount_tests.cpp @@ -50,8 +50,7 @@ BOOST_AUTO_TEST_CASE(GetSigOpCount) std::vector keys; for (int i = 0; i < 3; i++) { - CKey k; - k.MakeNewKey(true); + CKey k = GenerateRandomKey(); keys.push_back(k.GetPubKey()); } CScript s2 = GetScriptForMultisig(1, keys); @@ -120,8 +119,7 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) CCoinsView coinsDummy; CCoinsViewCache coins(&coinsDummy); // Create key - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); CPubKey pubkey = key.GetPubKey(); // Default flags const uint32_t flags{SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH}; diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 5329c6ac990d9..d1cb2531aaf62 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -487,8 +487,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) CMutableTransaction mtx; mtx.nVersion = 1; - CKey key; - key.MakeNewKey(true); // Need to use compressed keys in segwit or the signing will fail + CKey key = GenerateRandomKey(); // Need to use compressed keys in segwit or the signing will fail FillableSigningProvider keystore; BOOST_CHECK(keystore.AddKeyPubKey(key, key.GetPubKey())); CKeyID hash = key.GetPubKey().GetID(); @@ -564,18 +563,16 @@ SignatureData CombineSignatures(const CMutableTransaction& input1, const CMutabl BOOST_AUTO_TEST_CASE(test_witness) { FillableSigningProvider keystore, keystore2; - CKey key1, key2, key3, key1L, key2L; - CPubKey pubkey1, pubkey2, pubkey3, pubkey1L, pubkey2L; - key1.MakeNewKey(true); - key2.MakeNewKey(true); - key3.MakeNewKey(true); - key1L.MakeNewKey(false); - key2L.MakeNewKey(false); - pubkey1 = key1.GetPubKey(); - pubkey2 = key2.GetPubKey(); - pubkey3 = key3.GetPubKey(); - pubkey1L = key1L.GetPubKey(); - pubkey2L = key2L.GetPubKey(); + CKey key1 = GenerateRandomKey(); + CKey key2 = GenerateRandomKey(); + CKey key3 = GenerateRandomKey(); + CKey key1L = GenerateRandomKey(/*compressed=*/false); + CKey key2L = GenerateRandomKey(/*compressed=*/false); + CPubKey pubkey1 = key1.GetPubKey(); + CPubKey pubkey2 = key2.GetPubKey(); + CPubKey pubkey3 = key3.GetPubKey(); + CPubKey pubkey1L = key1L.GetPubKey(); + CPubKey pubkey2L = key2L.GetPubKey(); BOOST_CHECK(keystore.AddKeyPubKey(key1, pubkey1)); BOOST_CHECK(keystore.AddKeyPubKey(key2, pubkey2)); BOOST_CHECK(keystore.AddKeyPubKey(key1L, pubkey1L)); @@ -756,8 +753,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) t.vin[0].scriptSig << std::vector(65, 0); t.vout.resize(1); t.vout[0].nValue = 90*CENT; - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); t.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); constexpr auto CheckIsStandard = [](const auto& t) { diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 637f92bd0fcdf..f6456526bb054 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -116,8 +116,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) unsigned int initialPoolSize = m_node.mempool->size(); // Parent and Child Package - CKey parent_key; - parent_key.MakeNewKey(true); + CKey parent_key = GenerateRandomKey(); CScript parent_locking_script = GetScriptForDestination(PKHash(parent_key.GetPubKey())); auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0, /*input_height=*/0, /*input_signing_key=*/coinbaseKey, @@ -125,8 +124,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) /*output_amount=*/CAmount(49 * COIN), /*submit=*/false); CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); - CKey child_key; - child_key.MakeNewKey(true); + CKey child_key = GenerateRandomKey(); CScript child_locking_script = GetScriptForDestination(PKHash(child_key.GetPubKey())); auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*input_vout=*/0, /*input_height=*/101, /*input_signing_key=*/parent_key, @@ -170,11 +168,9 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) { // The signatures won't be verified so we can just use a placeholder - CKey placeholder_key; - placeholder_key.MakeNewKey(true); + CKey placeholder_key = GenerateRandomKey(); CScript spk = GetScriptForDestination(PKHash(placeholder_key.GetPubKey())); - CKey placeholder_key_2; - placeholder_key_2.MakeNewKey(true); + CKey placeholder_key_2 = GenerateRandomKey(); CScript spk2 = GetScriptForDestination(PKHash(placeholder_key_2.GetPubKey())); // Parent and Child Package @@ -266,8 +262,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) { LOCK(cs_main); unsigned int expected_pool_size = m_node.mempool->size(); - CKey parent_key; - parent_key.MakeNewKey(true); + CKey parent_key = GenerateRandomKey(); CScript parent_locking_script = GetScriptForDestination(PKHash(parent_key.GetPubKey())); // Unrelated transactions are not allowed in package submission. @@ -298,8 +293,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_parent_child.push_back(tx_parent); package_3gen.push_back(tx_parent); - CKey child_key; - child_key.MakeNewKey(true); + CKey child_key = GenerateRandomKey(); CScript child_locking_script = GetScriptForDestination(PKHash(child_key.GetPubKey())); auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*input_vout=*/0, /*input_height=*/101, /*input_signing_key=*/parent_key, @@ -309,8 +303,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_parent_child.push_back(tx_child); package_3gen.push_back(tx_child); - CKey grandchild_key; - grandchild_key.MakeNewKey(true); + CKey grandchild_key = GenerateRandomKey(); CScript grandchild_locking_script = GetScriptForDestination(PKHash(grandchild_key.GetPubKey())); auto mtx_grandchild = CreateValidMempoolTransaction(/*input_transaction=*/tx_child, /*input_vout=*/0, /*input_height=*/101, /*input_signing_key=*/child_key, @@ -434,8 +427,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) witness2.stack.emplace_back(2); witness2.stack.emplace_back(witnessScript.begin(), witnessScript.end()); - CKey child_key; - child_key.MakeNewKey(true); + CKey child_key = GenerateRandomKey(); CScript child_locking_script = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey())); CMutableTransaction mtx_child1; mtx_child1.nVersion = 1; @@ -504,8 +496,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) // This tests a potential censorship vector in which an attacker broadcasts a competing package // where a parent's witness is mutated. The honest package should be accepted despite the fact // that we don't allow witness replacement. - CKey grandchild_key; - grandchild_key.MakeNewKey(true); + CKey grandchild_key = GenerateRandomKey(); CScript grandchild_locking_script = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey())); auto mtx_grandchild = CreateValidMempoolTransaction(/*input_transaction=*/ptx_child2, /*input_vout=*/0, /*input_height=*/0, /*input_signing_key=*/child_key, @@ -595,8 +586,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->m_min_relay_feerate.GetFee(GetVirtualTransactionSize(*ptx_parent3)) <= low_fee_amt); // child spends parent1, parent2, and parent3 - CKey mixed_grandchild_key; - mixed_grandchild_key.MakeNewKey(true); + CKey mixed_grandchild_key = GenerateRandomKey(); CScript mixed_child_spk = GetScriptForDestination(WitnessV0KeyHash(mixed_grandchild_key.GetPubKey())); CMutableTransaction mtx_mixed_child; @@ -648,11 +638,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) MockMempoolMinFee(CFeeRate(5000)); LOCK(::cs_main); size_t expected_pool_size = m_node.mempool->size(); - CKey child_key; - child_key.MakeNewKey(true); + CKey child_key = GenerateRandomKey(); CScript parent_spk = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey())); - CKey grandchild_key; - grandchild_key.MakeNewKey(true); + CKey grandchild_key = GenerateRandomKey(); CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey())); // low-fee parent and high-fee child package diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 70705667b091d..8f0b72c2b0145 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1183,8 +1183,7 @@ bool LegacyScriptPubKeyMan::CanGenerateKeys() const CPubKey LegacyScriptPubKeyMan::GenerateNewSeed() { assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); return DeriveNewSeed(key); } diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index 95d5c1b9ce855..dfad0e2126973 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -47,8 +47,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) pubkeys[i] = keys[i].GetPubKey(); } - CKey uncompressedKey; - uncompressedKey.MakeNewKey(false); + CKey uncompressedKey = GenerateRandomKey(/*compressed=*/false); CPubKey uncompressedPubkey = uncompressedKey.GetPubKey(); std::unique_ptr& chain = m_node.chain; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 1c5ca1483f034..65297054df5b1 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -231,8 +231,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) keys.push_back(key); key.clear(); key.setObject(); - CKey futureKey; - futureKey.MakeNewKey(true); + CKey futureKey = GenerateRandomKey(); key.pushKV("scriptPubKey", HexStr(GetScriptForRawPubKey(futureKey.GetPubKey()))); key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1); key.pushKV("internal", UniValue(true)); @@ -704,8 +703,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) static size_t CalculateNestedKeyhashInputSize(bool use_max_sig) { // Generate ephemeral valid pubkey - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); CPubKey pubkey = key.GetPubKey(); // Generate pubkey hash @@ -789,8 +787,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) context.args = &m_args; context.chain = m_node.chain.get(); auto wallet = TestLoadWallet(context); - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); AddKey(*wallet, key); TestUnloadWallet(std::move(wallet)); @@ -898,8 +895,7 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) context.args = &m_args; context.chain = m_node.chain.get(); auto wallet = TestLoadWallet(context); - CKey key; - key.MakeNewKey(true); + CKey key = GenerateRandomKey(); AddKey(*wallet, key); std::string error; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 507174413dca7..70fb375efa97d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3578,8 +3578,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { // Make a seed - CKey seed_key; - seed_key.MakeNewKey(true); + CKey seed_key = GenerateRandomKey(); CPubKey seed = seed_key.GetPubKey(); assert(seed_key.VerifyPubKey(seed)); From fcd429664818f14cace580513e7e6159335b5416 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 6 Dec 2023 14:47:44 +0100 Subject: [PATCH 6/8] doc: fix typo and update incorrect comment --- src/init.cpp | 2 +- src/validationinterface.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index ac52b34fc5c56..c07da0da42ea7 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -301,7 +301,7 @@ void Shutdown(NodeContext& node) DumpMempool(*node.mempool, MempoolPath(*node.args)); } - // Drop transactions we were still watching, record fee estimations and Unregister + // Drop transactions we were still watching, record fee estimations and unregister // fee estimator from validation interface. if (node.fee_estimator) { node.fee_estimator->Flush(); diff --git a/src/validationinterface.h b/src/validationinterface.h index e1d6869fab4ea..d9292ae2c9626 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -150,7 +150,7 @@ class CValidationInterface { virtual void BlockConnected(ChainstateRole role, const std::shared_ptr &block, const CBlockIndex *pindex) {} /** * Notifies listeners of a block being disconnected - * Provides the block that was connected. + * Provides the block that was disconnected. * * Called on a background thread. Only called for the active chainstate, since * background chainstates should never disconnect blocks. From 5615e16b705d74bf6ebb7c39523844f97a41cb6f Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 6 Dec 2023 14:56:23 +0100 Subject: [PATCH 7/8] tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` The boolean indicates whether the transaction was added without enforcing mempool fee limits. m_mempool_limit_bypassed is the correct variable name. Also changes NewMempoolTransactionInfo booleans descriptions to the format that is consistent with the codebase. --- src/kernel/mempool_entry.h | 6 +++--- src/policy/fees.cpp | 2 +- src/test/fuzz/policy_estimator.cpp | 8 ++++---- src/test/policyestimator_tests.cpp | 24 ++++++++++++------------ 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index bd39c9cc5f69f..2adeaea652df8 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -226,7 +226,7 @@ struct NewMempoolTransactionInfo { * This boolean indicates whether the transaction was added * without enforcing mempool fee limits. */ - const bool m_from_disconnected_block; + const bool m_mempool_limit_bypassed; /* This boolean indicates whether the transaction is part of a package. */ const bool m_submitted_in_package; /* @@ -239,11 +239,11 @@ struct NewMempoolTransactionInfo { explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height, - const bool from_disconnected_block, const bool submitted_in_package, + const bool mempool_limit_bypassed, const bool submitted_in_package, const bool chainstate_is_current, const bool has_no_mempool_parents) : info{tx, fee, vsize, height}, - m_from_disconnected_block{from_disconnected_block}, + m_mempool_limit_bypassed{mempool_limit_bypassed}, m_submitted_in_package{submitted_in_package}, m_chainstate_is_current{chainstate_is_current}, m_has_no_mempool_parents{has_no_mempool_parents} {} diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 74c688060d26d..544054863609c 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -612,7 +612,7 @@ void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& // - the node is not behind // - the transaction is not dependent on any other transactions in the mempool // - it's not part of a package. - const bool validForFeeEstimation = !tx.m_from_disconnected_block && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; + const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index 40a1fc80f07df..beee3630be91b 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -47,10 +47,10 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx); const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight(), - /* m_from_disconnected_block */ false, - /* m_submitted_in_package */ false, - /* m_chainstate_is_current */ true, - /* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool()); + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/fuzzed_data_provider.ConsumeBool()); block_policy_estimator.processTransaction(tx_info); if (fuzzed_data_provider.ConsumeBool()) { (void)block_policy_estimator.removeTx(tx.GetHash()); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 75cdfb90dc46a..ede73c6895761 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -70,10 +70,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) feeV[j], virtual_size, entry.nHeight, - /* m_from_disconnected_block */ false, - /* m_submitted_in_package */ false, - /* m_chainstate_is_current */ true, - /* m_has_no_mempool_parents */ true)}; + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/true)}; GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); } uint256 hash = tx.GetHash(); @@ -171,10 +171,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) feeV[j], virtual_size, entry.nHeight, - /* m_from_disconnected_block */ false, - /* m_submitted_in_package */ false, - /* m_chainstate_is_current */ true, - /* m_has_no_mempool_parents */ true)}; + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/true)}; GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); } uint256 hash = tx.GetHash(); @@ -235,10 +235,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) feeV[j], virtual_size, entry.nHeight, - /* m_from_disconnected_block */ false, - /* m_submitted_in_package */ false, - /* m_chainstate_is_current */ true, - /* m_has_no_mempool_parents */ true)}; + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/true)}; GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); } uint256 hash = tx.GetHash(); From b1318dcc56a0181783ee7ddbd388ae878a0efc52 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 6 Dec 2023 15:05:19 +0100 Subject: [PATCH 8/8] test: change `m_submitted_in_package` input to fuzz data provider boolean In reality some mempool transaction might be submitted in a package, so change m_submitted_in_package to fuzz data provider boolean just like m_has_no_mempool_parents. --- src/test/fuzz/policy_estimator.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index beee3630be91b..a4e1947b9f6e3 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -45,12 +45,14 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) } const CTransaction tx{*mtx}; const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx); + const auto tx_submitted_in_package = fuzzed_data_provider.ConsumeBool(); + const auto tx_has_mempool_parents = fuzzed_data_provider.ConsumeBool(); const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight(), /*mempool_limit_bypassed=*/false, - /*submitted_in_package=*/false, + tx_submitted_in_package, /*chainstate_is_current=*/true, - /*has_no_mempool_parents=*/fuzzed_data_provider.ConsumeBool()); + tx_has_mempool_parents); block_policy_estimator.processTransaction(tx_info); if (fuzzed_data_provider.ConsumeBool()) { (void)block_policy_estimator.removeTx(tx.GetHash());