e62423d6f1514b022155edb5bc930cecc4236731 doc: Improve dependencies.md documentation (Nicola Leonardo Susca)
a3520f9d561e499d0c05808cfc497d4fa113c798 doc: Add dependency self-compilation info (Nicola Leonardo Susca)
d1fdc84c54934931c3f49204a688e43458af94dd doc: Remove Linux Kernel from dep. table (Nicola Leonardo Susca)
Pull request description:
Small improvements to the `dependencies.md` documentation as a follow-up for #31634.
**Linux Kernel** does not need to be in the dependencies as it is not required for cross-compiling from other systems, and users building on Linux should not expect they can build using any EOL kernel, see: https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957123270
**Runtime dependencies** can be in a separate table to improve readability. See: https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550
**Version used** is redundant as the depends package definition is already linked in the table and can thus be removed, see: https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972
ACKs for top commit:
maflcko:
lgtm ACK e62423d6f1514b022155edb5bc930cecc4236731 🛄
hebasto:
ACK e62423d6f1514b022155edb5bc930cecc4236731.
jonatack:
ACK e62423d6f1514b022155edb5bc930cecc4236731
Tree-SHA512: 586c450aec7ece5d543bcb12796a2bb7ff459e15c8813a7b5104a38d09fc51e7e902363ff023be48273ae2b1a1b0807a439c8523b4ea2e398b76b7c9a48d0dfb
fa981b90f53101bff2eda606d9479233e71736b5 ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32493
For some reason terminate or kill do not work inside the CI system under valgrind.
So disable the test for now, until a solution is found.
ACKs for top commit:
fanquake:
ACK fa981b90f53101bff2eda606d9479233e71736b5
mzumsande:
utACK fa981b90f53101bff2eda606d9479233e71736b5
Tree-SHA512: ce591fa7ffffbf757e2c15744e36a9e57300edf743400938e49fd02291f3977c551a3af1635bc7a6ccc1900d5ea150a64ee2ace46c1d765019ab11bd51035139
- Remove the "Version used" column from the dependencies tables as the
depends package definition which defines the version used is already
linked. In case a developer is interested in which PR introduced this
file/version they can use `git blame` on the package definition as
usual. This removes doc. maintenance overhead and eliminates the risk
of stale information about the "Version used", see comment:
https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972
- Separate dependency tables into build-time and run-time tables for
easier distinction of the two and to avoid repeating the same
information ("No"/"Yes") for better readability.
- Order dependencies alphabetically
The `dependencies.md` should mention that it is possible to self-compile
the dependencies and reference `depends/README.md` for instructions.
Also mention full path to `/doc/build-*.md` for clarity.
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.
This PR adds a concise top-level description, improving documentation consistency and alerting users to the potential impacts of using the command.
It is only used in test. There it is problematic, because it sometimes
relies on m_default_address_type. If the default were changed to
BECH32M, those tests would fail the assert(false).
So just use PKHash{} in all tests and remove GetDestinationForKey.
Windows application manifests provide several benefits. However, on the
master branch, the linker generates and embeds manifests only when
building with MSVC.
This change unifies manifest embedding for both native and
cross-compilation.
We would only modify the parent process' first --tmpdir arg.
Now we tack on an additional --tmpdir after the parent's arguments. Also simplifies the code.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
75a185ea3db3177e8e479ee61a39bcb51e08d9a6 test: add skip_if_running_under_valgrind() (fanquake)
Pull request description:
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
ACKs for top commit:
maflcko:
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
willcl-ark:
ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
Tree-SHA512: 7f45c3049ab39cc514024067bd6ac26598e99202c114b48459834c26c2e1273fa58af693878298e628a10c561b954850e49e76b39567b771bb0c0534a063a524
3b824169c7766460794bb445028ac55a7111ee3e doc: remove Carls substitute server from Guix docs (fanquake)
Pull request description:
This no-longer exists. Use one of the other Guix servers in the example.
ACKs for top commit:
achow101:
ACK 3b824169c7766460794bb445028ac55a7111ee3e
hebasto:
ACK 3b824169c7766460794bb445028ac55a7111ee3e, the listed substitute servers are the same as in https://guix.gnu.org/manual/en/html_node/Official-Substitute-Servers.html.
Tree-SHA512: dc3a362ccaa9ce8039d3c02158de9cd71082eb4dd790368bfb11c2942a5aae57e67779b5ff3108b532c4fb765811bd9e145eedb390fc48b52b43d334d5864865
a0eed55398f882d9390e50582b10272d18f2b836 run_command: Enable close_fds option to avoid lingering fds (Luke Dashjr)
c7c356a448657e154e6bad6c39a296cfd6dce30c cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux (Luke Dashjr)
4f5e04da135080291853f71e6f81dd0302224c3a Revert "remove unneeded close_fds option from cpp-subprocess" (Luke Dashjr)
Pull request description:
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).
> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and starting bitcoind again will fail). It's also a potential security issue if a child process is intended to be sandboxed at some point. Not to mention plain ugly :)
>
> cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in https://github.com/bitcoin/bitcoin/pull/29961 rather than fixing this during the migration, but this PR restores it, enables it for RunCommandParseJSON, and optimises it by iterating over /proc/self/fd/ like most other libraries do these days ([eg, glib]> (487b1fd20c/glib/gspawn.c (L1094))) since iterating all possible fd numbers [has been found to be problematic](https://bugzilla.redhat.com/show_bug.cgi?id=1537564).
>
> (Equivalent to https://github.com/bitcoin/bitcoin/pull/22417 was for boost::process)
ACKs for top commit:
achow101:
ACK a0eed55398f882d9390e50582b10272d18f2b836
hebasto:
ACK a0eed55398f882d9390e50582b10272d18f2b836, tested on Ubuntu 25.04:
vasild:
ACK a0eed55398f882d9390e50582b10272d18f2b836
Tree-SHA512: 7dc1cb6cc1f45ff7c4f53512e400baad1a033b4ebf14ba6f6ffa38588314932d6d01ef67b197f081e8202bb802659ac6a87998277797721d6d7b20efde8e9a6b
5bf91ba8800d23402536d758f02198eac0fd7d61 wallet: Drop unused fFromMe from CWalletTx (David Gumberg)
Pull request description:
This has been unused since commit fe52346, this is a re-opening of #9351.
ACKs for top commit:
maflcko:
lgtm ACK 5bf91ba8800d23402536d758f02198eac0fd7d61
achow101:
ACK 5bf91ba8800d23402536d758f02198eac0fd7d61
Tree-SHA512: b9a84f27b6cfe7796dcf629be6a8e01a97d931ea81ef088951d54d6691ffe79d22138baacc632375093cf3176a22c265e30a80f1f63c3bc620d08bf16f6a488f
faf9082a5f689e2e51a474bf654e4e9b6ca29685 test: Fix whitespace in prevector_tests.cpp (MarcoFalke)
fa7f04c8a7b7cbb4a1728bf2c9c6c7c8408b432a refactor: Remove UB in prevector reverse iterators (MarcoFalke)
Pull request description:
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils such as `std::reverse_iterator{it}` or `std::views::reverse` can be used out of the box.
So remove them, along with the ubsan suppressions, that are no longer used.
I've tagged this a refactor, because the code was always dead (unused outside of tests). And since commit 2925bd537cbd8c70594e23f6c4298b7101f7f73d it was completely dead. Also, I could not find a sanitizer that detects this type of UB.
ACKs for top commit:
l0rinc:
tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
achow101:
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
stickies-v:
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685, nice find.
theuni:
utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
Tree-SHA512: 31511d520a1c0fdd65c2e5f1a8ef6fd17464303b6bff88a5d9d9577adfee849d431deb510882b6f4e15e8fb7168861bc0d26fca3bed4278f57a9d6e7b1235dce
Since the sighash type field is written for atypical sighash types, we
can look at that field to figure out whether the psbt contains
unnecessary transactions.
Instead of having the caller have to figure out the correct sane default
to provide to FillPSBT, have FillPSBT do that by having it take the
sighash type as an optional. This further allows it to distinguish
between an explicit sighash type being provided and expecting the
default value to be used.
SignPSBTInput will need to report the specific things that caused an
error to callers, so change it to return a PSBTError. Additionally some
callers will now check the return value and report an error to the user.
Currently, this should not change any behavior as the things that
SignPBSTInput will error on are all first checked by its callers.
4b241867567203b204823a4558c2aa5767acf028 test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
8ba245cb8318c9be375ac2fb2a3e7ded875b4408 test: add constants for MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.
ACKs for top commit:
achow101:
ACK 4b241867567203b204823a4558c2aa5767acf028
rkrux:
re-ACK 4b24186
Tree-SHA512: f12919f71b3fff74df1d7ddaa8db455b1b139f7abd51d7f3fa5d750fc7dd613454b438c4e0dedad679476d414fa1da43ef1121e486b0bdfd97d5ef8bdf37f060
62fc42d475df4f23bd93313f95ee7b7eb0d4683f interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq)
c39ca9d4f7bc9ca155692ac949be2e61c0598a97 interfaces: move getTip implementation to miner (Sjors Provoost)
720f201e652885b9e0aec8e62a1bf9590052b320 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq)
e6c2f4ce7a841153510971f0236c527d1a499649 interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq)
02d4bc776bbe002ee624ec2c09d7c3f981be1b17 interfaces: remove redundant coinbase fee check in `waitNext` (ismaelsadeeq)
Pull request description:
#### Motivation
In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](https://github.com/bitcoin/bitcoin/blob/master/src/wallet) and just exposed in [src/interfaces/](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces) instead of being implemented there, so it can be more modular and accessible to unit tests.
However the some methods in the newly added `BlockTemplateImpl` and `MinerImpl` classes partially enforces this guideline, as the implementations of the `submitSolution`, `waitNext`, and `waitTipChanged` methods reside within the class itself.
#### What the PR Does
This PR introduces a simple refactor by moving certain method implementations from `BlockTemplateImpl` into the miner module. It introduces three new functions:
1. Remove rundundant coinbase fee check in `waitNext`
2. **`AddMerkleRootAndCoinbase`**: Computes the block's Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by `submitSolution` before the block is submitted for processing.
3. **`WaitAndCreateNewBlock`**: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns `nullptr`. The `waitNext` method in `BlockTemplateImpl` now simply wraps this function.
4. Move `GetTip` implementation to miner.
5. **`WaitTipChanged`**: Returns the tip when the chain it changes, or `nullopt` if a timeout or interrupt occurs. The `waitTipChanged` method in `MinerImpl` now calls `GetTip` after invoking `ChainTipChanged`, and returns the tip.
#### Behavior Change
- We now only `Assert` for a valid chainman and notifications pointer once.
ACKs for top commit:
achow101:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
Sjors:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
ryanofsky:
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
Tree-SHA512: 502632f94ced81f576b2c43cf015f1527e2c259e6ca253f670f5a6889171e2246372b4e709575701afa3f01d488d6633557fef54f48fe83bbaf1836ac5326c4f
Since CWallet::chainStateFlushed is now no-op, this test no longer tests
the concurrent writes scenario. There are no other cases where multiple
DatabaseBatches are open at the same time.
StopWallets, which was being called prior to UnloadWallets, performs an
unnecessary database closing step. This causes issues in UnloadWallets
which does additional database cleanups. Since the database closing step
is unnecessary, StopWallets is removed, and UnloadWallets is now called
from WalletLoaderImpl::stop.
chainStateFlushed is no longer needed since the best block is updated
after a block is scanned. Since the chainstate being flushed does not
necessarily coincide with the wallet having processed said block, it
does not entirely make sense for the wallet to be recording that block
as its best block, and this can cause race conditions where some blocks
are not processed. Thus, remove this notification.
Migrating a wallet requires having a bestblock record. This is always
the case in normal operation as such a record is always written on
wallet loading if it didn't already exist. However, within the unit
tests and benchmarks, this is not guaranteed. Since migration requires
the record, WalletMigration needs to also add this record before the
benchmark.
The only reason to call chainStateFlushed during wallet loading is to
ensure that the best block is written. Do these writes explicitly to
prepare for removing chainStateFlushed, while also ensuring that the
wallet's in memory state tracking is written to disk.
Additionally, after rescanning on wallet loading, instead of writing
the locator for the current chain tip, write the locator for the last
block that the rescan had scanned. This ensures that the stored best
block record matches the wallet's current state.
Any blocks dis/connected during the rescan are processed after the
rescan and the last block processed will be updated accordingly.
When a block is connected, if the new block had anything relevant to the
wallet, update the best block record on disk. If not, also sync the best
block record to disk every 144 blocks.
Also reuse the new WriteBestBlock method in BackupWallet.
a04f17a1882407db09b0a07338e12877ac1d9e92 doc: warn that CheckBlock() underestimates sigops (Sjors Provoost)
Pull request description:
Counting sigops in the witness requires context that `CheckBlock()` does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
The commit message contains some historical context.
ACKs for top commit:
ismaelsadeeq:
ACK a04f17a1882407db09b0a07338e12877ac1d9e92
ryanofsky:
Code review ACK a04f17a1882407db09b0a07338e12877ac1d9e92
Tree-SHA512: 26528367a7f3cfa8540ef0b90f7aa912c8f0bc057428f20a1fd1d4e232dac77747bc20044f0fcb0ffab8a2e1fb3dbe3dab46be749553a917744ddc7a829025cb
Made every signed/unsigned conversion in the serialization helpers explicit so the UBSan `implicit-sign-change` check passes and the `serialize.h` suppression can be dropped.
For consistency, a few other simple changes were also applied to the serialization helpers:
* remove redundant `inline` on function templates;
* unify formatting to make the differences between similar methods obvious.
