Skip to content

Refactor: Updated TransactionError to TransactionResponse - #342

Open
kevkevinpal wants to merge 200 commits into
masterfrom
TransactionErrorToTransactionResponse
Open

Refactor: Updated TransactionError to TransactionResponse#342
kevkevinpal wants to merge 200 commits into
masterfrom
TransactionErrorToTransactionResponse

Conversation

@kevkevinpal

Copy link
Copy Markdown
Owner

Summary

Similar to bitcoin#32958 and motivated by bitcoin#32958 (review), these changes

  • TransactionError to TransactionResponse
  • TransactionErrorString to TransactionResultString
  • RPCErrorFromTransactionError to RPCErrorFromTransactionResponse

The reason is that TransactionError has TransactionError::Ok, which means that it is not always an error. The correct naming would instead be TransactionResponse

Sjors and others added 3 commits April 23, 2026 15:50
When setting the fd limit to unlimited, the node fails to start:

ulimit -n unlimited
build/bin/bitcoind
Error: Not enough file descriptors available. -1 available, 160 required.

This was caused by RaiseFileDescriptorLimit() casting limitFD.rlim_cur
to int, which for RLIM_INFINITY overflows to -1. Fix it by returning
std::numeric_limits<int>::max() instead.

This commit also adds a functional test, which is skipped on environments
with a hard limit below infinity.

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
Co-authored-by: winterrdog
When setting the fd limit to 1 >> 31, the node fails to start:

ulimit -n 214748364
build/bin/bitcoind
Error: Not enough file descriptors available. -2147483648 available, 160 required.

Similar to the previous commit, this is fixed by capping the limit
to std::numeric_limits<int>::max().

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
On 32-bit systems we build with _FILE_OFFSET_BITS=64 (see CMakeLists.txt),
which makes rlim_t 64-bit when building against glibc (see bits/resource.h).
Since size_t could be 32-bit, clamp RLIMIT_MEMLOCK to
std::numeric_limits<size_t>::max() in PosixLockedPageAllocator::GetLimit().

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
@kevkevinpal
kevkevinpal force-pushed the TransactionErrorToTransactionResponse branch from cad26e6 to 592a833 Compare April 28, 2026 12:11
polespinasa and others added 10 commits May 21, 2026 01:05
Store wallet names into MigrationResult struct when migrating a wallet.
Also refactor the RPC and the wallet interface to rely on them instead of pointers to shared_ptr<CWallet> objects.

This allows in a future commit migrate wallet without loading them.
The variable is never set and will always be unbound.

The only way to set it correctly is via this hack:

.github/workflows/ci.yml-        # In the image build step, no external environment variables are available,
.github/workflows/ci.yml-        # so any settings will need to be written to the settings env file:
.github/workflows/ci.yml:        run: sed -i "s|\${INSTALL_BCC_TRACING_TOOLS}|true|g" ./ci/test/00_setup_env_native_asan.sh

So just silence the warning, which happens when running the task
locally:

```
./ci/test/00_setup_env_native_asan.sh: line 12: INSTALL_BCC_TRACING_TOOLS: unbound variable
```
Shellcheck marked the script as violating SC2044.

Instead of re-writing the Bash from scratch, just use Python.
This commit does not change behavior.

Suggested in bitcoin#33966 (comment)
The option defaults to true, so describe the intended exceptional use
case as disabling validation for tests and benchmarks.

Suggested in bitcoin#33966 (comment)
@kevkevinpal
kevkevinpal force-pushed the TransactionErrorToTransactionResponse branch from 592a833 to 1bcea55 Compare June 2, 2026 03:12
willcl-ark and others added 15 commits June 2, 2026 12:34
We are currently sometimes backlogged on waiting for runner selection.

