The global unsigned tx is decomposed into separate fields inside of
PSBT, which mirrors what PSBTv2 will do. However, we still need to get
the global unsigned tx so PSBT::GetUnsignedTx is introduced to do that.
In order to also have a stable unique ID, we also introduce
PSBT::GetUniqueID to replace uses of PSBT.tx.GetHash().
PSBTInput now has the previous txid and output index, and PSBTOutput has
the amount and script. We no longer need to access the global unsigned
tx for these fields.
Additionally, we can change iterating tx.vin and tx.vout to psbtx.inputs
and psbtx.outputs.
This is in prepration for use with PSBTv2 where the global unsigned tx
will not exist.
When decomposing a transaction into a PSBTv2, the tx version and
locktime need to be stored in their respective global fields. Add those
fields and fill them when constructing.
Instead of allowing PSBTs to be default constructor, force usage of the
deserialization constructor.
CombinePSBTs, DecodeBase64PSBT, and DecodeRawPSBT are all changed to
return std::optional or util::result rather than using an output
parameter to avoid the need for a default constructor.
PSBTInput should be aware of the previous txid, output index, and
sequence numbers for inputs, extracting them from the global
unsigned tx.
PSBTOutput should be aware of the output amount and script, extracting
them from the global unsigned tx.
This prepares for PSBTv2 where these fields are serialized.
Every key has a duplicate key lookup check, and many keys have fixed
size checks. These can be refactored to reduce code duplication.
Co-Authored-By: David Gumberg <davidzgumberg@gmail.com>
eab72d14d7 refactor: use SignOptions for MutableTransactionSignatureCreator (Sjors Provoost)
5ed41752c5 refactor: use SignOptions for SignTransaction (Sjors Provoost)
dc4a5d1270 refactor: use PSBTFillOptions for filling and signing (Sjors Provoost)
Pull request description:
Replace the `sign`, `finalize` , `bip32derivs` and `sighash_type` arguments that are passed to `FillPSBT()` and `SignPSBTInput()` with a `PSBTFillOptions` struct.
```cpp
struct PSBTFillOptions {
bool sign{true};
std::optional<int> sighash_type{std::nullopt};
bool finalize{true};
bool bip32_derivs{true};
};
```
Additionally this PR introduces:
```cpp
struct SignOptions {
int sighash_type{SIGHASH_DEFAULT};
};
```
This is used by `SignTransaction` and the `MutableTransactionSignatureCreator` constructor.
These changes make it easier to add additional options later without large code churn, such as `avoid_script_path` proposed in #32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.
ACKs for top commit:
w0xlt:
reACK eab72d14d7
optout21:
ACK eab72d14d7
sedited:
ACK eab72d14d7
Tree-SHA512: 097e3d042e794c9f47d03e1aafad184a4525aa765d274f6497122d4f41603e902191df6fbf9ce846dbcd7372a159b67e2234da7341ec6a6776be5685e3e6e6ff
Replace the sign, finalize , bip32derivs and sighash_type arguments which
are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.
This makes it easier to add additional options later without large code
churn, such as avoid_script_path proposed in #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.
8deed0df06 doc: add release notes for PR 34911 (rkrux)
1a85ca1dff rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses (rkrux)
f89d18c3b1 rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response (rkrux)
Pull request description:
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently.
The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the
response anymore unless the user requests it via -deprecatedrpc=fullrbf node
argument.
Also, remove the `bip125-replaceable` key from the mempool RPCs
responses (because it, too, has been deprecated since v29) unless the user
requests it via -deprecatedrpc=bip125 node argument.
ACKs for top commit:
optout21:
ACK 8deed0df06
sedited:
ACK 8deed0df06
instagibbs:
ACK 8deed0df06
Tree-SHA512: dfbbdf04ae25dafe81758bd85715af80d736e21bb913ab09ff8fd8225587b7a1bf1dd6b0f7ea05135504e11d43371d98e5f5fc6e3717d581e870e8c2bf97811b
a1e534bbf0 doc: clarify clang-tidy in developer notes (rkrux)
Pull request description:
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
ACKs for top commit:
sedited:
ACK a1e534bbf0
Tree-SHA512: e95cd44cdad9a9011c18136ec7f1db4e73f78f11c517f94c14cf128beaaea150881868c27e7c5c710c9cab94d285d61dc5e5278f347c229f5df495cd49c532ab
ef21e29298 doc: update llvm based coverage example (Max Edwards)
Pull request description:
The old example was working on my Mac but when run on Linux I was finding that I had missing coverage information.
On some systems the old example of using xargs to pass list of profraw files can exceed xargs's argument limit resulting in multiple executions of llvm-profdata merge command which overwrites it's output file losing coverage information. This PR switches the example to using a file with a list of filenames.
There are other ways to solve this like using bash's globbing or a one liner with temporary file but I felt like this was the most simple, robust and easy to understand.
ACKs for top commit:
vasild:
ACK ef21e29298
sedited:
ACK ef21e29298
Tree-SHA512: 2726d9b38088c1c0b6f04d4622bdc4ea28e16b13a962a3afc842798aa5e0eee4682752c73707c5580a43bdb9d0d84d542afd357f166e4dbb7f6beb7afc0522c8
14547eb489 kernel: guard btck::Handle move-assignment against self-move (Thomas)
Pull request description:
The move-assignment operator for `btck::Handle<>` in `src/kernel/bitcoinkernel_wrapper.h` unconditionally called `DestroyFunc(m_ptr)` before reading the source pointer. On a self-move (`h = std::move(h)`), this destroys the held resource and then restores the now-dangling pointer via `std::exchange(other.m_ptr, nullptr)` (since `&other == this`), which leaves `m_ptr` pointing at freed memory. The destructor then calls `DestroyFunc` again on it, resulting in a double-free.
Trace of `h = std::move(h)` with the old code, where `h.m_ptr == P`:
1. `DestroyFunc(m_ptr)` -> `delete P`. `this->m_ptr` still literally stores the now-dangling value `P`.
2. `std::exchange(other.m_ptr, nullptr)` — because `&other == this`, this reads the dangling `P` back, writes `nullptr` to `m_ptr`, and returns `P`.
3. `m_ptr = P` restores the dangling pointer.
4. `~Handle()` later runs `DestroyFunc(P)` -> double free, UB.
The copy-assignment operator already guards against self-assignment with `if (this != &other)`; the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias.
`Handle<>` is the base class of 16 public types in the kernel C++ API wrapper (`Transaction`, `Block`, `BlockHeader`, `ChainParams`, `Context`, `Coin`, `BlockValidationState`, `ScriptPubkey`, `TransactionOutput`, `Txid`, `OutPoint`, `TransactionInput`, `PrecomputedTransactionData`, `BlockHash`, `BlockSpentOutputs`, `TransactionSpentOutputs`), so self-move can arise from generic algorithms operating on containers of these types (`std::sort`, `std::remove`, erase-remove idioms, etc.).
Fix: mirror the copy-assignment pattern by guarding the move-assignment body with `if (this != &other)`, making a self-move a no-op.
Also extend `CheckHandle` in `src/test/kernel/test_kernel.cpp` to exercise self-move-assignment for every `Handle`-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged.
ACKs for top commit:
sedited:
Thanks, ACK 14547eb489
alexanderwiederin:
ACK 14547eb489
yuvicc:
lgtm ACK 14547eb489
Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa
fa204100e1 streams: Remove confusing DataStream::in_avail() (MarcoFalke)
fa5ab0220e move-only: Extract ProcessPong() helper (MarcoFalke)
Pull request description:
The `in_avail` alias is confusing (see commit message), so remove it. Also extract the `ProcessPong()` helper, as the only place that called `in_avail`.
ACKs for top commit:
l0rinc:
code review ACK fa204100e1
sedited:
ACK fa204100e1
Tree-SHA512: b5a069abb471c42d39e91a3c55fc7f18fb9ccf8591408b3dd43210d0287785369b6a5d6b59b847d85247a494f208dbbe4a7e88b24caf9b3e6c8fea2157cd0ee0
2a90b6132a Add release notes for exportasmap (Fabian Jahr)
8cb2d926b4 rpc: Add exportasmap RPC (Fabian Jahr)
Pull request description:
This depends on the embedded data PR itself (#28792), until merge this will remain in draft status. All commit but the last one are from there.
There has been interest in exporting the embedded data back into a file. This is implemented here with a simple `exportasmap` RPC which provides this functionality. The exported file can be used to verify the data integrity or statistical analysis using e.g. `contrib/asmap-tool.py`.
ACKs for top commit:
achow101:
ACK 2a90b6132a
sedited:
ACK 2a90b6132a
seduless:
Tested ACK 2a90b6132a
Tree-SHA512: c8453078efe916ed95bff0695aabd9123d805970e037fad5e9317f4d43e2a4b21970030265bc82b00f3a222ee3db11346eef1fb6fc9393429b9bc6449f1830f1
fa43da21f1 refactor: Run ShouldWarnOversizedDbCache calculation in u64 (MarcoFalke)
fa5801762e util: Return uint64_t from _MiB and _GiB operators (MarcoFalke)
fa9ddb01c9 test: Use MiB operator directly in cuckoocache_tests (MarcoFalke)
Pull request description:
Currently, the `_MiB` literal operator returns a value of type `size_t`. This is brittle and confusing:
* It is inherently impossible to represent larger values, like storage device usage.
* Similarly, it is not possible to even type an upper cap on the memory, see the failure in https://github.com/bitcoin/bitcoin/pull/34692#issuecomment-3973281952
* Using `size_t` isn't required here. The function is evaluated at compile time, and even 32-bit compilers can evaluate an `uint64_t` at compile time.
* Using `size_t` here encourages it to be used in more places, which will likely lead to more bugs and CVEs, such as https://github.com/bitcoin/bitcoin/pull/34109, https://github.com/bitcoin/bitcoin/pull/33724, etc.
Fix all issues, by:
* Marking the operator `consteval`, to ensure it is really only called at compile-time.
* Returning an `uint64_t` value.
* Using it in the place where it was previously not possible.
Review note:
This should have no downside, because the C++11 narrowing checks continue to work as expected. For example, typing `uint8_t{1_MiB};` will continue to fail with the correct error message `error: constant expression evaluates to 1048576 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]` (like it does on current master).
ACKs for top commit:
l0rinc:
ACK fa43da21f1
achow101:
ACK fa43da21f1
hebasto:
ACK fa43da21f1, I have reviewed the code and it looks OK.
hodlinator:
ACK fa43da21f1
Tree-SHA512: e19f6bd18c519c2a56d44fbc9e65b02e630bc71170f02a8a8955bf5c9bb2924b55ec2f810de2bbc961162aa668b7383c9fe9b5261fa1de7d9e8413003c32fa87
On some systems the old example of using xargs to pass list of profraw
files can exceed xargs's argument limit resulting in multiple executions
of llvm-profdata merge command which overwrites it's output file losing
coverage information. Switch to using a file with a list of filenames.
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
The alias of the size() method is confusing, because:
* It claims to be part of the Bitcoin Core stream subset (streams
interface), but this is not used by any other stream interface. Mostly
the `write(std::span)` and `read(std::span)` define the stream
interface.
* It casts the size_t to i32, but the only place that calls the function
casts that back to size_t.
* Providing this alias for size() without a proper reason is confusing.
Fix all issues by removing it and using the size() method.
RPCs: getrawmempool, getmempoolancestors, getmempooldescendants, getmempoolentry
This key has been marked deprecated since v29, it can be
removed from the RPC response now unless the user requests it via the
-deprecatedrpc=bip125 node argument.
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently. The getmempoolinfo RPC need not return
the deprecated `fullrbf` key in the response anymore unless the user requests
it via -deprecatedrpc=fullrbf node argument.
e6430b2773 bench: make `setup()` use single-iteration epochs (Lőrinc)
ba0078e3bf bench: fix ephemeral spend inputs (Lőrinc)
b8b7f896e8 bench: drop duplicate balance benchmark (Lőrinc)
Pull request description:
### Context
Found while reviewing https://github.com/bitcoin/bitcoin/pull/35018, which led to a few additional benchmark fixes.
### Problem
`MempoolCheckEphemeralSpends` populated every child input by writing to `vin[0]`.
That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts.
`WalletBalanceMine` duplicated `WalletBalanceClean` exactly.
The same review also showed that nanobench `setup()` can be misused as though it runs once per timed call.
It actually runs once per epoch, so using it with multiple epoch iterations can silently leave later iterations with different preconditions.
### Fix
Write each ephemeral-spend prevout to the matching child input and assert that the last input spends the last parent output.
Remove the duplicate wallet balance benchmark registration.
Add a nanobench assertion that `setup()` is only used with `epochIterations(1)`.
> [!NOTE]
> This PR included a few benchmark adjustments originally which made the tests unstable and were reverted.
ACKs for top commit:
davidgumberg:
crACK e6430b2773
sedited:
ACK e6430b2773
Tree-SHA512: 667c67975d7a3e21333f6c8a4e037ad700bc6dcbd7039e002534e4f2a9328195a618b78accf097e7c43402a1f1b25e32b0c39642fd465729ab4c337abe4f23f1
df44afdc98 kernel: expose btck_block_tree_entry_get_ancestor (Peter Zafonte)
Pull request description:
Callers building a block locator from a stale tip currently have to walk back one entry at a time using btck_block_tree_entry_get_previous. For large step sizes this means hundreds of C API calls per locator entry.
This exposes GetAncestor via btck_block_tree_entry_get_ancestor, which uses the existing CBlockIndex skiplist to reach any ancestor height in O(log N) and works on any chain, not just the active chain.
Includes a C++ convenience wrapper and tests ~~covering same-height, genesis, and out-of-range cases~~ in btck_block_tree_entry_tests.
See previous discussion here #34843
ACKs for top commit:
alexanderwiederin:
ACK df44afdc98
sedited:
ACK df44afdc98
Tree-SHA512: e953b634235129fa972e7f148016e8aee7bad61e16d50191d6724d11c71fe0f7623152ff61aa4c4ec5c8a5b500aaeb36613b5b2ff1e404d01ed7b65aa861f7aa
`setup()` in nanobench runs once per epoch, not once per timed call.
If an epoch executes the benchmark body multiple times, `setup()` can silently leave later iterations with different preconditions.
Make `setup()` force `epochIterations(1)` itself: keep rejecting incompatible larger explicit epoch sizes, but allow existing single-iteration callers such as `-sanity-check`.
With `setup()` handling this centrally, remove the redundant `epochIterations(1)` calls from the benchmarks that use it.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
`MempoolCheckEphemeralSpends` wrote every prevout to `tx2.vin[0]` instead of `tx2.vin[i]`.
That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts.
Write each prevout to `vin[i]` instead.
Add an assertion that the last child input spends the last parent output.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
3bf3b6d59a net: log SOCKS5 auth before sending (takeshikurosawaa)
b2debc9276 net: cleanup SOCKS5 auth logging (takeshikurosawaa)
Pull request description:
`Socks5()` logs the supplied `username` and `password` in `BCLog::PROXY`
when the server selects `USER_PASS`.
Keep the log entry for the auth path, but omit the credentials.
Log the message before sending the authentication data.
No functional behavior change intended.
ACKs for top commit:
davidgumberg:
crACK 3bf3b6d59a
theStack:
ACK 3bf3b6d59a
sedited:
ACK 3bf3b6d59a
Tree-SHA512: ef90fca297b954c8659d108259b7f25216c0c0ddb7305539154f14a7bfdad807b70b7ddd0cf6e4ad0127d0a1ccfdfbb3f4785d7662b134409375ff1830b9df9b
Previously, they were using a pattern of defininig a constant megabytes
symbol, and then multiplying that by 1_MiB.
It is easier to use the _MiB operator directly.
The move-assignment operator for btck::Handle<> unconditionally called
DestroyFunc(m_ptr) before reading the source pointer. On a self-move
(h = std::move(h)), this destroyed the held resource and then reassigned
the now-dangling pointer back to m_ptr via std::exchange, leading to a
double-free when the object is later destroyed.
Mirror the existing self-check in the copy-assignment operator by
guarding the move-assignment with 'if (this != &other)' so a self-move
becomes a no-op, leaving the object in a valid state as required by the
standard library.
Handle<> is the base of 16 public types in the kernel C++ API wrapper
(Transaction, Block, BlockHeader, ChainParams, Context, Coin,
BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint,
TransactionInput, PrecomputedTransactionData, BlockHash,
BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise
from generic algorithms operating on containers of these types.
Extend CheckHandle in test_kernel to cover self-move-assignment for
every Handle-derived type.
c5ec2d5313 logging: replace FormatLogStrInPlace with Format (stickies-v)
3b92ec2036 logging: replace BufferedLog with log::Entry (stickies-v)
8115001cd4 logging: pass log::Entry through to logging functions (stickies-v)
b414913c73 util: add timestamp and thread_name to log::Entry (stickies-v)
8a55b17751 util: make SourceLocation constructor explicit (stickies-v)
Pull request description:
Preparatory work to enable struct-based logging for kernel (#34374), but I think it's a mild improvement by itself too.
#34465 introduced `util::log::Entry`. This PR updates some internal functions and structs to make better use of that new plumbing.
Mostly a refactor, except for very minor timing differences for `timestamp` and `mocktime` which are now uniformly captured at `Entry` generation.
ACKs for top commit:
maflcko:
review ACK c5ec2d5313🚐
ryanofsky:
Code review ACK c5ec2d5313. Just suggested changes since last review: adding (void) std::move, making SourceLocation constructor explicit, dropping BufferedLog struct. Overall PR is a nice code simplification, and I don't think it has downsides other than buffered logs now containing [ignored](https://github.com/bitcoin/bitcoin/pull/34865#discussion_r3054379230) `should_ratelimit` fields, which can be fixed in a followup by separating log entries from log options.
sedited:
ACK c5ec2d5313
Tree-SHA512: 3943f380a426b2c1b405226f231db51548c737467a278936b36a4cc738e01a790258f0886817d7caa1dbba7874f2e7f9ad93c36a137fa35f721f2d988b9863aa