Selecting Warp or GitHub-hosted runners directly from the repository context avoids serializing all CI jobs behind a metadata job. This keeps forks on public runners while allowing upstream jobs to schedule immediately on the intended runner labels.
The seed ID is calculated from a pubkey produced by treating the seed as
a private key. This calculation includes a pubkey compression parameter,
even thought that compression is completely irrelevant for the usage of
the seed as a BIP 32 seed. Thus migration should detect if a seed has
been used multiple times by checking if the computed master key was
already processed.

The spkm_migration fuzzer needs to have it's added descriptors
accounting to be updated for this fix.
1. The trailing slash is redundant.
2. It causes warnings when using CMake >=3.31:
```
CMake Warning (dev) at CMakeLists.txt:596 (install):
  Policy CMP0177 is not set: install() DESTINATION paths are normalized.  Run
  "cmake --help-policy CMP0177" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.
This warning is for project developers.  Use -Wno-dev to suppress it.

```
Switch RPCResultOptions::print_elision to a variant
<HelpElisionNone, HelpElisionSkip, std::string> and add a constructor
that copies a result with replacement options.

Use the new ElideGroup() in TxDoc() to apply elision to field groups
instead of setting print_elision per field. Type::ELISION stays as a
deprecated alias.
This avoids implicit conversions from string literals to `std::string`,
eliminating the need to include `<string>` everywhere the `BENCHMARK()`
macro is used.
1. FetchBlock runs in a http worker thread. It acquires a PeerRef,
locks cs_main, then may later call BlockRequested which asserts
that CNodeState exists for the peer.

2. FinalizeNode may run in either the bitcoind or b-net threads. It
locks cs_main, fetches a PeerRef from RemovePeer, fetches a CNodeState,
and later removes it from m_node_states.

Because of the lock placement in FetchBlock, the http worker thread in 1)
can acquire a valid PeerRef and block while the b-net thread in 2) is
cleaning up the peer in FinalizeNode. When the worker thread later acquires
cs_main, it may crash in BlockRequested since no CNodeState exists. Fix
this by acquiring the lock earlier in FetchBlock.

The lock can be replaced with a net-specific lock when the remaining
CNodeState fields are moved to Peer.
…gle assert_equal with 3 args instead of multiple assert_equals
In a pruned node undergoing a deep reorg, FindMostWorkChain can
insert duplicate entries into m_blocks_unlinked.

This can happen when:
- Traversing from one candidate tip to the fork point adds blocks
whose parents have been pruned.
- Traversing from another candidate tip over the same fork inserts
the same pairs again, since the blocks are shared across
both branches.

When we finally download the missing parent from our peer and call
ReceivedBlockTransactions to process m_blocks_unlinked, the same
entry may be processed multiple times.

This can lead to re-insertion into setBlockIndexCandidates with
a modified nSequenceId, violating its ordering invariants and
causing undefined behavior. So avoid duplicate insertions into
m_blocks_unlinked here.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
when pruned node2 finally receives the blocks from
node0, node2 will process duplicate entires in
ReceivedBlockTransactions twice but it will only
insert into setBlockIndexCandidates if the block has
more work that the current chain tip.

the duplicate entries in m_blocks_unlinked in this
test are from height 1171 to 1294.

before this commit
- we invalidated height 1320 and chain tip became
1319. so we won't add duplicate entries (all have
<1319) into setBlockIndexCandidates and won't
have coverage for this UB scenario.

with this commit
- we invalidate height 1295 and chain tip became
1294. so we will process the duplicate entry
1294 in m_blocks_unlinked and have coverage
for this UB.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
remove misleading comment for m_blocks_unlinked since for
pruned nodes:
- usually A is the missing data (just like in non-pruned nodes)
- in PruneOneBlockFile, we remove entries once data for B is missing.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
For an entry A -> B in m_blocks_unlinked, the entry B was added into
m_blocks_unlinked either because:
- some ancestor of B was never received (or)
- some ancestor of B was pruned away.

Every insert must satisfy two invariants:
1. B has BLOCK_HAVE_DATA set.
2. No duplicate A -> B entries in m_blocks_unlinked (this is UB zone if
	this entry gets popped twice in ReceivedBlockTransactions and
	happens to be in setBlockIndexCandidates)

2 bugs (bitcoin#35070 and bitcoin#35168) discovered recently stemmed from the
m_blocks_unlinked insertion sites not enforcing these invariants.
So add a helper which wraps around insertion sites of m_blocks_unlinked
with these invariants.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Reserving the wallet rescan is the first thing done when starting a rescan.
As part of the reservation, clear any leftover state from previous
rescans (reset fAbortRescan). This prevents a race condition
where an abort request arrives before the rescan loop starts; without
this reset, the abort could be ignored and the rescan would proceed.

Co-authored-by: w0xlt <woltx@protonmail.com>
Co-authored-by: w0xlt <woltx@protonmail.com>
rkrux and others added 29 commits June 23, 2026 15:13
Each FuzzedSock used to own its mocked steady clock and call
MockableSteadyClock::SetMockTime() directly. Hold the clock by reference
to an externally provided FakeSteadyClock instead, so that several
FuzzedSock instances sharing a test case (e.g. one per peer, or one
created via Accept()) advance a single mocked clock, and the mocking goes
through the FakeSteadyClock RAII helper that resets mocktime on
destruction.

FakeSteadyClock is a LimitOne type, so each fuzz target constructs one
instance per iteration and passes it to ConsumeSock / ConsumeNode / the
FuzzedSock constructor.
…, reject sendtoaddress/sendmany

2fe3480 wallet: reject sendtoaddress and sendmany for external signers (Sjors Provoost)
bd5a32f doc: add taproot descriptor to getdescriptors example (woltx)
7131c82 doc: clarify which commands receive --chain, --fingerprint and --stdin (woltx)
4fdd4d8 doc: replace stale signtransaction wording with current signtx flow (woltx)
fab9225 doc, rpc: document enumerate model field and fingerprint deduplication (woltx)

Pull request description:

  This PR aligns the external signer documentation with current behavior, and makes one previously implicit behavior explicit.
  Per review feedback, each commit fixes a limited set of issues:

  * **doc, rpc: document enumerate model field and fingerprint deduplication** — the `enumerate` response uses the optional `model` field, which Bitcoin Core maps to the `name` field of the `enumeratesigners` RPC result. Duplicate fingerprints are skipped, and wallet operations require exactly one connected signer.

  * **doc: replace stale signtransaction wording with current signtx flow** — spending from an external signer wallet uses `send`/`sendall` (and `bumpfee` for fee-bumping), which invoke `<cmd> --stdin` and pass the `signtx` subcommand and PSBT over stdin.

  * **doc: clarify which commands receive --chain, --fingerprint and --stdin** — mark `--chain` and `--fingerprint` as required except for `enumerate`, keep `--stdin` required for protocol flexibility, and match the order and form of the actual invocations in the usage examples.

  * **doc: add taproot descriptor to getdescriptors example** — show the BIP86 `tr()` descriptor alongside the other address types.

  * **wallet: reject sendtoaddress and sendmany for external signers** — return a specific error instead of the misleading "Private keys are disabled for this wallet", with functional test coverage. Cherry-picked from bitcoin#33112 (thanks Sjors).

  How the documentation went stale:

  * The `enumerate` example has shown a `name` field since external signer support landed in bitcoin#16546, but the implementation has always read `model`.

   * `sendtoaddress`/`sendmany` external signer support was effectively precluded by bitcoin#21201, which was merged a few days before bitcoin#16546, so the interaction was missed in review and the documented `signtransaction` flow never existed in this form.

  * Fingerprint deduplication was added in bitcoin#35251.

  * The documentation was last updated in bitcoin#33765.

ACKs for top commit:
  Sjors:
    ACK 2fe3480
  optout21:
    ACK 2fe3480
  naiyoma:
    ACK 2fe3480

Tree-SHA512: 86859d2f81ac337f3b4b6578c6ee0151ffb76b8374dfa58e28e00ce4eb69dc200cd6bd2d0a99f73d0475c3824d6ac1cb9e2542b119ca124dd835132dc95cd023
146b3ad doc: remove libevent (fanquake)
96d7f55 vcpkg: remove libevent (fanquake)
0443943 ci: remove libevent (fanquake)
a0ca249 depends: remove libevent (fanquake)
35d2d06 cmake: remove libevent (fanquake)

Pull request description:

  This builds on all the work done by fjahr and pinheadmz to fully remove libevent from the codebase.

  Closes bitcoin#31194.

ACKs for top commit:
  fjahr:
    ACK 146b3ad
  dergoegge:
    ACK 146b3ad
  pinheadmz:
    ACK 146b3ad
  sedited:
    ACK 146b3ad

Tree-SHA512: ecd14be93d11603d7c373a41474a7df1734b48550b12cd37933b604860913a77d42ee08bc187610881bec239b0834c2486f8fe52299cd3315a57b79c2e95929d
…artup defaults)

b847626 test: refresh MiniWallet after node restart (Sjors Provoost)
f4e643c test: merge mining options in package feerate check (Sjors Provoost)
280ce6a miner: ensure block_max_weight is flattened before limit checks (Sjors Provoost)
65bd316 mining: clarify test_block_validity comment (Sjors Provoost)
978e721 test: use shared default_ipc_timeout (Sjors Provoost)

Pull request description:

  This implement the suggested followups from bitcoin#33966. Each commit links  to the original comment.

  The most important change is the extra asserts added in `miner: ensure block_max_weight is flattened before limit checks`.

ACKs for top commit:
  achow101:
    ACK b847626
  enirox001:
    tACK b847626
  sedited:
    ACK b847626
  w0xlt:
    ACK b847626

Tree-SHA512: 47678eaed604228269bd892ccf8ff58804745bbc7675b4a93528da9a9292a2eb1e0562cdb8341edac77178563420885b48282bb9e5c2b997b28f2fc64ceeff3d
Rename the `CCoinsViewDB` async compaction wrapper to `CompactFullAsync()` so it is distinct from the blocking `CDBWrapper::CompactFull()` primitive it calls.
Exercise `CCoinsViewDB::CompactFullAsync()` from the `coins_view_db` fuzz target so the new chainstate compaction wrapper can run concurrently with ordinary coins view operations.

The fuzz operation only schedules compaction, matching production; outstanding work is waited for by the `CCoinsViewDB` destructor at the end of the fuzz input.
…etation

abc33ff test: announce field must be 0 or 1 in sendcmpct (brunoerg)
2d0dce0 net_processing: fix BIP152 first integer interpretation (brunoerg)

Pull request description:

  Fixes bitcoin#35542

  According to the BIP152, the first integer in `sendcmpct` message shall be interpreted as a boolean (and MUST have a value of either 1 or 0). We currently correctly interpret it as boolean, however, we accept any value >=1 and treat it as `true`, deviating from the specification. This PR fixes it.

ACKs for top commit:
  edilmedeiros:
    utACK abc33ff
  davidgumberg:
    crACK bitcoin@abc33ff Seems reasonable to comply with BIP152 strictly, test looks good as well.
  Sjors:
    ACK abc33ff
  jonatack:
    re-ACK abc33ff
  achow101:
    ACK abc33ff
  w0xlt:
    ACK abc33ff

Tree-SHA512: 77fed86d4de81f7c35ff002b6e1b2a90882ea55f159075da4d34a619d1075f625fca34f929cdd981f1b2eb06f76b64f42cda46502dcd1b4a634c690b0882ec7c
703a671 fuzz: compact coins view db during fuzzing (Lőrinc)
0868c85 refactor: rename async coin compaction (Lőrinc)

Pull request description:

  **Problem:** bitcoin#35465 added async chainstate compaction, but the `coins_view_db` fuzz target did not exercise scheduling compaction alongside ordinary coins view operations.
  The async wrapper also shared the `CompactFull()` name with the blocking `CDBWrapper` primitive.

  **Fix:** Rename the coins DB wrapper to `CompactFullAsync()` and let `coins_view_db` randomly schedule it under `cs_main` (like in production).
  The fuzz operation only starts compaction and any running job is joined by the `CCoinsViewDB` destructor at the end of the fuzz input.

ACKs for top commit:
  sedited:
    ACK 703a671
  andrewtoth:
    ACK 703a671

Tree-SHA512: 9854c3acbaace795155e7469cb10938fbd872726cbb8a4b4ef71d6d352d00824498747215aee1b237e64452eae2e690a3f7d7daa6b6b0030e75a6f2ccc0802fb
…t after migrating

0cdd817 add release note (Pol Espinasa)
517d37c test: tests wallet migration with load_wallet disabled (Pol Espinasa)
b98dd63 rpc: Add load_wallet argument to migratewallet RPC (Pol Espinasa)
4acd063 wallet: make loading the wallet after migrating optional (Pol Espinasa)
97d08d6 refactor: store wallet names to MigrationResult (Pol Espinasa)

Pull request description:

  This PR is motivated by this [Stack Exchange question](https://bitcoin.stackexchange.com/questions/130713/bitcoin-core-quickest-method-legacy-descriptor-wallet-migration).

  Long story short, someone who has a node pruned before his legacy wallet birthday, is unable to migrate the wallet as it is not possible to load it.

  Loading is not necessary for migration, and migrating without wanting to use the wallet in that node is a valid use-case.

  This PR adds a new RPC argument to `migratewallet` that allow the user disabling the wallet loading.
  Second commits adds tests for it.

  Follow-up: Add an option to the GUI to not load the wallet after migrating.

ACKs for top commit:
  achow101:
    ACK 0cdd817
  w0xlt:
    ACK 0cdd817
  pablomartin4btc:
    ACK bitcoin@0cdd817

Tree-SHA512: 8389599e63603b1a532e1bfba0b6c652653386c001f5a881bd49843302b74ff4dbaa4131b5b377c24f483d42e0e70a92b96f760244e3c2e2b44ce08cd04ca1e0
9e6546c test: raise reindex mining RPC timeout (Lőrinc)

Pull request description:

  **Problem:** I often hit a timeout in `feature_reindex.py` when running functional tests locally in parallel in debug mode (especially on battery or in power-saving mode).
  **Fix:** Increase the test-local RPC timeout for the reindex mining setup.

ACKs for top commit:
  mercie-ux:
    ACK 9e6546c
  sedited:
    ACK 9e6546c

Tree-SHA512: d3541dd6752f943921a030393b00e684b3b5d00b93aa0b2b1f85c017698bdfe2216d7f5441bb7dd5f153453385e4df6f9cc8055589e57bca83fa48f0b7de4252
This doesn't build QT, so drop ninja and xz, and add a comment.
With libevent removed, and no Qt build, we can also drop pkgconf.
58560c2 ci: remove some packages from Chimera job (fanquake)

Pull request description:

  This job doesn't build QT, so drop `ninja` and `xz`.
  With libevent removed, and no Qt build, we can also drop `pkgconf`.

ACKs for top commit:
  sedited:
    ACK 58560c2
  hebasto:
    ACK 58560c2.

Tree-SHA512: 4537c8c77334637ea177321d95ad39ee103ab0080e8331f9fe9efb92a3bb4dc00fb59d2e250fd43c267c05f0a2b98e779949ff40723633c2f2bff345fa554a31
…d sendall

8ebfff0 doc: add send RPC release note (Sjors Provoost)
5884f5a wallet: remove experimental warning from send RPCs (Sjors Provoost)

Pull request description:

  The `send` RPC was introduced in v21 an initially marked experimental. The `sendall` RPC was added in v24, based on `send` and also marked experimental.

  I'm not aware of any proposed breaking changes, except bitcoin#35433 which follows the regular deprecation flow.

  Time to mark them as no longer experimental.

ACKs for top commit:
  w0xlt:
    ACK 8ebfff0
  achow101:
    ACK 8ebfff0
  polespinasa:
    ACK 8ebfff0
  pablomartin4btc:
    ACK 8ebfff0

Tree-SHA512: beb5321adaf871157bda396c8e5740daff95ffe342416914340ae4197accebe60236032d1329876b42405437b99f59079a56ec1e5ac592b753031ba2ebd36cfb
df9eb72 test: ensure group data cluster pointers are live (Greg Sanders)

Pull request description:

  Belt-and-suspenders check inside `SanityCheck` to avoid dangling pointers.

ACKs for top commit:
  l0rinc:
    code review ACK df9eb72
  marcofleon:
    lgtm ACK df9eb72
  sipa:
    Cannot-hurt ACK df9eb72
  sedited:
    ACK df9eb72

Tree-SHA512: a666b07a56401182aac156bf575ebe3e0a7fc89cd885ccb0b0e65d64da4df673be2b00cf1643a930da1cd65806924ecb6cd94cb18bcead5761aeae775fa5f2e3
…coverage

1a3cfdf fuzz: connman: cover AddLocalServices/RemoveLocalServices (Bruno Garcia)
c507fb3 fuzz: connman: add outbound-bytes invariants (Bruno Garcia)
4a6fce4 fuzz: connman: add AddNode/RemoveAddedNode invariants (Bruno Garcia)
a5859ed fuzz: connman: set m_local_services/m_use_addrman_outgoing/m_max_automatic_connections (Bruno Garcia)
4b84c91 fuzz: connman: add network activity invariants (Bruno Garcia)

Pull request description:

  This PR improves the `connman` fuzz target by replacing some "`(void)`" calls with actual invariant checks, adding coverage for previously uncovered methods, and exercising more initialization states.

    - Set `m_local_services`, `m_use_addrman_outgoing`, and
      `m_max_automatic_connections` via fuzzed values before `Init()` to
      explore more startup configurations.

    - Add network activity and outbound-bytes invariants.

    - Add `AddNode`/`RemoveAddedNode` invariants: e.g. a successful `AddNode`
      increases `GetAddedNodeInfo()` by one; adding the same node again
      must fail; a subsequent `RemoveAddedNode` must succeed and restore
      the original count.

    - Add coverage for `AddLocalServices`/`RemoveLocalServices`.

ACKs for top commit:
  nervana21:
    re-ACK 1a3cfdf
  frankomosh:
    reACK 1a3cfdf . Change from the diff is the restoring `(void)connman.RemoveAddedNode(random_string)` arm
  sedited:
    ACK 1a3cfdf

Tree-SHA512: c7b6799ca65d2e639d8ab9ab0cc77bae663f24fbda934446a8ee2e8ce9e8e36624d16b4f492b1714e2d67375edd35907cb9392d21f368d3d5298275ff1d05c72
…onfig` requirements

fb8a103 doc: Clarify build docs about `pkgconf` / `pkg-config` requirements (Hennadii Stepanov)

Pull request description:

  Since bitcoin#34411, `pkgconf`/`pkg-config` is no longer strictly required.

  It is currently required for ZeroMQ on several major distributions and operating systems, including Debian, Ubuntu, the BSD derivatives, and Gentoo.

  Regarding QRencode, our [`FindQRencode.cmake`](https://github.com/bitcoin/bitcoin/blob/master/cmake/module/FindQRencode.cmake) module treats `pkgconf`/`pkg-config` as optional and can successfully locate the package without it.

  This PR amends bitcoin#34411 and updates the build notes accordingly.

  Addresses bitcoin#34411 (review).

ACKs for top commit:
  purpleKarrot:
    ACK fb8a103
  sedited:
    ACK fb8a103

Tree-SHA512: 0f740c954f058ce69fe936a51cfe1dc36dc374d392a94343d5c9f6b0be9565e4163c01015b00fef4e58eb180a8a520e6e81ea8b206868b3bcdf077caf0d24d65
The local `static constexpr auto ERR` shadowed the `Sock::ERR` static
data member. Rename it to `accept_error`, per the Developer Notes'
shadowing guidance.

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
…ublicKey classes

8791c47 test: use ExtendedPrivateKey in wallet_taproot.py (rkrux)
89ceafa test: use ExtendedPrivateKey in wallet_listdescriptors.py (rkrux)
bbfffca test: use ExtendedPrivateKey in wallet_send.py (rkrux)
2ab6e59 test: use ExtendedPrivateKey in wallet_keypool.py (rkrux)
9e20118 test: use ExtendedPrivateKey in wallet_fundrawtransaction.py (rkrux)
06af0cd test: use ExtendedPrivateKey in wallet_descriptor.py (rkrux)
4100fac test: use ExtendedPrivateKey in wallet_createwallet.py (rkrux)
ff3f6de test: use ExtendedPrivateKey in wallet_bumpfee.py (rkrux)
003f2a0 test: use ExtendedPrivateKey in feature_notifications.py (rkrux)
f988e6d test: use ExtendedPrivateKey in wallet_importdescriptors.py (rkrux)
d2a03d5 test: add extendedkey.py unit tests by using BIP32 test vectors (rkrux)
afdb378 test: introduce ExtendedPrivateKey and ExtendedPublicKey classes (rkrux)
4dbaa7c test: generalise byte_to_base58 utility function to allow more version types (rkrux)

Pull request description:

  Many a times there has been a need to come up with dynamic xprvs and xpubs
  in the functional tests, but the lack of code that creates them dynamically has
  led to the presence of several hardcoded keys in the testing framework. This
  is not developer friendly and not self-documenting, clutters the testing code,
  and makes it difficult to update the tests in the future.

  This PR introduces two utility classes ExtendedPrivateKey and
  ExtendedPublicKey that allows the developer to create them on the fly
  to be used in the tests. I have intentionally not introduced any library for this
  purpose and have reused the existing libraries and functions in the framework.
  The implementation is supposed to provide basic functionality for creating
  xprv randomly or from a fixed seed, creating corresponding xpub, and
  deriving child xprvs and xpubs at custom derivation paths.

  I've updated many tests to show how these can be used, there are more
  tests as well that can be updated in the future to completely remove such
  non-deterministic hardcoded keys.

ACKs for top commit:
  achow101:
    ACK 8791c47
  w0xlt:
    ACK 8791c47

Tree-SHA512: f8ec4e09eaa6cc44b0f1c9a91337e570b12fb882c258be89b470de1a8cecf9d2fd40d9f02ee739dcbf639462ea7710aa145a3726f0f537f5a1f1e7772e5b019d
…FuzzedSock instances

6fa4132 fuzz: share a single mocked steady clock across FuzzedSock instances (Hao Xu)

Pull request description:

  This is a follow-up of bitcoin#35478 (comment), inspired by maflcko .

  Each FuzzedSock used to own its mocked steady clock and call MockableSteadyClock::SetMockTime() directly. Hold the clock by reference to an externally provided SteadyClockContext instead, so that several FuzzedSock instances sharing a test case (e.g. one per peer, or one created via Accept()) advance a single mocked clock, and the mocking goes through the SteadyClockContext RAII helper that resets mocktime on destruction.

  SteadyClockContext is a LimitOne type, so each fuzz target constructs one instance per iteration and passes it to ConsumeSock / ConsumeNode / the FuzzedSock constructor.

ACKs for top commit:
  maflcko:
    review ACK 6fa4132   🌕
  marcofleon:
    crACK 6fa4132

Tree-SHA512: 3c773b5c0c3ba42a8245c9ea6042b0bc767df4fad506305f3c200310616b48a59deb1542086eb4ce3e8a1407c4d6b42cef3b37cd84bfe80d4821972b8d3b4286
The `ERR` macro is defined on illumos-based systems in the `regset.h`
header included by the Boost.Test framework, which causes a compilation
error.

-BEGIN VERIFY SCRIPT-

ren1() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/util/sock.* ./src/httpserver.h ) ; }
ren1 RECV RecvEvent
ren1 SEND SendEvent
ren1 ERR  ErrorEvent

ren2() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren2 Sock::RECV Sock::RecvEvent
ren2 Sock::SEND Sock::SendEvent
ren2 Sock::ERR  Sock::ErrorEvent

-END VERIFY SCRIPT-
The `ERR` macro is defined on illumos-based systems in the `regset.h`
header included by the Boost.Test framework, which may cause a
compilation error.

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/qt/psbtoperationsdialog.* ) ; }
ren StatusLevel::INFO StatusLevel::Info
ren              INFO              Info
ren StatusLevel::WARN StatusLevel::Warn
ren              WARN              Warn
ren StatusLevel::ERR  StatusLevel::Error
ren              ERR               Error

-END VERIFY SCRIPT-
41ceea4 scripted-diff: Rename `StatusLevel::{INFO,WARN,ERR}` (Hennadii Stepanov)
f395acd scripted-diff: Rename `Sock::{RECV,SEND,ERR}` (Hennadii Stepanov)
7ac25c9 util, refactor: Rename local `ERR` in `Sock::Accept` (Hennadii Stepanov)

Pull request description:

  The `ERR` macro is [defined](https://github.com/illumos/illumos-gate/blob/10a869258e300c530ae56b29aa3bf43461ca98ff/usr/src/uts/intel/sys/regset.h#L101) on illumos-based systems in the `regset.h` header included by the Boost.Test framework, which causes a compilation error:
  - on [OmniOS](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958700):
  ```
  [484/792] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o
  FAILED: [code=1] src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o
  /usr/bin/g++ -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/src -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/univalue/include -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/minisketch/include -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/secp256k1/include -isystem /usr/gnu/include -pthread -O2 -g -std=c++20 -fno-extended-identifiers -fmacro-prefix-map=/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src=. -fstack-reuse=none -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -Werror -MD -MT src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o -c /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp
  In file included from /usr/include/sys/procfs_isa.h:39,
                   from /usr/include/sys/procfs.h:66,
                   from /usr/include/procfs.h:45,
                   from /usr/local/include/boost/test/impl/debug.ipp:85,
                   from /usr/local/include/boost/test/included/unit_test.hpp:20,
                   from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp:10:
  /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/sock.h:162:28: error: expected unqualified-id before numeric constant
    162 |     static constexpr Event ERR = 0b100;
        |                            ^~~
  ```
  - on [OpenIndiana](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958536/job/82891036242):
  ```
  [44](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958536/job/82891036242#step:7:545)
  [ 59%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o
  [ 59%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/addrman_tests.cpp.o
  In file included from /usr/include/sys/procfs_isa.h:39,
                   from /usr/include/sys/procfs.h:66,
                   from /usr/include/procfs.h:45,
                   from /usr/include/boost/test/impl/debug.ipp:85,
                   from /usr/include/boost/test/included/unit_test.hpp:20,
                   from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp:10:
  /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/sock.h:162:28: error: expected unqualified-id before numeric constant
    162 |     static constexpr Event ERR = 0b100;
        |                            ^~~
  ```

  ---

  `StatusLevel::{INFO,WARN,ERR}` has also been renamed to ensure future-proofing and consistency.

ACKs for top commit:
  maflcko:
    review ACK 41ceea4 💭
  sedited:
    ACK 41ceea4
  hodlinator:
    re-ACK 41ceea4

Tree-SHA512: bf3c76468f9b0167e22356c140306a626bf4f769076c260931d35047724970dd2c97985bde2ef3ceb192d560710832b535ef9bba2ea133c0bfe4eb6b2e8e447f
@kevkevinpal
kevkevinpal force-pushed the TransactionErrorToTransactionResponse branch from 78a2b0d to 9b53586 Compare June 28, 2026 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.